[PATCH v2 0/4] KVM: arm64: writable MIDR/REVIDR

[PATCH v2 0/4] KVM: arm64: writable MIDR/REVIDR

[Sebastian Ott]

Based on prior discussion [1] this series allows VMMs to change
MIDR/REVIDR to enable migration between machines that differ in
these registers. Since these are used for errata handling the
errata management series [2] is a prerequisite for this one.

changes for V2:
* let the guest actually observe the changed MIDR_EL1 value
* extra .set_user function
* added selftest

[1] https://lore.kernel.org/kvmarm/20250124151732.6072-1-shameerali.kolothum.thodi@huawei.com/T/#mb855bc51714095a164a7b26bb8bead1606e4b753
[2] https://lore.kernel.org/kvmarm/20250205132222.55816-1-shameerali.kolothum.thodi@huawei.com/T/

Sebastian Ott (4):
  KVM: arm64: Allow userspace to change MIDR_EL1
  KVM: arm64: Allow userspace to change REVIDR_EL1
  KVM: arm64: Allow userspace to change AIDR_EL1
  KVM: selftests: arm64: Test writes to MIDR,REVIDR,AIDR

 arch/arm64/include/asm/kvm_host.h             |   9 ++
 arch/arm64/kvm/sys_regs.c                     | 151 ++++++++----------
 .../testing/selftests/kvm/arm64/set_id_regs.c |  32 +++-
 3 files changed, 100 insertions(+), 92 deletions(-)

-- 
2.42.0

[PATCH v2 1/4] KVM: arm64: Allow userspace to change MIDR_EL1

[Sebastian Ott]

Enable VMMs to write MIDR_EL1 by treating it as a VM ID register.
Since MIDR_EL1 is not handled as a proper arm64_ftr_reg apply only
a sanity check against the writable mask to ensure the reserved
bits are 0.

Set up VPIDR_EL2 to hold the MIDR_EL1 value for the guest.

