diff mbox

[v5,05/10] hw/vfio/pci: split vfio_get_device

Message ID 1407594349-9291-6-git-send-email-eric.auger@linaro.org
State New
Headers show

Commit Message

Auger Eric Aug. 9, 2014, 2:25 p.m. UTC 
vfio_get_device now takes a VFIODevice as argument. The function is split
into 4 functional parts: dev_info query, device check, region populate
and interrupt populate. the last 3 are specialized by parent device and
are added into DeviceOps.

3 new fields are introduced in VFIODevice to store dev_info.

vfio_put_base_device is created.

---

v4->v5:
- cleanup up of error handling and get/put operations in
  vfio_check_device, vfio_populate_regions, vfio_populate_interrupts and
  vfio_get_device.
  - correct misuse of errno
  - vfio_populate_regions always returns 0
  - VFIODevice .name deallocation done in vfio_put_device instead of
    vfio_put_base_device
  - vfio_put_base_device done at vfio_get_device level.

Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
 hw/vfio/pci.c | 181 ++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 120 insertions(+), 61 deletions(-)

Comments

David Gibson Aug. 12, 2014, 2:41 a.m. UTC | #1 
On Sat, Aug 09, 2014 at 03:25:44PM +0100, Eric Auger wrote:
> vfio_get_device now takes a VFIODevice as argument. The function is split
> into 4 functional parts: dev_info query, device check, region populate
> and interrupt populate. the last 3 are specialized by parent device and
> are added into DeviceOps.

Why is splitting these up into 4 stages useful, rather than having a
single sub-class specific callback?
Auger Eric Aug. 12, 2014, 6:54 a.m. UTC | #2 
On 08/12/2014 04:41 AM, David Gibson wrote:
> On Sat, Aug 09, 2014 at 03:25:44PM +0100, Eric Auger wrote:
>> vfio_get_device now takes a VFIODevice as argument. The function is split
>> into 4 functional parts: dev_info query, device check, region populate
>> and interrupt populate. the last 3 are specialized by parent device and
>> are added into DeviceOps.
> 
> Why is splitting these up into 4 stages useful, rather than having a
> single sub-class specific callback?

Hi David,

VFIOPlatformDevice already inherits from SysBusDevice and hence cannot
inherit from another VFIODevice. Same for VFIOPCIDevice that inherits
from PCIDevice. This is why I created this non QOM struct. But did you
mean something else?

Then splitting into 4: This was to share some code between platform and
PCI (dev_info query) and vfio_get_device was quite big already. I
thought it makes sense to split it into functional parts.

Best Regards

Eric
>
David Gibson Aug. 13, 2014, 3:32 a.m. UTC | #3 
On Tue, Aug 12, 2014 at 08:54:34AM +0200, Eric Auger wrote:
> On 08/12/2014 04:41 AM, David Gibson wrote:
> > On Sat, Aug 09, 2014 at 03:25:44PM +0100, Eric Auger wrote:
> >> vfio_get_device now takes a VFIODevice as argument. The function is split
> >> into 4 functional parts: dev_info query, device check, region populate
> >> and interrupt populate. the last 3 are specialized by parent device and
> >> are added into DeviceOps.
> > 
> > Why is splitting these up into 4 stages useful, rather than having a
> > single sub-class specific callback?
> 
> Hi David,
> 
> VFIOPlatformDevice already inherits from SysBusDevice and hence cannot
> inherit from another VFIODevice. Same for VFIOPCIDevice that inherits
> from PCIDevice. This is why I created this non QOM struct. But did you
> mean something else?

Ah, yes, sorry, I missed that, though it's obvious now I think about
it.

> Then splitting into 4: This was to share some code between platform and
> PCI (dev_info query) and vfio_get_device was quite big already. I
> thought it makes sense to split it into functional parts.

