KVM: arm64: writable MIDR/REVIDR

Hi Sebastian,

arch/arm64/kvm/sys_regs.c static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,

Quoted 7 lines · Sebastian Ott

arch/arm64/kvm/sys_regs.c +7 -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);

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

Review

Quoted 6 lines

(quoted diff) +6 -0

+     /*
+      * 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
---

arch/arm64/include/asm/kvm_host.h +3 -0

index 7cfa024de4e3..3db8c773339e 100644
@@ -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:

arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h +4 -2

index 76ff095c6b6e..866411621a39 100644
@@ -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)) {

arch/arm64/kvm/hyp/nvhe/sysreg-sr.c +9 -1

index dba101565de3..a01be1add5ad 100644
@@ -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);

arch/arm64/kvm/hyp/vhe/sysreg-sr.c +11 -14

index 90b018e06f2c..1d4b9597eb29 100644
@@ -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);
 }

arch/arm64/kvm/sys_regs.c +40 -3

index f6cd1ea7fb55..aa1a0443dc6a 100644
@@ -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 },
 };

Quoted 24 lines · Oliver Upton

Hi Sebastian,

arch/arm64/kvm/sys_regs.c static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,

Quoted 7 lines · Sebastian Ott

arch/arm64/kvm/sys_regs.c +7 -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);

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

Review

Quoted 6 lines

(quoted diff) +6 -0

+     /*
+      * 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:

Quoted 8 lines

- 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.

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

Quoted 4 lines · 罗勇刚(Yonggang Luo)

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.

Quoted 12 lines · Marc Zyngier

Quoted 4 lines · 罗勇刚(Yonggang Luo)

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).

Quoted 4 lines

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.

arch/arm64/kvm/sys_regs.c static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,

Quoted 7 lines · Sebastian Ott

arch/arm64/kvm/sys_regs.c +7 -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);

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

Review

Quoted 7 lines

(quoted diff) +6 -0

+	/*
+	 * 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 :)