Re: [PATCH] KVM: arm64: Disable OS double lock visibility by default and ignore VMM writes

[Oliver Upton <oliver.upton@linux.dev>]

On Thu, Aug 08, 2024 at 06:10:33PM +0000, Shameerali Kolothum Thodi wrote:
> > I find myself asking *why* we need this, could you share some details
> > on the issue you're encountering?
> 
> Sorry, I missed the why part. Mainly for VM migration purposes as we have systems
> with DoubleLock implemented and not implemented(with DebugVer 8.2).

Ah, got it, thanks for the context.

> > 
> > Indeed, RAZ/WI is not a faithful implementation of FEAT_DoubleLock, but
> > I wouldn't expect it to be used in a VM in the first place.
> > 
> > On Thu, Aug 08, 2024 at 01:57:11PM +0100, Shameer Kolothum wrote:
> > > KVM exposes the OS double lock feature bit to Guests but returns
> > > RAZ/WI on Guest OSDLR_EL1 access. Make sure we are hiding OS double
> > > lock from Guests now. However we can't hide DoubleLock if the reported
> > > DebugVer is < 8.2. So report a minimum DebugVer of 8.2 to Guests.
> > 
> > What if a user wanted to virtualize an exact CPU model that only
> > implemented v8.0?
> 
> Yeah. I was a bit concerned as mentioned below of bumping up DebugVer to 8.2.
> But then I found a similar attempt you made a while back,
> https://lore.kernel.org/linux-arm-kernel/20211029003202.158161-1-oupton@google.com/T/#meee94d87db3f8042156557dbf9743bb03cf0aaa9

Using my own crap patches against me, drat! :-)

In all seriousness, this has since been resolved with the 'writable' ID
register infrastructure and corresponding changes raising the max
possible debug arch to v8.8. DebugVer will match the HW value up to
v8.8, but userspace can 'downgrade' the feature by writing a lesser
value.

So in the case of cross-system migration, the old value is still
accepted by KVM + advertised to the VM.

> > 
> > > All this may break migration from the older kernels. Take care of
> > > that by ignoring VMM writes for these values.
> > 
> > Ignoring userspace writes is a pretty big hammer. In situations where
> > KVM had advertised a feature that was outright not supported (e.g. IMP DEF
> > PMUs) it _might_ make sense. But with this change we're messing with a
> > CPU feature we *do* support.
> 
> The concern here is for the DebugVer I guess.

Indeed.

> But if VMs are not making use of any 8.0 specific features(as I understand it,
> only external debugger support is the difference), then is that an issue?

You're absolutely right to point out that v8.0 -> v8.2 doesn't change
anything from the VM's POV.

My concern is that the guest does not anticipate ID registers changing
values at runtime, and we should only reach for that big hammer if we
(KVM) have done something truly stupid.

Which never happens, of course :)

> > Would allowing userspace to downgrade ID_AA664DFR0_EL1.DoubleLock to
> > 0b1111 be enough?
> 
> Yeah. Could I guess. But then we need to check the DebugVer matches to 8.2 or not
> as well.

Eh, I don't think that KVM needs to be policing the VMM for total
compliance with the architecture. What's far more important is
guaranteeing the selected CPU feature set is a subset of what KVM
virtualizes.

So even though the architecture says

  !FEAT_DoubleLock && !FEAT_Debugv8p2

is not allowed, it isn't a problematic configuration for KVM. Still,
the defaults from KVM should still comply with the architecture as
closely as possible.

> Idea was, is there any point in exposing  features that are not supported or used
> by VMs in the first place.

And I generally agree, but the need to churn other fields to get to a
sane starting point gave me a bit of pause.

Would you be willing to cook up a patch that just opens up the
DoubleLock field to downgrades?

-- 
Thanks,
Oliver