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

[Duan, Zhenzhong]

>-----Original Message-----
>From: Nicolin Chen <nicolinc@nvidia.com>
>Sent: Sunday, November 12, 2023 1:47 AM
>Subject: Re: [PATCH v5 03/20] vfio/iommufd: Implement the iommufd backend
>
>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..

You are right, only for trace logs and error logs to be more accurate.

This also takes nesting into consideration, the same API will be called
by both QEMU cdev and vIOMMU. For cdev, is_ioas is true, for vIOMMU
which creates hwpt, is_ioas is false.

Let me know if you'd like to remove is_ioas totally or only for detach,
I'm fine with any.

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

It's not a hard requirement. I'm not sure how IOMMUFDPtObject
can help on this, especially for vIOMMU hwpt attaching.

Thanks
Zhenzhong