Re: [PATCH v9 23/25] iommu/arm-smmu-v3-kvm: Invalidate the SMMU TLBs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] arm-smmu-v3: incorrect WARN_ON usage at EL2 for hardware timeouts and host inputs
- [Medium] arm-smmu-v3: guaranteed timeout and hyp panic if host writes reserved bits to CR0
--

commit a876edb8ca1cb558cf1336f7a093e9a259c78f18
Author: Mostafa Saleh <smostafa@google.com>

iommu/arm-smmu-v3-kvm: Invalidate the SMMU TLBs

This commit implements the io-pgtable TLB operations to invalidate the shadow
stage-2 entries across all SMMUs in pKVM. It manages invalidation commands
and ensures the command queue is active before flushing TLBs.

> diff --git a/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3-hyp.c b/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3-hyp.c
> --- a/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3-hyp.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3-hyp.c

[ ... ]

> +static void smmu_tlb_inv_range(unsigned long iova, size_t size, size_t granule,
> +			       bool leaf)
> +{
> +	struct hyp_arm_smmu_v3_device *smmu;
> +
> +	for_each_smmu(smmu) {
> +		hyp_spin_lock(&smmu->hw_lock);
> +		/*
> +		 * Don't bother if CMDQ is disabled, this would be useful for the case
> +		 * when RPM is supported to avoid touching the SMMU MMIO when disabled.
> +		 * The hypervisor also asserts CMDQEN is enabled before the SMMU is
> +		 * enabled. As otherwise the host can prevent the hypervisor from doing
> +		 * TLB invalidations.
> +		 * When the SMMU is re-enabled the hypervisor cleans the TLBs.
> +		 */
> +		if (smmu->cmdq_active)
> +			WARN_ON(smmu_tlb_inv_range_smmu(smmu, iova, size,
> +							granule, leaf));

[Severity: High]
Under pKVM at EL2, WARN_ON() expands to a fatal BUG() instruction which
crashes the hypervisor.

Could this WARN_ON() trigger a panic if the hardware SMMU is slow to drain
its command queue? The call chain goes through smmu_tlb_inv_range_smmu(),
smmu_send_cmd(), smmu_sync_cmd(), and ultimately smmu_wait(), which returns
-ETIMEDOUT if the hardware polls for too long. Should this gracefully handle
the error or ignore it instead of asserting?

> +		hyp_spin_unlock(&smmu->hw_lock);
> +	}
> +}

[ ... ]

> @@ -721,6 +821,33 @@ static int smmu_update_ste_shadow(struct hyp_arm_smmu_v3_device *smmu, bool enab
>  static void smmu_emulate_enable(struct hyp_arm_smmu_v3_device *smmu)
>  {
>  	/* Enabling SMMU without CMDQ, means TLB invalidation won't work. */
>  	if (WARN_ON(!is_cmdq_enabled(smmu)))
>  		return;
>  
>  	WARN_ON(smmu_update_ste_shadow(smmu, true));
> +	/* Clean the TLBs each time the SMMU is enabled. */
> +	WARN_ON(smmu_flush_all_tlb(smmu));

[Severity: High]
Does this code panic the hypervisor if the SMMU hardware times out during
the TLB flush on enablement? Similar to the above, smmu_flush_all_tlb() can
return -ETIMEDOUT from smmu_wait() if the hardware is unresponsive.

>  }

[ ... ]

> @@ -748,6 +877,13 @@ static void smmu_emulate_cmdq_enable(struct hyp_arm_smmu_v3_device *smmu)
>  static void smmu_emulate_cmdq_disable(struct hyp_arm_smmu_v3_device *smmu)
>  {
> +	/*
> +	 * We can not enable the SMMU if the CMDQ is disabled and similarly
> +	 * we can not disable the CMDQ if the SMMU is enabled, as that can
> +	 * lead to stale TLBs.
> +	 */
> +	WARN_ON(is_smmu_enabled(smmu));

[Severity: High]
Is it safe to panic the hypervisor here based on host-supplied configuration?
If the host kernel supplies a CR0 value that disables the CMDQ while the
SMMU is still enabled, this will trigger the EL2 WARN_ON() and crash the
entire host.

> +
>  	WARN_ON(smmu_unshare_pages(smmu->cmdq_host.base_dma,
>  				   cmdq_size(&smmu->cmdq_host)));
>  }

[ ... ]

> @@ -1033,6 +1169,20 @@ static bool smmu_dabt_device(struct hyp_arm_smmu_v3_device *smmu,
>  		else
>  			writel_relaxed(val & mask, smmu->base + off);
>  
> +		/*
> +		 * Make sure writes to CR0 are immediately observed, that is important
> +		 * when synchronizing with TLB invalidation as reading CR0 is not enough
> +		 * to deduce the SMMU state, and we have to enforce the ack with the
> +		 * hw_lock acquired.
> +		 */
> +		if (off == ARM_SMMU_CR0) {
> +			u32 cr0 = val;
> +
> +			WARN_ON(smmu_wait(false,
> +					  readl_relaxed(smmu->base + ARM_SMMU_CR0ACK) == cr0));

[Severity: Medium]
Can this sequence cause a guaranteed timeout and hypervisor panic if the host
writes reserved bits to CR0?

The variable cr0 holds the raw, unmasked host input val. When the hypervisor
writes to the hardware CR0 register, the hardware will update CR0ACK to
reflect the accepted state, ignoring any reserved bits.

If the host writes a value with any bit set that the hardware ignores,
readl_relaxed(smmu->base + ARM_SMMU_CR0ACK) == cr0 will never evaluate
to true. This would cause smmu_wait() to poll until it hits its timeout and
return -ETIMEDOUT, triggering the WARN_ON() panic.

> +			smmu->cmdq_active = !!(cr0 & CR0_CMDQEN);
> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015200903.2735565-1-smostafa@google.com?part=23