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

[Max Gurtovoy <mgurtovoy@nvidia.com>]

On 1/18/2022 9:20 AM, Michael S. Tsirkin wrote:
> On Tue, Jan 18, 2022 at 07:14:56AM +0000, Parav Pandit wrote:
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Tuesday, January 18, 2022 12:38 PM
> > > > Subfunctions with PASID can be similarly managed by extending device
> > > identification and its MSIX/IMS vector details.
> > > > May be vf_number should be put in the union as,
> > > >
> > > > union device_id {
> > > > 	struct pci_vf vf_id; /* current */
> > > > 	struct pci_sf sf_id; /* future */
> > > > };
> > > >
> > > > So that they both can use command opcode.
> > > device id is not a good name, but yes. However this is why I think we should
> > > have a slightly more generic terminology, and more space for these IDs, and
> > > then we'd have a specific binding for VFs.
> > I couldn't think of better name for identifying a PCI VF. But yes have to think better name for sure.
> >
> > > > > I am not
> > > > > asking you to add such mechanisms straight away but the current
> > > > > proposal kind of obscures this to the point where I don't see how
> > > > > would we extend it with these things down the road.
> > > > Which part in specific make it obscure?
> > > just that the text is not generic. would be nicer if adding new types would
> > > involve only changing one or two places
> > >
> > > > New device type can be identifiable by above union.
> > > >
> > > > May be a better structure would be in patch-5 is:
> > > > Something like below,
> > > >
> > > > 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.

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 ?