Discussion of the VIRTIO specification
 help / color / mirror / Atom feed
From: Max Gurtovoy <mgurtovoy@nvidia.com>
To: Jason Wang <jasowang@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>
Cc: virtio-comment@lists.oasis-open.org, cohuck@redhat.com,
	stefanha@redhat.com, oren@nvidia.com, parav@nvidia.com,
	shahafs@nvidia.com, eperezma@redhat.com, aadam@redhat.com,
	bodong@nvidia.com, amikheev@nvidia.com
Subject: Re: [RFC PATCH v2 1/2] Add virtio Admin Virtqueue specification
Date: Mon, 2 Aug 2021 12:51:30 +0300	[thread overview]
Message-ID: <29acb062-e541-3359-cbdb-b777249c222c@nvidia.com> (raw)
In-Reply-To: <6de5eb17-1d0d-45d5-b1fa-e21989341b4f@redhat.com>


On 8/2/2021 5:25 AM, Jason Wang wrote:
>
> 在 2021/7/28 下午8:48, Michael S. Tsirkin 写道:
>> On Mon, Jul 26, 2021 at 07:52:53PM +0300, Max Gurtovoy wrote:
>>> Admin virtqueues will be used to send administrative commands to
>>> manipulate various features of the device which would not easily map
>>> into the configuration space.
>>>
>>> The same Admin command format will be used for all virtio devices. The
>>> Admin command set will include 4 types of command classes:
>>> 1. The generic common class
>>> 2. The transport specific class
>>> 3. The device specific class
>>> 4. The vendor specific class
>>>
>>> The above mechanism will enable adding various features to the virtio
>>> specification, e.g.:
>>> 1. Format virtio-blk devices in various configurations (512B block 
>>> size,
>>>     512B + 8B T10-DIF, 4K block size, 4k + 8B T10-DIF, etc..).
>>> 2. Live migration management.
>>> 3. Encrypt/Decrypt descriptors.
>>> 4. Virtualization management.
>>> 5. Get device error logs.
>>> 6. Implement advanced vendor/device/transport specific features.
>>> 7. Run device health test.
>>> 8. More.
>>>
>>> As virtio evolves beyond the para-virt/sw-emulated world, it's 
>>> mandatory
>>> for the specification to become flexible and allow a wider feature set.
>>> The corrent ctrl virtq that is defined for some of the virtio 
>>> devices is
>>> device specific and wasn't designed to be a generic virtq for
>>> admininistration.
>>>
>>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>> Lots of things on this list seem to make sense when
>> done from PF and affecting a VF.
>> I think from this POV the generic structure should include
>> a way to address one device from another.
>>
>>
>>> ---
>>>   admin-virtq.tex | 241 
>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>   content.tex     |   4 +
>>>   2 files changed, 245 insertions(+)
>>>   create mode 100644 admin-virtq.tex
>>>
>>> diff --git a/admin-virtq.tex b/admin-virtq.tex
>>> new file mode 100644
>>> index 0000000..ccec2ca
>>> --- /dev/null
>>> +++ b/admin-virtq.tex
>>> @@ -0,0 +1,241 @@
>>> +\section{Admin Virtqueues}\label{sec:Basic Facilities of a Virtio 
>>> Device / Admin Virtqueues}
>>> +
>>> +Admin virtqueues are used to send administrative commands to 
>>> manipulate
>>> +various features of the device which would not easily map into the
>>> +configuration space.
>>> +
>>> +Use of Admin virtqueues is negotiated by the VIRTIO_F_ADMIN_VQ
>>> +feature bit.
>>> +
>>> +Admin virtqueue index may vary among different device types.
>>> +
>>> +All commands are of the following form:
>>> +
>>> +\begin{lstlisting}
>>> +struct virtio_admin_cmd {
>>> +        /* Device-readable part */
>>> +        u8 class;
>>> +        u8 command;
>>> +        u8 command-specific-data[];
>>> +
>>> +        /* Device-writable part */
>>> +        u8 command-specific-result[];
>>> +        u8 status_type : 4;
>>> +        u8 reserved : 4;
>>> +        u8 status;
>>> +};
>>> +
>>> +/* Status type values */
>>> +#define VIRTIO_ADMIN_STATUS_TYPE_GENERIC               0
>>> +#define VIRTIO_ADMIN_STATUS_TYPE_CLASS_SPECIFIC        1
>>> +#define VIRTIO_ADMIN_STATUS_TYPE_COMMAND_SPECIFIC      2
>>> +#define VIRTIO_ADMIN_STATUS_TYPE_TRANSPORT_SPECIFIC    3
>>> +#define VIRTIO_ADMIN_STATUS_TYPE_DEVICE_SPECIFIC       4
>>> +#define VIRTIO_ADMIN_STATUS_TYPE_VENDOR_SPECIFIC       5
>>> +
>>> +/* Generic status values */
>>> +#define VIRTIO_ADMIN_STATUS_GENERIC_OK                     0
>>> +#define VIRTIO_ADMIN_STATUS_GENERIC_ERR                    1
>>> +#define VIRTIO_ADMIN_STATUS_GENERIC_INVALID_CLASS          2
>>> +#define VIRTIO_ADMIN_STATUS_GENERIC_INVALID_COMMAND        3
>>> +#define VIRTIO_ADMIN_STATUS_GENERIC_DATA_TRANSFER_ERR      4
>>> +#define VIRTIO_ADMIN_STATUS_GENERIC_DEVICE_INTERNAL_ERR    5
>>> +\end{lstlisting}
>>> +
>>> +The \field{class}, \field{command} and 
>>> \field{command-specific-data} are
>>> +set by the driver, and the device sets the \field{status_type}, the
>>> +\field{status} and  the \field{command-specific-result}, if needed.
>>> +
>>> +The virtio Admin command class codes are divided in the following 
>>> form:
>>> +
>>> +\begin{lstlisting}
>>> +/* class values that are transport, device and vendor independent */
>>> +#define VIRTIO_ADMIN_COMMON_CLASS_START    0
>>> +#define VIRTIO_ADMIN_COMMON_CLASS_END      63
>>> +
>>> +/* class values that are transport specific */
>>> +#define VIRTIO_ADMIN_TRANSPORT_CLASS_START  64
>>> +#define VIRTIO_ADMIN_TRANSPORT_CLASS_END    127
>>> +
>>> +/* class values that are device specific */
>>> +#define VIRTIO_ADMIN_DEVICE_CLASS_START     128
>>> +#define VIRTIO_ADMIN_DEVICE_CLASS_END       191
>>> +
>>> +/* class values that are vendor specific */
>>> +#define VIRTIO_ADMIN_VENDOR_CLASS_START     192
>>> +#define VIRTIO_ADMIN_VENDOR_CLASS_END       255
>>> +\end{lstlisting}
>>> +
>>> +\subsection{Admin command set}\label{sec:Basic Facilities of a 
>>> Virtio Device / Admin Virtqueues / Admin command set}
>>> +
>>> +Each virtio device that advertise VIRTIO_F_ADMIN_VQ feature, MUST
>>> +support all the mandatory admin commands. A device MAY support also
>>> +one or more optional admin commands.
>>> +
>>> +\subsubsection{Common command set}\label{sec:Basic Facilities of a 
>>> Virtio Device / Admin Virtqueues / Admin command set / Common 
>>> command set}
>>> +
>>> +The Common command set is a group of classes and commands within each
>>> +of these classes which are transport, device and vendor independent.
>>> +A mandatory class is a class that has at least one mandatory command.
>>> +The Common command set is summarized in following table:
>>> +
>>> +\begin{tabular}{|l|l|l|}
>>> +\hline
>>> +Class  & Description    & M/O \\
>>> +\hline \hline
>>> +0  & VIRTIO_ADMIN_DISCOVER_DEVICE    & M \\
>>> +\hline
>>> +1  & VIRTIO_ADMIN_DISCOVER_DEVICE_CLASS_COMMANDS    & M \\
>>> +\hline
>>> +2-63  & reserved    & - \\
>>> +\hline
>>> +\end{tabular}
>>> +
>>> +\paragraph{Discover device class}\label{sec:Basic Facilities of a 
>>> Virtio Device / Admin Virtqueues / Admin command set / Common 
>>> command set / Discover device class}
>>> +
>>> +This class (opcode: 0) of commands is used to query generic device
>>> +information. The following table describes the commands supported for
>>> +this class:
>>> +
>>> +\begin{tabular}{|l|l|l|}
>>> +\hline
>>> +Command  & Description    & M/O \\
>>> +\hline \hline
>>> +0  & VIRTIO_ADMIN_DISCOVER_DEVICE_IDENTITY    & M \\
>>> +\hline
>>> +1  & VIRTIO_ADMIN_DISCOVER_DEVICE_SUPPORTED_CLASSES & M \\
>>> +\hline
>>> +2-255  & reserved    & - \\
>>> +\hline
>>> +\end{tabular}
>> So there are several problems with this approach.
>> One is that there is no two way negotiation.
>> No way for device to know what will driver use in the end.
>> This breaks things like e.g. accelerating some configurations
>> but not others.
>>
>> Another is that everything is going through the admin vq.
>> Hard for hypervisor to intervene and change just some aspects
>> of the device.
>>
>> This is also a problem for validation, tricky
>> to find out during probe what does device support and whether
>> driver can work with it.
>
>
> Yes, so I think it makes sense to make the admin virtqueue as a 
> transport and then build other stuffs on top.


