From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-ID: <19121fd6-2bf5-eb60-d8bc-e930e0b0ae65@nvidia.com> Date: Mon, 4 Apr 2022 18:35:16 +0300 Subject: Re: [PATCH v1 2/5] Introduce Admin Command Set References: <20220302155608.24189-1-mgurtovoy@nvidia.com> <20220302155608.24189-3-mgurtovoy@nvidia.com> <20220404080607-mutt-send-email-mst@kernel.org> From: Max Gurtovoy In-Reply-To: <20220404080607-mutt-send-email-mst@kernel.org> MIME-Version: 1.0 Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit To: "Michael S. Tsirkin" Cc: virtio-comment@lists.oasis-open.org, cohuck@redhat.com, virtio-dev@lists.oasis-open.org, jasowang@redhat.com, parav@nvidia.com, shahafs@nvidia.com, oren@nvidia.com, stefanha@redhat.com List-ID: On 4/4/2022 3:50 PM, Michael S. Tsirkin wrote: > On Wed, Mar 02, 2022 at 05:56:05PM +0200, Max Gurtovoy wrote: >> This command set is used for essential administrative and management >> operations such as identify and resource management. >> >> Admin commands should be submitted to a well defined management >> interface. >> >> Reviewed-by: Parav Pandit >> Signed-off-by: Max Gurtovoy >> --- >> admin.tex | 129 +++++++++++++++++++++++++++++++++++++++++++++++ >> content.tex | 2 + >> introduction.tex | 2 + >> 3 files changed, 133 insertions(+) >> create mode 100644 admin.tex >> >> diff --git a/admin.tex b/admin.tex >> new file mode 100644 >> index 0000000..1e30e01 >> --- /dev/null >> +++ b/admin.tex >> @@ -0,0 +1,129 @@ >> +\section{Admin command set}\label{sec:Basic Facilities of a Virtio Device / Admin command set} >> + >> +The Admin command set defines the commands that can be issued to a well defined management >> +interface. > This is shorthand for what? Administration commands? > Let's eschew abbreviation pls. > Also, management is a bit oblique, and only the reader can judge how well it's defined :) > > > I would rephrase this along the lines of > > " For security and ease of management reasons, devices can expose an additional interface Why should we add security here ? Lets keep it simple. > to access the device besides the ones specified in the > transports section. For example, a host management system > might want to access the device while in use by driver, > or to configure the device before it is initialized by > the driver. > This access is facilitated through a set of administration commands. > > " > > > speaking of this, are we still going to use the "driver" > terminology for uses of this? There is no other way to access a device from the host/guess other than driver. A management system will have some interface provided by a driver to access a device. > > >> All the Admin commands are of the following form: >> + >> +\begin{lstlisting} >> +struct virtio_admin_cmd { >> + /* Device-readable part */ >> + le16 command; >> + /* >> + * 0 - self >> + * 1 - 65535 are reserved >> + */ >> + le16 dst_type; > I think we want to add the vdev id here in the generic command > and just have a special id that means "self". vdev id was added in later patch. No need for special id means self. Each device has only 1 id and not 2. This was proposed by Cornelia IIRC. > > >> + /* reserved for common cmd fields */ >> + u8 reserved[20]; >> + 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 >> +\end{tabular} >> + >> +The \field{command}, \field{dst_type} 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. >> + >> +The mandatory fields to be set by the driver, for all admin commands, are \field{command} and \field{dst_type}. >> + >> +The \field{command} defines the opcode for the command. The value for each command can be found in each command section. >> + >> +The \field{dst_type} defines the target virtio device for the command. This value should be set to 0 (self). >> + >> +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. >> + >> +The following table describes the Admin command set: >> + >> +\begin{tabular}{|l|l|l|} >> +\hline >> +Opcode & Command & M/O \\ >> +\hline \hline >> +0000h & VIRTIO_ADMIN_DEVICE_IDENTIFY & M \\ >> +\hline >> +0001h & VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY & O \\ >> +\hline >> +0002h - 7FFFh & Generic admin cmds & - \\ >> +\hline >> +8000h - FFFFh & Reserved & - \\ >> +\hline >> +\end{tabular} >> + >> +\subsection{VIRTIO ADMIN DEVICE IDENTIFY command}\label{sec:Basic Facilities of a Virtio Device / Admin command set / VIRTIO ADMIN DEVICE IDENTIFY command} >> + >> +The VIRTIO_ADMIN_DEVICE_IDENTIFY command has no command specific data set by the driver. >> +The \field{command} is set to VIRTIO_ADMIN_DEVICE_IDENTIFY. >> +Other common fields are reserved for this command and zeroed. >> + >> +This command upon success, returns a data buffer > let's avoid using buffer since we want to allow non-DMA uses > IIUC. just "result" is ok, right? How would device write data to host memory ? driver prepare a buffer and device will write to this buffer. > >> that describes information about the target virtio device. > target being what? destination? yes. > >> +This returned data buffer is of form: >> +\begin{lstlisting} >> +struct virtio_admin_device_identify_result { >> + /* For compatibility - indicates which of the below fields were returned > compatibility with what? I will remove "compatibility". It just indicates the valid fields. > >> + * (1 means that field was returned): >> + * Bit 0 - vdev_id >> + * Bit 1 - optional_caps_support >> + * Bits 2 - 63 - reserved for future fields >> + */ >> + le64 attrs_mask; > this seems to be some kind of thing listing supported commands, except > it's one way as opposed to negotiated like feature bits. From > experience this kind of one way capability might be problematic since > you never know what will be used. How about just using feature bits > instead? How many of these do you expect to be there? It's not features. I don't see a need for negotiation here. > > If not, please add a description of this. > E.g. along the lines of > > Field \field{attrs_mask} contains a bitmask of valid ?? as defined in ?? I described bit by bit and mentioned that this is a mask. For each bit the value of 1 is valid. > > >> + le64 vdev_id; > So this is a way to query the id of device itself? Since the only dst > type is self ... For example yes. This is one of the things you can get from this command. > > >> + /* This field indicates which of the below optional admin >> + * capabilities are supported by the device: >> + * Bit 0 - if set, VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY supported >> + * Bits 1 - 63 - reserved for future capabilities. >> + */ >> + le64 optional_caps_support; >> + u8 reserved[4072]; > What exactly is this reserved thing? Does this mean the structure is > always exactly 4k? Yes the result buffer size is 4k. >> +}; >> +\end{lstlisting} >> + >> +\subsection{VIRTIO ADMIN SUBSYSTEM IDENTIFY command}\label{sec:Basic Facilities of a Virtio Device / Admin command set / VIRTIO ADMIN SUBSYSTEM IDENTIFY command} >> + >> +The VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY command has no command specific data set by the driver. >> +The \field{command} is set to VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY. >> +Other common fields are reserved for this command and zeroed. > zeroed by driver? Yes. > >> + >> +This command upon success, returns a data buffer that describes information about the target virtio device subsystem. >> +This returned data buffer is of form: >> +\begin{lstlisting} >> +struct virtio_admin_subsystem_identify_result { >> + /* For compatibility - indicates which of the below fields were returned >> + * (1 means that field was returned): >> + * Bit 0 - vqn >> + * Bit 1 - num_supported_vdevs >> + * Bit 2 - max_vdev_id >> + * Bits 3 - 63 - reserved for future fields >> + */ >> + le64 attrs_mask; > add description here. Ok. >> + u8 vqn[16]; >> + /* Number of supported virtio devices by the subsystem */ >> + le64 num_supported_vdevs; >> + /* Maximum value of a valid vdev_id for the subsystem */ >> + le64 max_vdev_id; >> + u8 reserved[4056]; >> +}; >> +\end{lstlisting} >> diff --git a/content.tex b/content.tex >> index c6f116c..2e1df84 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 >> diff --git a/introduction.tex b/introduction.tex >> index 8e6611e..94dd7a2 100644 >> --- a/introduction.tex >> +++ b/introduction.tex >> @@ -258,5 +258,7 @@ \subsection{virtio subsystem}\label{sec:Introduction / Definitions / virtio subs >> >> The vdev_id value when combined with the VQN forms a globally unique value that identifies the virtio device. >> >> +VQN and vdev_id are exposed via Admin Command Set. >> + >> \newpage >> >> -- >> 2.21.0