| Message ID | 20250211143910.16775-2-sebott@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: arm64: writable MIDR/REVIDR | expand |
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 }, };
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 }, > }; >
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.
> > 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) {
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, ®) < 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
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
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.
> 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.
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
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 }, };
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(-)