On Fri, Apr 25, 2025 at 10:58:15PM -0700, Nicolin Chen wrote:
> To simplify the mappings from global VCMDQs to VINTFs' LVCMDQs, the design
> chose to do static allocations and mappings in the global reset function.
>
> However, with the user-owned VINTF support, it exposes a security concern:
> if user space VM only wants one LVCMDQ for a VINTF, statically mapping two
> LVCMDQs creates a hidden VCMDQ that user space could DoS attack by writing
> ramdon stuff to overwhelm the kernel with unhandleable IRQs.
>
Nit: I think it's worth mentioning that the current HW only supports 2
LVCMDQs. Since it's not clear from the driver as it calculates this by:
regval = readl_relaxed(REG_CMDQV(cmdqv, PARAM));
cmdqv->num_vintfs = 1 << FIELD_GET(CMDQV_NUM_VINTF_LOG2,regval);
cmdqv->num_vcmdqs = 1 << FIELD_GET(CMDQV_NUM_VCMDQ_LOG2, regval);
cmdqv->num_lvcmdqs_per_vintf = cmdqv->num_vcmdqs / cmdqv->num_vintfs;
Or maybe, re-word it to "if user space VM only wants one LVCMDQ for a
VINTF, the current driver statically maps num_lvcmdqs_per_vintf which
creates hidden vCMDQs [..]"
> Thus, to support the user-owned VINTF feature, a LVCMDQ mapping has to be
> done dynamically.
>
> HW allows pre-assigning global VCMDQs in the CMDQ_ALLOC registers, without
> finalizing the mappings by keeping CMDQV_CMDQ_ALLOCATED=0. So, add a pair
> of map/unmap helper that simply sets/clears that bit.
>
> Delay the LVCMDQ mappings to tegra241_vintf_hw_init(), and the unmappings
> to tegra241_vintf_hw_deinit().
>
> However, the dynamic LVCMDQ mapping/unmapping can complicate the timing of
> calling tegra241_vcmdq_hw_init/deinit(), which write LVCMDQ address space,
> i.e. requiring LVCMDQ to be mapped. Highlight that with a note to the top
> of either of them.
>
> Signed-off-by: Nicolin Chen <nicol...@nvidia.com>
> ---
> .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 37 +++++++++++++++++--
> 1 file changed, 33 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> index 8d418c131b1b..869c90b660c1 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> @@ -351,6 +351,7 @@ tegra241_cmdqv_get_cmdq(struct arm_smmu_device *smmu,
>
> /* HW Reset Functions */
>
> +/* This function is for LVCMDQ, so @vcmdq must not be unmapped yet */
> static void tegra241_vcmdq_hw_deinit(struct tegra241_vcmdq *vcmdq)
> {
> char header[64], *h = lvcmdq_error_header(vcmdq, header, 64);
> @@ -379,6 +380,7 @@ static void tegra241_vcmdq_hw_deinit(struct
> tegra241_vcmdq *vcmdq)
> dev_dbg(vcmdq->cmdqv->dev, "%sdeinited\n", h);
> }
>
> +/* This function is for LVCMDQ, so @vcmdq must be mapped prior */
> static int tegra241_vcmdq_hw_init(struct tegra241_vcmdq *vcmdq)
> {
> char header[64], *h = lvcmdq_error_header(vcmdq, header, 64);
> @@ -404,16 +406,42 @@ static int tegra241_vcmdq_hw_init(struct tegra241_vcmdq
> *vcmdq)
> return 0;
> }
>
> +/* Unmap a global VCMDQ from the pre-assigned LVCMDQ */
> +static void tegra241_vcmdq_unmap_lvcmdq(struct tegra241_vcmdq *vcmdq)
> +{
> + u32 regval = readl(REG_CMDQV(vcmdq->cmdqv, CMDQ_ALLOC(vcmdq->idx)));
> + char header[64], *h = lvcmdq_error_header(vcmdq, header, 64);
> +
> + writel(regval & ~CMDQV_CMDQ_ALLOCATED,
> + REG_CMDQV(vcmdq->cmdqv, CMDQ_ALLOC(vcmdq->idx)));
> + dev_dbg(vcmdq->cmdqv->dev, "%sunmapped\n", h);
> +}
> +
> static void tegra241_vintf_hw_deinit(struct tegra241_vintf *vintf)
> {
> - u16 lidx;
> + u16 lidx = vintf->cmdqv->num_lvcmdqs_per_vintf;
>
> - for (lidx = 0; lidx < vintf->cmdqv->num_lvcmdqs_per_vintf; lidx++)
> - if (vintf->lvcmdqs && vintf->lvcmdqs[lidx])
> + /* HW requires to unmap LVCMDQs in descending order */
> + while (lidx--) {
> + if (vintf->lvcmdqs && vintf->lvcmdqs[lidx]) {
> tegra241_vcmdq_hw_deinit(vintf->lvcmdqs[lidx]);
> + tegra241_vcmdq_unmap_lvcmdq(vintf->lvcmdqs[lidx]);
> + }
> + }
> vintf_write_config(vintf, 0);
> }
>
> +/* Map a global VCMDQ to the pre-assigned LVCMDQ */
> +static void tegra241_vcmdq_map_lvcmdq(struct tegra241_vcmdq *vcmdq)
> +{
> + u32 regval = readl(REG_CMDQV(vcmdq->cmdqv, CMDQ_ALLOC(vcmdq->idx)));
> + char header[64], *h = lvcmdq_error_header(vcmdq, header, 64);
> +
> + writel(regval | CMDQV_CMDQ_ALLOCATED,
> + REG_CMDQV(vcmdq->cmdqv, CMDQ_ALLOC(vcmdq->idx)));
> + dev_dbg(vcmdq->cmdqv->dev, "%smapped\n", h);
> +}
> +
> static int tegra241_vintf_hw_init(struct tegra241_vintf *vintf, bool hyp_own)
> {
> u32 regval;
> @@ -441,8 +469,10 @@ static int tegra241_vintf_hw_init(struct tegra241_vintf
> *vintf, bool hyp_own)
> */
> vintf->hyp_own = !!(VINTF_HYP_OWN & readl(REG_VINTF(vintf, CONFIG)));
>
> + /* HW requires to map LVCMDQs in ascending order */
> for (lidx = 0; lidx < vintf->cmdqv->num_lvcmdqs_per_vintf; lidx++) {
> if (vintf->lvcmdqs && vintf->lvcmdqs[lidx]) {
> + tegra241_vcmdq_map_lvcmdq(vintf->lvcmdqs[lidx]);
> ret = tegra241_vcmdq_hw_init(vintf->lvcmdqs[lidx]);
> if (ret) {
> tegra241_vintf_hw_deinit(vintf);
> @@ -476,7 +506,6 @@ static int tegra241_cmdqv_hw_reset(struct arm_smmu_device
> *smmu)
> for (lidx = 0; lidx < cmdqv->num_lvcmdqs_per_vintf; lidx++) {
> regval = FIELD_PREP(CMDQV_CMDQ_ALLOC_VINTF, idx);
> regval |= FIELD_PREP(CMDQV_CMDQ_ALLOC_LVCMDQ, lidx);
> - regval |= CMDQV_CMDQ_ALLOCATED;
> writel_relaxed(regval,
> REG_CMDQV(cmdqv, CMDQ_ALLOC(qidx++)));
> }
I can't confirm HW behaviour but the changes make sense to me.
Acked-by: Pranjal Shrivastava <pr...@google.com>
Thanks!
> --
> 2.43.0
>
