| Message ID | 20220401010832.3425787-3-oupton@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Limit feature register reads from AArch32 | expand |
On Thu, Mar 31, 2022 at 6:08 PM Oliver Upton <oupton@google.com> wrote: > > 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 <oupton@google.com> > --- > 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(+) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 0e96087885fe..7a65ac268a22 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -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); > > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c > index 97fe14aab1a3..5088a86ace5b 100644 > --- a/arch/arm64/kvm/handle_exit.c > +++ b/arch/arm64/kvm/handle_exit.c > @@ -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, > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 8b791256a5b4..4863592d060d 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -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. > + */ > +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(). Reviewed-by: Reiji Watanabe <reijiw@google.com> Thanks, Reiji > + 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 > -- > 2.35.1.1094.g7c7d902a7c-goog >
Hi Reiji, On Sun, Apr 03, 2022 at 08:57:47PM -0700, Reiji Watanabe wrote: > > +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(). 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. > Reviewed-by: Reiji Watanabe <reijiw@google.com> Appreciated! -- Thanks, Oliver
On Mon, Apr 04, 2022 at 05:28:33AM +0000, Oliver Upton wrote: > Hi Reiji, > > On Sun, Apr 03, 2022 at 08:57:47PM -0700, Reiji Watanabe wrote: > > > +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(). > > 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
Hi Oliver, On Mon, Apr 4, 2022 at 4:19 PM Oliver Upton <oupton@google.com> wrote: > > On Mon, Apr 04, 2022 at 05:28:33AM +0000, Oliver Upton wrote: > > Hi Reiji, > > > > On Sun, Apr 03, 2022 at 08:57:47PM -0700, Reiji Watanabe wrote: > > > > +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(). > > > > 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. 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
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 0e96087885fe..7a65ac268a22 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -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); diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c index 97fe14aab1a3..5088a86ace5b 100644 --- a/arch/arm64/kvm/handle_exit.c +++ b/arch/arm64/kvm/handle_exit.c @@ -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, diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 8b791256a5b4..4863592d060d 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -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. + */ +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); + 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
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 <oupton@google.com> --- 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(+)