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.
> ---
> 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;
