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
