Re: [PATCH v2 08/12] vfio/ap: Use vfio_[attach/detach]_device

[Eric Auger]

Hi Zhenzhong,

On 9/26/23 13:32, Zhenzhong Duan wrote:
> From: Eric Auger <eric.auger@redhat.com>
>
> Let the vfio-ap device use vfio_attach_device() and
> vfio_detach_device(), hence hiding the details of the used
> IOMMU backend.
>
> 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>
> ---
>  hw/vfio/ap.c | 68 +++++++++-------------------------------------------
>  1 file changed, 11 insertions(+), 57 deletions(-)
>
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 6e21d1da5a..16ea7fb3c2 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -53,40 +53,6 @@ struct VFIODeviceOps vfio_ap_ops = {
>      .vfio_compute_needs_reset = vfio_ap_compute_needs_reset,
>  };
>  
> -static void vfio_ap_put_device(VFIOAPDevice *vapdev)
> -{
> -    g_free(vapdev->vdev.name);
> -    vfio_put_base_device(&vapdev->vdev);
> -}
> -
> -static VFIOGroup *vfio_ap_get_group(VFIOAPDevice *vapdev, Error **errp)
> -{
> -    GError *gerror = NULL;
> -    char *symlink, *group_path;
> -    int groupid;
> -
> -    symlink = g_strdup_printf("%s/iommu_group", vapdev->vdev.sysfsdev);
> -    group_path = g_file_read_link(symlink, &gerror);
> -    g_free(symlink);
> -
> -    if (!group_path) {
> -        error_setg(errp, "%s: no iommu_group found for %s: %s",
> -                   TYPE_VFIO_AP_DEVICE, vapdev->vdev.sysfsdev, 
> gerror->message);
> -        g_error_free(gerror);
> -        return NULL;
> -    }
> -
> -    if (sscanf(basename(group_path), "%d", &groupid) != 1) {
> -        error_setg(errp, "vfio: failed to read %s", group_path);
> -        g_free(group_path);
> -        return NULL;
> -    }
> -
> -    g_free(group_path);
> -
> -    return vfio_get_group(groupid, &address_space_memory, errp);
> -}
> -
>  static void vfio_ap_req_notifier_handler(void *opaque)
>  {
>      VFIOAPDevice *vapdev = opaque;
> @@ -189,22 +155,15 @@ static void 
> vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev,
>  static void vfio_ap_realize(DeviceState *dev, Error **errp)
>  {
>      int ret;
> -    char *mdevid;
>      Error *err = NULL;
> -    VFIOGroup *vfio_group;
>      APDevice *apdev = AP_DEVICE(dev);
>      VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev);
> +    VFIODevice *vbasedev = &vapdev->vdev;
>  
> -    vfio_group = vfio_ap_get_group(vapdev, errp);
> -    if (!vfio_group) {
> -        return;
> -    }
> -
> -    vapdev->vdev.ops = &vfio_ap_ops;
> -    vapdev->vdev.type = VFIO_DEVICE_TYPE_AP;
> -    mdevid = basename(vapdev->vdev.sysfsdev);
> -    vapdev->vdev.name = g_strdup_printf("%s", mdevid);
> -    vapdev->vdev.dev = dev;
> +    vbasedev->name = g_path_get_basename(vbasedev->sysfsdev);
I think we shall document in the commit msg the fact we use

g_path_get_basename instead of basename here to match other device init
see 3e015d815b  use g_path_get_basename instead of basename

also leak of vbasedev->name

Eric

> +    vbasedev->ops = &vfio_ap_ops;
> +    vbasedev->type = VFIO_DEVICE_TYPE_AP;
> +    vbasedev->dev = dev;
>  
>      /*
>       * vfio-ap devices operate in a way compatible with discarding of
> @@ -214,9 +173,11 @@ static void vfio_ap_realize(DeviceState *dev, Error 
> **errp)
>       */
>      vapdev->vdev.ram_block_discard_allowed = true;
>  
> -    ret = vfio_get_device(vfio_group, mdevid, &vapdev->vdev, errp);
> +    ret = vfio_attach_device(vbasedev->name, vbasedev,
> +                             &address_space_memory, errp);
>      if (ret) {
> -        goto out_get_dev_err;
> +        g_free(vbasedev->name);
> +        return;
>      }
>  
>      vfio_ap_register_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX, &err);
> @@ -227,23 +188,16 @@ static void vfio_ap_realize(DeviceState *dev, Error 
> **errp)
>           */
>          error_report_err(err);
>      }
> -
> -    return;
> -
> -out_get_dev_err:
> -    vfio_ap_put_device(vapdev);
> -    vfio_put_group(vfio_group);
>  }
>  
>  static void vfio_ap_unrealize(DeviceState *dev)
>  {
>      APDevice *apdev = AP_DEVICE(dev);
>      VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev);
> -    VFIOGroup *group = vapdev->vdev.group;
>  
>      vfio_ap_unregister_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX);
> -    vfio_ap_put_device(vapdev);
> -    vfio_put_group(group);
> +    vfio_detach_device(&vapdev->vdev);
> +    g_free(vapdev->vdev.name);
>  }
>  
>  static Property vfio_ap_properties[] = {