Re: [Qemu-devel] [PATCH v7 03/16] hw/vfio/pci: introduce VFIODevice

[Eric Auger]

On 11/05/2014 06:35 PM, Alex Williamson wrote:
> Hi Eric,
> 
> On Fri, 2014-10-31 at 14:05 +0000, Eric Auger wrote:
>> Introduce the VFIODevice struct that is going to be shared by
>> VFIOPCIDevice and VFIOPlatformDevice.
>>
>> Additional fields will be added there later on for review
>> convenience.
>>
>> the group's device_list becomes a list of VFIODevice
>>
>> This obliges to rework the reset_handler which becomes generic and
>> calls VFIODevice ops that are specialized in each parent object.
>> Also functions that iterate on this list must take care that the
>> devices can be something else than VFIOPCIDevice. The type is used
>> to discriminate them.
>>
>> we profit from this step to change the prototype of
>> vfio_unmask_intx, vfio_mask_intx, vfio_disable_irqindex which now
>> apply to VFIODevice. They are renamed as *_irqindex.
>> The index is passed as parameter to anticipate their usage for
>> platform IRQs
> 
> I cringe when reviewers tell me this, so I apologize in advance, but
> there are logically at least 4 separate things happening in this patch:
> 
> 1) VFIODevice
> 2) VFIODeviceOps
> 3) irqindex conversions
> 4) strcmp(name) vs comparing ssss:bb:dd.f
> 
> I don't really see any dependencies between them, and
> I think they'd also be easier to review as 4 separate patches.  More
> below...

Hi Alex,

