Re: [PATCH v6 01/10] iommufd/viommu: Add IOMMUFD_OBJ_VDEVICE and IOMMU_VDEVICE_ALLOC ioctl

[Alexey Kardashevskiy <aik@amd.com>]

On 31/10/24 08:35, Nicolin Chen wrote:
> Introduce a new IOMMUFD_OBJ_VDEVICE to represent a physical device (struct
> device) against a vIOMMU (struct iommufd_viommu) object in a VM.
> 
> This vDEVICE object (and its structure) holds all the infos and attributes
> in the VM, regarding the device related to the vIOMMU.
> 
> As an initial patch, add a per-vIOMMU virtual ID. This can be:
>   - Virtual StreamID on a nested ARM SMMUv3, an index to a Stream Table
>   - Virtual DeviceID on a nested AMD IOMMU, an index to a Device Table
>   - Virtual RID on a nested Intel VT-D IOMMU, an index to a Context Table
> Potentially, this vDEVICE structure would hold some vData for Confidential
> Compute Architecture (CCA). Use this virtual ID to index an "vdevs" xarray
> that belongs to a vIOMMU object.
> 
> Add a new ioctl for vDEVICE allocations. Since a vDEVICE is a connection
> of a device object and an iommufd_viommu object, take two refcounts in the
> ioctl handler.
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>   drivers/iommu/iommufd/iommufd_private.h | 18 ++++++
>   include/linux/iommufd.h                 |  4 ++
>   include/uapi/linux/iommufd.h            | 22 +++++++
>   drivers/iommu/iommufd/main.c            |  6 ++
>   drivers/iommu/iommufd/viommu.c          | 76 +++++++++++++++++++++++++
>   5 files changed, 126 insertions(+)
> 
> diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
> index e8f5ef550cc9..062656c19a07 100644
> --- a/drivers/iommu/iommufd/iommufd_private.h
> +++ b/drivers/iommu/iommufd/iommufd_private.h
> @@ -507,8 +507,26 @@ static inline int iommufd_hwpt_replace_device(struct iommufd_device *idev,
>   	return iommu_group_replace_domain(idev->igroup->group, hwpt->domain);
>   }
>   
> +static inline struct iommufd_viommu *
> +iommufd_get_viommu(struct iommufd_ucmd *ucmd, u32 id)
> +{
> +	return container_of(iommufd_get_object(ucmd->ictx, id,
> +					       IOMMUFD_OBJ_VIOMMU),
> +			    struct iommufd_viommu, obj);
> +}
> +
>   int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd);
>   void iommufd_viommu_destroy(struct iommufd_object *obj);
> +int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd);
> +void iommufd_vdevice_destroy(struct iommufd_object *obj);
> +
> +struct iommufd_vdevice {
> +	struct iommufd_object obj;
> +	struct iommufd_ctx *ictx;
> +	struct iommufd_viommu *viommu;
> +	struct device *dev;
> +	u64 id; /* per-vIOMMU virtual ID */
> +};
>   
>   #ifdef CONFIG_IOMMUFD_TEST
>   int iommufd_test(struct iommufd_ucmd *ucmd);
> diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h
> index f03c75410938..ee58c5c573ec 100644
> --- a/include/linux/iommufd.h
> +++ b/include/linux/iommufd.h
> @@ -10,6 +10,7 @@
>   #include <linux/errno.h>
>   #include <linux/refcount.h>
>   #include <linux/types.h>
> +#include <linux/xarray.h>
>   
>   struct device;
>   struct file;
> @@ -31,6 +32,7 @@ enum iommufd_object_type {
>   	IOMMUFD_OBJ_ACCESS,
>   	IOMMUFD_OBJ_FAULT,
>   	IOMMUFD_OBJ_VIOMMU,
> +	IOMMUFD_OBJ_VDEVICE,
>   #ifdef CONFIG_IOMMUFD_TEST
>   	IOMMUFD_OBJ_SELFTEST,
>   #endif
> @@ -89,6 +91,8 @@ struct iommufd_viommu {
>   
>   	const struct iommufd_viommu_ops *ops;
>   
> +	struct xarray vdevs;
> +
>   	unsigned int type;
>   };
>   
> diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
> index a498d4838f9a..9b5236004b8e 100644
> --- a/include/uapi/linux/iommufd.h
> +++ b/include/uapi/linux/iommufd.h
> @@ -53,6 +53,7 @@ enum {
>   	IOMMUFD_CMD_FAULT_QUEUE_ALLOC = 0x8e,
>   	IOMMUFD_CMD_IOAS_MAP_FILE = 0x8f,
>   	IOMMUFD_CMD_VIOMMU_ALLOC = 0x90,
> +	IOMMUFD_CMD_VDEVICE_ALLOC = 0x91,
>   };
>   
>   /**
> @@ -864,4 +865,25 @@ struct iommu_viommu_alloc {
>   	__u32 out_viommu_id;
>   };
>   #define IOMMU_VIOMMU_ALLOC _IO(IOMMUFD_TYPE, IOMMUFD_CMD_VIOMMU_ALLOC)
> +
> +/**
> + * struct iommu_vdevice_alloc - ioctl(IOMMU_VDEVICE_ALLOC)
> + * @size: sizeof(struct iommu_vdevice_alloc)
> + * @viommu_id: vIOMMU ID to associate with the virtual device
> + * @dev_id: The physical device to allocate a virtual instance on the vIOMMU
> + * @out_vdevice_id: Object handle for the vDevice. Pass to IOMMU_DESTORY
> + * @virt_id: Virtual device ID per vIOMMU, e.g. vSID of ARM SMMUv3, vDeviceID
> + *           of AMD IOMMU, and vRID of a nested Intel VT-d to a Context Table


So it is one vdevice per a passed through device (say, a network 
adapter), right? I am asking as there are passed through devices and 
IOMMU devices, and (at least on AMD) IOMMUs look like PCI devices, both 
in hosts and guests. For example, from the above: "@dev_id: The physical 
device ..." - both a network card and IOMMU are physical, so dev_id is a 
NIC or IOMMU? I assume that шы a NIC (but it is a source of constant 
confusion).

Is there any plan to add guest device BDFn as well, or I can add one 
here for my TEE-IO exercise, if it is the right place? It is the same as 
vDeviceID for AMD but I am not sure about the others, hence the 
question. Thanks,


> + *
> + * Allocate a virtual device instance (for a physical device) against a vIOMMU.
> + * This instance holds the device's information (related to its vIOMMU) in a VM.
> + */
> +struct iommu_vdevice_alloc {
> +	__u32 size;
> +	__u32 viommu_id;
> +	__u32 dev_id;
> +	__u32 out_vdevice_id;
> +	__aligned_u64 virt_id;
> +};
> +#define IOMMU_VDEVICE_ALLOC _IO(IOMMUFD_TYPE, IOMMUFD_CMD_VDEVICE_ALLOC)
>   #endif
> diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c
> index cc514f9bc3e6..d735fe04197f 100644
> --- a/drivers/iommu/iommufd/main.c
> +++ b/drivers/iommu/iommufd/main.c
> @@ -308,6 +308,7 @@ union ucmd_buffer {
>   	struct iommu_option option;
>   	struct iommu_vfio_ioas vfio_ioas;
>   	struct iommu_viommu_alloc viommu;
> +	struct iommu_vdevice_alloc vdev;
>   #ifdef CONFIG_IOMMUFD_TEST
>   	struct iommu_test_cmd test;
>   #endif
> @@ -363,6 +364,8 @@ static const struct iommufd_ioctl_op iommufd_ioctl_ops[] = {
>   		 __reserved),
>   	IOCTL_OP(IOMMU_VIOMMU_ALLOC, iommufd_viommu_alloc_ioctl,
>   		 struct iommu_viommu_alloc, out_viommu_id),
> +	IOCTL_OP(IOMMU_VDEVICE_ALLOC, iommufd_vdevice_alloc_ioctl,
> +		 struct iommu_vdevice_alloc, virt_id),
>   #ifdef CONFIG_IOMMUFD_TEST
>   	IOCTL_OP(IOMMU_TEST_CMD, iommufd_test, struct iommu_test_cmd, last),
>   #endif
> @@ -501,6 +504,9 @@ static const struct iommufd_object_ops iommufd_object_ops[] = {
>   	[IOMMUFD_OBJ_VIOMMU] = {
>   		.destroy = iommufd_viommu_destroy,
>   	},
> +	[IOMMUFD_OBJ_VDEVICE] = {
> +		.destroy = iommufd_vdevice_destroy,
> +	},
>   #ifdef CONFIG_IOMMUFD_TEST
>   	[IOMMUFD_OBJ_SELFTEST] = {
>   		.destroy = iommufd_selftest_destroy,
> diff --git a/drivers/iommu/iommufd/viommu.c b/drivers/iommu/iommufd/viommu.c
> index 888239b78667..c82b4a07a4b1 100644
> --- a/drivers/iommu/iommufd/viommu.c
> +++ b/drivers/iommu/iommufd/viommu.c
> @@ -11,6 +11,7 @@ void iommufd_viommu_destroy(struct iommufd_object *obj)
>   	if (viommu->ops && viommu->ops->destroy)
>   		viommu->ops->destroy(viommu);
>   	refcount_dec(&viommu->hwpt->common.obj.users);
> +	xa_destroy(&viommu->vdevs);
>   }
>   
>   int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd)
> @@ -53,6 +54,7 @@ int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd)
>   		goto out_put_hwpt;
>   	}
>   
> +	xa_init(&viommu->vdevs);
>   	viommu->type = cmd->type;
>   	viommu->ictx = ucmd->ictx;
>   	viommu->hwpt = hwpt_paging;
> @@ -79,3 +81,77 @@ int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd)
>   	iommufd_put_object(ucmd->ictx, &idev->obj);
>   	return rc;
>   }
> +
> +void iommufd_vdevice_destroy(struct iommufd_object *obj)
> +{
> +	struct iommufd_vdevice *vdev =
> +		container_of(obj, struct iommufd_vdevice, obj);
> +	struct iommufd_viommu *viommu = vdev->viommu;
> +
> +	/* xa_cmpxchg is okay to fail if alloc returned -EEXIST previously */
> +	xa_cmpxchg(&viommu->vdevs, vdev->id, vdev, NULL, GFP_KERNEL);
> +	refcount_dec(&viommu->obj.users);
> +	put_device(vdev->dev);
> +}
> +
> +int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
> +{
> +	struct iommu_vdevice_alloc *cmd = ucmd->cmd;
> +	struct iommufd_vdevice *vdev, *curr;
> +	struct iommufd_viommu *viommu;
> +	struct iommufd_device *idev;
> +	u64 virt_id = cmd->virt_id;
> +	int rc = 0;
> +
> +	/* virt_id indexes an xarray */
> +	if (virt_id > ULONG_MAX)
> +		return -EINVAL;
> +
> +	viommu = iommufd_get_viommu(ucmd, cmd->viommu_id);
> +	if (IS_ERR(viommu))
> +		return PTR_ERR(viommu);
> +
> +	idev = iommufd_get_device(ucmd, cmd->dev_id);
> +	if (IS_ERR(idev)) {
> +		rc = PTR_ERR(idev);
> +		goto out_put_viommu;
> +	}
> +
> +	if (viommu->iommu_dev != __iommu_get_iommu_dev(idev->dev)) {
> +		rc = -EINVAL;
> +		goto out_put_idev;
> +	}
> +
> +	vdev = iommufd_object_alloc(ucmd->ictx, vdev, IOMMUFD_OBJ_VDEVICE);
> +	if (IS_ERR(vdev)) {
> +		rc = PTR_ERR(vdev);
> +		goto out_put_idev;
> +	}
> +
> +	vdev->id = virt_id;
> +	vdev->dev = idev->dev;
> +	get_device(idev->dev);
> +	vdev->viommu = viommu;
> +	refcount_inc(&viommu->obj.users);
> +
> +	curr = xa_cmpxchg(&viommu->vdevs, virt_id, NULL, vdev, GFP_KERNEL);
> +	if (curr) {
> +		rc = xa_err(curr) ?: -EEXIST;
> +		goto out_abort;
> +	}
> +
> +	cmd->out_vdevice_id = vdev->obj.id;
> +	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
> +	if (rc)
> +		goto out_abort;
> +	iommufd_object_finalize(ucmd->ictx, &vdev->obj);
> +	goto out_put_idev;
> +
> +out_abort:
> +	iommufd_object_abort_and_destroy(ucmd->ictx, &vdev->obj);
> +out_put_idev:
> +	iommufd_put_object(ucmd->ictx, &idev->obj);
> +out_put_viommu:
> +	iommufd_put_object(ucmd->ictx, &viommu->obj);
> +	return rc;
> +}

-- 
Alexey