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

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

[Shameer Kolothum]

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.

All this may break migration from the older kernels. Take care of
that by ignoring VMM writes for these values.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
Note:
 - I am not very sure on reporting DebugVer a min of 8.2. Hopefully this is
   fine. Please let me know.
   
Thanks,
Shameer
---  
 arch/arm64/kvm/sys_regs.c | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index c90324060436..06e57d7730d8 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1704,13 +1704,14 @@ static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
 	return val;
 }
 
-#define ID_REG_LIMIT_FIELD_ENUM(val, reg, field, limit)			       \
+#define ID_REG_LIMIT_FIELD_ENUM(val, reg, field, min_val, max_val)	       \
 ({									       \
 	u64 __f_val = FIELD_GET(reg##_##field##_MASK, val);		       \
+	(__f_val) = max_t(u64, __f_val, SYS_FIELD_VALUE(reg, field, min_val)); \
 	(val) &= ~reg##_##field##_MASK;					       \
 	(val) |= FIELD_PREP(reg##_##field##_MASK,			       \
 			    min(__f_val,				       \
-				(u64)SYS_FIELD_VALUE(reg, field, limit)));     \
+				(u64)SYS_FIELD_VALUE(reg, field, max_val)));   \
 	(val);								       \
 })
 
@@ -1719,7 +1720,7 @@ static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
 {
 	u64 val = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
 
-	val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64DFR0_EL1, DebugVer, V8P8);
+	val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64DFR0_EL1, DebugVer, V8P2, V8P8);
 
 	/*
 	 * Only initialize the PMU version if the vCPU was configured with one.
@@ -1732,6 +1733,10 @@ static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
 	/* Hide SPE from guests */
 	val &= ~ID_AA64DFR0_EL1_PMSVer_MASK;
 
+	/* Hide DoubleLock from guests */
+	val &= ~ID_AA64DFR0_EL1_DoubleLock_MASK;
+	val |= SYS_FIELD_PREP_ENUM(ID_AA64DFR0_EL1, DoubleLock, NI);
+
 	return val;
 }
 
@@ -1739,6 +1744,7 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
 			       const struct sys_reg_desc *rd,
 			       u64 val)
 {
+	u64 hw_val = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
 	u8 debugver = SYS_FIELD_GET(ID_AA64DFR0_EL1, DebugVer, val);
 	u8 pmuver = SYS_FIELD_GET(ID_AA64DFR0_EL1, PMUVer, val);
 
@@ -1765,6 +1771,28 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
 	 */
 	if (debugver < ID_AA64DFR0_EL1_DebugVer_IMP)
 		return -EINVAL;
+	else if (debugver < ID_AA64DFR0_EL1_DebugVer_V8P2) {
+		/*
+		 * KVM now reports a minimum DebugVer 8.2 to Guests. In order to keep
+		 * the migration working from old kernels, check and ignore the VMM
+		 * write.
+		 */
+		if ((hw_val & ID_AA64DFR0_EL1_DebugVer_MASK) ==
+		    (val & ID_AA64DFR0_EL1_DebugVer_MASK)) {
+			val &= ~ID_AA64DFR0_EL1_DebugVer_MASK;
+			val |= SYS_FIELD_PREP_ENUM(ID_AA64DFR0_EL1, DebugVer, V8P2);
+		}
+	}
+	/*
+	 * KVM used to expose OS double lock feature bit to Guests but returned
+	 * RAZ/WI on Guest OSDLR_EL1 access. We are hiding OS double lock now.
+	 * But for migration from old kernels to work ignore the VMM write.
+	 */
+	if ((hw_val & ID_AA64DFR0_EL1_DoubleLock_MASK) ==
+	    (val & ID_AA64DFR0_EL1_DoubleLock_MASK)) {
+		val &= ~ID_AA64DFR0_EL1_DoubleLock_MASK;
+		val |= SYS_FIELD_PREP_ENUM(ID_AA64DFR0_EL1, DoubleLock, NI);
+	}
 
 	return set_id_reg(vcpu, rd, val);
 }
@@ -1779,7 +1807,7 @@ static u64 read_sanitised_id_dfr0_el1(struct kvm_vcpu *vcpu,
 	if (kvm_vcpu_has_pmu(vcpu))
 		val |= SYS_FIELD_PREP(ID_DFR0_EL1, PerfMon, perfmon);
 
-	val = ID_REG_LIMIT_FIELD_ENUM(val, ID_DFR0_EL1, CopDbg, Debugv8p8);
+	val = ID_REG_LIMIT_FIELD_ENUM(val, ID_DFR0_EL1, CopDbg, NI, Debugv8p8);
 
 	return val;
 }
-- 
2.45.2

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

[Oliver Upton]

Hi Shameer,

I find myself asking *why* we need this, could you share some details
on the issue you're encountering?

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?

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

Would allowing userspace to downgrade ID_AA664DFR0_EL1.DoubleLock to
0b1111 be enough?

-- 
Thanks,
Oliver

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

[Shameerali Kolothum Thodi]

Hi Oliver,

> -----Original Message-----
> From: Oliver Upton <oliver.upton@linux.dev>
> Sent: Thursday, August 8, 2024 6:40 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
> 
> Hi Shameer,
> 
> 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).

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

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

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

Thanks,
Shameer

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

[Oliver Upton]

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

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

[Shameerali Kolothum Thodi]

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

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

[Oliver Upton]

On Thu, Aug 08, 2024 at 06:38:41PM +0000, Shameerali Kolothum Thodi wrote:

[...]

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

That'd be great, we've been playing a game of whack-a-mole with the ID
registers for some time. If you could handle all of the other fields
associated with the register(s) you need to tweak (e.g. DFR0 in this case)
that would be *extremely* helpful.

-- 
Thanks,
Oliver