Re: [RFC PATCH] Introduce admin virtqueue as a new transport

[Stefan Hajnoczi <stefanha@redhat.com>]

On Tue, Aug 03, 2021 at 11:20:06AM +0800, Jason Wang wrote:
> This patch introduces a new transport - the admin virtqueue. This
> transport is useful for implementing virtual devices with a limited
> transport specific resources or presenting the virtual device in a
> transport independent way.
> 
> This means, all the basic device facilities are provided solely via
> the the admin virtqueue. Additionally, the admin virtqueue is also in
> charge of the creating and destroying of the virtual device.
> 
> To be self-contained and not depend on the platform specific
> feature. Device MMU is also introduced for providing the DMA isolation
> among virtual devices.
> 
> With the help of the admin virtqueue, the presenting of the virtual
> device is done via the co-operation between the management device and
> its driver.
> 
> This is just a draft for demonstrating the basic ideas. Some possible
> enhancements:
> 
> - admin event virtqueue for reporting events like interrupts (on the
>   platform withouth MSI) and MMU translation failure
> - hardware friendly MMU translation table (e.g in the memory instead
>   of using control virtqueue commands)
> - command to kick the virtqueue
> 
> Comments are more than welcomed.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  content.tex | 639 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 639 insertions(+)
> 
> diff --git a/content.tex b/content.tex
> index 620c0e2..1f66d42 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -47,6 +47,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>  \item[DRIVER_OK (4)] Indicates that the driver is set up and ready to
>    drive the device.
>  
> +\item[DEVICE_MMU_FAIL (32)] Indicates that the device MMU has
> +  experienced an error from which it can't recover.

Will DEVICE_NEEDS_RESET be set together with this bit? The description
suggests that the device operation cannot proceed, so I guess a reset is
required.

> +
>  \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
>    an error from which it can't recover.
>  \end{description}
> @@ -515,6 +518,642 @@ \chapter{Virtio Transport Options}\label{sec:Virtio Transport Options}
>  Virtio can use various different buses, thus the standard is split
>  into virtio general and bus-specific sections.
>  
> +\section{Virtio Over Admin Virtqueue}\label{sec:Virtio Transport Options / Virtio Over Admin Virtqueue}
> +
> +Sometimes it's hard to implement the device in a transport specific
> +method. One example is that a physical device may try to present
> +multiple virtual devices with a limited transport specific
> +resources. Another example is to implement virtual devices which is

s/which is/which are/

> +transport independent. In those cases, the admin virtqueue provided by
> +the management device could be used to replace the transport specific

"management device" is being defined here but the sentence reads as if
referring to a previously-defined term ("the management device").

I suggest saying "a so-called management device" instead or defining it
more formally ("A management device is a device that acts as a container
for virtual devices. Virtual devices are controlled via the management
device's admin virtqueue." ... plus maybe something about whether
virtual devices are static or can be created/deleted dynamically).

> +method to implement the virtual device. Then the presenting of the
> +virtual device is done through the cooperation between the admin
> +virtqueue and the driver.

"the driver" == the management device's driver? (There is also the
virtual device's driver, so it can be confusing.)

> +
> +\subsection{Basic Concepts}\label{sec:Virtio Transport Options / Virtio over Admin Virtqueue / Basic Concepts}
> +
> +The device that offers the admin virtqueue (via feature
> +VIRTIO_F_ADMIN_VQ) is the management device of the virtual
> +devices. All commands are of the following form:

I wonder if the name "admin" should be replaced by "management" or
"mgmt". That way it's clear that the virtqueue is located on management
devices, not a generic admin virtqueue that non-management devices can
offer.

> +
> +\begin{lstlisting}
> +struct virtio_admin_ctrl {
> +        u64 device_id;
> +        u16 class;
> +        u16 command;
> +        u8 command-out-data[];
> +        u8 ack;
> +        u8 command-in-data[]
> +};
> +
> +/* ack values */
> +#define VIRTIO_ADMIN_OK     0
> +#define VIRTIO_ADMIN_ERR    1
> +\end{lstlisting}
> +
> +The device_id, class, command and command-out-data are set by
> +the driver, and the device sets the ack and command-in-data. 0 is used

More explicit:
s/0/The device_id value 0/

> +for identify the management device itself.
> +
> +\devicenormative{\subsubsection}{Basic Concepts}{Virtio Transport Options / Virtio Over Admin Virtqueue / Basic Concepts}
> +
> +The virtual device MUST not offer VIRTIO_F_ADMIN_VQ feature.

