RE: [virtio-comment] 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: Wednesday, June 21, 2023 4:06 PM

[..]
> > > 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.
> 
> Good point but since you say "a legacy driver" then "the driver" seems to refer
> to exactly that. How about:
> 
>  	when a legacy member device driver accesses a member device of
>  	a group using the
>  	legacy interface, an owner device driver can intercept
>  	the access and forward it to the owner device.
> 
Above is not correct.

We have 3 drivers in picture.
1. guest driver
2. hypervisor level member device driver
3. group owner device driver

Trying to write without introducing guest and hypervisor term,

A group owner device driver provides the service to access member device's configuration region. 
A member device driver avail this service when it wants to access the member device's configuration region.


> > 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.
>
> 
> This seems to ignore all my comments.
>
I replied after that, probably missed in exchanges.

How about:
The group owner device MUST hardwire PCI BAR0 in the PCI SR-IOV extended capability for the group member PCI VF devices when it supports legacy configuration access commands.
 
> 
> 
> > > > 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"?
> 
> Depends on context, generally we just say what it does. E.g. "VF BAR0 is
> hardwired to zero".
> 
Ok.

> 
> 
> > >
> > > 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.
> 
> I don't see why what you wrote is any better than what you had to be frank. You
> are coming up with our own terminology instead of just using what's in the SR
> IOV spec. Please don't.
> 
> > >
> > > > +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.
> 
> Maybe, though previously you said it just means not put any capabilities there.
> More importantly I don't see expose or not expose anywhere in the PCI or
> SRIOV spec.  When spec wants to say how not to have a given BAR it says exactly
> hardwired to zero.
> 
Hardwired to zero is fine, done above now.

> > >
> > > > +
> > > > +\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.
> 
> I generally am fine with using this term but we need to then define it before
> use.
> 
> This specific thing I would just drop.
> 
> Instead of wasting time just say device MUST hardwire VF BAR0 to zero as
> opposed to SHOULD, and we do not need to worry about how drivers recover. If
> they want to they will validate, if they don't then they won't.
>
This limitation seems fine to me.
Will drop.
 
> 
> > > > 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...
> 
> 
> if we are switching to admin commands for this, then let's not argue about it.
> 
Ok.