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

[Mostafa Saleh <smostafa@google.com>]

On Tue, Sep 22, 2026 at 01:45:11PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] iommu/arm-smmu-v3-kvm: dead-branch traps and incorrect WARN_ON usage at EL2
> --
> 
> commit 12e407f77295df6bfe7066853a3a177e9465aaf7
> Author: Mostafa Saleh <smostafa@google.com>
> 
> iommu/arm-smmu-v3-kvm: Invalidate the SMMU TLBs
> 
> This commit implements the io-pgtable flush operations to invalidate the
> shadow stage-2 in every SMMU. It tracks whether the SMMU command queue is
> enabled and issues invalidation commands to the hardware when active.
> 
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3.c
> > index 6f0ea3a4e48df..5417a5c2bf58a 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3.c
> 
> [ ... ]
> 
> > @@ -221,17 +220,139 @@ static int smmu_send_cmd(struct hyp_arm_smmu_v3_device *smmu,
> >  	return smmu_sync_cmd(smmu);
> >  }
> >  
> > +static int smmu_tlb_inv_vmid(struct hyp_arm_smmu_v3_device *smmu)
> 
> [ ... ]
> 
> > +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: Medium]
> Does this WARN_ON() panic the hypervisor if the hardware command queue
> stalls?
> 
> At EL2, WARN_ON() expands to a fatal BRK instruction. If the SMMU command
> queue is full or stalls for >1ms, smmu_wait() returns -ETIMEDOUT, which
> will bubble up here and cause an unrecoverable hypervisor panic.

There is no way to recover a command queue timeout, ignoring the error
might lead to TLB not being invalidated and leaking the memory content
so WARN_ON() is suitable for that.

> 
> > +		hyp_spin_unlock(&smmu->hw_lock);
> > +	}
> > +}
> 
> [ ... ]
> 
> > @@ -725,6 +873,8 @@ static void smmu_emulate_enable(struct hyp_arm_smmu_v3_device *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: Medium]
> This isn't a bug introduced by this patch, but looking at the existing code
> just above this in smmu_emulate_enable():
> 
> 	if (WARN_ON(!is_cmdq_enabled(smmu)))
> 		return;
> 
> Does this create a dead-branch trap? Because WARN_ON() is fatal at EL2, the
> intended graceful return on an invalid host state is unreachable, as the
> warning itself panics the hypervisor.
> 

Same as discussed before.

> [ ... ]
> 
> > @@ -745,6 +895,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: Medium]
> Does this allow the host to trivially panic the hypervisor by issuing an
> invalid sequence?
> 
> Since WARN_ON() expands to a panic at EL2, asserting directly on
> host-controlled state (disabling the CMDQ while the SMMU is enabled)
> turns a host misconfiguration into a hypervisor panic, bringing down the
> host and all protected guests.

Yes, the host can panic the system in many ways.

> 
> > +
> >  	WARN_ON(smmu_unshare_pages(smmu->cmdq_host.base_dma,
> >  				   cmdq_size(&smmu->cmdq_host)));
> >  }
> 
> [ ... ]
> 
> > @@ -1020,6 +1177,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]
> Could this strict equality check predictably time out and panic the
> hypervisor if the host writes reserved bits?
> 
> If the host writes RES0 bits to CR0, the hardware drops them, making the
> strict readl_relaxed(...) == cr0 check fail. The poll will then time out,
> and the WARN_ON() will fatally panic the hypervisor.

See above.

Thanks,
Mostafa

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