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

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.

Introduce two helpers __vfio_device_attach/detach_hwpt to facilitate
this change.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/iommufd.c | 89 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 70 insertions(+), 19 deletions(-)

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);

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Sent: Monday, October 30, 2023 9:52 PM
>Subject: Re: [PATCH v3 27/37] vfio/iommufd: Switch to manual hwpt allocation
>
>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 ?

Looks unnecessary for me as we prefer manual allocation.
The purpose of splitting is to make review easier.

>
>>
>> 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.

OK, will merge them in v4.

Thanks
Zhenzhong