| Message ID | 20240808125711.14368-1-shameerali.kolothum.thodi@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Disable OS double lock visibility by default and ignore VMM writes | expand |
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?
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
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?
> -----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! :-)
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.
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; }
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(-)