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

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Jun 28, 2022 at 12:52:51AM +0300, Max Gurtovoy wrote:
> 
> 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

Vdev_id -> member identifier ?

> 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.


maybe add here "devices of this type use the Virtio PCI transport
(link)"

I am not sure. Maybe we want a section about SR-IOV generally,
with mentions of the feature bit and the group type.
For now I think what you have is enough.

> > 
> > > > 
> > > > 
> > > > > 
> > > > > 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
> > > > > 
> 
> 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://lists.oasis-open.org/archives/virtio-comment/
> Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> Committee: https://www.oasis-open.org/committees/virtio/
> Join OASIS: https://www.oasis-open.org/join/