Re: [PATCH v6 2/5] Introduce admin command set

["Michael S. Tsirkin" <mst@redhat.com>]

On Sun, Jul 31, 2022 at 06:43:51PM +0300, Max Gurtovoy wrote:
> This command set is used for essential administrative and management
> operations.
> 
> Admin commands should be submitted to a well defined management
> interface.
> 
> Reviewed-by: Parav Pandit <parav@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>

Looks good. Minor comments.

> ---
>  admin.tex   | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  content.tex |  2 ++
>  2 files changed, 84 insertions(+)
>  create mode 100644 admin.tex
> 
> diff --git a/admin.tex b/admin.tex
> new file mode 100644
> index 0000000..d14f8f7
> --- /dev/null
> +++ b/admin.tex
> @@ -0,0 +1,82 @@
> +\section{Administration command set}\label{sec:Basic Facilities of a Virtio Device / Administration command set}
> +
> +The Administration command set (also known as Admin command set) defines the commands that can be issued using a management interface.

this is the only place where we talk about a management interface

A better definition maybe:

commands that can be used to control one device in a group through
another device in a group?


> +This mechanism, for example, can be used by a system administrator that wants to configure a device before it is initialized by its driver.

Let's just avoid mentioning system administrator here since that's easy
to confuse with admin commands.

	For example, this mechanism can be used to configure a device before it is initialized by its driver.


> +All the Admin commands are of the following form:
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd {
> +        /* Device-readable part */
> +        le16 command;
> +        /*
> +         * 0 - Self
> +         * 1 - SR-IOV
> +         * 2 - 65535 are reserved
> +         */
> +        le16 group_id;

we need 32 bits of padding to align the next field naturally.
Stick them where you see fit. 16 after command and 16 here?

> +        le64 group_member_id;
> +        /* reserved for common cmd fields */
> +        u8 reserved[12];
> +        u8 command_specific_data[];
> +
> +        /* Device-writable part */
> +        u8 status;
> +        u8 command_specific_error;
> +        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
> +03h   & VIRTIO_ADMIN_STATUS_INVALID_FIELD    & invalid field was set  \\
> +\hline
> +04h   & VIRTIO_ADMIN_STATUS_INVALID_GROUP    & invalid group identifier was set  \\
> +\hline
> +05h   & VIRTIO_ADMIN_STATUS_INVALID_MEM    & invalid group member identifier was set  \\
> +\hline
> +\end{tabular}
> +
> +The \field{command}, \field{group_id}, \field{group_member_id} 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.
> +
> +Reserved common fields are ignored by the device and to be zeroed by the driver.

Add this to conformance statement maybe.

> +
> +The mandatory fields to be set by the driver, for all admin commands, are the \field{command}, \field{group_id} and \field{group_member_id}.
> +
> +The optional unused fields to be zeroed by the driver.

A bit confusing - these are the only fields I see. Is this still
relevant?

> +The \field{command} defines the opcode for the command. The value for each command can be found in each command section.
> +
> +The \field{group_id} defines the designated device group for the command.

> +The \field{group_member_id} defines the designated device within the device group for the command.
> +See section \ref{sec:Introduction / Terminology / Device group} for group identifier and group member identifier numbering for various device groups.
> +


Hmm not I think it's not precise.

the command is sent by driver to specific a device.

this device (should we come up with a name for it? maybe
the administrative device?) together with the group type id is what defines the device
group. For example, for the SRIOV type the commands must be
sent to the PF and refer to the group including the PF
through which the commands are sent and its VFs.


For each group that supports specific commands, there is
one or more devices through which the commands can be given?



> +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 and should be ignored by the driver.
> +
> +The following table describes the Admin command set:
> +
> +\begin{tabular}{|l|l|}
> +\hline
> +Opcode & Command \\
> +\hline \hline
> +0000h - 7FFFh   & Generic admin cmds    \\
> +\hline
> +8000h - FFFFh   & Reserved    \\
> +\hline
> +\end{tabular}
> diff --git a/content.tex b/content.tex
> index 7508dd1..caab298 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -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.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
> -- 
> 2.21.0