RE: [PATCH v3 2/3] transport-pci: Introduce legacy registers access commands

[Parav Pandit <parav@nvidia.com>]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Sunday, June 4, 2023 10:14 AM
> 
> On Sun, Jun 04, 2023 at 01:51:20PM +0000, Parav Pandit wrote:
> >
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Sunday, June 4, 2023 9:22 AM
> >
> > > > +The driver that may use the driver notifications region of the VF
> > > > +device returned in this result likely attain higher performance
> > > > +or the drier may use the VIRTIO_ADMIN_CMD_LREG_WRITE command.
> > >
> > > Obtain I guess ... but how? There's no explanation.
> > >
> > Do you suggest to rewrite, above as below?
> >
> > The driver that MAY use the driver notifications region of the VF likely obtain
> higher performance.
> > The driver may use VIRTIO_ADMIN_CMD_LCC_REG_WRITE command for
> doorbell notifications.
> 
> No this does not address the issue that there is no description of how this
> command is used just a hint at what it returns. 

> So this gets us some offset into
> some bar now what?  
> I am guessing writing vqn at this offset into bar has the
> same effect as issuing VIRTIO_ADMIN_CMD_LCC_REG_WRITE with offset 16
> and data including the vqn?
> 
I can add the one line description that describes above. I take note for v4.

> 
> BTW all these may/must/should need to go into conformance section.
>
Conformance section for the AQ itself is missing. And I am aware to fix it post your v13 series.
 
> Generally the way we structure the spec is an explanation of the theory of
> operation (mostly missing here)
>
It is not any different from v2.
Theory of operation is not going to describe the whole driver design.
It is not likely the scope like any other section.
Section "Legacy Interfaces: SR-IOV VFs Registers Access" has theory of operation described and also in commit log.

This is like recent AQ patches and notification coalescing and more.

So if can pin point what exactly you want to see in theory of operation, it will be helpful.
Because for one person, 10 lines of theory is enough like how we have most of the spec, another wants 100 lines with pseudo code in appendix.

> v2 had other issues so I missed this one.
> 
> > But ok.
> >
> > No. It is not related to last command.
> > What does a PCI Device use BAR for?
> 
> To reserve address space and know which addresses to claim.
> 
> Generally if I heard "not use BAR0" I would assume that the meaning is that VF
> BAR0 in SRIOV expended capability should be 0. However, that affects all VFs
> and not just this VF so I don't really know what can it mean.
>
A PCI PF and VF device which wants to support legacy emulation should not expose BAR 0 in the struct virtio_pci_cap struct.
Instead it should use other BARs.

> > As described in the spec, it uses the BAR in struct virtio_pci_cap for exposing
> various things.
> 
> Now I'm confused.
> So do you mean the \field{bar} in virtio_pci_cap or PCI BAR?
>
Yes.
 
> > So it means that PCI VF should use other than PCI BAR 0 for various Virtio
> Structure PCI Capabilities that it exposes.
> 
> I suspect you then want to say "should not expose to driver any structures
> inside it's BAR0"?
Instead of bisecting at structure level, it is indicated on usage so that it doesn't need to bisect on "what about MSI-X".

> And does this include this new command you are adding or not?
> It mentions bar too.  What about e.g. MSIX tables? Could there be other
> capabilities referring to a BAR? 

> How does hypervisor know whether VF followed
> this rule (it's a should, not a hard rule after all)?
> 
It is not a MUST.
Hypervisor software using this VF very easily know this when VF is attached to the hypervisor driver by reading the capability.