| Message ID | 1423051870-26473-2-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 2015-02-04 at 13:11 +0100, Paolo Bonzini wrote: > With the next patch vfio_put_base_device will be called unconditionally at > instance_finalize time, which will mean calling it twice if vfio_populate_device > fails. This works, but it is slightly harder to follow. > > Change vfio_get_device to not touch the vbasedev struct until it will > definitely succeed, moving the vfio_populate_device call back to vfio-pci. > This way, vfio_put_base_device will only be called once and only on > non-error paths. > > Cc: Alex Williamson <alex.williamson@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/vfio/common.c | 31 ++++++++++++------------------- > hw/vfio/pci.c | 11 +++++++---- > include/hw/vfio/vfio-common.h | 1 - > 3 files changed, 19 insertions(+), 24 deletions(-) > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index cf483ff..242b71d 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -867,27 +867,28 @@ int vfio_get_device(VFIOGroup *group, const char *name, > VFIODevice *vbasedev) > { > struct vfio_device_info dev_info = { .argsz = sizeof(dev_info) }; > - int ret; > + int ret, fd; > > - ret = ioctl(group->fd, VFIO_GROUP_GET_DEVICE_FD, name); > - if (ret < 0) { > + fd = ioctl(group->fd, VFIO_GROUP_GET_DEVICE_FD, name); > + if (fd < 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-<bus> " > "or pci-stub and not already in use\n", group->groupid); > - return ret; > + return fd; > } > > - vbasedev->fd = ret; > - vbasedev->group = group; > - QLIST_INSERT_HEAD(&group->device_list, vbasedev, next); > - > - ret = ioctl(vbasedev->fd, VFIO_DEVICE_GET_INFO, &dev_info); > + ret = ioctl(fd, VFIO_DEVICE_GET_INFO, &dev_info); > if (ret) { > error_report("vfio: error getting device info: %m"); > - goto error; > + close(fd); > + return ret; > } > > + vbasedev->fd = fd; > + vbasedev->group = group; > + QLIST_INSERT_HEAD(&group->device_list, vbasedev, next); > + > vbasedev->num_irqs = dev_info.num_irqs; > vbasedev->num_regions = dev_info.num_regions; > vbasedev->flags = dev_info.flags; > @@ -896,20 +897,12 @@ int vfio_get_device(VFIOGroup *group, const char *name, > dev_info.num_irqs); > > vbasedev->reset_works = !!(dev_info.flags & VFIO_DEVICE_FLAGS_RESET); > - > - ret = vbasedev->ops->vfio_populate_device(vbasedev); > - > -error: > - if (ret) { > - vfio_put_base_device(vbasedev); > - } > - return ret; > + return 0; > } > > void vfio_put_base_device(VFIODevice *vbasedev) > { > QLIST_REMOVE(vbasedev, next); > - vbasedev->group = NULL; I can't figure out why this is necessary. If we don't instantiate a vfio device, then group will be NULL, which is what's used in the next patch to filter certain code paths, including this one. It would be just as incorrect to call those code paths on a finalized device, so why do we not clear this? Otherwise the series appears reasonable to me. Thanks, Alex > trace_vfio_put_base_device(vbasedev->fd); > close(vbasedev->fd); > }
On 04/02/2015 19:22, Alex Williamson wrote: > On Wed, 2015-02-04 at 13:11 +0100, Paolo Bonzini wrote: >> With the next patch vfio_put_base_device will be called unconditionally at >> instance_finalize time, which will mean calling it twice if vfio_populate_device >> fails. This works, but it is slightly harder to follow. >> >> Change vfio_get_device to not touch the vbasedev struct until it will >> definitely succeed, moving the vfio_populate_device call back to vfio-pci. >> This way, vfio_put_base_device will only be called once and only on >> non-error paths. >> >> Cc: Alex Williamson <alex.williamson@redhat.com> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> hw/vfio/common.c | 31 ++++++++++++------------------- >> hw/vfio/pci.c | 11 +++++++---- >> include/hw/vfio/vfio-common.h | 1 - >> 3 files changed, 19 insertions(+), 24 deletions(-) >> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index cf483ff..242b71d 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -867,27 +867,28 @@ int vfio_get_device(VFIOGroup *group, const char *name, >> VFIODevice *vbasedev) >> { >> struct vfio_device_info dev_info = { .argsz = sizeof(dev_info) }; >> - int ret; >> + int ret, fd; >> >> - ret = ioctl(group->fd, VFIO_GROUP_GET_DEVICE_FD, name); >> - if (ret < 0) { >> + fd = ioctl(group->fd, VFIO_GROUP_GET_DEVICE_FD, name); >> + if (fd < 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-<bus> " >> "or pci-stub and not already in use\n", group->groupid); >> - return ret; >> + return fd; >> } >> >> - vbasedev->fd = ret; >> - vbasedev->group = group; >> - QLIST_INSERT_HEAD(&group->device_list, vbasedev, next); >> - >> - ret = ioctl(vbasedev->fd, VFIO_DEVICE_GET_INFO, &dev_info); >> + ret = ioctl(fd, VFIO_DEVICE_GET_INFO, &dev_info); >> if (ret) { >> error_report("vfio: error getting device info: %m"); >> - goto error; >> + close(fd); >> + return ret; >> } >> >> + vbasedev->fd = fd; >> + vbasedev->group = group; >> + QLIST_INSERT_HEAD(&group->device_list, vbasedev, next); >> + >> vbasedev->num_irqs = dev_info.num_irqs; >> vbasedev->num_regions = dev_info.num_regions; >> vbasedev->flags = dev_info.flags; >> @@ -896,20 +897,12 @@ int vfio_get_device(VFIOGroup *group, const char *name, >> dev_info.num_irqs); >> >> vbasedev->reset_works = !!(dev_info.flags & VFIO_DEVICE_FLAGS_RESET); >> - >> - ret = vbasedev->ops->vfio_populate_device(vbasedev); >> - >> -error: >> - if (ret) { >> - vfio_put_base_device(vbasedev); >> - } >> - return ret; >> + return 0; >> } >> >> void vfio_put_base_device(VFIODevice *vbasedev) >> { >> QLIST_REMOVE(vbasedev, next); >> - vbasedev->group = NULL; > > I can't figure out why this is necessary. If we don't instantiate a > vfio device, then group will be NULL, which is what's used in the next > patch to filter certain code paths, including this one. Oh, I thought that the purpose of the statement was just to undo the assignment in vfio_get_device. The free of vbasedev->group is not here, so I didn't recognize the pattern you are mentioning. Now that you mentioned it, it makes sense. It does work either with or without that line. v3 will have to wait for tomorrow though. :) Thanks for the quick review! Paolo > It would be > just as incorrect to call those code paths on a finalized device, so why > do we not clear this? Otherwise the series appears reasonable to me. > Thanks, > > Alex > >> trace_vfio_put_base_device(vbasedev->fd); >> close(vbasedev->fd); >> } > > > >
diff --git a/hw/vfio/common.c b/hw/vfio/common.c index cf483ff..242b71d 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -867,27 +867,28 @@ int vfio_get_device(VFIOGroup *group, const char *name, VFIODevice *vbasedev) { struct vfio_device_info dev_info = { .argsz = sizeof(dev_info) }; - int ret; + int ret, fd; - ret = ioctl(group->fd, VFIO_GROUP_GET_DEVICE_FD, name); - if (ret < 0) { + fd = ioctl(group->fd, VFIO_GROUP_GET_DEVICE_FD, name); + if (fd < 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-<bus> " "or pci-stub and not already in use\n", group->groupid); - return ret; + return fd; } - vbasedev->fd = ret; - vbasedev->group = group; - QLIST_INSERT_HEAD(&group->device_list, vbasedev, next); - - ret = ioctl(vbasedev->fd, VFIO_DEVICE_GET_INFO, &dev_info); + ret = ioctl(fd, VFIO_DEVICE_GET_INFO, &dev_info); if (ret) { error_report("vfio: error getting device info: %m"); - goto error; + close(fd); + return ret; } + vbasedev->fd = fd; + vbasedev->group = group; + QLIST_INSERT_HEAD(&group->device_list, vbasedev, next); + vbasedev->num_irqs = dev_info.num_irqs; vbasedev->num_regions = dev_info.num_regions; vbasedev->flags = dev_info.flags; @@ -896,20 +897,12 @@ int vfio_get_device(VFIOGroup *group, const char *name, dev_info.num_irqs); vbasedev->reset_works = !!(dev_info.flags & VFIO_DEVICE_FLAGS_RESET); - - ret = vbasedev->ops->vfio_populate_device(vbasedev); - -error: - if (ret) { - vfio_put_base_device(vbasedev); - } - return ret; + return 0; } void vfio_put_base_device(VFIODevice *vbasedev) { QLIST_REMOVE(vbasedev, next); - vbasedev->group = NULL; trace_vfio_put_base_device(vbasedev->fd); close(vbasedev->fd); } diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 014a92c..2f5f718 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -195,7 +195,6 @@ 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 int vfio_populate_device(VFIODevice *vbasedev); /* * Disabling BAR mmaping can be slow, but toggling it around INTx can @@ -2934,12 +2933,11 @@ 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_populate_device = vfio_populate_device, }; -static int vfio_populate_device(VFIODevice *vbasedev) +static int vfio_pci_populate_device(VFIOPCIDevice *vdev) { - VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev); + VFIODevice *vbasedev = &vdev->vbasedev; struct vfio_region_info reg_info = { .argsz = sizeof(reg_info) }; struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) }; int i, ret = -1; @@ -3247,6 +3245,11 @@ static int vfio_initfn(PCIDevice *pdev) return ret; } + ret = vfio_pci_populate_device(vdev); + if (ret) { + return ret; + } + /* Get a copy of config space */ ret = pread(vdev->vbasedev.fd, vdev->pdev.config, MIN(pci_config_size(&vdev->pdev), vdev->config_size), diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h index 1d5bfe8..5f3679b 100644 --- a/include/hw/vfio/vfio-common.h +++ b/include/hw/vfio/vfio-common.h @@ -112,7 +112,6 @@ struct VFIODeviceOps { void (*vfio_compute_needs_reset)(VFIODevice *vdev); int (*vfio_hot_reset_multi)(VFIODevice *vdev); void (*vfio_eoi)(VFIODevice *vdev); - int (*vfio_populate_device)(VFIODevice *vdev); }; typedef struct VFIOGroup {
With the next patch vfio_put_base_device will be called unconditionally at instance_finalize time, which will mean calling it twice if vfio_populate_device fails. This works, but it is slightly harder to follow. Change vfio_get_device to not touch the vbasedev struct until it will definitely succeed, moving the vfio_populate_device call back to vfio-pci. This way, vfio_put_base_device will only be called once and only on non-error paths. Cc: Alex Williamson <alex.williamson@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/vfio/common.c | 31 ++++++++++++------------------- hw/vfio/pci.c | 11 +++++++---- include/hw/vfio/vfio-common.h | 1 - 3 files changed, 19 insertions(+), 24 deletions(-)