Hm, ok.  So splitting out dev_info_query certainly makes sense then.
But does splitting the two populate sections make sense?  Is it
plausible that two different VFIO capable busses would share one of
these functions but not the other?
Auger Eric Aug. 29, 2014, 10 a.m. UTC | #4 
On 08/13/2014 05:32 AM, David Gibson wrote:
> On Tue, Aug 12, 2014 at 08:54:34AM +0200, Eric Auger wrote:
>> On 08/12/2014 04:41 AM, David Gibson wrote:
>>> On Sat, Aug 09, 2014 at 03:25:44PM +0100, Eric Auger wrote:
>>>> vfio_get_device now takes a VFIODevice as argument. The function is split
>>>> into 4 functional parts: dev_info query, device check, region populate
>>>> and interrupt populate. the last 3 are specialized by parent device and
>>>> are added into DeviceOps.
>>>
>>> Why is splitting these up into 4 stages useful, rather than having a
>>> single sub-class specific callback?
>>
>> Hi David,
>>
>> VFIOPlatformDevice already inherits from SysBusDevice and hence cannot
>> inherit from another VFIODevice. Same for VFIOPCIDevice that inherits
>> from PCIDevice. This is why I created this non QOM struct. But did you
>> mean something else?
> 
> Ah, yes, sorry, I missed that, though it's obvious now I think about
> it.
> 
>> Then splitting into 4: This was to share some code between platform and
>> PCI (dev_info query) and vfio_get_device was quite big already. I
>> thought it makes sense to split it into functional parts.
> 
> Hm, ok.  So splitting out dev_info_query certainly makes sense then.
> But does splitting the two populate sections make sense?  Is it
> plausible that two different VFIO capable busses would share one of
> these functions but not the other?

Hi David,

Coming back to you on that topic. There is no other justification for
splitting the code into 3 functions except than having shorter functions
with reduced functionality. But I acknowledge it would simplify the diff
between original code and new one so I intend to keep a single
specialized functions instead of 3.

Best Regards

Eric

>
diff mbox

Patch

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 1a24398..5f218b7 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -213,12 +213,18 @@  typedef struct VFIODevice {
     bool reset_works;
     bool needs_reset;
     VFIODeviceOps *ops;
+    unsigned int num_irqs;
+    unsigned int num_regions;
+    unsigned int flags;
 } VFIODevice;
 
 struct VFIODeviceOps {
     bool (*vfio_compute_needs_reset)(VFIODevice *vdev);
     int (*vfio_hot_reset_multi)(VFIODevice *vdev);
     void (*vfio_eoi)(VFIODevice *vdev);
+    int (*vfio_check_device)(VFIODevice *vdev);
+    int (*vfio_populate_regions)(VFIODevice *vdev);
+    int (*vfio_populate_interrupts)(VFIODevice *vdev);
 };
 
 typedef struct VFIOPCIDevice {
@@ -305,6 +311,10 @@  static uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
 static void vfio_pci_write_config(PCIDevice *pdev, uint32_t addr,
                                   uint32_t val, int len);
 static void vfio_mmap_set_enabled(VFIOPCIDevice *vdev, bool enabled);
+static void vfio_put_base_device(VFIODevice *vbasedev);
+static int vfio_check_device(VFIODevice *vbasedev);
+static int vfio_populate_regions(VFIODevice *vbasedev);
+static int vfio_populate_interrupts(VFIODevice *vbasedev);
 
 /*
  * Common VFIO interrupt disable
@@ -3608,6 +3618,9 @@  static VFIODeviceOps vfio_pci_ops = {
     .vfio_compute_needs_reset = vfio_pci_compute_needs_reset,
     .vfio_hot_reset_multi = vfio_pci_hot_reset_multi,
     .vfio_eoi = vfio_eoi,
+    .vfio_check_device = vfio_check_device,
+    .vfio_populate_regions = vfio_populate_regions,
+    .vfio_populate_interrupts = vfio_populate_interrupts,
 };
 
 static void vfio_reset_handler(void *opaque)
@@ -3949,54 +3962,52 @@  static void vfio_put_group(VFIOGroup *group)
     }
 }
 
-static int vfio_get_device(VFIOGroup *group, const char *name,
-                           VFIOPCIDevice *vdev)
+static int vfio_check_device(VFIODevice *vbasedev)
 {
-    struct vfio_device_info dev_info = { .argsz = sizeof(dev_info) };
-    struct vfio_region_info reg_info = { .argsz = sizeof(reg_info) };
-    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) };
-    int ret, i;
-
-    ret = ioctl(group->fd, VFIO_GROUP_GET_DEVICE_FD, name);
-    if (ret < 0) {
-        error_report("vfio: error getting device %s from group %d: %m",
-                     name, group->groupid);
-        error_printf("Verify all devices in group %d are bound to vfio-pci "
-                     "or pci-stub and not already in use\n", group->groupid);
-        return ret;
+    if (!(vbasedev->flags & VFIO_DEVICE_FLAGS_PCI)) {
+        error_report("vfio: Um, this isn't a PCI device");
+        goto error;
     }
-
-    vdev->vbasedev.fd = ret;
-    vdev->vbasedev.group = group;
-    QLIST_INSERT_HEAD(&group->device_list, &vdev->vbasedev, next);
-
-    /* Sanity check device */
-    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_INFO, &dev_info);
-    if (ret) {
-        error_report("vfio: error getting device info: %m");
+    if (vbasedev->num_regions < VFIO_PCI_CONFIG_REGION_INDEX + 1) {
+        error_report("vfio: unexpected number of io regions %u",
+                     vbasedev->num_regions);
         goto error;
     }
-
-    DPRINTF("Device %s flags: %u, regions: %u, irgs: %u\n", name,
-            dev_info.flags, dev_info.num_regions, dev_info.num_irqs);
-
-    if (!(dev_info.flags & VFIO_DEVICE_FLAGS_PCI)) {
-        error_report("vfio: Um, this isn't a PCI device");
+    if (vbasedev->num_irqs < VFIO_PCI_MSIX_IRQ_INDEX + 1) {
+        error_report("vfio: unexpected number of irqs %u",
+                     vbasedev->num_irqs);
         goto error;
     }
+    return 0;
+error:
+    return -1;
+}
 
-    vdev->vbasedev.reset_works = !!(dev_info.flags & VFIO_DEVICE_FLAGS_RESET);
+static int vfio_populate_interrupts(VFIODevice *vbasedev)
+{
+    VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
+    int ret;
+    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) };
+    irq_info.index = VFIO_PCI_ERR_IRQ_INDEX;
 
