Re: [PATCH v3 10/15] vfio/ccw: Use vfio_[attach/detach]_device

[Eric Auger]

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.auger@redhat.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@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-2b6b31678b53@linux.ibm.com/
> Reviewed-by: Matthew Rosato <mjrosato@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;