diff mbox series

[v6,2/6] KVM: arm64: Reject attempts to set invalid debug arch version

Message ID 20230718164522.3498236-3-jingzhangos@google.com (mailing list archive)
State New, archived
Headers show
Series Enable writable for idregs DFR0,PFR0, MMFR{0,1,2, 3} | expand

Commit Message

Jing Zhang July 18, 2023, 4:45 p.m. UTC 
From: Oliver Upton <oliver.upton@linux.dev>

The debug architecture is mandatory in ARMv8, so KVM should not allow
userspace to configure a vCPU with less than that. Of course, this isn't
handled elegantly by the generic ID register plumbing, as the respective
ID register fields have a nonzero starting value.

Add an explicit check for debug versions less than v8 of the
architecture.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
 arch/arm64/kvm/sys_regs.c | 34 +++++++++++++++++++++++++++++++---
 1 file changed, 31 insertions(+), 3 deletions(-)

Comments

Oliver Upton July 21, 2023, 9:18 p.m. UTC | #1 
On Tue, Jul 18, 2023 at 04:45:18PM +0000, Jing Zhang wrote:
> From: Oliver Upton <oliver.upton@linux.dev>
> 
> The debug architecture is mandatory in ARMv8, so KVM should not allow
> userspace to configure a vCPU with less than that. Of course, this isn't
> handled elegantly by the generic ID register plumbing, as the respective
> ID register fields have a nonzero starting value.
> 
> Add an explicit check for debug versions less than v8 of the
> architecture.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> Signed-off-by: Jing Zhang <jingzhangos@google.com>

This patch needs to be broken up. You're doing a couple things:

 1) Forcing the behavior of the DebugVer field to be FTR_LOWER_SAFE, and
   adding the necessary check for a valid version

 2) Changing KVM's value for the field to expose up to Debugv8p8 to the
   guest.

The latter isn't described in the changelog at all, and worse yet the
ordering of the series is not bisectable. Changing the default value of
the field w/o allowing writes breaks migration.

So, please split this patch in two and consider stacking like so:

 - Change #1 above (field sanitization)

 - "KVM: arm64: Enable writable for ID_AA64DFR0_EL1 and ID_DFR0_EL1"

 - Change #2 above (advertise up to v8p8)
Jing Zhang July 21, 2023, 10:26 p.m. UTC | #2 
Hi Oliver,

On Fri, Jul 21, 2023 at 2:18 PM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> On Tue, Jul 18, 2023 at 04:45:18PM +0000, Jing Zhang wrote:
> > From: Oliver Upton <oliver.upton@linux.dev>
> >
> > The debug architecture is mandatory in ARMv8, so KVM should not allow
> > userspace to configure a vCPU with less than that. Of course, this isn't
> > handled elegantly by the generic ID register plumbing, as the respective
> > ID register fields have a nonzero starting value.
> >
> > Add an explicit check for debug versions less than v8 of the
> > architecture.
> >
> > Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> > Signed-off-by: Jing Zhang <jingzhangos@google.com>
>
> This patch needs to be broken up. You're doing a couple things:
>
>  1) Forcing the behavior of the DebugVer field to be FTR_LOWER_SAFE, and
>    adding the necessary check for a valid version
>
>  2) Changing KVM's value for the field to expose up to Debugv8p8 to the
>    guest.
>
> The latter isn't described in the changelog at all, and worse yet the
> ordering of the series is not bisectable. Changing the default value of
> the field w/o allowing writes breaks migration.
>
> So, please split this patch in two and consider stacking like so:
>
>  - Change #1 above (field sanitization)
>
>  - "KVM: arm64: Enable writable for ID_AA64DFR0_EL1 and ID_DFR0_EL1"
>
>  - Change #2 above (advertise up to v8p8)
>
> --
> Thanks,
> Oliver
Sure, I'll split it as you suggested.

Thanks,
Jing
diff mbox series

Patch

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index c1a5ec1a016e..053d8057ff1e 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1216,8 +1216,14 @@  static s64 kvm_arm64_ftr_safe_value(u32 id, const struct arm64_ftr_bits *ftrp,
 	/* Some features have different safe value type in KVM than host features */
 	switch (id) {
 	case SYS_ID_AA64DFR0_EL1:
-		if (kvm_ftr.shift == ID_AA64DFR0_EL1_PMUVer_SHIFT)
+		switch (kvm_ftr.shift) {
+		case ID_AA64DFR0_EL1_PMUVer_SHIFT:
 			kvm_ftr.type = FTR_LOWER_SAFE;
+			break;
+		case ID_AA64DFR0_EL1_DebugVer_SHIFT:
+			kvm_ftr.type = FTR_LOWER_SAFE;
+			break;
+		}
 		break;
 	case SYS_ID_DFR0_EL1:
 		if (kvm_ftr.shift == ID_DFR0_EL1_PerfMon_SHIFT)
@@ -1469,14 +1475,22 @@  static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
 	return val;
 }
 
+#define ID_REG_LIMIT_FIELD_ENUM(val, reg, field, limit)			       \
+({									       \
+	u64 __f_val = FIELD_GET(reg##_##field##_MASK, val);		       \
+	(val) &= ~reg##_##field##_MASK;					       \
+	(val) |= FIELD_PREP(reg##_##field##_MASK,			       \
+			min(__f_val, (u64)reg##_##field##_##limit));	       \
+	(val);								       \
+})
+
 static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
 					  const struct sys_reg_desc *rd)
 {
 	u64 val = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
 
 	/* Limit debug to ARMv8.0 */
-	val &= ~ID_AA64DFR0_EL1_DebugVer_MASK;
-	val |= SYS_FIELD_PREP_ENUM(ID_AA64DFR0_EL1, DebugVer, IMP);
+	val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64DFR0_EL1, DebugVer, V8P8);
 
 	/*
 	 * Only initialize the PMU version if the vCPU was configured with one.
@@ -1496,6 +1510,7 @@  static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
 			       const struct sys_reg_desc *rd,
 			       u64 val)
 {
+	u8 debugver = SYS_FIELD_GET(ID_AA64DFR0_EL1, DebugVer, val);
 	u8 pmuver = SYS_FIELD_GET(ID_AA64DFR0_EL1, PMUVer, val);
 
 	/*
@@ -1515,6 +1530,13 @@  static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
 	if (pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF)
 		val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
 
+	/*
+	 * ID_AA64DFR0_EL1.DebugVer is one of those awkward fields with a
+	 * nonzero minimum safe value.
+	 */
+	if (debugver < ID_AA64DFR0_EL1_DebugVer_IMP)
+		return -EINVAL;
+
 	return set_id_reg(vcpu, rd, val);
 }
 
@@ -1528,6 +1550,8 @@  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);
+
 	return val;
 }
 
@@ -1536,6 +1560,7 @@  static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
 			   u64 val)
 {
 	u8 perfmon = SYS_FIELD_GET(ID_DFR0_EL1, PerfMon, val);
+	u8 copdbg = SYS_FIELD_GET(ID_DFR0_EL1, CopDbg, val);
 
 	if (perfmon == ID_DFR0_EL1_PerfMon_IMPDEF) {
 		val &= ~ID_DFR0_EL1_PerfMon_MASK;
@@ -1551,6 +1576,9 @@  static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
 	if (perfmon != 0 && perfmon < ID_DFR0_EL1_PerfMon_PMUv3)
 		return -EINVAL;
 
+	if (copdbg < ID_DFR0_EL1_CopDbg_Armv8)
+		return -EINVAL;
+
 	return set_id_reg(vcpu, rd, val);
 }