Signed-off-by: Sebastian Ott <sebott@redhat.com>
---
 arch/arm64/include/asm/kvm_host.h |  3 ++
 arch/arm64/kvm/sys_regs.c         | 56 +++++++++++++++++++++++++++++--
 2 files changed, 56 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 7cfa024de4e3..3db8c773339e 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -373,6 +373,7 @@ struct kvm_arch {
 #define KVM_ARM_ID_REG_NUM	(IDREG_IDX(sys_reg(3, 0, 0, 7, 7)) + 1)
 	u64 id_regs[KVM_ARM_ID_REG_NUM];
 
+	u64 midr_el1;
 	u64 ctr_el0;
 
 	/* Masks for VNCR-backed and general EL2 sysregs */
@@ -1469,6 +1470,8 @@ static inline u64 *__vm_id_reg(struct kvm_arch *ka, u32 reg)
 	switch (reg) {
 	case sys_reg(3, 0, 0, 1, 0) ... sys_reg(3, 0, 0, 7, 7):
 		return &ka->id_regs[IDREG_IDX(reg)];
+	case SYS_MIDR_EL1:
+		return &ka->midr_el1;
 	case SYS_CTR_EL0:
 		return &ka->ctr_el0;
 	default:
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 82430c1e1dd0..7e1c9884f62a 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1666,7 +1666,7 @@ static bool is_feature_id_reg(u32 encoding)
  */
 static inline bool is_vm_ftr_id_reg(u32 id)
 {
-	if (id == SYS_CTR_EL0)
+	if (id == SYS_CTR_EL0 || id == SYS_MIDR_EL1)
 		return true;
 
 	return (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 &&
@@ -1999,6 +1999,47 @@ static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 	return 0;
 }
 
+static int set_id_reg_non_ftr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
+			      u64 val)
+{
+	u32 id = reg_to_encoding(rd);
+	int ret;
+
+	mutex_lock(&vcpu->kvm->arch.config_lock);
+	/*
+	 * Once the VM has started the ID registers are immutable. Reject any
+	 * write that does not match the final register value.
+	 */
+	if (kvm_vm_has_ran_once(vcpu->kvm)) {
+		if (val != read_id_reg(vcpu, rd))
+			ret = -EBUSY;
+		else
+			ret = 0;
+
+		mutex_unlock(&vcpu->kvm->arch.config_lock);
+		return ret;
+	}
+
+	/*
+	 * For non ftr regs do a limited test against the writable mask only.
+	 */
+	if ((rd->val & val) != val) {
+		mutex_unlock(&vcpu->kvm->arch.config_lock);
+		return -EINVAL;
+	}
+
+	kvm_set_vm_id_reg(vcpu->kvm, id, val);
+	/*
+	 * Since guest access to MIDR_EL1 is not trapped
+	 * set up VPIDR_EL2 to hold the MIDR_EL1 value.
+	 */
+	if (id == SYS_MIDR_EL1)
+		write_sysreg(val, vpidr_el2);
+
+	mutex_unlock(&vcpu->kvm->arch.config_lock);
+	return 0;
+}
+
 static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 		      u64 val)
 {
@@ -2493,6 +2534,15 @@ static bool access_mdcr(struct kvm_vcpu *vcpu,
 	return true;
 }
 
+#define FUNCTION_RESET(reg)						\
+	static u64 reset_##reg(struct kvm_vcpu *v,			\
+			       const struct sys_reg_desc *r)		\
+	{								\
+		return read_sysreg(reg);				\
+	}
+
+FUNCTION_RESET(midr_el1)
+
 
 /*
  * Architected system registers.
@@ -2542,6 +2592,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 
 	{ SYS_DESC(SYS_DBGVCR32_EL2), undef_access, reset_val, DBGVCR32_EL2, 0 },
 
+	{ ID_DESC(MIDR_EL1), .set_user = set_id_reg_non_ftr, .visibility = id_visibility,
+	  .reset = reset_midr_el1, .val = GENMASK_ULL(31, 0) },
 	{ SYS_DESC(SYS_MPIDR_EL1), NULL, reset_mpidr, MPIDR_EL1 },
 
 	/*
@@ -4594,13 +4646,11 @@ id_to_sys_reg_desc(struct kvm_vcpu *vcpu, u64 id,
 		return ((struct sys_reg_desc *)r)->val;			\
 	}
 
-FUNCTION_INVARIANT(midr_el1)
 FUNCTION_INVARIANT(revidr_el1)
 FUNCTION_INVARIANT(aidr_el1)
 
 /* ->val is filled in by kvm_sys_reg_table_init() */
 static struct sys_reg_desc invariant_sys_regs[] __ro_after_init = {
-	{ SYS_DESC(SYS_MIDR_EL1), NULL, reset_midr_el1 },
 	{ SYS_DESC(SYS_REVIDR_EL1), NULL, reset_revidr_el1 },
 	{ SYS_DESC(SYS_AIDR_EL1), NULL, reset_aidr_el1 },
 };
-- 
2.42.0

[PATCH v2 2/4] KVM: arm64: Allow userspace to change REVIDR_EL1

[Sebastian Ott]

Enable VMMs to write REVIDR_EL1 by treating it as a VM ID register.
Trap guest access of REVIDR_EL1 when the VMs value differs from hardware.

Signed-off-by: Sebastian Ott <sebott@redhat.com>
---
 arch/arm64/include/asm/kvm_host.h |  3 +++
 arch/arm64/kvm/sys_regs.c         | 11 ++++++++---
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 3db8c773339e..c8fba4111b77 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -374,6 +374,7 @@ struct kvm_arch {
 	u64 id_regs[KVM_ARM_ID_REG_NUM];
 
 	u64 midr_el1;
+	u64 revidr_el1;
 	u64 ctr_el0;
 
 	/* Masks for VNCR-backed and general EL2 sysregs */
@@ -1472,6 +1473,8 @@ static inline u64 *__vm_id_reg(struct kvm_arch *ka, u32 reg)
 		return &ka->id_regs[IDREG_IDX(reg)];
 	case SYS_MIDR_EL1:
 		return &ka->midr_el1;
+	case SYS_REVIDR_EL1:
+		return &ka->revidr_el1;
 	case SYS_CTR_EL0:
 		return &ka->ctr_el0;
 	default:
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 7e1c9884f62a..646c0a04e58a 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1666,7 +1666,8 @@ static bool is_feature_id_reg(u32 encoding)
  */
 static inline bool is_vm_ftr_id_reg(u32 id)
 {
-	if (id == SYS_CTR_EL0 || id == SYS_MIDR_EL1)
+	if (id == SYS_CTR_EL0 || id == SYS_MIDR_EL1 ||
+	    id == SYS_REVIDR_EL1)
 		return true;
 
 	return (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 &&
@@ -2542,6 +2543,7 @@ static bool access_mdcr(struct kvm_vcpu *vcpu,
 	}
 
 FUNCTION_RESET(midr_el1)
+FUNCTION_RESET(revidr_el1)
 
 
 /*
@@ -2595,6 +2597,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	{ ID_DESC(MIDR_EL1), .set_user = set_id_reg_non_ftr, .visibility = id_visibility,
 	  .reset = reset_midr_el1, .val = GENMASK_ULL(31, 0) },
 	{ SYS_DESC(SYS_MPIDR_EL1), NULL, reset_mpidr, MPIDR_EL1 },
+	{ ID_DESC(REVIDR_EL1), .set_user = set_id_reg_non_ftr, .visibility = id_visibility,
+	  .reset = reset_revidr_el1, .val = -1ULL },
 
 	/*
 	 * ID regs: all ID_SANITISED() entries here must have corresponding
@@ -4646,12 +4650,10 @@ id_to_sys_reg_desc(struct kvm_vcpu *vcpu, u64 id,
 		return ((struct sys_reg_desc *)r)->val;			\
 	}
 
-FUNCTION_INVARIANT(revidr_el1)
 FUNCTION_INVARIANT(aidr_el1)
 
 /* ->val is filled in by kvm_sys_reg_table_init() */
 static struct sys_reg_desc invariant_sys_regs[] __ro_after_init = {
-	{ SYS_DESC(SYS_REVIDR_EL1), NULL, reset_revidr_el1 },
 	{ SYS_DESC(SYS_AIDR_EL1), NULL, reset_aidr_el1 },
 };
 
@@ -4999,6 +5001,9 @@ static void vcpu_set_hcr(struct kvm_vcpu *vcpu)
 	else
 		vcpu->arch.hcr_el2 |= HCR_TID2;
 
+	if (kvm_read_vm_id_reg(kvm, SYS_REVIDR_EL1) != read_sysreg(REVIDR_EL1))
+		vcpu->arch.hcr_el2 |= HCR_TID1;
+
 	if (vcpu_el1_is_32bit(vcpu))
 		vcpu->arch.hcr_el2 &= ~HCR_RW;
 
-- 
2.42.0

[PATCH v2 3/4] KVM: arm64: Allow userspace to change AIDR_EL1

[Sebastian Ott]

Enable VMMs to write AIDR_EL1 by treating it as a VM ID register.
Trap guest access of AIDR_EL1 when the VMs value differs from hardware.

Since this was the last invariant register remove all the stuff
that was needed to handle these.

Signed-off-by: Sebastian Ott <sebott@redhat.com>
---
 arch/arm64/include/asm/kvm_host.h |  3 ++
 arch/arm64/kvm/sys_regs.c         | 90 +++----------------------------
 2 files changed, 10 insertions(+), 83 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index c8fba4111b77..de735e2ad9ce 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -375,6 +375,7 @@ struct kvm_arch {
 
 	u64 midr_el1;
 	u64 revidr_el1;
+	u64 aidr_el1;
 	u64 ctr_el0;
 
 	/* Masks for VNCR-backed and general EL2 sysregs */
@@ -1475,6 +1476,8 @@ static inline u64 *__vm_id_reg(struct kvm_arch *ka, u32 reg)
 		return &ka->midr_el1;
 	case SYS_REVIDR_EL1:
 		return &ka->revidr_el1;
+	case SYS_AIDR_EL1:
+		return &ka->aidr_el1;
 	case SYS_CTR_EL0:
 		return &ka->ctr_el0;
 	default:
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 646c0a04e58a..d388594d7b28 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1667,7 +1667,7 @@ static bool is_feature_id_reg(u32 encoding)
 static inline bool is_vm_ftr_id_reg(u32 id)
 {
 	if (id == SYS_CTR_EL0 || id == SYS_MIDR_EL1 ||
-	    id == SYS_REVIDR_EL1)
+	    id == SYS_REVIDR_EL1 || id == SYS_AIDR_EL1)
 		return true;
 
 	return (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 &&
@@ -2544,6 +2544,7 @@ static bool access_mdcr(struct kvm_vcpu *vcpu,
 
 FUNCTION_RESET(midr_el1)
 FUNCTION_RESET(revidr_el1)
+FUNCTION_RESET(aidr_el1)
 
 
 /*
@@ -2870,6 +2871,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	  .set_user = set_clidr, .val = ~CLIDR_EL1_RES0 },
 	{ SYS_DESC(SYS_CCSIDR2_EL1), undef_access },
 	{ SYS_DESC(SYS_SMIDR_EL1), undef_access },
+	{ ID_DESC(AIDR_EL1), .set_user = set_id_reg_non_ftr, .visibility = id_visibility,
+	  .reset = reset_aidr_el1, .val = -1ULL },
 	{ SYS_DESC(SYS_CSSELR_EL1), access_csselr, reset_unknown, CSSELR_EL1 },
 	ID_FILTERED(CTR_EL0, ctr_el0,
 		    CTR_EL0_DIC_MASK |
@@ -4634,61 +4637,6 @@ id_to_sys_reg_desc(struct kvm_vcpu *vcpu, u64 id,
 	return r;
 }
 
-/*
- * These are the invariant sys_reg registers: we let the guest see the
- * host versions of these, so they're part of the guest state.
- *
- * A future CPU may provide a mechanism to present different values to
- * the guest, or a future kvm may trap them.
- */
-
-#define FUNCTION_INVARIANT(reg)						\
-	static u64 reset_##reg(struct kvm_vcpu *v,			\
-			       const struct sys_reg_desc *r)		\
-	{								\
-		((struct sys_reg_desc *)r)->val = read_sysreg(reg);	\
-		return ((struct sys_reg_desc *)r)->val;			\
-	}
-
-FUNCTION_INVARIANT(aidr_el1)
-
-/* ->val is filled in by kvm_sys_reg_table_init() */
-static struct sys_reg_desc invariant_sys_regs[] __ro_after_init = {
-	{ SYS_DESC(SYS_AIDR_EL1), NULL, reset_aidr_el1 },
-};
-
-static int get_invariant_sys_reg(u64 id, u64 __user *uaddr)
-{
-	const struct sys_reg_desc *r;
-
-	r = get_reg_by_id(id, invariant_sys_regs,
-			  ARRAY_SIZE(invariant_sys_regs));
-	if (!r)
-		return -ENOENT;
-
-	return put_user(r->val, uaddr);
-}
-
-static int set_invariant_sys_reg(u64 id, u64 __user *uaddr)
-{
-	const struct sys_reg_desc *r;
-	u64 val;
-
-	r = get_reg_by_id(id, invariant_sys_regs,
-			  ARRAY_SIZE(invariant_sys_regs));
-	if (!r)
-		return -ENOENT;
-
-	if (get_user(val, uaddr))
-		return -EFAULT;
-
-	/* This is what we mean by invariant: you can't change it. */
-	if (r->val != val)
-		return -EINVAL;
-
-	return 0;
-}
-
 static int demux_c15_get(struct kvm_vcpu *vcpu, u64 id, void __user *uaddr)
 {
 	u32 val;
@@ -4770,15 +4718,10 @@ int kvm_sys_reg_get_user(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg,
 int kvm_arm_sys_reg_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	void __user *uaddr = (void __user *)(unsigned long)reg->addr;
-	int err;
 
 	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_DEMUX)
 		return demux_c15_get(vcpu, reg->id, uaddr);
 
-	err = get_invariant_sys_reg(reg->id, uaddr);
-	if (err != -ENOENT)
-		return err;
-
 	return kvm_sys_reg_get_user(vcpu, reg,
 				    sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
 }
@@ -4814,15 +4757,10 @@ int kvm_sys_reg_set_user(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg,
 int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	void __user *uaddr = (void __user *)(unsigned long)reg->addr;
-	int err;
 
 	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_DEMUX)
 		return demux_c15_set(vcpu, reg->id, uaddr);
 
-	err = set_invariant_sys_reg(reg->id, uaddr);
-	if (err != -ENOENT)
-		return err;
-
 	return kvm_sys_reg_set_user(vcpu, reg,
 				    sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
 }
@@ -4911,23 +4849,13 @@ static int walk_sys_regs(struct kvm_vcpu *vcpu, u64 __user *uind)
 
 unsigned long kvm_arm_num_sys_reg_descs(struct kvm_vcpu *vcpu)
 {
-	return ARRAY_SIZE(invariant_sys_regs)
-		+ num_demux_regs()
-		+ walk_sys_regs(vcpu, (u64 __user *)NULL);
+	return num_demux_regs()	+ walk_sys_regs(vcpu, (u64 __user *)NULL);
 }
 
 int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
 {
-	unsigned int i;
 	int err;
 
-	/* Then give them all the invariant registers' indices. */
-	for (i = 0; i < ARRAY_SIZE(invariant_sys_regs); i++) {
-		if (put_user(sys_reg_to_index(&invariant_sys_regs[i]), uindices))
-			return -EFAULT;
-		uindices++;
-	}
-
 	err = walk_sys_regs(vcpu, uindices);
 	if (err < 0)
 		return err;
@@ -5001,7 +4929,8 @@ static void vcpu_set_hcr(struct kvm_vcpu *vcpu)
 	else
 		vcpu->arch.hcr_el2 |= HCR_TID2;
 
-	if (kvm_read_vm_id_reg(kvm, SYS_REVIDR_EL1) != read_sysreg(REVIDR_EL1))
+	if ((kvm_read_vm_id_reg(kvm, SYS_REVIDR_EL1) != read_sysreg(REVIDR_EL1)) ||
+	    (kvm_read_vm_id_reg(kvm, SYS_AIDR_EL1) != read_sysreg(AIDR_EL1)))
 		vcpu->arch.hcr_el2 |= HCR_TID1;
 
 	if (vcpu_el1_is_32bit(vcpu))
@@ -5156,16 +5085,11 @@ int __init kvm_sys_reg_table_init(void)
 	valid &= check_sysreg_table(cp14_64_regs, ARRAY_SIZE(cp14_64_regs), true);
 	valid &= check_sysreg_table(cp15_regs, ARRAY_SIZE(cp15_regs), true);
 	valid &= check_sysreg_table(cp15_64_regs, ARRAY_SIZE(cp15_64_regs), true);
-	valid &= check_sysreg_table(invariant_sys_regs, ARRAY_SIZE(invariant_sys_regs), false);
 	valid &= check_sysreg_table(sys_insn_descs, ARRAY_SIZE(sys_insn_descs), false);
 
 	if (!valid)
 		return -EINVAL;
 
-	/* We abuse the reset function to overwrite the table itself. */
-	for (i = 0; i < ARRAY_SIZE(invariant_sys_regs); i++)
-		invariant_sys_regs[i].reset(NULL, &invariant_sys_regs[i]);
-
 	ret = populate_nv_trap_config();
 
 	for (i = 0; !ret && i < ARRAY_SIZE(sys_reg_descs); i++)
-- 
2.42.0

[PATCH v2 4/4] KVM: selftests: arm64: Test writes to MIDR,REVIDR,AIDR

[Sebastian Ott]

Assert that MIDR_EL1, REVIDR_EL1, AIDR_EL1 are writable from userspace,
that the changed values are visible to guests, and that they are
preserved across a vCPU reset.

Signed-off-by: Sebastian Ott <sebott@redhat.com>
---
 .../testing/selftests/kvm/arm64/set_id_regs.c | 32 +++++++++++++++----
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/kvm/arm64/set_id_regs.c b/tools/testing/selftests/kvm/arm64/set_id_regs.c
index 217541fe6536..d719c2ab1e31 100644
--- a/tools/testing/selftests/kvm/arm64/set_id_regs.c
+++ b/tools/testing/selftests/kvm/arm64/set_id_regs.c
@@ -230,6 +230,9 @@ static void guest_code(void)
 	GUEST_REG_SYNC(SYS_ID_AA64MMFR2_EL1);
 	GUEST_REG_SYNC(SYS_ID_AA64ZFR0_EL1);
 	GUEST_REG_SYNC(SYS_CTR_EL0);
+	GUEST_REG_SYNC(SYS_MIDR_EL1);
+	GUEST_REG_SYNC(SYS_REVIDR_EL1);
+	GUEST_REG_SYNC(SYS_AIDR_EL1);
 
 	GUEST_DONE();
 }
@@ -609,18 +612,31 @@ static void test_ctr(struct kvm_vcpu *vcpu)
 	test_reg_vals[encoding_to_range_idx(SYS_CTR_EL0)] = ctr;
 }
 
-static void test_vcpu_ftr_id_regs(struct kvm_vcpu *vcpu)
+static void test_id_reg(struct kvm_vcpu *vcpu, u32 id)
 {
 	u64 val;
 
+	val = vcpu_get_reg(vcpu, KVM_ARM64_SYS_REG(id));
+	val++;
+	vcpu_set_reg(vcpu, KVM_ARM64_SYS_REG(id), val);
+	test_reg_vals[encoding_to_range_idx(id)] = val;
+}
+
+static void test_vcpu_ftr_id_regs(struct kvm_vcpu *vcpu)
+{
 	test_clidr(vcpu);
 	test_ctr(vcpu);
 
-	val = vcpu_get_reg(vcpu, KVM_ARM64_SYS_REG(SYS_MPIDR_EL1));
-	val++;
-	vcpu_set_reg(vcpu, KVM_ARM64_SYS_REG(SYS_MPIDR_EL1), val);
+	test_id_reg(vcpu, SYS_MPIDR_EL1);
+	ksft_test_result_pass("%s\n", __func__);
+}
+
+static void test_vcpu_non_ftr_id_regs(struct kvm_vcpu *vcpu)
+{
+	test_id_reg(vcpu, SYS_MIDR_EL1);
+	test_id_reg(vcpu, SYS_REVIDR_EL1);
+	test_id_reg(vcpu, SYS_AIDR_EL1);
 
-	test_reg_vals[encoding_to_range_idx(SYS_MPIDR_EL1)] = val;
 	ksft_test_result_pass("%s\n", __func__);
 }
 
@@ -647,6 +663,9 @@ static void test_reset_preserves_id_regs(struct kvm_vcpu *vcpu)
 	test_assert_id_reg_unchanged(vcpu, SYS_MPIDR_EL1);
 	test_assert_id_reg_unchanged(vcpu, SYS_CLIDR_EL1);
 	test_assert_id_reg_unchanged(vcpu, SYS_CTR_EL0);
+	test_assert_id_reg_unchanged(vcpu, SYS_MIDR_EL1);
+	test_assert_id_reg_unchanged(vcpu, SYS_REVIDR_EL1);
+	test_assert_id_reg_unchanged(vcpu, SYS_AIDR_EL1);
 
 	ksft_test_result_pass("%s\n", __func__);
 }
@@ -675,13 +694,14 @@ int main(void)
 		   ARRAY_SIZE(ftr_id_aa64isar2_el1) + ARRAY_SIZE(ftr_id_aa64pfr0_el1) +
 		   ARRAY_SIZE(ftr_id_aa64pfr1_el1) + ARRAY_SIZE(ftr_id_aa64mmfr0_el1) +
 		   ARRAY_SIZE(ftr_id_aa64mmfr1_el1) + ARRAY_SIZE(ftr_id_aa64mmfr2_el1) +
-		   ARRAY_SIZE(ftr_id_aa64zfr0_el1) - ARRAY_SIZE(test_regs) + 2 +
+		   ARRAY_SIZE(ftr_id_aa64zfr0_el1) - ARRAY_SIZE(test_regs) + 3 +
 		   MPAM_IDREG_TEST;
 
 	ksft_set_plan(test_cnt);
 
 	test_vm_ftr_id_regs(vcpu, aarch64_only);
 	test_vcpu_ftr_id_regs(vcpu);
+	test_vcpu_non_ftr_id_regs(vcpu);
 	test_user_set_mpam_reg(vcpu);
 
 	test_guest_reg_read(vcpu);
-- 
2.42.0

Re: [PATCH v2 1/4] KVM: arm64: Allow userspace to change MIDR_EL1

[Oliver Upton]

Hi Sebastian,

On Tue, Feb 11, 2025 at 03:39:07PM +0100, Sebastian Ott wrote:
> +static int set_id_reg_non_ftr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> +			      u64 val)
> +{
> +	u32 id = reg_to_encoding(rd);
> +	int ret;
> +
> +	mutex_lock(&vcpu->kvm->arch.config_lock);

There's quite a few early outs, guard() might be a better fit than
explicitly dropping the lock.

> +	/*
> +	 * Since guest access to MIDR_EL1 is not trapped
> +	 * set up VPIDR_EL2 to hold the MIDR_EL1 value.
> +	 */
> +	if (id == SYS_MIDR_EL1)
> +		write_sysreg(val, vpidr_el2);

This is problematic for a couple reasons:

 - If the kernel isn't running at EL2, VPIDR_EL2 is undefined

 - VPIDR_EL2 needs to be handled as part of the vCPU context, not
   written to without a running vCPU. What would happen if two vCPUs
   have different MIDR values?

Here's a new diff with some hacks thrown in to handle VPIDR_EL2
correctly. Very lightly tested :)

Thanks,
Oliver
---
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 7cfa024de4e3..3db8c773339e 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -373,6 +373,7 @@ struct kvm_arch {
 #define KVM_ARM_ID_REG_NUM	(IDREG_IDX(sys_reg(3, 0, 0, 7, 7)) + 1)
 	u64 id_regs[KVM_ARM_ID_REG_NUM];
 
+	u64 midr_el1;
 	u64 ctr_el0;
 
 	/* Masks for VNCR-backed and general EL2 sysregs */
@@ -1469,6 +1470,8 @@ static inline u64 *__vm_id_reg(struct kvm_arch *ka, u32 reg)
 	switch (reg) {
 	case sys_reg(3, 0, 0, 1, 0) ... sys_reg(3, 0, 0, 7, 7):
 		return &ka->id_regs[IDREG_IDX(reg)];
+	case SYS_MIDR_EL1:
+		return &ka->midr_el1;
 	case SYS_CTR_EL0:
 		return &ka->ctr_el0;
 	default:
diff --git a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
index 76ff095c6b6e..866411621a39 100644
--- a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
+++ b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
@@ -168,9 +168,11 @@ static inline void __sysreg_restore_user_state(struct kvm_cpu_context *ctxt)
 }
 
 static inline void __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt,
-					      u64 mpidr)
+					      u64 midr, u64 mpidr)
 {
-	write_sysreg(mpidr,				vmpidr_el2);
+	write_sysreg(midr, vpidr_el2);
+	write_sysreg(mpidr, vmpidr_el2);
+
 
 	if (has_vhe() ||
 	    !cpus_have_final_cap(ARM64_WORKAROUND_SPECULATIVE_AT)) {
diff --git a/arch/arm64/kvm/hyp/nvhe/sysreg-sr.c b/arch/arm64/kvm/hyp/nvhe/sysreg-sr.c
index dba101565de3..a01be1add5ad 100644
--- a/arch/arm64/kvm/hyp/nvhe/sysreg-sr.c
+++ b/arch/arm64/kvm/hyp/nvhe/sysreg-sr.c
@@ -28,7 +28,15 @@ void __sysreg_save_state_nvhe(struct kvm_cpu_context *ctxt)
 
 void __sysreg_restore_state_nvhe(struct kvm_cpu_context *ctxt)
 {
-	__sysreg_restore_el1_state(ctxt, ctxt_sys_reg(ctxt, MPIDR_EL1));
+	u64 midr;
+
+	if (ctxt_is_guest(ctxt))
+		midr = kvm_read_vm_id_reg(kern_hyp_va(ctxt_to_vcpu(ctxt)->kvm),
+					  SYS_MIDR_EL1);
+	else
+		midr = read_cpuid_id();
+
+	__sysreg_restore_el1_state(ctxt, midr, ctxt_sys_reg(ctxt, MPIDR_EL1));
 	__sysreg_restore_common_state(ctxt);
 	__sysreg_restore_user_state(ctxt);
 	__sysreg_restore_el2_return_state(ctxt);
diff --git a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
index 90b018e06f2c..1d4b9597eb29 100644
--- a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
+++ b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
@@ -87,11 +87,12 @@ static void __sysreg_restore_vel2_state(struct kvm_vcpu *vcpu)
 	write_sysreg(__vcpu_sys_reg(vcpu, PAR_EL1),	par_el1);
 	write_sysreg(__vcpu_sys_reg(vcpu, TPIDR_EL1),	tpidr_el1);
 
-	write_sysreg(__vcpu_sys_reg(vcpu, MPIDR_EL1),		vmpidr_el2);
-	write_sysreg_el1(__vcpu_sys_reg(vcpu, MAIR_EL2),	SYS_MAIR);
-	write_sysreg_el1(__vcpu_sys_reg(vcpu, VBAR_EL2),	SYS_VBAR);
-	write_sysreg_el1(__vcpu_sys_reg(vcpu, CONTEXTIDR_EL2),	SYS_CONTEXTIDR);
-	write_sysreg_el1(__vcpu_sys_reg(vcpu, AMAIR_EL2),	SYS_AMAIR);
+	write_sysreg(kvm_read_vm_id_reg(vcpu->kvm, SYS_MIDR_EL1),	vpidr_el2);
+	write_sysreg(__vcpu_sys_reg(vcpu, MPIDR_EL1),			vmpidr_el2);
+	write_sysreg_el1(__vcpu_sys_reg(vcpu, MAIR_EL2),		SYS_MAIR);
+	write_sysreg_el1(__vcpu_sys_reg(vcpu, VBAR_EL2),		SYS_VBAR);
+	write_sysreg_el1(__vcpu_sys_reg(vcpu, CONTEXTIDR_EL2),		SYS_CONTEXTIDR);
+	write_sysreg_el1(__vcpu_sys_reg(vcpu, AMAIR_EL2),		SYS_AMAIR);
 
 	if (vcpu_el2_e2h_is_set(vcpu)) {
 		/*
@@ -191,7 +192,7 @@ void __vcpu_load_switch_sysregs(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpu_context *guest_ctxt = &vcpu->arch.ctxt;
 	struct kvm_cpu_context *host_ctxt;
-	u64 mpidr;
+	u64 midr, mpidr;
 
 	host_ctxt = host_data_ptr(host_ctxt);
 	__sysreg_save_user_state(host_ctxt);
@@ -222,10 +223,9 @@ void __vcpu_load_switch_sysregs(struct kvm_vcpu *vcpu)
 		if (vcpu_has_nv(vcpu)) {
 			/*
 			 * Use the guest hypervisor's VPIDR_EL2 when in a
-			 * nested state. The hardware value of MIDR_EL1 gets
-			 * restored on put.
+			 * nested state.
 			 */
-			write_sysreg(ctxt_sys_reg(guest_ctxt, VPIDR_EL2), vpidr_el2);
+			midr = ctxt_sys_reg(guest_ctxt, VPIDR_EL2);
 
 			/*
 			 * As we're restoring a nested guest, set the value
@@ -233,10 +233,11 @@ void __vcpu_load_switch_sysregs(struct kvm_vcpu *vcpu)
 			 */
 			mpidr = ctxt_sys_reg(guest_ctxt, VMPIDR_EL2);
 		} else {
+			midr = kvm_read_vm_id_reg(vcpu->kvm, SYS_MIDR_EL1);
 			mpidr = ctxt_sys_reg(guest_ctxt, MPIDR_EL1);
 		}
 
-		__sysreg_restore_el1_state(guest_ctxt, mpidr);
+		__sysreg_restore_el1_state(guest_ctxt, midr, mpidr);
 	}
 
 	vcpu_set_flag(vcpu, SYSREGS_ON_CPU);
@@ -271,9 +272,5 @@ void __vcpu_put_switch_sysregs(struct kvm_vcpu *vcpu)
 	/* Restore host user state */
 	__sysreg_restore_user_state(host_ctxt);
 
-	/* If leaving a nesting guest, restore MIDR_EL1 default view */
-	if (vcpu_has_nv(vcpu))
-		write_sysreg(read_cpuid_id(),	vpidr_el2);
-
 	vcpu_clear_flag(vcpu, SYSREGS_ON_CPU);
 }
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index f6cd1ea7fb55..aa1a0443dc6a 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1656,7 +1656,7 @@ static bool is_feature_id_reg(u32 encoding)
  */
 static inline bool is_vm_ftr_id_reg(u32 id)
 {
-	if (id == SYS_CTR_EL0)
+	if (id == SYS_CTR_EL0 || id == SYS_MIDR_EL1)
 		return true;
 
 	return (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 &&
@@ -1989,6 +1989,34 @@ static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 	return 0;
 }
 
+static int set_id_reg_non_ftr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
+			      u64 val)
+{
+	u32 id = reg_to_encoding(rd);
+
+	guard(mutex)(&vcpu->kvm->arch.config_lock);
+
+	/*
+	 * Once the VM has started the ID registers are immutable. Reject any
+	 * write that does not match the final register value.
+	 */
+	if (kvm_vm_has_ran_once(vcpu->kvm)) {
+		if (val != read_id_reg(vcpu, rd))
+			return -EBUSY;
+
+		return 0;
+	}
+
+	/*
+	 * For non ftr regs do a limited test against the writable mask only.
+	 */
+	if ((rd->val & val) != val)
+		return -EINVAL;
+
+	kvm_set_vm_id_reg(vcpu->kvm, id, val);
+	return 0;
+}
+
 static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 		      u64 val)
 {
@@ -2483,6 +2511,15 @@ static bool access_mdcr(struct kvm_vcpu *vcpu,
 	return true;
 }
 
+#define FUNCTION_RESET(reg)						\
+	static u64 reset_##reg(struct kvm_vcpu *v,			\
+			       const struct sys_reg_desc *r)		\
+	{								\
+		return read_sysreg(reg);				\
+	}
+
+FUNCTION_RESET(midr_el1)
+
 
 /*
  * Architected system registers.
@@ -2532,6 +2569,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 
 	{ SYS_DESC(SYS_DBGVCR32_EL2), undef_access, reset_val, DBGVCR32_EL2, 0 },
 
+	{ ID_DESC(MIDR_EL1), .set_user = set_id_reg_non_ftr, .visibility = id_visibility,
+	  .reset = reset_midr_el1, .val = GENMASK_ULL(31, 0) },
 	{ SYS_DESC(SYS_MPIDR_EL1), NULL, reset_mpidr, MPIDR_EL1 },
 
 	/*
@@ -4584,13 +4623,11 @@ id_to_sys_reg_desc(struct kvm_vcpu *vcpu, u64 id,
 		return ((struct sys_reg_desc *)r)->val;			\
 	}
 
-FUNCTION_INVARIANT(midr_el1)
 FUNCTION_INVARIANT(revidr_el1)
 FUNCTION_INVARIANT(aidr_el1)
 
 /* ->val is filled in by kvm_sys_reg_table_init() */
 static struct sys_reg_desc invariant_sys_regs[] __ro_after_init = {
-	{ SYS_DESC(SYS_MIDR_EL1), NULL, reset_midr_el1 },
 	{ SYS_DESC(SYS_REVIDR_EL1), NULL, reset_revidr_el1 },
 	{ SYS_DESC(SYS_AIDR_EL1), NULL, reset_aidr_el1 },
 };

Re: [PATCH v2 1/4] KVM: arm64: Allow userspace to change MIDR_EL1

[罗勇刚(Yonggang Luo)]

On Sat, Feb 15, 2025 at 6:15 PM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> Hi Sebastian,
>
> On Tue, Feb 11, 2025 at 03:39:07PM +0100, Sebastian Ott wrote:
> > +static int set_id_reg_non_ftr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> > +                           u64 val)
> > +{
> > +     u32 id = reg_to_encoding(rd);
> > +     int ret;
> > +
> > +     mutex_lock(&vcpu->kvm->arch.config_lock);
>
> There's quite a few early outs, guard() might be a better fit than
> explicitly dropping the lock.
>
> > +     /*
> > +      * Since guest access to MIDR_EL1 is not trapped
> > +      * set up VPIDR_EL2 to hold the MIDR_EL1 value.
> > +      */
> > +     if (id == SYS_MIDR_EL1)
> > +             write_sysreg(val, vpidr_el2);
>
> This is problematic for a couple reasons:
>
>  - If the kernel isn't running at EL2, VPIDR_EL2 is undefined
>
>  - VPIDR_EL2 needs to be handled as part of the vCPU context, not
>    written to without a running vCPU. What would happen if two vCPUs
>    have different MIDR values?
>
> Here's a new diff with some hacks thrown in to handle VPIDR_EL2
> correctly. Very lightly tested :)

Thans, I am also faced this issue, but other than this, I am also
facing a issue, after updating
MIDR_EL1, The CP15 register MIDR for aarch32 not updated.
The instruction is `MRC p15,0,<Rt>,c0,c0,0    ; Read CP15 Main ID Register` from
https://developer.arm.com/documentation/ddi0406/b/System-Level-Architecture/Protected-Memory-System-Architecture--PMSA-/CP15-registers-for-a-PMSA-implementation/c0--Main-ID-Register--MIDR-

The value of this instruction is not updated



>
> Thanks,
> Oliver
> ---
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 7cfa024de4e3..3db8c773339e 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -373,6 +373,7 @@ struct kvm_arch {
>  #define KVM_ARM_ID_REG_NUM     (IDREG_IDX(sys_reg(3, 0, 0, 7, 7)) + 1)
>         u64 id_regs[KVM_ARM_ID_REG_NUM];
>
> +       u64 midr_el1;
>         u64 ctr_el0;
>
>         /* Masks for VNCR-backed and general EL2 sysregs */
> @@ -1469,6 +1470,8 @@ static inline u64 *__vm_id_reg(struct kvm_arch *ka, u32 reg)
>         switch (reg) {
>         case sys_reg(3, 0, 0, 1, 0) ... sys_reg(3, 0, 0, 7, 7):
>                 return &ka->id_regs[IDREG_IDX(reg)];
> +       case SYS_MIDR_EL1:
> +               return &ka->midr_el1;
>         case SYS_CTR_EL0:
>                 return &ka->ctr_el0;
>         default:
> diff --git a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> index 76ff095c6b6e..866411621a39 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> @@ -168,9 +168,11 @@ static inline void __sysreg_restore_user_state(struct kvm_cpu_context *ctxt)
>  }
>
>  static inline void __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt,
> -                                             u64 mpidr)
> +                                             u64 midr, u64 mpidr)
>  {
> -       write_sysreg(mpidr,                             vmpidr_el2);
> +       write_sysreg(midr, vpidr_el2);
> +       write_sysreg(mpidr, vmpidr_el2);
> +
>
>         if (has_vhe() ||
>             !cpus_have_final_cap(ARM64_WORKAROUND_SPECULATIVE_AT)) {
> diff --git a/arch/arm64/kvm/hyp/nvhe/sysreg-sr.c b/arch/arm64/kvm/hyp/nvhe/sysreg-sr.c
> index dba101565de3..a01be1add5ad 100644
> --- a/arch/arm64/kvm/hyp/nvhe/sysreg-sr.c
> +++ b/arch/arm64/kvm/hyp/nvhe/sysreg-sr.c
> @@ -28,7 +28,15 @@ void __sysreg_save_state_nvhe(struct kvm_cpu_context *ctxt)
>
>  void __sysreg_restore_state_nvhe(struct kvm_cpu_context *ctxt)
>  {
> -       __sysreg_restore_el1_state(ctxt, ctxt_sys_reg(ctxt, MPIDR_EL1));
> +       u64 midr;
> +
> +       if (ctxt_is_guest(ctxt))
> +               midr = kvm_read_vm_id_reg(kern_hyp_va(ctxt_to_vcpu(ctxt)->kvm),
> +                                         SYS_MIDR_EL1);
> +       else
> +               midr = read_cpuid_id();
> +
> +       __sysreg_restore_el1_state(ctxt, midr, ctxt_sys_reg(ctxt, MPIDR_EL1));
>         __sysreg_restore_common_state(ctxt);
>         __sysreg_restore_user_state(ctxt);
>         __sysreg_restore_el2_return_state(ctxt);
> diff --git a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> index 90b018e06f2c..1d4b9597eb29 100644
> --- a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> +++ b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> @@ -87,11 +87,12 @@ static void __sysreg_restore_vel2_state(struct kvm_vcpu *vcpu)
>         write_sysreg(__vcpu_sys_reg(vcpu, PAR_EL1),     par_el1);
>         write_sysreg(__vcpu_sys_reg(vcpu, TPIDR_EL1),   tpidr_el1);
>
> -       write_sysreg(__vcpu_sys_reg(vcpu, MPIDR_EL1),           vmpidr_el2);
> -       write_sysreg_el1(__vcpu_sys_reg(vcpu, MAIR_EL2),        SYS_MAIR);
> -       write_sysreg_el1(__vcpu_sys_reg(vcpu, VBAR_EL2),        SYS_VBAR);
> -       write_sysreg_el1(__vcpu_sys_reg(vcpu, CONTEXTIDR_EL2),  SYS_CONTEXTIDR);
> -       write_sysreg_el1(__vcpu_sys_reg(vcpu, AMAIR_EL2),       SYS_AMAIR);
> +       write_sysreg(kvm_read_vm_id_reg(vcpu->kvm, SYS_MIDR_EL1),       vpidr_el2);
> +       write_sysreg(__vcpu_sys_reg(vcpu, MPIDR_EL1),                   vmpidr_el2);
> +       write_sysreg_el1(__vcpu_sys_reg(vcpu, MAIR_EL2),                SYS_MAIR);
> +       write_sysreg_el1(__vcpu_sys_reg(vcpu, VBAR_EL2),                SYS_VBAR);
> +       write_sysreg_el1(__vcpu_sys_reg(vcpu, CONTEXTIDR_EL2),          SYS_CONTEXTIDR);
> +       write_sysreg_el1(__vcpu_sys_reg(vcpu, AMAIR_EL2),               SYS_AMAIR);
>
>         if (vcpu_el2_e2h_is_set(vcpu)) {
>                 /*
> @@ -191,7 +192,7 @@ void __vcpu_load_switch_sysregs(struct kvm_vcpu *vcpu)
>  {
>         struct kvm_cpu_context *guest_ctxt = &vcpu->arch.ctxt;
>         struct kvm_cpu_context *host_ctxt;
> -       u64 mpidr;
> +       u64 midr, mpidr;
>
>         host_ctxt = host_data_ptr(host_ctxt);
>         __sysreg_save_user_state(host_ctxt);
> @@ -222,10 +223,9 @@ void __vcpu_load_switch_sysregs(struct kvm_vcpu *vcpu)
>                 if (vcpu_has_nv(vcpu)) {
>                         /*
>                          * Use the guest hypervisor's VPIDR_EL2 when in a
> -                        * nested state. The hardware value of MIDR_EL1 gets
> -                        * restored on put.
> +                        * nested state.
>                          */
> -                       write_sysreg(ctxt_sys_reg(guest_ctxt, VPIDR_EL2), vpidr_el2);
> +                       midr = ctxt_sys_reg(guest_ctxt, VPIDR_EL2);
>
>                         /*
>                          * As we're restoring a nested guest, set the value
> @@ -233,10 +233,11 @@ void __vcpu_load_switch_sysregs(struct kvm_vcpu *vcpu)
>                          */
>                         mpidr = ctxt_sys_reg(guest_ctxt, VMPIDR_EL2);
>                 } else {
> +                       midr = kvm_read_vm_id_reg(vcpu->kvm, SYS_MIDR_EL1);
>                         mpidr = ctxt_sys_reg(guest_ctxt, MPIDR_EL1);
>                 }
>
> -               __sysreg_restore_el1_state(guest_ctxt, mpidr);
> +               __sysreg_restore_el1_state(guest_ctxt, midr, mpidr);
>         }
>
>         vcpu_set_flag(vcpu, SYSREGS_ON_CPU);
> @@ -271,9 +272,5 @@ void __vcpu_put_switch_sysregs(struct kvm_vcpu *vcpu)
>         /* Restore host user state */
>         __sysreg_restore_user_state(host_ctxt);
>
> -       /* If leaving a nesting guest, restore MIDR_EL1 default view */
> -       if (vcpu_has_nv(vcpu))
> -               write_sysreg(read_cpuid_id(),   vpidr_el2);
> -
>         vcpu_clear_flag(vcpu, SYSREGS_ON_CPU);
>  }
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index f6cd1ea7fb55..aa1a0443dc6a 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1656,7 +1656,7 @@ static bool is_feature_id_reg(u32 encoding)
>   */
>  static inline bool is_vm_ftr_id_reg(u32 id)
>  {
> -       if (id == SYS_CTR_EL0)
> +       if (id == SYS_CTR_EL0 || id == SYS_MIDR_EL1)
>                 return true;
>
>         return (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 &&
> @@ -1989,6 +1989,34 @@ static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>         return 0;
>  }
>
> +static int set_id_reg_non_ftr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> +                             u64 val)
> +{
> +       u32 id = reg_to_encoding(rd);
> +
> +       guard(mutex)(&vcpu->kvm->arch.config_lock);
> +
> +       /*
> +        * Once the VM has started the ID registers are immutable. Reject any
> +        * write that does not match the final register value.
> +        */
> +       if (kvm_vm_has_ran_once(vcpu->kvm)) {
> +               if (val != read_id_reg(vcpu, rd))
> +                       return -EBUSY;
> +
> +               return 0;
> +       }
> +
> +       /*
> +        * For non ftr regs do a limited test against the writable mask only.
> +        */
> +       if ((rd->val & val) != val)
> +               return -EINVAL;
> +
> +       kvm_set_vm_id_reg(vcpu->kvm, id, val);
> +       return 0;
> +}
> +
>  static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>                       u64 val)
>  {
> @@ -2483,6 +2511,15 @@ static bool access_mdcr(struct kvm_vcpu *vcpu,
>         return true;
>  }
>
> +#define FUNCTION_RESET(reg)                                            \
> +       static u64 reset_##reg(struct kvm_vcpu *v,                      \
> +                              const struct sys_reg_desc *r)            \
> +       {                                                               \
> +               return read_sysreg(reg);                                \
> +       }
> +
> +FUNCTION_RESET(midr_el1)
> +
>
>  /*
>   * Architected system registers.
> @@ -2532,6 +2569,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>
>         { SYS_DESC(SYS_DBGVCR32_EL2), undef_access, reset_val, DBGVCR32_EL2, 0 },
>
> +       { ID_DESC(MIDR_EL1), .set_user = set_id_reg_non_ftr, .visibility = id_visibility,
> +         .reset = reset_midr_el1, .val = GENMASK_ULL(31, 0) },
>         { SYS_DESC(SYS_MPIDR_EL1), NULL, reset_mpidr, MPIDR_EL1 },
>
>         /*
> @@ -4584,13 +4623,11 @@ id_to_sys_reg_desc(struct kvm_vcpu *vcpu, u64 id,
>                 return ((struct sys_reg_desc *)r)->val;                 \
>         }
>
> -FUNCTION_INVARIANT(midr_el1)
>  FUNCTION_INVARIANT(revidr_el1)
>  FUNCTION_INVARIANT(aidr_el1)
>
>  /* ->val is filled in by kvm_sys_reg_table_init() */
>  static struct sys_reg_desc invariant_sys_regs[] __ro_after_init = {
> -       { SYS_DESC(SYS_MIDR_EL1), NULL, reset_midr_el1 },
>         { SYS_DESC(SYS_REVIDR_EL1), NULL, reset_revidr_el1 },
>         { SYS_DESC(SYS_AIDR_EL1), NULL, reset_aidr_el1 },
>  };
>


-- 
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo

Re: [PATCH v2 1/4] KVM: arm64: Allow userspace to change MIDR_EL1

[Marc Zyngier]

On Sat, 15 Feb 2025 16:16:44 +0000,
"罗勇刚(Yonggang Luo)" <luoyonggang@gmail.com> wrote:
> 
> On Sat, Feb 15, 2025 at 6:15 PM Oliver Upton <oliver.upton@linux.dev> wrote:
> >
> > Hi Sebastian,
> >
> > On Tue, Feb 11, 2025 at 03:39:07PM +0100, Sebastian Ott wrote:
> > > +static int set_id_reg_non_ftr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> > > +                           u64 val)
> > > +{
> > > +     u32 id = reg_to_encoding(rd);
> > > +     int ret;
> > > +
> > > +     mutex_lock(&vcpu->kvm->arch.config_lock);
> >
> > There's quite a few early outs, guard() might be a better fit than
> > explicitly dropping the lock.
> >
> > > +     /*
> > > +      * Since guest access to MIDR_EL1 is not trapped
> > > +      * set up VPIDR_EL2 to hold the MIDR_EL1 value.
> > > +      */
> > > +     if (id == SYS_MIDR_EL1)
> > > +             write_sysreg(val, vpidr_el2);
> >
> > This is problematic for a couple reasons:
> >
> >  - If the kernel isn't running at EL2, VPIDR_EL2 is undefined
> >
> >  - VPIDR_EL2 needs to be handled as part of the vCPU context, not
> >    written to without a running vCPU. What would happen if two vCPUs
> >    have different MIDR values?
> >
> > Here's a new diff with some hacks thrown in to handle VPIDR_EL2
> > correctly. Very lightly tested :)
> 
> Thans, I am also faced this issue, but other than this, I am also
> facing a issue, after updating
> MIDR_EL1, The CP15 register MIDR for aarch32 not updated.
> The instruction is `MRC p15,0,<Rt>,c0,c0,0    ; Read CP15 Main ID Register` from
> https://developer.arm.com/documentation/ddi0406/b/System-Level-Architecture/Protected-Memory-System-Architecture--PMSA-/CP15-registers-for-a-PMSA-implementation/c0--Main-ID-Register--MIDR-
> 
> The value of this instruction is not updated

