Skip to contentSkip to conversation

KVM: arm64: set_id_regs: Allow GICv3 support to be set at runtime

PATCH Applied Marc Zyngier 2 participants 3 messages Mar 23 – Mar 24

Subsystems (3)
KVM Kselftest KVM arm64
lore
Share & tools
lore

Marc Zyngier

author maintainer
#0 lore raw

PATCH 1/1 KVM: arm64: set_id_regs: Allow GICv3 support to be set at runtime

set_id_regs creates a GIC3 guest when possible, and then proceeds to write the ID registers as if they were not affected by the presence of a GIC. As it turns out, ID_AA64PFR1_EL1 is the proof of the contrary.

KVM now makes a point in exposing the GIC support to the guest, no matter what userspace says (userspace such as QEMU is known to write silly things at times).

Accommodate for this level of nonsense by teaching set_id_regs about fields that are mutable, and only compare registers that have been re-sanitised first.

Reported-by Mark Brown <[email protected]>
Signed-off-by Marc Zyngier <[email protected]>
---
 .../testing/selftests/kvm/arm64/set_id_regs.c | 52 ++++++++++++++++---
 1 file changed, 45 insertions(+), 7 deletions(-)
tools/testing/selftests/kvm/arm64/set_id_regs.c +45 -7
index 73de5be58bab0..24f70312b3557 100644
@@ -37,6 +37,9 @@ struct reg_ftr_bits {
 	 * For FTR_LOWER_SAFE, safe_val is used as the minimal safe value.
 	 */
 	int64_t safe_val;
+
+	/* Allowed to be changed by the host after run */
+	bool mutable;
 };
 
 struct test_feature_reg {

@@ -44,7 +47,7 @@ struct test_feature_reg {
 	const struct reg_ftr_bits *ftr_bits;
 };
 
-#define __REG_FTR_BITS(NAME, SIGNED, TYPE, SHIFT, MASK, SAFE_VAL)	\
+#define __REG_FTR_BITS(NAME, SIGNED, TYPE, SHIFT, MASK, SAFE_VAL, MUT)	\
 	{								\
 		.name = #NAME,						\
 		.sign = SIGNED,						\

@@ -52,15 +55,20 @@ struct test_feature_reg {
 		.shift = SHIFT,						\
 		.mask = MASK,						\
 		.safe_val = SAFE_VAL,					\
+		.mutable = MUT,						\
 	}
 
 #define REG_FTR_BITS(type, reg, field, safe_val) \
 	__REG_FTR_BITS(reg##_##field, FTR_UNSIGNED, type, reg##_##field##_SHIFT, \
-		       reg##_##field##_MASK, safe_val)
+		       reg##_##field##_MASK, safe_val, false)
+
+#define REG_FTR_BITS_MUTABLE(type, reg, field, safe_val) \
+	__REG_FTR_BITS(reg##_##field, FTR_UNSIGNED, type, reg##_##field##_SHIFT, \
+		       reg##_##field##_MASK, safe_val, true)
 
 #define S_REG_FTR_BITS(type, reg, field, safe_val) \
 	__REG_FTR_BITS(reg##_##field, FTR_SIGNED, type, reg##_##field##_SHIFT, \
-		       reg##_##field##_MASK, safe_val)
+		       reg##_##field##_MASK, safe_val, false)
 
 #define REG_FTR_END					\
 	{						\

@@ -134,7 +142,8 @@ static const struct reg_ftr_bits ftr_id_aa64pfr0_el1[] = {
 	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR0_EL1, CSV2, 0),
 	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR0_EL1, DIT, 0),
 	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR0_EL1, SEL2, 0),
-	REG_FTR_BITS(FTR_EXACT, ID_AA64PFR0_EL1, GIC, 0),
+	/* GICv3 support will be forced at run time if available */
+	REG_FTR_BITS_MUTABLE(FTR_EXACT, ID_AA64PFR0_EL1, GIC, 0),
 	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR0_EL1, EL3, 1),
 	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR0_EL1, EL2, 1),
 	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR0_EL1, EL1, 1),

@@ -634,12 +643,38 @@ static void test_user_set_mte_reg(struct kvm_vcpu *vcpu)
 		ksft_test_result_pass("ID_AA64PFR1_EL1.MTE_frac no longer 0xF\n");
 }
 
