Re: [virtio-comment] Re: [PATCH v5 2/7] Introduce admin command set

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

On Wed, May 18, 2022 at 05:16:13PM +0300, Max Gurtovoy wrote:
> 
> On 5/18/2022 4:50 PM, Cornelia Huck wrote:
> > On Wed, May 18 2022, Max Gurtovoy <mgurtovoy@nvidia.com> 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
> > > > > 
> > > > > +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.
> > > 
> > > 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.
> > > 
> > > > 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.
> > Why use the config space for that; just use feature bits, there are
> > enough of those, and we already have a defined protocol.
> 
> can you please propose something concrete ?
> 
> that will be scalable and will not add complexity to the feature negotiation
> mechanism we have today ?

I think you just say: "feature bits 64 to 127 are reserved for management
purposes" or something to this end. Then each command (or a group if
they are related) maps to a feature bit.

There's a cost in that the feature bits map to driver feature bits
which are writeable on device memory. But the cost scales well as
it's per group and not per device in a group.


> > 
> > > > 
> > > > > +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.
> > > 
> > > We will be sorry in the future for not doing extendable API.
> > > 
> > > I prefer keep it 128B unless there is a concrete reason for not doing so.
> > So just use a variable length structure, that should be extendable for
> > all future use cases.
> 
> I don't know how to develop compatible HW that use variable length
> structure.

There's a length field accompanying each descriptor.

> And why ? without any good reason.

To save memory space if we ever map the commands to memory: it's a
limited resource, often limited to 32 bit, but sometimes even to 16 bit.

-- 
MST