[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [RFC PATCH v2 1/2] Add virtio Admin Virtqueue specification
On Wed, Jul 28, 2021 at 01:59:26PM +0300, Max Gurtovoy wrote: > > On 7/28/2021 11:52 AM, Stefan Hajnoczi wrote: > > On Tue, Jul 27, 2021 at 06:29:49PM +0300, Max Gurtovoy wrote: > > > On 7/27/2021 5:28 PM, Cornelia Huck wrote: > > > > On Tue, Jul 27 2021, Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > > > > > > > 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> > > > > > > --- > > > > > > 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} > > > > This is very complex, and it feels like we're overengineering this. > > > Do you mean the status type and the status ? > > > > > > > > > + > > > > > > +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 > > > > > "advertises the VIRTIO_F_ADMIN_VQ feature" > > > > > > > > > > > +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} > > > > > > + > > > > > > +\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 \\ > > > > > These fields are not defined. I wonder why they are necessary - the > > > > > driver should already have this information. > > > > Agreed. > > > These are initial fields. > > > > > > We can add also model, serial_number and more in the future. > > > > > > > > > > > In general, I'm a little concerned that this whole infrastructure will > > > > > increase the complexity of VIRTIO significantly with little benefit. I > > > > > do think an admin virtqueue makes sense, e.g. for migration, but would > > > > > prefer it if we focus on actual commands first instead of > > > > > infrastructure. That way it will be clear what infrastructure is needed. > > > admin virtq is not only for migration. > > > > > > You'll be able to configure virtio device properties using user space tools > > > like: virtio-cli. > > > > > > For example: format a block device, manage virtual function resources using > > > its PF, query for error logs, device health and more. > > That sounds good. > > > > > In the SW world maybe all the above were redundant, but now that you have > > > more and more HW virtio devices the protocol should be more flexible and > > > adjust. > > HW is not special in this regard, I think this will be useful for > > software too. In-band admin commands are necessary for nested > > virtualization, for example. They also provide a standard admin > > interface for out-of-process devices (vhost-user, etc). > > > > > Few weeks ago I've sent a concrete commands for live migration but then I > > > was told that new infrastructure (admin virtq) should be developed and this > > > is what I did in this RFC. > > > > > > if you combine the 2 RFCs you can imagine what is needed here for adding > > > Live migration support. > > > > > > But I want to add it step by step. > > > > > > We need to agree on the infrastructure. > > > > > > > A concrete example would be good, but I think we can come up with a > > > > bare-bones spec to start with. > > > > > > > > - feature bit for the admin vq, as defined here > > > > - location of the admin vq is device specific > > > > - I think we can get away with two classes, as for feature bits (not > > > > device specificic and device specific); I don't think we need separate > > > > classes for transport or vendor specific > > > We need it for live migration probably. It will be a transport class. > > > > > > Vendor specific is also important to allow vendors develop their special > > > souse. > > > > > > > - make the format for the request simple (command + length + payload?) > > > I used almost the same format as virtio net ctrl queue. > > The virtio_net_ctrl packet format looks good to me, it's close to what > > Cornelia's command + length + payload suggestion: > > I guess I didn't understand Cornelia suggestion. > > > > > > struct virtio_net_ctrl { > > u8 class; > > u8 command; > > u8 command-specific-data[]; > > u8 ack; > > }; > > /* ack values */ > > #define VIRTIO_NET_OK 0 > > #define VIRTIO_NET_ERR 1 > > > > I'm not sure how vendor commands will be allocated though. Will each > > vendor get a unique class id to prevent collisions? If we want to > > support cross-implementation migration then it may be necessary to allow > > vendor command availability to change while the device is running. > > vendor specific commands can collide. > > Vendor A can implement class 192 to do X and Vendor B can implement class > 192 to do Y. > > what do you mean "support cross-implementation migration" ? Migrating from vhost_net to vDPA virtio-net, for example. Or migrating between two different vDPA virtio-net implementations. If vendor commands are all in a single namespace then the guest cannot use them without the risk of the command accidentally executing on the migration destination (where it has a different effect because the vendor has changed!). > > I prefer the simpler struct virtio_net_ctrl format to the more > > complicated one proposed in this patch series. > > This is the same besides adding status type > > u8 status_type : 4; > u8 reserved : 4; I'm not sure why it's needed. > I split "u8 command-specific-data[];" > to > "u8 command-specific-data[]; > u8 command-specific-result[];" > > to emphasize that there is some data that can be written by the device and some data written by the driver in the same command. > And this is also the case in virtio-net-ctrl, right ? The split makes sense to me. > > > > How many different (groups of) commands can we reasonably expect? Do we > > > > need a generic discovery command, or can we get away with a feature bit > > > > covering each new group of commands? > > > I can't predict the future but IMO we need a discovery command. > > > > > > We have many devices and more can be added in the future. > > A <u8 class, u8 command> space is 65536 bits or 8KB. I think admin > > commands would not be included in VIRTIO Feature Bits but instead > > reported via a separate admin command that returns up to 8KB of data: > > > > struct virtio_admin_report_cmds { > > /* Bitmap of available admin commands [Device->Driver] > > * bool command_present = > > * command_bits[class * 32 + command / 8] & (command % 8); > > */ > > u8 command_bits[8192]; > > }; > > Yes, I divided it to multiple commands per class to cover the case we will > need more than 1 bit to describe a command. > > But I guess we can add it later on. > > I think the above should be: > > bool command_present = command_bits[class * 32 + command / 8] & (1 << (command % 8)); > > isn't it ? You're right. I forgot to shift the bit :D. > Also what do you think about renaming <class, command> to <opcode, opmod> ? I need to understand how opcode and opmod values are used. I'm not sure. Stefan
Attachment:
signature.asc
Description: PGP signature
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]