How do you determine that MIDR isn't updated? How do you update the
userspace view?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v2 1/4] KVM: arm64: Allow userspace to change MIDR_EL1

[罗勇刚(Yonggang Luo)]

>
> How do you determine that MIDR isn't updated? How do you update the
> userspace view?
>
> Thanks,
>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.

The testing code is at
https://github.com/lygstate/arm64-kvm-hello-world/tree/midr/bare-metal-aarch32-qemu
qemu changes:
```
From 543f2f656952ab01509025b79d0198736ef68231 Mon Sep 17 00:00:00 2001
From: Yonggang Luo <luoyonggang@gmail.com>
Date: Sun, 16 Feb 2025 02:57:44 +0800
Subject: [PATCH] Add support midr options for arm virt machine

---
 hw/arm/virt.c         | 20 ++++++++++++++++++++
 include/hw/arm/virt.h |  1 +
 target/arm/kvm.c      | 21 +++++++++++++++++++++
 3 files changed, 42 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 4a5a9666e9..5ba690a70d 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2236,6 +2236,10 @@ static void machvirt_init(MachineState *machine)
         object_property_set_int(cpuobj, "mp-affinity",
                                 possible_cpus->cpus[n].arch_id, NULL);

+        if (vms->midr) {
+            object_property_set_int(cpuobj, "midr", vms->midr, NULL);
+        }
+
         cs = CPU(cpuobj);
         cs->cpu_index = n;

@@ -3348,6 +3352,17 @@ static const TypeInfo virt_machine_info = {
     },
 };

+static char * virt_get_midr(Object *obj, Error **errp)
+{
+    VirtMachineState *vms = VIRT_MACHINE(obj);
+    return g_strdup_printf("0x%08x", vms->midr);
+}
+static void virt_set_midr(Object *obj, const char *value, Error **errp)
+{
+    VirtMachineState *vms = VIRT_MACHINE(obj);
+    vms->midr = strtoul(value, 0, 0);
+}
+
 static void machvirt_machine_init(void)
 {
     type_register_static(&virt_machine_info);
@@ -3356,6 +3371,11 @@ type_init(machvirt_machine_init);

 static void virt_machine_10_0_options(MachineClass *mc)
 {
+    ObjectClass *oc = &mc->parent_class;
+    object_class_property_add_str(oc, "midr", virt_get_midr,
+        virt_set_midr);
+    object_class_property_set_description(oc, "midr",
+                "Set MDIR value for VIRT machine");
 }
 DEFINE_VIRT_MACHINE_AS_LATEST(10, 0)

diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index c8e94e6aed..fe200b0d76 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -179,6 +179,7 @@ struct VirtMachineState {
     PCIBus *bus;
     char *oem_id;
     char *oem_table_id;
+    uint32_t midr;
     bool ns_el2_virt_timer_irq;
 };

diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index da30bdbb23..577eaee505 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -1873,6 +1873,7 @@ static int kvm_arm_sve_set_vls(ARMCPU *cpu)
 }

 #define ARM_CPU_ID_MPIDR       3, 0, 0, 0, 5
+#define ARM_CPU_ID_MIDR_EL1    3, 0, 0, 0, 0

 int kvm_arch_init_vcpu(CPUState *cs)
 {
@@ -1920,6 +1921,26 @@ int kvm_arch_init_vcpu(CPUState *cs)
         return ret;
     }

+    {
+        uint64_t midr = cpu->midr;
+        ret = kvm_get_one_reg(cs, ARM64_SYS_REG(ARM_CPU_ID_MIDR_EL1), &midr);
+        if (ret) {
+            return ret;
+        }
+        printf("Get MIDR EL1 origin:0x%08x\n", (uint32_t)midr);
+        midr = cpu->midr;
+        ret = kvm_set_one_reg(cs, ARM64_SYS_REG(ARM_CPU_ID_MIDR_EL1), &midr);
+        printf("Set MIDR EL1:0x%08x\n", (uint32_t)midr);
+        if (ret) {
+            return ret;
+        }
+        ret = kvm_get_one_reg(cs, ARM64_SYS_REG(ARM_CPU_ID_MIDR_EL1), &midr);
+        printf("Get MIDR EL1:0x%08x\n", (uint32_t)midr);
+        if (ret) {
+            return ret;
+        }
+    }
+
     if (cpu_isar_feature(aa64_sve, cpu)) {
         ret = kvm_arm_sve_set_vls(cpu);
         if (ret) {
-- 
2.47.1.windows.1

```
The TCG running result:
```
/home/lygstate/work/qemu/build:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
/home/lygstate/work/qemu/build/qemu-system-aarch64
+ qemu-system-aarch64 -cpu cortex-a15 -accel tcg,thread=multi -m 1024M
-smp 1 -M virt,gic-version=3,midr=0x412fd050 -nographic -monitor none
-serial stdio -kernel
/home/lygstate/work/debian/arm64-kvm-hello-world/bare-metal-aarch32-qemu/hello_world.elf
Hello World midr:0x412fd050
```
The KVM running result:
```
/home/lygstate/work/qemu/build:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
/home/lygstate/work/qemu/build/qemu-system-aarch64
+ qemu-system-aarch64 -cpu host,aarch64=off -accel kvm -m 1024M -smp 1
-M virt,gic-version=3,midr=0x412fd050 -nographic -monitor none -serial
stdio -kernel /home/lygstate/work/debian/arm64-kvm-hello-world/bare-metal-aarch32-qemu/hello_world.elf
Get MIDR EL1 origin:0x410fd083
Set MIDR EL1:0x412fd050
Get MIDR EL1:0x412fd050
Hello World midr:0x410fd083
```

