| Message ID | 20230131205305.2726330-12-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio: Adopt iommufd | expand |
On 1/31/23 3:52 PM, Eric Auger wrote: > Let the vfio-ccw device use vfio_attach_device() and > vfio_detach_device(), hence hiding the details of the used > IOMMU backend. > > Also now all the devices have been migrated to use the new > vfio_attach_device/vfio_detach_device API, let's turn the > legacy functions into static functions, local to container.c. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > Signed-off-by: Yi Liu <yi.l.liu@intel.com> Hi Eric, While testing the cdev series on s390 I ran into a couple of issues with this patch, see below. > --- > include/hw/vfio/vfio-common.h | 4 -- > hw/vfio/ccw.c | 118 ++++++++-------------------------- > hw/vfio/container.c | 8 +-- > 3 files changed, 32 insertions(+), 98 deletions(-) > > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h > index 9465c4b021..1580f9617c 100644 > --- a/include/hw/vfio/vfio-common.h > +++ b/include/hw/vfio/vfio-common.h > @@ -176,10 +176,6 @@ void vfio_region_unmap(VFIORegion *region); > void vfio_region_exit(VFIORegion *region); > void vfio_region_finalize(VFIORegion *region); > void vfio_reset_handler(void *opaque); > -VFIOGroup *vfio_get_group(int groupid, AddressSpace *as, Error **errp); > -void vfio_put_group(VFIOGroup *group); > -int vfio_get_device(VFIOGroup *group, const char *name, > - VFIODevice *vbasedev, Error **errp); > int vfio_attach_device(VFIODevice *vbasedev, AddressSpace *as, Error **errp); > void vfio_detach_device(VFIODevice *vbasedev); > > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c > index 0354737666..6fde7849cc 100644 > --- a/hw/vfio/ccw.c > +++ b/hw/vfio/ccw.c > @@ -579,27 +579,32 @@ static void vfio_ccw_put_region(VFIOCCWDevice *vcdev) > g_free(vcdev->io_region); > } > > -static void vfio_ccw_put_device(VFIOCCWDevice *vcdev) > -{ > - g_free(vcdev->vdev.name); > - vfio_put_base_device(&vcdev->vdev); > -} > - > -static void vfio_ccw_get_device(VFIOGroup *group, VFIOCCWDevice *vcdev, > - Error **errp) > +static void vfio_ccw_realize(DeviceState *dev, Error **errp) > { > + CcwDevice *ccw_dev = DO_UPCAST(CcwDevice, parent_obj, dev); > + S390CCWDevice *cdev = DO_UPCAST(S390CCWDevice, parent_obj, ccw_dev); > + VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev); > + S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev); > + VFIODevice *vbasedev = &vcdev->vdev; > + Error *err = NULL; > char *name = g_strdup_printf("%x.%x.%04x", vcdev->cdev.hostid.cssid, > vcdev->cdev.hostid.ssid, > vcdev->cdev.hostid.devid); We can't get these cssid, ssid, devid values quite yet, they are currently 0s. That has to happen after cdc->realize() > - VFIODevice *vbasedev; > + int ret; > > - QLIST_FOREACH(vbasedev, &group->device_list, next) { > - if (strcmp(vbasedev->name, name) == 0) { > - error_setg(errp, "vfio: subchannel %s has already been attached", > - name); > - goto out_err; > + /* Call the class init function for subchannel. */ > + if (cdc->realize) { > + cdc->realize(cdev, vcdev->vdev.sysfsdev, &err); > + if (err) { > + goto out_err_propagate; > } > } > + vbasedev->sysfsdev = g_strdup_printf("/sys/bus/css/devices/%s/%s", > + name, cdev->mdevid); > + vbasedev->ops = &vfio_ccw_ops; > + vbasedev->type = VFIO_DEVICE_TYPE_CCW; > + vbasedev->name = name; vbasedev->name is being set to the wrong value here, it needs to be the uuid. See below for a suggested diff on top of this patch that solves the issue for me. Thanks, Matt diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c index 6fde7849cc..394b73358f 100644 --- a/hw/vfio/ccw.c +++ b/hw/vfio/ccw.c @@ -587,9 +587,6 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp) S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev); VFIODevice *vbasedev = &vcdev->vdev; Error *err = NULL; - char *name = g_strdup_printf("%x.%x.%04x", vcdev->cdev.hostid.cssid, - vcdev->cdev.hostid.ssid, - vcdev->cdev.hostid.devid); int ret; /* Call the class init function for subchannel. */ @@ -599,11 +596,14 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp) goto out_err_propagate; } } - vbasedev->sysfsdev = g_strdup_printf("/sys/bus/css/devices/%s/%s", - name, cdev->mdevid); + vbasedev->sysfsdev = g_strdup_printf("/sys/bus/css/devices/%x.%x.%04x/%s", + vcdev->cdev.hostid.cssid, + vcdev->cdev.hostid.ssid, + vcdev->cdev.hostid.devid, + cdev->mdevid); vbasedev->ops = &vfio_ccw_ops; vbasedev->type = VFIO_DEVICE_TYPE_CCW; - vbasedev->name = name; + vbasedev->name = g_strdup(cdev->mdevid); vbasedev->dev = &vcdev->cdev.parent_obj.parent_obj; /*
Hi Matthew, On 3/3/23 18:30, Matthew Rosato wrote: > On 1/31/23 3:52 PM, Eric Auger wrote: >> Let the vfio-ccw device use vfio_attach_device() and >> vfio_detach_device(), hence hiding the details of the used >> IOMMU backend. >> >> Also now all the devices have been migrated to use the new >> vfio_attach_device/vfio_detach_device API, let's turn the >> legacy functions into static functions, local to container.c. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> Signed-off-by: Yi Liu <yi.l.liu@intel.com> > Hi Eric, > > While testing the cdev series on s390 I ran into a couple of issues with this patch, see below. > >> --- >> include/hw/vfio/vfio-common.h | 4 -- >> hw/vfio/ccw.c | 118 ++++++++-------------------------- >> hw/vfio/container.c | 8 +-- >> 3 files changed, 32 insertions(+), 98 deletions(-) >> >> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h >> index 9465c4b021..1580f9617c 100644 >> --- a/include/hw/vfio/vfio-common.h >> +++ b/include/hw/vfio/vfio-common.h >> @@ -176,10 +176,6 @@ void vfio_region_unmap(VFIORegion *region); >> void vfio_region_exit(VFIORegion *region); >> void vfio_region_finalize(VFIORegion *region); >> void vfio_reset_handler(void *opaque); >> -VFIOGroup *vfio_get_group(int groupid, AddressSpace *as, Error **errp); >> -void vfio_put_group(VFIOGroup *group); >> -int vfio_get_device(VFIOGroup *group, const char *name, >> - VFIODevice *vbasedev, Error **errp); >> int vfio_attach_device(VFIODevice *vbasedev, AddressSpace *as, Error **errp); >> void vfio_detach_device(VFIODevice *vbasedev); >> >> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c >> index 0354737666..6fde7849cc 100644 >> --- a/hw/vfio/ccw.c >> +++ b/hw/vfio/ccw.c >> @@ -579,27 +579,32 @@ static void vfio_ccw_put_region(VFIOCCWDevice *vcdev) >> g_free(vcdev->io_region); >> } >> >> -static void vfio_ccw_put_device(VFIOCCWDevice *vcdev) >> -{ >> - g_free(vcdev->vdev.name); >> - vfio_put_base_device(&vcdev->vdev); >> -} >> - >> -static void vfio_ccw_get_device(VFIOGroup *group, VFIOCCWDevice *vcdev, >> - Error **errp) >> +static void vfio_ccw_realize(DeviceState *dev, Error **errp) >> { >> + CcwDevice *ccw_dev = DO_UPCAST(CcwDevice, parent_obj, dev); >> + S390CCWDevice *cdev = DO_UPCAST(S390CCWDevice, parent_obj, ccw_dev); >> + VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev); >> + S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev); >> + VFIODevice *vbasedev = &vcdev->vdev; >> + Error *err = NULL; >> char *name = g_strdup_printf("%x.%x.%04x", vcdev->cdev.hostid.cssid, >> vcdev->cdev.hostid.ssid, >> vcdev->cdev.hostid.devid); > > We can't get these cssid, ssid, devid values quite yet, they are currently 0s. That has to happen after cdc->realize() > > >> - VFIODevice *vbasedev; >> + int ret; >> >> - QLIST_FOREACH(vbasedev, &group->device_list, next) { >> - if (strcmp(vbasedev->name, name) == 0) { >> - error_setg(errp, "vfio: subchannel %s has already been attached", >> - name); >> - goto out_err; >> + /* Call the class init function for subchannel. */ >> + if (cdc->realize) { >> + cdc->realize(cdev, vcdev->vdev.sysfsdev, &err); >> + if (err) { >> + goto out_err_propagate; >> } >> } >> + vbasedev->sysfsdev = g_strdup_printf("/sys/bus/css/devices/%s/%s", >> + name, cdev->mdevid); >> + vbasedev->ops = &vfio_ccw_ops; >> + vbasedev->type = VFIO_DEVICE_TYPE_CCW; >> + vbasedev->name = name; > vbasedev->name is being set to the wrong value here, it needs to be the uuid. > > See below for a suggested diff on top of this patch that solves the issue for me. > > Thanks, > Matt > > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c > index 6fde7849cc..394b73358f 100644 > --- a/hw/vfio/ccw.c > +++ b/hw/vfio/ccw.c > @@ -587,9 +587,6 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp) > S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev); > VFIODevice *vbasedev = &vcdev->vdev; > Error *err = NULL; > - char *name = g_strdup_printf("%x.%x.%04x", vcdev->cdev.hostid.cssid, > - vcdev->cdev.hostid.ssid, > - vcdev->cdev.hostid.devid); > int ret; > > /* Call the class init function for subchannel. */ > @@ -599,11 +596,14 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp) > goto out_err_propagate; > } > } > - vbasedev->sysfsdev = g_strdup_printf("/sys/bus/css/devices/%s/%s", > - name, cdev->mdevid); > + vbasedev->sysfsdev = g_strdup_printf("/sys/bus/css/devices/%x.%x.%04x/%s", > + vcdev->cdev.hostid.cssid, > + vcdev->cdev.hostid.ssid, > + vcdev->cdev.hostid.devid, > + cdev->mdevid); > vbasedev->ops = &vfio_ccw_ops; > vbasedev->type = VFIO_DEVICE_TYPE_CCW; > - vbasedev->name = name; > + vbasedev->name = g_strdup(cdev->mdevid); > vbasedev->dev = &vcdev->cdev.parent_obj.parent_obj; > > /* > > Thank you very much for your report. This will be handled in the next version. Eric
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h index 9465c4b021..1580f9617c 100644 --- a/include/hw/vfio/vfio-common.h +++ b/include/hw/vfio/vfio-common.h @@ -176,10 +176,6 @@ void vfio_region_unmap(VFIORegion *region); void vfio_region_exit(VFIORegion *region); void vfio_region_finalize(VFIORegion *region); void vfio_reset_handler(void *opaque); -VFIOGroup *vfio_get_group(int groupid, AddressSpace *as, Error **errp); -void vfio_put_group(VFIOGroup *group); -int vfio_get_device(VFIOGroup *group, const char *name, - VFIODevice *vbasedev, Error **errp); int vfio_attach_device(VFIODevice *vbasedev, AddressSpace *as, Error **errp); void vfio_detach_device(VFIODevice *vbasedev); diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c index 0354737666..6fde7849cc 100644 --- a/hw/vfio/ccw.c +++ b/hw/vfio/ccw.c @@ -579,27 +579,32 @@ static void vfio_ccw_put_region(VFIOCCWDevice *vcdev) g_free(vcdev->io_region); } -static void vfio_ccw_put_device(VFIOCCWDevice *vcdev) -{ - g_free(vcdev->vdev.name); - vfio_put_base_device(&vcdev->vdev); -} - -static void vfio_ccw_get_device(VFIOGroup *group, VFIOCCWDevice *vcdev, - Error **errp) +static void vfio_ccw_realize(DeviceState *dev, Error **errp) { + CcwDevice *ccw_dev = DO_UPCAST(CcwDevice, parent_obj, dev); + S390CCWDevice *cdev = DO_UPCAST(S390CCWDevice, parent_obj, ccw_dev); + VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev); + S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev); + VFIODevice *vbasedev = &vcdev->vdev; + Error *err = NULL; char *name = g_strdup_printf("%x.%x.%04x", vcdev->cdev.hostid.cssid, vcdev->cdev.hostid.ssid, vcdev->cdev.hostid.devid); - VFIODevice *vbasedev; + int ret; - QLIST_FOREACH(vbasedev, &group->device_list, next) { - if (strcmp(vbasedev->name, name) == 0) { - error_setg(errp, "vfio: subchannel %s has already been attached", - name); - goto out_err; + /* Call the class init function for subchannel. */ + if (cdc->realize) { + cdc->realize(cdev, vcdev->vdev.sysfsdev, &err); + if (err) { + goto out_err_propagate; } } + vbasedev->sysfsdev = g_strdup_printf("/sys/bus/css/devices/%s/%s", + name, cdev->mdevid); + vbasedev->ops = &vfio_ccw_ops; + vbasedev->type = VFIO_DEVICE_TYPE_CCW; + vbasedev->name = name; + vbasedev->dev = &vcdev->cdev.parent_obj.parent_obj; /* * All vfio-ccw devices are believed to operate in a way compatible with @@ -609,80 +614,18 @@ static void vfio_ccw_get_device(VFIOGroup *group, VFIOCCWDevice *vcdev, * needs to be set before vfio_get_device() for vfio common to handle * ram_block_discard_disable(). */ - vcdev->vdev.ram_block_discard_allowed = true; - if (vfio_get_device(group, vcdev->cdev.mdevid, &vcdev->vdev, errp)) { - goto out_err; - } + vbasedev->ram_block_discard_allowed = true; - vcdev->vdev.ops = &vfio_ccw_ops; - vcdev->vdev.type = VFIO_DEVICE_TYPE_CCW; - vcdev->vdev.name = name; - vcdev->vdev.dev = &vcdev->cdev.parent_obj.parent_obj; - - return; - -out_err: - g_free(name); -} - -static VFIOGroup *vfio_ccw_get_group(S390CCWDevice *cdev, Error **errp) -{ - char *tmp, group_path[PATH_MAX]; - ssize_t len; - int groupid; - - tmp = g_strdup_printf("/sys/bus/css/devices/%x.%x.%04x/%s/iommu_group", - cdev->hostid.cssid, cdev->hostid.ssid, - cdev->hostid.devid, cdev->mdevid); - len = readlink(tmp, group_path, sizeof(group_path)); - g_free(tmp); - - if (len <= 0 || len >= sizeof(group_path)) { - error_setg(errp, "vfio: no iommu_group found"); - return NULL; - } - - group_path[len] = 0; - - if (sscanf(basename(group_path), "%d", &groupid) != 1) { - error_setg(errp, "vfio: failed to read %s", group_path); - return NULL; - } - - return vfio_get_group(groupid, &address_space_memory, errp); -} - -static void vfio_ccw_realize(DeviceState *dev, Error **errp) -{ - VFIOGroup *group; - CcwDevice *ccw_dev = DO_UPCAST(CcwDevice, parent_obj, dev); - S390CCWDevice *cdev = DO_UPCAST(S390CCWDevice, parent_obj, ccw_dev); - VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev); - S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev); - Error *err = NULL; - - /* Call the class init function for subchannel. */ - if (cdc->realize) { - cdc->realize(cdev, vcdev->vdev.sysfsdev, &err); - if (err) { - goto out_err_propagate; - } - } - - group = vfio_ccw_get_group(cdev, &err); - if (!group) { - goto out_group_err; - } - - vfio_ccw_get_device(group, vcdev, &err); - if (err) { - goto out_device_err; + ret = vfio_attach_device(vbasedev, &address_space_memory, errp); + if (ret) { + g_free(vbasedev->name); + g_free(vbasedev->sysfsdev); } vfio_ccw_get_region(vcdev, &err); if (err) { - goto out_region_err; + goto out_get_dev_err; } vfio_ccw_register_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX, &err); @@ -714,11 +657,8 @@ out_irq_notifier_err: vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX); out_io_notifier_err: vfio_ccw_put_region(vcdev); -out_region_err: - vfio_ccw_put_device(vcdev); -out_device_err: - vfio_put_group(group); -out_group_err: +out_get_dev_err: + vfio_detach_device(vbasedev); if (cdc->unrealize) { cdc->unrealize(cdev); } @@ -732,14 +672,12 @@ static void vfio_ccw_unrealize(DeviceState *dev) S390CCWDevice *cdev = DO_UPCAST(S390CCWDevice, parent_obj, ccw_dev); VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev); S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev); - VFIOGroup *group = vcdev->vdev.group; vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX); vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_CRW_IRQ_INDEX); vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_IO_IRQ_INDEX); vfio_ccw_put_region(vcdev); - vfio_ccw_put_device(vcdev); - vfio_put_group(group); + vfio_detach_device(&vcdev->vdev); if (cdc->unrealize) { cdc->unrealize(cdev); diff --git a/hw/vfio/container.c b/hw/vfio/container.c index 6e466ef037..b9ee56067c 100644 --- a/hw/vfio/container.c +++ b/hw/vfio/container.c @@ -993,7 +993,7 @@ static void vfio_disconnect_container(VFIOGroup *group) } } -VFIOGroup *vfio_get_group(int groupid, AddressSpace *as, Error **errp) +static VFIOGroup *vfio_get_group(int groupid, AddressSpace *as, Error **errp) { VFIOGroup *group; VFIOContainer *bcontainer; @@ -1062,7 +1062,7 @@ free_group_exit: return NULL; } -void vfio_put_group(VFIOGroup *group) +static void vfio_put_group(VFIOGroup *group) { if (!group || !QLIST_EMPTY(&group->device_list)) { return; @@ -1083,8 +1083,8 @@ void vfio_put_group(VFIOGroup *group) } } -int vfio_get_device(VFIOGroup *group, const char *name, - VFIODevice *vbasedev, Error **errp) +static int vfio_get_device(VFIOGroup *group, const char *name, + VFIODevice *vbasedev, Error **errp) { struct vfio_device_info dev_info = { .argsz = sizeof(dev_info) }; int ret, fd;