Re: [PATCH v2 4/6] KVM: arm64: add emulation for CTR_EL0 register

From: Oliver Upton <oliver.upton@linux.dev>
To: Marc Zyngier <maz@kernel.org>
Cc: Sebastian Ott <sebott@redhat.com>,
	linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev,
	linux-kernel@vger.kernel.org, James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>
Subject: Re: [PATCH v2 4/6] KVM: arm64: add emulation for CTR_EL0 register
Date: Fri, 3 May 2024 10:27:46 -0700	[thread overview]
Message-ID: <ZjUekqVOwCWMAKdE@linux.dev> (raw)
In-Reply-To: <861q6irj2t.wl-maz@kernel.org>

On Fri, May 03, 2024 at 04:50:02PM +0100, Marc Zyngier wrote:
> > Marc, I know this goes against what you had suggested earlier, is there
> > something in particular that you think warrants the consistency
> > checks?
> 
> The problem is that we have a dependency chain: individual cache
> levels are validated against CLIDR/CCSIDR, which are themselves
> validated against CTR_EL0.
> 
> Change one, and everything becomes inconsistent. I absolutely don't
> trust userspace to do a good job on that

Violent agreement on this point, heh. 

> and not validating this will result in extremely hard to debug issues
> in the guest. Which is why CTR_EL0 was an invariant the first place,
> and everything derived from it.

Sure, but userspace can completely hose the guest in tons of spectacular
ways, I don't see why feature ID registers require thorough
cross-checking of relationships between CPU features.

We already fail at this. Just looking at ID_AA64ISAR0_EL1, we do not
enforce any of the "FEAT_X implies FEAT_Y" relationships between all of
the crypto extensions. Userspace can also setup ID_AA64MMFR0_EL1 to
advertise that no translation granule is supported by the MMU.

I agree that KVM needs to sanitize feature ID registers against the
capabilities of hardware + KVM itself. Beyond that cross-checking
userspace against itself is difficult to get right, and I'm worried
about what the tangled mess will look like when we finish up the
plumbing for the whole feature ID space.

> Take for example CLIDR_EL1.Lo{UU,UIS,C}. Their values depend on
> CTR_EL0.{IDC,DIC}. SW is free to check one or the other. If you don't
> have this dependency, you're in for some serious trouble.

Right, we absolutely need to sanitize these against *hardware*, and
using CTR_EL0 definitely the way to go. Userspace cannot promise a
stricter cache coherency model than what's offered in hardware.

Making sure userspace's values for CLIDR_EL1 and CTR_EL0 agree with each
other shouldn't matter if we've determined hardware coherency is at least
as strict as the model described through these registers.

Without the cross-check, it would be possible for userspace to setup the
vCPU as:

 - CTR_EL0.{IDC,DIC} = {1, 1}
 - CLIDR_EL1.Lo{UU,UIS,C} = {1, 1, 1}

But we would only allow this if hardware was {IDC,DIC} = {1,1}. So while
the values presented to the guest aren't consistent with one another, it
seems in the worst case the guest will do I$ maintenance where it isn't
actually necessary.

-- 
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel