On Fri, Apr 12, 2024 at 08:47:05PM -0700, Nicolin Chen wrote:
> Introduce a new ioctl to set a per-viommu device virtual id that should be
> linked to the physical device id (or just a struct device pointer).
>
> Since a viommu (user space IOMMU instance) can have multiple devices while
> it's not ideal to confine a device to one single user space IOMMU instance
> either, these two shouldn't just do a 1:1 mapping. Add two xarrays in their
> structures to bind them bidirectionally.
Since I would like to retain the dev_id, I think this is probably
better done with an allocated struct per dev-id:
struct dev_id {
struct iommufd_device *idev;
struct iommufd_viommu *viommu;
u64 vdev_id;
u64 driver_private; // Ie the driver can store the pSID here
struct list_head idev_entry;
};
> @@ -135,7 +135,16 @@ void iommufd_device_destroy(struct iommufd_object *obj)
> {
> struct iommufd_device *idev =
> container_of(obj, struct iommufd_device, obj);
> + struct iommufd_viommu *viommu;
> + unsigned long index;
>
> + xa_for_each(&idev->viommus, index, viommu) {
> + if (viommu->ops->unset_dev_id)
> + viommu->ops->unset_dev_id(viommu, idev->dev);
> + xa_erase(&viommu->idevs, idev->obj.id);
> + xa_erase(&idev->viommus, index);
> + }
Then this turns into list_for_each(idev->viommu_vdevid_list)
> +int iommufd_viommu_set_device_id(struct iommufd_ucmd *ucmd)
> +{
> + struct iommu_viommu_set_dev_id *cmd = ucmd->cmd;
> + unsigned int dev_id, viommu_id;
> + struct iommufd_viommu *viommu;
> + struct iommufd_device *idev;
> + int rc;
> +
> + idev = iommufd_get_device(ucmd, cmd->dev_id);
> + if (IS_ERR(idev))
> + return PTR_ERR(idev);
> + dev_id = idev->obj.id;
> +
> + viommu = iommufd_get_viommu(ucmd, cmd->viommu_id);
> + if (IS_ERR(viommu)) {
> + rc = PTR_ERR(viommu);
> + goto out_put_idev;
> + }
> +
> + if (viommu->iommu_dev != idev->dev->iommu->iommu_dev) {
> + rc = -EINVAL;
> + goto out_put_viommu;
> + }
> +
> + if (!viommu->ops->set_dev_id || !viommu->ops->unset_dev_id) {
> + rc = -EOPNOTSUPP;
> + goto out_put_viommu;
> + }
> +
> + rc = xa_alloc(&idev->viommus, &viommu_id, viommu,
> + XA_LIMIT(viommu->obj.id, viommu->obj.id),
> + GFP_KERNEL_ACCOUNT);
> + if (rc)
> + goto out_put_viommu;
> +
> + rc = xa_alloc(&viommu->idevs, &dev_id, idev,
> + XA_LIMIT(dev_id, dev_id), GFP_KERNEL_ACCOUNT);
> + if (rc)
> + goto out_xa_erase_viommu;
Both of these are API mis-uses, you don't want an allocating xarray
you just want to use xa_cmpxchg
Put an xarray in the viommu object and fill it with pointers to the
struct dev_id thing above
The driver can piggyback on this xarray too if it wants, so we only
need one.
xa_cmpchg to install the user requests vdev_id only if the vdev_id is
already unused.
> @@ -51,6 +51,7 @@ enum {
> IOMMUFD_CMD_HWPT_GET_DIRTY_BITMAP,
> IOMMUFD_CMD_HWPT_INVALIDATE,
> IOMMUFD_CMD_VIOMMU_ALLOC,
> + IOMMUFD_CMD_VIOMMU_SET_DEV_ID,
> };
We almost certainly will need a REMOVE_DEV_ID so userspace can have
sensible error handling.
> +
> +/**
> + * struct iommu_viommu_set_dev_id - ioctl(IOMMU_VIOMMU_SET_DEV_ID)
> + * @size: sizeof(struct iommu_viommu_set_dev_id)
> + * @viommu_id: viommu ID to associate with the device to store its virtual ID
> + * @dev_id: device ID to set a device virtual ID
> + * @__reserved: Must be 0
> + * @id: Device virtual ID
> + *
> + * Set a viommu-specific virtual ID of a device
> + */
> +struct iommu_viommu_set_dev_id {
> + __u32 size;
> + __u32 viommu_id;
> + __u32 dev_id;
> + __u32 __reserved;
> + __aligned_u64 id;
I think I'd consistently call this vdev_id throughout the API
Jason
