Re: [PATCH v1 3/5] Introduce DEVICE INFO Admin command

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

On Mon, Apr 04, 2022 at 06:44:11PM +0300, Max Gurtovoy wrote:
> 
> On 4/4/2022 3:57 PM, Michael S. Tsirkin wrote:
> > On Wed, Mar 02, 2022 at 05:56:06PM +0200, Max Gurtovoy wrote:
> > > The DEVICE INFO command will return a basic information for a virtio
> > > device.
> > > 
> > > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > > ---
> > >   admin.tex | 35 +++++++++++++++++++++++++++++++++--
> > >   1 file changed, 33 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/admin.tex b/admin.tex
> > > index 1e30e01..9a8969b 100644
> > > --- a/admin.tex
> > > +++ b/admin.tex
> > > @@ -67,7 +67,9 @@ \section{Admin command set}\label{sec:Basic Facilities of a Virtio Device / Admi
> > >   \hline
> > >   0001h   & VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY    & O  \\
> > >   \hline
> > > -0002h - 7FFFh   & Generic admin cmds    & -  \\
> > > +0002h   & VIRTIO_ADMIN_DEVICE_INFO    & O  \\
> > > +\hline
> > > +0003h - 7FFFh   & Generic admin cmds    & -  \\
> > >   \hline
> > >   8000h - FFFFh   & Reserved    & - \\
> > >   \hline
> > > @@ -94,7 +96,8 @@ \subsection{VIRTIO ADMIN DEVICE IDENTIFY command}\label{sec:Basic Facilities of
> > >          /* 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.
> > > +        * Bit 1 - if set, VIRTIO_ADMIN_DEVICE_INFO supported
> > > +        * Bits 2 - 63 - reserved for future capabilities.
> > >           */
> > >          le64 optional_caps_support;
> > >          u8 reserved[4072];
> > > @@ -127,3 +130,31 @@ \subsection{VIRTIO ADMIN SUBSYSTEM IDENTIFY command}\label{sec:Basic Facilities
> > >          u8 reserved[4056];
> > >   };
> > >   \end{lstlisting}
> > > +
> > > +\subsection{VIRTIO ADMIN DEVICE INFO command}\label{sec:Basic Facilities of a Virtio Device / Admin command set / VIRTIO ADMIN DEVICE INFO command}
> > > +
> > > +The VIRTIO_ADMIN_DEVICE_INFO command has no command specific data set by the driver.
> > > +The \field{command} is set to VIRTIO_ADMIN_DEVICE_INFO.
> > > +
> > > +The VIRTIO_ADMIN_DEVICE_INFO upon success, returns a data buffer that describes a basic information for the target virtio device.
> > > +Upon success, the returned data buffer is of form:
> > > +\begin{lstlisting}
> > > +struct virtio_admin_device_info_result {
> > > +        /* For compatibility - indicates which of the below fields were returned
> > > +         * (1 means that field was returned):
> > So how is this supposed to be handled. You need to write this up.
> > Also having each command have a bitmap seems like an overkill
> > to me. But if that is the way you are going just put
> > this in the generic part.
> > 
> > 
> > > +         * Bit 0 - vf_number
> > > +         * Bits 1 - 63 - reserved for future fields
> > we generally say "future use" not "future fields".
> 
> ok.
> 
> 
> > > +         */
> > > +        le64 attrs_mask;
> > > +        /* The virtual function number */
> > > +        le16 vf_number;
> > > +        u8 reserved[1014];
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +\begin{note}
> > > +{Bit 0 in \field{attrs_mask} will be set only if the target virtio device represents a PCI Virtual function.
> > > +In this case, the \field{vf_number} value can't be greater than TotalVFs value as defined in the PCI
> > > +specification and can't be equal to zero. If bit 0 is not set, the driver will ignore \field{vf_number}.}
> > > +\end{note}
> > 
> > I personally would just make vf # *be* the device ID, with no need for an
> > extra indirection. It doesn't hurt much though ...
> 
> This is implementation specific.
> 
> You need to have a way to identify the vf number.

So my point is, I see no need to have a random set
of VFs or have an abstract ID as an indirection layer -
we can just say id == VF# and always manage all VF #s up to N.

This will cut out a bunch of commands.

And hey, if we see the need for an indirection down the road we
will be add it as a separate dst_type.

> > 
> > But I do not seem to understand how it all works here. I think a generic
> > description explaining how all this is used is necessary, pls add a
> > writeup.
> > 
> > E.g.
> > 
> > So I start with a device and through a feature bit I find out it
> > supports admin commands. Then I query that and find the max vdev id.
> > Query the ID of each device. Then through that query VF number.
> > Something like this.
> > 
> > Looks in device operation section for some examples.
> 
> Ok I'll try to add more explanation.
> 
> 
> > 
> > 
> > > +
> > > -- 
> > > 2.21.0