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

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

On Sun, Jun 04, 2023 at 03:01:52PM +0000, Parav Pandit wrote:
> 
> 
> > 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.

Good point, but the section itself is not missing, just a couple of links
in the conformance chapter. Sounds like a trivial omission we
can correct as an editorial fix.

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

I certainly tried for AQ.

For notification coalescing there's a whole paragraph:
\subparagraph{Operation}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / Operation}



> These commands are not special.
> Commands described "what" they do. "When and how" is not described in the spec.

If the "what" is "return offset for notifications" but there's no
description on what these notifications are then the "what" is
at best incomplete.

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

I don't think so personally.
As bigger issues get fixed, we start polishing up smaller ones.
It is reasonable to ask that people try to decide on a high level
design up front to avoid sending you back to the drawing board -
though that's unavoidable sometimes, a straw can break camel's back,
but that's not the case here.

Nitpicking when the design itself is undergoing big changes is
unproductive for everyone - reviewers and contributors.
Now you are getting asked to clarify things is a good sign that
the design is converging. The less time we keep arguing about
protocol the faster this will happen.

And I commented on previous versions too that the notification part is
not near ready, and suggested splitting it up to a follow up patch.
This is still the case, and I still think you should get the 4 config
access commands merged first then focus on notifications.

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

Check up on the difference between SHOULD and MUST in the RFC.
If things break without this then you make it MUST. SHOULD is
a recommendation.

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

Isn't code saving the reason you want to prevent VFs from using BAR0?

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

How do you check this? BAR0 size 0? Then say so. That affects all VFs and some of
them might not have legacy access.

-- 
MST