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

[Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>]

> -----Original Message-----
> From: Oliver Upton <oliver.upton@linux.dev>
> Sent: Thursday, August 8, 2024 7:31 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: kvmarm@lists.linux.dev; linux-arm-kernel@lists.infradead.org;
> maz@kernel.org; will@kernel.org; catalin.marinas@arm.com;
> james.morse@arm.com; suzuki.poulose@arm.com; yuzenghui
> <yuzenghui@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>;
> Linuxarm <linuxarm@huawei.com>
> Subject: Re: [PATCH] KVM: arm64: Disable OS double lock visibility by default
> and ignore VMM writes
> 
> 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! :-)

😊. Not intentional. I found it after I prepared this patch and felt
validated!.

> 
> 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.

Yeah. That was the bit I was not sure about. Ok, fine then.

> > 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.

Yeah. Got it.

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

Sure. Will do.(And we might need more as well. Let me see.)

Thanks,
Shameer