Re: [PATCH v9 18/25] iommu/arm-smmu-v3-kvm: Shadow STEs

[Mostafa Saleh <smostafa@google.com>]

On Thu, Oct 15, 2026 at 08:30:26PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] hyp-arm64: fatal WARN_ON(skip) on normal pkvm_iommu_alloc_pages() -ENOMEM failure
> - [High] iommu/arm-smmu-v3: torn STE write sets Valid bit before configuration
> --
> 
> commit bc7189831be67d30110e4ed04545c980d138dc99
> Author: Mostafa Saleh <smostafa@google.com>
> 
>     iommu/arm-smmu-v3-kvm: Shadow STEs
>     
>     Shadow the STE when the host sends CFGI_STE: copy the host entry as is
>     into the hypervisor-owned stream table, which is the one the hardware
>     uses. For 2-level tables the host L2 table is shared and pinned while
>     the entry is read, and the hypervisor L2 table is allocated from the
>     IOMMU pool the first time its L1 slot is used.
> 
> > 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 0fa98915e00d4..132f3a9ba3084 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
> [ ... ]
> > @@ -290,6 +293,91 @@ static int smmu_init_cmdq(struct hyp_arm_smmu_v3_device *smmu)
> [ ... ]
> > +static int smmu_reshadow_ste(struct hyp_arm_smmu_v3_device *smmu, u32 sid, bool leaf)
> > +{
> [ ... ]
> > +		/* No hypervisor entry, first time the L2 is populated. */
> > +		if (!l1tab[l1_idx].l2ptr) {
> > +			struct arm_smmu_strtab_l2 *l2table;
> > +
> > +			l2table = pkvm_iommu_alloc_pages(get_order(sizeof(*l2table)));
> > +			if (!l2table)
> > +				return -ENOMEM;
> 
> [Severity: High]
> If this allocation fails, smmu_reshadow_ste() returns -ENOMEM, which causes
> smmu_filter_command() to return true. In smmu_emulate_cmdq_insert(), this
> triggers a preexisting WARN_ON(skip):
> 
> static int smmu_emulate_cmdq_insert(struct hyp_arm_smmu_v3_device *smmu)
> {
>         ...
>         skip = smmu_filter_command(smmu, cmd);
>         if (WARN_ON(skip))
>                 continue;
>         ...
> }
> 
> Since WARN_ON() expands to a fatal BRK at EL2, does this allow a normal
> out-of-memory condition to panic the hypervisor?
> 

Allocation shouldn't fail as the carveout should cover the worst case.

> > +			/* Ensure L2 is observed before writing the ptr. */
> > +			dma_wmb();
> > +			arm_smmu_write_strtab_l1_desc(&l1tab[l1_idx], hyp_virt_to_phys(l2table));
> > +		}
> > +		l2ptr = hyp_phys_to_virt(le64_to_cpu(l1tab[l1_idx].l2ptr) &
> > +				STRTAB_L1_DESC_L2PTR_MASK);
> > +		hyp_ste_ptr = &l2ptr->stes[arm_smmu_strtab_l2_idx(sid)];
> > +	}
> > +
> > +	for (i = 0; i < STRTAB_STE_DWORDS; ++i)
> > +		WRITE_ONCE(hyp_ste_ptr->data[i], host_ste_ptr->data[i]);
> 
> [Severity: High]
> Does this copy sequence write the Valid bit (bit 0 of DWord 0) before the
> rest of the stream table entry is fully populated?
> 
> Because the loop starts at i = 0, hyp_ste_ptr->data[0] is written first.
> If the hardware concurrently fetches this entry, could it read a Valid STE
> while DWords 1 through 7 still contain uninitialized data?
> 

This only copies the host STE (which follows the spec) for bisectability
that is fixed next by using the ste writer.

Thanks,
Mostafa

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