According to this, the MIDR EL1 is updated properly, but the MIDR for
aarch32 is not updated, and I don't know how to hook the update for
MIDR for aarch32
--
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo

Re: [PATCH v2 1/4] KVM: arm64: Allow userspace to change MIDR_EL1

[Marc Zyngier]

On Sat, 15 Feb 2025 19:04:20 +0000,
"罗勇刚(Yonggang Luo)" <luoyonggang@gmail.com> wrote:
> 
> According to this, the MIDR EL1 is updated properly, but the MIDR for
> aarch32 is not updated, and I don't know how to hook the update for
> MIDR for aarch32

It is the same thing. The AArch32 view is configured the same way as
the AArch64 view, and there is nothing to do at all (that's what
VPIDR_EL2 is all about).

With Oliver's change, I'm correctly getting a different MIDR using a
hacked up kvmtool, see below. I suspect you're not running with the
correct patch applied.

	M.

* kvmtool hack:

diff --git a/arm/aarch64/kvm-cpu.c b/arm/aarch64/kvm-cpu.c
index c8be10b..f8ecbfe 100644
--- a/arm/aarch64/kvm-cpu.c
+++ b/arm/aarch64/kvm-cpu.c
@@ -128,6 +128,18 @@ static void reset_vcpu_aarch64(struct kvm_cpu *vcpu)
 	}
 }
 
