Re: [PATCH v3 09/15] vfio/ap: Use vfio_[attach/detach]_device

["Cédric Le Goater" <clg@redhat.com>]

On 10/3/23 12:14, Eric Auger wrote:
> Let the vfio-ap device use vfio_attach_device() and
> vfio_detach_device(), hence hiding the details of the used
> IOMMU backend.
> 
> We take the opportunity to use g_path_get_basename() which
> is prefered, as suggested by
> 3e015d815b ("use g_path_get_basename instead of basename")
> 
> 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:
> - Mention g_path_get_basename in commit message and properly free
>    vbasedev->name, call vfio_detach_device
> ---
>   hw/vfio/ap.c | 70 ++++++++++------------------------------------------
>   1 file changed, 13 insertions(+), 57 deletions(-)
> 
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 6e21d1da5a..d0b587b3b1 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);
> +    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);
> @@ -225,25 +186,20 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
>            * Report this error, but do not make it a failing condition.
>            * Lack of this IRQ in the host does not prevent normal operation.
>            */
> +        vfio_detach_device(vbasedev);
>           error_report_err(err);
> +        g_free(vbasedev->name);
>       }
> -
> -    return;
> -
> -out_get_dev_err:
> -    vfio_ap_put_device(vapdev);
> -    vfio_put_group(vfio_group);
>   }


To be consistent with vfio_(pci)_realize(), I would introduce the same
failure path at the end the routine :

   out_detach:
       vfio_detach_device(vbasedev);
   error:
       error_prepend(errp, VFIO_MSG_PREFIX, vbasedev->name);
       g_free(vbasedev->name);


and add the VFIO_MSG_PREFIX while we are at it.

This is minor, so :

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.



>   
>   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[] = {