Re: [PATCH v9 13/25] iommu/arm-smmu-v3-kvm: Add MMIO emulation

[Mostafa Saleh <smostafa@google.com>]

On Thu, Oct 15, 2026 at 08:30:38PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] iommu: mmio emulation bitwise logic corrupted on big-endian hosts
> --
> 
> commit 5a437530a55c608782e029a478141ad9b342c024
> Author: Mostafa Saleh <smostafa@google.com>
> 
> iommu/arm-smmu-v3-kvm: Add MMIO emulation
> 
> This commit adds a data abort handler for the SMMUs to handle MMIO emulation.
> Currently, most registers act as a passthrough, and upcoming patches will
> add CMDQ/STE emulation with logic for specific register access.
> 
> > 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
> [ ... ]
> > @@ -128,6 +132,170 @@ static int smmu_init(void)
> > +static bool smmu_dabt_device(struct hyp_arm_smmu_v3_device *smmu,
> > +			     struct user_pt_regs *regs,
> > +			     u64 esr, u32 off)
> > +{
> [ ... ]
> > +	bool is_xzr = (rd == 31);
> > +	u64 val = is_xzr ? 0 : regs->regs[rd];
> 
> [Severity: High]
> Does extracting the register value directly from the CPU context here bypass
> endianness conversion on big-endian hosts?

mmm, I haven't considered that before.
However, BE support was removed on arm64 [1]
https://lore.kernel.org/all/20260918145217.1669-1-will@kernel.org/

So, I'd say keep it as is.

> 
> When the host (EL1) uses writel_relaxed() on a big-endian machine, it
> byte-swaps the native value to little-endian representation before issuing
> the store. The MMIO trap then retrieves this raw, byte-swapped value
> directly into val.
> 
> > +
> > +	switch (off) {
> > +	case ARM_SMMU_IDR0:
> [ ... ]
> > +	case ARM_SMMU_CR0:
> > +		if (len != sizeof(u32))
> > +			break;
> > +
> > +		if (is_write) {
> > +			/* Enforce ATS checks. */
> > +			if (smmu->features & ARM_SMMU_FEAT_ATS)
> > +				val |= CR0_ATSCHK;
> 
> [Severity: High]
> Can this bitwise operation corrupt the configuration on big-endian hosts?
> 
> Because val contains a byte-swapped value, applying a native bitmask like
> CR0_ATSCHK modifies bits at the wrong offset.
> 

Same as above.

> > +			mask = read_write;
> > +		} else {
> > +			mask = read_write & ~CR0_ATSCHK;
> > +		}
> > +		break;
> [ ... ]
> > +	if (is_write) {
> > +		if (len == sizeof(u64))
> > +			writeq_relaxed(val & mask, smmu->base + off);
> > +		else
> > +			writel_relaxed(val & mask, smmu->base + off);
> 
> [Severity: High]
> Will this double-swap the corrupted value?
> 
> Since val was already byte-swapped by the host's write, writing it back
> using EL2's writel_relaxed() applies a second byte-swap, propagating the
> incorrect state to the SMMU hardware.
> 
> Read paths face a similar issue where the readl_relaxed() output is passed
> directly to the guest without being swapped back into the format expected by
> the guest's own le32_to_cpu() calls.
> 

Same as above.

Thanks,
Mostafa

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