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;
> };