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

ALOK TIWARI
2025-04-29T19:49:32Z

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