Re: [virtio-comment] [PATCH v2 1/4] Add virtio Admin Virtqueue

[Max Gurtovoy <mgurtovoy@nvidia.com>]

On 1/27/2022 5:14 AM, Jason Wang wrote:
> å 2022/1/24 äå5:39, Max Gurtovoy åé:
> > In one of the many use cases a user wants to manipulate features and
> > configuration of the virtio devices regardless of the device type
> > (net/block/console). Some of this configuration is generic enough. i.e
> > Number of MSI-X vectors of a virtio PCI VF device. There is a need to do
> > such features query and manipulation by its parent PCI PF.
> >
> > Currently virtio specification defines control virtqueue to manipulate
> > features and configuration of the device it operates on. However,
> > control virtqueue commands are device type specific, which makes it very
> > difficult to extend for device agnostic commands.
> >
> > To support this requirement in elegant way, this patch introduces a new
> > admin virtqueue. Admin virtqueue will use the same command format for 
> > all
> > types of virtio devices.
> >
> > Manipulate features via admin virtqueue is asynchronous, scalable, easy
> > to extend and doesn't require additional and expensive on-die resources
> > to be allocated for every new feature that will be added in the future.
> >
> > Subsequent patches make use of this admin virtqueue.
> >
> > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > ---
> > Â admin-virtq.tex | 89 +++++++++++++++++++++++++++++++++++++++++++++++++
> > Â content.texÂÂÂÂ |Â 9 +++--
> > Â 2 files changed, 96 insertions(+), 2 deletions(-)
> > Â create mode 100644 admin-virtq.tex
> >
> > diff --git a/admin-virtq.tex b/admin-virtq.tex
> > new file mode 100644
> > index 0000000..1a41c22
> > --- /dev/null
> > +++ b/admin-virtq.tex
> > @@ -0,0 +1,89 @@
> > +\section{Admin Virtqueues}\label{sec:Basic Facilities of a Virtio 
> > Device / Admin Virtqueues}
> > +
> > +Admin virtqueue is used to send administrative commands to manipulate
> > +various features of the device and/or to manipulate various features,
> > +if possible, of another device within the same group (e.g. PCI VFs of
> > +a parent PCI PF device are grouped together. These devices can be
> > +optionally managed by its parent PCI PF using its admin virtqueue.).
> > +
> > +Use of Admin virtqueue is negotiated by the VIRTIO_F_ADMIN_VQ
> > +feature bit.
> > +
> > +Admin virtqueue index may vary among different device types.
> > +
> > +Regardless of device offering VIRTIO_F_IN_ORDER, admin queue command 
> > buffers
> > +are used by the device in out of order manner.
> > +
> > +The Admin command set defines the commands that may be issued only 
> > to the admin
> > +virtqueue. Each virtio device that advertises the VIRTIO_F_ADMIN_VQ 
> > feature, MUST
> > +support all the mandatory admin commands. A device MAY support also 
> > one or more
> > +optional admin commands. All commands are of the following form:
>
>
> Do we need a way for advertising the supported optional commands (or 
> features bits)? Otherwise dealing with the compatibility will result 
> of per command probing.
>
>
> > +
> > +\begin{lstlisting}
> > +struct virtio_admin_cmd {
> > +ÂÂÂÂÂÂÂ /* Device-readable part */
> > +ÂÂÂÂÂÂÂ u16 command;
>
>
> Two questions:
>
> 1) It looks to me it's better to have a explicit device_id here other 
> than embed it in each command

this was already discussed.

We agreed that some commands are not referring to different device so 
no, we don't need to put it in generic structure.

I'm not against putting such id but we don't have an exact idea how will 
SF device_id will look like.

Maybe it will be some 128 bit uuid ? maybe u64 ? how can we guess ?


> 2) virtio-net cvq use class/command which seems more elegant, e.g all 
> commands belongs to MSI could be grouped into the same class

why can't we do it in u16 command opcode ? are you open for changes or 
should we copy/paste from net cvq ?

Let's ask differently. Why we need class/command structure ? why is it 
more elegant ?

