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

[Parav Pandit <parav@nvidia.com>]

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?

>> +
>> +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?

> 
>> +
>> +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?

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org