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

[sashiko-bot@kernel.org]

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.

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

> +}

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