no problem I am going to split it.
> 
>>
>> Signed-off-by: Eric Auger <address@hidden>
>>
>> ---
>>
>> v4->v5:
>> - fix style issues
>> - in vfio_initfn, rework allocation of vdev->vbasedev.name and
>>   replace snprintf by g_strdup_printf
>> ---
>>  hw/vfio/pci.c | 241 
>> +++++++++++++++++++++++++++++++++++-----------------------
>>  1 file changed, 147 insertions(+), 94 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 93181bf..0531744 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -48,6 +48,11 @@
>>  #define VFIO_ALLOW_KVM_MSI 1
>>  #define VFIO_ALLOW_KVM_MSIX 1
>>  
>> +enum {
>> +    VFIO_DEVICE_TYPE_PCI = 0,
>> +    VFIO_DEVICE_TYPE_PLATFORM = 1,
> 
> VFIO_DEVICE_TYPE_PLATFORM gets dropped in patch 8 and re-added in patch
> 9.  Let's remove it here and let it's first appearance be in patch 9.

yes sure. My bad.
> 
>> +};
>> +
>>  struct VFIOPCIDevice;
>>  
>>  typedef struct VFIOQuirk {
>> @@ -185,9 +190,27 @@ typedef struct VFIOMSIXInfo {
>>      void *mmap;
>>  } VFIOMSIXInfo;
>>  
>> +typedef struct VFIODeviceOps VFIODeviceOps;
>> +
>> +typedef struct VFIODevice {
>> +    QLIST_ENTRY(VFIODevice) next;
>> +    struct VFIOGroup *group;
>> +    char *name;
>> +    int fd;
>> +    int type;
>> +    bool reset_works;
>> +    bool needs_reset;
>> +    VFIODeviceOps *ops;
>> +} VFIODevice;
>> +
>> +struct VFIODeviceOps {
>> +    bool (*vfio_compute_needs_reset)(VFIODevice *vdev);
>> +    int (*vfio_hot_reset_multi)(VFIODevice *vdev);
>> +};
>> +
>>  typedef struct VFIOPCIDevice {
>>      PCIDevice pdev;
>> -    int fd;
>> +    VFIODevice vbasedev;
>>      VFIOINTx intx;
>>      unsigned int config_size;
>>      uint8_t *emulated_config_bits; /* QEMU emulated bits, little-endian */
>> @@ -203,20 +226,16 @@ typedef struct VFIOPCIDevice {
>>      VFIOBAR bars[PCI_NUM_REGIONS - 1]; /* No ROM */
>>      VFIOVGA vga; /* 0xa0000, 0x3b0, 0x3c0 */
>>      PCIHostDeviceAddress host;
>> -    QLIST_ENTRY(VFIOPCIDevice) next;
>> -    struct VFIOGroup *group;
>>      EventNotifier err_notifier;
>>      uint32_t features;
>>  #define VFIO_FEATURE_ENABLE_VGA_BIT 0
>>  #define VFIO_FEATURE_ENABLE_VGA (1 << VFIO_FEATURE_ENABLE_VGA_BIT)
>>      int32_t bootindex;
>>      uint8_t pm_cap;
>> -    bool reset_works;
>>      bool has_vga;
>>      bool pci_aer;
>>      bool has_flr;
>>      bool has_pm_reset;
>> -    bool needs_reset;
>>      bool rom_read_failed;
>>  } VFIOPCIDevice;
>>  
>> @@ -224,7 +243,7 @@ typedef struct VFIOGroup {
>>      int fd;
>>      int groupid;
>>      VFIOContainer *container;
>> -    QLIST_HEAD(, VFIOPCIDevice) device_list;
>> +    QLIST_HEAD(, VFIODevice) device_list;
>>      QLIST_ENTRY(VFIOGroup) next;
>>      QLIST_ENTRY(VFIOGroup) container_next;
>>  } VFIOGroup;
>> @@ -277,7 +296,7 @@ static void vfio_mmap_set_enabled(VFIOPCIDevice *vdev, 
>> bool enabled);
>>  /*
>>   * Common VFIO interrupt disable
>>   */
>> -static void vfio_disable_irqindex(VFIOPCIDevice *vdev, int index)
>> +static void vfio_disable_irqindex(VFIODevice *vbasedev, int index)
>>  {
>>      struct vfio_irq_set irq_set = {
>>          .argsz = sizeof(irq_set),
>> @@ -287,37 +306,37 @@ static void vfio_disable_irqindex(VFIOPCIDevice *vdev, 
>> int index)
>>          .count = 0,
>>      };
>>  
>> -    ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, &irq_set);
>> +    ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, &irq_set);
>>  }
>>  
>>  /*
>>   * INTx
>>   */
>> -static void vfio_unmask_intx(VFIOPCIDevice *vdev)
>> +static void vfio_unmask_irqindex(VFIODevice *vbasedev, int index)
>>  {
>>      struct vfio_irq_set irq_set = {
>>          .argsz = sizeof(irq_set),
>>          .flags = VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_UNMASK,
>> -        .index = VFIO_PCI_INTX_IRQ_INDEX,
>> +        .index = index,
>>          .start = 0,
>>          .count = 1,
>>      };
> 
> We're turning these into a generic function, but the function assumes a
> single start/count.  Do we want to reflect that in the name or args?
> For instance, maybe it should be vfio_unmask_simple_irqindex() to
> reflect the common use case of a single interrupt per index and we can
> create another function later for a more complete specification should
> we ever need it.  Thanks,
OK

Thanks for your time

Best Regards

Eric
> 
> Alex
> 
>>  
>> -    ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, &irq_set);
>> +    ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, &irq_set);
>>  }
>>  
>>  #ifdef CONFIG_KVM /* Unused outside of CONFIG_KVM code */
>> -static void vfio_mask_intx(VFIOPCIDevice *vdev)
>> +static void vfio_mask_irqindex(VFIODevice *vbasedev, int index)
>>  {
>>      struct vfio_irq_set irq_set = {
>>          .argsz = sizeof(irq_set),
>>          .flags = VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_MASK,
>> -        .index = VFIO_PCI_INTX_IRQ_INDEX,
>> +        .index = index,
>>          .start = 0,
>>          .count = 1,
>>      };
>>  
>> -    ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, &irq_set);
>> +    ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, &irq_set);
>>  }
>>  #endif
>>  
>> @@ -381,7 +400,7 @@ static void vfio_eoi(VFIOPCIDevice *vdev)
>>  
>>      vdev->intx.pending = false;
>>      pci_irq_deassert(&vdev->pdev);
>> -    vfio_unmask_intx(vdev);
>> +    vfio_unmask_irqindex(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
>>  }
>>  
>>  static void vfio_enable_intx_kvm(VFIOPCIDevice *vdev)
>> @@ -404,7 +423,7 @@ static void vfio_enable_intx_kvm(VFIOPCIDevice *vdev)
>>  
>>      /* Get to a known interrupt state */
>>      qemu_set_fd_handler(irqfd.fd, NULL, NULL, vdev);
>> -    vfio_mask_intx(vdev);
>> +    vfio_mask_irqindex(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
>>      vdev->intx.pending = false;
>>      pci_irq_deassert(&vdev->pdev);
>>  
>> @@ -434,7 +453,7 @@ static void vfio_enable_intx_kvm(VFIOPCIDevice *vdev)
>>  
>>      *pfd = irqfd.resamplefd;
>>  
>> -    ret = ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, irq_set);
>> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>>      g_free(irq_set);
>>      if (ret) {
>>          error_report("vfio: Error: Failed to setup INTx unmask fd: %m");
>> @@ -442,7 +461,7 @@ static void vfio_enable_intx_kvm(VFIOPCIDevice *vdev)
>>      }
>>  
>>      /* Let'em rip */
>> -    vfio_unmask_intx(vdev);
>> +    vfio_unmask_irqindex(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
>>  
>>      vdev->intx.kvm_accel = true;
>>  
>> @@ -458,7 +477,7 @@ fail_irqfd:
>>      event_notifier_cleanup(&vdev->intx.unmask);
>>  fail:
>>      qemu_set_fd_handler(irqfd.fd, vfio_intx_interrupt, NULL, vdev);
>> -    vfio_unmask_intx(vdev);
>> +    vfio_unmask_irqindex(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
>>  #endif
>>  }
>>  
>> @@ -479,7 +498,7 @@ static void vfio_disable_intx_kvm(VFIOPCIDevice *vdev)
>>       * Get to a known state, hardware masked, QEMU ready to accept new
>>       * interrupts, QEMU IRQ de-asserted.
>>       */
>> -    vfio_mask_intx(vdev);
>> +    vfio_mask_irqindex(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
>>      vdev->intx.pending = false;
>>      pci_irq_deassert(&vdev->pdev);
>>  
>> @@ -497,7 +516,7 @@ static void vfio_disable_intx_kvm(VFIOPCIDevice *vdev)
>>      vdev->intx.kvm_accel = false;
>>  
>>      /* If we've missed an event, let it re-fire through QEMU */
>> -    vfio_unmask_intx(vdev);
>> +    vfio_unmask_irqindex(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
>>  
>>      trace_vfio_disable_intx_kvm(vdev->host.domain, vdev->host.bus,
>>                                  vdev->host.slot, vdev->host.function);
>> @@ -583,7 +602,7 @@ static int vfio_enable_intx(VFIOPCIDevice *vdev)
>>      *pfd = event_notifier_get_fd(&vdev->intx.interrupt);
>>      qemu_set_fd_handler(*pfd, vfio_intx_interrupt, NULL, vdev);
>>  
>> -    ret = ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, irq_set);
>> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>>      g_free(irq_set);
>>      if (ret) {
>>          error_report("vfio: Error: Failed to setup INTx fd: %m");
>> @@ -608,7 +627,7 @@ static void vfio_disable_intx(VFIOPCIDevice *vdev)
>>  
>>      timer_del(vdev->intx.mmap_timer);
>>      vfio_disable_intx_kvm(vdev);
>> -    vfio_disable_irqindex(vdev, VFIO_PCI_INTX_IRQ_INDEX);
>> +    vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
>>      vdev->intx.pending = false;
>>      pci_irq_deassert(&vdev->pdev);
>>      vfio_mmap_set_enabled(vdev, true);
>> @@ -698,7 +717,7 @@ static int vfio_enable_vectors(VFIOPCIDevice *vdev, bool 
>> msix)
>>          fds[i] = fd;
>>      }
>>  
>> -    ret = ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, irq_set);
>> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>>  
>>      g_free(irq_set);
>>  
>> @@ -795,7 +814,7 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev, 
>> unsigned int nr,
>>       * increase them as needed.
>>       */
>>      if (vdev->nr_vectors < nr + 1) {
>> -        vfio_disable_irqindex(vdev, VFIO_PCI_MSIX_IRQ_INDEX);
>> +        vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX);
>>          vdev->nr_vectors = nr + 1;
>>          ret = vfio_enable_vectors(vdev, true);
>>          if (ret) {
>> @@ -823,7 +842,7 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev, 
>> unsigned int nr,
>>              *pfd = event_notifier_get_fd(&vector->interrupt);
>>          }
>>  
>> -        ret = ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, irq_set);
>> +        ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>>          g_free(irq_set);
>>          if (ret) {
>>              error_report("vfio: failed to modify vector, %d", ret);
>> @@ -874,7 +893,7 @@ static void vfio_msix_vector_release(PCIDevice *pdev, 
>> unsigned int nr)
>>  
>>          *pfd = event_notifier_get_fd(&vector->interrupt);
>>  
>> -        ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, irq_set);
>> +        ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>>  
>>          g_free(irq_set);
>>      }
>> @@ -1033,7 +1052,7 @@ static void vfio_disable_msix(VFIOPCIDevice *vdev)
>>      }
>>  
>>      if (vdev->nr_vectors) {
>> -        vfio_disable_irqindex(vdev, VFIO_PCI_MSIX_IRQ_INDEX);
>> +        vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX);
>>      }
>>  
>>      vfio_disable_msi_common(vdev);
>> @@ -1044,7 +1063,7 @@ static void vfio_disable_msix(VFIOPCIDevice *vdev)
>>  
>>  static void vfio_disable_msi(VFIOPCIDevice *vdev)
>>  {
>> -    vfio_disable_irqindex(vdev, VFIO_PCI_MSI_IRQ_INDEX);
>> +    vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSI_IRQ_INDEX);
>>      vfio_disable_msi_common(vdev);
>>  
>>      trace_vfio_disable_msi(vdev->host.domain, vdev->host.bus,
>> @@ -1188,7 +1207,7 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
>>      off_t off = 0;
>>      size_t bytes;
>>  
>> -    if (ioctl(vdev->fd, VFIO_DEVICE_GET_REGION_INFO, &reg_info)) {
>> +    if (ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_REGION_INFO, &reg_info)) {
>>          error_report("vfio: Error getting ROM info: %m");
>>          return;
>>      }
>> @@ -1218,7 +1237,8 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
>>      memset(vdev->rom, 0xff, size);
>>  
>>      while (size) {
>> -        bytes = pread(vdev->fd, vdev->rom + off, size, vdev->rom_offset + 
>> off);
>> +        bytes = pread(vdev->vbasedev.fd, vdev->rom + off,
>> +                      size, vdev->rom_offset + off);
>>          if (bytes == 0) {
>>              break;
>>          } else if (bytes > 0) {
>> @@ -1312,6 +1332,7 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
>>      off_t offset = vdev->config_offset + PCI_ROM_ADDRESS;
>>      DeviceState *dev = DEVICE(vdev);
>>      char name[32];
>> +    int fd = vdev->vbasedev.fd;
>>  
>>      if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
>>          /* Since pci handles romfile, just print a message and return */
>> @@ -1330,10 +1351,10 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
>>       * Use the same size ROM BAR as the physical device.  The contents
>>       * will get filled in later when the guest tries to read it.
>>       */
>> -    if (pread(vdev->fd, &orig, 4, offset) != 4 ||
>> -        pwrite(vdev->fd, &size, 4, offset) != 4 ||
>> -        pread(vdev->fd, &size, 4, offset) != 4 ||
>> -        pwrite(vdev->fd, &orig, 4, offset) != 4) {
>> +    if (pread(fd, &orig, 4, offset) != 4 ||
>> +        pwrite(fd, &size, 4, offset) != 4 ||
>> +        pread(fd, &size, 4, offset) != 4 ||
>> +        pwrite(fd, &orig, 4, offset) != 4) {
>>          error_report("%s(%04x:%02x:%02x.%x) failed: %m",
>>                       __func__, vdev->host.domain, vdev->host.bus,
>>                       vdev->host.slot, vdev->host.function);
>> @@ -2345,7 +2366,8 @@ static uint32_t vfio_pci_read_config(PCIDevice *pdev, 
>> uint32_t addr, int len)
>>      if (~emu_bits & (0xffffffffU >> (32 - len * 8))) {
>>          ssize_t ret;
>>  
>> -        ret = pread(vdev->fd, &phys_val, len, vdev->config_offset + addr);
>> +        ret = pread(vdev->vbasedev.fd, &phys_val, len,
>> +                    vdev->config_offset + addr);
>>          if (ret != len) {
>>              error_report("%s(%04x:%02x:%02x.%x, 0x%x, 0x%x) failed: %m",
>>                           __func__, vdev->host.domain, vdev->host.bus,
>> @@ -2375,7 +2397,8 @@ static void vfio_pci_write_config(PCIDevice *pdev, 
>> uint32_t addr,
>>                                  addr, val, len);
>>  
>>      /* Write everything to VFIO, let it filter out what we can't write */
>> -    if (pwrite(vdev->fd, &val_le, len, vdev->config_offset + addr) != len) {
>> +    if (pwrite(vdev->vbasedev.fd, &val_le, len, vdev->config_offset + addr)
>> +                != len) {
>>          error_report("%s(%04x:%02x:%02x.%x, 0x%x, 0x%x, 0x%x) failed: %m",
>>                       __func__, vdev->host.domain, vdev->host.bus,
>>                       vdev->host.slot, vdev->host.function, addr, val, len);
>> @@ -2743,7 +2766,7 @@ static int vfio_setup_msi(VFIOPCIDevice *vdev, int pos)
>>      bool msi_64bit, msi_maskbit;
>>      int ret, entries;
>>  
>> -    if (pread(vdev->fd, &ctrl, sizeof(ctrl),
>> +    if (pread(vdev->vbasedev.fd, &ctrl, sizeof(ctrl),
>>                vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) {
>>          return -errno;
>>      }
>> @@ -2782,23 +2805,24 @@ static int vfio_early_setup_msix(VFIOPCIDevice *vdev)
>>      uint8_t pos;
>>      uint16_t ctrl;
>>      uint32_t table, pba;
>> +    int fd = vdev->vbasedev.fd;
>>  
>>      pos = pci_find_capability(&vdev->pdev, PCI_CAP_ID_MSIX);
>>      if (!pos) {
>>          return 0;
>>      }
>>  
>> -    if (pread(vdev->fd, &ctrl, sizeof(ctrl),
>> +    if (pread(fd, &ctrl, sizeof(ctrl),
>>                vdev->config_offset + pos + PCI_CAP_FLAGS) != sizeof(ctrl)) {
>>          return -errno;
>>      }
>>  
>> -    if (pread(vdev->fd, &table, sizeof(table),
>> +    if (pread(fd, &table, sizeof(table),
>>                vdev->config_offset + pos + PCI_MSIX_TABLE) != sizeof(table)) 
>> {
>>          return -errno;
>>      }
>>  
>> -    if (pread(vdev->fd, &pba, sizeof(pba),
>> +    if (pread(fd, &pba, sizeof(pba),
>>                vdev->config_offset + pos + PCI_MSIX_PBA) != sizeof(pba)) {
>>          return -errno;
>>      }
>> @@ -2950,7 +2974,7 @@ static void vfio_map_bar(VFIOPCIDevice *vdev, int nr)
>>               vdev->host.function, nr);
>>  
>>      /* Determine what type of BAR this is for registration */
>> -    ret = pread(vdev->fd, &pci_bar, sizeof(pci_bar),
>> +    ret = pread(vdev->vbasedev.fd, &pci_bar, sizeof(pci_bar),
>>                  vdev->config_offset + PCI_BASE_ADDRESS_0 + (4 * nr));
>>      if (ret != sizeof(pci_bar)) {
>>          error_report("vfio: Failed to read BAR %d (%m)", nr);
>> @@ -3365,12 +3389,12 @@ static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, 
>> bool single)
>>                               single ? "one" : "multi");
>>  
>>      vfio_pci_pre_reset(vdev);
>> -    vdev->needs_reset = false;
>> +    vdev->vbasedev.needs_reset = false;
>>  
>>      info = g_malloc0(sizeof(*info));
>>      info->argsz = sizeof(*info);
>>  
>> -    ret = ioctl(vdev->fd, VFIO_DEVICE_GET_PCI_HOT_RESET_INFO, info);
>> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_PCI_HOT_RESET_INFO, 
>> info);
>>      if (ret && errno != ENOSPC) {
>>          ret = -errno;
>>          if (!vdev->has_pm_reset) {
>> @@ -3386,7 +3410,7 @@ static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, 
>> bool single)
>>      info->argsz = sizeof(*info) + (count * sizeof(*devices));
>>      devices = &info->devices[0];
>>  
>> -    ret = ioctl(vdev->fd, VFIO_DEVICE_GET_PCI_HOT_RESET_INFO, info);
>> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_PCI_HOT_RESET_INFO, 
>> info);
>>      if (ret) {
>>          ret = -errno;
>>          error_report("vfio: hot reset info failed: %m");
>> @@ -3402,6 +3426,7 @@ static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, 
>> bool single)
>>      for (i = 0; i < info->count; i++) {
>>          PCIHostDeviceAddress host;
>>          VFIOPCIDevice *tmp;
>> +        VFIODevice *vbasedev_iter;
>>  
>>          host.domain = devices[i].segment;
>>          host.bus = devices[i].bus;
>> @@ -3433,7 +3458,11 @@ static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, 
>> bool single)
>>          }
>>  
>>          /* Prep dependent devices for reset and clear our marker. */
>> -        QLIST_FOREACH(tmp, &group->device_list, next) {
>> +        QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
>> +            if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI) {
>> +                continue;
>> +            }
>> +            tmp = container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
>>              if (vfio_pci_host_match(&host, &tmp->host)) {
>>                  if (single) {
>>                      error_report("vfio: found another in-use device "
>> @@ -3443,7 +3472,7 @@ static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, 
>> bool single)
>>                      goto out_single;
>>                  }
>>                  vfio_pci_pre_reset(tmp);
>> -                tmp->needs_reset = false;
>> +                tmp->vbasedev.needs_reset = false;
>>                  multi = true;
>>                  break;
>>              }
>> @@ -3482,7 +3511,7 @@ static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, 
>> bool single)
>>      }
>>  
>>      /* Bus reset! */
>> -    ret = ioctl(vdev->fd, VFIO_DEVICE_PCI_HOT_RESET, reset);
>> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_PCI_HOT_RESET, reset);
>>      g_free(reset);
>>  
>>      trace_vfio_pci_hot_reset_result(vdev->host.domain,
>> @@ -3496,6 +3525,7 @@ out:
>>      for (i = 0; i < info->count; i++) {
>>          PCIHostDeviceAddress host;
>>          VFIOPCIDevice *tmp;
>> +        VFIODevice *vbasedev_iter;
>>  
>>          host.domain = devices[i].segment;
>>          host.bus = devices[i].bus;
>> @@ -3516,7 +3546,11 @@ out:
>>              break;
>>          }
>>  
>> -        QLIST_FOREACH(tmp, &group->device_list, next) {
>> +        QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
>> +            if (vbasedev_iter->type != VFIO_DEVICE_TYPE_PCI) {
>> +                continue;
>> +            }
>> +            tmp = container_of(vbasedev_iter, VFIOPCIDevice, vbasedev);
>>              if (vfio_pci_host_match(&host, &tmp->host)) {
>>                  vfio_pci_post_reset(tmp);
>>                  break;
>> @@ -3550,28 +3584,41 @@ static int vfio_pci_hot_reset_one(VFIOPCIDevice 
>> *vdev)
>>      return vfio_pci_hot_reset(vdev, true);
>>  }
>>  
>> -static int vfio_pci_hot_reset_multi(VFIOPCIDevice *vdev)
>> +static int vfio_pci_hot_reset_multi(VFIODevice *vbasedev)
>>  {
>> +    VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
>>      return vfio_pci_hot_reset(vdev, false);
>>  }
>>  
>> -static void vfio_pci_reset_handler(void *opaque)
>> +static bool vfio_pci_compute_needs_reset(VFIODevice *vbasedev)
>> +{
>> +    VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
>> +    if (!vbasedev->reset_works || (!vdev->has_flr && vdev->has_pm_reset)) {
>> +        vbasedev->needs_reset = true;
>> +    }
>> +    return vbasedev->needs_reset;
>> +}
>> +
>> +static VFIODeviceOps vfio_pci_ops = {
>> +    .vfio_compute_needs_reset = vfio_pci_compute_needs_reset,
>> +    .vfio_hot_reset_multi = vfio_pci_hot_reset_multi,
>> +};
>> +
>> +static void vfio_reset_handler(void *opaque)
>>  {
>>      VFIOGroup *group;
>> -    VFIOPCIDevice *vdev;
>> +    VFIODevice *vbasedev;
>>  
>>      QLIST_FOREACH(group, &group_list, next) {
>> -        QLIST_FOREACH(vdev, &group->device_list, next) {
>> -            if (!vdev->reset_works || (!vdev->has_flr && 
>> vdev->has_pm_reset)) {
>> -                vdev->needs_reset = true;
>> -            }
>> +        QLIST_FOREACH(vbasedev, &group->device_list, next) {
>> +            vbasedev->ops->vfio_compute_needs_reset(vbasedev);
>>          }
>>      }
>>  
>>      QLIST_FOREACH(group, &group_list, next) {
>> -        QLIST_FOREACH(vdev, &group->device_list, next) {
>> -            if (vdev->needs_reset) {
>> -                vfio_pci_hot_reset_multi(vdev);
>> +        QLIST_FOREACH(vbasedev, &group->device_list, next) {
>> +            if (vbasedev->needs_reset) {
>> +                vbasedev->ops->vfio_hot_reset_multi(vbasedev);
>>              }
>>          }
>>      }
>> @@ -3860,7 +3907,7 @@ static VFIOGroup *vfio_get_group(int groupid, 
>> AddressSpace *as)
>>      }
>>  
>>      if (QLIST_EMPTY(&group_list)) {
>> -        qemu_register_reset(vfio_pci_reset_handler, NULL);
>> +        qemu_register_reset(vfio_reset_handler, NULL);
>>      }
>>  
>>      QLIST_INSERT_HEAD(&group_list, group, next);
>> @@ -3892,7 +3939,7 @@ static void vfio_put_group(VFIOGroup *group)
>>      g_free(group);
>>  
>>      if (QLIST_EMPTY(&group_list)) {
>> -        qemu_unregister_reset(vfio_pci_reset_handler, NULL);
>> +        qemu_unregister_reset(vfio_reset_handler, NULL);
>>      }
>>  }
>>  
>> @@ -3913,12 +3960,12 @@ static int vfio_get_device(VFIOGroup *group, const 
>> char *name,
>>          return ret;
>>      }
>>  
>> -    vdev->fd = ret;
>> -    vdev->group = group;
>> -    QLIST_INSERT_HEAD(&group->device_list, vdev, next);
>> +    vdev->vbasedev.fd = ret;
>> +    vdev->vbasedev.group = group;
>> +    QLIST_INSERT_HEAD(&group->device_list, &vdev->vbasedev, next);
>>  
>>      /* Sanity check device */
>> -    ret = ioctl(vdev->fd, VFIO_DEVICE_GET_INFO, &dev_info);
>> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_INFO, &dev_info);
>>      if (ret) {
>>          error_report("vfio: error getting device info: %m");
>>          goto error;
>> @@ -3932,7 +3979,7 @@ static int vfio_get_device(VFIOGroup *group, const 
>> char *name,
>>          goto error;
>>      }
>>  
>> -    vdev->reset_works = !!(dev_info.flags & VFIO_DEVICE_FLAGS_RESET);
>> +    vdev->vbasedev.reset_works = !!(dev_info.flags & 
>> VFIO_DEVICE_FLAGS_RESET);
>>  
>>      if (dev_info.num_regions < VFIO_PCI_CONFIG_REGION_INDEX + 1) {
>>          error_report("vfio: unexpected number of io regions %u",
>> @@ -3948,7 +3995,7 @@ static int vfio_get_device(VFIOGroup *group, const 
>> char *name,
>>      for (i = VFIO_PCI_BAR0_REGION_INDEX; i < VFIO_PCI_ROM_REGION_INDEX; 
>> i++) {
>>          reg_info.index = i;
>>  
>> -        ret = ioctl(vdev->fd, VFIO_DEVICE_GET_REGION_INFO, &reg_info);
>> +        ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_REGION_INFO, 
>> &reg_info);
>>          if (ret) {
>>              error_report("vfio: Error getting region %d info: %m", i);
>>              goto error;
>> @@ -3962,14 +4009,14 @@ static int vfio_get_device(VFIOGroup *group, const 
>> char *name,
>>          vdev->bars[i].flags = reg_info.flags;
>>          vdev->bars[i].size = reg_info.size;
>>          vdev->bars[i].fd_offset = reg_info.offset;
>> -        vdev->bars[i].fd = vdev->fd;
>> +        vdev->bars[i].fd = vdev->vbasedev.fd;
>>          vdev->bars[i].nr = i;
>>          QLIST_INIT(&vdev->bars[i].quirks);
>>      }
>>  
>>      reg_info.index = VFIO_PCI_CONFIG_REGION_INDEX;
>>  
>> -    ret = ioctl(vdev->fd, VFIO_DEVICE_GET_REGION_INFO, &reg_info);
>> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_REGION_INFO, &reg_info);
>>      if (ret) {
>>          error_report("vfio: Error getting config info: %m");
>>          goto error;
>> @@ -3992,7 +4039,7 @@ static int vfio_get_device(VFIOGroup *group, const 
>> char *name,
>>              .index = VFIO_PCI_VGA_REGION_INDEX,
>>           };
>>  
>> -        ret = ioctl(vdev->fd, VFIO_DEVICE_GET_REGION_INFO, &vga_info);
>> +        ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_REGION_INFO, 
>> &vga_info);
>>          if (ret) {
>>              error_report(
>>                  "vfio: Device does not support requested feature x-vga");
>> @@ -4009,7 +4056,7 @@ static int vfio_get_device(VFIOGroup *group, const 
>> char *name,
>>          }
>>  
>>          vdev->vga.fd_offset = vga_info.offset;
>> -        vdev->vga.fd = vdev->fd;
>> +        vdev->vga.fd = vdev->vbasedev.fd;
>>  
>>          vdev->vga.region[QEMU_PCI_VGA_MEM].offset = QEMU_PCI_VGA_MEM_BASE;
>>          vdev->vga.region[QEMU_PCI_VGA_MEM].nr = QEMU_PCI_VGA_MEM;
>> @@ -4027,7 +4074,7 @@ static int vfio_get_device(VFIOGroup *group, const 
>> char *name,
>>      }
>>      irq_info.index = VFIO_PCI_ERR_IRQ_INDEX;
>>  
>> -    ret = ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
>> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
>>      if (ret) {
>>          /* This can fail for an old kernel or legacy PCI dev */
>>          trace_vfio_get_device_get_irq_info_failure();
>> @@ -4043,19 +4090,20 @@ static int vfio_get_device(VFIOGroup *group, const 
>> char *name,
>>  
>>  error:
>>      if (ret) {
>> -        QLIST_REMOVE(vdev, next);
>> -        vdev->group = NULL;
>> -        close(vdev->fd);
>> +        QLIST_REMOVE(&vdev->vbasedev, next);
>> +        vdev->vbasedev.group = NULL;
>> +        close(vdev->vbasedev.fd);
>>      }
>>      return ret;
>>  }
>>  
>>  static void vfio_put_device(VFIOPCIDevice *vdev)
>>  {
>> -    QLIST_REMOVE(vdev, next);
>> -    vdev->group = NULL;
>> -    trace_vfio_put_device(vdev->fd);
>> -    close(vdev->fd);
>> +    QLIST_REMOVE(&vdev->vbasedev, next);
>> +    vdev->vbasedev.group = NULL;
>> +    trace_vfio_put_device(vdev->vbasedev.fd);
>> +    close(vdev->vbasedev.fd);
>> +    g_free(vdev->vbasedev.name);
>>      if (vdev->msix) {
>>          g_free(vdev->msix);
>>          vdev->msix = NULL;
>> @@ -4124,7 +4172,7 @@ static void vfio_register_err_notifier(VFIOPCIDevice 
>> *vdev)
>>      *pfd = event_notifier_get_fd(&vdev->err_notifier);
>>      qemu_set_fd_handler(*pfd, vfio_err_notifier_handler, NULL, vdev);
>>  
>> -    ret = ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, irq_set);
>> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>>      if (ret) {
>>          error_report("vfio: Failed to set up error notification");
>>          qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
>> @@ -4157,7 +4205,7 @@ static void vfio_unregister_err_notifier(VFIOPCIDevice 
>> *vdev)
>>      pfd = (int32_t *)&irq_set->data;
>>      *pfd = -1;
>>  
>> -    ret = ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, irq_set);
>> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>>      if (ret) {
>>          error_report("vfio: Failed to de-assign error fd: %m");
>>      }
>> @@ -4169,7 +4217,8 @@ static void vfio_unregister_err_notifier(VFIOPCIDevice 
>> *vdev)
>>  
>>  static int vfio_initfn(PCIDevice *pdev)
>>  {
>> -    VFIOPCIDevice *pvdev, *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
>> +    VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
>> +    VFIODevice *vbasedev_iter;
>>      VFIOGroup *group;
>>      char path[PATH_MAX], iommu_group_path[PATH_MAX], *group_name;
>>      ssize_t len;
>> @@ -4187,6 +4236,13 @@ static int vfio_initfn(PCIDevice *pdev)
>>          return -errno;
>>      }
>>  
>> +    vdev->vbasedev.ops = &vfio_pci_ops;
>> +
>> +    vdev->vbasedev.type = VFIO_DEVICE_TYPE_PCI;
>> +    g_strdup_printf(vdev->vbasedev.name, "%04x:%02x:%02x.%01x",
>> +            vdev->host.domain, vdev->host.bus, vdev->host.slot,
>> +            vdev->host.function);
>> +
>>      strncat(path, "iommu_group", sizeof(path) - strlen(path) - 1);
>>  
>>      len = readlink(path, iommu_group_path, sizeof(path));
>> @@ -4216,12 +4272,8 @@ static int vfio_initfn(PCIDevice *pdev)
>>              vdev->host.domain, vdev->host.bus, vdev->host.slot,
>>              vdev->host.function);
>>  
>> -    QLIST_FOREACH(pvdev, &group->device_list, next) {
>> -        if (pvdev->host.domain == vdev->host.domain &&
>> -            pvdev->host.bus == vdev->host.bus &&
>> -            pvdev->host.slot == vdev->host.slot &&
>> -            pvdev->host.function == vdev->host.function) {
>> -
>> +    QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
>> +        if (strcmp(vbasedev_iter->name, vdev->vbasedev.name) == 0) {
>>              error_report("vfio: error: device %s is already attached", 
>> path);
>>              vfio_put_group(group);
>>              return -EBUSY;
>> @@ -4236,7 +4288,7 @@ static int vfio_initfn(PCIDevice *pdev)
>>      }
>>  
>>      /* Get a copy of config space */
>> -    ret = pread(vdev->fd, vdev->pdev.config,
>> +    ret = pread(vdev->vbasedev.fd, vdev->pdev.config,
>>                  MIN(pci_config_size(&vdev->pdev), vdev->config_size),
>>                  vdev->config_offset);
>>      if (ret < (int)MIN(pci_config_size(&vdev->pdev), vdev->config_size)) {
>> @@ -4323,7 +4375,7 @@ out_put:
>>  static void vfio_exitfn(PCIDevice *pdev)
>>  {
>>      VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
>> -    VFIOGroup *group = vdev->group;
>> +    VFIOGroup *group = vdev->vbasedev.group;
>>  
>>      vfio_unregister_err_notifier(vdev);
>>      pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
>> @@ -4349,8 +4401,9 @@ static void vfio_pci_reset(DeviceState *dev)
>>  
>>      vfio_pci_pre_reset(vdev);
>>  
>> -    if (vdev->reset_works && (vdev->has_flr || !vdev->has_pm_reset) &&
>> -        !ioctl(vdev->fd, VFIO_DEVICE_RESET)) {
>> +    if (vdev->vbasedev.reset_works &&
>> +        (vdev->has_flr || !vdev->has_pm_reset) &&
>> +        !ioctl(vdev->vbasedev.fd, VFIO_DEVICE_RESET)) {
>>          trace_vfio_pci_reset_flr(vdev->host.domain, vdev->host.bus,
>>                                    vdev->host.slot, vdev->host.function);
>>          goto post_reset;
>> @@ -4362,8 +4415,8 @@ static void vfio_pci_reset(DeviceState *dev)
>>      }
>>  
>>      /* If nothing else works and the device supports PM reset, use it */
>> -    if (vdev->reset_works && vdev->has_pm_reset &&
>> -        !ioctl(vdev->fd, VFIO_DEVICE_RESET)) {
>> +    if (vdev->vbasedev.reset_works && vdev->has_pm_reset &&
>> +        !ioctl(vdev->vbasedev.fd, VFIO_DEVICE_RESET)) {
>>          trace_vfio_pci_reset_pm(vdev->host.domain, vdev->host.bus,
>>                                  vdev->host.slot, vdev->host.function);
>>          goto post_reset;
> 
> 
>