Re: [virtio-comment] Re: [PATCH v5 1/7] Introduce device group

[Max Gurtovoy <mgurtovoy@nvidia.com>]

On 6/2/2022 9:59 AM, Michael S. Tsirkin wrote:
> On Thu, Jun 02, 2022 at 10:21:23AM +0800, Jason Wang wrote:
> > On Wed, Jun 1, 2022 at 9:44 PM Max Gurtovoy <mgurtovoy@nvidia.com> wrote:
> > > On 5/18/2022 4:32 PM, Cornelia Huck wrote:
> > > > On Wed, May 18 2022, Max Gurtovoy <mgurtovoy@nvidia.com> wrote:
> > > >
> > > > > Hi MST,
> > > > >
> > > > > On 5/15/2022 6:25 PM, Michael S. Tsirkin wrote:
> > > > > > On Wed, Apr 27, 2022 at 01:58:18AM +0300, Max Gurtovoy wrote:
> > > > > > > +\subsection{Device group}\label{sec:Introduction / Terminology / Device group}
> > > > > > > +
> > > > > > > +A device group includes one or more virtio devices.
> > > > > > > +Each virtio device has a unique virtio device id (vdev_id) within a device group. A valid vdev_id is a 64-bit field in the range of
> > > > > > > +0x0 - 0xFFFFFFFFFFFFFFF0. Vdev_id 0xFFFFFFFFFFFFFFFF is a value that refers to all devices in a device group and isn't a valid vdev_id.
> > > > > > > +
> > > > > > > +For now, the supported device groups are:
> > > > > > > +\begin{enumerate}
> > > > > > > +\item Type 1 - A virtio PCI SR-IOV physical function (PF) and its PCI SR-IOV virtual functions (VFs). For this group type, the PF device has vdev_id that is equal to 0
> > > > > > > +and the VF devices have vdev_id's that are equal to their vf_number (according to the PCI SR-IOV specification).
> > > > > > > +\end{enumerate}
> > > > > > > +
> > > > > > >     \section{Structure Specifications}\label{sec:Structure Specifications}
> > > > > > In context of virtualization type 1 already refers to a specific type
> > > > > > of hypervisor.
> > > > > >
> > > > > > I suggest simply "SR-IOV type" - this way users do not need to remember
> > > > > > special terminology.
> > > > > This is 12 lines addition commit with simple definition.
> > > > >
> > > > > I didn't mentioned hypervisors here.
> > > > >
> > > > > I will stick to your suggestion and use name instead of numbers
> > > > > (although I don't understand how can a use that knows how to read spec
> > > > > will be confused here), but I would like Jason and Cornelia to ack on
> > > > > this during this review cycle.
> > > > >
> > > > > When we'll get 3 acks on this name - I'll update it for v6.
> > > > So, do you want to imply some kind of numbering? I don't like "Type 1",
> > > > either. If the type needs to be referenced in code, it should have a
> > > > #define or such; otherwise, "SR-IOV type" would be fine.
> > > ok I'll change it to be:
> > >
> > > diff --git a/introduction.tex b/introduction.tex
> > > index aa9ec1b..bba70a6 100644
> > > --- a/introduction.tex
> > > +++ b/introduction.tex
> > > @@ -156,6 +156,18 @@ \subsection{Transition from earlier specification
> > > drafts}\label{sec:Transition f
> > >    sections tagged "Legacy Interface" in the section title.
> > >    These highlight the changes made since the earlier drafts.
> > >
> > > +\subsection{Device group}\label{sec:Introduction / Terminology / Device
> > > group}
> > > +
> > > +A device group includes one or more virtio devices.
> > > +Each virtio device has a unique virtio device id (vdev_id) within a
> > > device group. A valid vdev_id is a 64-bit field in the range of
> > > +0x0 - 0xFFFFFFFFFFFFFFF0. Vdev_id 0xFFFFFFFFFFFFFFFF is a value that
> > > refers to all devices in a device group and isn't a valid vdev_id.
> BTW I don't really like inventing terms with underscores.
> Let's stick to english if we can. By the way "id" is not a word, either
> ;) We have a couple of instances which we really should fix.
> And "virtio device id" is confusingly similar to
> virtio "device id" where device id is a term from pci.
> And abbreviations really should be capitalized or end with a comma
> but really best just avoided.
>
> How about group member identifier? So
>
> Each virtio device within a group has a unique member identifier.
>
>
>
> > > +
> > > +For now, the supported device groups are:
> > > +\begin{enumerate}
> > > +\item SR-IOV type - A virtio PCI SR-IOV physical function (PF) and its
> > > PCI SR-IOV virtual functions (VFs). For this group type, the PF device
> > > has vdev_id that is equal to 0
> > > +and the VF devices have vdev_id's that are equal to their vf_number
> > > (according to the PCI SR-IOV specification).
> A bit better just from grammar perspective:
>
>   \item SR-IOV type - the group includes a virtio PCI SR-IOV physical function (PF) and
>   all its virtual functions (VFs). For this group type, the PF device
>   has vdev_id of 0;
>   each VF has a vdev_id matching it's VF number [link to SRIOV spec].
>
> but note ideas on terminology above.

