Re: [PATCH v3 27/37] vfio/iommufd: Switch to manual hwpt allocation

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

On 10/26/23 12:30, Zhenzhong Duan wrote:
> IOMMUFD supports auto allocated hwpt and manually allocated one.
> Manually allocated hwpt has benefit that its life cycle is under
> user's control, so it could be used as stage 2 page table by nested
> feature in the future.

Would an option be useful to switch from one mode to another ?

> 
> Introduce two helpers __vfio_device_attach/detach_hwpt to facilitate
> this change.

I think merging this change with the previous patch makes sense.
It doesn't add much to keep it as a standalone patch unless we
want a feature toggle.

Thanks,

C.


> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   hw/vfio/iommufd.c | 89 +++++++++++++++++++++++++++++++++++++----------
>   1 file changed, 70 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index aee64d63f3..c1daaf1c39 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -217,38 +217,91 @@ static VFIOIOASHwpt *vfio_container_get_hwpt(VFIOIOMMUFDContainer *container,
>   static void vfio_container_put_hwpt(IOMMUFDBackend *be, VFIOIOASHwpt *hwpt)
>   {
>       QLIST_REMOVE(hwpt, next);
> +    iommufd_backend_free_id(be->fd, hwpt->hwpt_id);
>       g_free(hwpt);
>   }
>   
> -static int vfio_device_attach_container(VFIODevice *vbasedev,
> -                                        VFIOIOMMUFDContainer *container,
> -                                        Error **errp)
> +static int __vfio_device_attach_hwpt(VFIODevice *vbasedev, uint32_t hwpt_id,
> +                                     Error **errp)
>   {
> -    int ret, iommufd = vbasedev->iommufd->fd;
> -    VFIOIOASHwpt *hwpt;
>       struct vfio_device_attach_iommufd_pt attach_data = {
>           .argsz = sizeof(attach_data),
>           .flags = 0,
> -        .pt_id = container->ioas_id,
> +        .pt_id = hwpt_id,
>       };
> +    int ret;
>   
> -    /* Attach device to an ioas within iommufd */
>       ret = ioctl(vbasedev->fd, VFIO_DEVICE_ATTACH_IOMMUFD_PT, &attach_data);
>       if (ret) {
>           error_setg_errno(errp, errno,
> -                         "[iommufd=%d] error attach %s (%d) to ioasid=%d",
> -                         container->be->fd, vbasedev->name, vbasedev->fd,
> -                         attach_data.pt_id);
> +                         "[iommufd=%d] error attach %s (%d) to hwpt_id=%d",
> +                         vbasedev->iommufd->fd, vbasedev->name, vbasedev->fd,
> +                         hwpt_id);
> +    }
> +    return ret;
> +}
> +
> +static int __vfio_device_detach_hwpt(VFIODevice *vbasedev, Error **errp)
> +{
> +    struct vfio_device_detach_iommufd_pt detach_data = {
> +        .argsz = sizeof(detach_data),
> +        .flags = 0,
> +    };
> +    int ret;
> +
> +    ret = ioctl(vbasedev->fd, VFIO_DEVICE_DETACH_IOMMUFD_PT, &detach_data);
> +    if (ret) {
> +        error_setg_errno(errp, errno, "detach %s from ioas failed",
> +                         vbasedev->name);
> +    }
> +    return ret;
> +}
> +
> +static int vfio_device_attach_container(VFIODevice *vbasedev,
> +                                        VFIOIOMMUFDContainer *container,
> +                                        Error **errp)
> +{
> +    int ret, iommufd = vbasedev->iommufd->fd;
> +    VFIOIOASHwpt *hwpt;
> +    uint32_t hwpt_id;
> +    Error *err = NULL;
> +
> +    /* try to attach to an existing hwpt in this container */
> +    QLIST_FOREACH(hwpt, &container->hwpt_list, next) {
> +        ret = __vfio_device_attach_hwpt(vbasedev, hwpt->hwpt_id, &err);
> +        if (ret) {
> +            const char *msg = error_get_pretty(err);
> +
> +            trace_vfio_iommufd_fail_attach_existing_hwpt(msg);
> +            error_free(err);
> +            err = NULL;
> +        } else {
> +            goto found_hwpt;
> +        }
> +    }
> +
> +    ret = iommufd_backend_alloc_hwpt(iommufd, vbasedev->devid,
> +                                     container->ioas_id, &hwpt_id);
> +
> +    if (ret) {
> +        error_setg_errno(errp, errno, "error alloc shadow hwpt");
>           return ret;
> +    }
>   
> +    /* Attach device to an hwpt within iommufd */
> +    ret = __vfio_device_attach_hwpt(vbasedev, hwpt_id, errp);
> +    if (ret) {
> +        iommufd_backend_free_id(iommufd, hwpt_id);
> +        return ret;
>       }
> -    hwpt = vfio_container_get_hwpt(container, attach_data.pt_id);
>   
> +    hwpt = vfio_container_get_hwpt(container, hwpt_id);
> +found_hwpt:
>       QLIST_INSERT_HEAD(&hwpt->device_list, vbasedev, next);
>       vbasedev->hwpt = hwpt;
>   
>       trace_vfio_iommufd_attach_device(iommufd, vbasedev->name, vbasedev->fd,
> -                                     container->ioas_id, attach_data.pt_id);
> +                                     container->ioas_id, hwpt->hwpt_id);
>       return ret;
>   }
>   
> @@ -256,14 +309,12 @@ static void vfio_device_detach_container(VFIODevice *vbasedev,
>                                            VFIOIOMMUFDContainer *container)
>   {
>       VFIOIOASHwpt *hwpt = vbasedev->hwpt;
> -    struct vfio_device_attach_iommufd_pt detach_data = {
> -        .argsz = sizeof(detach_data),
> -        .flags = 0,
> -    };
> +    Error *err = NULL;
> +    int ret;
>   
> -    if (ioctl(vbasedev->fd, VFIO_DEVICE_DETACH_IOMMUFD_PT, &detach_data)) {
> -        error_report("detach %s from ioas id=%d failed: %m", vbasedev->name,
> -                     container->ioas_id);
> +    ret = __vfio_device_detach_hwpt(vbasedev, &err);
> +    if (ret) {
> +        error_report_err(err);
>       }
>   
>       QLIST_REMOVE(vbasedev, next);