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

[Cornelia Huck <cohuck@redhat.com>]

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.

>>>>>
>>>>> 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!

>
>
>>
>>>>>
>>>>> 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.

>
>
>>
>> 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.