Is there a design reason why this isn't possible?

> +
> +\drivernormative{\subsubsection}{Basic Concepts}{Virtio Transport Options / Virtio Over Admin Virtqueue / Basic Concepts}
> +
> +The driver SHOULD negotiate VIRTIO_F_ADMIN_VQ if the device offers it.
> +
> +\subsection{Virtual Device Discovery}\label{sec:Virtio Transport Options / Virtio Over Admin Virtqueue / Virtual Device Discovery}
> +
> +The management device is discovered through a transport and device
> +specific method. Virtual devices is created and discovered via the

s/devices is created/devices are created/

> +admin virtqueue.
> +
> +\subsection{Admin Virtqueue Capabilities}\label{sec:Virtio Transport Options / Virtio Over Admin Virtqueue / Admin Virtqueue Capabilities}
> +
> +The capabilites that are supported by the admin virtqueue could be
> +fetched through the following commands:
> +
> +\begin{lstlisting}
> +#define VIRTIO_ADMIN_CTRL_CAP    0
> + #define VIRTIO_ADMIN_CTRL_CAP_GET        0
> +\end{lstlisting}
> +
> +The VIRTIO_ADMIN_CTRL_CAP_GET is used to get the capabilites that are
> +supported by the admin virtqueue through a u64 which is a bit mask of
> +the capabilies in command-in-data. There's no command-out-data.

I'm not sure what this paragraph is describing. Is VIRTIO_ADMIN_CTRL_CAP
a struct virtio_admin_ctrl::command value?

> +
> +The capabilies that is currently supported are:
> +
> +\begin{lstlisting}
> +#define VIRTIO_ADMIN_F_CAP_VDEV    1
> +\end{lstlisting}
> +
> +The VIRTIO_ADMIN_F_CAP_VDEV capability demonstrates that the virtual
> +devices is created, configured and destroyed through admin

s/devices is created/devices are created/

s/through admin/through the admin/

> +virtqueue. That means the admin virtqueue is the transport for the
> +virtual devices.

At this point 3 terms have been used: admin, management, and vdev. I
think admin is more general than management or vdev, since there is a
vdev capability and other capabilities could be added later? I think
management and vdev mean the same thing?

Does this mean there are effectively two separate concepts:
- Admin virtqueue: a generic mechanism for special commands
- Vdev: the ability for a management device to create, configure, and
  destroy virtual devices
?

Another question: I wonder why there is an admin virtqueue feature bit
instead of a new VIRTIO device ID for the management device? Does this
mean regular virtio-net, virtio-blk, etc devices can have an admin queue
in addition to their normal role?

> +
> +\devicenormative{Admin Virtqueue Capabilities}\label{sec:Virtio Transport Options / Virtio Over Admin Virtqueue / Admin Virtqueue Capabilities}
> +
> +The management device MUST support VIRTIO_ADMIN_CTRL_CAP class when

Oh, VIRTIO_ADMIN_CTRL_CAP is a class value. That wasn't clear when the
constant value was defined.

> +VIRTIO_F_ADMIN_VQ is offered.
> +
> +The management device MUST fail VIRTIO_ADMIN_CTRL_CAP class when the
> +\field{device_id} is not zero.
> +
> +\drivernormative{Admin Virtqueue Capabilities}\label{sec:Virtio Transport Options / Virtio Over Admin Virtqueue / Admin Virtqueue Capabilities}
> +
> +The driver MUST use 0 as \field{device_id} for VIRTIO_ADMIN_CTRL_CAP
> +class.
> +
> +\subsection{Device Management}\label{sec:Virtio Transport Options / Virtio Over Admin Virtqueue / Device Management}
> +
> +When the admin virtqueue offers VIRTIO_ADMIN_F_CAP_VDEV capibility,

s/offers/offers the/

s/capibility/capability/

> +virtual devices must be created and discovered through the admin
> +virtqueue.

Are management devices with statically pre-allocated virtual devices
supported? The text makes it sound like vdev creation is dynamic and
always performed by the driver.

> +\begin{lstlisting}
> +struct virtio_admin_ctrl_vdev_attribute {
> +       u32 device_id;

This name is easy to confuse with the struct virtio_admin_ctrl's
device_id. They have different meanings. Want to rename struct
virtio_admin_ctrl's field to "vdev_id"?

> +       u8 config[];

This field contains device-specific creation attributes, not the initial
contents of the Device Configuration Space? Other names like args,
params, or attrs are less likely to be confused with the Device
Configuration Space.

> +};
> +
> +#define VIRTIO_ADMIN_CTRL_VDEV    2
> + #define VIRTIO_ADMIN_CTRL_VDEV_CREATE        0
> + #define VIRTIO_ADMIN_CTRL_VDEV_DESTROY        1
> +\end{lstlisting}
> +
> +The VIRTIO_ADMIN_CTRL_VDEV_CREAT command is used to create a virtual

