[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [PATCH v8 8/9] admin: command list discovery
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. +
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]