what transport exactly ?

it should be a virtq for devices to open.


>
>
>>
>>
>> Can't we map this stuff into memory instead?
>> Maybe have an admin config structure
>> that is separate from regular config?
>> Only issue we can't then make it too big.
>> But so far I don't see a lot of info used, most is reserved.
>
>
> It should work (especially for PCI).

you're trying to invent some mechanism that is not flexible and non trivial.

You have already a virtq - so lets use it.



>
> Thanks
>
>
>>
>>
>>
>>> +
>>> +\subparagraph{Device identity command}\label{sec:Basic Facilities 
>>> of a Virtio Device / Admin Virtqueues / Admin command set / Common 
>>> command set / Discover device class / Device identity command}
>>> +
>>> +This mandatory command should return device identity in the following
>>> +structure:
>>> +
>>> +\begin{tabular}{|l|l|l|}
>>> +\hline
>>> +Bytes  & Description    & M/O \\
>>> +\hline \hline
>>> +03:00  & VIRTIO DEVICE ID    & M \\
>>> +\hline
>>> +05:04  & VIRTIO TRANSPORT ID    & M \\
>>> +\hline
>>> +4095:06  & reserved    & - \\
>>> + \hline
>>> +\end{tabular}
>>> +
>>> +\subparagraph{Device supported classes command}\label{sec:Basic 
>>> Facilities of a Virtio Device / Admin Virtqueues / Admin command set 
>>> / Common command set / Discover device class / Device supported 
>>> classes command}
>>> +
>>> +This mandatory command should return device identity in the following
>>> +structure:
>>> +
>>> +\begin{tabular}{|l|l|l|}
>>> +\hline
>>> +Bytes  & Description    & M/O \\
>>> +\hline \hline
>>> +03:00  & VIRTIO_ADMIN_CLASS_0_PROPERTIES    & M \\
>>> +\hline
>>> +07:04  & VIRTIO_ADMIN_CLASS_1_PROPERTIES    & M \\
>>> + \hline
>>> +11:08  & VIRTIO_ADMIN_CLASS_2_PROPERTIES    & M \\
>>> +\hline
>>> +...  & ...    & M \\
>>> +\hline
>>> +1023:1020  & VIRTIO_ADMIN_CLASS_255_PROPERTIES    & M \\
>>> +\hline
>>> +4095:1024  & reserved    & - \\
>>> +\hline
>>> +\end{tabular}
>>> +
>>> +The above structure is divided to 256 sections of 4B each, that will
>>> +describe whether a device supports a certain class code. The
>>> +following 3072B are reserved. each of the non-reserved 4B sections 
>>> will
>>> +follow the following data structure:
>>> +
>>> +\begin{tabular}{|l|l|l|}
>>> +\hline
>>> +Bits  & Description    & M/O \\
>>> +\hline \hline
>>> +0  & SUPPORTED (1: yes, 0:no)    & M \\
>>> +\hline
>>> +31:01  & reserved    & - \\
>>> + \hline
>>> +\end{tabular}
>>> +
>>> +\paragraph{Discover device class commands class}\label{sec:Basic 
>>> Facilities of a Virtio Device / Admin Virtqueues / Admin command set 
>>> / Common command set / Discover device class commands class}
>>> +
>>> +This class (opcode: 1) of commands is used to query supported commands
>>> +for each supported device class.
>>> The following table describes the commands
>>> +supported for this class:
>>> +
>>> +\begin{tabular}{|l|l|l|}
>>> +\hline
>>> +Command  & Description    & M/O \\
>>> +\hline \hline
>>> +0  & VIRTIO_ADMIN_CLASS_0_COMMANDS_IDENTITY    & M \\
>>> +\hline
>>> +1  & VIRTIO_ADMIN_CLASS_1_COMMANDS_IDENTITY    & M \\
>>> +\hline
>>> +2  & VIRTIO_ADMIN_CLASS_2_COMMANDS_IDENTITY    & M \\
>>> +\hline
>>> +...  & ...    & M \\
>>> +\hline
>>> +255  & VIRTIO_ADMIN_CLASS_255_COMMANDS_IDENTITY    & M \\
>>> +\hline
>>> +\end{tabular}
>>> +
>>> +Each command in this class, will return class identity in the 
>>> following structure:
>>> +
>>> +\begin{tabular}{|l|l|l|}
>>> +\hline
>>> +Bytes  & Description    & M/O \\
>>> +\hline \hline
>>> +03:00  & VIRTIO_ADMIN_COMMAND_0_PROPERTIES    & M \\
>>> +\hline
>>> +07:04  & VIRTIO_ADMIN_COMMAND_1_PROPERTIES    & M \\
>>> + \hline
>>> +11:08  & VIRTIO_ADMIN_COMMAND_2_PROPERTIES    & M \\
>>> +\hline
>>> +...  & ...    & M \\
>>> +\hline
>>> +1023:1020  & VIRTIO_ADMIN_COMMAND_255_PROPERTIES    & M \\
>>> +\hline
>>> +4095:1024  & reserved    & - \\
>>> +\hline
>>> +\end{tabular}
>>> +
>>> +The above structure is divided to 256 sections of 4B each, that will
>>> +describe whether a class supports a certain command code. The
>>> +following 3072B are reserved. each of the non-reserved 4B sections 
>>> will
>>> +follow the following data structure:
>>> +
>>> +\begin{tabular}{|l|l|l|}
>>> +\hline
>>> +Bits  & Description    & M/O \\
>>> +\hline \hline
>>> +0  & SUPPORTED (1: yes, 0:no)    & M \\
>>> +\hline
>>> +31:01  & reserved    & - \\
>>> + \hline
>>> +\end{tabular}
>>> +
>>> +\subsubsection{Transport command set}\label{sec:Basic Facilities of 
>>> a Virtio Device / Admin Virtqueues / Admin command set / Transport 
>>> command set}
>>> +
>>> +The Transport command set is a group of classes and commands within
>>> +each of these classes which are transport specific. Refer to each
>>> +transport section for more information on the supported commands.
>>> +
>>> +\subsubsection{Device command set}\label{sec:Basic Facilities of a 
>>> Virtio Device / Admin Virtqueues / Admin command set / Device 
>>> command set}
>>> +
>>> +The Device command set is a group of classes and commands within
>>> +each of these classes which are device specific. Refer to each
>>> +device section for more information on the supported commands.
>>> +
>>> +\subsubsection{Vendor specific command set}\label{sec:Basic 
>>> Facilities of a Virtio Device / Admin Virtqueues / Admin command set 
>>> / Vendor specific command set}
>>> +
>>> +The Vendor specific command set is a group of classes and commands
>>> +within each of these classes which are vendor specific. Refer to
>>> +each vendor specification for more information on the supported
>>> +commands.
>> Here's another example.
>> It's important that there is a way to make a device completely
>> generic without vendor specific expensions.
>> Would be hard to do here.
>>
>> That's a generic comment.
>>
>> but specifically I am very reluctant to add vendor specific stuff like
>> this directly in the spec. We already have VIRTIO_PCI_CAP_VENDOR_CFG
>> and if that is not sufficient I would like to know why
>> before we add more vendor specific stuff.
>>
>>
>>> diff --git a/content.tex b/content.tex
>>> index c266fd5..d350175 100644
>>> --- a/content.tex
>>> +++ b/content.tex
>>> @@ -407,6 +407,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
>>> @@ -6671,6 +6673,8 @@ \chapter{Reserved Feature 
>>> Bits}\label{sec:Reserved Feature Bits}
>>>     data to be provided in driver notification and the delivery 
>>> method is
>>>     transport specific.
>>>     For more details about driver notifications over PCI see 
>>> \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / 
>>> PCI-specific Initialization And Device Operation / Available Buffer 
>>> Notifications}.
>>> +  \item[VIRTIO_F_ADMIN_VQ(40)] This feature indicates that
>>> +  the device supports administration virtqueue negotiation.
>>>     \end{description}
>>>   --
>>> 2.21.0
>


  reply	other threads:[~2021-08-02  9:51 UTC|newest]

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-26 16:52 [RFC PATCH v2 1/2] Add virtio Admin Virtqueue specification Max Gurtovoy
2021-07-26 16:52 ` [RFC PATCH v2 2/2] virtio-blk: add support for VIRTIO_F_ADMIN_VQ Max Gurtovoy
2021-07-27 12:24   ` Stefan Hajnoczi
2021-07-27 16:08     ` [virtio-comment] " Max Gurtovoy
2021-07-28  8:25       ` Stefan Hajnoczi
2021-07-27 10:27 ` [RFC PATCH v2 1/2] Add virtio Admin Virtqueue specification Stefan Hajnoczi
2021-07-27 14:28   ` [virtio-comment] " Cornelia Huck
2021-07-27 15:29     ` Max Gurtovoy
2021-07-28  8:52       ` Stefan Hajnoczi
2021-07-28 10:59         ` Max Gurtovoy
2021-07-28 13:42           ` Stefan Hajnoczi
2021-07-28 14:20             ` Max Gurtovoy
2021-07-29  8:48               ` Stefan Hajnoczi
2021-08-01 10:46                 ` [virtio-comment] " Max Gurtovoy
2021-08-02 12:58                   ` Stefan Hajnoczi
2021-07-28 12:53       ` Michael S. Tsirkin
2021-07-30  6:45         ` [virtio-comment] " Cornelia Huck
2021-07-28 12:48 ` Michael S. Tsirkin
2021-07-29 14:51   ` Max Gurtovoy
2021-07-30  7:05     ` [virtio-comment] " Cornelia Huck
2021-07-31 11:34       ` Max Gurtovoy
2021-07-31 22:26         ` Michael S. Tsirkin
2021-07-31 22:53           ` Max Gurtovoy
2021-08-01  8:16             ` Michael S. Tsirkin
2021-08-01  8:38               ` Max Gurtovoy
2021-08-02  2:17             ` Jason Wang
2021-08-02  2:19               ` Jason Wang
2021-08-02  9:54               ` Max Gurtovoy
2021-08-02 14:51                 ` [virtio-comment] " Cornelia Huck
2021-08-02 15:27                   ` Max Gurtovoy
2021-08-02 17:28                     ` Michael S. Tsirkin
2021-08-03  3:39                     ` Jason Wang
2021-08-03  8:32                       ` Max Gurtovoy
2021-08-03  9:01                         ` Jason Wang
2021-08-03  9:21                           ` Max Gurtovoy
2021-08-03 10:04                             ` [virtio-comment] " Jason Wang
2021-07-30  7:36     ` Michael S. Tsirkin
2021-07-31 11:53       ` Max Gurtovoy
2021-07-31 22:17         ` Michael S. Tsirkin
2021-07-31 23:46           ` Max Gurtovoy
2021-08-02 13:22             ` Stefan Hajnoczi
2021-08-02 14:34               ` [virtio-comment] " Cornelia Huck
2021-08-02 14:58                 ` Max Gurtovoy
2021-08-02 16:39                   ` Stefan Hajnoczi
2021-08-02 15:21             ` [virtio-comment] " Cornelia Huck
2021-08-02 16:03               ` Max Gurtovoy
2021-08-02 17:05                 ` Michael S. Tsirkin
2021-08-03  6:28                   ` [virtio-comment] " Cornelia Huck
2021-08-03  6:41                     ` Jason Wang
2021-08-03  6:51                       ` [virtio-comment] " Cornelia Huck
2021-08-03  7:55                         ` Max Gurtovoy
2021-08-03  8:55                           ` Cornelia Huck
2021-08-03  9:04                             ` Max Gurtovoy
2021-08-02  2:25   ` Jason Wang
2021-08-02  9:51     ` Max Gurtovoy [this message]
2021-08-02 17:07     ` Michael S. Tsirkin
2021-08-03  3:22       ` Jason Wang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=29acb062-e541-3359-cbdb-b777249c222c@nvidia.com \
    --to=mgurtovoy@nvidia.com \
    --cc=aadam@redhat.com \
    --cc=amikheev@nvidia.com \
    --cc=bodong@nvidia.com \
    --cc=cohuck@redhat.com \
    --cc=eperezma@redhat.com \
    --cc=jasowang@redhat.com \
    --cc=mst@redhat.com \
    --cc=oren@nvidia.com \
    --cc=parav@nvidia.com \
    --cc=shahafs@nvidia.com \
    --cc=stefanha@redhat.com \
    --cc=virtio-comment@lists.oasis-open.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox