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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3458 bytes --]

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.

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]