RE: [PATCH v6 4/4] transport-pci: Introduce group legacy group member config region access

[Parav Pandit <parav@nvidia.com>]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Monday, June 19, 2023 12:17 PM

> I'd love to just put this stuff in a separate tex document and just link there from
> the generic thing.
> Or if it's too hard, please create new sections for this stuff. But in this case each
> of these sections has to start with repeating the description when things apply.
> E.g. "when using legacy interface"
> when using commands "A B C to access" etc etc.
> 
> I wrote a bunch of comments below.
>
I will stick to them as most things are taken out to generic section anyway in patch 3.
Only PCI transport part stays here.
I will move some more text to generic section.
 
> > +As described in PCIe base specification \hyperref[intro:PCIe]{[PCIe]}
> > +PCI VFs do not support the I/O space BAR. Due to this limitation, a
> > +PCI VF hardware device that supports legacy interfaces cannot be
> > +attached as passthrough device to the guest virtual machine.
> 
> That's not the only reason. Let's keep things generic and start by saying
> something like "Some configurations with a need to support legacy drivers can
> not implement a transitional device. For example .... ".
>
I will avoid For example part and move this to generic section.
 
> > +
> > +To have a transitional or legacy virtio device in the guest virtual
> > +machine, a PCI PF device as a group owner may
> 
> may -> can outside conformance statements.
> 
Ack.

> > support accessing its group member
> 
> Pls keep it generic. A group owner ....
>
Ack
 
> 
> > +device's legacy configuration region using the group administration
> > +commands listed at \ref{sec:Basic Facilities of a Virtio Device /
> > +Device groups / Group administration commands / Legacy Interfaces: legacy
> configuration access commands}.
> > +These legacy configuration access commands are transported via the
> > +administration virtqueue.
> 
> are these called legacy configuration access commands then?
> why do you introduce this term? it does not look like it's used anywhere else.
> 
It is used and referred PCI section " Legacy Interfaces: Group member device Configuration Region Access".
> 
> I would prefer it if you just use the term "legacy interface"
> it is well defined and widely used in the spec.
> 
It is "legacy interface", with suffix for group member config access.
In future, there may be a section for other things than config access.

> again leave virtqueue out of it pls we'll have other ways down the road. what
> matters is that they are transferred to a group member by sending legacy
> configuration access commands to the group owner.
>
Ack.
It is theory of operation, not a normative.
Tomorrow if other than aq exists, one will write as "AQ or xyz"..

But fine to me to reduce changes from 216 to less lines. :)
 
> 
> > This mechanism enables minimal involvement of the
> > +hypervisor software only for the purpose of the legacy configuration region
> access.
> 
> Leave marketing out of it pls, kill last sentence.
>
Huh, I don't see theory of operation and scope of the hypervisor as marketing at all.
I will remove as it makes this patch shorter.
 
> 
> 
> > +
> > +When a guest virtual machine requests legacy configuration and device
> > +specific regions
> 
> wait so now it's legacy configuration and device specific regions and these two
> are distinct?
> 
> > of group member device, a hypervisor software accesses
> > +it using an administration virtqueue
> 
> please talk of administration commands. virtqueue does not matter at all.
> 
Ack.

> > on behalf of the guest virtual machine.
> 
> 
> 
> To put it in our terms, something like this:
> 	when a legacy driver accesses a member device of
> 	a group using the
> 	legacy interface, a modern driver can intercept
> 	the access and forward it to the owner device.
> 
I will not mention "modern driver" as it has zero reference in spec and don't want to bring Linux terms here.
"the driver can intercept" is enough.

> 
> 
> > +
> > +Even though virtqueue driver notifications can be communicated
> > +through administration virtqueue, if the group member device support
> > +such
> 
> supports?
>
Ack.
 
> > +notifications using a memory-mapped operation, such driver
> > +notifications are sent using a group member device defined notification
> region.
> 
> what does "are sent" imply here? is there a matching conformance statement?
> who sends them?
> 
It has the same meaning as "Driver notifications" section first line has.
Isnt it implicit that driver notifications are sent by driver based on "Driver notifications" section?

> > +
> > +The group owner device
> 
> this is the 1st you mention any group or its owner.
> 
> > should not expose PCI BAR0
> 
> Not PCI BAR0. That is in header. You mean VF BAR0.
> 
I will rewrite it as,

The group owner device should not expose PCI BAR0 in the PCI SR-IOV extended capability for the group member PCI VF devices when it prefers to
support legacy interface for legacy configuration access using this group owner.

> > for the group member
> > +devices when it prefers to support legacy interface for legacy
> > +configuration access using its group owner.
> 
> don't use should outside conformance
> 
What should be used instead of "should"?

> 
> I think this specific case actually should be more specific.
> Something like:
> 
> - For PCI SRIOV groups, when group owner device implements commands
>   A,B,C the group owner's VF BAR 0 is hardwired to 0
>   in its PCI SRIOV capability.
>
I wrote it slightly differently above.
 
> 
> > +This facilitates hypervisor software to operate with least amount of
> > +complexities
> 
> complexity is its own plural
> 
> > to emulate the legacy interface I/O space BAR and passthrough
> > +other PCI BARs and PCI device capabilities to the guest virtual
> > +machine without any translation.
> > +
> > +The group member device should not expose PCI BAR0 in various PCI
> capabilities.
> > +
> > +\devicenormative{\subsubsection}{Legacy Interfaces Requirements:
> > +Group Member Device Legacy Configuration Access}{Virtio Transport
> > +Options / Virtio Over PCI Bus / Legacy Interfaces Requirements: Group
> > +Member Device Legacy Configuration Access}
> > +
> > +When a PCI SR-IOV group owner device supports
> > +VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ,
> > +VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE,
> > +VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ,
> > +VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE commands, the group
> owner
> > +device SHOULD NOT expose PCI BAR0 in the SR-IOV Extended capability.
> > +This is to facilitate the software to emulate I/O region BAR0 for supporting
> the legacy interface.
> 
> not PCI BAR0 - VF BAR0. Check the PCI spec you can not "not expose it". if you
> want to register can be "unimplemented".
> Base Address registers are hardwired to zero
> 
> But it is better to be specific on what should happen. hardwire VF BAR0 to 0,
> right?
> 
Hardwire to zero and not expose is same thing to me.

> 
> > +
> > +\drivernormative{\subsubsection}{Legacy Interfaces Requirements:
> > +Group Member Device Legacy Configuration Access}{Virtio Transport
> > +Options / Virtio Over PCI Bus / Legacy Interfaces Requirements: Group
> > +Member Device Legacy Configuration Access}
> > +
> > +The driver SHOULD NOT emulate I/O region BAR0 if a device group
> > +member exposes a PCI BAR0.
> 
> what does "emulate" mean here? which driver it is?
>
I will add the line to theory of operation, but I recollect "emulate" line came from you.
 
> > diff --git a/transport-pci.tex b/transport-pci.tex index
> > a5c6719..3647485 100644
> > --- a/transport-pci.tex
> > +++ b/transport-pci.tex
> > @@ -541,6 +541,8 @@ \subsubsection{Notification structure
> > layout}\label{sec:Virtio Transport Options  struct virtio_pci_notify_cap {
> >          struct virtio_pci_cap cap;
> >          le32 notify_off_multiplier; /* Multiplier for
> > queue_notify_off. */
> > +        u8 legacy_q_notify_supported;
> 
> I am still mulling whether VIRTIO_PCI_CAP_NOTIFY_CFG would be better.
> 
> 
> > +	u8 reserved[3];
> 
> 
> align with spaces pls.
> 
Ack.

> 
> >  };
> >  \end{lstlisting}
> >
> > @@ -560,6 +562,14 @@ \subsubsection{Notification structure
> > layout}\label{sec:Virtio Transport Options  the same Queue Notify address for
> all queues.
> >  \end{note}
> >
> > +\field{legacy_q_notify_supported} when set to 1,
> 
> 
> Do you mean something like:
> When \field{legacy_q_notify_supported} this indicates that ...
>
Yes will fix.
 
> > indicates that the device
> > +supports legacy queue notifications at this notification location.
> 
> And what location? this refers to what? the location described by this capability
> maybe?
> 
Ack.

> 
> More specifically, assume a transitional device (with an IO BAR as normal). Does
> presence of this flag in the capability imply we can use a legacy queue but use
> this notification with this capability?
I don't know how this combination can be even possible by the device other than a buggy software who will do both.

> I worry that if we allow that, it opens floodgates of people who are too lazy to
> upgrade to 1.0 but just hack this notification in there.
>
Don't understand the concern.
If a device has transitional device, it is 1.0 device supporting legacy with IOBAR and newer interface.
So...
 
> 
> 
> 
> 
> > Legacy Queue
> > +Notification address is derived within a BAR for a virtqueue:
> > +
> > +\begin{lstlisting}
> > +        cap.offset
> > +\end{lstlisting}
> > +
> >  \devicenormative{\paragraph}{Notification capability}{Virtio
> > Transport Options / Virtio Over PCI Bus / PCI Device Layout / Notification
> capability}  The device MUST present at least one notification capability.
> >
> 
> I don't get what this is saying.  I *think* you really mean is that
> queue_notify_off is 0.
> 
Please refer to the current notification address definition.
Above is written in similar way where it doesnt need to describe queue_notify_off.
> 
> > @@ -596,6 +606,14 @@ \subsubsection{Notification structure
> > layout}\label{sec:Virtio Transport Options  cap.length >=
> > queue_notify_off * notify_off_multiplier + 4  \end{lstlisting}
> >
> > +\paragraph{Legacy Interfaces: Notification
> > +capability}\label{par:Virtio Transport Options / Virtio Over PCI Bus
> > +/ PCI Device Layout / Legacy Interfaces: Notification capability}
> > +
> > +The device SHOULD set \field{legacy_q_notify_supported}
> 
> why SHOULD and not MUST here - if driver checks for it then it won't work
> without no?
> 
If device doesn't want to support queue notifications, it has ability to set to 0.

> > when the device
> > +notification location supports legacy driver notifications.
> 
> legacy driver notifications is ambigous.
> what you want here and elsewhere is something like "when used through a
> legacy interface, device supports driver
>
Ack.