Re: [PATCH v9 15/25] iommu/arm-smmu-v3-kvm: Add CMDQ functions

[Mostafa Saleh <smostafa@google.com>]

On Thu, Oct 15, 2026 at 08:30:05PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] iommu/arm-smmu-v3-kvm: swapped WFE logic causes queue polling delays
> --
> 
> commit 533e5a914cd22f171b26c2c5c0e2acfb11756ea5
> Author: Mostafa Saleh <smostafa@google.com>
> 
> iommu/arm-smmu-v3-kvm: Add CMDQ functions
> 
> This commit adds command queue accessors for the ARM SMMUv3 in the
> hypervisor. It includes command queuing and synchronization functions,
> with a 1 ms polling timeout that sleeps in WFE when SEV is supported.
> 
> > 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
> > index 02e7c61758329..c9fc21e405e79 100644
> > --- 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
> [ ... ]
> > @@ -77,6 +113,85 @@ static int smmu_unshare_pages(phys_addr_t addr, size_t size)
> [ ... ]
> > +static int smmu_add_cmd(struct hyp_arm_smmu_v3_device *smmu,
> > +			struct arm_smmu_cmd *cmd)
> > +{
> > +	int ret;
> > +
> > +	hyp_assert_lock_held(&smmu->hw_lock);
> > +	ret = smmu_wait(smmu->features & ARM_SMMU_FEAT_SEV,
> > +			!smmu_cmdq_full(&smmu->cmdq));
> 
> [Severity: High]
> Does this cause the CPU to stall until a timer event?
> 
> When the hypervisor inserts a batch of commands that exceeds the command
> queue size, smmu_add_cmd() waits for queue space and passes
> smmu->features & ARM_SMMU_FEAT_SEV as the __use_wfe argument to
> smmu_wait(), executing wfe().
> 
> However, the SMMU only generates an SEV on a CMD_SYNC with CS_SEV. Since
> hw_lock prevents concurrent insertions and the hypervisor only inserts
> CMD_SYNC at the end of the batch, no SEVs are generated while the queue
> drains. The CPU sleeps in wfe() and relies solely on the timer event
> stream to wake up, introducing massive artificial delays. Stalling EL2
> with interrupts masked for milliseconds degrades system stability and
> risks triggering timeouts.
> 

Not true, the SMMU generates an event when queue become non-full check:
arm_smmu_cmdq_poll_until_not_full()

Also in the spec: 4.1.2
    When SMMU_IDR0.SEV == 1, the SMMU triggers a WFE wake-up event
    when a Command queue becomes non-full and an agent external to the SMMU
    could have observed that the queue was previously full.

> > +	if (ret)
> > +		return ret;
> > +
> > +	smmu_add_cmd_raw(smmu, cmd->data);
> > +	writel(smmu->cmdq.llq.prod, smmu->cmdq.prod_reg);
> > +	return 0;
> > +}
> > +
> > +static int smmu_sync_cmd(struct hyp_arm_smmu_v3_device *smmu)
> > +{
> > +	int ret;
> > +	struct arm_smmu_cmd cmd;
> > +
> > +	hyp_assert_lock_held(&smmu->hw_lock);
> > +	cmd = arm_smmu_make_cmd_sync(CMDQ_SYNC_0_CS_SEV, 0);
> > +	ret = smmu_add_cmd(smmu, &cmd);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return smmu_wait(false, smmu_cmdq_empty(&smmu->cmdq));
> 
> [Severity: High]
> Is the WFE logic swapped between smmu_add_cmd() and smmu_sync_cmd()?
> 
> Here, smmu_sync_cmd() inserts a CMD_SYNC with CS_SEV, which will
> explicitly generate an SEV upon completion. Yet it passes false for
> __use_wfe to smmu_wait(), forcing a busy-wait (cpu_relax()) and ignoring
> the precise hardware SEV wakeup it requested.

I guess the sync command can leverage the WFE also, as I was focused
on the empty condition when I wrote it, but that is just an
optimization.

Thanks,
Mostafa

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