Hi Eric, >-----Original Message----- >From: qemu-devel-bounces+zhenzhong.duan=intel....@nongnu.org <qemu- >devel-bounces+zhenzhong.duan=intel....@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.au...@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.au...@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.au...@redhat.com> >>>>> Signed-off-by: Yi Liu <yi.l....@intel.com> >>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@intel.com> >>>>> Reviewed-by: Matthew Rosato <mjros...@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- >>>>> 2b6b31678...@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-2b6b31678...@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- >26a097a89...@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- >2b6b31678...@linux.ibm.com/ >). Thanks Eric OK, then go ahead. Thanks Zhenzhong