| Message ID | 20231003101530.288864-10-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Prerequisite changes for IOMMUFD support | expand |
On 10/3/23 12:14, Eric Auger wrote: > Let the vfio-ap device use vfio_attach_device() and > vfio_detach_device(), hence hiding the details of the used > IOMMU backend. > > We take the opportunity to use g_path_get_basename() which > is prefered, as suggested by > 3e015d815b ("use g_path_get_basename instead of basename") > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > > --- > > v2 -> v3: > - Mention g_path_get_basename in commit message and properly free > vbasedev->name, call vfio_detach_device > --- > hw/vfio/ap.c | 70 ++++++++++------------------------------------------ > 1 file changed, 13 insertions(+), 57 deletions(-) > > diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c > index 6e21d1da5a..d0b587b3b1 100644 > --- a/hw/vfio/ap.c > +++ b/hw/vfio/ap.c > @@ -53,40 +53,6 @@ struct VFIODeviceOps vfio_ap_ops = { > .vfio_compute_needs_reset = vfio_ap_compute_needs_reset, > }; > > -static void vfio_ap_put_device(VFIOAPDevice *vapdev) > -{ > - g_free(vapdev->vdev.name); > - vfio_put_base_device(&vapdev->vdev); > -} > - > -static VFIOGroup *vfio_ap_get_group(VFIOAPDevice *vapdev, Error **errp) > -{ > - GError *gerror = NULL; > - char *symlink, *group_path; > - int groupid; > - > - symlink = g_strdup_printf("%s/iommu_group", vapdev->vdev.sysfsdev); > - group_path = g_file_read_link(symlink, &gerror); > - g_free(symlink); > - > - if (!group_path) { > - error_setg(errp, "%s: no iommu_group found for %s: %s", > - TYPE_VFIO_AP_DEVICE, vapdev->vdev.sysfsdev, gerror->message); > - g_error_free(gerror); > - return NULL; > - } > - > - if (sscanf(basename(group_path), "%d", &groupid) != 1) { > - error_setg(errp, "vfio: failed to read %s", group_path); > - g_free(group_path); > - return NULL; > - } > - > - g_free(group_path); > - > - return vfio_get_group(groupid, &address_space_memory, errp); > -} > - > static void vfio_ap_req_notifier_handler(void *opaque) > { > VFIOAPDevice *vapdev = opaque; > @@ -189,22 +155,15 @@ static void vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev, > static void vfio_ap_realize(DeviceState *dev, Error **errp) > { > int ret; > - char *mdevid; > Error *err = NULL; > - VFIOGroup *vfio_group; > APDevice *apdev = AP_DEVICE(dev); > VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev); > + VFIODevice *vbasedev = &vapdev->vdev; > > - vfio_group = vfio_ap_get_group(vapdev, errp); > - if (!vfio_group) { > - return; > - } > - > - vapdev->vdev.ops = &vfio_ap_ops; > - vapdev->vdev.type = VFIO_DEVICE_TYPE_AP; > - mdevid = basename(vapdev->vdev.sysfsdev); > - vapdev->vdev.name = g_strdup_printf("%s", mdevid); > - vapdev->vdev.dev = dev; > + vbasedev->name = g_path_get_basename(vbasedev->sysfsdev); > + vbasedev->ops = &vfio_ap_ops; > + vbasedev->type = VFIO_DEVICE_TYPE_AP; > + vbasedev->dev = dev; > > /* > * vfio-ap devices operate in a way compatible with discarding of > @@ -214,9 +173,11 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp) > */ > vapdev->vdev.ram_block_discard_allowed = true; > > - ret = vfio_get_device(vfio_group, mdevid, &vapdev->vdev, errp); > + ret = vfio_attach_device(vbasedev->name, vbasedev, > + &address_space_memory, errp); > if (ret) { > - goto out_get_dev_err; > + g_free(vbasedev->name); > + return; > } > > vfio_ap_register_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX, &err); > @@ -225,25 +186,20 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp) > * Report this error, but do not make it a failing condition. > * Lack of this IRQ in the host does not prevent normal operation. > */ > + vfio_detach_device(vbasedev); > error_report_err(err); > + g_free(vbasedev->name); > } > - > - return; > - > -out_get_dev_err: > - vfio_ap_put_device(vapdev); > - vfio_put_group(vfio_group); > } To be consistent with vfio_(pci)_realize(), I would introduce the same failure path at the end the routine : out_detach: vfio_detach_device(vbasedev); error: error_prepend(errp, VFIO_MSG_PREFIX, vbasedev->name); g_free(vbasedev->name); and add the VFIO_MSG_PREFIX while we are at it. This is minor, so : Reviewed-by: Cédric Le Goater <clg@redhat.com> Thanks, C. > > static void vfio_ap_unrealize(DeviceState *dev) > { > APDevice *apdev = AP_DEVICE(dev); > VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev); > - VFIOGroup *group = vapdev->vdev.group; > > vfio_ap_unregister_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX); > - vfio_ap_put_device(vapdev); > - vfio_put_group(group); > + vfio_detach_device(&vapdev->vdev); > + g_free(vapdev->vdev.name); > } > > static Property vfio_ap_properties[] = {
On 10/3/23 11:25 AM, Cédric Le Goater wrote: > On 10/3/23 12:14, Eric Auger wrote: >> Let the vfio-ap device use vfio_attach_device() and >> vfio_detach_device(), hence hiding the details of the used >> IOMMU backend. >> >> We take the opportunity to use g_path_get_basename() which >> is prefered, as suggested by >> 3e015d815b ("use g_path_get_basename instead of basename") >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> >> --- >> >> v2 -> v3: >> - Mention g_path_get_basename in commit message and properly free >> vbasedev->name, call vfio_detach_device >> --- >> hw/vfio/ap.c | 70 ++++++++++------------------------------------------ >> 1 file changed, 13 insertions(+), 57 deletions(-) >> >> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c >> index 6e21d1da5a..d0b587b3b1 100644 >> --- a/hw/vfio/ap.c >> +++ b/hw/vfio/ap.c >> @@ -53,40 +53,6 @@ struct VFIODeviceOps vfio_ap_ops = { >> .vfio_compute_needs_reset = vfio_ap_compute_needs_reset, >> }; >> -static void vfio_ap_put_device(VFIOAPDevice *vapdev) >> -{ >> - g_free(vapdev->vdev.name); >> - vfio_put_base_device(&vapdev->vdev); >> -} >> - >> -static VFIOGroup *vfio_ap_get_group(VFIOAPDevice *vapdev, Error **errp) >> -{ >> - GError *gerror = NULL; >> - char *symlink, *group_path; >> - int groupid; >> - >> - symlink = g_strdup_printf("%s/iommu_group", vapdev->vdev.sysfsdev); >> - group_path = g_file_read_link(symlink, &gerror); >> - g_free(symlink); >> - >> - if (!group_path) { >> - error_setg(errp, "%s: no iommu_group found for %s: %s", >> - TYPE_VFIO_AP_DEVICE, vapdev->vdev.sysfsdev, gerror->message); >> - g_error_free(gerror); >> - return NULL; >> - } >> - >> - if (sscanf(basename(group_path), "%d", &groupid) != 1) { >> - error_setg(errp, "vfio: failed to read %s", group_path); >> - g_free(group_path); >> - return NULL; >> - } >> - >> - g_free(group_path); >> - >> - return vfio_get_group(groupid, &address_space_memory, errp); >> -} >> - >> static void vfio_ap_req_notifier_handler(void *opaque) >> { >> VFIOAPDevice *vapdev = opaque; >> @@ -189,22 +155,15 @@ static void vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev, >> static void vfio_ap_realize(DeviceState *dev, Error **errp) >> { >> int ret; >> - char *mdevid; >> Error *err = NULL; >> - VFIOGroup *vfio_group; >> APDevice *apdev = AP_DEVICE(dev); >> VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev); >> + VFIODevice *vbasedev = &vapdev->vdev; >> - vfio_group = vfio_ap_get_group(vapdev, errp); >> - if (!vfio_group) { >> - return; >> - } >> - >> - vapdev->vdev.ops = &vfio_ap_ops; >> - vapdev->vdev.type = VFIO_DEVICE_TYPE_AP; >> - mdevid = basename(vapdev->vdev.sysfsdev); >> - vapdev->vdev.name = g_strdup_printf("%s", mdevid); >> - vapdev->vdev.dev = dev; >> + vbasedev->name = g_path_get_basename(vbasedev->sysfsdev); >> + vbasedev->ops = &vfio_ap_ops; >> + vbasedev->type = VFIO_DEVICE_TYPE_AP; >> + vbasedev->dev = dev; >> /* >> * vfio-ap devices operate in a way compatible with discarding of >> @@ -214,9 +173,11 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp) >> */ >> vapdev->vdev.ram_block_discard_allowed = true; >> - ret = vfio_get_device(vfio_group, mdevid, &vapdev->vdev, errp); >> + ret = vfio_attach_device(vbasedev->name, vbasedev, >> + &address_space_memory, errp); >> if (ret) { >> - goto out_get_dev_err; >> + g_free(vbasedev->name); >> + return; >> } >> vfio_ap_register_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX, &err); >> @@ -225,25 +186,20 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp) >> * Report this error, but do not make it a failing condition. >> * Lack of this IRQ in the host does not prevent normal operation. >> */ >> + vfio_detach_device(vbasedev); >> error_report_err(err); >> + g_free(vbasedev->name); This patch overall looks good to me and passes basic tests with vfio-ap devices. But I note that this addition of detach+free here runs counter to what the comment block above it states and prior behavior (where we did not goto out_get_dev_err for this case and expect the realize to complete successfully despite this error). In this error case, we only report the local 'err' contents and nothing is propagated into 'errp' -- which means that to the caller dc->realize() should be viewed as successful (errp is NULL) and so we should be able to assume a subsequent dc->unrealize() will do this g_free+detach later. >> } >> - >> - return; >> - >> -out_get_dev_err: >> - vfio_ap_put_device(vapdev); >> - vfio_put_group(vfio_group); >> } > > > To be consistent with vfio_(pci)_realize(), I would introduce the same > failure path at the end the routine : > > out_detach: > vfio_detach_device(vbasedev); > error: > error_prepend(errp, VFIO_MSG_PREFIX, vbasedev->name); > g_free(vbasedev->name); So based on my comment above, I think you'd only need the 'error:' case now, but otherwise adding this error_prepend seems reasonable to me too. Thanks, Matt > > > and add the VFIO_MSG_PREFIX while we are at it. > > This is minor, so : > > Reviewed-by: Cédric Le Goater <clg@redhat.com> > > Thanks, > > C. > > > >> static void vfio_ap_unrealize(DeviceState *dev) >> { >> APDevice *apdev = AP_DEVICE(dev); >> VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev); >> - VFIOGroup *group = vapdev->vdev.group; >> vfio_ap_unregister_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX); >> - vfio_ap_put_device(vapdev); >> - vfio_put_group(group); >> + vfio_detach_device(&vapdev->vdev); >> + g_free(vapdev->vdev.name); >> } >> static Property vfio_ap_properties[] = { >
Hi Matthew, On 10/4/23 01:08, Matthew Rosato wrote: > On 10/3/23 11:25 AM, Cédric Le Goater wrote: >> On 10/3/23 12:14, Eric Auger wrote: >>> Let the vfio-ap device use vfio_attach_device() and >>> vfio_detach_device(), hence hiding the details of the used >>> IOMMU backend. >>> >>> We take the opportunity to use g_path_get_basename() which >>> is prefered, as suggested by >>> 3e015d815b ("use g_path_get_basename instead of basename") >>> >>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>> >>> --- >>> >>> v2 -> v3: >>> - Mention g_path_get_basename in commit message and properly free >>> vbasedev->name, call vfio_detach_device >>> --- >>> hw/vfio/ap.c | 70 ++++++++++------------------------------------------ >>> 1 file changed, 13 insertions(+), 57 deletions(-) >>> >>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c >>> index 6e21d1da5a..d0b587b3b1 100644 >>> --- a/hw/vfio/ap.c >>> +++ b/hw/vfio/ap.c >>> @@ -53,40 +53,6 @@ struct VFIODeviceOps vfio_ap_ops = { >>> .vfio_compute_needs_reset = vfio_ap_compute_needs_reset, >>> }; >>> -static void vfio_ap_put_device(VFIOAPDevice *vapdev) >>> -{ >>> - g_free(vapdev->vdev.name); >>> - vfio_put_base_device(&vapdev->vdev); >>> -} >>> - >>> -static VFIOGroup *vfio_ap_get_group(VFIOAPDevice *vapdev, Error **errp) >>> -{ >>> - GError *gerror = NULL; >>> - char *symlink, *group_path; >>> - int groupid; >>> - >>> - symlink = g_strdup_printf("%s/iommu_group", vapdev->vdev.sysfsdev); >>> - group_path = g_file_read_link(symlink, &gerror); >>> - g_free(symlink); >>> - >>> - if (!group_path) { >>> - error_setg(errp, "%s: no iommu_group found for %s: %s", >>> - TYPE_VFIO_AP_DEVICE, vapdev->vdev.sysfsdev, gerror->message); >>> - g_error_free(gerror); >>> - return NULL; >>> - } >>> - >>> - if (sscanf(basename(group_path), "%d", &groupid) != 1) { >>> - error_setg(errp, "vfio: failed to read %s", group_path); >>> - g_free(group_path); >>> - return NULL; >>> - } >>> - >>> - g_free(group_path); >>> - >>> - return vfio_get_group(groupid, &address_space_memory, errp); >>> -} >>> - >>> static void vfio_ap_req_notifier_handler(void *opaque) >>> { >>> VFIOAPDevice *vapdev = opaque; >>> @@ -189,22 +155,15 @@ static void vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev, >>> static void vfio_ap_realize(DeviceState *dev, Error **errp) >>> { >>> int ret; >>> - char *mdevid; >>> Error *err = NULL; >>> - VFIOGroup *vfio_group; >>> APDevice *apdev = AP_DEVICE(dev); >>> VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev); >>> + VFIODevice *vbasedev = &vapdev->vdev; >>> - vfio_group = vfio_ap_get_group(vapdev, errp); >>> - if (!vfio_group) { >>> - return; >>> - } >>> - >>> - vapdev->vdev.ops = &vfio_ap_ops; >>> - vapdev->vdev.type = VFIO_DEVICE_TYPE_AP; >>> - mdevid = basename(vapdev->vdev.sysfsdev); >>> - vapdev->vdev.name = g_strdup_printf("%s", mdevid); >>> - vapdev->vdev.dev = dev; >>> + vbasedev->name = g_path_get_basename(vbasedev->sysfsdev); >>> + vbasedev->ops = &vfio_ap_ops; >>> + vbasedev->type = VFIO_DEVICE_TYPE_AP; >>> + vbasedev->dev = dev; >>> /* >>> * vfio-ap devices operate in a way compatible with discarding of >>> @@ -214,9 +173,11 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp) >>> */ >>> vapdev->vdev.ram_block_discard_allowed = true; >>> - ret = vfio_get_device(vfio_group, mdevid, &vapdev->vdev, errp); >>> + ret = vfio_attach_device(vbasedev->name, vbasedev, >>> + &address_space_memory, errp); >>> if (ret) { >>> - goto out_get_dev_err; >>> + g_free(vbasedev->name); >>> + return; >>> } >>> vfio_ap_register_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX, &err); >>> @@ -225,25 +186,20 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp) >>> * Report this error, but do not make it a failing condition. >>> * Lack of this IRQ in the host does not prevent normal operation. >>> */ >>> + vfio_detach_device(vbasedev); >>> error_report_err(err); >>> + g_free(vbasedev->name); > This patch overall looks good to me and passes basic tests with vfio-ap devices. But I note that this addition of detach+free here runs counter to what the comment block above it states and prior behavior (where we did not goto out_get_dev_err for this case and expect the realize to complete successfully despite this error). > > In this error case, we only report the local 'err' contents and nothing is propagated into 'errp' -- which means that to the caller dc->realize() should be viewed as successful (errp is NULL) and so we should be able to assume a subsequent dc->unrealize() will do this g_free+detach later. yes you totally right. Just need to do the error_report_err(err); in that case. Thanks! Eric > >>> } >>> - >>> - return; >>> - >>> -out_get_dev_err: >>> - vfio_ap_put_device(vapdev); >>> - vfio_put_group(vfio_group); >>> } >> >> To be consistent with vfio_(pci)_realize(), I would introduce the same >> failure path at the end the routine : >> >> out_detach: >> vfio_detach_device(vbasedev); >> error: >> error_prepend(errp, VFIO_MSG_PREFIX, vbasedev->name); >> g_free(vbasedev->name); > So based on my comment above, I think you'd only need the 'error:' case now, but otherwise adding this error_prepend seems reasonable to me too. > > Thanks, > Matt > >> >> and add the VFIO_MSG_PREFIX while we are at it. >> >> This is minor, so : >> >> Reviewed-by: Cédric Le Goater <clg@redhat.com> >> >> Thanks, >> >> C. >> >> >> >>> static void vfio_ap_unrealize(DeviceState *dev) >>> { >>> APDevice *apdev = AP_DEVICE(dev); >>> VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev); >>> - VFIOGroup *group = vapdev->vdev.group; >>> vfio_ap_unregister_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX); >>> - vfio_ap_put_device(vapdev); >>> - vfio_put_group(group); >>> + vfio_detach_device(&vapdev->vdev); >>> + g_free(vapdev->vdev.name); >>> } >>> static Property vfio_ap_properties[] = {
Hi Cédric, On 10/3/23 17:25, Cédric Le Goater wrote: > On 10/3/23 12:14, Eric Auger wrote: >> Let the vfio-ap device use vfio_attach_device() and >> vfio_detach_device(), hence hiding the details of the used >> IOMMU backend. >> >> We take the opportunity to use g_path_get_basename() which >> is prefered, as suggested by >> 3e015d815b ("use g_path_get_basename instead of basename") >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> >> --- >> >> v2 -> v3: >> - Mention g_path_get_basename in commit message and properly free >> vbasedev->name, call vfio_detach_device >> --- >> hw/vfio/ap.c | 70 ++++++++++------------------------------------------ >> 1 file changed, 13 insertions(+), 57 deletions(-) >> >> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c >> index 6e21d1da5a..d0b587b3b1 100644 >> --- a/hw/vfio/ap.c >> +++ b/hw/vfio/ap.c >> @@ -53,40 +53,6 @@ struct VFIODeviceOps vfio_ap_ops = { >> .vfio_compute_needs_reset = vfio_ap_compute_needs_reset, >> }; >> -static void vfio_ap_put_device(VFIOAPDevice *vapdev) >> -{ >> - g_free(vapdev->vdev.name); >> - vfio_put_base_device(&vapdev->vdev); >> -} >> - >> -static VFIOGroup *vfio_ap_get_group(VFIOAPDevice *vapdev, Error **errp) >> -{ >> - GError *gerror = NULL; >> - char *symlink, *group_path; >> - int groupid; >> - >> - symlink = g_strdup_printf("%s/iommu_group", vapdev->vdev.sysfsdev); >> - group_path = g_file_read_link(symlink, &gerror); >> - g_free(symlink); >> - >> - if (!group_path) { >> - error_setg(errp, "%s: no iommu_group found for %s: %s", >> - TYPE_VFIO_AP_DEVICE, vapdev->vdev.sysfsdev, >> gerror->message); >> - g_error_free(gerror); >> - return NULL; >> - } >> - >> - if (sscanf(basename(group_path), "%d", &groupid) != 1) { >> - error_setg(errp, "vfio: failed to read %s", group_path); >> - g_free(group_path); >> - return NULL; >> - } >> - >> - g_free(group_path); >> - >> - return vfio_get_group(groupid, &address_space_memory, errp); >> -} >> - >> static void vfio_ap_req_notifier_handler(void *opaque) >> { >> VFIOAPDevice *vapdev = opaque; >> @@ -189,22 +155,15 @@ static void >> vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev, >> static void vfio_ap_realize(DeviceState *dev, Error **errp) >> { >> int ret; >> - char *mdevid; >> Error *err = NULL; >> - VFIOGroup *vfio_group; >> APDevice *apdev = AP_DEVICE(dev); >> VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev); >> + VFIODevice *vbasedev = &vapdev->vdev; >> - vfio_group = vfio_ap_get_group(vapdev, errp); >> - if (!vfio_group) { >> - return; >> - } >> - >> - vapdev->vdev.ops = &vfio_ap_ops; >> - vapdev->vdev.type = VFIO_DEVICE_TYPE_AP; >> - mdevid = basename(vapdev->vdev.sysfsdev); >> - vapdev->vdev.name = g_strdup_printf("%s", mdevid); >> - vapdev->vdev.dev = dev; >> + vbasedev->name = g_path_get_basename(vbasedev->sysfsdev); >> + vbasedev->ops = &vfio_ap_ops; >> + vbasedev->type = VFIO_DEVICE_TYPE_AP; >> + vbasedev->dev = dev; >> /* >> * vfio-ap devices operate in a way compatible with discarding of >> @@ -214,9 +173,11 @@ static void vfio_ap_realize(DeviceState *dev, >> Error **errp) >> */ >> vapdev->vdev.ram_block_discard_allowed = true; >> - ret = vfio_get_device(vfio_group, mdevid, &vapdev->vdev, errp); >> + ret = vfio_attach_device(vbasedev->name, vbasedev, >> + &address_space_memory, errp); >> if (ret) { >> - goto out_get_dev_err; >> + g_free(vbasedev->name); >> + return; >> } >> vfio_ap_register_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX, >> &err); >> @@ -225,25 +186,20 @@ static void vfio_ap_realize(DeviceState *dev, >> Error **errp) >> * Report this error, but do not make it a failing condition. >> * Lack of this IRQ in the host does not prevent normal >> operation. >> */ >> + vfio_detach_device(vbasedev); >> error_report_err(err); >> + g_free(vbasedev->name); >> } >> - >> - return; >> - >> -out_get_dev_err: >> - vfio_ap_put_device(vapdev); >> - vfio_put_group(vfio_group); >> } > > > To be consistent with vfio_(pci)_realize(), I would introduce the same > failure path at the end the routine : > > out_detach: > vfio_detach_device(vbasedev); > error: > error_prepend(errp, VFIO_MSG_PREFIX, vbasedev->name); > g_free(vbasedev->name); > > > and add the VFIO_MSG_PREFIX while we are at it. following Matthew's comment we do not have any need for error handling in vfio_ap_realize() anymore. so just removing the improper additions: + vfio_detach_device(vbasedev); + g_free(vbasedev->name); Thanks Eric > > This is minor, so : > > Reviewed-by: Cédric Le Goater <clg@redhat.com> > > Thanks, > > C. > > > >> static void vfio_ap_unrealize(DeviceState *dev) >> { >> APDevice *apdev = AP_DEVICE(dev); >> VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev); >> - VFIOGroup *group = vapdev->vdev.group; >> vfio_ap_unregister_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX); >> - vfio_ap_put_device(vapdev); >> - vfio_put_group(group); >> + vfio_detach_device(&vapdev->vdev); >> + g_free(vapdev->vdev.name); >> } >> static Property vfio_ap_properties[] = { >
On 10/4/23 5:58 AM, Eric Auger wrote: > Hi Cédric, > > On 10/3/23 17:25, Cédric Le Goater wrote: >> On 10/3/23 12:14, Eric Auger wrote: >>> Let the vfio-ap device use vfio_attach_device() and >>> vfio_detach_device(), hence hiding the details of the used >>> IOMMU backend. >>> >>> We take the opportunity to use g_path_get_basename() which >>> is prefered, as suggested by >>> 3e015d815b ("use g_path_get_basename instead of basename") >>> >>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>> >>> --- >>> >>> v2 -> v3: >>> - Mention g_path_get_basename in commit message and properly free >>> vbasedev->name, call vfio_detach_device >>> --- >>> hw/vfio/ap.c | 70 ++++++++++------------------------------------------ >>> 1 file changed, 13 insertions(+), 57 deletions(-) >>> >>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c >>> index 6e21d1da5a..d0b587b3b1 100644 >>> --- a/hw/vfio/ap.c >>> +++ b/hw/vfio/ap.c >>> @@ -53,40 +53,6 @@ struct VFIODeviceOps vfio_ap_ops = { >>> .vfio_compute_needs_reset = vfio_ap_compute_needs_reset, >>> }; >>> -static void vfio_ap_put_device(VFIOAPDevice *vapdev) >>> -{ >>> - g_free(vapdev->vdev.name); >>> - vfio_put_base_device(&vapdev->vdev); >>> -} >>> - >>> -static VFIOGroup *vfio_ap_get_group(VFIOAPDevice *vapdev, Error **errp) >>> -{ >>> - GError *gerror = NULL; >>> - char *symlink, *group_path; >>> - int groupid; >>> - >>> - symlink = g_strdup_printf("%s/iommu_group", vapdev->vdev.sysfsdev); >>> - group_path = g_file_read_link(symlink, &gerror); >>> - g_free(symlink); >>> - >>> - if (!group_path) { >>> - error_setg(errp, "%s: no iommu_group found for %s: %s", >>> - TYPE_VFIO_AP_DEVICE, vapdev->vdev.sysfsdev, >>> gerror->message); >>> - g_error_free(gerror); >>> - return NULL; >>> - } >>> - >>> - if (sscanf(basename(group_path), "%d", &groupid) != 1) { >>> - error_setg(errp, "vfio: failed to read %s", group_path); >>> - g_free(group_path); >>> - return NULL; >>> - } >>> - >>> - g_free(group_path); >>> - >>> - return vfio_get_group(groupid, &address_space_memory, errp); >>> -} >>> - >>> static void vfio_ap_req_notifier_handler(void *opaque) >>> { >>> VFIOAPDevice *vapdev = opaque; >>> @@ -189,22 +155,15 @@ static void >>> vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev, >>> static void vfio_ap_realize(DeviceState *dev, Error **errp) >>> { >>> int ret; >>> - char *mdevid; >>> Error *err = NULL; >>> - VFIOGroup *vfio_group; >>> APDevice *apdev = AP_DEVICE(dev); >>> VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev); >>> + VFIODevice *vbasedev = &vapdev->vdev; >>> - vfio_group = vfio_ap_get_group(vapdev, errp); >>> - if (!vfio_group) { >>> - return; >>> - } >>> - >>> - vapdev->vdev.ops = &vfio_ap_ops; >>> - vapdev->vdev.type = VFIO_DEVICE_TYPE_AP; >>> - mdevid = basename(vapdev->vdev.sysfsdev); >>> - vapdev->vdev.name = g_strdup_printf("%s", mdevid); >>> - vapdev->vdev.dev = dev; >>> + vbasedev->name = g_path_get_basename(vbasedev->sysfsdev); >>> + vbasedev->ops = &vfio_ap_ops; >>> + vbasedev->type = VFIO_DEVICE_TYPE_AP; >>> + vbasedev->dev = dev; >>> /* >>> * vfio-ap devices operate in a way compatible with discarding of >>> @@ -214,9 +173,11 @@ static void vfio_ap_realize(DeviceState *dev, >>> Error **errp) >>> */ >>> vapdev->vdev.ram_block_discard_allowed = true; >>> - ret = vfio_get_device(vfio_group, mdevid, &vapdev->vdev, errp); >>> + ret = vfio_attach_device(vbasedev->name, vbasedev, >>> + &address_space_memory, errp); >>> if (ret) { >>> - goto out_get_dev_err; >>> + g_free(vbasedev->name); >>> + return; >>> } >>> vfio_ap_register_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX, >>> &err); >>> @@ -225,25 +186,20 @@ static void vfio_ap_realize(DeviceState *dev, >>> Error **errp) >>> * Report this error, but do not make it a failing condition. >>> * Lack of this IRQ in the host does not prevent normal >>> operation. >>> */ >>> + vfio_detach_device(vbasedev); >>> error_report_err(err); >>> + g_free(vbasedev->name); >>> } >>> - >>> - return; >>> - >>> -out_get_dev_err: >>> - vfio_ap_put_device(vapdev); >>> - vfio_put_group(vfio_group); >>> } >> >> >> To be consistent with vfio_(pci)_realize(), I would introduce the same >> failure path at the end the routine : >> >> out_detach: >> vfio_detach_device(vbasedev); >> error: >> error_prepend(errp, VFIO_MSG_PREFIX, vbasedev->name); >> g_free(vbasedev->name); >> >> >> and add the VFIO_MSG_PREFIX while we are at it. > > following Matthew's comment we do not have any need for error handling > in vfio_ap_realize() anymore. > > so just removing the improper additions: > + vfio_detach_device(vbasedev); > + g_free(vbasedev->name); > > Thanks Just to clarify, there is still a little error handling needed for the error case on vfio_attach_device(), in that case you are passing errp into vfio_attach_device and that should have an error propogated when ret!=0, meaning we won't get the dc->unrealize() later. Device wasn't attached, but we did allocate memory for vbasedev->name already so we need to undo that part ourselves. That could be inline (as you do already in this patch) or, if you choose to add the VFIO_MSG_PREFIX it might make sense to put it in an error: label as Cedric suggested for additional later use. FWIW, I'm fine with either but here's a snippet of what I mean for the sake of clarity: Approach 1 (w/ new prepend): ret = vfio_attach_device(vbasedev->name, vbasedev, &address_space_memory, errp); if (ret) { goto error; } vfio_ap_register_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX, &err); if (err) { /* * Report this error, but do not make it a failing condition. * Lack of this IRQ in the host does not prevent normal operation. */ error_report_err(err); } return; error: error_prepend(errp, VFIO_MSG_PREFIX, vbasedev->name); g_free(vbasedev->name); Approach 2 (no prepend): ret = vfio_attach_device(vbasedev->name, vbasedev, &address_space_memory, errp); if (ret) { g_free(vbasedev->name); return; } vfio_ap_register_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX, &err); if (err) { /* * Report this error, but do not make it a failing condition. * Lack of this IRQ in the host does not prevent normal operation. */ error_report_err(err); } return; With either of these approaches: Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com> Thanks, Matt > > Eric >> >> This is minor, so : >> >> Reviewed-by: Cédric Le Goater <clg@redhat.com> >> >> Thanks, >> >> C. >> >> >> >>> static void vfio_ap_unrealize(DeviceState *dev) >>> { >>> APDevice *apdev = AP_DEVICE(dev); >>> VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev); >>> - VFIOGroup *group = vapdev->vdev.group; >>> vfio_ap_unregister_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX); >>> - vfio_ap_put_device(vapdev); >>> - vfio_put_group(group); >>> + vfio_detach_device(&vapdev->vdev); >>> + g_free(vapdev->vdev.name); >>> } >>> static Property vfio_ap_properties[] = { >> >
Hi Matthew, On 10/4/23 15:41, Matthew Rosato wrote: > On 10/4/23 5:58 AM, Eric Auger wrote: >> Hi Cédric, >> >> On 10/3/23 17:25, Cédric Le Goater wrote: >>> On 10/3/23 12:14, Eric Auger wrote: >>>> Let the vfio-ap device use vfio_attach_device() and >>>> vfio_detach_device(), hence hiding the details of the used >>>> IOMMU backend. >>>> >>>> We take the opportunity to use g_path_get_basename() which >>>> is prefered, as suggested by >>>> 3e015d815b ("use g_path_get_basename instead of basename") >>>> >>>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>> >>>> --- >>>> >>>> v2 -> v3: >>>> - Mention g_path_get_basename in commit message and properly free >>>> vbasedev->name, call vfio_detach_device >>>> --- >>>> hw/vfio/ap.c | 70 ++++++++++------------------------------------------ >>>> 1 file changed, 13 insertions(+), 57 deletions(-) >>>> >>>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c >>>> index 6e21d1da5a..d0b587b3b1 100644 >>>> --- a/hw/vfio/ap.c >>>> +++ b/hw/vfio/ap.c >>>> @@ -53,40 +53,6 @@ struct VFIODeviceOps vfio_ap_ops = { >>>> .vfio_compute_needs_reset = vfio_ap_compute_needs_reset, >>>> }; >>>> -static void vfio_ap_put_device(VFIOAPDevice *vapdev) >>>> -{ >>>> - g_free(vapdev->vdev.name); >>>> - vfio_put_base_device(&vapdev->vdev); >>>> -} >>>> - >>>> -static VFIOGroup *vfio_ap_get_group(VFIOAPDevice *vapdev, Error **errp) >>>> -{ >>>> - GError *gerror = NULL; >>>> - char *symlink, *group_path; >>>> - int groupid; >>>> - >>>> - symlink = g_strdup_printf("%s/iommu_group", vapdev->vdev.sysfsdev); >>>> - group_path = g_file_read_link(symlink, &gerror); >>>> - g_free(symlink); >>>> - >>>> - if (!group_path) { >>>> - error_setg(errp, "%s: no iommu_group found for %s: %s", >>>> - TYPE_VFIO_AP_DEVICE, vapdev->vdev.sysfsdev, >>>> gerror->message); >>>> - g_error_free(gerror); >>>> - return NULL; >>>> - } >>>> - >>>> - if (sscanf(basename(group_path), "%d", &groupid) != 1) { >>>> - error_setg(errp, "vfio: failed to read %s", group_path); >>>> - g_free(group_path); >>>> - return NULL; >>>> - } >>>> - >>>> - g_free(group_path); >>>> - >>>> - return vfio_get_group(groupid, &address_space_memory, errp); >>>> -} >>>> - >>>> static void vfio_ap_req_notifier_handler(void *opaque) >>>> { >>>> VFIOAPDevice *vapdev = opaque; >>>> @@ -189,22 +155,15 @@ static void >>>> vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev, >>>> static void vfio_ap_realize(DeviceState *dev, Error **errp) >>>> { >>>> int ret; >>>> - char *mdevid; >>>> Error *err = NULL; >>>> - VFIOGroup *vfio_group; >>>> APDevice *apdev = AP_DEVICE(dev); >>>> VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev); >>>> + VFIODevice *vbasedev = &vapdev->vdev; >>>> - vfio_group = vfio_ap_get_group(vapdev, errp); >>>> - if (!vfio_group) { >>>> - return; >>>> - } >>>> - >>>> - vapdev->vdev.ops = &vfio_ap_ops; >>>> - vapdev->vdev.type = VFIO_DEVICE_TYPE_AP; >>>> - mdevid = basename(vapdev->vdev.sysfsdev); >>>> - vapdev->vdev.name = g_strdup_printf("%s", mdevid); >>>> - vapdev->vdev.dev = dev; >>>> + vbasedev->name = g_path_get_basename(vbasedev->sysfsdev); >>>> + vbasedev->ops = &vfio_ap_ops; >>>> + vbasedev->type = VFIO_DEVICE_TYPE_AP; >>>> + vbasedev->dev = dev; >>>> /* >>>> * vfio-ap devices operate in a way compatible with discarding of >>>> @@ -214,9 +173,11 @@ static void vfio_ap_realize(DeviceState *dev, >>>> Error **errp) >>>> */ >>>> vapdev->vdev.ram_block_discard_allowed = true; >>>> - ret = vfio_get_device(vfio_group, mdevid, &vapdev->vdev, errp); >>>> + ret = vfio_attach_device(vbasedev->name, vbasedev, >>>> + &address_space_memory, errp); >>>> if (ret) { >>>> - goto out_get_dev_err; >>>> + g_free(vbasedev->name); >>>> + return; >>>> } >>>> vfio_ap_register_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX, >>>> &err); >>>> @@ -225,25 +186,20 @@ static void vfio_ap_realize(DeviceState *dev, >>>> Error **errp) >>>> * Report this error, but do not make it a failing condition. >>>> * Lack of this IRQ in the host does not prevent normal >>>> operation. >>>> */ >>>> + vfio_detach_device(vbasedev); >>>> error_report_err(err); >>>> + g_free(vbasedev->name); >>>> } >>>> - >>>> - return; >>>> - >>>> -out_get_dev_err: >>>> - vfio_ap_put_device(vapdev); >>>> - vfio_put_group(vfio_group); >>>> } >>> >>> To be consistent with vfio_(pci)_realize(), I would introduce the same >>> failure path at the end the routine : >>> >>> out_detach: >>> vfio_detach_device(vbasedev); >>> error: >>> error_prepend(errp, VFIO_MSG_PREFIX, vbasedev->name); >>> g_free(vbasedev->name); >>> >>> >>> and add the VFIO_MSG_PREFIX while we are at it. >> following Matthew's comment we do not have any need for error handling >> in vfio_ap_realize() anymore. >> >> so just removing the improper additions: >> + vfio_detach_device(vbasedev); >> + g_free(vbasedev->name); >> >> Thanks > Just to clarify, there is still a little error handling needed for the error case on vfio_attach_device(), in that case you are passing errp into vfio_attach_device and that should have an error propogated when ret!=0, meaning we won't get the dc->unrealize() later. Device wasn't attached, but we did allocate memory for vbasedev->name already so we need to undo that part ourselves. That could be inline (as you do already in this patch) or, if you choose to add the VFIO_MSG_PREFIX it might make sense to put it in an error: label as Cedric suggested for additional later use. FWIW, I'm fine with either but here's a snippet of what I mean for the sake of clarity: yes thank you for the notice > > > Approach 1 (w/ new prepend): > > ret = vfio_attach_device(vbasedev->name, vbasedev, > &address_space_memory, errp); > if (ret) { > goto error; > } > > vfio_ap_register_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX, &err); > if (err) { > /* > * Report this error, but do not make it a failing condition. > * Lack of this IRQ in the host does not prevent normal operation. > */ > error_report_err(err); > } > > return; > > error: > error_prepend(errp, VFIO_MSG_PREFIX, vbasedev->name); > g_free(vbasedev->name); So I took that one > > > Approach 2 (no prepend): > > ret = vfio_attach_device(vbasedev->name, vbasedev, > &address_space_memory, errp); > if (ret) { > g_free(vbasedev->name); > return; > } > > vfio_ap_register_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX, &err); > if (err) { > /* > * Report this error, but do not make it a failing condition. > * Lack of this IRQ in the host does not prevent normal operation. > */ > error_report_err(err); > } > > return; > > > With either of these approaches: > > Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com> Thank you for your time! Eric > > Thanks, > Matt > >> Eric >>> This is minor, so : >>> >>> Reviewed-by: Cédric Le Goater <clg@redhat.com> >>> >>> Thanks, >>> >>> C. >>> >>> >>> >>>> static void vfio_ap_unrealize(DeviceState *dev) >>>> { >>>> APDevice *apdev = AP_DEVICE(dev); >>>> VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev); >>>> - VFIOGroup *group = vapdev->vdev.group; >>>> vfio_ap_unregister_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX); >>>> - vfio_ap_put_device(vapdev); >>>> - vfio_put_group(group); >>>> + vfio_detach_device(&vapdev->vdev); >>>> + g_free(vapdev->vdev.name); >>>> } >>>> static Property vfio_ap_properties[] = {
diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c index 6e21d1da5a..d0b587b3b1 100644 --- a/hw/vfio/ap.c +++ b/hw/vfio/ap.c @@ -53,40 +53,6 @@ struct VFIODeviceOps vfio_ap_ops = { .vfio_compute_needs_reset = vfio_ap_compute_needs_reset, }; -static void vfio_ap_put_device(VFIOAPDevice *vapdev) -{ - g_free(vapdev->vdev.name); - vfio_put_base_device(&vapdev->vdev); -} - -static VFIOGroup *vfio_ap_get_group(VFIOAPDevice *vapdev, Error **errp) -{ - GError *gerror = NULL; - char *symlink, *group_path; - int groupid; - - symlink = g_strdup_printf("%s/iommu_group", vapdev->vdev.sysfsdev); - group_path = g_file_read_link(symlink, &gerror); - g_free(symlink); - - if (!group_path) { - error_setg(errp, "%s: no iommu_group found for %s: %s", - TYPE_VFIO_AP_DEVICE, vapdev->vdev.sysfsdev, gerror->message); - g_error_free(gerror); - return NULL; - } - - if (sscanf(basename(group_path), "%d", &groupid) != 1) { - error_setg(errp, "vfio: failed to read %s", group_path); - g_free(group_path); - return NULL; - } - - g_free(group_path); - - return vfio_get_group(groupid, &address_space_memory, errp); -} - static void vfio_ap_req_notifier_handler(void *opaque) { VFIOAPDevice *vapdev = opaque; @@ -189,22 +155,15 @@ static void vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev, static void vfio_ap_realize(DeviceState *dev, Error **errp) { int ret; - char *mdevid; Error *err = NULL; - VFIOGroup *vfio_group; APDevice *apdev = AP_DEVICE(dev); VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev); + VFIODevice *vbasedev = &vapdev->vdev; - vfio_group = vfio_ap_get_group(vapdev, errp); - if (!vfio_group) { - return; - } - - vapdev->vdev.ops = &vfio_ap_ops; - vapdev->vdev.type = VFIO_DEVICE_TYPE_AP; - mdevid = basename(vapdev->vdev.sysfsdev); - vapdev->vdev.name = g_strdup_printf("%s", mdevid); - vapdev->vdev.dev = dev; + vbasedev->name = g_path_get_basename(vbasedev->sysfsdev); + vbasedev->ops = &vfio_ap_ops; + vbasedev->type = VFIO_DEVICE_TYPE_AP; + vbasedev->dev = dev; /* * vfio-ap devices operate in a way compatible with discarding of @@ -214,9 +173,11 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp) */ vapdev->vdev.ram_block_discard_allowed = true; - ret = vfio_get_device(vfio_group, mdevid, &vapdev->vdev, errp); + ret = vfio_attach_device(vbasedev->name, vbasedev, + &address_space_memory, errp); if (ret) { - goto out_get_dev_err; + g_free(vbasedev->name); + return; } vfio_ap_register_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX, &err); @@ -225,25 +186,20 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp) * Report this error, but do not make it a failing condition. * Lack of this IRQ in the host does not prevent normal operation. */ + vfio_detach_device(vbasedev); error_report_err(err); + g_free(vbasedev->name); } - - return; - -out_get_dev_err: - vfio_ap_put_device(vapdev); - vfio_put_group(vfio_group); } static void vfio_ap_unrealize(DeviceState *dev) { APDevice *apdev = AP_DEVICE(dev); VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev); - VFIOGroup *group = vapdev->vdev.group; vfio_ap_unregister_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX); - vfio_ap_put_device(vapdev); - vfio_put_group(group); + vfio_detach_device(&vapdev->vdev); + g_free(vapdev->vdev.name); } static Property vfio_ap_properties[] = {
Let the vfio-ap device use vfio_attach_device() and vfio_detach_device(), hence hiding the details of the used IOMMU backend. We take the opportunity to use g_path_get_basename() which is prefered, as suggested by 3e015d815b ("use g_path_get_basename instead of basename") Signed-off-by: Eric Auger <eric.auger@redhat.com> Signed-off-by: Yi Liu <yi.l.liu@intel.com> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- v2 -> v3: - Mention g_path_get_basename in commit message and properly free vbasedev->name, call vfio_detach_device --- hw/vfio/ap.c | 70 ++++++++++------------------------------------------ 1 file changed, 13 insertions(+), 57 deletions(-)