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 6:10 PM

> > 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.
>
Yes, to be done.
 
> > > 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.
>
There are many details about "access other device" design details located in the commit log, which is not present in the AQ section.

 
> For notification coalescing there's a whole paragraph:
> \subparagraph{Operation}\label{sec:Device Types / Network Device / Device
> Operation / Control Virtqueue / Notifications Coalescing / Operation}
>
The paragraph explains "what" part of the operation, not the design of how/when things should be done by the driver. 
I am not against adding the theory of operation, but I clearly see being nitpicked.

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

The command description says:
"This command returns the notify offset of the member VF for virtqueue driver notifications"

What more to tell than this, as "driver notifications" is clearly described in the section 2.9 for it.
May be I can make it little more verbose.

> > 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.
> 
Most or all questions about notify were literally answered in v2.
I missed to add them to the alternatives considered section in the commit log.
I will add it.

> 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.
>
After you asked about this in v1, I further clarified in v2.
I would like to cover the notification part as well. It is really the small piece as it is not baking any special or new concept.

> > > 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.
>
One may be still able to work by having it as "SHOULD", it may be just too much of code without much use.
Hence, I kept it as SHOULD.

> Isn't code saving the reason you want to prevent VFs from using BAR0?
>
Code and driver complexity both.
 
> > 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.

I don't think we need to enforce this by writing what PCI spec already defines.
For example, we don't write how MSI-X PBA addresses to be programmed when programing a vector and how to memory map PCI BAR region.
It is left to the driver to figure out how to detect BAR0 supported or not for a device.
Multiple solutions exists left to the driver to decide.