Re: [PATCH v4 22/28] iommu/arm-smmu-v3-kvm: Emulate CMDQ for host

[Mostafa Saleh <smostafa@google.com>]

On Fri, Sep 12, 2025 at 03:18:08PM +0100, Will Deacon wrote:
> On Tue, Aug 19, 2025 at 09:51:50PM +0000, Mostafa Saleh wrote:
> > Don’t allow access to the command queue from the host:
> > - ARM_SMMU_CMDQ_BASE: Only allowed to be written when CMDQ is disabled, we
> >   use it to keep track of the host command queue base.
> >   Reads return the saved value.
> > - ARM_SMMU_CMDQ_PROD: Writes trigger command queue emulation which sanitises
> >   and filters the whole range. Reads returns the host copy.
> > - ARM_SMMU_CMDQ_CONS: Writes move the sw copy of the cons, but the host can’t
> >   skip commands once submitted. Reads return the emulated value and the error
> >   bits in the actual cons.
> > 
> > Signed-off-by: Mostafa Saleh <smostafa@google.com>
> > ---
> >  .../iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3.c  | 108 +++++++++++++++++-
> >  1 file changed, 105 insertions(+), 3 deletions(-)
> > 
> > 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 554229e466f3..10c6461bbf12 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
> > @@ -325,6 +325,88 @@ static bool is_cmdq_enabled(struct hyp_arm_smmu_v3_device *smmu)
> >  	return FIELD_GET(CR0_CMDQEN, smmu->cr0);
> >  }
> >  
> > +static bool smmu_filter_command(struct hyp_arm_smmu_v3_device *smmu, u64 *command)
> > +{
> > +	u64 type = FIELD_GET(CMDQ_0_OP, command[0]);
> > +
> > +	switch (type) {
> > +	case CMDQ_OP_CFGI_STE:
> > +		/* TBD: SHADOW_STE*/
> > +		break;
> > +	case CMDQ_OP_CFGI_ALL:
> > +	{
> > +		/*
> > +		 * Linux doesn't use range STE invalidation, and only use this
> > +		 * for CFGI_ALL, which is done on reset and not on an new STE
> > +		 * being used.
> > +		 * Although, this is not architectural we rely on the current Linux
> > +		 * implementation.
> > +		 */
> > +		WARN_ON((FIELD_GET(CMDQ_CFGI_1_RANGE, command[1]) != 31));
> > +		break;
> > +	}
> > +	case CMDQ_OP_TLBI_NH_ASID:
> > +	case CMDQ_OP_TLBI_NH_VA:
> > +	case 0x13: /* CMD_TLBI_NH_VAA: Not used by Linux */
> > +	{
> > +		/* Only allow VMID = 0*/
> > +		if (FIELD_GET(CMDQ_TLBI_0_VMID, command[0]) == 0)
> > +			break;
> > +		break;
> > +	}
> > +	case 0x10: /* CMD_TLBI_NH_ALL: Not used by Linux */
> > +	case CMDQ_OP_TLBI_EL2_ALL:
> > +	case CMDQ_OP_TLBI_EL2_VA:
> > +	case CMDQ_OP_TLBI_EL2_ASID:
> > +	case CMDQ_OP_TLBI_S12_VMALL:
> > +	case 0x23: /* CMD_TLBI_EL2_VAA: Not used by Linux */
> > +		/* Malicous host */
> > +		return WARN_ON(true);
> > +	case CMDQ_OP_CMD_SYNC:
> > +		if (FIELD_GET(CMDQ_SYNC_0_CS, command[0]) == CMDQ_SYNC_0_CS_IRQ) {
> > +			/* Allow it, but let the host timeout, as this should never happen. */
> > +			command[0] &= ~CMDQ_SYNC_0_CS;
> > +			command[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_SEV);
> > +			command[1] &= ~CMDQ_SYNC_1_MSIADDR_MASK;
> > +		}
> > +		break;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> > +static void smmu_emulate_cmdq_insert(struct hyp_arm_smmu_v3_device *smmu)
> > +{
> > +	u64 *host_cmdq = hyp_phys_to_virt(smmu->cmdq_host.q_base & Q_BASE_ADDR_MASK);
> > +	int idx;
> > +	u64 cmd[CMDQ_ENT_DWORDS];
> > +	bool skip;
> > +
> > +	if (!is_cmdq_enabled(smmu))
> > +		return;
> > +
> > +	while (!queue_empty(&smmu->cmdq_host.llq)) {
> > +		/* Wait for the command queue to have some space. */
> > +		WARN_ON(smmu_wait_event(smmu, !smmu_cmdq_full(&smmu->cmdq)));
> > +
> > +		idx = Q_IDX(&smmu->cmdq_host.llq, smmu->cmdq_host.llq.cons);
> > +		/* Avoid TOCTOU */
> > +		memcpy(cmd, &host_cmdq[idx * CMDQ_ENT_DWORDS], CMDQ_ENT_DWORDS << 3);
> > +		skip = smmu_filter_command(smmu, cmd);
> > +		if (!skip)
> > +			smmu_add_cmd_raw(smmu, cmd);
> > +		queue_inc_cons(&smmu->cmdq_host.llq);
> > +	}
> 
> Hmmm. There's something I'd not considered before here.
> 
> Ideally, the data structures that are shadowed by the hypervisor would
> be mapped as normal-WB cacheable in both the host and the hypervisor so
> we don't have to worry about coherency and we get the performance
> benefits from the caches. Indeed, I think that's how you've mapped
> 'host_cmdq' above _however_ I sadly don't think we can do that if the
> actual SMMU hardware isn't coherent.
> 
> We don't have a way to say things like "The STEs and CMDQ are coherent
> but the CDs and Stage-1 page-tables aren't" so that means we have to
> treat the shadowed structures populated by the host in the same way as
> the host-owned structures that are consumed directly by the hardware.
> Consequently, we should either be using non-cacheable mappings at EL2
> for these structures or doing CMOs around the accesses.

Thanks for catching that, I missed it, I think we can keep the host shared
as cacheable, and use CMOs when accessing it, I will have a closer look.

Thanks,
Mostafa


> 
> Will