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