+static uint64_t reset_mutable_bits(uint32_t id, uint64_t val)
+{
+	struct test_feature_reg *reg = NULL;
+
+	for (int i = 0; i < ARRAY_SIZE(test_regs); i++) {
+		if (test_regs[i].reg == id) {
+			reg = &test_regs[i];
+			break;
+		}
+	}
+
+	if (!reg)
+		return val;
+
+	for (const struct reg_ftr_bits *bits = reg->ftr_bits; bits->type != FTR_END; bits++) {
+		if (bits->mutable) {
+			val &= ~bits->mask;
+			val |= bits->safe_val << bits->shift;
+		}
+	}
+
+	return val;
+}
+
 static void test_guest_reg_read(struct kvm_vcpu *vcpu)
 {
 	bool done = false;
 	struct ucall uc;
 
 	while (!done) {
+		uint64_t val;
+
 		vcpu_run(vcpu);
 
 		switch (get_ucall(vcpu, &uc)) {

@@ -647,9 +682,11 @@ static void test_guest_reg_read(struct kvm_vcpu *vcpu)
 			REPORT_GUEST_ASSERT(uc);
 			break;
 		case UCALL_SYNC:
+			val = test_reg_vals[encoding_to_range_idx(uc.args[2])];
+			val = reset_mutable_bits(uc.args[2], val);
+
 			/* Make sure the written values are seen by guest */
-			TEST_ASSERT_EQ(test_reg_vals[encoding_to_range_idx(uc.args[2])],
-				       uc.args[3]);
+			TEST_ASSERT_EQ(val, reset_mutable_bits(uc.args[2], uc.args[3]));
 			break;
 		case UCALL_DONE:
 			done = true;

@@ -740,7 +777,8 @@ static void test_assert_id_reg_unchanged(struct kvm_vcpu *vcpu, uint32_t encodin
 	uint64_t observed;
 
 	observed = vcpu_get_reg(vcpu, KVM_ARM64_SYS_REG(encoding));
-	TEST_ASSERT_EQ(test_reg_vals[idx], observed);
+	TEST_ASSERT_EQ(reset_mutable_bits(test_reg_vals[idx], encoding),
+		       reset_mutable_bits(observed, encoding));
 }
 
 static void test_reset_preserves_id_regs(struct kvm_vcpu *vcpu)
Signature · 1 line
2.47.3
Tested-by on v1 1/1
#1 lore raw
Quoted 4 lines · Marc Zyngier

set_id_regs creates a GIC3 guest when possible, and then proceeds to write the ID registers as if they were not affected by the presence of a GIC. As it turns out, ID_AA64PFR1_EL1 is the proof of the contrary.

Thanks, this fixes the issues I was seeing in -next:

Tested-by Mark Brown <[email protected]>
author maintainer Applied
#2 lore raw
In reply to Marc Zyngier
Quoted 10 lines · Marc Zyngier

set_id_regs creates a GIC3 guest when possible, and then proceeds to write the ID registers as if they were not affected by the presence of a GIC. As it turns out, ID_AA64PFR1_EL1 is the proof of the contrary.

KVM now makes a point in exposing the GIC support to the guest, no matter what userspace says (userspace such as QEMU is known to write silly things at times).

[...]

Applied to next, thanks!

[1/1] KVM: arm64: set_id_regs: Allow GICv3 support to be set at runtime

      commit: 2b821a6205b8b7fa7f0486507a63a46a06562a2d

Cheers,

	M.
Signature · 1 line
Without deviation from the norm, progress is not possible.