| Message ID | 20210922124704.600087-12-tabba@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Fixed features for protected VMs | expand |
Hi Fuad, On Wed, 22 Sep 2021 13:47:03 +0100, Fuad Tabba <tabba@google.com> wrote: > > Trap accesses to restricted features for VMs running in protected > mode. > > Access to feature registers are emulated, and only supported > features are exposed to protected VMs. > > Accesses to restricted registers as well as restricted > instructions are trapped, and an undefined exception is injected > into the protected guests, i.e., with EC = 0x0 (unknown reason). > This EC is the one used, according to the Arm Architecture > Reference Manual, for unallocated or undefined system registers > or instructions. > > Only affects the functionality of protected VMs. Otherwise, > should not affect non-protected VMs when KVM is running in > protected mode. > > Signed-off-by: Fuad Tabba <tabba@google.com> > --- > arch/arm64/kvm/hyp/nvhe/switch.c | 60 ++++++++++++++++++++++++++++++++ > 1 file changed, 60 insertions(+) > > diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c > index 49080c607838..2bf5952f651b 100644 > --- a/arch/arm64/kvm/hyp/nvhe/switch.c > +++ b/arch/arm64/kvm/hyp/nvhe/switch.c > @@ -20,6 +20,7 @@ > #include <asm/kprobes.h> > #include <asm/kvm_asm.h> > #include <asm/kvm_emulate.h> > +#include <asm/kvm_fixed_config.h> > #include <asm/kvm_hyp.h> > #include <asm/kvm_mmu.h> > #include <asm/fpsimd.h> > @@ -28,6 +29,7 @@ > #include <asm/thread_info.h> > > #include <nvhe/mem_protect.h> > +#include <nvhe/sys_regs.h> > > /* Non-VHE specific context */ > DEFINE_PER_CPU(struct kvm_host_data, kvm_host_data); > @@ -158,6 +160,49 @@ static void __pmu_switch_to_host(struct kvm_cpu_context *host_ctxt) > write_sysreg(pmu->events_host, pmcntenset_el0); > } > > +/** > + * Handler for protected VM restricted exceptions. > + * > + * Inject an undefined exception into the guest and return true to indicate that > + * the hypervisor has handled the exit, and control should go back to the guest. > + */ > +static bool kvm_handle_pvm_restricted(struct kvm_vcpu *vcpu, u64 *exit_code) > +{ > + __inject_undef64(vcpu); > + return true; > +} > + > +/** > + * Handler for protected VM MSR, MRS or System instruction execution in AArch64. > + * > + * Returns true if the hypervisor has handled the exit, and control should go > + * back to the guest, or false if it hasn't. > + */ > +static bool kvm_handle_pvm_sys64(struct kvm_vcpu *vcpu, u64 *exit_code) > +{ > + if (kvm_handle_pvm_sysreg(vcpu, exit_code)) > + return true; > + else > + return kvm_hyp_handle_sysreg(vcpu, exit_code); nit: drop the else. I wonder though: what if there is an overlap between between the pVM handling and the normal KVM stuff? Are we guaranteed that there is none? For example, ESR_ELx_EC_SYS64 is used when working around some bugs (see the TX2 TVM handling). What happens if you return early and don't let it happen? This has a huge potential for some bad breakage. > +} > + > +/** > + * Handler for protected floating-point and Advanced SIMD accesses. > + * > + * Returns true if the hypervisor has handled the exit, and control should go > + * back to the guest, or false if it hasn't. > + */ > +static bool kvm_handle_pvm_fpsimd(struct kvm_vcpu *vcpu, u64 *exit_code) > +{ > + /* Linux guests assume support for floating-point and Advanced SIMD. */ > + BUILD_BUG_ON(!FIELD_GET(ARM64_FEATURE_MASK(ID_AA64PFR0_FP), > + PVM_ID_AA64PFR0_ALLOW)); > + BUILD_BUG_ON(!FIELD_GET(ARM64_FEATURE_MASK(ID_AA64PFR0_ASIMD), > + PVM_ID_AA64PFR0_ALLOW)); > + > + return kvm_hyp_handle_fpsimd(vcpu, exit_code); > +} > + > static const exit_handler_fn hyp_exit_handlers[] = { > [0 ... ESR_ELx_EC_MAX] = NULL, > [ESR_ELx_EC_CP15_32] = kvm_hyp_handle_cp15, > @@ -170,8 +215,23 @@ static const exit_handler_fn hyp_exit_handlers[] = { > [ESR_ELx_EC_PAC] = kvm_hyp_handle_ptrauth, > }; > > +static const exit_handler_fn pvm_exit_handlers[] = { > + [0 ... ESR_ELx_EC_MAX] = NULL, > + [ESR_ELx_EC_CP15_32] = kvm_hyp_handle_cp15, > + [ESR_ELx_EC_CP15_64] = kvm_hyp_handle_cp15, Heads up, this one was bogus, and I removed it in my patches[1]. But it really begs the question: given that you really don't want to handle any AArch32 for protected VMs, why handling anything at all the first place? You really should let the exit happen and let the outer run loop deal with it. > + [ESR_ELx_EC_SYS64] = kvm_handle_pvm_sys64, > + [ESR_ELx_EC_SVE] = kvm_handle_pvm_restricted, > + [ESR_ELx_EC_FP_ASIMD] = kvm_handle_pvm_fpsimd, > + [ESR_ELx_EC_IABT_LOW] = kvm_hyp_handle_iabt_low, > + [ESR_ELx_EC_DABT_LOW] = kvm_hyp_handle_dabt_low, > + [ESR_ELx_EC_PAC] = kvm_hyp_handle_ptrauth, > +}; > + > static const exit_handler_fn *kvm_get_exit_handler_array(struct kvm *kvm) > { > + if (unlikely(kvm_vm_is_protected(kvm))) > + return pvm_exit_handlers; > + > return hyp_exit_handlers; > } > > -- > 2.33.0.464.g1972c5931b-goog > > Thanks, M. [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/commit/?h=kvm-arm64/early-ec-handlers&id=f84ff369795ed47f2cd5e556170166ee8b3a988f
Hi Marc, On Mon, Oct 4, 2021 at 6:27 PM Marc Zyngier <maz@kernel.org> wrote: > > Hi Fuad, > > On Wed, 22 Sep 2021 13:47:03 +0100, > Fuad Tabba <tabba@google.com> wrote: > > > > Trap accesses to restricted features for VMs running in protected > > mode. > > > > Access to feature registers are emulated, and only supported > > features are exposed to protected VMs. > > > > Accesses to restricted registers as well as restricted > > instructions are trapped, and an undefined exception is injected > > into the protected guests, i.e., with EC = 0x0 (unknown reason). > > This EC is the one used, according to the Arm Architecture > > Reference Manual, for unallocated or undefined system registers > > or instructions. > > > > Only affects the functionality of protected VMs. Otherwise, > > should not affect non-protected VMs when KVM is running in > > protected mode. > > > > Signed-off-by: Fuad Tabba <tabba@google.com> > > --- > > arch/arm64/kvm/hyp/nvhe/switch.c | 60 ++++++++++++++++++++++++++++++++ > > 1 file changed, 60 insertions(+) > > > > diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c > > index 49080c607838..2bf5952f651b 100644 > > --- a/arch/arm64/kvm/hyp/nvhe/switch.c > > +++ b/arch/arm64/kvm/hyp/nvhe/switch.c > > @@ -20,6 +20,7 @@ > > #include <asm/kprobes.h> > > #include <asm/kvm_asm.h> > > #include <asm/kvm_emulate.h> > > +#include <asm/kvm_fixed_config.h> > > #include <asm/kvm_hyp.h> > > #include <asm/kvm_mmu.h> > > #include <asm/fpsimd.h> > > @@ -28,6 +29,7 @@ > > #include <asm/thread_info.h> > > > > #include <nvhe/mem_protect.h> > > +#include <nvhe/sys_regs.h> > > > > /* Non-VHE specific context */ > > DEFINE_PER_CPU(struct kvm_host_data, kvm_host_data); > > @@ -158,6 +160,49 @@ static void __pmu_switch_to_host(struct kvm_cpu_context *host_ctxt) > > write_sysreg(pmu->events_host, pmcntenset_el0); > > } > > > > +/** > > + * Handler for protected VM restricted exceptions. > > + * > > + * Inject an undefined exception into the guest and return true to indicate that > > + * the hypervisor has handled the exit, and control should go back to the guest. > > + */ > > +static bool kvm_handle_pvm_restricted(struct kvm_vcpu *vcpu, u64 *exit_code) > > +{ > > + __inject_undef64(vcpu); > > + return true; > > +} > > + > > +/** > > + * Handler for protected VM MSR, MRS or System instruction execution in AArch64. > > + * > > + * Returns true if the hypervisor has handled the exit, and control should go > > + * back to the guest, or false if it hasn't. > > + */ > > +static bool kvm_handle_pvm_sys64(struct kvm_vcpu *vcpu, u64 *exit_code) > > +{ > > + if (kvm_handle_pvm_sysreg(vcpu, exit_code)) > > + return true; > > + else > > + return kvm_hyp_handle_sysreg(vcpu, exit_code); > > nit: drop the else. Will do. > I wonder though: what if there is an overlap between between the pVM > handling and the normal KVM stuff? Are we guaranteed that there is > none? > > For example, ESR_ELx_EC_SYS64 is used when working around some bugs > (see the TX2 TVM handling). What happens if you return early and don't > let it happen? This has a huge potential for some bad breakage. This is a tough one. Especially because it's dealing with bugs, there is no guarantee really. I think that for the TVM handling there is no overlap and the TVM handling code in kvm_hyp_handle_sysreg() will be invoked. However, workarounds could always be added, and if that happens, we need to make sure that they're on all paths. One solution is to make sure that such code is in a common function called by both paths. Not sure how we could enforce that other than by documenting it. What do you think? > > +} > > + > > +/** > > + * Handler for protected floating-point and Advanced SIMD accesses. > > + * > > + * Returns true if the hypervisor has handled the exit, and control should go > > + * back to the guest, or false if it hasn't. > > + */ > > +static bool kvm_handle_pvm_fpsimd(struct kvm_vcpu *vcpu, u64 *exit_code) > > +{ > > + /* Linux guests assume support for floating-point and Advanced SIMD. */ > > + BUILD_BUG_ON(!FIELD_GET(ARM64_FEATURE_MASK(ID_AA64PFR0_FP), > > + PVM_ID_AA64PFR0_ALLOW)); > > + BUILD_BUG_ON(!FIELD_GET(ARM64_FEATURE_MASK(ID_AA64PFR0_ASIMD), > > + PVM_ID_AA64PFR0_ALLOW)); > > + > > + return kvm_hyp_handle_fpsimd(vcpu, exit_code); > > +} > > + > > static const exit_handler_fn hyp_exit_handlers[] = { > > [0 ... ESR_ELx_EC_MAX] = NULL, > > [ESR_ELx_EC_CP15_32] = kvm_hyp_handle_cp15, > > @@ -170,8 +215,23 @@ static const exit_handler_fn hyp_exit_handlers[] = { > > [ESR_ELx_EC_PAC] = kvm_hyp_handle_ptrauth, > > }; > > > > +static const exit_handler_fn pvm_exit_handlers[] = { > > + [0 ... ESR_ELx_EC_MAX] = NULL, > > + [ESR_ELx_EC_CP15_32] = kvm_hyp_handle_cp15, > > + [ESR_ELx_EC_CP15_64] = kvm_hyp_handle_cp15, > > Heads up, this one was bogus, and I removed it in my patches[1]. > > But it really begs the question: given that you really don't want to > handle any AArch32 for protected VMs, why handling anything at all the > first place? You really should let the exit happen and let the outer > run loop deal with it. Good point. Will fix this. Cheers, /fuad > > + [ESR_ELx_EC_SYS64] = kvm_handle_pvm_sys64, > > + [ESR_ELx_EC_SVE] = kvm_handle_pvm_restricted, > > + [ESR_ELx_EC_FP_ASIMD] = kvm_handle_pvm_fpsimd, > > + [ESR_ELx_EC_IABT_LOW] = kvm_hyp_handle_iabt_low, > > + [ESR_ELx_EC_DABT_LOW] = kvm_hyp_handle_dabt_low, > > + [ESR_ELx_EC_PAC] = kvm_hyp_handle_ptrauth, > > +}; > > + > > static const exit_handler_fn *kvm_get_exit_handler_array(struct kvm *kvm) > > { > > + if (unlikely(kvm_vm_is_protected(kvm))) > > + return pvm_exit_handlers; > > + > > return hyp_exit_handlers; > > } > > > > -- > > 2.33.0.464.g1972c5931b-goog > > > > > > Thanks, > > M. > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/commit/?h=kvm-arm64/early-ec-handlers&id=f84ff369795ed47f2cd5e556170166ee8b3a988f > > -- > Without deviation from the norm, progress is not possible.
diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c index 49080c607838..2bf5952f651b 100644 --- a/arch/arm64/kvm/hyp/nvhe/switch.c +++ b/arch/arm64/kvm/hyp/nvhe/switch.c @@ -20,6 +20,7 @@ #include <asm/kprobes.h> #include <asm/kvm_asm.h> #include <asm/kvm_emulate.h> +#include <asm/kvm_fixed_config.h> #include <asm/kvm_hyp.h> #include <asm/kvm_mmu.h> #include <asm/fpsimd.h> @@ -28,6 +29,7 @@ #include <asm/thread_info.h> #include <nvhe/mem_protect.h> +#include <nvhe/sys_regs.h> /* Non-VHE specific context */ DEFINE_PER_CPU(struct kvm_host_data, kvm_host_data); @@ -158,6 +160,49 @@ static void __pmu_switch_to_host(struct kvm_cpu_context *host_ctxt) write_sysreg(pmu->events_host, pmcntenset_el0); } +/** + * Handler for protected VM restricted exceptions. + * + * Inject an undefined exception into the guest and return true to indicate that + * the hypervisor has handled the exit, and control should go back to the guest. + */ +static bool kvm_handle_pvm_restricted(struct kvm_vcpu *vcpu, u64 *exit_code) +{ + __inject_undef64(vcpu); + return true; +} + +/** + * Handler for protected VM MSR, MRS or System instruction execution in AArch64. + * + * Returns true if the hypervisor has handled the exit, and control should go + * back to the guest, or false if it hasn't. + */ +static bool kvm_handle_pvm_sys64(struct kvm_vcpu *vcpu, u64 *exit_code) +{ + if (kvm_handle_pvm_sysreg(vcpu, exit_code)) + return true; + else + return kvm_hyp_handle_sysreg(vcpu, exit_code); +} + +/** + * Handler for protected floating-point and Advanced SIMD accesses. + * + * Returns true if the hypervisor has handled the exit, and control should go + * back to the guest, or false if it hasn't. + */ +static bool kvm_handle_pvm_fpsimd(struct kvm_vcpu *vcpu, u64 *exit_code) +{ + /* Linux guests assume support for floating-point and Advanced SIMD. */ + BUILD_BUG_ON(!FIELD_GET(ARM64_FEATURE_MASK(ID_AA64PFR0_FP), + PVM_ID_AA64PFR0_ALLOW)); + BUILD_BUG_ON(!FIELD_GET(ARM64_FEATURE_MASK(ID_AA64PFR0_ASIMD), + PVM_ID_AA64PFR0_ALLOW)); + + return kvm_hyp_handle_fpsimd(vcpu, exit_code); +} + static const exit_handler_fn hyp_exit_handlers[] = { [0 ... ESR_ELx_EC_MAX] = NULL, [ESR_ELx_EC_CP15_32] = kvm_hyp_handle_cp15, @@ -170,8 +215,23 @@ static const exit_handler_fn hyp_exit_handlers[] = { [ESR_ELx_EC_PAC] = kvm_hyp_handle_ptrauth, }; +static const exit_handler_fn pvm_exit_handlers[] = { + [0 ... ESR_ELx_EC_MAX] = NULL, + [ESR_ELx_EC_CP15_32] = kvm_hyp_handle_cp15, + [ESR_ELx_EC_CP15_64] = kvm_hyp_handle_cp15, + [ESR_ELx_EC_SYS64] = kvm_handle_pvm_sys64, + [ESR_ELx_EC_SVE] = kvm_handle_pvm_restricted, + [ESR_ELx_EC_FP_ASIMD] = kvm_handle_pvm_fpsimd, + [ESR_ELx_EC_IABT_LOW] = kvm_hyp_handle_iabt_low, + [ESR_ELx_EC_DABT_LOW] = kvm_hyp_handle_dabt_low, + [ESR_ELx_EC_PAC] = kvm_hyp_handle_ptrauth, +}; + static const exit_handler_fn *kvm_get_exit_handler_array(struct kvm *kvm) { + if (unlikely(kvm_vm_is_protected(kvm))) + return pvm_exit_handlers; + return hyp_exit_handlers; }
Trap accesses to restricted features for VMs running in protected mode. Access to feature registers are emulated, and only supported features are exposed to protected VMs. Accesses to restricted registers as well as restricted instructions are trapped, and an undefined exception is injected into the protected guests, i.e., with EC = 0x0 (unknown reason). This EC is the one used, according to the Arm Architecture Reference Manual, for unallocated or undefined system registers or instructions. Only affects the functionality of protected VMs. Otherwise, should not affect non-protected VMs when KVM is running in protected mode. Signed-off-by: Fuad Tabba <tabba@google.com> --- arch/arm64/kvm/hyp/nvhe/switch.c | 60 ++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+)