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

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

On Sun, Jun 04, 2023 at 02:32:23PM +0000, Parav Pandit wrote:
> > 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.

Hmm missing where? It was supposed to be added here:

commit bf1d6b0d24ae899d08c7cf24ed480cf5881c49ef
Author: Michael S. Tsirkin <mst@redhat.com>
Date:   Sun Nov 20 19:43:45 2022 -0500

    admin: conformance clauses

what did I miss?   


> > 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.

yes but I think this assumes a bit much. you start by saying there's no
IO BAR. Okay, it's not even mandatory to give motivation, but can't
hurt. The next step is you describe a bunch of commands. Presumably
the reader will draw the conclusion that they replace an io bar?
But it's implicit, and that is not good.

So, I imagine we have a driver that translates legacy accesses
to AQ commands, yes? And it also somehow uses the extra
command to get an address and then translates notifications
to writes at that address? This kind of thing is what
I called a theory of operation.

A couple of paragraphs.



> > 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".

But what is the answer?

> > 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.

which capability? E.g. there's a vendor specific capability we don't
even know what's in there. If you really want to save code in
the hypervisor this "should" is pretty useless.

-- 
MST