Re: [virtio-comment] Re: [PATCH v10 4/4] transport-pci: Introduce group legacy group member config region access

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

On Thu, Jul 06, 2023 at 07:07:42PM +0000, Parav Pandit wrote:
> > From: virtio-comment@lists.oasis-open.org <virtio-comment@lists.oasis-
> > open.org> On Behalf Of Michael S. Tsirkin
> > Sent: Thursday, July 6, 2023 3:01 PM
> 
> > > +\begin{lstlisting}
> > > +struct virtio_pci_legacy_notify_region {
> > > +        u8 owner;  /* When set to 1, notification region is of the
> > > +owner device */
> > 
> > I propose we rename this to flags:
> > 	0x0 - end of list (driver should ignore following values until end of list)
> > 	0x1 - owner
> > 	0x2 - member
> > other values - reserved
> >
> 
> > and prescribe that driver ignores any reserved values.
> > 
> > basically we are following how the virtio capabilities work.
> >
> Looks good. Will change it.
> 
> > > +The group owner device hardwires VF BAR0 to zero in the SR-IOV
> > > +Extended capability.
> > > +
> > > +The group member device does not use PCI BAR0 in all the Virtio PCI
> > > +capabilities listed in section \ref{sec:Virtio Transport Options / Virtio Over
> > PCI Bus / Virtio Structure PCI Capabilities}.
> > 
> > Just drop this last one. How can they use it if there's no VF BAR0?
> >
> Hardwaring BAR0 is in BAR area of the VF.
> But virtio pci capabilities without this can still report bar 0 and virtio capabilities.

How do they do this?

> Ofcourse, it is a broken device.
> But skipping this line imply that "because VF BAR0 is hardwired", this metadata in pci capability must not expose it.
> 
> Why not write it extra thing instead of implying it?
> We already wrote few duplicate things to make reader life easier.

If you really want to go ahead, but prefix it with "In consequence" and
do not start a new paragraph, so reader knows it's not an extra
requirement.

So I started writing it using correct grammar and spec terminology:

	In consequence, non of the group member devices has BAR0, and
	in particular none of the virtio structure capabilities
	of a member device has \field{bar} with the value of 0.


However, this actually is kind of wrong (and so is your text).  Not all
caps have a RO bar value.  virtio_pci_cfg_cap has bar that is RW by
driver.  So if we are going this route we also need to explain that it's
true for all caps except virtio_pci_cfg_cap.  And for virtio_pci_cfg_cap
driver is not allowed to write 0 there.

Frankly too much trouble but if you want to, keep trying.


-- 
MST