Re: [PATCH v4 01/14] iommufd/viommu: Introduce IOMMUFD_OBJ_VDEVICE and its related struct

[Alexey Kardashevskiy <aik@amd.com>]

On 22/10/24 11:20, Nicolin Chen wrote:
> Introduce a new IOMMUFD_OBJ_VDEVICE to represent a physical device, i.e.
> iommufd_device (idev) object, against an iommufd_viommu (vIOMMU) object in
> the VM. This vDEVICE object (and its structure) holds all the information
> and attributes in a 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 ID on a nested Intel VT-D IOMMU, an index to a Context Table
> Potentially, this vDEVICE structure can hold some vData for Confidential
> Compute Architecture (CCA).
> 
> Add a pair of vdevice_alloc and vdevice_free in struct iommufd_viommu_ops
> to allow driver-level vDEVICE structure allocations.
> 
> Similar to iommufd_viommu_alloc, add an iommufd_vdevice_alloc helper, so
> IOMMU drivers can allocate core-embedded style structures.
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>   include/linux/iommufd.h | 32 ++++++++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
> 
> diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h
> index 5c13c35952d8..5d61a1d2947a 100644
> --- a/include/linux/iommufd.h
> +++ b/include/linux/iommufd.h
> @@ -31,6 +31,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
> @@ -92,6 +93,14 @@ struct iommufd_viommu {
>   	unsigned int type;
>   };
>   
> +struct iommufd_vdevice {
> +	struct iommufd_object obj;
> +	struct iommufd_ctx *ictx;
> +	struct iommufd_device *idev;
> +	struct iommufd_viommu *viommu;
> +	u64 id; /* per-vIOMMU virtual ID */
> +};
> +
>   /**
>    * struct iommufd_viommu_ops - vIOMMU specific operations
>    * @free: Free all driver-specific parts of an iommufd_viommu. The memory of the
> @@ -101,12 +110,24 @@ struct iommufd_viommu {
>    *                       must be defined in include/uapi/linux/iommufd.h.
>    *                       It must fully initialize the new iommu_domain before
>    *                       returning. Upon failure, ERR_PTR must be returned.
> + * @vdevice_alloc: Allocate a driver-managed iommufd_vdevice to init some driver
> + *                 specific structure or HW procedure. Note that the core-level
> + *                 structure is filled by the iommufd core after calling this op.
> + *                 It is suggested to call iommufd_vdevice_alloc() helper for
> + *                 a bundled allocation of the core and the driver structures,
> + *                 using the ictx pointer in the given @viommu.
> + * @vdevice_free: Free a driver-managed iommufd_vdevice to de-init its structure
> + *                or HW procedure. The memory of the vdevice will be free-ed by
> + *                iommufd core.
>    */
>   struct iommufd_viommu_ops {
>   	void (*free)(struct iommufd_viommu *viommu);
>   	struct iommu_domain *(*domain_alloc_nested)(
>   		struct iommufd_viommu *viommu,
>   		const struct iommu_user_data *user_data);
> +	struct iommufd_vdevice *(*vdevice_alloc)(struct iommufd_viommu *viommu,
> +						 struct device *dev, u64 id);
> +	void (*vdevice_free)(struct iommufd_vdevice *vdev);
>   };
>   
>   #if IS_ENABLED(CONFIG_IOMMUFD)
> @@ -200,4 +221,15 @@ _iommufd_object_alloc(struct iommufd_ctx *ictx, size_t size,
>   			ret->member.ops = viommu_ops;                          \
>   		ret;                                                           \
>   	})
> +#define iommufd_vdevice_alloc(ictx, drv_struct, member)                        \
> +	({                                                                     \
> +		static_assert(                                                 \
> +			__same_type(struct iommufd_vdevice,                    \
> +				    ((struct drv_struct *)NULL)->member));     \
> +		static_assert(offsetof(struct drv_struct, member.obj) == 0);   \
> +		container_of(_iommufd_object_alloc(ictx,                       \
> +						   sizeof(struct drv_struct),  \
> +						   IOMMUFD_OBJ_VDEVICE),       \
> +			     struct drv_struct, member.obj);                   \
> +	})
>   #endif

A nit: it hurts eyes to read:

mock_vdev = iommufd_vdevice_alloc(viommu->ictx, mock_vdevice, core);

vs.

mock_vdev = iommufd_vdevice_alloc(viommu->ictx, struct mock_vdevice, core);

as for the former I go searching for a "mock_vdevice" variable and for 
the latter it is clear it is 1) a macro 2) which does some type checking.

also, it makes it impossible to pass things like typeof(..) or a type 
from typedef. Thanks,


-- 
Alexey