Re: [PATCH v2 21/22] iommu/tegra241-cmdqv: Add user-space use support

[ALOK TIWARI <alok.a.tiwari@oracle.com>]

Hi Nicolin,


On 26-04-2025 11:28, Nicolin Chen wrote:
> The CMDQV HW supports a user-space use for virtualization cases. It allows
> the VM to issue guest-level TLBI or ATC_INV commands directly to the queue
> and executes them without a VMEXIT, as HW will replace the VMID field in a
> TLBI command and the SID field in an ATC_INV command with the preset VMID
> and SID.
> 
[clip]
> +
> +/**
> + * struct iommu_viommu_tegra241_cmdqv - NVIDIA Tegra241 CMDQV Virtual Interface
> + *                                      (IOMMU_VIOMMU_TYPE_TEGRA241_CMDQV)
> + * @out_vintf_page0_pgoff: Offset of the VINTF page0 for mmap syscall
> + * @out_vintf_page0_pgsz: Size of the VINTF page0 for mmap syscall
> + *
> + * Both @out_vintf_page0_pgoff and @out_vintf_page0_pgsz are given by the kernel
> + * for user space to mmap the VINTF page0 from the host physical address space
> + * to the guest physical address space so that a guest kernel can directly R/W
> + * access to the VINTF page0 in order to control its virtual comamnd queues.

typo comamnd

> + */
> +struct iommu_viommu_tegra241_cmdqv {
> +	__aligned_u64 out_vintf_page0_pgoff;
> +	__aligned_u64 out_vintf_page0_pgsz;
>   };
>   
>   /**
> @@ -1152,9 +1183,23 @@ struct iommu_veventq_alloc {
>   /**
>    * enum iommu_vcmdq_type - Virtual Command Queue Type
>    * @IOMMU_VCMDQ_TYPE_DEFAULT: Reserved for future use
> + * @IOMMU_VCMDQ_TYPE_TEGRA241_CMDQV: NVIDIA Tegra241 CMDQV Extension for SMMUv3
>    */
>   enum iommu_vcmdq_type {
>   	IOMMU_VCMDQ_TYPE_DEFAULT = 0,
> +	/*
> +	 * TEGRA241_CMDQV requirements (otherwise it will fail)
> +	 * - alloc starts from the lowest @index=0 in ascending order
> +	 * - destroy starts from the last allocated @index in descending order
> +	 * - @addr must be aligned to @length in bytes and be mmapped in IOAS
> +	 * - @length must be a power of 2, with a minimum 32 bytes and a maximum
> +	 *   1 ^ idr[1].CMDQS x 16 bytes (do GET_HW_INFO call to read idr[1] in

This line is ambiguous to me
intended to express a power of 2.
1 ^ idr[1].CMDQS x 16 bytes -> (2 ^ idr[1].CMDQS) x 16 bytes ?

You could consider like this
(2 ^ idr[1].CMDQS) * 16 bytes (use GET_HW_INFO call to read idr[1] from
struct iommu_hw_info_arm_smmuv3)
or more clear (2 to the power of idr[1].CMDQS)

> +	 *   struct iommu_hw_info_arm_smmuv3)
> +	 * - suggest to back the queue memory with contiguous physical pages or
> +	 *   a single huge page with alignment of the queue size, limit vSMMU's
> +	 *   IDR1.CMDQS to the huge page size divided by 16 bytes
> +	 */
> +	IOMMU_VCMDQ_TYPE_TEGRA241_CMDQV = 1,
>   };
>   
>   /**
[clip]
>   	struct arm_smmu_device *smmu = vsmmu->smmu;
> diff --git a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> index 869c90b660c1..88e2b6506b3a 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> @@ -8,7 +8,9 @@
>   #include <linux/dma-mapping.h>
>   #include <linux/interrupt.h>
>   #include <linux/iommu.h>
> +#include <linux/iommufd.h>
>   #include <linux/iopoll.h>
> +#include <uapi/linux/iommufd.h>
>   
>   #include <acpi/acpixf.h>
>   
[clip]
> +
> +/**
> + * struct tegra241_vintf_sid - Virtual Interface Stream ID Replacement
> + * @core: Embedded iommufd_vdevice structure, holding virtual Stream ID
> + * @vintf: Parent VINTF pointer
> + * @sid: Physical Stream ID
> + * @id: Slot index in the VINTF

@idx

> + */
> +struct tegra241_vintf_sid {
> +	struct iommufd_vdevice core;
> +	struct tegra241_vintf *vintf;
> +	u32 sid;
> +	u8 idx;
>   };
[clip]
> +	/*
> +	 * HW requires to map LVCMDQs in ascending order, so reject if the
> +	 * previous lvcmdqs is not allocated yet.
> +	 */
> +	if (index) {
> +		prev = vintf->lvcmdqs[index - 1];
> +		if (!prev)
> +			return ERR_PTR(-EIO);
> +	}
> +	/*
> +	 * @length must be a power of 2, in range of
> +	 *   [ 32, 1 ^ (idr[1].CMDQS + CMDQ_ENT_SZ_SHIFT) ]

2 ^ (idr[1].CMDQS + CMDQ_ENT_SZ_SHIFT) or 1 << idr[1].CMDQS

> +	 */
> +	max_n_shift = FIELD_GET(IDR1_CMDQS,
> +				readl_relaxed(smmu->base + ARM_SMMU_IDR1));


LGTM, aside from a minor cosmetic thing.

Thanks,
Alok