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:42 AM

> > > 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:
>
Conformance chapter doesn't have any links to aq requirements.

 
> 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? 
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.
>
Yes, but I don't see such details for the AQ, notification coalescing.
These commands are not special.
Commands described "what" they do. "When and how" is not described in the spec.
It is left to the driver to compose such things.

How Net device should use "queue reset" is not explained in theory of operation.
What is described is mechanics of queue reset steps...

Those details were put in the commit log, and not in the spec.
If you want to add theory of operation like nvme spec, we need to have dedicated section.
I don't see a reason, why this should be the first patch set to do so in v4.

I am more than happy to add, but it can be done in generic way as follow up.
 
> 
> 
> > > 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?
>
BAR 0 should not be used at all by the VF device for anything including 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?
> >
> 
> ?
> 
New command also should not report BAR 0.

> > > 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.
> 
All the pci vendor capabilities for which all the code already exists.
It is not related to code saving.

Even those cap check is not needed.
Hypervisor driver needs to only check that BAR-0 is empty of not from the PCI layer.