Re: [virtio-dev] Re: [PATCH v3 0/3] transport-pci: Introduce legacy registers access using AQ

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

On Thu, Jun 08, 2023 at 07:00:32PM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Thursday, June 8, 2023 2:31 PM
> 
> > > > I'll do a proper review after the forum. Generally lots of small
> > > > things. Went looking just to give you a couple of
> > > > examples:
> > > > 	  too many mentions of VFs and PFs.
> > > > 	  text should talk about owner and member. Minimise
> > > > 	  mention of VFs to make it easier to extend to
> > > > 	  different group types.
> > > >
> > > True but most additions are in PCI transport chapter.
> > > But will change to member and owner.
> > 
> > Another thing that bothers me is that it references admin commands that are
> > defined later in the spec. I don't like it that we are making the reader jump back
> > and forth ...
> > Maybe it's better to put this in the admin command chapter.
> > 
> I considered that before.
> In its current form, there is very less back-n-forth jump.
> This is because admin chapter mostly enumerates the opcode.
> 
> And PCI legacy chapter talks about rest of the theory and command details.
> 
> When/if we have more transports for this, probably a generic place will be suitable.
> 
> Moving now to admin will surely have more back-n-forth as it needs to talk about PCI part and that PCI part will reside in PCI section.

well the point is that it's *back" not *forth*.
IOW add the command description in admin chapter,
make it point to legacy description in pci chapter which reader has
already read.

> > > > another example:
> > > > 	+The PCI VF device SHOULD NOT expose PCI BAR 0 when it prefers to
> > > > support
> > > >
> > > > VFs don't expose BARs at all. PF exposes VF BARs in SRIOV capability.
> > > >
> > > Yes, it is exposed by PF, the wording of "PCI VF device exposing" is not right.
> > > I will reword it.
> > 
> > So here's an example wording, I don't insist on it exactly but the point is to show
> > how we should use spec terminology whereever
> > possible:
> > 
> > If an owner of an SRIOV group supports all of
> > VIRTIO_ADMIN_CMD_LCC_REG_WRITE,
> > VIRTIO_ADMIN_CMD_LCC_REG_READ .... then it SHOULD NOT expose VF BAR0
> > (of non 0 size) as part of its SRIOV capability; this is to facilitate emulating IO
> > BAR0 for the legacy interface in software.
> > 
> Yes good one. Will use.

right and my point is try to word all of it like this.