Re: [PATCH v5 03/20] vfio/iommufd: Implement the iommufd backend

[Nicolin Chen <nicolinc@nvidia.com>]

On Thu, Nov 09, 2023 at 07:45:12PM +0800, Zhenzhong Duan wrote:

> +static int iommufd_cdev_attach_ioas_hwpt(VFIODevice *vbasedev, bool is_ioas,
> +                                         uint32_t id, Error **errp)
> +{
> +    int ret, iommufd = vbasedev->iommufd->fd;
> +    struct vfio_device_attach_iommufd_pt attach_data = {
> +        .argsz = sizeof(attach_data),
> +        .flags = 0,
> +        .pt_id = id,
> +    };
> +    const char *str = is_ioas ? "ioas" : "hwpt";
> +
> +    /* Attach device to an IOAS or hwpt 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 %s_id=%d",
> +                         iommufd, vbasedev->name, vbasedev->fd, str, id);
> +    } else {
> +        trace_iommufd_cdev_attach_ioas_hwpt(iommufd, vbasedev->name,
> +                                            vbasedev->fd, str, id);
> +    }
> +    return ret;
> +}
> +
> +static int iommufd_cdev_detach_ioas_hwpt(VFIODevice *vbasedev, bool is_ioas,
> +                                         uint32_t id, Error **errp)
> +{
> +    int ret, iommufd = vbasedev->iommufd->fd;
> +    struct vfio_device_detach_iommufd_pt detach_data = {
> +        .argsz = sizeof(detach_data),
> +        .flags = 0,
> +    };
> +    const char *str = is_ioas ? "ioas" : "hwpt";
> +
> +    ret = ioctl(vbasedev->fd, VFIO_DEVICE_DETACH_IOMMUFD_PT, &detach_data);
> +    if (ret) {
> +        error_setg_errno(errp, errno, "detach %s from %s failed",
> +                         vbasedev->name, str);
> +    } else {
> +        trace_iommufd_cdev_detach_ioas_hwpt(iommufd, vbasedev->name, str, id);
> +    }
> +    return ret;
> +}

Being a bit late to the game, I might have missed some review
history here, yet any reason why we changed the attach/detach
APIs to specify is_ioas? The attach kernel uAPI generically
handles this without requiring an is_ioas input, and it could
be interpreted to attaching both ioas and hwpt (auto). On the
hand, the detach uAPI doesn't even care about id. So, I don't
see a value of the is_ioas except the trace logs..

If we have such a hard requirement somewhere, shall we create
an IOMMUFDPtObject structure that holds the type (ioas/hwpt)?

Thanks
Nic