update to:

"

+\subsection{Device group}\label{sec:Introduction / Terminology / Device 
group}
+
+A device group includes one or more virtio devices.
+Each virtio device has a unique group member identifier 
(group_member_id) within a device group. A valid group member identifier
+is a 64-bit field in the range of 0x0 - 0xFFFFFFFFFFFFFFF0. Vdev_id 
0xFFFFFFFFFFFFFFFF is a value that refers to all devices in a
+device group and isn't a valid group member identifier.
+
+For now, the supported device groups are:
+\begin{enumerate}
+\item SR-IOV type - this group includes a virtio PCI SR-IOV physical 
function (PF) and all its virtual functions (VFs).
+For this group type, the PF device has group member identifier of 0. 
Each VF has a group member identifier matching it's VF number
+(according to PCI Express Base Specification, Single Root I/O 
Virtualization and Sharing chapter).
+\end{enumerate}

"

It's 90% suggested by MST.

> > > +\end{enumerate}
> > > +
> > >    \section{Structure Specifications}\label{sec:Structure Specifications}
> > >
> > > MST/Jason/Cornelia,
> > >
> > > can you add some Reviewed-By signatures if the above is agreed ?
> > If I understand this correctly, the idea of "device group" is to allow
> > different groups to be managed by a single admin virtqueue?
> >
> > And I feel that mixing transport specific definitions in the general
> > admin virtqueue might not be optimal. So I wonder whether it's better
> > to just say this is a transport specific type. And define it in the
> > PCI transport part.
> >
> > Thanks
> Well it's a single paragraph here, I think it's ok for now for completeness
> sake rather than have reader chase references just to figure out an
> example.
>
> But I do agree this sentence about SRIOV type has to be repeated
> in the pci transport section for completeness of that one.
Please suggest exactly what to add and where to add it.

And it will be done in V6.

> > > > This publicly archived list offers a means to provide input to the
> > > > OASIS Virtual I/O Device (VIRTIO) TC.
> > > >
> > > > In order to verify user consent to the Feedback License terms and
> > > > to minimize spam in the list archive, subscription is required
> > > > before posting.
> > > >
> > > > Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> > > > Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> > > > List help: virtio-comment-help@lists.oasis-open.org
> > > > List archive: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.oasis-open.org%2Farchives%2Fvirtio-comment%2F&amp;data=05%7C01%7Cmgurtovoy%40nvidia.com%7Ce4f8c9bcf9bd4d714ddc08da446583c4%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637897500193215232%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=wfat54XCPxc1pbJ3Os%2B3S2HLs%2FF2HjV4sYtjlllkX14%3D&amp;reserved=0
> > > > Feedback License: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fwho%2Fipr%2Ffeedback_license.pdf&amp;data=05%7C01%7Cmgurtovoy%40nvidia.com%7Ce4f8c9bcf9bd4d714ddc08da446583c4%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637897500193215232%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=aa7ZUanV6np2NuROfW%2BnugNCnEFB%2FWr%2B1FSCv5LwAE0%3D&amp;reserved=0
> > > > List Guidelines: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fpolicies-guidelines%2Fmailing-lists&amp;data=05%7C01%7Cmgurtovoy%40nvidia.com%7Ce4f8c9bcf9bd4d714ddc08da446583c4%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637897500193215232%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=yPuKPUevurVlY0%2B%2Bg31pjVKDk5dIIJBbmdyMQUB5GFc%3D&amp;reserved=0
> > > > Committee: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fcommittees%2Fvirtio%2F&amp;data=05%7C01%7Cmgurtovoy%40nvidia.com%7Ce4f8c9bcf9bd4d714ddc08da446583c4%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637897500193215232%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=9uLpj4f0klAdNethEgLAdziquwZ%2FU%2FG7qPEvCzEDlH0%3D&amp;reserved=0
> > > > Join OASIS: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fjoin%2F&amp;data=05%7C01%7Cmgurtovoy%40nvidia.com%7Ce4f8c9bcf9bd4d714ddc08da446583c4%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637897500193215232%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=ja9uSVhwbuwxER1nJTu7qFB5v%2FK6XA6akrDCLJzOj%2BQ%3D&amp;reserved=0