KVM: arm64: BUG FIX: Correctly handle zero register transfers
Subsystems (4)
v4 cover KVM: arm64: BUG FIX: Correctly handle zero register transfers
ARM64 CPU has zero register which is read-only, with a value of 0. However, KVM currently incorrectly recognizes it being SP (because Rt == 31, and in struct user_pt_regs 'regs' array is followed by SP), resulting in invalid value being read, or even SP corruption on write.
The problem has been discovered by performing an operation
*((volatile int *)reg) = 0;
which compiles as "str xzr, [xx]", and resulted in strange values being written.
v3 => v4:
- Unwrapped assignment in patch 0003
v2 => v3:
- Brought back some const modifiers in unaffected functions
v1 => v2:
- Changed type of transfer value to u64 and store it directly in struct sys_reg_params instead of a pointer
- Use lower_32_bits()/upper_32_bits() where appropriate
- Fixed wrong usage of 'Rt' instead of 'Rt2' in kvm_handle_cp_64(), overlooked in v1
- Do not write value back when reading
Pavel Fedin (4):
KVM: arm64: Correctly handle zero register during MMIO
KVM: arm64: Remove const from struct sys_reg_params
KVM: arm64: Correctly handle zero register in system register accesses
KVM: arm64: Get rid of old vcpu_reg()
arch/arm/include/asm/kvm_emulate.h | 12 ++++ arch/arm/kvm/mmio.c | 5 +- arch/arm/kvm/psci.c | 20 +++--- arch/arm64/include/asm/kvm_emulate.h | 18 +++-- arch/arm64/kvm/handle_exit.c | 2 +- arch/arm64/kvm/sys_regs.c | 123 +++++++++++++++++------------------ arch/arm64/kvm/sys_regs.h | 8 +-- arch/arm64/kvm/sys_regs_generic_v8.c | 4 +- 8 files changed, 105 insertions(+), 87 deletions(-)
Signature · 1 line
2.4.4
PATCH v4 1/4 KVM: arm64: Correctly handle zero register during MMIO
On ARM64 register index of 31 corresponds to both zero register and SP. However, all memory access instructions, use ZR as transfer register. SP is used only as a base register in indirect memory addressing, or by register-register arithmetics, which cannot be trapped here.
Correct emulation is achieved by introducing new register accessor functions, which can do special handling for reg_num == 31. These new accessors intentionally do not rely on old vcpu_reg() on ARM64, because it is to be removed. Since the affected code is shared by both ARM flavours, implementations of these accessors are also added to ARM32 code.
This patch fixes setting MMIO register to a random value (actually SP) instead of zero by something like:
*((volatile int *)reg) = 0;
compilers tend to generate "str wzr, [xx]" here
--- arch/arm/include/asm/kvm_emulate.h | 12 ++++++++++++ arch/arm/kvm/mmio.c | 5 +++-- arch/arm64/include/asm/kvm_emulate.h | 13 +++++++++++++ 3 files changed, 28 insertions(+), 2 deletions(-)
arch/arm/include/asm/kvm_emulate.h +12 -0
@@ -28,6 +28,18 @@unsigned long *vcpu_reg (struct kvm_vcpu *vcpu,u8 reg_num);unsigned long *vcpu_spsr (struct kvm_vcpu *vcpu); +static inline unsigned long vcpu_get_reg (const struct kvm_vcpu *vcpu, +u8 reg_num) +{ +return *vcpu_reg (vcpu, reg_num); +} + +static inline void vcpu_set_reg (const struct kvm_vcpu *vcpu,u8 reg_num, +unsigned long val) +{ + *vcpu_reg (vcpu, reg_num) = val; +} +bool kvm_condition_valid (struct kvm_vcpu *vcpu);void kvm_skip_instr (struct kvm_vcpu *vcpu,bool is_wide_instr);void kvm_inject_undefined (struct kvm_vcpu *vcpu);
arch/arm/kvm/mmio.c +3 -2
@@ -115,7 +115,7 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)trace_kvm_mmio (KVM_TRACE_MMIO_READ , len, run->mmio.phys_addr, data); data =vcpu_data_host_to_guest (vcpu, data, len); - *vcpu_reg (vcpu, vcpu->arch.mmio_decode.rt) = data; +vcpu_set_reg (vcpu, vcpu->arch.mmio_decode.rt, data); }return 0 ;
@@ -186,7 +186,8 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run, rt = vcpu->arch.mmio_decode.rt;if (is_write) { - data =vcpu_data_guest_to_host (vcpu, *vcpu_reg (vcpu, rt), len); + data =vcpu_data_guest_to_host (vcpu,vcpu_get_reg (vcpu, rt), + len);trace_kvm_mmio (KVM_TRACE_MMIO_WRITE , len, fault_ipa, data);mmio_write_buf (data_buf, len, data);
arch/arm64/include/asm/kvm_emulate.h +13 -0
@@ -109,6 +109,19 @@ static inline unsigned long *vcpu_reg(const struct kvm_vcpu *vcpu, u8 reg_num)return (unsigned long *)&vcpu_gp_regs (vcpu)->regs.regs[reg_num]; } +static inline unsigned long vcpu_get_reg (const struct kvm_vcpu *vcpu, +u8 reg_num) +{ +return (reg_num ==31 ) ?0 :vcpu_gp_regs (vcpu)->regs.regs[reg_num]; +} + +static inline void vcpu_set_reg (struct kvm_vcpu *vcpu,u8 reg_num, +unsigned long val) +{ +if (reg_num !=31 ) +vcpu_gp_regs (vcpu)->regs.regs[reg_num] = val; +} +/* Get vcpu SPSR for current mode */ static inline unsigned long *vcpu_spsr (const struct kvm_vcpu *vcpu) {
Signature · 1 line
2.4.4
Re: [PATCH v4 1/4] KVM: arm64: Correctly handle zero register during MMIO
Quoted 36 lines · Pavel Fedin
On ARM64 register index of 31 corresponds to both zero register and SP. However, all memory access instructions, use ZR as transfer register. SP is used only as a base register in indirect memory addressing, or by register-register arithmetics, which cannot be trapped here.
Correct emulation is achieved by introducing new register accessor functions, which can do special handling for
reg_num== 31. These new accessors intentionally do not rely on oldvcpu_reg()on ARM64, because it is to be removed. Since the affected code is shared by both ARM flavours, implementations of these accessors are also added to ARM32 code.This patch fixes setting MMIO register to a random value (actually SP) instead of zero by something like:
*((volatile int *)reg) = 0;
compilers tend to generate "str wzr, [xx]" here
Signed-off-by Pavel Fedin <[email protected]>Reviewed-by Marc Zyngier <[email protected]>--- arch/arm/include/asm/kvm_emulate.h | 12 ++++++++++++ arch/arm/kvm/mmio.c | 5 +++-- arch/arm64/include/asm/kvm_emulate.h | 13 +++++++++++++ 3 files changed, 28 insertions(+), 2 deletions(-)arch/arm/include/asm/kvm_emulate.h +2
-0@@ -28,6 +28,18 @@unsigned long *vcpu_reg (struct kvm_vcpu *vcpu,u8 reg_num);unsigned long *vcpu_spsr (struct kvm_vcpu *vcpu); +static inline unsigned long vcpu_get_reg (const struct kvm_vcpu *vcpu, +u8 reg_num)
arch/arm/include/asm/kvm_emulate.h line 42
Quoted 10 lines
arch/arm/include/asm/kvm_emulate.h +10
-0+{ +return *vcpu_reg (vcpu, reg_num); +} + +static inline void vcpu_set_reg (const struct kvm_vcpu *vcpu,u8 reg_num, +unsigned long val) +{ + *vcpu_reg (vcpu, reg_num) = val; +} +
This makes a 32bit compile scream (making these vcpu pointer const is not a good idea).
I'll fix it locally.
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
PATCH v4 2/4 KVM: arm64: Remove const from struct sys_reg_params
In reply to Pavel FedinFurther rework is going to introduce a dedicated storage for transfer register value in struct sys_reg_params. Before doing this we have to remove 'const' modifiers from it in all accessor functions and their callers.
--- arch/arm64/kvm/sys_regs.c | 36 ++++++++++++++++++------------------ arch/arm64/kvm/sys_regs.h | 4 ++-- arch/arm64/kvm/sys_regs_generic_v8.c | 2 +- 3 files changed, 21 insertions(+), 21 deletions(-)
arch/arm64/kvm/sys_regs.c +18 -18
@@ -78,7 +78,7 @@ static u32 get_ccsidr(u32 csselr)* See note at ARMv7 ARM B1.14.4 (TL;DR: S/W ops are not easily virtualized). */ static bool access_dcsw (struct kvm_vcpu *vcpu, -const struct sys_reg_params *p, +struct sys_reg_params *p,const struct sys_reg_desc *r) {if (!p->is_write)
@@ -94,7 +94,7 @@ static bool access_dcsw(struct kvm_vcpu *vcpu,* sys_regs and leave it in complete control of the caches. */ static bool access_vm_reg (struct kvm_vcpu *vcpu, -const struct sys_reg_params *p, +struct sys_reg_params *p,const struct sys_reg_desc *r) {unsigned long val;
@@ -122,7 +122,7 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu,* for both AArch64 and AArch32 accesses. */ static bool access_gic_sgi (struct kvm_vcpu *vcpu, -const struct sys_reg_params *p, +struct sys_reg_params *p,const struct sys_reg_desc *r) {u64 val;
@@ -137,7 +137,7 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu, }static bool trap_raz_wi (struct kvm_vcpu *vcpu, -const struct sys_reg_params *p, +struct sys_reg_params *p,const struct sys_reg_desc *r) {if (p->is_write)
@@ -147,7 +147,7 @@ static bool trap_raz_wi(struct kvm_vcpu *vcpu, }static bool trap_oslsr_el1 (struct kvm_vcpu *vcpu, -const struct sys_reg_params *p, +struct sys_reg_params *p,const struct sys_reg_desc *r) {if (p->is_write) {
@@ -159,7 +159,7 @@ static bool trap_oslsr_el1(struct kvm_vcpu *vcpu, }static bool trap_dbgauthstatus_el1 (struct kvm_vcpu *vcpu, -const struct sys_reg_params *p, +struct sys_reg_params *p,const struct sys_reg_desc *r) {if (p->is_write) {
@@ -200,7 +200,7 @@ static bool trap_dbgauthstatus_el1(struct kvm_vcpu *vcpu,* now use the debug registers. */ static bool trap_debug_regs (struct kvm_vcpu *vcpu, -const struct sys_reg_params *p, +struct sys_reg_params *p,const struct sys_reg_desc *r) {if (p->is_write) {
@@ -225,7 +225,7 @@ static bool trap_debug_regs(struct kvm_vcpu *vcpu,* hyp.S code switches between host and guest values in future. */ static inline void reg_to_dbg (struct kvm_vcpu *vcpu, -const struct sys_reg_params *p, +struct sys_reg_params *p,u64 *dbg_reg) {u64 val = *vcpu_reg (vcpu, p->Rt);
@@ -240,7 +240,7 @@ static inline void reg_to_dbg(struct kvm_vcpu *vcpu, }static inline void dbg_to_reg (struct kvm_vcpu *vcpu, -const struct sys_reg_params *p, +struct sys_reg_params *p,u64 *dbg_reg) {u64 val = *dbg_reg;
@@ -252,7 +252,7 @@ static inline void dbg_to_reg(struct kvm_vcpu *vcpu, }static inline bool trap_bvr (struct kvm_vcpu *vcpu, -const struct sys_reg_params *p, +struct sys_reg_params *p,const struct sys_reg_desc *rd) {u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->reg];
@@ -294,7 +294,7 @@ static inline void reset_bvr(struct kvm_vcpu *vcpu, }static inline bool trap_bcr (struct kvm_vcpu *vcpu, -const struct sys_reg_params *p, +struct sys_reg_params *p,const struct sys_reg_desc *rd) {u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->reg];
@@ -337,7 +337,7 @@ static inline void reset_bcr(struct kvm_vcpu *vcpu, }static inline bool trap_wvr (struct kvm_vcpu *vcpu, -const struct sys_reg_params *p, +struct sys_reg_params *p,const struct sys_reg_desc *rd) {u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->reg];
@@ -380,7 +380,7 @@ static inline void reset_wvr(struct kvm_vcpu *vcpu, }static inline bool trap_wcr (struct kvm_vcpu *vcpu, -const struct sys_reg_params *p, +struct sys_reg_params *p,const struct sys_reg_desc *rd) {u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->reg];
@@ -687,7 +687,7 @@ static const struct sys_reg_desc sys_reg_descs[] = { };static bool trap_dbgidr (struct kvm_vcpu *vcpu, -const struct sys_reg_params *p, +struct sys_reg_params *p,const struct sys_reg_desc *r) {if (p->is_write) {
@@ -706,7 +706,7 @@ static bool trap_dbgidr(struct kvm_vcpu *vcpu, }static bool trap_debug32 (struct kvm_vcpu *vcpu, -const struct sys_reg_params *p, +struct sys_reg_params *p,const struct sys_reg_desc *r) {if (p->is_write) {
@@ -731,7 +731,7 @@ static bool trap_debug32(struct kvm_vcpu *vcpu,*/ static inline bool trap_xvr (struct kvm_vcpu *vcpu, -const struct sys_reg_params *p, +struct sys_reg_params *p,const struct sys_reg_desc *rd) {u64 *dbg_reg = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->reg];
@@ -991,7 +991,7 @@ int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu, struct kvm_run *run)* Return 0 if the access has been handled, and -1 if not. */ static int emulate_cp (struct kvm_vcpu *vcpu, -const struct sys_reg_params *params, +struct sys_reg_params *params,const struct sys_reg_desc *table,size_t num) {
@@ -1175,7 +1175,7 @@ int kvm_handle_cp14_32(struct kvm_vcpu *vcpu, struct kvm_run *run) }static int emulate_sys_reg (struct kvm_vcpu *vcpu, -const struct sys_reg_params *params) +struct sys_reg_params *params) {size_t num;const struct sys_reg_desc *table, *r;
arch/arm64/kvm/sys_regs.h +2 -2
@@ -44,7 +44,7 @@ struct sys_reg_desc {/* Trapped access from guest, if non-NULL. */ bool (*access)(struct kvm_vcpu *, -const struct sys_reg_params *, +struct sys_reg_params *,const struct sys_reg_desc *);/* Initialization for vcpu. */
@@ -77,7 +77,7 @@ static inline bool ignore_write(struct kvm_vcpu *vcpu, }static inline bool read_zero (struct kvm_vcpu *vcpu, -const struct sys_reg_params *p) +struct sys_reg_params *p) { *vcpu_reg (vcpu, p->Rt) =0 ;return true;
arch/arm64/kvm/sys_regs_generic_v8.c +1 -1
@@ -31,7 +31,7 @@#include "sys_regs.h" static bool access_actlr (struct kvm_vcpu *vcpu, -const struct sys_reg_params *p, +struct sys_reg_params *p,const struct sys_reg_desc *r) {if (p->is_write)
Signature · 1 line
2.4.4
Re: [PATCH v4 2/4] KVM: arm64: Remove const from struct sys_reg_params
Quoted 11 lines · Pavel Fedin
Further rework is going to introduce a dedicated storage for transfer register value in struct
sys_reg_params. Before doing this we have to remove 'const' modifiers from it in all accessor functions and their callers.Signed-off-by Pavel Fedin <[email protected]>--- arch/arm64/kvm/sys_regs.c | 36 ++++++++++++++++++------------------ arch/arm64/kvm/sys_regs.h | 4 ++-- arch/arm64/kvm/sys_regs_generic_v8.c | 2 +- 3 files changed, 21 insertions(+), 21 deletions(-)
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
PATCH v4 3/4 KVM: arm64: Correctly handle zero register in system register accesses
In reply to Pavel FedinSystem register accesses also use zero register for Rt == 31, and therefore using it will also result in getting SP value instead. This patch makes them also using new accessors, introduced by the previous patch. Since register value is no longer directly associated with storage inside vCPU context structure, we introduce a dedicated storage for it in struct sys_reg_params.
This refactor also gets rid of "massive hack" in kvm_handle_cp_64().
--- arch/arm64/kvm/sys_regs.c | 87 +++++++++++++++++------------------- arch/arm64/kvm/sys_regs.h | 4 +- arch/arm64/kvm/sys_regs_generic_v8.c | 2 +- 3 files changed, 45 insertions(+), 48 deletions(-)
arch/arm64/kvm/sys_regs.c +42 -45
@@ -97,18 +97,16 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu,struct sys_reg_params *p, const struct sys_reg_desc *r) { - unsigned long val; bool was_enabled = vcpu_has_cache_enabled(vcpu);BUG_ON (!p->is_write); - val = *vcpu_reg (vcpu, p->Rt);if (!p->is_aarch32) { -vcpu_sys_reg (vcpu, r->reg) = val; +vcpu_sys_reg (vcpu, r->reg) = p->regval; }else {if (!p->is_32bit) -vcpu_cp15_64_high (vcpu, r->reg) = val >>32 ; -vcpu_cp15_64_low (vcpu, r->reg) = val &0xffffffffUL ; +vcpu_cp15_64_high (vcpu, r->reg) =upper_32_bits (p->regval); +vcpu_cp15_64_low (vcpu, r->reg) =lower_32_bits (p->regval); }kvm_toggle_cache (vcpu, was_enabled);
@@ -125,13 +123,10 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu, struct sys_reg_params *p, const struct sys_reg_desc *r) { - u64 val; -if (!p->is_write)return read_from_write_only (vcpu, p); - val = *vcpu_reg (vcpu, p->Rt); -vgic_v3_dispatch_sgi (vcpu, val); +vgic_v3_dispatch_sgi (vcpu, p->regval);return true; }
@@ -153,7 +148,7 @@ static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,if (p->is_write) {return ignore_write (vcpu, p); }else { - *vcpu_reg (vcpu, p->Rt) = (1 <<3 ); + p->regval = (1 <<3 );return true; } }
@@ -167,7 +162,7 @@ static bool trap_dbgauthstatus_el1(struct kvm_vcpu *vcpu, }else {u32 val; asmvolatile ("mrs %0, dbgauthstatus_el1" :"=r" (val)); - *vcpu_reg (vcpu, p->Rt) = val; + p->regval = val;return true; } }
@@ -204,13 +199,13 @@ static bool trap_debug_regs(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) {if (p->is_write) { -vcpu_sys_reg (vcpu, r->reg) = *vcpu_reg (vcpu, p->Rt); +vcpu_sys_reg (vcpu, r->reg) = p->regval; vcpu->arch.debug_flags |=KVM_ARM64_DEBUG_DIRTY ; }else { - *vcpu_reg (vcpu, p->Rt) =vcpu_sys_reg (vcpu, r->reg); + p->regval =vcpu_sys_reg (vcpu, r->reg); } -trace_trap_reg (__func__, r->reg, p->is_write, *vcpu_reg (vcpu, p->Rt)); +trace_trap_reg (__func__, r->reg, p->is_write, p->regval);return true; }
@@ -228,7 +223,7 @@ static inline void reg_to_dbg(struct kvm_vcpu *vcpu,struct sys_reg_params *p, u64 *dbg_reg) { - u64 val = *vcpu_reg(vcpu, p->Rt); + u64 val = p->regval;if (p->is_32bit) { val &=0xffffffffUL ;
@@ -243,12 +238,9 @@ static inline void dbg_to_reg(struct kvm_vcpu *vcpu,struct sys_reg_params *p, u64 *dbg_reg) { - u64 val = *dbg_reg; - + p->regval = *dbg_reg;if (p->is_32bit) - val &=0xffffffffUL ; - - *vcpu_reg (vcpu, p->Rt) = val; + p->regval &=0xffffffffUL ; } static inline bool trap_bvr(struct kvm_vcpu *vcpu,
@@ -697,10 +689,10 @@ static bool trap_dbgidr(struct kvm_vcpu *vcpu,u64 pfr =read_system_reg (SYS_ID_AA64PFR0_EL1 );u32 el3 = !!cpuid_feature_extract_field (pfr,ID_AA64PFR0_EL3_SHIFT ); - *vcpu_reg (vcpu, p->Rt) = ((((dfr >>ID_AA64DFR0_WRPS_SHIFT ) &0xf ) <<28 ) | - (((dfr >>ID_AA64DFR0_BRPS_SHIFT ) &0xf ) <<24 ) | - (((dfr >>ID_AA64DFR0_CTX_CMPS_SHIFT ) &0xf ) <<20 ) | - (6 <<16 ) | (el3 <<14 ) | (el3 <<12 )); + p->regval = ((((dfr >>ID_AA64DFR0_WRPS_SHIFT ) &0xf ) <<28 ) | + (((dfr >>ID_AA64DFR0_BRPS_SHIFT ) &0xf ) <<24 ) | + (((dfr >>ID_AA64DFR0_CTX_CMPS_SHIFT ) &0xf ) <<20 ) + | (6 <<16 ) | (el3 <<14 ) | (el3 <<12 ));return true; } }
@@ -710,10 +702,10 @@ static bool trap_debug32(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) {if (p->is_write) { -vcpu_cp14 (vcpu, r->reg) = *vcpu_reg (vcpu, p->Rt); +vcpu_cp14 (vcpu, r->reg) = p->regval; vcpu->arch.debug_flags |=KVM_ARM64_DEBUG_DIRTY ; }else { - *vcpu_reg (vcpu, p->Rt) =vcpu_cp14 (vcpu, r->reg); + p->regval =vcpu_cp14 (vcpu, r->reg); }return true;
@@ -740,12 +732,12 @@ static inline bool trap_xvr(struct kvm_vcpu *vcpu,u64 val = *dbg_reg; val &=0xffffffffUL ; - val |= *vcpu_reg (vcpu, p->Rt) <<32 ; + val |= p->regval <<32 ; *dbg_reg = val; vcpu->arch.debug_flags |=KVM_ARM64_DEBUG_DIRTY ; }else { - *vcpu_reg (vcpu, p->Rt) = *dbg_reg >>32 ; + p->regval = *dbg_reg >>32 ; }trace_trap_reg (__func__, rd->reg, p->is_write, *dbg_reg);
@@ -1062,12 +1054,12 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu, {struct sys_reg_params params;u32 hsr =kvm_vcpu_get_hsr (vcpu); +int Rt = (hsr >>5 ) &0xf ;int Rt2 = (hsr >>10 ) &0xf ; params.is_aarch32 = true; params.is_32bit = false; params.CRm = (hsr >>1 ) &0xf ; - params.Rt = (hsr >>5 ) &0xf ; params.is_write = ((hsr &1 ) ==0 ); params.Op0 =0 ;
@@ -1076,15 +1068,12 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu, params.CRn =0 ;/* -* Massive hack here. Store Rt2 in the top 32bits so we only -* have one register to deal with. As we use the same trap +* Make a 64-bit value out of Rt and Rt2. As we use the same trap * backends between AArch32 and AArch64, we get away with it. */ if (params.is_write) { -u64 val = *vcpu_reg (vcpu, params.Rt); - val &=0xffffffff ; - val |= *vcpu_reg (vcpu, Rt2) <<32 ; - *vcpu_reg (vcpu, params.Rt) = val; + params.regval =vcpu_get_reg (vcpu, Rt) &0xffffffff ; + params.regval |=vcpu_get_reg (vcpu, Rt2) <<32 ; }if (!emulate_cp (vcpu, ¶ms, target_specific, nr_specific))
@@ -1095,11 +1084,10 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,unhandled_cp_access (vcpu, ¶ms); out: -/* Do the opposite hack for the read side */ +/* Split up the value between registers for the read side */ if (!params.is_write) { -u64 val = *vcpu_reg (vcpu, params.Rt); - val >>=32 ; - *vcpu_reg (vcpu, Rt2) = val; +vcpu_set_reg (vcpu, Rt,lower_32_bits (params.regval)); +vcpu_set_reg (vcpu, Rt2,upper_32_bits (params.regval)); }return 1 ;
@@ -1118,21 +1106,24 @@ static int kvm_handle_cp_32(struct kvm_vcpu *vcpu, {struct sys_reg_params params;u32 hsr =kvm_vcpu_get_hsr (vcpu); +int Rt = (hsr >>5 ) &0xf ; params.is_aarch32 = true; params.is_32bit = true; params.CRm = (hsr >>1 ) &0xf ; - params.Rt = (hsr >>5 ) &0xf ; + params.regval =vcpu_get_reg (vcpu, Rt); params.is_write = ((hsr &1 ) ==0 ); params.CRn = (hsr >>10 ) &0xf ; params.Op0 =0 ; params.Op1 = (hsr >>14 ) &0x7 ; params.Op2 = (hsr >>17 ) &0x7 ; -if (!emulate_cp (vcpu, ¶ms, target_specific, nr_specific)) -return 1 ; -if (!emulate_cp (vcpu, ¶ms, global, nr_global)) +if (!emulate_cp (vcpu, ¶ms, target_specific, nr_specific) || + !emulate_cp (vcpu, ¶ms, global, nr_global)) { +if (!params.is_write) +vcpu_set_reg (vcpu, Rt, params.regval);return 1 ; + }unhandled_cp_access (vcpu, ¶ms);return 1 ;
@@ -1230,6 +1221,8 @@ int kvm_handle_sys_reg(struct kvm_vcpu *vcpu, struct kvm_run *run) {struct sys_reg_params params;unsigned long esr =kvm_vcpu_get_hsr (vcpu); +int Rt = (esr >>5 ) &0x1f ; +int ret;trace_kvm_handle_sys_reg (esr);
@@ -1240,10 +1233,14 @@ int kvm_handle_sys_reg(struct kvm_vcpu *vcpu, struct kvm_run *run) params.CRn = (esr >>10 ) &0xf ; params.CRm = (esr >>1 ) &0xf ; params.Op2 = (esr >>17 ) &0x7 ; - params.Rt = (esr >>5 ) &0x1f ; + params.regval =vcpu_get_reg (vcpu, Rt); params.is_write = !(esr &1 ); -return emulate_sys_reg (vcpu, ¶ms); + ret =emulate_sys_reg (vcpu, ¶ms); + +if (!params.is_write) +vcpu_set_reg (vcpu, Rt, params.regval); +return ret; }/******************************************************************************
arch/arm64/kvm/sys_regs.h +2 -2
@@ -28,7 +28,7 @@ struct sys_reg_params {u8 CRn;u8 CRm;u8 Op2; -u8 Rt; +u64 regval;bool is_write;bool is_aarch32;bool is_32bit;/* Only valid if is_aarch32 is true */
@@ -79,7 +79,7 @@ static inline bool ignore_write(struct kvm_vcpu *vcpu,static inline bool read_zero (struct kvm_vcpu *vcpu,struct sys_reg_params *p) { - *vcpu_reg (vcpu, p->Rt) =0 ; + p->regval =0 ;return true; }
arch/arm64/kvm/sys_regs_generic_v8.c +1 -1
@@ -37,7 +37,7 @@ static bool access_actlr(struct kvm_vcpu *vcpu,if (p->is_write)return ignore_write (vcpu, p); - *vcpu_reg (vcpu, p->Rt) =vcpu_sys_reg (vcpu,ACTLR_EL1 ); + p->regval =vcpu_sys_reg (vcpu,ACTLR_EL1 );return true; }
Signature · 1 line
2.4.4
PATCH v4 4/4 KVM: arm64: Get rid of old vcpu_reg()
In reply to Pavel FedinUsing oldstyle vcpu_reg() accessor is proven to be inappropriate and unsafe on ARM64. This patch converts the rest of use cases to new accessors and completely removes vcpu_reg() on ARM64.
--- arch/arm/kvm/psci.c | 20 ++++++++++---------- arch/arm64/include/asm/kvm_emulate.h | 11 +++-------- arch/arm64/kvm/handle_exit.c | 2 +- 3 files changed, 14 insertions(+), 19 deletions(-)
arch/arm/kvm/psci.c +10 -10
@@ -75,7 +75,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)unsigned long context_id;phys_addr_t target_pc; - cpu_id = *vcpu_reg (source_vcpu,1 ) &MPIDR_HWID_BITMASK ; + cpu_id =vcpu_get_reg (source_vcpu,1 ) &MPIDR_HWID_BITMASK ;if (vcpu_mode_is_32bit (source_vcpu)) cpu_id &= ~((u32 )0 );
@@ -94,8 +94,8 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)return PSCI_RET_INVALID_PARAMS ; } - target_pc = *vcpu_reg (source_vcpu,2 ); - context_id = *vcpu_reg (source_vcpu,3 ); + target_pc =vcpu_get_reg (source_vcpu,2 ); + context_id =vcpu_get_reg (source_vcpu,3 );kvm_reset_vcpu (vcpu);
@@ -114,7 +114,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)* NOTE: We always update r0 (or x0) because for PSCI v0.1 * the general puspose registers are undefined upon CPU_ON. */ - *vcpu_reg (vcpu,0 ) = context_id; +vcpu_set_reg (vcpu,0 , context_id); vcpu->arch.power_off = false;smp_mb ();/* Make sure the above is visible */
@@ -134,8 +134,8 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu)struct kvm *kvm = vcpu->kvm;struct kvm_vcpu *tmp; - target_affinity = *vcpu_reg (vcpu,1 ); - lowest_affinity_level = *vcpu_reg (vcpu,2 ); + target_affinity =vcpu_get_reg (vcpu,1 ); + lowest_affinity_level =vcpu_get_reg (vcpu,2 );/* Determine target affinity mask */ target_affinity_mask =psci_affinity_mask (lowest_affinity_level);
@@ -209,7 +209,7 @@ int kvm_psci_version(struct kvm_vcpu *vcpu)static int kvm_psci_0_2_call (struct kvm_vcpu *vcpu) {int ret =1 ; -unsigned long psci_fn = *vcpu_reg (vcpu,0 ) & ~((u32 )0 ); +unsigned long psci_fn =vcpu_get_reg (vcpu,0 ) & ~((u32 )0 );unsigned long val;switch (psci_fn) {
@@ -273,13 +273,13 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu)break ; } - *vcpu_reg (vcpu,0 ) = val; +vcpu_set_reg (vcpu,0 , val);return ret; }static int kvm_psci_0_1_call (struct kvm_vcpu *vcpu) { -unsigned long psci_fn = *vcpu_reg (vcpu,0 ) & ~((u32 )0 ); +unsigned long psci_fn =vcpu_get_reg (vcpu,0 ) & ~((u32 )0 );unsigned long val;switch (psci_fn) {
@@ -295,7 +295,7 @@ static int kvm_psci_0_1_call(struct kvm_vcpu *vcpu)break ; } - *vcpu_reg (vcpu,0 ) = val; +vcpu_set_reg (vcpu,0 , val);return 1 ; }
arch/arm64/include/asm/kvm_emulate.h +3 -8
@@ -100,15 +100,10 @@ static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu) }/* -* vcpu_reg should always be passed a register number coming from a -* read of ESR_EL2. Otherwise, it may give the wrong result on AArch32 -* with banked registers. +* vcpu_get_reg and vcpu_set_reg should always be passed a register number +* coming from a read of ESR_EL2. Otherwise, it may give the wrong result on +* AArch32 with banked registers. */ -static inline unsigned long *vcpu_reg (const struct kvm_vcpu *vcpu,u8 reg_num) -{ -return (unsigned long *)&vcpu_gp_regs (vcpu)->regs.regs[reg_num]; -} -static inline unsigned long vcpu_get_reg (const struct kvm_vcpu *vcpu,u8 reg_num) {
arch/arm64/kvm/handle_exit.c +1 -1
@@ -37,7 +37,7 @@ static int handle_hvc(struct kvm_vcpu *vcpu, struct kvm_run *run) {int ret; -trace_kvm_hvc_arm64 (*vcpu_pc (vcpu), *vcpu_reg (vcpu,0 ), +trace_kvm_hvc_arm64 (*vcpu_pc (vcpu),vcpu_get_reg (vcpu,0 ),kvm_vcpu_hvc_get_imm (vcpu)); ret =kvm_psci_call (vcpu);
Signature · 1 line
2.4.4
Quoted 26 lines · Pavel Fedin
ARM64 CPU has zero register which is read-only, with a value of 0. However, KVM currently incorrectly recognizes it being SP (because Rt == 31, and in struct
user_pt_regs'regs' array is followed by SP), resulting in invalid value being read, or even SP corruption on write.The problem has been discovered by performing an operation
*((volatile int *)reg) = 0;
which compiles as "str xzr, [xx]", and resulted in strange values being written.
v3 => v4:
- Unwrapped assignment in patch 0003v2 => v3:
- Brought back some const modifiers in unaffected functionsv1 => v2:
- Changed type of transfer value to u64 and store it directly in structsys_reg_paramsinstead of a pointer
- Use lower_32_bits()/upper_32_bits() where appropriate
- Fixed wrong usage of 'Rt' instead of 'Rt2' inkvm_handle_cp_64(), overlooked in v1
- Do not write value back when reading
Quoted 6 lines
Pavel Fedin (4):
KVM: arm64: Correctly handle zero register during MMIO
KVM: arm64: Remove const from structsys_reg_params
KVM: arm64: Correctly handle zero register in system register accesses
KVM: arm64: Get rid of oldvcpu_reg()
FYI, I tried writing test cases for this issue with kvm-unit-tests. The issue didn't reproduce for me. It's quite possible my test cases are flawed, so I'm not making any claims about the validity of the series (I also see that it has already been acked and pulled). But, if Pavel doesn't mind trying them out on his system, then it'd be good to know if they reproduce there. I'd like to find out if it's a test case problem or something else strange going on with environments.
kvm-unit-tests patch attached
Thanks,
drew
Quoted 1 line
From
6576833b5e45801f0226316afae7daf0936a0aeeMon Sep 17 00:00:00 2001
From: Andrew Jones <[email protected]>
Date: Fri, 4 Dec 2015 23:55:53 +0100
Subject: [kvm-unit-tests PATCH] arm64: add xzr emulator test
--- arm/xzr-test.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++ config/config-arm64.mak | 4 +++- 2 files changed, 64 insertions(+), 1 deletion(-) create mode 100644 arm/xzr-test.c
arm/xzr-test.c new file +61 -0
@@ -0,0 +1,61 @@ +#include <libcflat.h> +#include <chr-testdev.h> +#include <asm/setup.h> +#include <asm/smp.h> +#include <asm/mmu.h> +#include <asm/io.h> + +static void check_xzr_sysreg (void ) +{ +uint64_t val; + +flush_tlb_all (); +mmu_disable ();/* Tell KVM to set HCR_TVM for this VCPU */ + + asmvolatile ("msr ttbr0_el1, %0" : :"r" (0x5555555555555555 &PAGE_MASK )); +isb (); + asmvolatile ("mrs %0, ttbr0_el1" :"=r" (val)); +isb (); +report ("sysreg: sanity check: read 0x%016lx" , val == (0x5555555555555555 &PAGE_MASK ), val); + + asmvolatile ("msr ttbr0_el1, xzr" ); +isb (); + asmvolatile ("mrs %0, ttbr0_el1" :"=r" (val)); +isb (); +report ("sysreg: xzr check: read 0x%016lx" , val ==0 , val); + +halt (); +} + +static uint32_t *steal_mmio_addr (void ) +{ +/* +* Steal an MMIO addr from chr-testdev. Before calling exit() +* chr-testdev must be reinit. +*/ +return (uint32_t *)(0x0a003e00UL /* base */ +0x40 /* queue pfn */ ); +} + +int main (void ) +{ +volatile uint32_t *addr =steal_mmio_addr (); +uint32_t val; +long i; + +writel (0x55555555 , addr); + val =readl (addr); +report ("mmio: sanity check: read 0x%08lx" , val ==0x55555555 , val); + +mb (); + asmvolatile ("str wzr, [%0]" : :"r" (addr)); + val =readl (addr); +report ("mmio: 'str wzr' check: read 0x%08lx" , val ==0 , val); + +chr_testdev_init (); + +smp_boot_secondary (1 , check_xzr_sysreg); +for (i =0 ; i <1000000000 ; ++i) +cpu_relax (); + +return report_summary (); +}
config/config-arm64.mak +3 -1
@@ -12,9 +12,11 @@ cflatobjs += lib/arm64/processor.o cflatobjs += lib/arm64/spinlock.o # arm64 specific tests -tests = +tests = $(TEST_DIR)/xzr-test.flat include config/config-arm-common.mak arch_clean: arm_clean $(RM) lib/arm64/.*.d + +$(TEST_DIR)/xzr-test.elf: $(cstart.o) $(TEST_DIR)/xzr-test.o
Signature · 1 line
1.8.3.1
Hello!
Quoted 3 lines
FYI, I tried writing test cases for this issue with kvm-unit-tests. The issue didn't reproduce for me. It's quite possible my test cases are flawed, so I'm not making any claims about the validity of the series
This is indeed very interesting, so i'll take a look at it.
For now i've just only took a quick glance at the code, and i have at least one suggestion. Could you happen to have sp == 0 incheck_xzr_sysreg()? In this case it will magically work.
Also, you could try to write a test which tries to overwrite xzr. Something like:
volatile int *addr1;
volatile int *addr2;
asm volatile("str %3, [%1]\n\t"
"ldr wzr, [%1]\n\t"
"str wzr, [%2]\n\t",
"ldr %0, [%2]\n\t"
:"=r"(res):"r"(addr1), "r"(addr2), "r"(some_nonzero_val):"memory"); Then check for res == some_nonzero_val. If they are equal, you've got the bug :)
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
Quoted 14 lines · Pavel Fedin
Hello!
Quoted 3 lines
FYI, I tried writing test cases for this issue with kvm-unit-tests. The issue didn't reproduce for me. It's quite possible my test cases are flawed, so I'm not making any claims about the validity of the series
This is indeed very interesting, so i'll take a look at it.
For now i've just only took a quick glance at the code, and i have at least one suggestion. Could you happen to have sp == 0 incheck_xzr_sysreg()? In this case it will magically work.
Also, you could try to write a test which tries to overwrite xzr. Something like:volatile int *addr1;
volatile int *addr2;
Quoted 8 lines
asm volatile("str %3, [%1]\n\t"
"ldr wzr, [%1]\n\t" "str wzr, [%2]\n\t", "ldr %0, [%2]\n\t" :"=r"(res):"r"(addr1), "r"(addr2), "r"(some_nonzero_val):"memory");Then check for res ==
some_nonzero_val. If they are equal, you've got the bug :)
Besides the fixes mentioned in other mails, I did add this load to xzr tests too. For mmio we get the expected failure. mrs seems to work though, but maybe that's expected.
qemu-system-aarch64 -machine virt,accel=kvm -cpu host \
-device virtio-serial-device -device virtconsole,chardev=ctd \
-chardev testdev,id=ctd -display none -serial stdio \
-kernel arm/xzr-test.flat -smp 2
PASS: mmio: sanity check: read 0x55555555
FAIL: mmio: 'str wzr' check: read 0x0badc0de
FAIL: mmio: 'ldr wzr' check: read 0x0badc0de
PASS: sysreg: sp = 0x00000000401affe0
FAIL: sysreg: from xzr check: read 0xffffc0de0badc0de
PASS: sysreg: to xzr check: read 0x0000000000000000
SUMMARY: 6 tests, 3 unexpected failures
Return value from qemu: 3
Updated test attached.
drew
Quoted 1 line
From
ef5af811a72c14977e7958ee94b0c7b0fb99e6e8Mon Sep 17 00:00:00 2001
From: Andrew Jones <[email protected]>
Date: Fri, 4 Dec 2015 23:55:53 +0100
Subject: [kvm-unit-tests PATCH] arm64: add xzr emulator test
---
v2:
- added Pavel's fixes
- changed target sysreg
arm/xzr-test.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++ config/config-arm64.mak | 4 ++- 2 files changed, 92 insertions(+), 1 deletion(-) create mode 100644 arm/xzr-test.c
arm/xzr-test.c new file +89 -0
@@ -0,0 +1,89 @@ +#include <libcflat.h> +#include <chr-testdev.h> +#include <asm/setup.h> +#include <asm/smp.h> +#include <asm/mmu.h> +#include <asm/io.h> +#include <asm/thread_info.h> + + +static void check_xzr_sysreg (void ) +{ +uint64_t val; + +#if 0 +flush_tlb_all (); +mmu_disable ();/* Tell KVM to set HCR_TVM for this VCPU */ +#endif + + val = current_stack_pointer; +report ("sysreg: sp = 0x%016lx" , val !=0 , val); + + asmvolatile ("msr sp_el0, %0" : :"r" (0xdeadc0de0badc0de )); +isb (); + +#if 0 + asmvolatile ("msr ttbr0_el1, %0" : :"r" (0x5555555555555555 &PAGE_MASK )); +isb (); + asmvolatile ("mrs %0, ttbr0_el1" :"=r" (val)); +isb (); +report ("sysreg: sanity check: read 0x%016lx" , val == (0x5555555555555555 &PAGE_MASK ), val); + + asmvolatile ("msr ttbr0_el1, xzr" ); +isb (); + asmvolatile ("mrs %0, ttbr0_el1" :"=r" (val)); +isb (); +report ("sysreg: xzr check: read 0x%016lx" , val ==0 , val); +#endif + asmvolatile ("msr dbgbvr0_el1, xzr" ); +isb (); + asmvolatile ("mrs %0, dbgbvr0_el1" :"=r" (val)); +isb (); +report ("sysreg: from xzr check: read 0x%016lx" , val ==0 , val); + asmvolatile ("mrs xzr, dbgbvr0_el1" ); +isb (); + asmvolatile ("mov %0, xzr" :"=r" (val)); +report ("sysreg: to xzr check: read 0x%016lx" , val ==0 , val); + +halt (); +} + +static uint32_t *steal_mmio_addr (void ) +{ +/* +* Steal an MMIO addr from chr-testdev. Before calling exit() +* chr-testdev must be reinit. +*/ +return (uint32_t *)(0x0a003e00UL /* base */ +0x40 /* queue pfn */ ); +} + +int main (void ) +{ +volatile uint32_t *addr =steal_mmio_addr (); +uint32_t val; +long i; + + asmvolatile ("msr sp_el0, %0" : :"r" (0xdeadc0de0badc0de )); +isb (); + +writel (0x55555555 , addr); + val =readl (addr); +report ("mmio: sanity check: read 0x%08lx" , val ==0x55555555 , val); + +mb (); + asmvolatile ("str wzr, [%0]" : :"r" (addr)); + val =readl (addr); +report ("mmio: 'str wzr' check: read 0x%08lx" , val ==0 , val); +mb (); + asmvolatile ("ldr wzr, [%0]" : :"r" (addr)); +report ("mmio: 'ldr wzr' check: read 0x%08lx" , val ==0 , val); + +writel (0 , addr); +chr_testdev_init (); + +smp_boot_secondary (1 , check_xzr_sysreg); +for (i =0 ; i <1000000000 ; ++i) +cpu_relax (); + +return report_summary (); +}
config/config-arm64.mak +3 -1
@@ -12,9 +12,11 @@ cflatobjs += lib/arm64/processor.o cflatobjs += lib/arm64/spinlock.o # arm64 specific tests -tests = +tests = $(TEST_DIR)/xzr-test.flat include config/config-arm-common.mak arch_clean: arm_clean $(RM) lib/arm64/.*.d + +$(TEST_DIR)/xzr-test.elf: $(cstart.o) $(TEST_DIR)/xzr-test.o
Signature · 1 line
1.8.3.1
Quoted 33 lines · Andrew Jones
Quoted 14 lines · Pavel Fedin
Hello!
Quoted 3 lines
FYI, I tried writing test cases for this issue with kvm-unit-tests. The issue didn't reproduce for me. It's quite possible my test cases are flawed, so I'm not making any claims about the validity of the series
This is indeed very interesting, so i'll take a look at it.
For now i've just only took a quick glance at the code, and i have at least one suggestion. Could you happen to have sp == 0 incheck_xzr_sysreg()? In this case it will magically work.
Also, you could try to write a test which tries to overwrite xzr. Something like:volatile int *addr1;
volatile int *addr2;Quoted 8 lines
asm volatile("str %3, [%1]\n\t"
"ldr wzr, [%1]\n\t" "str wzr, [%2]\n\t", "ldr %0, [%2]\n\t" :"=r"(res):"r"(addr1), "r"(addr2), "r"(some_nonzero_val):"memory");Then check for res ==
some_nonzero_val. If they are equal, you've got the bug :)Besides the fixes mentioned in other mails, I did add this load to xzr tests too. For mmio we get the expected failure. mrs seems to work though, but maybe that's expected.
qemu-system-aarch64 -machine virt,accel=kvm -cpu host \
-device virtio-serial-device -device virtconsole,chardev=ctd \
-chardev testdev,id=ctd -display none -serial stdio \
-kernel arm/xzr-test.flat -smp 2
Quoted 7 lines
PASS: mmio: sanity check: read
0x55555555
FAIL: mmio: 'str wzr' check: read0x0badc0de
FAIL: mmio: 'ldr wzr' check: read0x0badc0de
PASS: sysreg: sp =0x00000000401affe0
FAIL: sysreg: from xzr check: read0xffffc0de0badc0de
PASS: sysreg: to xzr check: read0x0000000000000000
I messed up the "load into xzr" test royally in the last attached patch. It was quite wrong. I have now tested
asm volatile(
"str %3, [%1]\n\t"
"ldr wzr, [%1]\n\t"
"str wzr, [%2]\n\t"
"ldr %0, [%2]\n\t"
:"=r"(val):"r"(addr), "r"(addr2), "r"(0x55555555):"memory");report("mmio: 'ldr wzr' check: read 0x%08lx", val != 0x55555555, val);
which passes and
val = readl(addr);
printf("addr = 0x%08lx\n", val);
val = readl(addr2);
printf("addr2 = 0x%08lx\n", val);
gives
addr = 0x55555555
addr2 = 0x00000000
So it looks like we don't "change" xzr somehow with loads. Anyway, I probably won't clean this test up and post it. I don't think we really need to add it as a regression test, unless others disagree and would like to see it added.
Thanks,
drew
Hello!
Quoted 2 lines
I messed up the "load into xzr" test royally in the last attached patch. It was quite wrong.
Yes, because "mov %0, xzr" is not trapped.
Quoted 11 lines
I have now tested
asm volatile(
"str %3, [%1]\n\t" "ldr wzr, [%1]\n\t" "str wzr, [%2]\n\t" "ldr %0, [%2]\n\t" :"=r"(val):"r"(addr), "r"(addr2), "r"(0x55555555):"memory");
report("mmio: 'ldr wzr' check: read 0x%08lx", val != 0x55555555, val);which passes
I guess i forgot to mention that both addr and addr2 have to be MMIO registers. If they are plain memory, then of course everything
will work because they are not trapped.
Quoted 4 lines
Anyway, I
probably won't clean this test up and post it. I don't think we really need to add it as a regression test, unless others disagree and would like to see it added.
Considering how difficult it was to find this problem, and how tricky and unobvious it is, i would ask to add this test. Especially
considering you've already written it. At least it will serve as a reminder about the problem.
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
Quoted 17 lines · Pavel Fedin
Hello!
Quoted 2 lines
I messed up the "load into xzr" test royally in the last attached patch. It was quite wrong.
Yes, because "mov %0, xzr" is not trapped.
Quoted 10 lines
I have now tested
asm volatile(
"str %3, [%1]\n\t" "ldr wzr, [%1]\n\t" "str wzr, [%2]\n\t" "ldr %0, [%2]\n\t" :"=r"(val):"r"(addr), "r"(addr2), "r"(0x55555555):"memory");
report("mmio: 'ldr wzr' check: read 0x%08lx", val != 0x55555555, val);
Quoted 4 lines
Quoted 1 line
which passes
I guess i forgot to mention that both addr and addr2 have to be MMIO registers. If they are plain memory, then of course everything
will work because they are not trapped.
Yes, my round two (which still didn't fail) used mmio for both addr and addr2.
Quoted 8 lines
Quoted 5 lines
Anyway, I
probably won't clean this test up and post it. I don't think we really need to add it as a regression test, unless others disagree and would like to see it added.Considering how difficult it was to find this problem, and how tricky and unobvious it is, i would ask to add this test. Especially
considering you've already written it. At least it will serve as a reminder about the problem.
OK. I need to wrap up some other work right now, but then I'll clean this patch up and send it properly.
Thanks,
drew
Quoted 11 lines
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello!
Quoted 4 lines
But, if Pavel doesn't
mind trying them out on his system, then it'd be good to know if they reproduce there. I'd like to find out if it's a test case problem or something else strange going on with environments.
Does not build, applied to master:
--- cut ---
aarch64-unknown-linux-gnu-gcc -std=gnu99 -ffreestanding -Wextra -O2 -I lib -I lib/libfdt -g -MMD -MF arm/.xzr-test.d -Wall
-fomit-frame-pointer -fno-stack-protector -c -o arm/xzr-test.o arm/xzr-test.carm/xzr-test.c: In function 'check_xzr_sysreg':arm/xzr-test.c:13:2: warning: implicit declaration of function 'mmu_disable' [-Wimplicit-function-declaration]
mmu_disable(); /* Tell KVM to set HCR_TVM for this VCPU */
^
aarch64-unknown-linux-gnu-gcc -std=gnu99 -ffreestanding -Wextra -O2 -I lib -I lib/libfdt -g -MMD -MF arm/.xzr-test.d -Wall
-fomit-frame-pointer -fno-stack-protector -nostdlib -o arm/xzr-test.elf \
-Wl,-T,arm/flat.lds,--build-id=none,-Ttext=40080000 \
arm/xzr-test.o arm/cstart64.o lib/libcflat.a lib/libfdt/libfdt.a /usr/lib/gcc/aarch64-unknown-linux-gnu/4.9.0/libgcc.alib/arm/libeabi.aarm/xzr-test.o: In function `check_xzr_sysreg':
/cygdrive/d/Projects/kvm-unit-tests/arm/xzr-test.c:13: undefined reference to `mmu_disable'
--- cut ---
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
Quoted 7 lines · Pavel Fedin
Hello!
Quoted 4 lines
But, if Pavel doesn't
mind trying them out on his system, then it'd be good to know if they reproduce there. I'd like to find out if it's a test case problem or something else strange going on with environments.
Quoted 16 lines
Does not build, applied to master:
--- cut ---
aarch64-unknown-linux-gnu-gcc -std=gnu99 -ffreestanding -Wextra -O2 -I lib -I lib/libfdt -g -MMD -MF arm/.xzr-test.d -Wall
-fomit-frame-pointer -fno-stack-protector -c -oarm/xzr-test.oarm/xzr-test.carm/xzr-test.c: In function 'check_xzr_sysreg':arm/xzr-test.c:13:2: warning: implicit declaration of function 'mmu_disable' [-Wimplicit-function-declaration]
mmu_disable(); /* Tell KVM to setHCR_TVMfor this VCPU */
^
aarch64-unknown-linux-gnu-gcc -std=gnu99 -ffreestanding -Wextra -O2 -I lib -I lib/libfdt -g -MMD -MF arm/.xzr-test.d -Wall
-fomit-frame-pointer -fno-stack-protector -nostdlib -o arm/xzr-test.elf \
-Wl,-T,arm/flat.lds,--build-id=none,-Ttext=40080000 \
arm/xzr-test.oarm/cstart64.olib/libcflat.alib/libfdt/libfdt.a/usr/lib/gcc/aarch64-unknown-linux-gnu/4.9.0/libgcc.alib/arm/libeabi.aarm/xzr-test.o: In function `check_xzr_sysreg':
/cygdrive/d/Projects/kvm-unit-tests/arm/xzr-test.c:13: undefined reference to `mmu_disable'
--- cut ---
Have you done a git pull of your kvm-unit-tests repo lately? The patch that introduces mmu_disable was commit a few months ago or so. Other than your repo just not having mmu_disable(), then I can't think of why it compiles for me and not you. If you have done a recent git pull, then maybe do a 'make distclean; ./configure; make'
Thanks,
drew
Quoted 11 lines
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello!
Quoted 3 lines
FYI, I tried writing test cases for this issue with kvm-unit-tests. The issue didn't reproduce for me. It's quite possible my test cases are flawed
Indeed they are, a very little thing fell through again... :)
It's not just SP, it's SP_EL0. And you never initialize it to anything because your code always runs in kernel mode, so it's just
zero, so you get your zero.
But if you add a little thing in the beginning of your main():
asm volatile("msr sp_el0, %0" : : "r" (0xDEADC0DE0BADC0DE));
then you have it:
--- cut ---
[root@thunderx-2 kvm-unit-tests]# ./arm-run arm/xzr-test.flat -smp 2
qemu-system-aarch64 -machine virt,accel=kvm:tcg,gic-version=host -cpu host -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd -display none -serial stdio -kernel arm/xzr-test.flat -smp 2
PASS: mmio: sanity check: read 0x55555555
FAIL: mmio: 'str wzr' check: read 0x0badc0devm_setup_vq: virtqueue 0 already setup! base=0xa003e00chr_testdev_init: chr-testdev: can't init virtqueues
--- cut ---
Here i run only MMIO test, because i could not compile sysreg one, so i simply commented it out.
P.S. Could you also apply something like the following to arm/run:
--- cut ---
arm/run | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
arm/run +5 -2
@@ -33,7 +33,11 @@ if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \ exit 2 fi -M='-machine virt,accel=kvm:tcg' +if $qemu $M,? 2>&1 | grep gic-version > /dev/null; then + GIC='gic-version=host,' +fi + +M="-machine virt,${GIC}accel=kvm:tcg" chr_testdev='-device virtio-serial-device' chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd' --- cut ---
Without it qemu does not work on GICv3-only hardware, like my board, because it defaults to gic-version=2. I don't post the patch
on the mailing lists, because in order to be able to post this 5-liner i'll need to go through the formal approval procedure at my company, and i just don't want to bother for a single small fix. :) Will do as a "Reported-by:".
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
Quoted 6 lines · Pavel Fedin
Hello!
Quoted 3 lines
FYI, I tried writing test cases for this issue with kvm-unit-tests. The issue didn't reproduce for me. It's quite possible my test cases are flawed
Quoted 6 lines
Indeed they are, a very little thing fell through again... :)
It's not just SP, it'sSP_EL0. And you never initialize it to anything because your code always runs in kernel mode, so it's just
zero, so you get your zero.
But if you add a little thing in the beginning of yourmain():asm
volatile("msr sp_el0, %0" : : "r" (0xDEADC0DE0BADC0DE));
Ah! Thanks for this. The mmio test does now fail for me too. The sysreg test still doesn't fail for me (even though I'm doing the above on the vcpu I use for that too). Maybe there's something weird with which reg I'm using, and whether or not my attempt to get trapping enabled on it is working the way I expected. I'll play with it some more.
Quoted 26 lines
then you have it:
--- cut ---
[root@thunderx-2 kvm-unit-tests]# ./arm-run arm/xzr-test.flat -smp 2
qemu-system-aarch64 -machine virt,accel=kvm:tcg,gic-version=host -cpu host -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd -display none -serial stdio -kernel arm/xzr-test.flat -smp 2
PASS: mmio: sanity check: read0x55555555
FAIL: mmio: 'str wzr' check: read0x0badc0devm_setup_vq: virtqueue 0 already setup! base=0xa003e00chr_testdev_init: chr-testdev: can't init virtqueues
--- cut ---Here i run only MMIO test, because i could not compile sysreg one, so i simply commented it out.
P.S. Could you also apply something like the following to arm/run:
--- cut ---
arm/run | 6 +++++-1 file changed, 5 insertions(+), 1 deletion(-)arm/run +0
-0@@ -33,7 +33,11 @@ if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \ exit 2 fi
arm/run if $qemu $M -chardev testdev,id=id -initrd . 2>&1 \ · line 39
Quoted 10 lines
arm/run +5
-2-M='-machine virt,accel=kvm:tcg' +if $qemu $M,? 2>&1 | grep gic-version > /dev/null; then + GIC='gic-version=host,' +fi + +M="-machine virt,${GIC}accel=kvm:tcg" chr_testdev='-device virtio-serial-device' chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd' --- cut ---
Yes, I'll send a patch for this soon. I actually have something similar to this in my local tree already, I just hadn't bothered sending it as I didn't think anybody else needed it yet.
Quoted 4 lines
Without it qemu does not work on GICv3-only hardware, like my board, because it defaults to gic-version=2. I don't post the patch
on the mailing lists, because in order to be able to post this 5-liner i'll need to go through the formal approval procedure at my company, and i just don't want to bother for a single small fix. :) Will do as a "Reported-by:".
It'd be nice if you could go through the procedure. You've been sending patches to KVM, and ideally we'll start trying to send kvm-unit-tests patches along with feature patches.
Thanks,
drew
Quoted 12 lines · Andrew Jones
Quoted 11 lines · Pavel Fedin
Hello!
Quoted 3 lines
FYI, I tried writing test cases for this issue with kvm-unit-tests. The issue didn't reproduce for me. It's quite possible my test cases are flawed
Indeed they are, a very little thing fell through again... :)
It's not just SP, it'sSP_EL0. And you never initialize it to anything because your code always runs in kernel mode, so it's just
zero, so you get your zero.
But if you add a little thing in the beginning of yourmain():
Quoted 8 lines
Quoted 1 line
asm
volatile("msr sp_el0, %0" : : "r" (0xDEADC0DE0BADC0DE));Ah! Thanks for this. The mmio test does now fail for me too. The sysreg test still doesn't fail for me (even though I'm doing the above on the vcpu I use for that too). Maybe there's something weird with which reg I'm using, and whether or not my attempt to get trapping enabled on it is working the way I expected. I'll play with it some more.
Must be the trapping thing. I switched to dbgbvr0_el1, which has trapping enabled on it until it's touched, and was able the reproduce the xzr issue it.
drew