+static void reset_vcpu_midr(struct kvm_cpu *vcpu)
+{
+	struct kvm_one_reg reg;
+	u64 midr = 0xbadf00d5;
+
+	reg.id = ARM64_SYS_REG(ARM_CPU_ID, 0);
+	reg.addr = (u64)&midr;
+
+	if (ioctl(vcpu->vcpu_fd, KVM_SET_ONE_REG, &reg) < 0)
+		die("KVM_SET_ONE_REG failed (set_midr vcpu%ld", vcpu->cpu_id);
+}
+
 void kvm_cpu__select_features(struct kvm *kvm, struct kvm_vcpu_init *init)
 {
 	if (kvm->cfg.arch.aarch32_guest) {
@@ -181,6 +193,8 @@ void kvm_cpu__reset_vcpu(struct kvm_cpu *vcpu)
 			die_perror("sched_setaffinity");
 	}
 
+	reset_vcpu_midr(vcpu);
+
 	if (kvm->cfg.arch.aarch32_guest)
 		return reset_vcpu_aarch32(vcpu);
 	else

* arm64 host:

$ cat /proc/cpuinfo 
processor	: 0
BogoMIPS	: 200.00
Features	: fp asimd evtstrm aes pmull sha1 sha2 crc32 cpuid
CPU implementer	: 0x41
CPU architecture: 8
CPU variant	: 0x0
CPU part	: 0xd03
CPU revision	: 4

* arm64 guest:

# cat /proc/cpuinfo
processor	: 0
BogoMIPS	: 200.00
Features	: fp asimd evtstrm aes pmull sha1 sha2 crc32 cpuid
CPU implementer	: 0xba
CPU architecture: 8
CPU variant	: 0xd
CPU part	: 0x00d
CPU revision	: 5

* arm32 guest:

# cat /proc/cpuinfo 
processor	: 0
model name	: ARMv7 Processor rev 5 (v7l)
BogoMIPS	: 200.00
Features	: half thumb fastmult vfp edsp neon vfpv3 tls vfpv4 idiva idivt vfpd32 lpae evtstrm aes pmull sha1 sha2 crc32 
CPU implementer	: 0xba
CPU architecture: 7
CPU variant	: 0xd
CPU part	: 0x00d
CPU revision	: 5

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v2 1/4] KVM: arm64: Allow userspace to change MIDR_EL1