s/CREAT/CREATE/

> +device. The command-out-data for VIRTIO_ADMIN_CTRL_CREATE is the
> +virtio device id (\field{device_id}) and device specific configuration
> +(\field{config}) for creating the virtual device. When succeed, the
> +device returns a u64 as a unique identifier of the created virtual
> +device in command-in-data.

Is there a way to distinguish between different types of errors:
- No more resources to create another virtual device
- Invalid config value
- Unsupported device_id value
?

> +
> +The VIRTIO_ADMIN_CTRL_VDEV_DESTROY command is used to destroy a
> +virtual device which is identified by its 64bit identifier
> +\field{virtual_device_id}. There's no command-in-data for
> +VIRTIO_ADMIN_CTRL_DESTROY command.
> +
> +\devicenormative{Device Management}\label{sec:Virtio Transport Options / Virtio Over Admin Virtqueue / Device Management}
> +
> +The management device MUST fail the VIRTIO_ADMIN_CTRL_VDEV_CREATE if
> +\field{device_id} is not 0.

This is where the naming really gets confusing. This "device_id" is the
struct virtio_admin_ctrl field and not the struct
virtio_admin_ctrl_vdev_attribute "device_id" field. Please rename one of
them.

> +
> +The management device MUST fail the VIRTIO_ADMIN_CTRL_VDEV_DESTROY if
> +\field{device_id} is 0.
> +
> +All virtual devices MUST be created via admin virtqueue if the admin
> +virtqueue offers VIRTIO_F_CTRL_VDEV.

I'm not sure what the purpose of this statement is. Does it imply that
all virtual devices are destroy on device reset?

> +
> +The management device MAY map implement the virtual device in a
> +transport specific way.

I'm not sure what the purpose of this statement is.

> +
> +\drivernormative{Driver Management}\label{sec:Virtio Transport Options / Virtio Over Admin Virtqueue / Device Management}
> +
> +The management driver MUST use 0 as \field{device_id} for
> +VIRTIO_ADMIN_CTRL_VDEV_CREATE command.
> +
> +The management driver SHOULD make sure the virtual device is not used
> +by any driver before trying to destroy it.
> +
> +\subsection{Features Negotiation}\label{sec:Virtio Transport Options / Virtio Over Admin Virtqueue / Features Negotiation}
> +
> +When the admin virtqueue offers VIRTIO_ADMIN_F_CAP_VDEV capability,

s/VIRTIO_ADMIN_F_CAP_VDEV capability/the VIRTIO_ADMIN_F_CAP_VDEV capability/

> +the feature negotiation of virtual devices could be done by the
> +following commands:

What does "could" mean? IIUC this mechanism is the only way to negotiate
features because this admin queue *is* the VIRTIO Transport for the
virtual device. Therefore:
s/could be/is/

> +
> +\begin{lstlisting}
> +#define VIRTIO_ADMIN_CTRL_FEAT    3
> + #define VIRTIO_ADMIN_CTRL_FEAT_DEVICE_GET        0
> + #define VIRTIO_ADMIN_CTRL_FEAT_DRIVER_SET        1
> + #define VIRTIO_ADMIN_CTRL_FEAT_DRIVER_GET        2
> +\end{lstlisting}
> +
> +The VIRTIO_ADMIN_CTRL_FEAT_DEVICE_GET is to get the features offered
> +by a virtual device.
> +
> +The VIRTIO_ADMIN_CTRL_FEAT_DRIVER_SET is for driver to accept feature
> +bits offered by the virtual device.
> +
> +The VIRTIO_ADMIN_CTRL_FEAT_DRIVER_GET is to get the features accepted
> +by both the virtual driver and the device.
> +
> +The features is 64 bits mask of the virtio features bit. For
> +VIRTIO_ADMIN_CTRL_DRIVER_SET, the feature is passed to the device
> +through command-out-data. For VIRTIO_ADMIN_CTRL_FEAT_DEVICE_GET and
> +VIRTIO_ADMIN_CTRL_DRIVER_GET the feature is returned for the device
> +through command-in-data.
> +
> +\devicenormative{Features Negotiation}\label{sec:Virtio Transport Options / Virtio Over Admin Virtqueue / Features Negotiation}
> +
> +The management device MUST fail VIRTIO_ADMIN_F_CTRL_FEAT class for the
> +command that use 0 as its \field{virtual_device_id}.

