diff mbox series

[v2,09/12] vfio/ccw: Use vfio_[attach/detach]_device

Message ID 20230926113255.1177834-10-zhenzhong.duan@intel.com
State New
Headers show
Series Prerequisite change for IOMMUFD support | expand

Commit Message

Zhenzhong Duan Sept. 26, 2023, 11:32 a.m. UTC 
From: Eric Auger <eric.auger@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.auger@redhat.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@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(-)

Comments

Eric Auger Sept. 27, 2023, 10 a.m. UTC | #1 
On 9/26/23 13:32, Zhenzhong Duan wrote:
> From: Eric Auger <eric.auger@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.auger@redhat.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@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

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

Eric


> -    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;
> +    int ret;
>  
>      /* Call the class init function for subchannel. */
>      if (cdc->realize) {
> @@ -663,14 +589,25 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
>          }
>      }
>  
> -    group = vfio_ccw_get_group(cdev, &err);
> -    if (!group) {
> -        goto out_group_err;
> -    }
> +    vbasedev->ops = &vfio_ccw_ops;
> +    vbasedev->type = VFIO_DEVICE_TYPE_CCW;
> +    vbasedev->name = g_strdup(cdev->mdevid);
> +    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(vbasedev->name, vbasedev,
> +                             &address_space_memory, errp);
> +    if (ret) {
> +        goto out_attach_dev_err;
>      }
>  
>      vfio_ccw_get_region(vcdev, &err);
> @@ -708,10 +645,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 +660,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 7f3798b152..65516b319e 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;
Zhenzhong Duan Sept. 27, 2023, 12:09 p.m. UTC | #2 
>-----Original Message-----
>From: Eric Auger <eric.auger@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.auger@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.auger@redhat.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@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
Matthew Rosato Sept. 27, 2023, 1:25 p.m. UTC | #3 
On 9/27/23 8:09 AM, Duan, Zhenzhong wrote:
> 
> 
>> -----Original Message-----
>> From: Eric Auger <eric.auger@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.auger@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.auger@redhat.com>
>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@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-2b6b31678b53@linux.ibm.com/

Thanks,
Matt
Zhenzhong Duan Sept. 28, 2023, 2:55 a.m. UTC | #4 
Hi Eric, Matthew,

>-----Original Message-----
>From: Matthew Rosato <mjrosato@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.auger@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.auger@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.auger@redhat.com>
>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@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-
>2b6b31678b53@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
diff mbox series

Patch

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);
-    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;
+    int ret;
 
     /* Call the class init function for subchannel. */
     if (cdc->realize) {
@@ -663,14 +589,25 @@  static void vfio_ccw_realize(DeviceState *dev, Error **errp)
         }
     }
 
-    group = vfio_ccw_get_group(cdev, &err);
-    if (!group) {
-        goto out_group_err;
-    }
+    vbasedev->ops = &vfio_ccw_ops;
+    vbasedev->type = VFIO_DEVICE_TYPE_CCW;
+    vbasedev->name = g_strdup(cdev->mdevid);
+    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(vbasedev->name, vbasedev,
+                             &address_space_memory, errp);
+    if (ret) {
+        goto out_attach_dev_err;
     }
 
     vfio_ccw_get_region(vcdev, &err);
@@ -708,10 +645,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 +660,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 7f3798b152..65516b319e 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;