[罗勇刚(Yonggang Luo)]

On Mon, Feb 17, 2025 at 2:09 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Sat, 15 Feb 2025 19:04:20 +0000,
> "罗勇刚(Yonggang Luo)" <luoyonggang@gmail.com> wrote:
> >
> > According to this, the MIDR EL1 is updated properly, but the MIDR for
> > aarch32 is not updated, and I don't know how to hook the update for
> > MIDR for aarch32
>
> It is the same thing. The AArch32 view is configured the same way as
> the AArch64 view, and there is nothing to do at all (that's what
> VPIDR_EL2 is all about).
>
> With Oliver's change, I'm correctly getting a different MIDR using a
> hacked up kvmtool, see below. I suspect you're not running with the
> correct patch applied.
>
The applied patch is V2, with writing vpidr_el2 disabled, Is that the cause?
The branch is here
https://github.com/lygstate/linux/tree/main
I am not applying your patch

-- 
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo

Re: [PATCH v2 1/4] KVM: arm64: Allow userspace to change MIDR_EL1

[Marc Zyngier]

On Sun, 16 Feb 2025 18:55:23 +0000,
"罗勇刚(Yonggang Luo)" <luoyonggang@gmail.com> wrote:
> 
> On Mon, Feb 17, 2025 at 2:09 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Sat, 15 Feb 2025 19:04:20 +0000,
> > "罗勇刚(Yonggang Luo)" <luoyonggang@gmail.com> wrote:
> > >
> > > According to this, the MIDR EL1 is updated properly, but the MIDR for
> > > aarch32 is not updated, and I don't know how to hook the update for
> > > MIDR for aarch32
> >
> > It is the same thing. The AArch32 view is configured the same way as
> > the AArch64 view, and there is nothing to do at all (that's what
> > VPIDR_EL2 is all about).
> >
> > With Oliver's change, I'm correctly getting a different MIDR using a
> > hacked up kvmtool, see below. I suspect you're not running with the
> > correct patch applied.
> >
> The applied patch is V2, with writing vpidr_el2 disabled, Is that the cause?
> The branch is here
> https://github.com/lygstate/linux/tree/main
> I am not applying your patch

