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.
>
>>
>> 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/
Thanks,
Matt
