Re: [PATCH v5 3/6] KVM: arm64: Reject attempts to set invalid debug arch version

[Oliver Upton <oliver.upton@xxxxxxxxx>]

Jing,

On Mon, Jul 10, 2023 at 07:24:26PM +0000, Jing Zhang wrote:
> From: Oliver Upton <oliver.upton@xxxxxxxxx>
> 
> 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@xxxxxxxxx>
> Signed-off-by: Jing Zhang <jingzhangos@xxxxxxxxxx>

This patch should be ordered before the change that permits writes to
the DebugVer field (i.e. the previous patch). While we're at it, there's
another set of prerequisites for actually making the field writable.

As Suraj pointed out earlier, we need to override the type of the field
to be FTR_LOWER_SAFE instead of EXACT. Beyond that, KVM limits the field
to 0x6, which is the minimum value for an ARMv8 implementation. We can
relax this limitation up to v8p8, as I believe all of the changes are to
external debug and wouldn't affect a KVM guest.

Below is my (untested) diff on top of your series for both of these
changes.

-- 
Thanks,
Oliver

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 78ccc95624fa..35c4ab8cb5c8 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)
@@ -1466,14 +1472,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.
@@ -1529,6 +1543,8 @@ static u64 read_sanitised_id_dfr0_el1(struct kvm_vcpu *vcpu,
 	u8 perfmon = pmuver_to_perfmon(kvm_arm_pmu_get_pmuver_limit());
 	u64 val = read_sanitised_ftr_reg(SYS_ID_DFR0_EL1);
 
+	val = ID_REG_LIMIT_FIELD_ENUM(val, ID_DFR0_EL1, CopDbg, Debugv8p8);
+
 	val &= ~ID_DFR0_EL1_PerfMon_MASK;
 	if (kvm_vcpu_has_pmu(vcpu))
 		val |= SYS_FIELD_PREP(ID_DFR0_EL1, PerfMon, perfmon);