Hi Eric, Matthew, >-----Original Message----- >From: Matthew Rosato <mjros...@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.au...@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.au...@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.au...@redhat.com> >>>> Signed-off-by: Yi Liu <yi.l....@intel.com> >>>> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@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- >2b6b31678...@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