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

[Mostafa Saleh <smostafa@google.com>]

On Thu, Oct 15, 2026 at 08:28:22PM +0000, sashiko-bot@kernel.org wrote:
> 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.
> 

No way to recover from -ETIMEDOUT as discussed before, at this HW is
propably in a really bad state.

> >  }
> 
> [ ... ]
> 
> > @@ -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.
> 

The kernel can panic itself in many other ways.

> > +
> >  	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.
> 


Same as above.

Thanks,
Mostafa

> > +			smmu->cmdq_active = !!(cr0 & CR0_CMDQEN);
> > +		}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261015200903.2735565-1-smostafa@google.com?part=23