s/virtual_device_id/device_id/ in this version of the document, but I
think virtual_device_id would be clearer :)

> +
> +\drivernormative{Features Negotiation}\label{sec:Virtio Transport Options / Virtio Over Admin Virtqueue / Features Negotiation}
> +
> +The management driver MAY mediate between the feature negotiation
> +request of the virtual devices and the admin virtqueue. E.g when
> +offering features to the virtual device, the management driver MAY
> +exclude some features in order to limit the behaviour of the virtual
> +device.
> +
> +\subsection{Device Status}\label{sec:Virtio Transport Options / Virtio Over Admin Virtqueue / Device Status}
> +
> +When the admin virtqueue offers VIRTIO_ADNIN_F_CAP_VDEV capability,

s/VIRTIO_ADNIN_F_CAP_VDEV/the VIRTIO_ADNIN_F_CAP_VDEV/

s/ADNIN/ADMIN/

> +the status of virtual device could be accessed by the following
> +commands:

s/could be/is/

> +
> +\begin{lstlisting}
> +#define VIRTIO_ADMIN_CTRL_STATUS    4
> + #define VIRTIO_ADMIN_CTRL_STATUS_SET        0
> + #define VIRTIO_ADMIN_CTRL_STATUS_GET        1
> +\end{lstlisting}
> +
> +The VIRTIO_ADMIN_CTRL_STATUS_SET is used to set the device status of
> +the virtual device here. The command-out-data is the one byte status
> +to set to the device. There's no command-in-data for this command.
> +
> +The VIRTIO_ADMIN_CTRL_STATUS_GET is used to get the device status of
> +the virtual device. The command-in-data is the one byte status
> +returned from the device. There's no command-out-data for this
> +command.
> +
> +\devicenormative{Device Status}\label{sec:Virtio Transport Options / Virtio Over Admin Virtqueue / Device Status}
> +
> +The management device MUST start the reset of a virtual device when 0
> +is written via VIRTIO_ADMIN_CTRL_STATUS_SET, the success of this
> +command demonstrate the success of the reset.
> +
> +The management device MUST present 0 through
> +VIRTIO_ADMIN_CTRL_STATUS_GET once the reset is done.
> +
> +The management device MUST fail the device status access if
> +\field{device_id} is zero.
> +
> +\drivernormative{Device Status}\label{sec:Virtio Transport Options / Virtio Over Admin Virtqueue / Device Status}
> +
> +After writing 0 via VIRTIO_ADMIN_CTRL_STATUS_SET, the driver MUST wait
> +for the success of the command before re-initializing the device.
> +
> +\subsection{Device Generation}\label{sec:Virtio Transport Options / Virtio Over Admin Virtqueue / Device Genreation}

s/Genreation/Generation/

> +
> +When the admin virtqueue offers VIRTIO_ADMIN_F_CAP_VDEV capability,
> +the device generation could be read from the following commands:
> +
> +\begin{lstlisting}
> +#define VIRTIO_ADMIN_CTRL_GENERATION    5
> + #define VIRTIO_ADMIN_CTRL_GENERATION_GET        0
> +\end{lstlisting}
> +
> +The VIRTIO_ADMIN_CTRL_GENERATION_GET is used to get the device generation
> +of the virtual device. The command-in-data is the u32 device
> +generation returned from the device. There's no command-out-data for
> +this command.

The term "device generation" is undefined. IIUC this is the
"configuration generation field", "config_generation", or "configuration
atomicity value" in the spec. Please use one of those existing terms.

I have run out of time and am pausing the review here for now.

Will drivers sleep or busy wait for admin vq command completion? I guess
it's unavoidable since other transports usually offer synchronous
operations. The driver code may not able to deschedule since it was not
necessary with the other transports.

An interesting by-product of this approach may be that the admin
virtqueue transport can enable inter-VM device emulation. It might be a
natural way to let VM A emulate devices for VM B by letting VM A handle
the admin virtqueue. Basically a variant of the virtio-vhost-user
approach I tried out a few years ago, but now we're using the admin
virtqueue instead of the vhost-user protocol for inter-VM device
emulation.

Stefan

Attachment: signature.asc
Description: PGP signature