Re: [virtio-comment] Re: [PATCH v11 08/10] admin: command list discovery

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

On Thu, Apr 06, 2023 at 12:27:36AM -0400, Parav Pandit wrote:
> 
> On 4/3/2023 11:03 AM, Michael S. Tsirkin 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.
> > 
> > An alternative is per-type VQs. This is possible but will
> > require more per-transport work. Discovery through the vq
> > helps keep things contained.
> > 
> > e.g. lack of support for some command can switch to a legacy mode
> > 
> Above description about legacy is not clear. what is legacy mode in context
> of admin vq?
> > note that commands are expected to be avolved by adding new
> s/to be avolved/to evolve/
> 
> > fields to command specific data at the tail, so
> > we generally do not need feature bits for compatibility.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > ---
> > dropped S.O.B by Max
> > explain in commit log how commands will evolve, comment by Stefan
> > replace will use with is capable of use, comment by Stefan
> > typo fix reported by David and Lingshan
> > rename cmds to cmd_opcodes suggested by Jiri
> > list group_type explicitly in command description suggested by Jiri
> > clarify how can device not support some commands
> > always say group administration commands, comment by Jiri
> > ---
> >   admin.tex | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 101 insertions(+), 1 deletion(-)
> > 
> > diff --git a/admin.tex b/admin.tex
> > index f201bcd..9552797 100644
> > --- a/admin.tex
> > +++ b/admin.tex
> > @@ -113,7 +113,9 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
> >   \hline
> >   opcode & Name & Command Description \\
> >   \hline \hline
> > -0x0000 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
> > +0x0000 & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\
> > +0x0001 & VIRTIO_ADMIN_CMD_LIST_USE & Provides to device list of commands used for this group type \\
> > +0x0002 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
> >   \hline
> >   0x8000 - 0xFFFF & - & Reserved for future commands (possibly using a different structure)    \\
> >   \hline
> > @@ -183,6 +185,104 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
> >   depends on these structures and is described separately or is
> >   implicit in the structure description.
> > +Before sending any group 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 is
> > +capable of using via VIRTIO_ADMIN_CMD_LIST_USE.
> > +
> > +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
> > +       le64 device_admin_cmd_opcodes[];
> > +};
> > +\end{lstlisting}
> > +
> > +This structure is an array of 64 bit values in little-endian byte
> > +order, in which a bit is set if the specific command opcode
> > +is supported. Thus, \field{device_admin_cmd_opcodes[0]} refers to the
> > +first 64-bit value in this array corresponding to opcodes 0 to
> > +63, \field{device_admin_cmd_opcodes[1]} is the second 64-bit value
> > +corresponding to opcodes 64 to 127, etc.
> > +For example, the array of size 2 including
> > +the values 0x3 in \field{device_admin_cmd_opcodes[0]}
> > +and 0x1 in \field{device_admin_cmd_opcodes[1]} indicates that only
> > +opcodes 0, 1 and 64 are supported.
> > +The length of the array depends on the supported opcodes - it is
> > +large enough to include bits set for all supported opcodes,
> > +that is the length can be calculated by starting with the largest
> > +supported opcode adding one, dividing by 64 and rounding up.
> > +In other words, for
> > +VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_USE the
> > +length of \field{command_specific_result} and
> > +\field{command_specific_data} respectively will be
> > +$DIV_ROUND_UP(max_cmd, 64) * 8$ where DIV_ROUND_UP is integer division
> > +with round up and \field{max_cmd} is the largest available command opcode.
> > +
> > +The array is also allowed to be larger and to additionally
> > +include an arbitrary number of all-zero entries.
> > +
> > +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_cmd_opcodes[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 group administration commands supported for the group type
> > +specified by \field{group_type}.
> > +
> > +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 group administration commands used by the driver
> > +with the group type specified by \field{group_type}.
> > +
> > +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.
> > +
> We need to clarify that if LIST_USE command can be called only one time or
> multiple times.
> 
> For example first time for limited functionality, it enabled 10 commands
> specific to SR-IOV.
> Later when user wants to use SIOV, it enables additional SIOV specific
> command set.


clarified in a conformance statement


> > +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
> > +(for example, if the driver is not capable to use
> > +some required commands), the device
> > +completes the command with status
> > +VIRTIO_ADMIN_STATUS_INVALID_FIELD.
> > +
> > +Note: drivers are assumed not to set bits in
> > +device_admin_cmd_opcodes
> > +if they are not familiar with how the command opcode
> > +is used, since devices could have dependencies between
> > +command opcodes.
> > +
> Can you please change to use the singular notion for device and driver in
> the note like rest of the above description?
> 
> > +It is assumed that all members in a group support and are used
> > +with the same list of commands. However, for owner devices
> > +supporting multiple group types, the list of supported commands
> > +might differ between different group types.
> > +
> >   \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Administration Virtqueues}
> >   An administration virtqueue of an owner device is used to submit
> 
> This publicly archived list offers a means to provide input to the
> OASIS Virtual I/O Device (VIRTIO) TC.
> 
> In order to verify user consent to the Feedback License terms and
> to minimize spam in the list archive, subscription is required
> before posting.
> 
> Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> List help: virtio-comment-help@lists.oasis-open.org
> List archive: https://lists.oasis-open.org/archives/virtio-comment/
> Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> Committee: https://www.oasis-open.org/committees/virtio/
> Join OASIS: https://www.oasis-open.org/join/
>