qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [RFC v4 04/13] hw/vfio/pci: introduce VFIODevice


From: Eric Auger
Subject: Re: [Qemu-devel] [RFC v4 04/13] hw/vfio/pci: introduce VFIODevice
Date: Wed, 23 Jul 2014 12:02:36 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0

On 07/09/2014 12:41 AM, Alex Williamson wrote:
> On Mon, 2014-07-07 at 13:27 +0100, 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
>>
>> Signed-off-by: Eric Auger <address@hidden>
>> ---
>>  hw/vfio/pci.c | 243 
>> +++++++++++++++++++++++++++++++++++-----------------------
>>  1 file changed, 149 insertions(+), 94 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index a7df3de..d0bee62 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -44,6 +44,11 @@
>>  #define VFIO_ALLOW_KVM_MSI 1
>>  #define VFIO_ALLOW_KVM_MSIX 1
>>  
>> +enum {
>> +    VFIO_DEVICE_TYPE_PCI = 0,
>> +    VFIO_DEVICE_TYPE_PLATFORM = 1,
>> +};
>> +
>>  struct VFIOPCIDevice;
>>  
>>  typedef struct VFIOQuirk {
>> @@ -173,9 +178,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 */
>> @@ -191,20 +214,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;
>>  
>> @@ -212,7 +231,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;
>> @@ -265,7 +284,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),
>> @@ -275,37 +294,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,
>>      };
>>  
>> -    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
>>  
>> @@ -369,7 +388,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)
>> @@ -392,7 +411,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);
>>  
>> @@ -422,7 +441,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");
>> @@ -430,7 +449,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;
>>  
>> @@ -447,7 +466,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
>>  }
>>  
>> @@ -468,7 +487,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);
>>  
>> @@ -486,7 +505,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);
>>  
>>      DPRINTF("%s(%04x:%02x:%02x.%x) KVM INTx accel disabled\n",
>>              __func__, vdev->host.domain, vdev->host.bus,
>> @@ -574,7 +593,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");
>> @@ -599,7 +618,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);
>> @@ -678,7 +697,7 @@ static int vfio_enable_vectors(VFIOPCIDevice *vdev, bool 
>> msix)
>>          }
>>      }
>>  
>> -    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);
>>  
>> @@ -777,7 +796,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) {
>> @@ -805,7 +824,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);
>> @@ -856,7 +875,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);
>>      }
>> @@ -1016,7 +1035,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);
>> @@ -1027,7 +1046,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);
>>  
>>      DPRINTF("%s(%04x:%02x:%02x.%x)\n", __func__, vdev->host.domain,
>> @@ -1173,7 +1192,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;
>>      }
>> @@ -1203,7 +1222,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) {
>> @@ -1297,6 +1317,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 */
>> @@ -1315,10 +1336,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);
>> @@ -2302,7 +2323,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,
>> @@ -2332,7 +2354,8 @@ static void vfio_pci_write_config(PCIDevice *pdev, 
>> uint32_t addr,
>>              vdev->host.function, 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);
>> @@ -2702,7 +2725,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;
>>      }
>> @@ -2741,23 +2764,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;
>>      }
>> @@ -2913,7 +2937,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);
>> @@ -3334,12 +3358,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) {
>> @@ -3355,7 +3379,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");
>> @@ -3370,6 +3394,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;
>> @@ -3401,7 +3426,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) {
>>                      DPRINTF("vfio: found another in-use device "
>> @@ -3411,7 +3440,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;
>>              }
>> @@ -3450,7 +3479,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);
>>  
>>      DPRINTF("%04x:%02x:%02x.%x hot reset: %s\n", vdev->host.domain,
>> @@ -3462,6 +3491,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;
>> @@ -3482,7 +3512,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;
>> @@ -3516,28 +3550,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);
> 
> nit, extra white space      ^
Hi Alex,

OK corrected
> 
>>      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);
>>              }
>>          }
>>      }
>> @@ -3682,7 +3729,8 @@ static int vfio_connect_container(VFIOGroup *group, 
>> AddressSpace *as)
>>  
>>          if (container->iommu_data.type1.error) {
>>              ret = container->iommu_data.type1.error;
>> -            error_report("vfio: memory listener initialization failed for 
>> container");
>> +            error_report("vfio: memory listener initialization failed"
>> +                         " for container");
> 
> Generally not good to split strings that would otherwise be search-able.
OK
> 
>>              goto listener_release_exit;
>>          }
>>  
>> @@ -3826,7 +3874,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);
>> @@ -3858,7 +3906,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);
>>      }
>>  }
>>  
>> @@ -3879,12 +3927,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;
>> @@ -3898,7 +3946,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",
>> @@ -3914,7 +3962,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;
>> @@ -3928,14 +3976,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;
>> @@ -3959,7 +4007,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");
>> @@ -3976,7 +4024,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;
>> @@ -3994,7 +4042,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 */
>>          DPRINTF("VFIO_DEVICE_GET_IRQ_INFO failure: %m\n");
>> @@ -4010,19 +4058,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;
>> +    QLIST_REMOVE(&vdev->vbasedev, next);
>> +    vdev->vbasedev.group = NULL;
>>      DPRINTF("vfio_put_device: close vdev->fd\n");
>> -    close(vdev->fd);
>> +    close(vdev->vbasedev.fd);
>> +    g_free(vdev->vbasedev.name);
>>      if (vdev->msix) {
>>          g_free(vdev->msix);
>>          vdev->msix = NULL;
>> @@ -4091,7 +4140,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);
>> @@ -4124,7 +4173,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");
>>      }
>> @@ -4136,7 +4185,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;
>> @@ -4154,6 +4204,14 @@ static int vfio_initfn(PCIDevice *pdev)
>>          return -errno;
>>      }
>>  
>> +    vdev->vbasedev.ops = &vfio_pci_ops;
>> +
>> +    vdev->vbasedev.type = VFIO_DEVICE_TYPE_PCI;
>> +    vdev->vbasedev.name = g_malloc0(PATH_MAX);
>> +    snprintf(vdev->vbasedev.name, PATH_MAX, "%04x:%02x:%02x.%01x",
>> +            vdev->host.domain, vdev->host.bus, vdev->host.slot,
>> +            vdev->host.function);
>> +
> 
> asprintf(3)?  This is a deterministic length, so PATH_MAX is especially
> ridiculous.
agreed, will use asprintf instead.

Regarding the new VFIODevice overall, does it match your expectations in
term of factorization. Do you think there are the requested fields there.

Thanks you in advance

Best Regards

Eric
> 
>>      strncat(path, "iommu_group", sizeof(path) - strlen(path) - 1);
>>  
>>      len = readlink(path, iommu_group_path, sizeof(path));
>> @@ -4183,12 +4241,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;
>> @@ -4203,7 +4257,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)) {
>> @@ -4291,7 +4345,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);
>> @@ -4317,8 +4371,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)) {
>>          DPRINTF("%04x:%02x:%02x.%x FLR/VFIO_DEVICE_RESET\n", 
>> vdev->host.domain,
>>              vdev->host.bus, vdev->host.slot, vdev->host.function);
>>          goto post_reset;
>> @@ -4330,8 +4385,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)) {
>>          DPRINTF("%04x:%02x:%02x.%x PCI PM Reset\n", vdev->host.domain,
>>              vdev->host.bus, vdev->host.slot, vdev->host.function);
>>          goto post_reset;
> 
> 
> 




reply via email to

[Prev in Thread] Current Thread [Next in Thread]