[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [PATCH v5 2/7] Introduce admin command set
On Wed, May 18, 2022 at 04:39:54PM +0300, Max Gurtovoy wrote:
>
> On 5/15/2022 6:23 PM, Michael S. Tsirkin wrote:
> > On Wed, Apr 27, 2022 at 01:58:19AM +0300, Max Gurtovoy wrote:
> > > This command set is used for essential administrative and management
> > > operations.
> > >
> > > 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 | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > content.tex | 2 +
> > > 2 files changed, 125 insertions(+)
> > > create mode 100644 admin.tex
> > >
> > > diff --git a/admin.tex b/admin.tex
> > > new file mode 100644
> > > index 0000000..6725daa
> > > --- /dev/null
> > > +++ b/admin.tex
> > > @@ -0,0 +1,123 @@
> > > +\section{Administration command set}\label{sec:Basic Facilities of a Virtio Device / Administration command set}
> > > +
> > > +The Administration command set (also known as Admin command set) defines the commands that can be issued using a management interface.
> > > +This mechanism, for example, can be used by a system administrator that wants to configure a device before it is initialized by its driver.
> > > +
> > > +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;
> > > + /* 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 designated 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 and should be ignored by the driver.
> > > +
> > > +The following table describes the Admin command set:
> > > +
> > > +\begin{tabular}{|l|l|l|}
> > > +\hline
> > > +Opcode & Command & M/O \\
> > > +\hline \hline
> > > +0000h & VIRTIO_ADMIN_DEVICE_CAPS_IDENTIFY & M \\
> > > +\hline
> > > +0001h & VIRTIO_ADMIN_DEVICE_CAPS_ACCEPT & M \\
> > > +\hline
> > > +0002h - 7FFFh & Generic admin cmds & - \\
> > > +\hline
> > > +8000h - FFFFh & Reserved & - \\
> > > +\hline
> > > +\end{tabular}
> > > +
> > > +\subsection{VIRTIO ADMIN DEVICE CAPS IDENTIFY command}\label{sec:Basic Facilities of a Virtio Device / Admin command set / VIRTIO ADMIN DEVICE CAPS IDENTIFY command}
> > why without _ here?
>
> The pdf generator doesn't like _
>
> If someone will fix it, I'll add _
All upper case in the title is a bad idea anyway.
Please come up with a name in english for each command and
then put this readable name in the title.
> >
> > > +
> > > +The VIRTIO_ADMIN_DEVICE_CAPS_IDENTIFY command has no command specific data set by the driver.
> > > +The \field{command} is set to VIRTIO_ADMIN_DEVICE_CAPS_IDENTIFY by the driver.
> > > +
> > > +The device, upon success, returns a result that describes information about the designated virtio device.
> > result really just means a result structure right? let's say so.
>
> "The device, upon success, returns a result structure that describes
> information about the designated virtio device."
>
> is the above ok ?
>
> MST, Jason and Cornelia please ack.
ok
> > > +This result is of form:
> > > +\begin{lstlisting}
> > > +struct virtio_admin_device_caps_identify_result {
> > > + /* Indicates which of the below fields were returned
> > > + * (1 means that field was returned):
> > what does this mean "field is returned"? above restult is returned.
>
> It means: if bit i is 1, then the value/values described by bit i are valid.
>
> Is this ok ?
>
> > > + * Bit 0 - device_admin_caps
> > > + * Bits 1 - 63 - reserved for future fields
> > > + */
> > > + le64 attrs_mask;
> > > + /* This field indicates which of the below admin
> > > + * capabilities are supported by the device:
> > > + * Bits 0 - 63 - reserved for future capabilities.
> > > + */
> > > + le64 device_admin_caps;
> >
> > so all of the field is reserved?
>
> the bellow 112 bytes are reserved.
>
> the result is 128B and for this stage 112B are reserved for future
> extensions.
>
> I minimized it from 4k to 128B.
>
> Please ack.
All this aggressive padding is pointless IMHO. We need to describe how
driver and device should validate the structure being future
proof, and being ready to
handle larger structure sizes and smaller buffer sizes.
> >
> > > + u8 reserved[112];
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +\subsection{VIRTIO ADMIN DEVICE CAPS ACCEPT command}\label{sec:Basic Facilities of a Virtio Device / Admin command set / VIRTIO ADMIN DEVICE CAPS ACCEPT command}
> > > +
> > > +The VIRTIO_ADMIN_DEVICE_CAPS_ACCEPT command is used by the driver to acknowledge those admin capabilities it understands and wishes to use.
> >
> > ok so we have a protocol here, kind of like feature negotiation. Please write its description.
> > e.g. is it ok to change accepted caps? when? can device change its caps
> > etc etc etc.
>
> I don't understand what does this mean to change a cap ?
>
> Device can offer a cap and driver can accept it if it wishes to use it.
>
> That is it.
let's say driver sends multiple commands to accept. legal?
what if capabilities depend on each other? how should
driver validate? and on failure how should it react?
same questions for device ...
> I added this mechanism just for your request.
>
> I never saw a device that asks acceptance from driver but I did my best to
> fulfill your request.
And now that you started down the road you will discover it's
a lot of work just duplicating existing functionality.
which would be more usefully spent extending existing functionality,
which isn't always complete either :(
> >
> > Avoiding this kind of spec work is exactly why me and jason keep telling
> > you to consider just using features instead. Add a 64 bit admin features
> > field to the PCI transport and be done with it. CCW and MMIO already
> > have feature selector so it's trivial to add feature bits.
>
> It's not scalable for admin mechanism and I don't want to perform 100
> write/read from configuration space instead of doing all in 1 admin command.
Where would 100 read/write come from?
"not sclable" normally implies growth when something increases.
what exactly increases here? way I see it, feature bits
are per group, any number of elements in a group would not
require any more memory/accesses.
> >
> >
> > > +The \field{command} is set to VIRTIO_ADMIN_DEVICE_CAPS_ACCEPT by the driver.
> > > +
> > > +The command specific data set by the driver is of form:
> > > +\begin{lstlisting}
> > > +struct virtio_admin_device_caps_accept_data {
> > > + /* Indicates which of the below fields were set
> > > + * (1 means that field is set):
> >
> > yes we all know that 1 means set.
> >
> > do you really mean field is valid maybe?
> yes valid == set.
> >
> >
> > > + * Bit 0 - driver_admin_caps
> > > + * Bits 1 - 63 - reserved for future fields
> > > + */
> > > + le64 attrs_mask;
> > looks like going overboard. just send 64 caps bits and be done with it.
> > and rename accept_data to accept_caps.
> this is the command specific data.
> >
> > > + /* This field indicates which of the below admin
> > > + * capabilities are supported by the driver:
> > > + * Bits 0 - 63 - reserved for future capabilities.
> > > + */
> > > + le64 driver_admin_caps;
> > > + u8 reserved[112];
> >
> > I just noticed this. Please do not add this huge amount of padding
> > everywhere. instead, explain that device must be ready to accept
> > a smaller or larger buffer depending on feature bits.
>
> It's not huge. It's 128B command data.
Could be worse, yes :) But could be better.
> We will be sorry in the future for not doing extendable API.
Instead of trying to guess how much will be enough for everyone,
we need to specify how future struct size changes will be handled.
> I prefer keep it 128B unless there is a concrete reason for not doing so.
Because Jason wants an option to put this stuff in MMIO
down the road and this wastes memory space.
> >
> > > +};
> > > +\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
> > > --
> > > 2.21.0
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]