> > +ÂÂÂÂÂÂÂ u8 command_specific_data[];
> > +
> > +ÂÂÂÂÂÂÂ /* Device-writable part */
> > +ÂÂÂÂÂÂÂ u8 status;
> > +ÂÂÂÂÂÂÂ u8 command_specific_error;
>
>
> I wonder the reason why not using a single field for the above two 
> fields. (Or any value for separating command specific error out, we 
> don't do that for virtio-net cvq).

each command have its own specific errors.

virtio net cvq is not generic - it's a net device cvqueue.

To make it generic we need to separate.

> > +ÂÂÂÂÂÂÂ u8 command_specific_result[];
> > +};
> > +\end{lstlisting}
> > +
> > +The following table describes the generic Admin status codes:
> > +
> > +\begin{tabular}{|l|l|l|}
> > +\hline
> > +Opcode & Status & Description \\
> > +\hline \hline
> > +00h & VIRTIO_ADMIN_STATUS_OK & successful completion \\
> > +\hline
> > +01h & VIRTIO_ADMIN_STATUS_CS_ERR & command specific error \\
> > +\hline
> > +02hÂÂ & VIRTIO_ADMIN_STATUS_COMMAND_UNSUPPORTEDÂÂÂ & unsupported or 
> > invalid opcode \\
> > +\hline
> > +\end{tabular}
> > +
> > +The \field{command} and \field{command_specific_data} are
> > +set by the driver, and the device sets the \field{status}, the
> > +\field{command_specific_error} and the \field{command_specific_result},
> > +if needed.
> > +
> > +The \field{command_specific_error} should be inspected by the driver 
> > only if \field{status} is set to
> > +VIRTIO_ADMIN_STATUS_CS_ERR by the device. In this case, the content 
> > of \field{command_specific_error}
> > +holds the command specific error. If \field{status} is not set to 
> > VIRTIO_ADMIN_STATUS_CS_ERR, the
> > +\field{command_specific_error} value is undefined.
> > +
> > +The following table describes the Admin command set:
> > +
> > +\begin{tabular}{|l|l|l|}
> > +\hline
> > +Opcode & Command & M/O \\
> > +\hline \hline
> > +0000hÂÂ & VIRTIO_ADMIN_CAPS_IDENTIFYÂÂÂ & MÂ \\
> > +\hline
> > +0001h - 7FFFhÂÂ & Generic admin cmdsÂÂÂ & -Â \\
> > +\hline
> > +8000h - FFFFhÂÂ & ReservedÂÂÂ & - \\
> > +\hline
> > +\end{tabular}
>
>
> See above, I'd suggest to use class/command as virtio-net cvq.

see my comment above.

> > +
> > +\subsection{VIRTIO ADMIN CAPS IDENTIFY command}\label{sec:Basic 
> > Facilities of a Virtio Device / Admin Virtqueues / VIRTIO ADMIN CAPS 
> > IDENTIFY command}
> > +
> > +The VIRTIO_ADMIN_CAPS_IDENTIFY command has no command specific data 
> > set by the driver.
> > +This command upon success, returns a data buffer that describes 
> > information about the supported
> > +admin capabilities by the device. This information is of form:
> > +\begin{lstlisting}
> > +struct virtio_admin_caps_identify_result {
> > +ÂÂÂÂÂÂ /*
> > +ÂÂÂÂÂÂÂ * caps[0] - Bit 0x0 - Bit 0x7 are reserved
> > +ÂÂÂÂÂÂÂ * caps[1] - Bit 0x0 - Bit 0x7 are reserved
> > +ÂÂÂÂÂÂÂ * caps[2] - Bit 0x0 - Bit 0x7 are reserved
> > +ÂÂÂÂÂÂÂ * ....
> > +ÂÂÂÂÂÂÂ * caps[8191] - Bit 0x0 - Bit 0x7 are reserved
> > +ÂÂÂÂÂÂÂ */
> > +ÂÂÂÂÂÂ u8 caps[8192];
> > +};
>
>
> Ok, so I see the identify command. But I wonder if we can do that via 
> the feature bits?

