[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [PATCH v1 2/5] Introduce Admin Command Set
On Mon, Apr 04, 2022 at 06:35:16PM +0300, Max Gurtovoy wrote:
>
> 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 <parav@nvidia.com>
> > > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > > ---
> > > 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.
you want some motivation, this has consistently been a problem with this
project, motivation is not well documented. I say add all that can be
remotely relevant in here. The security refers to IOMMU using VF# for
protection, I think it's relevant.
> > 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.
Heh. However with admin commands a driver of device X accesses device Y.
It is not *the driver* - it's a different driver.
Given we are already getting confused, it looks like we need to differentiate
between the two types of driver by using some other term here.
I suggest "administrator" but "manager" or "management driver" could be
other options. I wonder what do others think.
> >
> >
> > > 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.
Yea, it's ok with that patch. The way it's split isn't ideal
unfortunately maybe move it into this patch in the next version.
> >
> >
> > > + /* 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.
commands could use MMIO with no DMA.
> >
> > > that describes information about the target virtio device.
> > target being what? destination?
>
> yes.
then make all wording consistent please.
>
> >
> > > +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.
I guess I do ;) Donnu what do others think.
> >
> > 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.
Look at e.g. RSS as a better example of documenting this.
>
> >
> >
> > > + 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.
I don't see why this is useful.
> >
> >
> > > + /* 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.
>
That's huge, will be a problem if it's MMIO and not DMA, and
mostly just wasted. why not make it as big as it needs to be?
> > > +};
> > > +\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.
you want to go over places like this and everywhere say who does what.
>
> >
> > > +
> > > +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
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]