KVM: arm64: writable MIDR/REVIDR
v3 cover KVM: arm64: writable MIDR/REVIDR
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 V3:
* handle VPIDR_EL2 as part of vcpu ctxt - thanks Oliver!
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/[email protected]/T/#mb855bc51714095a164a7b26bb8bead1606e4b753
[2] https://lore.kernel.org/kvmarm/[email protected]/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/hyp/include/hyp/sysreg-sr.h | 5 +- arch/arm64/kvm/hyp/nvhe/sysreg-sr.c | 10 +- arch/arm64/kvm/hyp/vhe/sysreg-sr.c | 28 ++-- arch/arm64/kvm/sys_regs.c | 137 +++++++----------- .../testing/selftests/kvm/arm64/set_id_regs.c | 32 +++- 6 files changed, 108 insertions(+), 113 deletions(-)
Signature · 1 line
2.42.0
PATCH v3 1/4 KVM: arm64: Allow userspace to change MIDR_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.
--- arch/arm64/include/asm/kvm_host.h | 3 ++ arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h | 5 +-- arch/arm64/kvm/hyp/nvhe/sysreg-sr.c | 10 +++++- arch/arm64/kvm/hyp/vhe/sysreg-sr.c | 28 ++++++--------- arch/arm64/kvm/sys_regs.c | 42 ++++++++++++++++++++-- 5 files changed, 64 insertions(+), 24 deletions(-)
arch/arm64/include/asm/kvm_host.h +3 -0
@@ -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 +3 -2
@@ -168,9 +168,10 @@ 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
@@ -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 +10 -18
@@ -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);
@@ -220,23 +221,18 @@ void __vcpu_load_switch_sysregs(struct kvm_vcpu *vcpu)__sysreg_restore_vel2_state (vcpu); }else {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. -*/ -write_sysreg (ctxt_sys_reg (guest_ctxt,VPIDR_EL2 ), vpidr_el2); -/* * As we're restoring a nested guest, set the value * provided by the guest hypervisor. */ + midr =ctxt_sys_reg (guest_ctxt,VPIDR_EL2 ); 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 +267,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 +39 -3
77 lines not on this page · raw message
Signature · 1 line
2.42.0
PATCH v3 2/4 KVM: arm64: Allow userspace to change REVIDR_EL1
In reply to Sebastian OttEnable 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.
--- arch/arm64/include/asm/kvm_host.h | 3 +++ arch/arm64/kvm/sys_regs.c | 11 ++++++++--- 2 files changed, 11 insertions(+), 3 deletions(-)
arch/arm64/include/asm/kvm_host.h +3 -0
@@ -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 :
arch/arm64/kvm/sys_regs.c +8 -3
@@ -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 &&
@@ -2528,6 +2529,7 @@ static bool access_mdcr(struct kvm_vcpu *vcpu, }FUNCTION_RESET (midr_el1) +FUNCTION_RESET (revidr_el1)/*
@@ -2581,6 +2583,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
@@ -4632,12 +4636,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 }, };
@@ -4985,6 +4987,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 ;
Signature · 1 line
2.42.0
PATCH v3 3/4 KVM: arm64: Allow userspace to change AIDR_EL1
In reply to Sebastian OttEnable 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.
--- arch/arm64/include/asm/kvm_host.h | 3 ++ arch/arm64/kvm/sys_regs.c | 90 +++---------------------------- 2 files changed, 10 insertions(+), 83 deletions(-)
arch/arm64/include/asm/kvm_host.h +3 -0
@@ -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 :
arch/arm64/kvm/sys_regs.c +7 -83
163 lines not on this page · raw message
Signature · 1 line
2.42.0
PATCH v3 4/4 KVM: selftests: arm64: Test writes to MIDR,REVIDR,AIDR
In reply to Sebastian OttAssert 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.
--- .../testing/selftests/kvm/arm64/set_id_regs.c | 32 +++++++++++++++---- 1 file changed, 26 insertions(+), 6 deletions(-)
tools/testing/selftests/kvm/arm64/set_id_regs.c +26 -6
@@ -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);
Signature · 1 line
2.42.0
Hi Sebastian,
Quoted 7 lines · 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 V3:
* handleVPIDR_EL2as part of vcpu ctxt - thanks Oliver!
Thanks for respinning. While your changes are looking good, as I got ready to apply this series I wound up peeling the onion a bit further and have a few more concerns:
- Current KVM allows guests to read SMIDR_EL1 despite the fact that we do not support SME (this is part of TID1 traps)
- The "invariant" values that KVM presents to userspace originate from the boot CPU, not the CPU that resets the ID registers for a VM
- A VMM that wants to present big-little can do so on current KVM by affining vCPUs, but cannot with this series
All of this is to say, I think your series is going to collide with the pre-existing pile of crap we have. I'm going to pick up these changes and rework them so we can send a fix for #1 to stable trees and (hopefully) avoid breaking the old "invariant" behavior.
I'll post what I have as soon as I test it, hopefully we can get this shaped up for 6.15.
Thanks,
Oliver
Hi Oliver,
Quoted 22 lines · Oliver Upton
Quoted 7 lines · 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 V3:
* handleVPIDR_EL2as part of vcpu ctxt - thanks Oliver!Thanks for respinning. While your changes are looking good, as I got ready to apply this series I wound up peeling the onion a bit further and have a few more concerns:
- Current KVM allows guests to read
SMIDR_EL1despite the fact that we do not support SME (this is part of TID1 traps)- The "invariant" values that KVM presents to userspace originate from the boot CPU, not the CPU that resets the ID registers for a VM
- A VMM that wants to present big-little can do so on current KVM by affining vCPUs, but cannot with this series
Quoted 7 lines
All of this is to say, I think your series is going to collide with the pre-existing pile of crap we have. I'm going to pick up these changes and rework them so we can send a fix for #1 to stable trees and (hopefully) avoid breaking the old "invariant" behavior.
I'll post what I have as soon as I test it, hopefully we can get this shaped up for 6.15.
Sry, for the additional work I've caused. I gave what you have in next a spin and it looks good so far.
Thank you very much!
Sebastian
Quoted 28 lines · Sebastian Ott
Quoted 27 lines · Oliver Upton
Quoted 7 lines · 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 V3:
* handleVPIDR_EL2as part of vcpu ctxt - thanks Oliver!Thanks for respinning. While your changes are looking good, as I got ready to apply this series I wound up peeling the onion a bit further and have a few more concerns:
- Current KVM allows guests to read
SMIDR_EL1despite the fact that we do not support SME (this is part of TID1 traps)- The "invariant" values that KVM presents to userspace originate from the boot CPU, not the CPU that resets the ID registers for a VM
- A VMM that wants to present big-little can do so on current KVM by affining vCPUs, but cannot with this series
All of this is to say, I think your series is going to collide with the pre-existing pile of crap we have. I'm going to pick up these changes and rework them so we can send a fix for #1 to stable trees and (hopefully) avoid breaking the old "invariant" behavior.
Quoted 6 lines
Quoted 2 lines
I'll post what I have as soon as I test it, hopefully we can get this shaped up for 6.15.
Sry, for the additional work I've caused. I gave what you have in next a spin and it looks good so far.
Thank you very much!
Don't apologize -- it isn't your fault what we had already was a mess! :)
Thanks for kicking the tires.
Best,
Oliver