KVM: arm64: Limit feature register reads from AArch32
v2 cover KVM: arm64: Limit feature register reads from AArch32
KVM/arm64 does not restrict the guest's view of the AArch32 feature registers when read from AArch32. HCR_EL2.TID3 is cleared for AArch32 guests, meaning that register reads come straight from hardware. This is problematic as KVM relies on read_sanitised_ftr_reg() to expose a set of features consistent for a particular system.
Appropriate handlers must first be put in place for CP10 and CP15 ID register accesses before setting TID3. Rather than exhaustively enumerating each of the encodings for CP10 and CP15 registers, take the lazy route and aim the register accesses at the AArch64 system register table.
Patch 1 reroutes the CP15 registers into the AArch64 table, taking care to immediately RAZ undefined ranges of registers. This is done to avoid possibly conflicting with encodings for future AArch64 registers.
Patch 2 installs an exit handler for the CP10 ID registers and also relies on the general AArch64 register handler to implement reads.
Finally, patch 3 actually sets TID3 for AArch32 guests, providing known-safe values for feature register accesses.
Series applies cleanly to kvmarm/fixes at commit:
8872d9b3e35a ("KVM: arm64: Drop unneeded minor version check from PSCI v1.x handler")
There is an argument that the series is in fact a bug fix for running AArch32 VMs on heterogeneous systems. To that end, it could be blamed/backported to when we first knew better:
93390c0a1b20 ("arm64: KVM: Hide unsupported AArch64 CPU features from guests")
But I left that tag off as in the aforementioned change skipping AArch32 was intentional.
Tested with AArch32 kvm-unit-tests and booting an AArch32 debian guest on a Raspberry Pi 4.
v1: https://lore.kernel.org/kvmarm/[email protected]/
v1 -> v2:
- Actually set TID3! Oops.
- Refactor kvm_emulate_cp15_id_reg() to check preconditions before proceeding to emulation (Reiji)
- Tighten up comment on kvm_is_cp15_id_reg() to indicate that the only other trapped ID register (CTR) is already handled in the cp15 register table (Reiji)
Oliver Upton (3):
KVM: arm64: Wire up CP15 feature registers to their AArch64
equivalents
KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler
KVM: arm64: Start trapping ID registers for 32 bit guests
arch/arm64/include/asm/kvm_arm.h | 3 +- arch/arm64/include/asm/kvm_emulate.h | 8 -- arch/arm64/include/asm/kvm_host.h | 1 + arch/arm64/kvm/handle_exit.c | 1 + arch/arm64/kvm/sys_regs.c | 129 +++++++++++++++++++++++++++ 5 files changed, 133 insertions(+), 9 deletions(-)
Signature · 1 line
2.35.1.1094.g7c7d902a7c-goog
PATCH v2 1/3 KVM: arm64: Wire up CP15 feature registers to their AArch64 equivalents
KVM currently does not trap ID register accesses from an AArch32 EL1. This is painful for a couple of reasons. Certain unimplemented features are visible to AArch32 EL1, as we limit PMU to version 3 and the debug architecture to v8.0. Additionally, we attempt to paper over heterogeneous systems by using register values that are safe system-wide. All this hard work is completely sidestepped because KVM does not set TID3 for AArch32 guests.
Fix up handling of CP15 feature registers by simply rerouting to their AArch64 aliases. Punt setting HCR_EL2.TID3 to a later change, as we need to fix up the oddball CP10 feature registers still.
--- arch/arm64/kvm/sys_regs.c | 68 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+)
arch/arm64/kvm/sys_regs.c +68 -0
@@ -2339,6 +2339,67 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,return 1 ; } +static int emulate_sys_reg (struct kvm_vcpu *vcpu,struct sys_reg_params *params); + +/** +* kvm_emulate_cp15_id_reg() - Handles an MRC trap on a guest CP15 access where +* CRn=0, which corresponds to the AArch32 feature +* registers. +* @vcpu: the vCPU pointer +* @params: the system register access parameters. +* +* Our cp15 system register tables do not enumerate the AArch32 feature +* registers. Conveniently, our AArch64 table does, and the AArch32 system +* register encoding can be trivially remapped into the AArch64 for the feature +* registers: Append op0=3, leaving op1, CRn, CRm, and op2 the same. +* +* According to DDI0487G.b G7.3.1, paragraph "Behavior of VMSAv8-32 32-bit +* System registers with (coproc=0b1111, CRn==c0)", read accesses from this +* range are either UNKNOWN or RES0. Rerouting remains architectural as we +* treat undefined registers in this range as RAZ. +*/ +static int kvm_emulate_cp15_id_reg (struct kvm_vcpu *vcpu, +struct sys_reg_params *params) +{ +int Rt =kvm_vcpu_sys_get_rt (vcpu); +int ret =1 ; + +/* Treat impossible writes to RO registers as UNDEFINED */ +if (params->is_write) { +unhandled_cp_access (vcpu, params); +return 1 ; + } + + params->Op0 =3 ; + +/* +* All registers where CRm > 3 are known to be UNKNOWN/RAZ from AArch32. +* Avoid conflicting with future expansion of AArch64 feature registers +* and simply treat them as RAZ here. +*/ +if (params->CRm >3 ) + params->regval =0 ; +else + ret =emulate_sys_reg (vcpu, params); + +vcpu_set_reg (vcpu, Rt, params->regval); +return ret; +} + +/** +* kvm_is_cp15_id_reg() - Returns true if the specified CP15 register is an +* AArch32 ID register. +* @params: the system register access parameters +* +* Note that CP15 ID registers where CRm=0 are excluded from this check. The +* only register trapped in the CRm=0 range is CTR, which is already handled in +* the cp15 register table. +*/ +static inline bool kvm_is_cp15_id_reg (struct sys_reg_params *params) +{ +return params->CRn ==0 && params->Op1 ==0 && params->CRm !=0 ; +} +/** * kvm_handle_cp_32 -- handles a mrc/mcr trap on a guest CP14/CP15 access * @vcpu: The VCPU pointer
@@ -2360,6 +2421,13 @@ static int kvm_handle_cp_32(struct kvm_vcpu *vcpu, params.Op1 = (esr >>14 ) &0x7 ; params.Op2 = (esr >>17 ) &0x7 ; +/* +* Certain AArch32 ID registers are handled by rerouting to the AArch64 +* system register table. +*/ +if (ESR_ELx_EC (esr) == ESR_ELx_EC_CP15_32 &&kvm_is_cp15_id_reg (¶ms)) +return kvm_emulate_cp15_id_reg (vcpu, ¶ms); +if (!emulate_cp (vcpu, ¶ms, global, nr_global)) {if (!params.is_write)vcpu_set_reg (vcpu, Rt, params.regval);
Signature · 6 lines
2.35.1.1094.g7c7d902a7c-goog _______________________________________________ kvmarm mailing list [email protected] https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 1/3] KVM: arm64: Wire up CP15 feature registers to their AArch64 equivalents
Quoted 9 lines · Oliver Upton
KVM currently does not trap ID register accesses from an AArch32 EL1. This is painful for a couple of reasons. Certain unimplemented features are visible to AArch32 EL1, as we limit PMU to version 3 and the debug architecture to v8.0. Additionally, we attempt to paper over heterogeneous systems by using register values that are safe system-wide. All this hard work is completely sidestepped because KVM does not set TID3 for AArch32 guests.
Quoted 5 lines
Fix up handling of CP15 feature registers by simply rerouting to their AArch64 aliases. Punt setting HCR_EL2.TID3 to a later change, as we need to fix up the oddball CP10 feature registers still.
Signed-off-by Oliver Upton <[email protected]>
Marc Zyngier
maintainerRe: [PATCH v2 1/3] KVM: arm64: Wire up CP15 feature registers to their AArch64 equivalents
In reply to Oliver UptonQuoted 60 lines · Oliver Upton
KVM currently does not trap ID register accesses from an AArch32 EL1. This is painful for a couple of reasons. Certain unimplemented features are visible to AArch32 EL1, as we limit PMU to version 3 and the debug architecture to v8.0. Additionally, we attempt to paper over heterogeneous systems by using register values that are safe system-wide. All this hard work is completely sidestepped because KVM does not set TID3 for AArch32 guests.
Fix up handling of CP15 feature registers by simply rerouting to their AArch64 aliases. Punt setting HCR_EL2.TID3 to a later change, as we need to fix up the oddball CP10 feature registers still.
Signed-off-by Oliver Upton <[email protected]>--- arch/arm64/kvm/sys_regs.c | 68 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+)arch/arm64/kvm/sys_regs.c +34
-0@@ -2339,6 +2339,67 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,return 1 ; } +static int emulate_sys_reg (struct kvm_vcpu *vcpu,struct sys_reg_params *params); + +/** +* kvm_emulate_cp15_id_reg() - Handles an MRC trap on a guest CP15 access where +* CRn=0, which corresponds to the AArch32 feature +* registers. +* @vcpu: the vCPU pointer +* @params: the system register access parameters. +* +* Our cp15 system register tables do not enumerate the AArch32 feature +* registers. Conveniently, our AArch64 table does, and the AArch32 system +* register encoding can be trivially remapped into the AArch64 for the feature +* registers: Append op0=3, leaving op1, CRn, CRm, and op2 the same. +* +* According to DDI0487G.b G7.3.1, paragraph "Behavior of VMSAv8-32 32-bit +* System registers with (coproc=0b1111, CRn==c0)", read accesses from this +* range are either UNKNOWN or RES0. Rerouting remains architectural as we +* treat undefined registers in this range as RAZ. +*/ +static int kvm_emulate_cp15_id_reg (struct kvm_vcpu *vcpu, +struct sys_reg_params *params) +{ +int Rt =kvm_vcpu_sys_get_rt (vcpu); +int ret =1 ; + +/* Treat impossible writes to RO registers as UNDEFINED */ +if (params->is_write) { +unhandled_cp_access (vcpu, params); +return 1 ; + } + + params->Op0 =3 ; + +/*
arch/arm64/kvm/sys_regs.c static int kvm_handle_cp_64(struct kvm_vcpu *vcpu, · line 2385
Quoted 10 lines
arch/arm64/kvm/sys_regs.c +10
-0+* All registers where CRm > 3 are known to be UNKNOWN/RAZ from AArch32. +* Avoid conflicting with future expansion of AArch64 feature registers +* and simply treat them as RAZ here. +*/ +if (params->CRm >3 ) + params->regval =0 ; +else + ret =emulate_sys_reg (vcpu, params); + +vcpu_set_reg (vcpu, Rt, params->regval);
It feels odd to update Rt without checking whether the read has succeeded. In your case, this is harmless, but would break with the approach I'm outlining below.
arch/arm64/kvm/sys_regs.c static int kvm_handle_cp_64(struct kvm_vcpu *vcpu, · line 2396
Quoted 11 lines
arch/arm64/kvm/sys_regs.c +11
-0+return ret; +} + +/** +* kvm_is_cp15_id_reg() - Returns true if the specified CP15 register is an +* AArch32 ID register. +* @params: the system register access parameters +* +* Note that CP15 ID registers where CRm=0 are excluded from this check. The +* only register trapped in the CRm=0 range is CTR, which is already handled in +* the cp15 register table.
There is also the fact that CTR_EL0 has Op1=3 while CTR has Op1=0, which prevents it from fitting in your scheme.
Quoted 10 lines
arch/arm64/kvm/sys_regs.c +6
-0+*/ +static inline bool kvm_is_cp15_id_reg (struct sys_reg_params *params) +{ +return params->CRn ==0 && params->Op1 ==0 && params->CRm !=0 ; +} +/** * kvm_handle_cp_32 -- handles a mrc/mcr trap on a guest CP14/CP15 access * @vcpu: The VCPU pointer
arch/arm64/kvm/sys_regs.c static int kvm_handle_cp_32(struct kvm_vcpu *vcpu, · line 2429
Quoted 9 lines
arch/arm64/kvm/sys_regs.c +6
-0@@ -2360,6 +2421,13 @@ static int kvm_handle_cp_32(struct kvm_vcpu *vcpu, params.Op1 = (esr >>14 ) &0x7 ; params.Op2 = (esr >>17 ) &0x7 ; +/* +* Certain AArch32 ID registers are handled by rerouting to the AArch64 +* system register table. +*/ +if (ESR_ELx_EC (esr) == ESR_ELx_EC_CP15_32 &&kvm_is_cp15_id_reg (¶ms)) +return kvm_emulate_cp15_id_reg (vcpu, ¶ms);
I think this is a bit ugly. We reach this point from a function that was cp15-specific, and now we are reconstructing the context. I'd rather this is moved to kvm_handle_cp15_32(), and treated there (untested):
arch/arm64/kvm/sys_regs.c +19 -15
@@ -2350,28 +2350,21 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,* @run: The kvm_run struct */ static int kvm_handle_cp_32 (struct kvm_vcpu *vcpu, +struct sys_reg_params *params,const struct sys_reg_desc *global,size_t nr_global) { -struct sys_reg_params params; -u32 esr =kvm_vcpu_get_esr (vcpu);int Rt =kvm_vcpu_sys_get_rt (vcpu); - params.CRm = (esr >>1 ) &0xf ; - params.regval =vcpu_get_reg (vcpu, Rt); - params.is_write = ((esr &1 ) ==0 ); - params.CRn = (esr >>10 ) &0xf ; - params.Op0 =0 ; - params.Op1 = (esr >>14 ) &0x7 ; - params.Op2 = (esr >>17 ) &0x7 ; + params->regval =vcpu_get_reg (vcpu, Rt); -if (!emulate_cp (vcpu, ¶ms, global, nr_global)) { -if (!params.is_write) -vcpu_set_reg (vcpu, Rt, params.regval); +if (!emulate_cp (vcpu, params, global, nr_global)) { +if (!params->is_write) +vcpu_set_reg (vcpu, Rt, params->regval);return 1 ; } -unhandled_cp_access (vcpu, ¶ms); +unhandled_cp_access (vcpu, params);return 1 ; }
@@ -2382,7 +2375,14 @@ int kvm_handle_cp15_64(struct kvm_vcpu *vcpu)int kvm_handle_cp15_32 (struct kvm_vcpu *vcpu) { -return kvm_handle_cp_32 (vcpu, cp15_regs,ARRAY_SIZE (cp15_regs)); +struct sys_reg_params params; + + params =esr_cp1x_32_to_params (kvm_vcpu_get_esr (vcpu)); + +if (params.Op1 ==0 && params.CRn ==0 && params.CRm) +return kvm_emulate_cp15_id_reg (vcpu, ¶ms); + +return kvm_handle_cp_32 (vcpu, ¶ms, cp15_regs,ARRAY_SIZE (cp15_regs)); }int kvm_handle_cp14_64 (struct kvm_vcpu *vcpu)
@@ -2392,7 +2392,11 @@ int kvm_handle_cp14_64(struct kvm_vcpu *vcpu)int kvm_handle_cp14_32 (struct kvm_vcpu *vcpu) { -return kvm_handle_cp_32 (vcpu, cp14_regs,ARRAY_SIZE (cp14_regs)); +struct sys_reg_params params; + + params =esr_cp1x_32_to_params (kvm_vcpu_get_esr (vcpu)); + +return kvm_handle_cp_32 (vcpu, ¶ms, cp14_regs,ARRAY_SIZE (cp14_regs)); }static bool is_imp_def_sys_reg (struct sys_reg_params *params)
arch/arm64/kvm/sys_regs.h +7 -0
@@ -35,6 +35,13 @@ struct sys_reg_params { .Op2 = ((esr) >> 17) & 0x7, \ .is_write = !((esr) & 1) }) +#define esr_cp1x_32_to_params (esr) \ + ((struct sys_reg_params){ .Op1 = ((esr) >> 14) & 0x7, \ + .CRn = ((esr) >> 10) & 0xf, \ + .CRm = ((esr) >> 1) & 0xf, \ + .Op2 = ((esr) >> 17) & 0x7, \ + .is_write = !((esr) & 1) }) +struct sys_reg_desc {/* Sysreg string for debug */ const char *name;
What do you think?
M.
Signature · 5 lines
Without deviation from the norm, progress is not possible. _______________________________________________ kvmarm mailing list [email protected] https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 1/3] KVM: arm64: Wire up CP15 feature registers to their AArch64 equivalents
Hi Marc,
Quoted 7 lines · Marc Zyngier
Quoted 7 lines
(quoted diff) +7
-0+ /* + * All registers where CRm > 3 are known to be UNKNOWN/RAZ from AArch32. + * Avoid conflicting with future expansion of AArch64 feature registers + * and simply treat them as RAZ here. + */ + if (params->CRm > 3) + params->regval = 0;
Quoted 9 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+ else + ret = emulate_sys_reg(vcpu, params); + + vcpu_set_reg(vcpu, Rt, params->regval);It feels odd to update Rt without checking whether the read has succeeded. In your case, this is harmless, but would break with the approach I'm outlining below.
A total kludge to avoid yet another level of indentation :) I'll go about this the right way next spin.
Quoted 28 lines
Review
Quoted 11 lines
(quoted diff) +11
-0+ return ret; +} + +/** + * kvm_is_cp15_id_reg() - Returns true if the specified CP15 register is an + * AArch32 ID register. + * @params: the system register access parameters + * + * Note that CP15 ID registers where CRm=0 are excluded from this check. The + * only register trapped in the CRm=0 range is CTR, which is already handled in + * the cp15 register table.There is also the fact that
CTR_EL0has Op1=3 while CTR has Op1=0, which prevents it from fitting in your scheme.Quoted 13 lines
+ */
(quoted diff) +5
-0+static inline bool kvm_is_cp15_id_reg(struct sys_reg_params *params) +{ + return params->CRn == 0 && params->Op1 == 0 && params->CRm != 0; +} + /** * kvm_handle_cp_32 -- handles a mrc/mcr trap on a guest CP14/CP15 access * @vcpu: The VCPU pointer@@ -2360,6 +2421,13 @@ static int kvm_handle_cp_32(struct kvm_vcpu *vcpu, params.Op1 = (esr >> 14) & 0x7; params.Op2 = (esr >> 17) & 0x7;
Quoted 12 lines
Review static int kvm_handle_cp_32(struct kvm_vcpu *vcpu, · line 2429
Quoted 6 lines
(quoted diff) +6
-0+ /* + * Certain AArch32 ID registers are handled by rerouting to the AArch64 + * system register table. + */ + if (ESR_ELx_EC(esr) == ESR_ELx_EC_CP15_32 && kvm_is_cp15_id_reg(¶ms)) + return kvm_emulate_cp15_id_reg(vcpu, ¶ms);I think this is a bit ugly. We reach this point from a function that was cp15-specific, and now we are reconstructing the context. I'd rather this is moved to
kvm_handle_cp15_32(), and treated there (untested):
Completely agree, hoisting this would be much more elegant.
Quoted 88 lines
arch/arm64/kvm/sys_regs.c +19
-15@@ -2350,28 +2350,21 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,* @run: The kvm_run struct */ static int kvm_handle_cp_32 (struct kvm_vcpu *vcpu, +struct sys_reg_params *params,const struct sys_reg_desc *global,size_t nr_global) { -struct sys_reg_params params; -u32 esr =kvm_vcpu_get_esr (vcpu);int Rt =kvm_vcpu_sys_get_rt (vcpu); - params.CRm = (esr >>1 ) &0xf ; - params.regval =vcpu_get_reg (vcpu, Rt); - params.is_write = ((esr &1 ) ==0 ); - params.CRn = (esr >>10 ) &0xf ; - params.Op0 =0 ; - params.Op1 = (esr >>14 ) &0x7 ; - params.Op2 = (esr >>17 ) &0x7 ; + params->regval =vcpu_get_reg (vcpu, Rt); -if (!emulate_cp (vcpu, ¶ms, global, nr_global)) { -if (!params.is_write) -vcpu_set_reg (vcpu, Rt, params.regval); +if (!emulate_cp (vcpu, params, global, nr_global)) { +if (!params->is_write) +vcpu_set_reg (vcpu, Rt, params->regval);return 1 ; } -unhandled_cp_access (vcpu, ¶ms); +unhandled_cp_access (vcpu, params);return 1 ; }@@ -2382,7 +2375,14 @@ int kvm_handle_cp15_64(struct kvm_vcpu *vcpu)int kvm_handle_cp15_32 (struct kvm_vcpu *vcpu) { -return kvm_handle_cp_32 (vcpu, cp15_regs,ARRAY_SIZE (cp15_regs)); +struct sys_reg_params params; + + params =esr_cp1x_32_to_params (kvm_vcpu_get_esr (vcpu)); + +if (params.Op1 ==0 && params.CRn ==0 && params.CRm) +return kvm_emulate_cp15_id_reg (vcpu, ¶ms); + +return kvm_handle_cp_32 (vcpu, ¶ms, cp15_regs,ARRAY_SIZE (cp15_regs)); }int kvm_handle_cp14_64 (struct kvm_vcpu *vcpu)@@ -2392,7 +2392,11 @@ int kvm_handle_cp14_64(struct kvm_vcpu *vcpu)int kvm_handle_cp14_32 (struct kvm_vcpu *vcpu) { -return kvm_handle_cp_32 (vcpu, cp14_regs,ARRAY_SIZE (cp14_regs)); +struct sys_reg_params params; + + params =esr_cp1x_32_to_params (kvm_vcpu_get_esr (vcpu)); + +return kvm_handle_cp_32 (vcpu, ¶ms, cp14_regs,ARRAY_SIZE (cp14_regs)); }static bool is_imp_def_sys_reg (struct sys_reg_params *params)arch/arm64/kvm/sys_regs.h +7
-0@@ -35,6 +35,13 @@ struct sys_reg_params { .Op2 = ((esr) >> 17) & 0x7, \ .is_write = !((esr) & 1) }) +#define esr_cp1x_32_to_params (esr) \ + ((struct sys_reg_params){ .Op1 = ((esr) >> 14) & 0x7, \ + .CRn = ((esr) >> 10) & 0xf, \ + .CRm = ((esr) >> 1) & 0xf, \ + .Op2 = ((esr) >> 17) & 0x7, \ + .is_write = !((esr) & 1) }) +struct sys_reg_desc {/* Sysreg string for debug */ const char *name;
Quoted 1 line
What do you think?
Way better. Your suggested patch looks correct, I'll fold all of this together and test it out. Thanks for the suggestions :)
--
Best,
Oliver
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
PATCH v2 2/3 KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler
In reply to Oliver UptonIn order to enable HCR_EL2.TID3 for AArch32 guests KVM needs to handle traps where ESR_EL2.EC=0x8, which corresponds to an attempted VMRS access from an ID group register. Specifically, the MVFR{0-2} registers are accessed this way from AArch32. Conveniently, these registers are architecturally mapped to MVFR{0-2}_EL1 in AArch64. Furthermore, KVM already handles reads to these aliases in AArch64.
Plumb VMRS read traps through to the general AArch64 system register handler.
--- arch/arm64/include/asm/kvm_host.h | 1 + arch/arm64/kvm/handle_exit.c | 1 + arch/arm64/kvm/sys_regs.c | 61 +++++++++++++++++++++++++++++++ 3 files changed, 63 insertions(+)
arch/arm64/include/asm/kvm_host.h +1 -0
@@ -673,6 +673,7 @@ int kvm_handle_cp14_64(struct kvm_vcpu *vcpu);int kvm_handle_cp15_32 (struct kvm_vcpu *vcpu);int kvm_handle_cp15_64 (struct kvm_vcpu *vcpu);int kvm_handle_sys_reg (struct kvm_vcpu *vcpu); +int kvm_handle_cp10_id (struct kvm_vcpu *vcpu);void kvm_reset_sys_regs (struct kvm_vcpu *vcpu);
arch/arm64/kvm/handle_exit.c +1 -0
@@ -167,6 +167,7 @@ static exit_handle_fn arm_exit_handlers[] = { [ESR_ELx_EC_CP15_64] = kvm_handle_cp15_64, [ESR_ELx_EC_CP14_MR] = kvm_handle_cp14_32, [ESR_ELx_EC_CP14_LS] = kvm_handle_cp14_load_store, + [ESR_ELx_EC_CP10_ID] = kvm_handle_cp10_id, [ESR_ELx_EC_CP14_64] = kvm_handle_cp14_64, [ESR_ELx_EC_HVC32] = handle_hvc, [ESR_ELx_EC_SMC32] = handle_smc,
arch/arm64/kvm/sys_regs.c +61 -0
67 lines not on this page · raw message
Signature · 1 line
2.35.1.1094.g7c7d902a7c-goog
Re: [PATCH v2 2/3] KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler
Quoted 92 lines · Oliver Upton
In order to enable HCR_EL2.TID3 for AArch32 guests KVM needs to handle traps where ESR_EL2.EC=0x8, which corresponds to an attempted VMRS access from an ID group register. Specifically, the MVFR{0-2} registers are accessed this way from AArch32. Conveniently, these registers are architecturally mapped to MVFR{0-2}_EL1 in AArch64. Furthermore, KVM already handles reads to these aliases in AArch64.
Plumb VMRS read traps through to the general AArch64 system register handler.
Signed-off-by Oliver Upton <[email protected]>--- arch/arm64/include/asm/kvm_host.h | 1 + arch/arm64/kvm/handle_exit.c | 1 + arch/arm64/kvm/sys_regs.c | 61 +++++++++++++++++++++++++++++++ 3 files changed, 63 insertions(+)arch/arm64/include/asm/kvm_host.h +1
-0@@ -673,6 +673,7 @@ int kvm_handle_cp14_64(struct kvm_vcpu *vcpu);int kvm_handle_cp15_32 (struct kvm_vcpu *vcpu);int kvm_handle_cp15_64 (struct kvm_vcpu *vcpu);int kvm_handle_sys_reg (struct kvm_vcpu *vcpu); +int kvm_handle_cp10_id (struct kvm_vcpu *vcpu);void kvm_reset_sys_regs (struct kvm_vcpu *vcpu);arch/arm64/kvm/handle_exit.c +1
-0@@ -167,6 +167,7 @@ static exit_handle_fn arm_exit_handlers[] = { [ESR_ELx_EC_CP15_64] = kvm_handle_cp15_64, [ESR_ELx_EC_CP14_MR] = kvm_handle_cp14_32, [ESR_ELx_EC_CP14_LS] = kvm_handle_cp14_load_store, + [ESR_ELx_EC_CP10_ID] = kvm_handle_cp10_id, [ESR_ELx_EC_CP14_64] = kvm_handle_cp14_64, [ESR_ELx_EC_HVC32] = handle_hvc, [ESR_ELx_EC_SMC32] = handle_smc,arch/arm64/kvm/sys_regs.c +42
-0@@ -2341,6 +2341,67 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,static int emulate_sys_reg (struct kvm_vcpu *vcpu,struct sys_reg_params *params); +/* +* The CP10 ID registers are architecturally mapped to AArch64 feature +* registers. Abuse that fact so we can rely on the AArch64 handler for accesses +* from AArch32. +*/ +static bool kvm_esr_cp10_id_to_sys64 (u32 esr,struct sys_reg_params *params) +{ + params->is_write = ((esr &1 ) ==0 ); + params->Op0 =3 ; + params->Op1 =0 ; + params->CRn =0 ; + params->CRm =3 ; + +switch ((esr >>10 ) &0xf ) { +/* MVFR0 */ +case 0b0111 : + params->Op2 =0 ; +break ; +/* MVFR1 */ +case 0b0110 : + params->Op2 =1 ; +break ; +/* MVFR2 */ +case 0b0101 : + params->Op2 =2 ; +break ; +default : +return false; + } + +return true; +} + +/** +* kvm_handle_cp10_id() - Handles a VMRS trap on guest access to a 'Media and +* VFP Register' from AArch32. +* @vcpu: The vCPU pointer +* +* MVFR{0-2} are architecturally mapped to the AArch64 MVFR{0-2}_EL1 registers. +* Work out the correct AArch64 system register encoding and reroute to the +* AArch64 system register emulation. +*/
arch/arm64/kvm/sys_regs.c static int kvm_handle_cp_64(struct kvm_vcpu *vcpu, · line 2395
Quoted 10 lines
arch/arm64/kvm/sys_regs.c +10
-0+int kvm_handle_cp10_id (struct kvm_vcpu *vcpu) +{ +int Rt =kvm_vcpu_sys_get_rt (vcpu); +u32 esr =kvm_vcpu_get_esr (vcpu); +struct sys_reg_params params; +int ret; + +/* UNDEF on any unhandled register or an attempted write */ +if (!kvm_esr_cp10_id_to_sys64 (esr, ¶ms) || params.is_write) { +kvm_inject_undefined (vcpu);
Nit: For debugging, it might be more useful to use unhandled_cp_access() (, which needs to be changed to support ESR_ELx_EC_CP10_ID though) rather than directly calling kvm_inject_undefined().
Thanks,
Reiji
Quoted 13 lines
arch/arm64/kvm/sys_regs.c +9
-1+return 1 ; + } + + ret =emulate_sys_reg (vcpu, ¶ms); + +vcpu_set_reg (vcpu, Rt, params.regval); +return ret; +} +/** * kvm_emulate_cp15_id_reg() - Handles an MRC trap on a guest CP15 access where * CRn=0, which corresponds to the AArch32 feature --
Quoted 2 lines
2.35.1.1094.g7c7d902a7c-goog
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 2/3] KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler
Hi Reiji,
Quoted 6 lines · Reiji Watanabe
Quoted 6 lines
arch/arm64/kvm/sys_regs.c +6
-0+int kvm_handle_cp10_id (struct kvm_vcpu *vcpu) +{ +int Rt =kvm_vcpu_sys_get_rt (vcpu); +u32 esr =kvm_vcpu_get_esr (vcpu); +struct sys_reg_params params; +int ret;
Quoted 8 lines
arch/arm64/kvm/sys_regs.c static int kvm_handle_cp_64(struct kvm_vcpu *vcpu, · line 2395
Quoted 4 lines
arch/arm64/kvm/sys_regs.c +4
-0+ +/* UNDEF on any unhandled register or an attempted write */ +if (!kvm_esr_cp10_id_to_sys64 (esr, ¶ms) || params.is_write) { +kvm_inject_undefined (vcpu);Nit: For debugging, it might be more useful to use
unhandled_cp_access()(, which needs to be changed to supportESR_ELx_EC_CP10_IDthough) rather than directly callingkvm_inject_undefined().
A very worthy nit, you spotted my laziness in shunting straight to kvm_inject_undefined() :)
Thinking about this a bit more deeply, this code should be dead. The only time either of these conditions would happen is on a broken implementation. Probably should still handle it gracefully in case the CP10 handling in KVM becomes (or is in my own patch!) busted.
Quoted 1 line
Reviewed-by Reiji Watanabe <[email protected]>
Appreciated!
--
Thanks,
Oliver
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 2/3] KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler
Quoted 18 lines · Oliver Upton
Hi Reiji,
Quoted 6 lines · Reiji Watanabe
Quoted 6 lines
arch/arm64/kvm/sys_regs.c +6
-0+int kvm_handle_cp10_id (struct kvm_vcpu *vcpu) +{ +int Rt =kvm_vcpu_sys_get_rt (vcpu); +u32 esr =kvm_vcpu_get_esr (vcpu); +struct sys_reg_params params; +int ret;Quoted 8 lines
arch/arm64/kvm/sys_regs.c static int kvm_handle_cp_64(struct kvm_vcpu *vcpu, · line 2395
Quoted 4 lines
arch/arm64/kvm/sys_regs.c +4
-0+ +/* UNDEF on any unhandled register or an attempted write */ +if (!kvm_esr_cp10_id_to_sys64 (esr, ¶ms) || params.is_write) { +kvm_inject_undefined (vcpu);Nit: For debugging, it might be more useful to use
unhandled_cp_access()(, which needs to be changed to supportESR_ELx_EC_CP10_IDthough) rather than directly callingkvm_inject_undefined().
Quoted 7 lines
A very worthy nit, you spotted my laziness in shunting straight to
kvm_inject_undefined():)Thinking about this a bit more deeply, this code should be dead. The only time either of these conditions would happen is on a broken implementation. Probably should still handle it gracefully in case the CP10 handling in KVM becomes (or is in my own patch!) busted.
Actually, on second thought: any objections to leaving this as-is? kvm_esr_cp10_id_to_sys64() spits out sys_reg_params that point at the MRS alias for the VMRS register. Even if that call succeeds, the params that get printed out by unhandled_cp_access() do not match the actual register the guest was accessing. And if the call fails, ->Op2 is uninitialized.
Sorry for backtracking here.
--
Thanks,
Oliver
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 2/3] KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler
Hi Oliver,
Quoted 28 lines · Oliver Upton
Quoted 18 lines · Oliver Upton
Hi Reiji,
Quoted 6 lines · Reiji Watanabe
Quoted 6 lines
arch/arm64/kvm/sys_regs.c +6
-0+int kvm_handle_cp10_id (struct kvm_vcpu *vcpu) +{ +int Rt =kvm_vcpu_sys_get_rt (vcpu); +u32 esr =kvm_vcpu_get_esr (vcpu); +struct sys_reg_params params; +int ret;Quoted 8 lines
arch/arm64/kvm/sys_regs.c static int kvm_handle_cp_64(struct kvm_vcpu *vcpu, · line 2395
Quoted 4 lines
arch/arm64/kvm/sys_regs.c +4
-0+ +/* UNDEF on any unhandled register or an attempted write */ +if (!kvm_esr_cp10_id_to_sys64 (esr, ¶ms) || params.is_write) { +kvm_inject_undefined (vcpu);Nit: For debugging, it might be more useful to use
unhandled_cp_access()(, which needs to be changed to supportESR_ELx_EC_CP10_IDthough) rather than directly callingkvm_inject_undefined().Quoted 7 lines
A very worthy nit, you spotted my laziness in shunting straight to
kvm_inject_undefined():)Thinking about this a bit more deeply, this code should be dead. The only time either of these conditions would happen is on a broken implementation. Probably should still handle it gracefully in case the CP10 handling in KVM becomes (or is in my own patch!) busted.
Quoted 6 lines
Actually, on second thought: any objections to leaving this as-is?
kvm_esr_cp10_id_to_sys64()spits outsys_reg_paramsthat point at the MRS alias for the VMRS register. Even if that call succeeds, the params that get printed out byunhandled_cp_access()do not match the actual register the guest was accessing. And if the call fails, ->Op2 is uninitialized.
I understand a few additional changes are needed for this. Or simply use WARN_ON_ONCE ? (since this cannot be created by the guest but by a KVM or CPU problem)
I'm fine with the current code as-is though:)
Thanks,
Reiji
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
PATCH v2 3/3 KVM: arm64: Start trapping ID registers for 32 bit guests
In reply to Oliver UptonTo date KVM has not trapped ID register accesses from AArch32, meaning that guests get an unconstrained view of what hardware supports. This can be a serious problem because we try to base the guest's feature registers on values that are safe system-wide. Furthermore, KVM does not implement the latest ISA in the PMU and Debug architecture, so we constrain these fields to supported values.
Since KVM now correctly handles CP15 and CP10 register traps, we no longer need to clear HCR_EL2.TID3 for 32 bit guests and will instead emulate reads with their safe values.
--- arch/arm64/include/asm/kvm_arm.h | 3 ++- arch/arm64/include/asm/kvm_emulate.h | 8 -------- 2 files changed, 2 insertions(+), 9 deletions(-)
arch/arm64/include/asm/kvm_arm.h +2 -1
@@ -80,11 +80,12 @@* FMO: Override CPSR.F and enable signaling with VF * SWIO: Turn set/way invalidates into set/way clean+invalidate * PTW: Take a stage2 fault if a stage1 walk steps in device memory +* TID3: Trap EL1 reads of group 3 ID registers */ #define HCR_GUEST_FLAGS (HCR_TSC | HCR_TSW | HCR_TWE | HCR_TWI | HCR_VM | \ HCR_BSU_IS | HCR_FB | HCR_TACR | \ HCR_AMO | HCR_SWIO | HCR_TIDCP | HCR_RW | HCR_TLOR | \ - HCR_FMO | HCR_IMO | HCR_PTW ) + HCR_FMO | HCR_IMO | HCR_PTW | HCR_TID3 )#define HCR_VIRT_EXCP_MASK (HCR_VSE | HCR_VI | HCR_VF)#define HCR_HOST_NVHE_FLAGS (HCR_RW | HCR_API | HCR_APK | HCR_ATA)#define HCR_HOST_NVHE_PROTECTED_FLAGS (HCR_HOST_NVHE_FLAGS | HCR_TSC)
arch/arm64/include/asm/kvm_emulate.h +0 -8
@@ -75,14 +75,6 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)if (test_bit (KVM_ARM_VCPU_EL1_32BIT , vcpu->arch.features)) vcpu->arch.hcr_el2 &= ~HCR_RW ; -/* -* TID3: trap feature register accesses that we virtualise. -* For now this is conditional, since no AArch32 feature regs -* are currently virtualised. -*/ -if (!vcpu_el1_is_32bit (vcpu)) - vcpu->arch.hcr_el2 |=HCR_TID3 ; -if (cpus_have_const_cap (ARM64_MISMATCHED_CACHE_TYPE ) ||vcpu_el1_is_32bit (vcpu)) vcpu->arch.hcr_el2 |=HCR_TID2 ;
Signature · 1 line
2.35.1.1094.g7c7d902a7c-goog
Re: [PATCH v2 3/3] KVM: arm64: Start trapping ID registers for 32 bit guests
Quoted 8 lines · Oliver Upton
To date KVM has not trapped ID register accesses from AArch32, meaning that guests get an unconstrained view of what hardware supports. This can be a serious problem because we try to base the guest's feature registers on values that are safe system-wide. Furthermore, KVM does not implement the latest ISA in the PMU and Debug architecture, so we constrain these fields to supported values.
Quoted 5 lines
Since KVM now correctly handles CP15 and CP10 register traps, we no longer need to clear HCR_EL2.TID3 for 32 bit guests and will instead emulate reads with their safe values.
Signed-off-by Oliver Upton <[email protected]>
BTW, due to this, on a system that supports PMUv3, ID_DFR0_E1 value will become 0 for the aarch32 guest without PMUv3. This is the correct behavior, but it affects migration. I'm not sure how much we should care about migration of the aarch32 guest though (and it will be resolved once ID registers become configurable anyway).
Thanks,
Reiji
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 3/3] KVM: arm64: Start trapping ID registers for 32 bit guests
Hi Reiji,
Quoted 15 lines · Reiji Watanabe
Quoted 8 lines · Oliver Upton
To date KVM has not trapped ID register accesses from AArch32, meaning that guests get an unconstrained view of what hardware supports. This can be a serious problem because we try to base the guest's feature registers on values that are safe system-wide. Furthermore, KVM does not implement the latest ISA in the PMU and Debug architecture, so we constrain these fields to supported values.
Quoted 5 lines
Since KVM now correctly handles CP15 and CP10 register traps, we no longer need to clear HCR_EL2.TID3 for 32 bit guests and will instead emulate reads with their safe values.
Signed-off-by Oliver Upton <[email protected]>
Quoted 7 lines
Reviewed-by Reiji Watanabe <[email protected]>BTW, due to this, on a system that supports PMUv3,
ID_DFR0_E1value will become 0 for the aarch32 guest without PMUv3. This is the correct behavior, but it affects migration. I'm not sure how much we should care about migration of the aarch32 guest though (and it will be resolved once ID registers become configurable anyway).
I believe userspace has been accessing the sanitised values of these feature registers the entire time, so we should be OK on the UAPI side.
From the guest's perspective, I don't believe there is a meaningful change. Even if the guest were to believe the value it sees in ID_DFR0.PerfMon, it'll crash and burn on the first attempt to poke a PMU register as we synthesize an UNDEF, right? At least now we cover our tracks and ensure the vCPU correctly identifies itself to the guest.
This is, of course, unless I missed something painfully obvious :)
--
Thanks,
Oliver
Re: [PATCH v2 3/3] KVM: arm64: Start trapping ID registers for 32 bit guests
Hi Oliver,
Quoted 21 lines · Oliver Upton
Hi Reiji,
Quoted 17 lines · Reiji Watanabe
Quoted 8 lines · Oliver Upton
To date KVM has not trapped ID register accesses from AArch32, meaning that guests get an unconstrained view of what hardware supports. This can be a serious problem because we try to base the guest's feature registers on values that are safe system-wide. Furthermore, KVM does not implement the latest ISA in the PMU and Debug architecture, so we constrain these fields to supported values.
Quoted 5 lines
Since KVM now correctly handles CP15 and CP10 register traps, we no longer need to clear HCR_EL2.TID3 for 32 bit guests and will instead emulate reads with their safe values.
Signed-off-by Oliver Upton <[email protected]>Reviewed-by Reiji Watanabe <[email protected]>
Quoted 8 lines
Quoted 5 lines
BTW, due to this, on a system that supports PMUv3,
ID_DFR0_E1value will become 0 for the aarch32 guest without PMUv3. This is the correct behavior, but it affects migration. I'm not sure how much we should care about migration of the aarch32 guest though (and it will be resolved once ID registers become configurable anyway).I believe userspace has been accessing the sanitised values of these feature registers the entire time, so we should be OK on the UAPI side.
You are right:)
I totally forgot this was just about trapping. Sorry for the noise.
Thanks,
Reiji
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm