Re: [RFC PATCH v2 1/2] Add virtio Admin Virtqueue specification

[Max Gurtovoy <mgurtovoy@nvidia.com>]

On 8/2/2021 5:51 PM, Cornelia Huck wrote:
> On Mon, Aug 02 2021, Max Gurtovoy <mgurtovoy@nvidia.com> wrote:
>
> > On 8/2/2021 5:17 AM, Jason Wang wrote:
> > > å 2021/8/1 äå6:53, Max Gurtovoy åé:
> > > > On 8/1/2021 1:26 AM, Michael S. Tsirkin wrote:
> > > > > On Sat, Jul 31, 2021 at 02:34:25PM +0300, Max Gurtovoy wrote:
> > > > > > On 7/30/2021 10:05 AM, Cornelia Huck wrote:
> > > > > > > On Thu, Jul 29 2021, Max Gurtovoy <mgurtovoy@nvidia.com> wrote:
> > > > > > >
> > > > > > > > On 7/28/2021 3:48 PM, Michael S. Tsirkin wrote:
> > > > > > > > > On Mon, Jul 26, 2021 at 07:52:53PM +0300, Max Gurtovoy wrote:
> > > > > > > > > > +\subsubsection{Vendor specific command set}\label{sec:Basic
> > > > > > > > > > Facilities of a Virtio Device / Admin Virtqueues / Admin
> > > > > > > > > > command set / Vendor specific command set}
> > > > > > > > > > +
> > > > > > > > > > +The Vendor specific command set is a group of classes and
> > > > > > > > > > commands
> > > > > > > > > > +within each of these classes which are vendor specific. Refer to
> > > > > > > > > > +each vendor specification for more information on the supported
> > > > > > > > > > +commands.
> > > > > > > > > Here's another example.
> > > > > > > > > It's important that there is a way to make a device completely
> > > > > > > > > generic without vendor specific expensions.
> > > > > > > > > Would be hard to do here.
> > > > > > > > >
> > > > > > > > > That's a generic comment.
> > > > > > > > >
> > > > > > > > > but specifically I am very reluctant to add vendor specific
> > > > > > > > > stuff like
> > > > > > > > > this directly in the spec. We already have
> > > > > > > > > VIRTIO_PCI_CAP_VENDOR_CFG
> > > > > > > > > and if that is not sufficient I would like to know why
> > > > > > > > > before we add more vendor specific stuff.
> > > > > > > > We are adding an option to add vendor specific commands. We're not
> > > > > > > > adding it to the spec since each vendor will have its own manual for
> > > > > > > > that.
> > > > > > > IMHO, that way madness lies. I want to be able to look at the spec
> > > > > > > and
> > > > > > > be able to implement a compliant device or a compliant driver. If a
> > > > > > > vendor has some special feature they want to support, put it in the
> > > > > > > spec, so that it is possible to actually implement it.
> > > > > > You can implement a compliant device and a compliant. why do you
> > > > > > think you
> > > > > > can't ?
> If I want to implement a device/driver, I need a standard to refer
> to. How can something vendor-specific be standard? If it is useful, add
> it to the virtio spec.

again, Linux driver will be generic.

It will include a char dev to user space virtio-cli that will enable 
sending admin commands to do administration.

Nobody asked to add vendor specific code to the driver.

The spec allows vendor specific stuff today that are not usable and I'm 
trying to create a usable API without adding one line of vendor code to 
driver.

You just need to open a window for few commands like others do and like 
you already did in the spec.

NVIDIA HW internals are different from VENDOR_A and VENDOR_B.

I want to debug my HW or enable some accelerations in it.

This is the way to do it.

> > > > > > Some features are vendor/sub-vendor specific.
> > >
> > > Please don't do this. This breaks the efforts for making virtio as a
> > > standard and generic device.
> > no it doesn't.
> >
> > virtio already has vendor specific section.
> But only as a pci-specific capability, with a very narrow scope and
> constraints; that is a very far cry from what you're proposing!

why creating some pci vendor specific channel is ok but creating few 
admin command isn't ?

> > > > > > And as mentioned, you already added it to the spec so I guess it
> > > > > > was for a
> > > > > > reason.
> > > > > Well we basically just reserved a capability ID so if people add their
> > > > > vendor specific ones they don't conflict with each other or
> > > > > capabilities
> > > > > we'll add in the future.
> > > > it shouldn't bother you as a driver maintainer. I don't think vendor
> > > > specific code should go to Linux kernel.
> > > >
> > > > No conflict will happen.
> > > >
> > > > There is no harm in having a virtio-cli tool that will pass-through
> > > > commands from command line to the device using the admin queue.
> > > >
> > > > All the HW devices I know have sw tool that can access it and virtio
> > > > shouldn't be different.
> > > >
> > > > Also, I can develop my own vendor specific user space driver for
> > > > virtio-blk for example (lets say in SPDK framework). And this driver
> > > > will be aware of this vendor specific capabilities.
> > >
> > > That's even worse since the driver can only work for the vendor
> > > specific device which is in fact not a standard device.
> > this is not true.
> What is not true? Adding some magic vendor-specific stuff that is needed
> for the device to function does make it de facto non-standard.

This is not true because I can write a user space driver any way I want.

This discussion is repeating in cycles.

You already added vendor specific channel to the spec so why you ask me 
the need for it ?

lets progress to other sections.

> > > For the vendor specific capability, the spec has already said that it
> > > will be only used for debugging/reporting purpose:
> > >
> > > """
> > >
> > > The optional Vendor data capability allows the device to present
> > > vendor-specific data to the driver, without
> > > conflicts, for debugging and/or reporting purposes,
> > > and without conflicting with standard functionality.
> > >
> > > """
> > >
> > > It looks to me what you're trying to invent may violate the spec.
> > this is also not true.
> What is not true about that? That capability has even more restrictions
> than what is cited here.