Re: [PATCH v8 8/9] admin: command list discovery

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

On Tue, Nov 22, 2022 at 04:25:23PM +0100, Cornelia Huck wrote:
> On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > Add commands to find out which commands does each group support,
> > as well as enable their use by driver.
> > This will be especially useful once we have multiple group types.
> >
> > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  admin.tex | 130 +++++++++++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 114 insertions(+), 16 deletions(-)
> >
> > diff --git a/admin.tex b/admin.tex
> > index bfa63a2..589e06a 100644
> > --- a/admin.tex
> > +++ b/admin.tex
> > @@ -72,13 +72,14 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
> >  
> >          /* Device-writable part */
> >          le16 status;
> > +        le16 status_qualifier;
> >          /* unused, reserved for future extensions */
> > -        u8 reserved2[6];
> > +        u8 reserved2[4];
> >          u8 command_specific_result[];
> >  };
> >  \end{lstlisting}
> >  
> > -For all commands, \field{opcode}, \field{group_id} and if
> > +For all commands, \field{opcode}, \field{group_type} and if
> 
> Hm, this change probably belongs to a different patch?

Yes, will move.

> >  necessary \field{group_member_id} and \field{command_specific_data} are
> >  set by the driver, and the owner device sets \field{status} and if
> >  needed \field{status_qualifier} and
> > @@ -93,18 +94,20 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
> >  
> >  \begin{tabular}{|l|l|}
> >  \hline
> > -opcode & Command Description \\
> > +opcode & Name & Command Description \\
> 
> Maybe use this table heading from the start?


Yes, will move.

> >  \hline \hline
> > -0000h - 7FFFh   & Group administration commands    \\
> > +0000h & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\
> 
> 0x0000 & VIRTIO_ADMIN_CMD_LIST_QUERY & Query the device about the list of commands supported for this group type \\
> 
> > +0001h & VIRTIO_ADMIN_CMD_LIST_USE & Provides to device list of commands used for this group type \\
> 
> 0x0001 & VIRTIO_ADMIN_CMD_LIST_USE & Inform the device about the list of commands used for this group type \\
> 
> > +0002h - 7FFFh & - &  Group administration commands    \\
> 
> 0x0002 - 0x7FFF & - & Further group administration commands \\
> 
> >  \hline
> >  8000h - FFFFh   & Reserved    \\
> 
> 0x8000 - 0xFFFF & - & Reserved \\

Using 0x instead of h suffix? Sure.

We should probably document the syntax in introduction.tex
though this seems low priority.

> >  \hline
> >  \end{tabular}
> >  
> > -The \field{group_id} specifies the device group.
> > -The \field{group_member_id} specifies the member device within the group.
> > +The \field{group_type} specifies the group type identifier.
> > +The \field{group_member_id} specifies the member identifier within the group.
> 
> This likely should go into the patch initially introducing those lines.


Yes, will move.

> >  See section \ref{sec:Introduction / Terminology / Device group}
> > -for the definition of the group identifier and group member
> > +for the definition of the group type identifier and group member
> >  identifier.
> >  
> >  The following table describes possible \field{status} values,

and this too.

> > @@ -113,22 +116,117 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
> >  
> >  \begin{tabular}{|l|l|l|}
> >  \hline
> > -Status & Name & Description \\
> > +Status (decimal) & Name & Description \\
> 
> Here as well. Although I think we can simply omit the "(decimal)" when
> we drop the "h".
> 
> >  \hline \hline
> > -00h   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\
> > +00   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\
> >  \hline
> > -01h   & VIRTIO_ADMIN_STATUS_INVALID_OPCODE    & unsupported or invalid \field{opcode}  \\
> > -\hline
> > -02h   & VIRTIO_ADMIN_STATUS_INVALID_FIELD    & invalid field within command specific data  \\
> > -\hline
> > -03h   & VIRTIO_ADMIN_STATUS_INVALID_GROUP    & invalid group identifier was set  \\
> > -\hline
> > -04h   & VIRTIO_ADMIN_STATUS_INVALID_MEM    & invalid group member identifier was set  \\
> > +22   & VIRTIO_ADMIN_STATUS_EINVAL    & invalid command \\
> 
> This also looks like it is in the wrong patch?

right.

> >  \hline
> >  other   & -    & group administration command error  \\
> >  \hline
> >  \end{tabular}
> >  
> > +When \field{status} is VIRTIO_ADMIN_STATUS_OK, \field{status_qialifier}
> 
> status_qualifier

right

> > +is reserved and set to zero by the device.
> > +
> > +When \field{status} is VIRTIO_ADMIN_STATUS_EINVAL,
> > +the following table describes possible \field{status_qialifier} values:
> > +\begin{tabular}{|l|l|l|}
> > +\hline
> > +Status & Name & Description \\
> > +\hline \hline
> > +00h   & VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND   & command error: no additional information  \\
> 
> Either 0x00, or decimal (which one is better?)

I think I prefer 0x here. And maybe I will add status values in both hex
and decimal (I used decimal to be consistent with linux headers but
fundamentally what we use most of the time is hex).

> > +\hline
> > +01h   & VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE    & unsupported or invalid \field{opcode}  \\
> > +\hline
> > +02h   & VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD    & unsupported or invalid field within \field{command_specific_data}  \\
> > +\hline
> > +03h   & VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP    & unsupported or invalid \field{group_type} was set  \\
> 
> s/was set//
> 
> > +\hline
> > +04h   & VIRTIO_ADMIN_STATUS_Q_INVALID_MEM    & unsupported or invalid \field{group_member_id} was set  \\
> 
> s/was set//
> 
> > +\hline
> > +other   & -    & group administration command error  \\
> 
> Again the question whether this is something that can be defined per
> group type.

