Re: [PATCH v21 02/15] KVM: arm64: Refuse to start a guest with S1PIE or S1POE but not TCR2

[Lorenzo Stoakes (ARM)]

On Wed, Oct 14, 2026 at 06:48:37PM +0100, Mark Brown wrote:
> On Wed, Oct 14, 2026 at 04:47:22PM +0100, Lorenzo Stoakes (ARM) wrote:
> > On Sat, Oct 03, 2026 at 01:30:05PM +0100, Marc Zyngier wrote:
>
> > > This *absolutely* needs to be data driven, and we're not going back to
> > > over two years ago. We already have most of what is needed in
> > > config.c, and it is only a matter of making sure that S1PxE is only
> > > enabled for the guest if TCR2 and ATS1A are also present. If that
> > > means additional sanitisation of the idregs when finalised, so be it.
>
> I do agree that's what we should have, I started down that route but as
> I noted in the commit log it felt like I was pulling in disproportionate
> refactoring given the existing gap.
>
> > > If userspace decides to expose crap in the ID registers, that's its
> > > own problem, and we're not in the business of enforcing idiotic
> > > configurations. The only thing that matters is that the state that KVM
> > > deals with is consistent.
>
> > As I understand it: don't return an error for a configuration that doesn't
> > make sense, enforce RES0 instead.
>
> I went with erroring out there to make sure that we didn't make any
> mistakes with the ordering of our checks - I didn't want to accidentally
> end up with a situation where we look at the dependencies for feature A,
> decide that it's OK, then later check feature B which it depends on and
> turn it off due to it missing it's own prerequisite.
>
> We can resolve that like you've done here by having the full dependency
> stack in every check but I worried that this would be error prone due to
> all the duplication, however looking at your code I think we can resolve
> that anyway, see blow.
>
> Userspace really shouldn't be setting up guests where we need to make
> adjustments in the first place, and setting up guests isn't that hot a
> path so like Marc says so long as KVM protects itself we're fine.
>
> >  #define kvm_has_s1pie(k)				\
> > -	(kvm_has_feat((k), ID_AA64MMFR3_EL1, S1PIE, IMP))
> > +	(kvm_has_tcr2(k) &&				\
> > +	 kvm_has_feat((k), ID_AA64ISAR2_EL1, ATS1A, IMP) &&	\
> > +	 kvm_has_feat((k), ID_AA64MMFR3_EL1, S1PIE, IMP))
>
> This will mean that every time we check for an individual feature
> throughout KVM we check for all the dependencies as well.  A lot of the
> time that's not really going to matter but we have these checks in hot
> paths like context switches.
>
> > +static bool feat_s1pie(struct kvm *kvm)
> > +{
> > +	return kvm_has_s1pie(kvm);
> > +}
>
> We should put the dependencies in here where they're only checked during
> initialisation instead, that way the hot paths only need to look at the
> leaf features.  We could also deal with the duplication concern by
> having the functions call each other rather than duplicating their
> contents:
>
> 	static bool feat_gcs(struct kvm *kvm)
> 	{
> 		if (!feat_s1pie(kvm))
> 			return false;
>
> 		return kvm_has_gcs(kvm);
> 	}
>
> (or whatever stylistic variation of that.)
>
> This approach also needs the code for updating the ID registers since
> config.c doesn't do that at the moment.  Some of the existing visibility
> limiting stuff (like for 32 bit ID registers without AArch32 or
> ID_AA64ZFR0_EL1 without SVE) could be moved over too.

Hopefully Fuad + Marc's efforts [0] make all of this fairly easy :) as we
now have a specific dependency mechanism, and with GCS added on respin
should enforce all this correctly.

>
> TBH when I said in the commit log I was thinking of something more data
> driven I was thinking more of having the dependencies expressed as pure
> data, more like the way the test in set_id_regs is written.  This also
> seems fine to me, the checks are similar.  It's just much more invasive.

Indeed. Perhaps the config.c approach will make it easier to convert JSON
-> code also?

--
Cheers, Lorenzo

[0]:https://lore.kernel.org/r/20261020064306.1542865-1-fuad.tabba@xxxxxxxxx