Re: [PATCH v7 23/24] iommu/arm-smmu-v3-kvm: Enable nesting

[Mostafa Saleh <smostafa@google.com>]

On Wed, Aug 26, 2026 at 09:38:31AM -0300, Jason Gunthorpe wrote:
> On Wed, Aug 26, 2026 at 08:54:19AM +0000, Mostafa Saleh wrote:
> > On Tue, Aug 25, 2026 at 05:59:50PM -0300, Jason Gunthorpe wrote:
> > > > [ ... 129 lines skipped ... ]
> > > > +	ret = smmu_attach_stage_2(&target);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +	hyp_spin_lock(&smmu->hw_lock);
> > > > +	cur_valid = FIELD_GET(STRTAB_STE_0_V, le64_to_cpu(hyp_ste_ptr->data[0]));
> > > > +	target_cfg = FIELD_GET(STRTAB_STE_0_CFG, le64_to_cpu(target.data[0]));
> > > > +	target_valid = FIELD_GET(STRTAB_STE_0_V, le64_to_cpu(target.data[0]));
> > > > +	if ((cur_valid && !target_valid) ||
> > > > +	    (target_cfg == STRTAB_STE_0_CFG_ABORT)) {
> > > > +		WRITE_ONCE(hyp_ste_ptr->data[0], target.data[0]);
> > > > +		WARN_ON(smmu_send_cmd(smmu, &cfgi_cmd));
> > > > +		for (i = 1; i < STRTAB_STE_DWORDS; i++)
> > > > +			WRITE_ONCE(hyp_ste_ptr->data[i], target.data[i]);
> > > > +	} else {
> > > > +		for (i = 1; i < STRTAB_STE_DWORDS; i++)
> > > > +			WRITE_ONCE(hyp_ste_ptr->data[i], target.data[i]);
> > > > +		WARN_ON(smmu_send_cmd(smmu, &cfgi_cmd));
> > > > +		WRITE_ONCE(hyp_ste_ptr->data[0], target.data[0]);
> > > > +	}
> > > 
> > > This doesn't look good enough, a driver can't safe writely to a valid
> > > STE in any order like this, and it can't make it non-valid or risk
> > > breaking guests. We had this bug in linux already, the hitless STE
> > > update in the hypervisor is mandatory for linux guests using PASID.
> > 
> > This piggy-backs on the kernel algorithm, as described in the comment
> > in this patch:
> > +       /*
> > +        * Summary of each host emulated state vs real HW.
> > +        * |    Host    |       HW      |
> > +        * ==============================
> > +        * |    V=0     |       V=0     |
> > +        * |    Abort   |       Abort   |
> > +        * |    Bypass  |       S2      |
> > +        * |    S1      |       S1+S2   |
> > +        *
> > +        * For the host, any V=0 transition is not hitless, all other permutations of
> > +        * (abort, bypass, S1) transitions are hitless.
> > +        * For the HW state, any V=0 transition is not hitless, as all the S2 config is
> > +        * always the same (ttbr, vtcr...), all other transitions should be hitless too.
> > +        * However, the host is not trusted, which means that any V=0 <=> V=1 transitions
> > +        * or any transition to an abort STE we need to enforce writing order of the STE
> > +        * dword 0 and add CFGI.
> > +        * Otherwise, we write the STE in the opposite order to cover cases from abort
> > +        * to S2 or nested.
> > +        */
> > 
> > So this way, the hypervisor doesn't break the STE and keeps the host
> > STE transitions hitless.
> > 
> > Or there is an example that I missed and is broken by this?
> 
> If we are exiting S1DSS mode it looks like this clears S1DSS before
> changing to S2 only mode? That would be a bug.
> 

From S1DSS (nested in HW) to bypass (S2 in HW), the kernel STE writer
will do a hitless update by writing DWORD 0 => CFGI => DOWRD1-3 => CFGI
The hypervisor will transition Nested to S2 only on the first CFGI which
is safe.

However, it would be harder to reason about a buggy or a malicious kernel,
my impression was it can only shoot it's own foot with bad STEs.

I will give this more thought.

> There are also gotchas where writing things out of order can make the
> STE UNPREDICTABLE that must be avoided too.
> 
> We really don't need two versions of this stuf, just use the
> programmer we know it is right at this point.

The problem with that it needs to pull a lot of code out of the
SMMUv3 driver which has things as struct arm_smmu_master.
I will need to check how that can work, but I will try to avoid
that, specially this is not a generic STE writer it has quite
small transitions which shouldn't change in the future.

Thanks,
Mostafa

> 
> Jason