Well, there is no magic. If you don't write to VPIDR_EL2, not much
will happen. You need to apply Oliver's change instead of the first
patch in this series.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v2 1/4] KVM: arm64: Allow userspace to change MIDR_EL1

[罗勇刚(Yonggang Luo)]

> Well, there is no magic. If you don't write to VPIDR_EL2, not much
> will happen. You need to apply Oliver's change instead of the first
> patch in this series.

Thanks, after applying Oliver's change, it's working, my concern post
at the beginning is to ask if
 Oliver's can resolve the issue:)
Now after testing Oliver's change, it's working, thanks
Sebastian,Oliver and Marc Zyngier:)

My test result now is:
```
/home/lygstate/work/qemu/build:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
/home/lygstate/work/qemu/build/qemu-system-aarch64
+ qemu-system-aarch64 -cpu host,aarch64=off -accel kvm -m 1024M -smp 1
-M virt,gic-version=3,midr=0x412fd050 -nographic -monitor none -serial
stdio -kernel /home/lygstate/work/debian/arm64-kvm-hello-world/bare-metal-aarch32-qemu/hello_world.elf
Get MIDR EL1 origin:0x410fd083
Set MIDR EL1:0x412fd050
Get MIDR EL1:0x412fd050
Hello World midr:0x412fd050
```


