| Message ID | 20231004154518.334760-11-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Prerequisite changes for IOMMUFD support | expand |
Hi Eric, >-----Original Message----- >From: Eric Auger <eric.auger@redhat.com> >Sent: Wednesday, October 4, 2023 11:44 PM >Subject: [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device > >Let the vfio-ccw device use vfio_attach_device() and >vfio_detach_device(), hence hiding the details of the used >IOMMU backend. > >Note that the migration reduces the following trace >"vfio: subchannel %s has already been attached" (featuring >cssid.ssid.devid) into "device is already attached" > >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> >Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com> > >--- > >v3: >- simplified vbasedev->dev setting > >v2 -> v3: >- Hopefully fix confusion beteen vbasedev->name, mdevid and sysfsdev > while keeping into account Matthew's comment > https://lore.kernel.org/qemu-devel/6e04ab8f-dc84-e9c2-deea- >2b6b31678b53@linux.ibm.com/ >--- > include/hw/vfio/vfio-common.h | 5 -- > hw/vfio/ccw.c | 122 +++++++++------------------------- > hw/vfio/common.c | 10 +-- > 3 files changed, 37 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..6ec35fedc9 100644 >--- a/hw/vfio/ccw.c >+++ b/hw/vfio/ccw.c >@@ -572,88 +572,15 @@ 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; >+ char *name; >+ int ret; > > /* Call the class init function for subchannel. */ > if (cdc->realize) { >@@ -663,14 +590,31 @@ static void vfio_ccw_realize(DeviceState *dev, Error >**errp) > } > } > >- group = vfio_ccw_get_group(cdev, &err); >- if (!group) { >- goto out_group_err; >- } >+ name = g_strdup_printf("%x.%x.%04x", vcdev->cdev.hostid.cssid, >+ vcdev->cdev.hostid.ssid, >+ vcdev->cdev.hostid.devid); >+ vbasedev->sysfsdev = g_strdup_printf("/sys/bus/css/devices/%s/%s", >+ name, >+ cdev->mdevid); Hoping not late for you to include this in v5. I think no need to re-assign sysfsdev as it's a user property, we'd better to keep the original user value. Also looks a memory leak here. >+ vbasedev->ops = &vfio_ccw_ops; >+ vbasedev->type = VFIO_DEVICE_TYPE_CCW; >+ vbasedev->name = name; There will be a potential failure when a second mdev device under same cssid.ssid.devid attached. We can use cdev->mdevid as name. Maybe you can use v2 of this patch, I remember these two issues are already addressed in v2. Thanks Zhenzhong
Hi Zhenzhong, On 10/8/23 12:21, Duan, Zhenzhong wrote: > Hi Eric, > >> -----Original Message----- >> From: Eric Auger <eric.auger@redhat.com> >> Sent: Wednesday, October 4, 2023 11:44 PM >> Subject: [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device >> >> Let the vfio-ccw device use vfio_attach_device() and >> vfio_detach_device(), hence hiding the details of the used >> IOMMU backend. >> >> Note that the migration reduces the following trace >> "vfio: subchannel %s has already been attached" (featuring >> cssid.ssid.devid) into "device is already attached" >> >> 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> >> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com> >> >> --- >> >> v3: >> - simplified vbasedev->dev setting >> >> v2 -> v3: >> - Hopefully fix confusion beteen vbasedev->name, mdevid and sysfsdev >> while keeping into account Matthew's comment >> https://lore.kernel.org/qemu-devel/6e04ab8f-dc84-e9c2-deea- >> 2b6b31678b53@linux.ibm.com/ >> --- >> include/hw/vfio/vfio-common.h | 5 -- >> hw/vfio/ccw.c | 122 +++++++++------------------------- >> hw/vfio/common.c | 10 +-- >> 3 files changed, 37 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..6ec35fedc9 100644 >> --- a/hw/vfio/ccw.c >> +++ b/hw/vfio/ccw.c >> @@ -572,88 +572,15 @@ 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; >> + char *name; >> + int ret; >> >> /* Call the class init function for subchannel. */ >> if (cdc->realize) { >> @@ -663,14 +590,31 @@ static void vfio_ccw_realize(DeviceState *dev, Error >> **errp) >> } >> } >> >> - group = vfio_ccw_get_group(cdev, &err); >> - if (!group) { >> - goto out_group_err; >> - } >> + name = g_strdup_printf("%x.%x.%04x", vcdev->cdev.hostid.cssid, >> + vcdev->cdev.hostid.ssid, >> + vcdev->cdev.hostid.devid); >> + vbasedev->sysfsdev = g_strdup_printf("/sys/bus/css/devices/%s/%s", >> + name, >> + cdev->mdevid); > Hoping not late for you to include this in v5. > I think no need to re-assign sysfsdev as it's a user property, we'd better to > keep the original user value. Also looks a memory leak here. OK I removed it. > >> + vbasedev->ops = &vfio_ccw_ops; >> + vbasedev->type = VFIO_DEVICE_TYPE_CCW; >> + vbasedev->name = name; > There will be a potential failure when a second mdev device under > same cssid.ssid.devid attached. We can use cdev->mdevid as name. But this mathes vfio_ccw_get_device() existing code where vcdev->vdev.name = name; and name = g_strdup_printf("%x.%x.%04x", cdev->hostid.cssid, cdev->hostid.ssid, cdev->hostid.devid); cdev->mdevid is passed as first arg of vfio_attach_device() instead . i think this also matches https://lore.kernel.org/all/PH7PR11MB67222DD282F98E03095FBA8A92C1A@PH7PR11MB6722.namprd11.prod.outlook.com/ no? Thanks Eric > > Maybe you can use v2 of this patch, I remember these two issues are already addressed in v2. > > Thanks > Zhenzhong > >
>-----Original Message----- >From: Eric Auger <eric.auger@redhat.com> >Sent: Monday, October 9, 2023 1:46 AM >Subject: Re: [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device > >Hi Zhenzhong, >On 10/8/23 12:21, Duan, Zhenzhong wrote: >> Hi Eric, >> >>> -----Original Message----- >>> From: Eric Auger <eric.auger@redhat.com> >>> Sent: Wednesday, October 4, 2023 11:44 PM >>> Subject: [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device >>> >>> Let the vfio-ccw device use vfio_attach_device() and >>> vfio_detach_device(), hence hiding the details of the used >>> IOMMU backend. >>> >>> Note that the migration reduces the following trace >>> "vfio: subchannel %s has already been attached" (featuring >>> cssid.ssid.devid) into "device is already attached" >>> >>> 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> >>> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com> >>> >>> --- >>> >>> v3: >>> - simplified vbasedev->dev setting >>> >>> v2 -> v3: >>> - Hopefully fix confusion beteen vbasedev->name, mdevid and sysfsdev >>> while keeping into account Matthew's comment >>> https://lore.kernel.org/qemu-devel/6e04ab8f-dc84-e9c2-deea- >>> 2b6b31678b53@linux.ibm.com/ >>> --- >>> include/hw/vfio/vfio-common.h | 5 -- >>> hw/vfio/ccw.c | 122 +++++++++------------------------- >>> hw/vfio/common.c | 10 +-- >>> 3 files changed, 37 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..6ec35fedc9 100644 >>> --- a/hw/vfio/ccw.c >>> +++ b/hw/vfio/ccw.c >>> @@ -572,88 +572,15 @@ 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; >>> + char *name; >>> + int ret; >>> >>> /* Call the class init function for subchannel. */ >>> if (cdc->realize) { >>> @@ -663,14 +590,31 @@ static void vfio_ccw_realize(DeviceState *dev, Error >>> **errp) >>> } >>> } >>> >>> - group = vfio_ccw_get_group(cdev, &err); >>> - if (!group) { >>> - goto out_group_err; >>> - } >>> + name = g_strdup_printf("%x.%x.%04x", vcdev->cdev.hostid.cssid, >>> + vcdev->cdev.hostid.ssid, >>> + vcdev->cdev.hostid.devid); >>> + vbasedev->sysfsdev = g_strdup_printf("/sys/bus/css/devices/%s/%s", >>> + name, >>> + cdev->mdevid); >> Hoping not late for you to include this in v5. >> I think no need to re-assign sysfsdev as it's a user property, we'd better to >> keep the original user value. Also looks a memory leak here. >OK I removed it. >> >>> + vbasedev->ops = &vfio_ccw_ops; >>> + vbasedev->type = VFIO_DEVICE_TYPE_CCW; >>> + vbasedev->name = name; >> There will be a potential failure when a second mdev device under >> same cssid.ssid.devid attached. We can use cdev->mdevid as name. >But this mathes vfio_ccw_get_device() existing code where >vcdev->vdev.name = name; and >name = g_strdup_printf("%x.%x.%04x", cdev->hostid.cssid, > cdev->hostid.ssid, > cdev->hostid.devid); I suspect this is a bug of the existing code. > >cdev->mdevid is passed as first arg of vfio_attach_device() instead . vfio_attach_device() uses cdev->mdevid to get device FD, nothing more. If we use cssid.ssid.devid as name, then different mdev under same cssid.ssid.devid will have same name, and the second mdev attachment will fail to attach in vfio_attach_device(): 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; } } > >i think this also matches >https://lore.kernel.org/all/PH7PR11MB67222DD282F98E03095FBA8A92C1A@PH >7PR11MB6722.namprd11.prod.outlook.com/ >no? It doesn't match what Mattew suggested: https://lore.kernel.org/qemu-devel/6e04ab8f-dc84-e9c2-deea-2b6b31678b53@linux.ibm.com/ Thanks Zhenzhong
Hi Zhenzhong, On 10/9/23 03:25, Duan, Zhenzhong wrote: > >> -----Original Message----- >> From: Eric Auger <eric.auger@redhat.com> >> Sent: Monday, October 9, 2023 1:46 AM >> Subject: Re: [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device >> >> Hi Zhenzhong, >> On 10/8/23 12:21, Duan, Zhenzhong wrote: >>> Hi Eric, >>> >>>> -----Original Message----- >>>> From: Eric Auger <eric.auger@redhat.com> >>>> Sent: Wednesday, October 4, 2023 11:44 PM >>>> Subject: [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device >>>> >>>> Let the vfio-ccw device use vfio_attach_device() and >>>> vfio_detach_device(), hence hiding the details of the used >>>> IOMMU backend. >>>> >>>> Note that the migration reduces the following trace >>>> "vfio: subchannel %s has already been attached" (featuring >>>> cssid.ssid.devid) into "device is already attached" >>>> >>>> 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> >>>> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com> >>>> >>>> --- >>>> >>>> v3: >>>> - simplified vbasedev->dev setting >>>> >>>> v2 -> v3: >>>> - Hopefully fix confusion beteen vbasedev->name, mdevid and sysfsdev >>>> while keeping into account Matthew's comment >>>> https://lore.kernel.org/qemu-devel/6e04ab8f-dc84-e9c2-deea- >>>> 2b6b31678b53@linux.ibm.com/ >>>> --- >>>> include/hw/vfio/vfio-common.h | 5 -- >>>> hw/vfio/ccw.c | 122 +++++++++------------------------- >>>> hw/vfio/common.c | 10 +-- >>>> 3 files changed, 37 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..6ec35fedc9 100644 >>>> --- a/hw/vfio/ccw.c >>>> +++ b/hw/vfio/ccw.c >>>> @@ -572,88 +572,15 @@ 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; >>>> + char *name; >>>> + int ret; >>>> >>>> /* Call the class init function for subchannel. */ >>>> if (cdc->realize) { >>>> @@ -663,14 +590,31 @@ static void vfio_ccw_realize(DeviceState *dev, Error >>>> **errp) >>>> } >>>> } >>>> >>>> - group = vfio_ccw_get_group(cdev, &err); >>>> - if (!group) { >>>> - goto out_group_err; >>>> - } >>>> + name = g_strdup_printf("%x.%x.%04x", vcdev->cdev.hostid.cssid, >>>> + vcdev->cdev.hostid.ssid, >>>> + vcdev->cdev.hostid.devid); >>>> + vbasedev->sysfsdev = g_strdup_printf("/sys/bus/css/devices/%s/%s", >>>> + name, >>>> + cdev->mdevid); >>> Hoping not late for you to include this in v5. >>> I think no need to re-assign sysfsdev as it's a user property, we'd better to >>> keep the original user value. Also looks a memory leak here. >> OK I removed it. >>>> + vbasedev->ops = &vfio_ccw_ops; >>>> + vbasedev->type = VFIO_DEVICE_TYPE_CCW; >>>> + vbasedev->name = name; >>> There will be a potential failure when a second mdev device under >>> same cssid.ssid.devid attached. We can use cdev->mdevid as name. >> But this mathes vfio_ccw_get_device() existing code where >> vcdev->vdev.name = name; and >> name = g_strdup_printf("%x.%x.%04x", cdev->hostid.cssid, >> cdev->hostid.ssid, >> cdev->hostid.devid); > I suspect this is a bug of the existing code. Then I would prefer we fix it separately. This patch migrates the existing code without functional change intended. > >> cdev->mdevid is passed as first arg of vfio_attach_device() instead . > vfio_attach_device() uses cdev->mdevid to get device FD, nothing more. > > If we use cssid.ssid.devid as name, then different mdev under same cssid.ssid.devid will have same name, and the second mdev attachment will fail to attach in vfio_attach_device(): > > 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; > } > } I get your point but this conversion matches the existing vfio_ccw_get_device() code: 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; } } > >> i think this also matches >> https://lore.kernel.org/all/PH7PR11MB67222DD282F98E03095FBA8A92C1A@PH >> 7PR11MB6722.namprd11.prod.outlook.com/ >> no? > It doesn't match what Mattew suggested: https://lore.kernel.org/qemu-devel/6e04ab8f-dc84-e9c2-deea-2b6b31678b53@linux.ibm.com/ this was RFC v3. At that time we did not pass any "name" arg to vfio_attach_device(VFIODevice *vbasedev, AddressSpace *as, Error **errp) and vbasedev->name was used when calling vfio_get_device() while we now use cdev->mdevid. Besides Mattew ran some basic tests on PATCH v3: https://lore.kernel.org/all/33b7803c-f231-d4fb-d9d9-26a097a89e93@redhat.com/ So I would be tempted to leave it as is (without the sysfsdev overwrite which came from Mattew's suggestion in https://lore.kernel.org/qemu-devel/6e04ab8f-dc84-e9c2-deea-2b6b31678b53@linux.ibm.com/ ). Thanks Eric > > Thanks > Zhenzhong >
Hi Eric, >-----Original Message----- >From: qemu-devel-bounces+zhenzhong.duan=intel.com@nongnu.org <qemu- >devel-bounces+zhenzhong.duan=intel.com@nongnu.org> On Behalf Of Eric >Auger >Sent: Monday, October 9, 2023 4:15 PM >Subject: Re: [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device > >Hi Zhenzhong, > >On 10/9/23 03:25, Duan, Zhenzhong wrote: >> >>> -----Original Message----- >>> From: Eric Auger <eric.auger@redhat.com> >>> Sent: Monday, October 9, 2023 1:46 AM >>> Subject: Re: [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device >>> >>> Hi Zhenzhong, >>> On 10/8/23 12:21, Duan, Zhenzhong wrote: >>>> Hi Eric, >>>> >>>>> -----Original Message----- >>>>> From: Eric Auger <eric.auger@redhat.com> >>>>> Sent: Wednesday, October 4, 2023 11:44 PM >>>>> Subject: [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device >>>>> >>>>> Let the vfio-ccw device use vfio_attach_device() and >>>>> vfio_detach_device(), hence hiding the details of the used >>>>> IOMMU backend. >>>>> >>>>> Note that the migration reduces the following trace >>>>> "vfio: subchannel %s has already been attached" (featuring >>>>> cssid.ssid.devid) into "device is already attached" >>>>> >>>>> 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> >>>>> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com> >>>>> >>>>> --- >>>>> >>>>> v3: >>>>> - simplified vbasedev->dev setting >>>>> >>>>> v2 -> v3: >>>>> - Hopefully fix confusion beteen vbasedev->name, mdevid and sysfsdev >>>>> while keeping into account Matthew's comment >>>>> https://lore.kernel.org/qemu-devel/6e04ab8f-dc84-e9c2-deea- >>>>> 2b6b31678b53@linux.ibm.com/ >>>>> --- >>>>> include/hw/vfio/vfio-common.h | 5 -- >>>>> hw/vfio/ccw.c | 122 +++++++++------------------------- >>>>> hw/vfio/common.c | 10 +-- >>>>> 3 files changed, 37 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..6ec35fedc9 100644 >>>>> --- a/hw/vfio/ccw.c >>>>> +++ b/hw/vfio/ccw.c >>>>> @@ -572,88 +572,15 @@ 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; >>>>> + char *name; >>>>> + int ret; >>>>> >>>>> /* Call the class init function for subchannel. */ >>>>> if (cdc->realize) { >>>>> @@ -663,14 +590,31 @@ static void vfio_ccw_realize(DeviceState *dev, >Error >>>>> **errp) >>>>> } >>>>> } >>>>> >>>>> - group = vfio_ccw_get_group(cdev, &err); >>>>> - if (!group) { >>>>> - goto out_group_err; >>>>> - } >>>>> + name = g_strdup_printf("%x.%x.%04x", vcdev->cdev.hostid.cssid, >>>>> + vcdev->cdev.hostid.ssid, >>>>> + vcdev->cdev.hostid.devid); >>>>> + vbasedev->sysfsdev = g_strdup_printf("/sys/bus/css/devices/%s/%s", >>>>> + name, >>>>> + cdev->mdevid); >>>> Hoping not late for you to include this in v5. >>>> I think no need to re-assign sysfsdev as it's a user property, we'd better to >>>> keep the original user value. Also looks a memory leak here. >>> OK I removed it. >>>>> + vbasedev->ops = &vfio_ccw_ops; >>>>> + vbasedev->type = VFIO_DEVICE_TYPE_CCW; >>>>> + vbasedev->name = name; >>>> There will be a potential failure when a second mdev device under >>>> same cssid.ssid.devid attached. We can use cdev->mdevid as name. >>> But this mathes vfio_ccw_get_device() existing code where >>> vcdev->vdev.name = name; and >>> name = g_strdup_printf("%x.%x.%04x", cdev->hostid.cssid, >>> cdev->hostid.ssid, >>> cdev->hostid.devid); >> I suspect this is a bug of the existing code. >Then I would prefer we fix it separately. This patch migrates the >existing code without functional change intended. OK, make sense. I'll do that after your v5 get in. > >> >>> cdev->mdevid is passed as first arg of vfio_attach_device() instead . >> vfio_attach_device() uses cdev->mdevid to get device FD, nothing more. >> >> If we use cssid.ssid.devid as name, then different mdev under same >cssid.ssid.devid will have same name, and the second mdev attachment will fail to >attach in vfio_attach_device(): >> >> 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; >> } >> } >I get your point but this conversion matches the existing >vfio_ccw_get_device() code: > 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; > } > } > >> >>> i think this also matches >>> >https://lore.kernel.org/all/PH7PR11MB67222DD282F98E03095FBA8A92C1A@PH >>> 7PR11MB6722.namprd11.prod.outlook.com/ >>> no? >> It doesn't match what Mattew suggested: https://lore.kernel.org/qemu- >devel/6e04ab8f-dc84-e9c2-deea-2b6b31678b53@linux.ibm.com/ >this was RFC v3. At that time we did not pass any "name" arg to > >vfio_attach_device(VFIODevice *vbasedev, AddressSpace *as, Error **errp) >and vbasedev->name was used when calling vfio_get_device() while we now >use cdev->mdevid. Besides Mattew ran some basic tests on PATCH v3: >https://lore.kernel.org/all/33b7803c-f231-d4fb-d9d9- >26a097a89e93@redhat.com/ >So I would be tempted to leave it as is (without the sysfsdev overwrite >which came from Mattew's suggestion in >https://lore.kernel.org/qemu-devel/6e04ab8f-dc84-e9c2-deea- >2b6b31678b53@linux.ibm.com/ >). Thanks Eric OK, then go ahead. Thanks 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..6ec35fedc9 100644 --- a/hw/vfio/ccw.c +++ b/hw/vfio/ccw.c @@ -572,88 +572,15 @@ 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; + char *name; + int ret; /* Call the class init function for subchannel. */ if (cdc->realize) { @@ -663,14 +590,31 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp) } } - group = vfio_ccw_get_group(cdev, &err); - if (!group) { - goto out_group_err; - } + name = g_strdup_printf("%x.%x.%04x", vcdev->cdev.hostid.cssid, + vcdev->cdev.hostid.ssid, + vcdev->cdev.hostid.devid); + 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 = dev; - 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(cdev->mdevid, vbasedev, + &address_space_memory, errp); + if (ret) { + goto out_attach_dev_err; } vfio_ccw_get_region(vcdev, &err); @@ -708,10 +652,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 +667,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 f4c33c9858..56cfe94d97 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;