It doesn't scale. I tried it. It doesn't work.


Thanks.

> Thanks
>
>
> > +\end{lstlisting}
> > diff --git a/content.tex b/content.tex
> > index 32de668..c524fab 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -99,10 +99,10 @@ \section{Feature Bits}\label{sec:Basic Facilities 
> > of a Virtio Device / Feature B
> > Â \begin{description}
> > Â \item[0 to 23] Feature bits for the specific device type
> > Â -\item[24 to 40] Feature bits reserved for extensions to the queue and
> > +\item[24 to 41] Feature bits reserved for extensions to the queue and
> > ÂÂÂ feature negotiation mechanisms
> > Â -\item[41 and above] Feature bits reserved for future extensions.
> > +\item[42 and above] Feature bits reserved for future extensions.
> > Â \end{description}
> > Â Â \begin{note}
> > @@ -449,6 +449,8 @@ \section{Exporting Objects}\label{sec:Basic 
> > Facilities of a Virtio Device / Expo
> > Â types. It is RECOMMENDED that devices generate version 4
> > Â UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}.
> > Â +\input{admin-virtq.tex}
> > +
> > Â \chapter{General Initialization And Device 
> > Operation}\label{sec:General Initialization And Device Operation}
> > Â Â We start with an overview of device initialization, then expand 
> > on the
> > @@ -6847,6 +6849,9 @@ \chapter{Reserved Feature 
> > Bits}\label{sec:Reserved Feature Bits}
> > ÂÂÂ that the driver can reset a queue individually.
> > ÂÂÂ See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / 
> > Virtqueue Reset}.
> > Â +Â \item[VIRTIO_F_ADMIN_VQ (41)] This feature indicates that
> > +Â the device supports administration virtqueue negotiation.
> > +
> > Â \end{description}
> > Â Â \drivernormative{\section}{Reserved Feature Bits}{Reserved 
> > Feature Bits}
>
>
> This publicly archived list offers a means to provide input to the
> OASIS Virtual I/O Device (VIRTIO) TC.
>
> In order to verify user consent to the Feedback License terms and
> to minimize spam in the list archive, subscription is required
> before posting.
>
> Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> List help: virtio-comment-help@lists.oasis-open.org
> List archive: 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.oasis-open.org%2Farchives%2Fvirtio-comment%2F&amp;data=04%7C01%7Cmgurtovoy%40nvidia.com%7Ca448108b88d84d87023d08d9e143238e%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637788501921358048%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=SW%2FM98QZ7Av2wsv1NoekveDdQmlfWMYcvZQiFvpLcAE%3D&amp;reserved=0
> Feedback License: 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fwho%2Fipr%2Ffeedback_license.pdf&amp;data=04%7C01%7Cmgurtovoy%40nvidia.com%7Ca448108b88d84d87023d08d9e143238e%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637788501921358048%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=C5D9qmFqbNrx7CoTzsx%2B9qalts534XPaImHlcszwm1M%3D&amp;reserved=0
> List Guidelines: 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fpolicies-guidelines%2Fmailing-lists&amp;data=04%7C01%7Cmgurtovoy%40nvidia.com%7Ca448108b88d84d87023d08d9e143238e%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637788501921358048%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=bi7OJDuH9CvoKdxtA01dn5SpZwDp3Bna4%2B00bVOuz2o%3D&amp;reserved=0
> Committee: 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fcommittees%2Fvirtio%2F&amp;data=04%7C01%7Cmgurtovoy%40nvidia.com%7Ca448108b88d84d87023d08d9e143238e%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637788501921358048%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=SdGNrxTUZVIe2WJwb%2BdbT2UxagM9AdEzDWXccFvRE3s%3D&amp;reserved=0
> Join OASIS: 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fjoin%2F&amp;data=04%7C01%7Cmgurtovoy%40nvidia.com%7Ca448108b88d84d87023d08d9e143238e%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637788501921358048%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=ynKmp30vKJMw%2FL4IHbBFY9EebT%2BsVX2%2Boz7IIcO%2F1xg%3D&amp;reserved=0