Re: [virtio-comment] Re: [PATCH v1 2/2] transport-pci: Add legacy register access conformance section

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

On Wed, May 03, 2023 at 10:53:56AM -0400, Parav Pandit wrote:
> 
> 
> On 5/3/2023 1:48 AM, Michael S. Tsirkin wrote:
> > On Wed, May 03, 2023 at 06:26:59AM +0300, Parav Pandit wrote:
> > > Add device and driver conformanace section for legacy registers access
> > > commands interface.
> > > 
> > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
> > > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > > ---
> > >   conformance.tex           |  2 ++
> > >   transport-pci-vf-regs.tex | 31 +++++++++++++++++++++++++++++++
> > >   2 files changed, 33 insertions(+)
> > > 
> > > diff --git a/conformance.tex b/conformance.tex
> > > index 01ccd69..dbd8cd6 100644
> > > --- a/conformance.tex
> > > +++ b/conformance.tex
> > > @@ -109,6 +109,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
> > >   \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / PCI configuration access capability}
> > >   \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
> > >   \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
> > > +\item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
> > >   \end{itemize}
> > >   \conformance{\subsection}{MMIO Driver Conformance}\label{sec:Conformance / Driver Conformance / MMIO Driver Conformance}
> > > @@ -194,6 +195,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
> > >   \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
> > >   \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Used Buffer Notifications}
> > >   \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
> > > +\item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
> > >   \end{itemize}
> > >   \conformance{\subsection}{MMIO Device Conformance}\label{sec:Conformance / Device Conformance / MMIO Device Conformance}
> > > diff --git a/transport-pci-vf-regs.tex b/transport-pci-vf-regs.tex
> > > index 16ced32..7d0574b 100644
> > > --- a/transport-pci-vf-regs.tex
> > > +++ b/transport-pci-vf-regs.tex
> > > @@ -82,3 +82,34 @@ \subsubsection{Legacy Queue Notify Offset Query}\label{sec:Virtio Transport Opti
> > >   	le64 offset; /* Byte offset within the BAR */
> > >   };
> > >   \end{lstlisting}
> > > +
> > > +\devicenormative{\paragraph}{SR-IOV VFs Legacy Registers Access}{Virtio Transport Options / Virtio Over PCI Bus / SR-IOV Legacy Registers Access}
> > > +
> > > +If the PCI PF device supports legacy registers access, it SHOULD set
> > > +corresponding bits for commands VIRTIO_ADMIN_CMD_LREG_WRITE,
> > > +VIRTIO_ADMIN_CMD_LREG_READ and VIRTIO_ADMIN_CMD_LQ_NOTIFY_QUERY in
> > > +command result of VIRTIO_ADMIN_CMD_LIST_QUERY in
> > > +\field{device_admin_cmd_opcodes}.
> > 
> > Don't repeat documentation of VIRTIO_ADMIN_CMD_LIST_QUERY please.
> yeah, I had dual thoughts, I am fine if this looks duplicate.
> Will remove.
> 
> > just say all these must be supported.
> > In fact what are you saying here? That all 3 are supported
> > or none at all? What about just
> > VIRTIO_ADMIN_CMD_LREG_READ/VIRTIO_ADMIN_CMD_LREG_WRITE?
> > Looks like a slower but working way to do notifications
> > is through a common config write, no?
> Notifications to be done using the notification region exposed and queried
> using the 3rd QUERY command.
> > 
> > > +
> > > +The device MUST support legacy configuration registers to its defined width.
> > 
> > what is this?
> > 
> Each register has its defined width as 8-bit, 16-bit, 32-bit etc.
> So device support the access to its defined width.
> May be to rewrite it differently?

Again you are duplicating the existing text, no?
And in this case, contradicting it?

	When using the legacy interface the driver MAY access
	the device-specific configuration region using any width accesses, and
	a transitional device MUST present driver with the same results as
	when accessed using the ``natural'' access method (i.e.
	32-bit accesses for 32-bit fields, etc).


> > > +
> > > +The device MAY fail legacy configuration registers access when either the
> > > +access is for an incorrct register width or if the register offset is incorrect.
> > 
> Spell checkers didnt capture the error of incorrct. Need to find better
> tool.
> 
> > with which error code?
> EINVAL
> but should we repeat the general section here?

this is a command specific case, isn't it?


> > 
> > > +
> > > +The device MUST allow access of one or multiple bytes of the registers when
> > > +such register is defined as byte array, for example \field{mac} of \field{struct
> > > +virtio_net_config} of the Network Device.
> > 
> > so which accesses need to be supported then?
> > 
> Not sure I follow the question.
> Can you please explain?

Consider mac, do you allow access to any length at all, from 1 to 6
bytes?