>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.



-- 
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo

Re: [PATCH v2 1/4] KVM: arm64: Allow userspace to change MIDR_EL1

[Sebastian Ott]

Hello Oliver,

On Sat, 15 Feb 2025, Oliver Upton wrote:
> On Tue, Feb 11, 2025 at 03:39:07PM +0100, Sebastian Ott wrote:
>> +static int set_id_reg_non_ftr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>> +			      u64 val)
>> +{
>> +	u32 id = reg_to_encoding(rd);
>> +	int ret;
>> +
>> +	mutex_lock(&vcpu->kvm->arch.config_lock);
>
> There's quite a few early outs, guard() might be a better fit than
> explicitly dropping the lock.

Yea, I thought about that too but most of the other functions in that file
use the classic lock primitives. But you're right - it looks cleaner.

>
>> +	/*
>> +	 * Since guest access to MIDR_EL1 is not trapped
>> +	 * set up VPIDR_EL2 to hold the MIDR_EL1 value.
>> +	 */
>> +	if (id == SYS_MIDR_EL1)
>> +		write_sysreg(val, vpidr_el2);
>
> This is problematic for a couple reasons:
>
> - If the kernel isn't running at EL2, VPIDR_EL2 is undefined
>
> - VPIDR_EL2 needs to be handled as part of the vCPU context, not
>   written to without a running vCPU. What would happen if two vCPUs
>   have different MIDR values?

Indeed. Sry, I hadn't thought about that. That makes much more sense now.

> Here's a new diff with some hacks thrown in to handle VPIDR_EL2
> correctly. Very lightly tested :)

Thank you very much! I've integrated that and currently run some tests
with it.

Sebastian