| Message ID | 20230926113255.1177834-10-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Prerequisite change for IOMMUFD support | expand |
On 9/26/23 13:32, Zhenzhong Duan wrote: > From: Eric Auger <eric.auger@redhat.com> > > 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> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > include/hw/vfio/vfio-common.h | 5 -- > hw/vfio/ccw.c | 115 ++++++++-------------------------- > hw/vfio/common.c | 10 +-- > 3 files changed, 30 insertions(+), 100 deletions(-) > > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h > index 12fbfbc37d..c486bdef2a 100644 > --- a/include/hw/vfio/vfio-common.h > +++ b/include/hw/vfio/vfio-common.h > @@ -202,7 +202,6 @@ typedef struct { > hwaddr pages; > } VFIOBitmap; > > -void vfio_put_base_device(VFIODevice *vbasedev); > void vfio_disable_irqindex(VFIODevice *vbasedev, int index); > void vfio_unmask_single_irqindex(VFIODevice *vbasedev, int index); > void vfio_mask_single_irqindex(VFIODevice *vbasedev, int index); > @@ -220,11 +219,7 @@ 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); > struct vfio_device_info *vfio_get_device_info(int fd); > -int vfio_get_device(VFIOGroup *group, const char *name, > - VFIODevice *vbasedev, Error **errp); > int vfio_attach_device(char *name, 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 1e2fce83b0..6893a30ab1 100644 > --- a/hw/vfio/ccw.c > +++ b/hw/vfio/ccw.c > @@ -572,88 +572,14 @@ 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) > -{ > - S390CCWDevice *cdev = S390_CCW_DEVICE(vcdev); > - char *name = g_strdup_printf("%x.%x.%04x", cdev->hostid.cssid, > - cdev->hostid.ssid, > - cdev->hostid.devid); Digging into that few months later, new vfio_device_groupid uses + tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); which is set as a prop here DEFINE_PROP_STRING("sysfsdev", VFIOCCWDevice, vdev.sysfsdev), we need to double check if this matches, this is not obvious at first sight. At least if this is true this needs to be documented in the commit msg then the subchannel name is char *name = g_strdup_printf("%x.%x.%04x", cdev->hostid.cssid, cdev->hostid.ssid, cdev->hostid.devid); 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; } } while new code use + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { + if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) { + error_setg(errp, "device is already attached"); + vfio_put_group(group); + return -EBUSY; + } + } We really need to double check the names, ie. name, vbasedev->name. That's a mess and that's my bad. Thanks Eric > - VFIODevice *vbasedev; > - > - 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; > - } > - } > - > - /* > - * All vfio-ccw devices are believed to operate in a way compatible with > - * discarding of memory in RAM blocks, ie. pages pinned in the host are > - * in the current working set of the guest driver and therefore never > - * overlap e.g., with pages available to the guest balloon driver. This > - * 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, cdev->mdevid, &vcdev->vdev, errp)) { > - goto out_err; > - } > - > - vcdev->vdev.ops = &vfio_ccw_ops; > - vcdev->vdev.type = VFIO_DEVICE_TYPE_CCW; > - vcdev->vdev.name = name; > - vcdev->vdev.dev = DEVICE(vcdev); > - > - 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; > S390CCWDevice *cdev = S390_CCW_DEVICE(dev); > VFIOCCWDevice *vcdev = VFIO_CCW(cdev); > S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev); > + VFIODevice *vbasedev = &vcdev->vdev; > Error *err = NULL; > + int ret; > > /* Call the class init function for subchannel. */ > if (cdc->realize) { > @@ -663,14 +589,25 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp) > } > } > > - group = vfio_ccw_get_group(cdev, &err); > - if (!group) { > - goto out_group_err; > - } > + vbasedev->ops = &vfio_ccw_ops; > + vbasedev->type = VFIO_DEVICE_TYPE_CCW; > + vbasedev->name = g_strdup(cdev->mdevid); > + vbasedev->dev = &vcdev->cdev.parent_obj.parent_obj; > > - vfio_ccw_get_device(group, vcdev, &err); > - if (err) { > - goto out_device_err; > + /* > + * All vfio-ccw devices are believed to operate in a way compatible with > + * discarding of memory in RAM blocks, ie. pages pinned in the host are > + * in the current working set of the guest driver and therefore never > + * overlap e.g., with pages available to the guest balloon driver. This > + * needs to be set before vfio_get_device() for vfio common to handle > + * ram_block_discard_disable(). > + */ > + vbasedev->ram_block_discard_allowed = true; > + > + ret = vfio_attach_device(vbasedev->name, vbasedev, > + &address_space_memory, errp); > + if (ret) { > + goto out_attach_dev_err; > } > > vfio_ccw_get_region(vcdev, &err); > @@ -708,10 +645,9 @@ out_irq_notifier_err: > 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: > + vfio_detach_device(vbasedev); > +out_attach_dev_err: > + g_free(vbasedev->name); > if (cdc->unrealize) { > cdc->unrealize(cdev); > } > @@ -724,14 +660,13 @@ static void vfio_ccw_unrealize(DeviceState *dev) > S390CCWDevice *cdev = S390_CCW_DEVICE(dev); > VFIOCCWDevice *vcdev = VFIO_CCW(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); > + g_free(vcdev->vdev.name); > > if (cdc->unrealize) { > cdc->unrealize(cdev); > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 7f3798b152..65516b319e 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -2335,7 +2335,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; > char path[32]; > @@ -2402,7 +2402,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; > @@ -2447,8 +2447,8 @@ retry: > return info; > } > > -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) > { > g_autofree struct vfio_device_info *info = NULL; > int fd; > @@ -2506,7 +2506,7 @@ int vfio_get_device(VFIOGroup *group, const char *name, > return 0; > } > > -void vfio_put_base_device(VFIODevice *vbasedev) > +static void vfio_put_base_device(VFIODevice *vbasedev) > { > if (!vbasedev->group) { > return;
>-----Original Message----- >From: Eric Auger <eric.auger@redhat.com> >Sent: Wednesday, September 27, 2023 6:00 PM >Subject: Re: [PATCH v2 09/12] vfio/ccw: Use vfio_[attach/detach]_device > > > >On 9/26/23 13:32, Zhenzhong Duan wrote: >> From: Eric Auger <eric.auger@redhat.com> >> >> 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> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> include/hw/vfio/vfio-common.h | 5 -- >> hw/vfio/ccw.c | 115 ++++++++-------------------------- >> hw/vfio/common.c | 10 +-- >> 3 files changed, 30 insertions(+), 100 deletions(-) >> >> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h >> index 12fbfbc37d..c486bdef2a 100644 >> --- a/include/hw/vfio/vfio-common.h >> +++ b/include/hw/vfio/vfio-common.h >> @@ -202,7 +202,6 @@ typedef struct { >> hwaddr pages; >> } VFIOBitmap; >> >> -void vfio_put_base_device(VFIODevice *vbasedev); >> void vfio_disable_irqindex(VFIODevice *vbasedev, int index); >> void vfio_unmask_single_irqindex(VFIODevice *vbasedev, int index); >> void vfio_mask_single_irqindex(VFIODevice *vbasedev, int index); >> @@ -220,11 +219,7 @@ 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); >> struct vfio_device_info *vfio_get_device_info(int fd); >> -int vfio_get_device(VFIOGroup *group, const char *name, >> - VFIODevice *vbasedev, Error **errp); >> int vfio_attach_device(char *name, 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 1e2fce83b0..6893a30ab1 100644 >> --- a/hw/vfio/ccw.c >> +++ b/hw/vfio/ccw.c >> @@ -572,88 +572,14 @@ 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) >> -{ >> - S390CCWDevice *cdev = S390_CCW_DEVICE(vcdev); >> - char *name = g_strdup_printf("%x.%x.%04x", cdev->hostid.cssid, >> - cdev->hostid.ssid, >> - cdev->hostid.devid); >Digging into that few months later, > >new vfio_device_groupid uses > >+ tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); > >which is set as a prop here > DEFINE_PROP_STRING("sysfsdev", VFIOCCWDevice, vdev.sysfsdev), >we need to double check if this matches, this is not obvious at first sight. At least >if this is true this needs to be documented in the commit msg Good finding. Indeed, there is a gap here if we use a symbol link as sysfsdev. See s390_ccw_get_dev_info() for details. But if it's not a symbol link, I think they are same. > >then the subchannel name is > char *name = g_strdup_printf("%x.%x.%04x", cdev->hostid.cssid, > cdev->hostid.ssid, > cdev->hostid.devid); > 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; > } > } > >while new code use >+ QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { >+ if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) { >+ error_setg(errp, "device is already attached"); >+ vfio_put_group(group); >+ return -EBUSY; >+ } >+ } > >We really need to double check the names, ie. >name, vbasedev->name. That's a mess and that's my bad. Thanks for catching, I think below change will make it same as original code: diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c index 6893a30ab1..a8ea0a6fa8 100644 --- a/hw/vfio/ccw.c +++ b/hw/vfio/ccw.c @@ -580,6 +580,9 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp) VFIODevice *vbasedev = &vcdev->vdev; Error *err = NULL; int ret; + char *name = g_strdup_printf("%x.%x.%04x", cdev->hostid.cssid, + cdev->hostid.ssid, + cdev->hostid.devid); /* Call the class init function for subchannel. */ if (cdc->realize) { @@ -591,7 +594,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp) vbasedev->ops = &vfio_ccw_ops; vbasedev->type = VFIO_DEVICE_TYPE_CCW; - vbasedev->name = g_strdup(cdev->mdevid); + vbasedev->name = name; vbasedev->dev = &vcdev->cdev.parent_obj.parent_obj; /* @@ -604,7 +607,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp) */ vbasedev->ram_block_discard_allowed = true; - ret = vfio_attach_device(vbasedev->name, vbasedev, + ret = vfio_attach_device(cdev->mdevid, vbasedev, &address_space_memory, errp); if (ret) { goto out_attach_dev_err; Thanks Zhenzhong
On 9/27/23 8:09 AM, Duan, Zhenzhong wrote: > > >> -----Original Message----- >> From: Eric Auger <eric.auger@redhat.com> >> Sent: Wednesday, September 27, 2023 6:00 PM >> Subject: Re: [PATCH v2 09/12] vfio/ccw: Use vfio_[attach/detach]_device >> >> >> >> On 9/26/23 13:32, Zhenzhong Duan wrote: >>> From: Eric Auger <eric.auger@redhat.com> >>> >>> 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> >>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>> --- >>> include/hw/vfio/vfio-common.h | 5 -- >>> hw/vfio/ccw.c | 115 ++++++++-------------------------- >>> hw/vfio/common.c | 10 +-- >>> 3 files changed, 30 insertions(+), 100 deletions(-) >>> >>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h >>> index 12fbfbc37d..c486bdef2a 100644 >>> --- a/include/hw/vfio/vfio-common.h >>> +++ b/include/hw/vfio/vfio-common.h >>> @@ -202,7 +202,6 @@ typedef struct { >>> hwaddr pages; >>> } VFIOBitmap; >>> >>> -void vfio_put_base_device(VFIODevice *vbasedev); >>> void vfio_disable_irqindex(VFIODevice *vbasedev, int index); >>> void vfio_unmask_single_irqindex(VFIODevice *vbasedev, int index); >>> void vfio_mask_single_irqindex(VFIODevice *vbasedev, int index); >>> @@ -220,11 +219,7 @@ 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); >>> struct vfio_device_info *vfio_get_device_info(int fd); >>> -int vfio_get_device(VFIOGroup *group, const char *name, >>> - VFIODevice *vbasedev, Error **errp); >>> int vfio_attach_device(char *name, 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 1e2fce83b0..6893a30ab1 100644 >>> --- a/hw/vfio/ccw.c >>> +++ b/hw/vfio/ccw.c >>> @@ -572,88 +572,14 @@ 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) >>> -{ >>> - S390CCWDevice *cdev = S390_CCW_DEVICE(vcdev); >>> - char *name = g_strdup_printf("%x.%x.%04x", cdev->hostid.cssid, >>> - cdev->hostid.ssid, >>> - cdev->hostid.devid); >> Digging into that few months later, >> >> new vfio_device_groupid uses >> >> + tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); >> >> which is set as a prop here >> DEFINE_PROP_STRING("sysfsdev", VFIOCCWDevice, vdev.sysfsdev), >> we need to double check if this matches, this is not obvious at first sight. At least >> if this is true this needs to be documented in the commit msg > > Good finding. Indeed, there is a gap here if we use a symbol link as sysfsdev. > See s390_ccw_get_dev_info() for details. But if it's not a symbol link, I think > they are same. > >> >> then the subchannel name is >> char *name = g_strdup_printf("%x.%x.%04x", cdev->hostid.cssid, >> cdev->hostid.ssid, >> cdev->hostid.devid); >> 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; >> } >> } >> >> while new code use >> + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { >> + if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) { >> + error_setg(errp, "device is already attached"); >> + vfio_put_group(group); >> + return -EBUSY; >> + } >> + } >> >> We really need to double check the names, ie. >> name, vbasedev->name. That's a mess and that's my bad. > > Thanks for catching, I think below change will make it same as original code: > > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c > index 6893a30ab1..a8ea0a6fa8 100644 > --- a/hw/vfio/ccw.c > +++ b/hw/vfio/ccw.c > @@ -580,6 +580,9 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp) > VFIODevice *vbasedev = &vcdev->vdev; > Error *err = NULL; > int ret; > + char *name = g_strdup_printf("%x.%x.%04x", cdev->hostid.cssid, > + cdev->hostid.ssid, > + cdev->hostid.devid); > > /* Call the class init function for subchannel. */ > if (cdc->realize) { > @@ -591,7 +594,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp) > > vbasedev->ops = &vfio_ccw_ops; > vbasedev->type = VFIO_DEVICE_TYPE_CCW; > - vbasedev->name = g_strdup(cdev->mdevid); > + vbasedev->name = name; > vbasedev->dev = &vcdev->cdev.parent_obj.parent_obj; > > /* > @@ -604,7 +607,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp) > */ > vbasedev->ram_block_discard_allowed = true; > > - ret = vfio_attach_device(vbasedev->name, vbasedev, > + ret = vfio_attach_device(cdev->mdevid, vbasedev, > &address_space_memory, errp); > if (ret) { > goto out_attach_dev_err; > > Thanks > Zhenzhong I haven't tried this particular version of the patches yet (either Eric F. or I will), but it looks like this change would re-introduce at least part of the breakage I reported during the RFC? https://lore.kernel.org/qemu-devel/6e04ab8f-dc84-e9c2-deea-2b6b31678b53@linux.ibm.com/ Thanks, Matt
Hi Eric, Matthew, >-----Original Message----- >From: Matthew Rosato <mjrosato@linux.ibm.com> >Sent: Wednesday, September 27, 2023 9:26 PM >Subject: Re: [PATCH v2 09/12] vfio/ccw: Use vfio_[attach/detach]_device > >On 9/27/23 8:09 AM, Duan, Zhenzhong wrote: >> >> >>> -----Original Message----- >>> From: Eric Auger <eric.auger@redhat.com> >>> Sent: Wednesday, September 27, 2023 6:00 PM >>> Subject: Re: [PATCH v2 09/12] vfio/ccw: Use vfio_[attach/detach]_device >>> >>> >>> >>> On 9/26/23 13:32, Zhenzhong Duan wrote: >>>> From: Eric Auger <eric.auger@redhat.com> >>>> >>>> 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> >>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>> --- >>>> include/hw/vfio/vfio-common.h | 5 -- >>>> hw/vfio/ccw.c | 115 ++++++++-------------------------- >>>> hw/vfio/common.c | 10 +-- >>>> 3 files changed, 30 insertions(+), 100 deletions(-) >>>> >>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio- >common.h >>>> index 12fbfbc37d..c486bdef2a 100644 >>>> --- a/include/hw/vfio/vfio-common.h >>>> +++ b/include/hw/vfio/vfio-common.h >>>> @@ -202,7 +202,6 @@ typedef struct { >>>> hwaddr pages; >>>> } VFIOBitmap; >>>> >>>> -void vfio_put_base_device(VFIODevice *vbasedev); >>>> void vfio_disable_irqindex(VFIODevice *vbasedev, int index); >>>> void vfio_unmask_single_irqindex(VFIODevice *vbasedev, int index); >>>> void vfio_mask_single_irqindex(VFIODevice *vbasedev, int index); >>>> @@ -220,11 +219,7 @@ 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); >>>> struct vfio_device_info *vfio_get_device_info(int fd); >>>> -int vfio_get_device(VFIOGroup *group, const char *name, >>>> - VFIODevice *vbasedev, Error **errp); >>>> int vfio_attach_device(char *name, 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 1e2fce83b0..6893a30ab1 100644 >>>> --- a/hw/vfio/ccw.c >>>> +++ b/hw/vfio/ccw.c >>>> @@ -572,88 +572,14 @@ 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) >>>> -{ >>>> - S390CCWDevice *cdev = S390_CCW_DEVICE(vcdev); >>>> - char *name = g_strdup_printf("%x.%x.%04x", cdev->hostid.cssid, >>>> - cdev->hostid.ssid, >>>> - cdev->hostid.devid); >>> Digging into that few months later, >>> >>> new vfio_device_groupid uses >>> >>> + tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev); >>> >>> which is set as a prop here >>> DEFINE_PROP_STRING("sysfsdev", VFIOCCWDevice, vdev.sysfsdev), >>> we need to double check if this matches, this is not obvious at first sight. At >least >>> if this is true this needs to be documented in the commit msg >> >> Good finding. Indeed, there is a gap here if we use a symbol link as sysfsdev. >> See s390_ccw_get_dev_info() for details. But if it's not a symbol link, I think >> they are same. Digged this further. I think it's ok even if a smbol link is provided to vbasedev->sysfsdev. Because we just want to get iommu group number. vfio_device_groupid() can survive even with a symbol link such as: /sys/bus/mdev/devices/7e270a25-e163-4922-af60-757fc8ed48c6/iommu_group >> >>> >>> then the subchannel name is >>> char *name = g_strdup_printf("%x.%x.%04x", cdev->hostid.cssid, >>> cdev->hostid.ssid, >>> cdev->hostid.devid); >>> 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; >>> } >>> } >>> >>> while new code use >>> + QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { >>> + if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) { >>> + error_setg(errp, "device is already attached"); >>> + vfio_put_group(group); >>> + return -EBUSY; >>> + } >>> + } >>> >>> We really need to double check the names, ie. >>> name, vbasedev->name. That's a mess and that's my bad. >> >> Thanks for catching, I think below change will make it same as original code: >> >> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c >> index 6893a30ab1..a8ea0a6fa8 100644 >> --- a/hw/vfio/ccw.c >> +++ b/hw/vfio/ccw.c >> @@ -580,6 +580,9 @@ static void vfio_ccw_realize(DeviceState *dev, Error >**errp) >> VFIODevice *vbasedev = &vcdev->vdev; >> Error *err = NULL; >> int ret; >> + char *name = g_strdup_printf("%x.%x.%04x", cdev->hostid.cssid, >> + cdev->hostid.ssid, >> + cdev->hostid.devid); >> >> /* Call the class init function for subchannel. */ >> if (cdc->realize) { >> @@ -591,7 +594,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error >**errp) >> >> vbasedev->ops = &vfio_ccw_ops; >> vbasedev->type = VFIO_DEVICE_TYPE_CCW; >> - vbasedev->name = g_strdup(cdev->mdevid); >> + vbasedev->name = name; >> vbasedev->dev = &vcdev->cdev.parent_obj.parent_obj; >> >> /* >> @@ -604,7 +607,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error >**errp) >> */ >> vbasedev->ram_block_discard_allowed = true; >> >> - ret = vfio_attach_device(vbasedev->name, vbasedev, >> + ret = vfio_attach_device(cdev->mdevid, vbasedev, >> &address_space_memory, errp); >> if (ret) { >> goto out_attach_dev_err; >> >> Thanks >> Zhenzhong > >I haven't tried this particular version of the patches yet (either Eric F. or I will), but >it looks like this change would re-introduce at least part of the breakage I >reported during the RFC? > >https://lore.kernel.org/qemu-devel/6e04ab8f-dc84-e9c2-deea- >2b6b31678b53@linux.ibm.com/ You are right, my mistake. I just remembered I have included your suggested change in above link to this patch. So no need to add more change here. It looks like your change also fixed a vbasedev->name issue, from "cssid.ssid.devid" to "mdevid". Look forward to your test result with this series, thanks! BRs. Zhenzhong
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h index 12fbfbc37d..c486bdef2a 100644 --- a/include/hw/vfio/vfio-common.h +++ b/include/hw/vfio/vfio-common.h @@ -202,7 +202,6 @@ typedef struct { hwaddr pages; } VFIOBitmap; -void vfio_put_base_device(VFIODevice *vbasedev); void vfio_disable_irqindex(VFIODevice *vbasedev, int index); void vfio_unmask_single_irqindex(VFIODevice *vbasedev, int index); void vfio_mask_single_irqindex(VFIODevice *vbasedev, int index); @@ -220,11 +219,7 @@ 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); struct vfio_device_info *vfio_get_device_info(int fd); -int vfio_get_device(VFIOGroup *group, const char *name, - VFIODevice *vbasedev, Error **errp); int vfio_attach_device(char *name, 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 1e2fce83b0..6893a30ab1 100644 --- a/hw/vfio/ccw.c +++ b/hw/vfio/ccw.c @@ -572,88 +572,14 @@ 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) -{ - S390CCWDevice *cdev = S390_CCW_DEVICE(vcdev); - char *name = g_strdup_printf("%x.%x.%04x", cdev->hostid.cssid, - cdev->hostid.ssid, - cdev->hostid.devid); - VFIODevice *vbasedev; - - 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; - } - } - - /* - * All vfio-ccw devices are believed to operate in a way compatible with - * discarding of memory in RAM blocks, ie. pages pinned in the host are - * in the current working set of the guest driver and therefore never - * overlap e.g., with pages available to the guest balloon driver. This - * 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, cdev->mdevid, &vcdev->vdev, errp)) { - goto out_err; - } - - vcdev->vdev.ops = &vfio_ccw_ops; - vcdev->vdev.type = VFIO_DEVICE_TYPE_CCW; - vcdev->vdev.name = name; - vcdev->vdev.dev = DEVICE(vcdev); - - 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; S390CCWDevice *cdev = S390_CCW_DEVICE(dev); VFIOCCWDevice *vcdev = VFIO_CCW(cdev); S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev); + VFIODevice *vbasedev = &vcdev->vdev; Error *err = NULL; + int ret; /* Call the class init function for subchannel. */ if (cdc->realize) { @@ -663,14 +589,25 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp) } } - group = vfio_ccw_get_group(cdev, &err); - if (!group) { - goto out_group_err; - } + vbasedev->ops = &vfio_ccw_ops; + vbasedev->type = VFIO_DEVICE_TYPE_CCW; + vbasedev->name = g_strdup(cdev->mdevid); + vbasedev->dev = &vcdev->cdev.parent_obj.parent_obj; - vfio_ccw_get_device(group, vcdev, &err); - if (err) { - goto out_device_err; + /* + * All vfio-ccw devices are believed to operate in a way compatible with + * discarding of memory in RAM blocks, ie. pages pinned in the host are + * in the current working set of the guest driver and therefore never + * overlap e.g., with pages available to the guest balloon driver. This + * needs to be set before vfio_get_device() for vfio common to handle + * ram_block_discard_disable(). + */ + vbasedev->ram_block_discard_allowed = true; + + ret = vfio_attach_device(vbasedev->name, vbasedev, + &address_space_memory, errp); + if (ret) { + goto out_attach_dev_err; } vfio_ccw_get_region(vcdev, &err); @@ -708,10 +645,9 @@ out_irq_notifier_err: 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: + vfio_detach_device(vbasedev); +out_attach_dev_err: + g_free(vbasedev->name); if (cdc->unrealize) { cdc->unrealize(cdev); } @@ -724,14 +660,13 @@ static void vfio_ccw_unrealize(DeviceState *dev) S390CCWDevice *cdev = S390_CCW_DEVICE(dev); VFIOCCWDevice *vcdev = VFIO_CCW(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); + g_free(vcdev->vdev.name); if (cdc->unrealize) { cdc->unrealize(cdev); diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 7f3798b152..65516b319e 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -2335,7 +2335,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; char path[32]; @@ -2402,7 +2402,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; @@ -2447,8 +2447,8 @@ retry: return info; } -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) { g_autofree struct vfio_device_info *info = NULL; int fd; @@ -2506,7 +2506,7 @@ int vfio_get_device(VFIOGroup *group, const char *name, return 0; } -void vfio_put_base_device(VFIODevice *vbasedev) +static void vfio_put_base_device(VFIODevice *vbasedev) { if (!vbasedev->group) { return;