Re: [PATCH v3 11/15] KVM: arm64: Add trap handlers for protected VMs
Hi Will, On Thu, Aug 12, 2021 at 11:46 AM Will Deacon wrote: > > On Mon, Jul 19, 2021 at 05:03:42PM +0100, Fuad Tabba wrote: > > Add trap handlers for protected VMs. These are mainly for Sys64 > > and debug traps. > > > > No functional change intended as these are not hooked in yet to > > the guest exit handlers introduced earlier. So even when trapping > > is triggered, the exit handlers would let the host handle it, as > > before. > > > > Signed-off-by: Fuad Tabba > > --- > > arch/arm64/include/asm/kvm_fixed_config.h | 178 + > > arch/arm64/include/asm/kvm_host.h | 2 + > > arch/arm64/include/asm/kvm_hyp.h | 3 + > > arch/arm64/kvm/Makefile | 2 +- > > arch/arm64/kvm/arm.c | 11 + > > arch/arm64/kvm/hyp/nvhe/Makefile | 2 +- > > arch/arm64/kvm/hyp/nvhe/sys_regs.c| 443 ++ > > arch/arm64/kvm/pkvm.c | 183 + > > 8 files changed, 822 insertions(+), 2 deletions(-) > > create mode 100644 arch/arm64/include/asm/kvm_fixed_config.h > > create mode 100644 arch/arm64/kvm/hyp/nvhe/sys_regs.c > > create mode 100644 arch/arm64/kvm/pkvm.c > > > > diff --git a/arch/arm64/include/asm/kvm_fixed_config.h > > b/arch/arm64/include/asm/kvm_fixed_config.h > > new file mode 100644 > > index ..b39a5de2c4b9 > > --- /dev/null > > +++ b/arch/arm64/include/asm/kvm_fixed_config.h > > @@ -0,0 +1,178 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > +/* > > + * Copyright (C) 2021 Google LLC > > + * Author: Fuad Tabba > > + */ > > + > > +#ifndef __ARM64_KVM_FIXED_CONFIG_H__ > > +#define __ARM64_KVM_FIXED_CONFIG_H__ > > + > > +#include > > + > > +/* > > + * This file contains definitions for features to be allowed or restricted > > for > > + * guest virtual machines as a baseline, depending on what mode KVM is > > running > > + * in and on the type of guest is running. > > s/is running/that is running/ Ack. > > + * > > + * The features are represented as the highest allowed value for a feature > > in > > + * the feature id registers. If the field is set to all ones (i.e., > > 0b), > > + * then it's only restricted by what the system allows. If the feature is > > set to > > + * another value, then that value would be the maximum value allowed and > > + * supported in pKVM, even if the system supports a higher value. > > Given that some fields are signed whereas others are unsigned, I think the > wording could be a bit tighter here when it refers to "maximum". > > > + * > > + * Some features are forced to a certain value, in which case a SET bitmap > > is > > + * used to force these values. > > + */ > > + > > + > > +/* > > + * Allowed features for protected guests (Protected KVM) > > + * > > + * The approach taken here is to allow features that are: > > + * - needed by common Linux distributions (e.g., flooating point) > > s/flooating/floating Ack. > > + * - are trivial, e.g., supporting the feature doesn't introduce or > > require the > > + * tracking of additional state > ... in KVM. Ack. > > + * - not trapable > > s/not trapable/cannot be trapped/ Ack > > + */ > > + > > +/* > > + * - Floating-point and Advanced SIMD: > > + * Don't require much support other than maintaining the context, which > > KVM > > + * already has. > > I'd rework this sentence. We have to support fpsimd because Linux guests > rely on it. Ack > > + * - AArch64 guests only (no support for AArch32 guests): > > + * Simplify support in case of asymmetric AArch32 systems. > > I don't think asymmetric systems come into this really; AArch32 on its > own adds lots of complexity in trap handling, emulation, condition codes > etc. Restricting guests to AArch64 means we don't have to worry about the > AArch32 exception model or emulation of 32-bit instructions. Ack > > + * - RAS (v1) > > + * v1 doesn't require much additional support, but later versions do. > > Be more specific? Ack > > + * - Data Independent Timing > > + * Trivial > > + * Remaining features are not supported either because they require too > > much > > + * support from KVM, or risk leaking guest data. > > I think we should drop this sentence -- it makes it sounds like we can't > be arsed :) Ack. > > + */ > > +#define PVM_ID_AA64PFR0_ALLOW (\ > > + FEATURE(ID_AA64PFR0_FP) | \ > > + FIELD_PREP(FEATURE(ID_AA64PFR0_EL0), ID_AA64PFR0_ELx_64BIT_ONLY) | \ > > + FIELD_PREP(FEATURE(ID_AA64PFR0_EL1), ID_AA64PFR0_ELx_64BIT_ONLY) | \ > > + FIELD_PREP(FEATURE(ID_AA64PFR0_EL2), ID_AA64PFR0_ELx_64BIT_ONLY) | \ > > + FIELD_PREP(FEATURE(ID_AA64PFR0_EL3), ID_AA64PFR0_ELx_64BIT_ONLY) | \ > > + FIELD_PREP(FEATURE(ID_AA64PFR0_RAS), ID_AA64PFR0_RAS_V1) | \ > > I think having the FIELD_PREP entries in the ALLOW mask is quite confusing > here -- naively you would expect to be able to bitwise-and the host register > value with the ALLOW mask and get the sanitised version back, but with these > here you have t
Re: [PATCH v3 11/15] KVM: arm64: Add trap handlers for protected VMs
On Mon, Jul 19, 2021 at 05:03:42PM +0100, Fuad Tabba wrote: > Add trap handlers for protected VMs. These are mainly for Sys64 > and debug traps. > > No functional change intended as these are not hooked in yet to > the guest exit handlers introduced earlier. So even when trapping > is triggered, the exit handlers would let the host handle it, as > before. > > Signed-off-by: Fuad Tabba > --- > arch/arm64/include/asm/kvm_fixed_config.h | 178 + > arch/arm64/include/asm/kvm_host.h | 2 + > arch/arm64/include/asm/kvm_hyp.h | 3 + > arch/arm64/kvm/Makefile | 2 +- > arch/arm64/kvm/arm.c | 11 + > arch/arm64/kvm/hyp/nvhe/Makefile | 2 +- > arch/arm64/kvm/hyp/nvhe/sys_regs.c| 443 ++ > arch/arm64/kvm/pkvm.c | 183 + > 8 files changed, 822 insertions(+), 2 deletions(-) > create mode 100644 arch/arm64/include/asm/kvm_fixed_config.h > create mode 100644 arch/arm64/kvm/hyp/nvhe/sys_regs.c > create mode 100644 arch/arm64/kvm/pkvm.c > > diff --git a/arch/arm64/include/asm/kvm_fixed_config.h > b/arch/arm64/include/asm/kvm_fixed_config.h > new file mode 100644 > index ..b39a5de2c4b9 > --- /dev/null > +++ b/arch/arm64/include/asm/kvm_fixed_config.h > @@ -0,0 +1,178 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (C) 2021 Google LLC > + * Author: Fuad Tabba > + */ > + > +#ifndef __ARM64_KVM_FIXED_CONFIG_H__ > +#define __ARM64_KVM_FIXED_CONFIG_H__ > + > +#include > + > +/* > + * This file contains definitions for features to be allowed or restricted > for > + * guest virtual machines as a baseline, depending on what mode KVM is > running > + * in and on the type of guest is running. s/is running/that is running/ > + * > + * The features are represented as the highest allowed value for a feature in > + * the feature id registers. If the field is set to all ones (i.e., 0b), > + * then it's only restricted by what the system allows. If the feature is > set to > + * another value, then that value would be the maximum value allowed and > + * supported in pKVM, even if the system supports a higher value. Given that some fields are signed whereas others are unsigned, I think the wording could be a bit tighter here when it refers to "maximum". > + * > + * Some features are forced to a certain value, in which case a SET bitmap is > + * used to force these values. > + */ > + > + > +/* > + * Allowed features for protected guests (Protected KVM) > + * > + * The approach taken here is to allow features that are: > + * - needed by common Linux distributions (e.g., flooating point) s/flooating/floating > + * - are trivial, e.g., supporting the feature doesn't introduce or require > the > + * tracking of additional state ... in KVM. > + * - not trapable s/not trapable/cannot be trapped/ > + */ > + > +/* > + * - Floating-point and Advanced SIMD: > + * Don't require much support other than maintaining the context, which KVM > + * already has. I'd rework this sentence. We have to support fpsimd because Linux guests rely on it. > + * - AArch64 guests only (no support for AArch32 guests): > + * Simplify support in case of asymmetric AArch32 systems. I don't think asymmetric systems come into this really; AArch32 on its own adds lots of complexity in trap handling, emulation, condition codes etc. Restricting guests to AArch64 means we don't have to worry about the AArch32 exception model or emulation of 32-bit instructions. > + * - RAS (v1) > + * v1 doesn't require much additional support, but later versions do. Be more specific? > + * - Data Independent Timing > + * Trivial > + * Remaining features are not supported either because they require too much > + * support from KVM, or risk leaking guest data. I think we should drop this sentence -- it makes it sounds like we can't be arsed :) > + */ > +#define PVM_ID_AA64PFR0_ALLOW (\ > + FEATURE(ID_AA64PFR0_FP) | \ > + FIELD_PREP(FEATURE(ID_AA64PFR0_EL0), ID_AA64PFR0_ELx_64BIT_ONLY) | \ > + FIELD_PREP(FEATURE(ID_AA64PFR0_EL1), ID_AA64PFR0_ELx_64BIT_ONLY) | \ > + FIELD_PREP(FEATURE(ID_AA64PFR0_EL2), ID_AA64PFR0_ELx_64BIT_ONLY) | \ > + FIELD_PREP(FEATURE(ID_AA64PFR0_EL3), ID_AA64PFR0_ELx_64BIT_ONLY) | \ > + FIELD_PREP(FEATURE(ID_AA64PFR0_RAS), ID_AA64PFR0_RAS_V1) | \ I think having the FIELD_PREP entries in the ALLOW mask is quite confusing here -- naively you would expect to be able to bitwise-and the host register value with the ALLOW mask and get the sanitised version back, but with these here you have to go field-by-field to compute the common value. So perhaps move those into a PVM_ID_AA64PFR0_RESTRICT mask or something? Then pvm_access_id_aa64pfr0() will become a little easier to read, I think. > + FEATURE(ID_AA64PFR0_ASIMD) | \ > + FEATURE(ID_AA64PFR0_DIT) \ > + ) > + > +/* > + * - Branch Target Identification > + * - Speculative Store Byp
[PATCH v3 11/15] KVM: arm64: Add trap handlers for protected VMs
Add trap handlers for protected VMs. These are mainly for Sys64 and debug traps. No functional change intended as these are not hooked in yet to the guest exit handlers introduced earlier. So even when trapping is triggered, the exit handlers would let the host handle it, as before. Signed-off-by: Fuad Tabba --- arch/arm64/include/asm/kvm_fixed_config.h | 178 + arch/arm64/include/asm/kvm_host.h | 2 + arch/arm64/include/asm/kvm_hyp.h | 3 + arch/arm64/kvm/Makefile | 2 +- arch/arm64/kvm/arm.c | 11 + arch/arm64/kvm/hyp/nvhe/Makefile | 2 +- arch/arm64/kvm/hyp/nvhe/sys_regs.c| 443 ++ arch/arm64/kvm/pkvm.c | 183 + 8 files changed, 822 insertions(+), 2 deletions(-) create mode 100644 arch/arm64/include/asm/kvm_fixed_config.h create mode 100644 arch/arm64/kvm/hyp/nvhe/sys_regs.c create mode 100644 arch/arm64/kvm/pkvm.c diff --git a/arch/arm64/include/asm/kvm_fixed_config.h b/arch/arm64/include/asm/kvm_fixed_config.h new file mode 100644 index ..b39a5de2c4b9 --- /dev/null +++ b/arch/arm64/include/asm/kvm_fixed_config.h @@ -0,0 +1,178 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright (C) 2021 Google LLC + * Author: Fuad Tabba + */ + +#ifndef __ARM64_KVM_FIXED_CONFIG_H__ +#define __ARM64_KVM_FIXED_CONFIG_H__ + +#include + +/* + * This file contains definitions for features to be allowed or restricted for + * guest virtual machines as a baseline, depending on what mode KVM is running + * in and on the type of guest is running. + * + * The features are represented as the highest allowed value for a feature in + * the feature id registers. If the field is set to all ones (i.e., 0b), + * then it's only restricted by what the system allows. If the feature is set to + * another value, then that value would be the maximum value allowed and + * supported in pKVM, even if the system supports a higher value. + * + * Some features are forced to a certain value, in which case a SET bitmap is + * used to force these values. + */ + + +/* + * Allowed features for protected guests (Protected KVM) + * + * The approach taken here is to allow features that are: + * - needed by common Linux distributions (e.g., flooating point) + * - are trivial, e.g., supporting the feature doesn't introduce or require the + * tracking of additional state + * - not trapable + */ + +/* + * - Floating-point and Advanced SIMD: + * Don't require much support other than maintaining the context, which KVM + * already has. + * - AArch64 guests only (no support for AArch32 guests): + * Simplify support in case of asymmetric AArch32 systems. + * - RAS (v1) + * v1 doesn't require much additional support, but later versions do. + * - Data Independent Timing + * Trivial + * Remaining features are not supported either because they require too much + * support from KVM, or risk leaking guest data. + */ +#define PVM_ID_AA64PFR0_ALLOW (\ + FEATURE(ID_AA64PFR0_FP) | \ + FIELD_PREP(FEATURE(ID_AA64PFR0_EL0), ID_AA64PFR0_ELx_64BIT_ONLY) | \ + FIELD_PREP(FEATURE(ID_AA64PFR0_EL1), ID_AA64PFR0_ELx_64BIT_ONLY) | \ + FIELD_PREP(FEATURE(ID_AA64PFR0_EL2), ID_AA64PFR0_ELx_64BIT_ONLY) | \ + FIELD_PREP(FEATURE(ID_AA64PFR0_EL3), ID_AA64PFR0_ELx_64BIT_ONLY) | \ + FIELD_PREP(FEATURE(ID_AA64PFR0_RAS), ID_AA64PFR0_RAS_V1) | \ + FEATURE(ID_AA64PFR0_ASIMD) | \ + FEATURE(ID_AA64PFR0_DIT) \ + ) + +/* + * - Branch Target Identification + * - Speculative Store Bypassing + * These features are trivial to support + */ +#define PVM_ID_AA64PFR1_ALLOW (\ + FEATURE(ID_AA64PFR1_BT) | \ + FEATURE(ID_AA64PFR1_SSBS) \ + ) + +/* + * No support for Scalable Vectors: + * Requires additional support from KVM + */ +#define PVM_ID_AA64ZFR0_ALLOW (0ULL) + +/* + * No support for debug, including breakpoints, and watchpoints: + * Reduce complexity and avoid exposing/leaking guest data + * + * NOTE: The Arm architecture mandates support for at least the Armv8 debug + * architecture, which would include at least 2 hardware breakpoints and + * watchpoints. Providing that support to protected guests adds considerable + * state and complexity, and risks leaking guest data. Therefore, the reserved + * value of 0 is used for debug-related fields. + */ +#define PVM_ID_AA64DFR0_ALLOW (0ULL) + +/* + * These features are chosen because they are supported by KVM and to limit the + * confiruation state space and make it more deterministic. + * - 40-bit IPA + * - 16-bit ASID + * - Mixed-endian + * - Distinction between Secure and Non-secure Memory + * - Mixed-endian at EL0 only + * - Non-context synchronizing exception entry and exit + */ +#define PVM_ID_AA64MMFR0_ALLOW (\ + FIELD_PREP(FEATURE(ID_AA64MMFR0_PARANGE), ID_AA64MMFR0_PARANGE_40) | \ + FIELD_PREP(FEATURE(ID_AA64MMFR0_ASID), ID_AA64MMFR0_ASID_16) | \ + FEATURE
