Re: [PATCH 08/14] vfio: add vfio-pci-base class

[John Levon <john.levon@nutanix.com>]

On Fri, Apr 25, 2025 at 02:46:48PM +0200, Cédric Le Goater wrote:

> After this patch, here is what we have for the base class :
>     static const Property vfio_pci_base_dev_properties[] = {
>         DEFINE_PROP_ON_OFF_AUTO("x-pre-copy-dirty-page-tracking", VFIOPCIDevice,
>                                 vbasedev.pre_copy_dirty_page_tracking,
>                                 ON_OFF_AUTO_ON),
>         DEFINE_PROP_ON_OFF_AUTO("x-device-dirty-page-tracking", VFIOPCIDevice,
>                                 vbasedev.device_dirty_page_tracking,
>                                 ON_OFF_AUTO_ON),
>         DEFINE_PROP_UINT32("x-intx-mmap-timeout-ms", VFIOPCIDevice,
>                            intx.mmap_timeout, 1100),
>         DEFINE_PROP_ON_OFF_AUTO("enable-migration", VFIOPCIDevice,
>                                 vbasedev.enable_migration, ON_OFF_AUTO_AUTO),
>         DEFINE_PROP("x-migration-multifd-transfer", VFIOPCIDevice,
>                     vbasedev.migration_multifd_transfer,
>                     vfio_pci_migration_multifd_transfer_prop, OnOffAuto,
>                     .set_default = true, .defval.i = ON_OFF_AUTO_AUTO),
>         DEFINE_PROP_BOOL("migration-events", VFIOPCIDevice,
>                          vbasedev.migration_events, false),
>         DEFINE_PROP_BOOL("x-no-mmap", VFIOPCIDevice, vbasedev.no_mmap, false),
>         DEFINE_PROP_BOOL("x-balloon-allowed", VFIOPCIDevice,
>                          vbasedev.ram_block_discard_allowed, false),
>         DEFINE_PROP_BOOL("x-no-kvm-intx", VFIOPCIDevice, no_kvm_intx, false),
>         DEFINE_PROP_BOOL("x-no-kvm-msi", VFIOPCIDevice, no_kvm_msi, false),
>         DEFINE_PROP_BOOL("x-no-kvm-msix", VFIOPCIDevice, no_kvm_msix, false),
>         DEFINE_PROP_BOOL("x-no-kvm-ioeventfd", VFIOPCIDevice, no_kvm_ioeventfd,
>                          false),
>         DEFINE_PROP_BOOL("x-no-vfio-ioeventfd", VFIOPCIDevice, no_vfio_ioeventfd,
>                          false),
>         DEFINE_PROP_UINT32("x-pci-vendor-id", VFIOPCIDevice, vendor_id, PCI_ANY_ID),
>         DEFINE_PROP_UINT32("x-pci-device-id", VFIOPCIDevice, device_id, PCI_ANY_ID),
>         DEFINE_PROP_UINT32("x-pci-sub-vendor-id", VFIOPCIDevice,
>                            sub_vendor_id, PCI_ANY_ID),
>         DEFINE_PROP_UINT32("x-pci-sub-device-id", VFIOPCIDevice,
>                            sub_device_id, PCI_ANY_ID),
>         DEFINE_PROP_OFF_AUTO_PCIBAR("x-msix-relocation", VFIOPCIDevice, msix_relo,
>                                     OFF_AUTO_PCIBAR_OFF),
>     };
> and for vfio-pci :
>     static const Property vfio_pci_dev_properties[] = {
>         DEFINE_PROP_PCI_HOST_DEVADDR("host", VFIOPCIDevice, host),
>         DEFINE_PROP_UUID_NODEFAULT("vf-token", VFIOPCIDevice, vf_token),
>         DEFINE_PROP_STRING("sysfsdev", VFIOPCIDevice, vbasedev.sysfsdev),
>         DEFINE_PROP_ON_OFF_AUTO("display", VFIOPCIDevice,
>                                 display, ON_OFF_AUTO_OFF),
>         DEFINE_PROP_UINT32("xres", VFIOPCIDevice, display_xres, 0),
>         DEFINE_PROP_UINT32("yres", VFIOPCIDevice, display_yres, 0),
>         DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features,
>                         VFIO_FEATURE_ENABLE_VGA_BIT, false),
>         DEFINE_PROP_BIT("x-req", VFIOPCIDevice, features,
>                         VFIO_FEATURE_ENABLE_REQ_BIT, true),
>         DEFINE_PROP_BIT("x-igd-opregion", VFIOPCIDevice, features,
>                         VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT, false),
>         DEFINE_PROP_UINT32("x-igd-gms", VFIOPCIDevice, igd_gms, 0),
>         DEFINE_PROP_BIT("x-igd-lpc", VFIOPCIDevice, features,
>                         VFIO_FEATURE_ENABLE_IGD_LPC_BIT, false),
>         DEFINE_PROP_ON_OFF_AUTO("x-igd-legacy-mode", VFIOPCIDevice,
>                                 igd_legacy_mode, ON_OFF_AUTO_AUTO),
>         DEFINE_PROP_BOOL("x-no-geforce-quirks", VFIOPCIDevice,
>                          no_geforce_quirks, false),
>         DEFINE_PROP_UNSIGNED_NODEFAULT("x-nv-gpudirect-clique", VFIOPCIDevice,
>                                        nv_gpudirect_clique,
>                                        qdev_prop_nv_gpudirect_clique, uint8_t),
>     #ifdef CONFIG_IOMMUFD
>         DEFINE_PROP_LINK("iommufd", VFIOPCIDevice, vbasedev.iommufd,
>                          TYPE_IOMMUFD_BACKEND, IOMMUFDBackend *),
>     #endif
>         DEFINE_PROP_BOOL("skip-vsc-check", VFIOPCIDevice, skip_vsc_check, true),
>     };
> Graphic property and host device definitions are excluded from the
> base class it seems. This might fit vfio-user needs but it looks
> like a quick hack from the vfio-pci side. It needs more work.

Can you suggest a preferred approach? I'm definitely not wedded to the current
way (after all, I didn't write it !), but I'm not sure how else we could do it.

Perhaps if there's some way to deregister properties when vfio-user
instantiates?

> > > Can you remind me why the vfio-pci class for vfio-user can not
> > > inherit directly from vfio-pci ?
> > 
> > For the above reason: we'd inherit many properties that don't work for
> > vfio-user.
> 
> What do you mean by "don't work" ? functionally irrelevant ?

I don't know the answer to that in general. Certainly some are just irrelevant
(like sysfsdev), but it's entirely possible the other stuff actively breaks.
Presumably you agree it's not good to introduce potential footguns for users
here?

regards
john