Re: [PATCH 1/5] Add virtio Admin Virtqueue specification

[Max Gurtovoy <mgurtovoy@nvidia.com>]

On 1/19/2022 2:21 PM, Parav Pandit wrote:
>> From: Max Gurtovoy <mgurtovoy@nvidia.com>
>> Sent: Wednesday, January 19, 2022 5:04 PM
> [..]
>>>>>> struct virtio_admin_pci_virt_property_set {
>>>>>> 	enum virtio_device_identifier_type type; /* pci pf, pci vf,
>>>>>> subfunction
>>>>> */
>>>>>> 	union virtio_device_identifier {
>>>>>> 		struct virtio_pci_dev_id pf_vf; /* current */
>>>>>> 		struct virtio_subfunction sf; /* future */
>>>>>> 	};
>>>>>> 	enum virtio_interrupt_type interrupt_type; /* msix, ims=device
>>>>> specific, intx, something else */
>>>>>> 	union virtio_interrupt_config {
>>>>>> 		struct virtio_pci_msix_config msix_config;
>>>>>> 	};
>>>>>> };
>>>>>>
>>>>>> struct virtio_pci_interrupt_config {
>>>>>> 	le16 msix_count;
>>>>>> };
>>>>> you do not need a union straight away, Simply use something like
>>>>> this "device identifier" everywhere and then add some text
>>>>> explaining that currently it is a VF number and that admin device is a PF.
>>>> Unless we reserve some bytes, I fail to see how can it be future compatible
>> for unknown device id type for subfunction.
>>> So reserve some bytes then. 4 should be plenty.
> I am not comfortable reserving 4 bytes for sf, though it is good option and already in use in one OS for more a year now.
>
>> Ok so in V2 we'll use 4 bytes as device identifier to be generic.
>>
>> We can call it lid (local id) of vlid (virtio local id) ?
>>
>> Are we ok with one of the above names ?
>>
> I go back to rethink the structure, and don’t see a need to abstract something which is so well defined.
>
> I see need of below structures, how should it be made more abstract without breaking backward compat and without defining as TLV.

I agree, I don't see why it's not possible to use a different command 
opcode for vf interrupt configuration and sf interrupt configuration.

We're not short in opcodes and it's very elegant and extendable IMO.

I think the order should be:

1. add adminq to virtio spec with one simple example (say MSIX config 
for VFs)

2. in parallel submission for admin commands: S-IOV support, num VQs 
config, feature bits config and more...

The below example emphasizes that the adminq protocol is flexible and 
easily extendable.

>
> struct virtio_admin_pci_vf_interrupt_config {
> 	/* v1 current */
> 	le64 property_mask; /* bit 0 valid */
> 	le16 vf_number;
> 	le16 msix_count;
> };
>
> struct virtio_admin_pci_vf_interrupt_config {
> 	/* v2 near future, backward compatible */
> 	le64 property_mask; /* bit 0, 1 valid */
> 	le16 vf_number;
> 	le16 msix_count;
> 	le16 ims_count;
> };
>
> struct virtio_admin_pci_sf_interrupt_config {
> 	/* v3 future, new struct, no need of backward compat */
> 	le64 property_mask; /* bit 0,1,2, valid */
> 	le32 sf_number;
> 	/* is 4 bytes enough to describe sf,
> 	 * what if community decides uuid to identify sf?
> 	 * How about we take out device identifier outside of this struct?
> 	 */
> 	le16 msix_count;
> 	le16 ims_count;
> 	le16 pci_caps; /* pci atomics enable */
> };
>
> virtio_unknown_transport_interrupt_config {
> 	/* vX future */
> 	le64 property_mask;
> 	<unknown len> device identifier;
> 	le16 msix_count;
> 	le16 ims_count;
> };