probably - above ones are generic, let's see if we need specific ones.
if yes will be easy to add.


> > +\hline
> > +\end{tabular}
> > +
> > +Before sending any administration commands to the device, the
> > +commands in question are reported to the device as used by the driver.
> > +Initially (after reset), only two commands are assumed to be used:
> > +VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_USE.
> > +
> > +Accordingly,
> > +before sending any other commands for any member of a specific
> > +group to the device, the driver issues the commands
> > +VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_USE.
> 
> What about
> 
> "Before sending any administration commands to the device, the driver
> needs to communicate to the device which commands it is going to
> use. Initially (after reset), only two commands are assumed to be used:
> VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_USE.
> 
> Before sending any other commands for any member of a specific group to
> the device, the driver queries the supported commands via
> VIRTIO_ADMIN_CMD_LIST_QUERY and sends the commands it will use via
> VIRTIO_ADMIN_CMD_LIST_USE."
> 

Sounds good, thanks!

> > +
> > +Commands VIRTIO_ADMIN_CMD_LIST_QUERY and
> > +VIRTIO_ADMIN_CMD_LIST_USE
> > +both use the following structure describing the
> > +command opcodes:
> > +
> > +\begin{lstlisting}
> > +struct virtio_admin_cmd_list {
> > +       /* Indicates which of the below fields were returned
> > +       le32 device_admin_cmds[];
> > +};
> > +\end{lstlisting}
> > +
> > +This structure is an array of 32 bit values in little-endian byte
> > +order, in which a bit is set if the specific command opcode
> > +is supported. Thus, \field{device_admin_cmds[0]} refers to the first 32-bit value
> > +in this array corresponding to opcodes 0 to 31,
> > +\field{device_admin_cmds[1]} is the second 32-bit value
> > +corresponding to opcodes 32 to 63, etc.
> > +For example, the array of size 2 including
> > +the values 0x3 in \field{device_admin_cmds[0]}
> > +and 0x1 in \field{device_admin_cmds[1]} indicates that only opcodes 0, 1 and 32
> > +are supported.
> > +
> > +Accordingly, bits 0 and 1 corresponding to opcode 0
> > +(VIRTIO_ADMIN_CMD_LIST_QUERY) and 1
> > +(VIRTIO_ADMIN_CMD_LIST_USE) are
> > +always set in \field{device_admin_cmds[0]} returned by VIRTIO_ADMIN_CMD_LIST_QUERY.
> > +
> > +For the command VIRTIO_ADMIN_CMD_LIST_QUERY, \field{opcode} is set to 0x0.
> > +The \field{group_member_id} is unused. It is set to zero by driver.
> > +This command has no command specific data.
> > +The device, upon success, returns a result in
> > +\field{command_specific_result} in the format
> > +\field{struct virtio_admin_cmd_list} describing the
> > +list of administration commands supported for the given group.
> > +
> > +
> > +For the command VIRTIO_ADMIN_CMD_LIST_USE, \field{opcode}
> > +is set to 0x1.
> > +The \field{group_member_id} is unused. It is set to zero by driver.
> > +The \field{command_specific_data} is in the format
> > +\field{struct virtio_admin_cmd_list} describing the
> > +list of administration commands used by the driver.
> > +This command has no command specific result.
> > +
> > +The driver issues the command VIRTIO_ADMIN_CMD_LIST_QUERY to
> > +query the list of commands valid for this group and before sending
> > +any commands for any member of a group.
> > +
> > +The driver then enables use of some of the opcodes by sending to
> > +the device the command VIRTIO_ADMIN_CMD_LIST_USE with a subset
> > +of the list returned by VIRTIO_ADMIN_CMD_LIST_QUERY that is
> > +both understood and used by the driver.
> > +
> > +If the device supports the command list used by the driver, the
> > +device completes the command with status VIRTIO_ADMIN_STATUS_OK.
> > +If the device does not support the command list, the device
> > +completes the command with status
> > +VIRTIO_ADMIN_STATUS_INVALID_FIELD.
> > +
> > +Note: drivers are assumed not to set bits in device_admin_cmds
> > +if they are not familiar with how the command opcode
> > +is used, since devices could have dependencies between
> > +command opcodes.
> > +
> > +It is assumed that all members in a group support and are used
> > +with the same list of commands.
> 
> Can the driver later change its mind, i.e. use VIRTIO_ADMIN_CMD_LIST_USE
> a second time with a different set of commands? If yes, can it add
> commands, or only withdraw them?

I think it's ok to allow changing them arbitrarily at any time.
If driver wants to "lock down" the list then all it has to do
is send a list with VIRTIO_ADMIN_CMD_LIST_USE cleared.

It seemed clear along the lines of since it's not prohibited it's
allowed but since the question arose I will add a conformance clause for
this.


> What happens if a driver tries to send a command that it had not
> included in the list?


This is covered in conformance statements in the next patch.

+
+Device MUST validate commands against the list used by
+driver and MUST fail any commands not in the list with
+\field{status} set to VIRTIO_ADMIN_STATUS_EINVAL
+and \field{status_qualifier} set to
+VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE.
+