Re: [RFC v3 02/19] vfio-user: add VFIO base abstract class

[Alex Williamson]

On Mon,  8 Nov 2021 16:46:30 -0800
John Johnson <john.g.johnson@oracle.com> wrote:

> Add an abstract base class both the kernel driver
> and user socket implementations can use to share code.
> 
> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> ---
>  hw/vfio/pci.h |  16 +++++++--
>  hw/vfio/pci.c | 112 
> +++++++++++++++++++++++++++++++++++-----------------------
>  2 files changed, 81 insertions(+), 47 deletions(-)
> 
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index 6477751..bbc78aa 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -114,8 +114,13 @@ typedef struct VFIOMSIXInfo {
>      unsigned long *pending;
>  } VFIOMSIXInfo;
>  
> -#define TYPE_VFIO_PCI "vfio-pci"
> -OBJECT_DECLARE_SIMPLE_TYPE(VFIOPCIDevice, VFIO_PCI)
> +/*
> + * TYPE_VFIO_PCI_BASE is an abstract type used to share code
> + * between VFIO implementations that use a kernel driver
> + * with those that use user sockets.
> + */
> +#define TYPE_VFIO_PCI_BASE "vfio-pci-base"
> +OBJECT_DECLARE_SIMPLE_TYPE(VFIOPCIDevice, VFIO_PCI_BASE)
>  
>  struct VFIOPCIDevice {
>      PCIDevice pdev;
> @@ -175,6 +180,13 @@ struct VFIOPCIDevice {
>      Notifier irqchip_change_notifier;
>  };
>  
> +#define TYPE_VFIO_PCI "vfio-pci"
> +OBJECT_DECLARE_SIMPLE_TYPE(VFIOKernPCIDevice, VFIO_PCI)
> +
> +struct VFIOKernPCIDevice {
> +    VFIOPCIDevice device;
> +};
> +
>  /* Use uin32_t for vendor & device so PCI_ANY_ID expands and cannot match hw 
> */
>  static inline bool vfio_pci_is(VFIOPCIDevice *vdev, uint32_t vendor, 
> uint32_t device)
>  {
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index e1ea1d8..122edf8 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -231,7 +231,7 @@ static void vfio_intx_update(VFIOPCIDevice *vdev, 
> PCIINTxRoute *route)
>  
>  static void vfio_intx_routing_notifier(PCIDevice *pdev)
>  {
> -    VFIOPCIDevice *vdev = VFIO_PCI(pdev);
> +    VFIOPCIDevice *vdev = VFIO_PCI_BASE(pdev);
>      PCIINTxRoute route;
>  
>      if (vdev->interrupt != VFIO_INT_INTx) {
> @@ -457,7 +457,7 @@ static void vfio_update_kvm_msi_virq(VFIOMSIVector 
> *vector, MSIMessage msg,
>  static int vfio_msix_vector_do_use(PCIDevice *pdev, unsigned int nr,
>                                     MSIMessage *msg, IOHandler *handler)
>  {
> -    VFIOPCIDevice *vdev = VFIO_PCI(pdev);
> +    VFIOPCIDevice *vdev = VFIO_PCI_BASE(pdev);
>      VFIOMSIVector *vector;
>      int ret;
>  
> @@ -542,7 +542,7 @@ static int vfio_msix_vector_use(PCIDevice *pdev,
>  
>  static void vfio_msix_vector_release(PCIDevice *pdev, unsigned int nr)
>  {
> -    VFIOPCIDevice *vdev = VFIO_PCI(pdev);
> +    VFIOPCIDevice *vdev = VFIO_PCI_BASE(pdev);
>      VFIOMSIVector *vector = &vdev->msi_vectors[nr];
>  
>      trace_vfio_msix_vector_release(vdev->vbasedev.name, nr);
> @@ -1063,7 +1063,7 @@ static const MemoryRegionOps vfio_vga_ops = {
>   */
>  static void vfio_sub_page_bar_update_mapping(PCIDevice *pdev, int bar)
>  {
> -    VFIOPCIDevice *vdev = VFIO_PCI(pdev);
> +    VFIOPCIDevice *vdev = VFIO_PCI_BASE(pdev);
>      VFIORegion *region = &vdev->bars[bar].region;
>      MemoryRegion *mmap_mr, *region_mr, *base_mr;
>      PCIIORegion *r;
> @@ -1109,7 +1109,7 @@ static void vfio_sub_page_bar_update_mapping(PCIDevice 
> *pdev, int bar)
>   */
>  uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len)
>  {
> -    VFIOPCIDevice *vdev = VFIO_PCI(pdev);
> +    VFIOPCIDevice *vdev = VFIO_PCI_BASE(pdev);
>      uint32_t emu_bits = 0, emu_val = 0, phys_val = 0, val;
>  
>      memcpy(&emu_bits, vdev->emulated_config_bits + addr, len);
> @@ -1142,7 +1142,7 @@ uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t 
> addr, int len)
>  void vfio_pci_write_config(PCIDevice *pdev,
>                             uint32_t addr, uint32_t val, int len)
>  {
> -    VFIOPCIDevice *vdev = VFIO_PCI(pdev);
> +    VFIOPCIDevice *vdev = VFIO_PCI_BASE(pdev);
>      uint32_t val_le = cpu_to_le32(val);
>  
>      trace_vfio_pci_write_config(vdev->vbasedev.name, addr, val, len);
> @@ -2782,7 +2782,7 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice 
> *vdev)
>  
>  static void vfio_realize(PCIDevice *pdev, Error **errp)
>  {
> -    VFIOPCIDevice *vdev = VFIO_PCI(pdev);
> +    VFIOPCIDevice *vdev = VFIO_PCI_BASE(pdev);
>      VFIODevice *vbasedev_iter;
>      VFIOGroup *group;
>      char *tmp, *subsys, group_path[PATH_MAX], *group_name;
> @@ -3105,7 +3105,7 @@ error:
>  
>  static void vfio_instance_finalize(Object *obj)
>  {
> -    VFIOPCIDevice *vdev = VFIO_PCI(obj);
> +    VFIOPCIDevice *vdev = VFIO_PCI_BASE(obj);
>      VFIOGroup *group = vdev->vbasedev.group;
>  
>      vfio_display_finalize(vdev);
> @@ -3125,7 +3125,7 @@ static void vfio_instance_finalize(Object *obj)
>  
>  static void vfio_exitfn(PCIDevice *pdev)
>  {
> -    VFIOPCIDevice *vdev = VFIO_PCI(pdev);
> +    VFIOPCIDevice *vdev = VFIO_PCI_BASE(pdev);
>  
>      vfio_unregister_req_notifier(vdev);
>      vfio_unregister_err_notifier(vdev);
> @@ -3144,7 +3144,7 @@ static void vfio_exitfn(PCIDevice *pdev)
>  
>  static void vfio_pci_reset(DeviceState *dev)
>  {
> -    VFIOPCIDevice *vdev = VFIO_PCI(dev);
> +    VFIOPCIDevice *vdev = VFIO_PCI_BASE(dev);
>  
>      trace_vfio_pci_reset(vdev->vbasedev.name);
>  
> @@ -3184,7 +3184,7 @@ post_reset:
>  static void vfio_instance_init(Object *obj)
>  {
>      PCIDevice *pci_dev = PCI_DEVICE(obj);
> -    VFIOPCIDevice *vdev = VFIO_PCI(obj);
> +    VFIOPCIDevice *vdev = VFIO_PCI_BASE(obj);
>  
>      device_add_bootindex_property(obj, &vdev->bootindex,
>                                    "bootindex", NULL,
> @@ -3201,38 +3201,75 @@ static void vfio_instance_init(Object *obj)
>      pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
>  }
>  
> -static Property vfio_pci_dev_properties[] = {
> -    DEFINE_PROP_PCI_HOST_DEVADDR("host", VFIOPCIDevice, host),
> -    DEFINE_PROP_STRING("sysfsdev", VFIOPCIDevice, vbasedev.sysfsdev),
> +static 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_UINT32("x-intx-mmap-timeout-ms", VFIOPCIDevice,
> +                       intx.mmap_timeout, 1100),
> +    DEFINE_PROP_BOOL("x-enable-migration", VFIOPCIDevice,
> +                     vbasedev.enable_migration, false),
> +    DEFINE_PROP_BOOL("x-no-mmap", VFIOPCIDevice, vbasedev.no_mmap, 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_OFF_AUTO_PCIBAR("x-msix-relocation", VFIOPCIDevice, 
> msix_relo,
> +                                OFF_AUTOPCIBAR_OFF),
> +    /*
> +     * TODO - support passed fds... is this necessary?
> +     * DEFINE_PROP_STRING("vfiofd", VFIOPCIDevice, vfiofd_name),
> +     * DEFINE_PROP_STRING("vfiogroupfd, VFIOPCIDevice, vfiogroupfd_name),
> +     */
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void vfio_pci_base_dev_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    PCIDeviceClass *pdc = PCI_DEVICE_CLASS(klass);
> +
> +    device_class_set_props(dc, vfio_pci_base_dev_properties);
> +    dc->desc = "VFIO PCI base device";
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +    pdc->exit = vfio_exitfn;
> +    pdc->config_read = vfio_pci_read_config;
> +    pdc->config_write = vfio_pci_write_config;
> +}
> +
> +static const TypeInfo vfio_pci_base_dev_info = {
> +    .name = TYPE_VFIO_PCI_BASE,
> +    .parent = TYPE_PCI_DEVICE,
> +    .instance_size = 0,
> +    .abstract = true,
> +    .class_init = vfio_pci_base_dev_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { INTERFACE_PCIE_DEVICE },
> +        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> +        { }
> +    },
> +};
> +
> +static Property vfio_pci_dev_properties[] = {
> +    DEFINE_PROP_PCI_HOST_DEVADDR("host", VFIOPCIDevice, host),
> +    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_UINT32("x-intx-mmap-timeout-ms", VFIOPCIDevice,
> -                       intx.mmap_timeout, 1100),
>      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_BOOL("x-enable-migration", VFIOPCIDevice,
> -                     vbasedev.enable_migration, 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-geforce-quirks", VFIOPCIDevice,
>                       no_geforce_quirks, 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,


Not sure I understand some of these choices.  The kernel interface
options like "host" and "sysfsdev" obviously belong here, and it makes
sense for quirks related to real hardware to be here, igd, nvidia, gpu
direct, etc.  But the rest I'm a bit lost.  Why would overriding the
PCI IDs, disabling ballooning, disabling the device request interrupt,
enabling exposing a VGA region, supporting a display, be unique to the
kernel interface?  Thanks,

Alex

> @@ -3243,13 +3280,6 @@ static Property vfio_pci_dev_properties[] = {
>      DEFINE_PROP_UNSIGNED_NODEFAULT("x-nv-gpudirect-clique", VFIOPCIDevice,
>                                     nv_gpudirect_clique,
>                                     qdev_prop_nv_gpudirect_clique, uint8_t),
> -    DEFINE_PROP_OFF_AUTO_PCIBAR("x-msix-relocation", VFIOPCIDevice, 
> msix_relo,
> -                                OFF_AUTOPCIBAR_OFF),
> -    /*
> -     * TODO - support passed fds... is this necessary?
> -     * DEFINE_PROP_STRING("vfiofd", VFIOPCIDevice, vfiofd_name),
> -     * DEFINE_PROP_STRING("vfiogroupfd, VFIOPCIDevice, vfiogroupfd_name),
> -     */
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -3261,25 +3291,16 @@ static void vfio_pci_dev_class_init(ObjectClass 
> *klass, void *data)
>      dc->reset = vfio_pci_reset;
>      device_class_set_props(dc, vfio_pci_dev_properties);
>      dc->desc = "VFIO-based PCI device assignment";
> -    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>      pdc->realize = vfio_realize;
> -    pdc->exit = vfio_exitfn;
> -    pdc->config_read = vfio_pci_read_config;
> -    pdc->config_write = vfio_pci_write_config;
>  }
>  
>  static const TypeInfo vfio_pci_dev_info = {
>      .name = TYPE_VFIO_PCI,
> -    .parent = TYPE_PCI_DEVICE,
> -    .instance_size = sizeof(VFIOPCIDevice),
> +    .parent = TYPE_VFIO_PCI_BASE,
> +    .instance_size = sizeof(VFIOKernPCIDevice),
>      .class_init = vfio_pci_dev_class_init,
>      .instance_init = vfio_instance_init,
>      .instance_finalize = vfio_instance_finalize,
> -    .interfaces = (InterfaceInfo[]) {
> -        { INTERFACE_PCIE_DEVICE },
> -        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> -        { }
> -    },
>  };
>  
>  static Property vfio_pci_dev_nohotplug_properties[] = {
> @@ -3298,12 +3319,13 @@ static void 
> vfio_pci_nohotplug_dev_class_init(ObjectClass *klass, void *data)
>  static const TypeInfo vfio_pci_nohotplug_dev_info = {
>      .name = TYPE_VFIO_PCI_NOHOTPLUG,
>      .parent = TYPE_VFIO_PCI,
> -    .instance_size = sizeof(VFIOPCIDevice),
> +    .instance_size = sizeof(VFIOKernPCIDevice),
>      .class_init = vfio_pci_nohotplug_dev_class_init,
>  };
>  
>  static void register_vfio_pci_dev_type(void)
>  {
> +    type_register_static(&vfio_pci_base_dev_info);
>      type_register_static(&vfio_pci_dev_info);
>      type_register_static(&vfio_pci_nohotplug_dev_info);
>  }