-    if (dev_info.num_regions < VFIO_PCI_CONFIG_REGION_INDEX + 1) {
-        error_report("vfio: unexpected number of io regions %u",
-                     dev_info.num_regions);
-        goto error;
+    ret = ioctl(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");
+    } else if (irq_info.count == 1) {
+        vdev->pci_aer = true;
+    } else {
+        error_report("vfio: %s Could not enable error recovery for the device",
+                     vbasedev->name);
     }
+    return 0;
+}
 
-    if (dev_info.num_irqs < VFIO_PCI_MSIX_IRQ_INDEX + 1) {
-        error_report("vfio: unexpected number of irqs %u", dev_info.num_irqs);
-        goto error;
-    }
+static int vfio_populate_regions(VFIODevice *vbasedev)
+{
+    struct vfio_region_info reg_info = { .argsz = sizeof(reg_info) };
+    int i, ret = 0;
+    VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
 
     for (i = VFIO_PCI_BAR0_REGION_INDEX; i < VFIO_PCI_ROM_REGION_INDEX; i++) {
         reg_info.index = i;
@@ -4007,7 +4018,7 @@  static int vfio_get_device(VFIOGroup *group, const char *name,
             goto error;
         }
 
-        DPRINTF("Device %s region %d:\n", name, i);
+        DPRINTF("Device %s region %d:\n", vbasedev->name, i);
         DPRINTF("  size: 0x%lx, offset: 0x%lx, flags: 0x%lx\n",
                 (unsigned long)reg_info.size, (unsigned long)reg_info.offset,
                 (unsigned long)reg_info.flags);
@@ -4028,7 +4039,7 @@  static int vfio_get_device(VFIOGroup *group, const char *name,
         goto error;
     }
 
-    DPRINTF("Device %s config:\n", name);
+    DPRINTF("Device %s config:\n", vbasedev->name);
     DPRINTF("  size: 0x%lx, offset: 0x%lx, flags: 0x%lx\n",
             (unsigned long)reg_info.size, (unsigned long)reg_info.offset,
             (unsigned long)reg_info.flags);
@@ -4040,7 +4051,7 @@  static int vfio_get_device(VFIOGroup *group, const char *name,
     vdev->config_offset = reg_info.offset;
 
     if ((vdev->features & VFIO_FEATURE_ENABLE_VGA) &&
-        dev_info.num_regions > VFIO_PCI_VGA_REGION_INDEX) {
+        vbasedev->num_regions > VFIO_PCI_VGA_REGION_INDEX) {
         struct vfio_region_info vga_info = {
             .argsz = sizeof(vga_info),
             .index = VFIO_PCI_VGA_REGION_INDEX,
@@ -4059,6 +4070,7 @@  static int vfio_get_device(VFIOGroup *group, const char *name,
             error_report("vfio: Unexpected VGA info, flags 0x%lx, size 0x%lx",
                          (unsigned long)vga_info.flags,
                          (unsigned long)vga_info.size);
+            ret = -1;
             goto error;
         }
 
@@ -4079,42 +4091,89 @@  static int vfio_get_device(VFIOGroup *group, const char *name,
 
         vdev->has_vga = true;
     }
-    irq_info.index = VFIO_PCI_ERR_IRQ_INDEX;
+error:
+    return ret;
+}
+
+static int vfio_get_device(VFIOGroup *group, const char *name,
+                           VFIODevice *vbasedev)
+{
+    struct vfio_device_info dev_info = { .argsz = sizeof(dev_info) };
+    struct vfio_region_info reg_info = { .argsz = sizeof(reg_info) };
+    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) };
+    int ret;
 
-    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
+    ret = ioctl(group->fd, VFIO_GROUP_GET_DEVICE_FD, name);
+    if (ret < 0) {
+        error_report("vfio: error getting device %s from group %d: %m",
+                     name, group->groupid);
+        error_printf("Verify all devices in group %d are bound to vfio-pci "
+                     "or pci-stub and not already in use\n", group->groupid);
+        return ret;
+    }
+
+    vbasedev->fd = ret;
+    vbasedev->group = group;
+    QLIST_INSERT_HEAD(&group->device_list, vbasedev, next);
+
+    ret = ioctl(vbasedev->fd, VFIO_DEVICE_GET_INFO, &dev_info);
     if (ret) {
-        /* This can fail for an old kernel or legacy PCI dev */
-        DPRINTF("VFIO_DEVICE_GET_IRQ_INFO failure: %m\n");
-        ret = 0;
-    } else if (irq_info.count == 1) {
-        vdev->pci_aer = true;
-    } else {
-        error_report("vfio: %04x:%02x:%02x.%x "
-                     "Could not enable error recovery for the device",
-                     vdev->host.domain, vdev->host.bus, vdev->host.slot,
-                     vdev->host.function);
+        error_report("vfio: error getting device info: %m");
+        goto error;
+    }
+
+    vbasedev->num_irqs = dev_info.num_irqs;
+    vbasedev->num_regions = dev_info.num_regions;
+    vbasedev->flags = dev_info.flags;
+
+    DPRINTF("Device %s flags: %u, regions: %u, irgs: %u\n", name,
+            dev_info.flags, dev_info.num_regions, dev_info.num_irqs);
+
+    vbasedev->reset_works = !!(dev_info.flags & VFIO_DEVICE_FLAGS_RESET);
+
+    /* call device specific functions */
+    ret = vbasedev->ops->vfio_check_device(vbasedev);
+    if (ret) {
+        error_report("vfio: error when checking device %s\n",
+                     vbasedev->name);
+        goto error;
+    }
+    ret = vbasedev->ops->vfio_populate_regions(vbasedev);
+    if (ret) {
+        error_report("vfio: error when populating regions of device %s\n",
+                     vbasedev->name);
+        goto error;
+    }
+    ret = vbasedev->ops->vfio_populate_interrupts(vbasedev);
+    if (ret) {
+        error_report("vfio: error when populating interrupts of device %s\n",
+                     vbasedev->name);
+        goto error;
     }
 
 error:
     if (ret) {
-        QLIST_REMOVE(&vdev->vbasedev, next);
-        vdev->vbasedev.group = NULL;
-        close(vdev->vbasedev.fd);
+        vfio_put_base_device(vbasedev);
     }
     return ret;
 }
 
+void vfio_put_base_device(VFIODevice *vbasedev)
+{
+    QLIST_REMOVE(vbasedev, next);
+    vbasedev->group = NULL;
+    DPRINTF("vfio_put_base_device: close vdev->fd\n");
+    close(vbasedev->fd);
+}
+
 static void vfio_put_device(VFIOPCIDevice *vdev)
 {
-    QLIST_REMOVE(&vdev->vbasedev, next);
-    vdev->vbasedev.group = NULL;
-    DPRINTF("vfio_put_device: close vdev->fd\n");
-    close(vdev->vbasedev.fd);
     g_free(vdev->vbasedev.name);
     if (vdev->msix) {
         g_free(vdev->msix);
         vdev->msix = NULL;
     }
+    vfio_put_base_device(&vdev->vbasedev);
 }
 
 static void vfio_err_notifier_handler(void *opaque)
@@ -4287,7 +4346,7 @@  static int vfio_initfn(PCIDevice *pdev)
         }
     }
 
-    ret = vfio_get_device(group, path, vdev);
+    ret = vfio_get_device(group, path, &vdev->vbasedev);
     if (ret) {
         error_report("vfio: failed to get device %s", path);
         vfio_put_group(group);