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

["Cédric Le Goater" <clg@redhat.com>]

On 4/25/25 15:01, John Levon wrote:
> 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?
Do we know which properties are required for the vfio-user variant of the
vfio-pci device ?

I'd be tempted to start with an empty abstract vfio-pci-base device class.
This wouldn't change the current vfio-pci device much, and the vfio-user
variant would duplicate the necessary properties.


Thanks,

C.