RE: [virtio-dev] [PATCH v13] admin: Add group member legacy register access commands

[Parav Pandit <parav@nvidia.com>]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Sunday, July 9, 2023 9:47 AM
> > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
> > Signed-off-by: Parav Pandit <parav@nvidia.com>
> 
> Ugh. I have a feeling you are rewriting what I wrote so we keep doing more and
> more review rounds.  I would prefer it if I could just comment instead of
> rewriting - that's really going way beyond normal review - but that seems to get
> us nowhere so here we are.  But given this, if you don't want to use what I wrote
> then please comment.
>
Please see inline, why some parts are rewritten.
Only two lines to my knowledge are rewritten because you wrote,

" when the \field{flags} is 0x2 this is specified by the VF BARn"

Above "this is specified" was confusing to read.

It meant to me as that "this" refers to "bar".
So it interpreted to me as 

"when flags is 0x2, bar is specified by the Base Address Registers in the PCI ...".

Here bar is not specified, bar specifies which register of which device to refer to.

What we wanted to say that,

"bar" field here points to (specifies) the Base Address Registers in the PCI ...

So that part was rewritten.

> 
> > ---
> > changelog:
> > v12->v13:
> > - added article
> > - add hyphen between little and endian
> > - mentioned vq index depth of 16-bit
> > - rewrote alternative approach line
> > - mention vq index, length and endianness in mmio description
> > - fixed padding bytes size from 7 to 6 bytes
> > - rewrote bar field description
> > - offset alignment text added
> > - added text to ignore reserved notification entries
> > - device and driver conformance lines added for notification info
> > command fields
> > - dropped group member prefix to the driver
> > - reworded text for flags requirements
> > - reworded to say all driver notifications in conformance
> > - itemize conformance entries under command to ease reading

Most of above comments were addressed as_is as you wrote of asked to write.

[..]

> > +struct virtio_admin_cmd_legacy_notify_info_result {
> > +        struct virtio_pci_legacy_notify_info entries[4]; };
> > +\end{lstlisting}
> > +
> > +The \field{flags} value of 0x1 indicates that the notification
> > +address is of the owner device, value of 0x2 indicates that the
> > +notification address is of the member device, the value of 0
> > +indicates that all the entries starting from that entry are invalid
> > +entries in \field{entries}. All other values in \field{flags} are
> > +reserved. The driver skips the entries whose \field{flag} contains reserved
> value.
> 
> contain reserved values.
>
Will fix this.

> > +
> > +The \field{bar} values 0x1 to 0x5 specifies a Base Address register
> > +(BAR),
> 
> specify
>
Will fix this.
 
> 
> > +when the \field{flags} is 0x1, \field{bar} specifies the Base Address
> > +Registers
> 
> one of the Base Address Registers
> 
> > +in the PCI header of the owner device, when the \field{flags} is 0x2,
> > +\field{bar} specifies the VF BARn registers in the SR-IOV Extended
> > +Capability of the device.
> 
Will fix this.

> These attempts to clarify only confused.
> Problem with this is that VF BARn is not valled "Base Address register"
> you also did not capitalize "r" here. And also VF BARn in the capability actually
> refers to VF0 only. Others are calculated from that.
> 
> This is why I wrote what I wrote in the previous review.
>
It is confusing, but I will take it as_is.
 
> 
> 
> > +
> > +The \field{offset} indicates the notification address relative to the
> > +base address associated with the BAR indicated in \field{bar}. This
> > +value
> 
> let's not invent the new term base address. Just "relative to the BAR" should be
> enough.
Will fix it.

> 
> > +is 2-byte aligned.
> > +
> > +When the command completes successfully,
> > +\field{command_specific_result} is in the format of \field{struct
> > +virtio_admin_cmd_legacy_notify_info_result}. The device can supply up
> > +to 4 entries each with a different notification address. In this
> > +case, any of the entries can be used by the driver. The order of the
> > +entries serves as a preference hint to the driver. The driver is
> > +expected to utilize the entries placed earlier in the array to the later ones.
> The driver is also expected to ignore reserved entries.
> 
> 
> You need to document end of list too.
>
End of list is described where the flag values 0,1,2 are described.
Should we duplicate here?
 
> I wrote all of this in my previous review. Donnu why you decided to rewrite yet
> again but here we are.
> 
I rechecked.
I didnât find rewrite for above lines in [1].
You rewrote the conformance section.

[1] https://lists.oasis-open.org/archives/virtio-comment/202307/msg00125.html


> > +If within \field{struct virtio_admin_cmd_legacy_notify_info_result}
> > +returned by VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO, the \field{flags}
> > +value for a specific \field{struct virtio_pci_legacy_notify_info}
> > +entry is 0x0, the driver MUST ignore this entry and all the following
> > +\field{entries}. The driver MUST additionally validate, for each
> > +entry, that \begin{itemize} \item the \field{flags} is either 0x0,
> > +0x1 or 0x2 \item the \field{bar} corresponds to a valid BAR of either
> > +the owner or the member device, depending on the \field{flags} \item
> > +the \field{offset} is 2-byte aligned and corresponds to an address
> > +within the BAR specified by the \field{bar} on \field{flags}
> > +\end{itemize}, any entry which does not meet these constraints MUST
> > +be ignored by the driver.
>
I didnât rewrite this fully.

You were missing the word "ignore" after MUST in [2], so I added it.
[2] https://lists.oasis-open.org/archives/virtio-comment/202307/msg00125.html

I added normative which you didnât write, but you gave comments for offset, alignment.
I added those.
 
Secondly you had comment after your rewrite  as:

"and MUST ignore an entry if any of these constraints are
violated; this is to allow for future extensions."

Such multi-line single sentence is hard to read.
Hence, I added, "," and slightly reworded as "any entry which does not ".

I missed the end of list conformance line, sorry about it. Adding it.

> Wait a second.
> - end of list not documented - we want conformance statements
> - for end of list I am guessing other fields do not matter
> - entries after end of list also do not matter?
>
We added the description for end of list where the actual flags value is described.
Missed in conformance. Will add it.