Hi Matthew,
On 10/4/23 01:01, Matthew Rosato wrote:
> On 10/3/23 6:14 AM, Eric Auger wrote:
>> 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>
>>
>> ---
>> 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/
> Reviewed-by: Matthew Rosato <mjros...@linux.ibm.com>
>
> Tested out using some vfio-ccw devices and verified attach/detach worked /
> sysfsdev was correct.
Thank you very much for the testing and teh review. I dare to keep your
R-b despite the vbasedev->dev = dev setting simplification as pointed
out by Cédric.
Eric
>
>> ---
>> 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..84eafb2e87 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 = &vcdev->cdev.parent_obj.parent_obj;
>>
>> - 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;
