Hi,
Changes since v2 [1]:
- Both trapping and setting of feature id registers are toggled by an allowed
features bitmap of the feature id registers (Will)
- Documentation explaining the rationale behind allowed/blocked features (Drew)
- Restrict protected VM features by checking and restricting VM capabilities
- Misc small fixes and tidying up (mostly Will)
- Remove dependency on Will's protected VM user ABI series [2]
- Rebase on 5.14-rc2
- Carried Will's acks
Changes since v1 [3]:
- Restrict protected VM features based on an allowed features rather than
rejected ones (Drew)
- Add more background describing protected KVM to the cover letter (Alex)
This patch series adds support for restricting CPU features for protected VMs
in KVM (pKVM) [4].
Various VM feature configurations are allowed in KVM/arm64, each requiring
specific handling logic to deal with traps, context-switching and potentially
emulation. Achieving feature parity in pKVM therefore requires either elevating
this logic to EL2 (and substantially increasing the TCB) or continuing to trust
the host handlers at EL1. Since neither of these options are especially
appealing, pKVM instead limits the CPU features exposed to a guest to a fixed
configuration based on the underlying hardware and which can mostly be provided
straightforwardly by EL2.
This series approaches that by restricting CPU features exposed to protected
guests. Features advertised through feature registers are limited, which pKVM
enforces by trapping register accesses and instructions associated with these
features.
This series is based on 5.14-rc2. You can find the applied series here [5].
Cheers,
/fuad
[1] https://lore.kernel.org/kvmarm/20210615133950.693489-1-tabba@google.com/
[2] https://lore.kernel.org/kvmarm/20210603183347.1695-1-will@kernel.org/
[3] https://lore.kernel.org/kvmarm/20210608141141.997398-1-tabba@google.com/
[4] Once complete, protected KVM adds the ability to create protected VMs.
These protected VMs are protected from the host Linux kernel (and from other
VMs), where the host does not have access to guest memory,even if compromised.
Normal (nVHE) guests can still be created and run in parallel with protected
VMs. Their functionality should not be affected.
For protected VMs, the host should not even have access to a protected guest's
state or anything that would enable it to manipulate it (e.g., vcpu register
context and el2 system registers); only hyp would have that access. If the host
could access that state, then it might be able to get around the protection
provided. Therefore, anything that is sensitive and that would require such
access needs to happen at hyp, hence the code in nvhe running only at hyp.
For more details about pKVM, please refer to Will's talk at KVM Forum 2020:
https://mirrors.edge.kernel.org/pub/linux/kernel/people/will/slides/kvmforum-2020-edited.pdfhttps://www.youtube.com/watch?v=edqJSzsDRxk
[5] https://android-kvm.googlesource.com/linux/+/refs/heads/tabba/el2_fixed_feature_v3
Fuad Tabba (15):
KVM: arm64: placeholder to check if VM is protected
KVM: arm64: Remove trailing whitespace in comment
KVM: arm64: MDCR_EL2 is a 64-bit register
KVM: arm64: Fix names of config register fields
KVM: arm64: Refactor sys_regs.h,c for nVHE reuse
KVM: arm64: Restore mdcr_el2 from vcpu
KVM: arm64: Track value of cptr_el2 in struct kvm_vcpu_arch
KVM: arm64: Add feature register flag definitions
KVM: arm64: Add config register bit definitions
KVM: arm64: Guest exit handlers for nVHE hyp
KVM: arm64: Add trap handlers for protected VMs
KVM: arm64: Move sanitized copies of CPU features
KVM: arm64: Trap access to pVM restricted features
KVM: arm64: Handle protected guests at 32 bits
KVM: arm64: Restrict protected VM capabilities
arch/arm64/include/asm/cpufeature.h | 4 +-
arch/arm64/include/asm/kvm_arm.h | 54 ++-
arch/arm64/include/asm/kvm_asm.h | 2 +-
arch/arm64/include/asm/kvm_fixed_config.h | 188 +++++++++
arch/arm64/include/asm/kvm_host.h | 15 +-
arch/arm64/include/asm/kvm_hyp.h | 5 +-
arch/arm64/include/asm/sysreg.h | 15 +-
arch/arm64/kernel/cpufeature.c | 8 +-
arch/arm64/kvm/Makefile | 2 +-
arch/arm64/kvm/arm.c | 75 +++-
arch/arm64/kvm/debug.c | 2 +-
arch/arm64/kvm/hyp/include/hyp/switch.h | 76 +++-
arch/arm64/kvm/hyp/nvhe/Makefile | 2 +-
arch/arm64/kvm/hyp/nvhe/debug-sr.c | 2 +-
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 6 -
arch/arm64/kvm/hyp/nvhe/switch.c | 72 +++-
arch/arm64/kvm/hyp/nvhe/sys_regs.c | 445 ++++++++++++++++++++++
arch/arm64/kvm/hyp/vhe/debug-sr.c | 2 +-
arch/arm64/kvm/hyp/vhe/switch.c | 12 +-
arch/arm64/kvm/hyp/vhe/sysreg-sr.c | 2 +-
arch/arm64/kvm/pkvm.c | 213 +++++++++++
arch/arm64/kvm/sys_regs.c | 34 +-
arch/arm64/kvm/sys_regs.h | 31 ++
23 files changed, 1172 insertions(+), 95 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
base-commit: 2734d6c1b1a089fb593ef6a23d4b70903526fe0c
--
2.32.0.402.g57bb445576-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Add a function to check whether a VM is protected (under pKVM).
Since the creation of protected VMs isn't enabled yet, this is a
placeholder that always returns false. The intention is for this
to become a check for protected VMs in the future (see Will's RFC
[*]).
No functional change intended.
Signed-off-by: Fuad Tabba <redacted>
[*] https://lore.kernel.org/kvmarm/20210603183347.1695-1-will@kernel.org/
---
arch/arm64/include/asm/kvm_host.h | 5 +++++
1 file changed, 5 insertions(+)
Change the names of hcr_el2 register fields to match the Arm
Architecture Reference Manual. Easier for cross-referencing and
for grepping.
Also, change the name of CPTR_EL2_RES1 to CPTR_NVHE_EL2_RES1,
because res1 bits are different for VHE.
No functional change intended.
Acked-by: Will Deacon <will@kernel.org>
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/include/asm/kvm_arm.h | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
Refactor sys_regs.h and sys_regs.c to make it easier to reuse
common code. It will be used in nVHE in a later patch.
Note that the refactored code uses __inline_bsearch for find_reg
instead of bsearch to avoid copying the bsearch code for nVHE.
No functional change intended.
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/include/asm/sysreg.h | 3 +++
arch/arm64/kvm/sys_regs.c | 30 +-----------------------------
arch/arm64/kvm/sys_regs.h | 31 +++++++++++++++++++++++++++++++
3 files changed, 35 insertions(+), 29 deletions(-)
On deactivating traps, restore the value of mdcr_el2 from the
newly created and preserved host value vcpu context, rather than
directly reading the hardware register.
Up until and including this patch the two values are the same,
i.e., the hardware register and the vcpu one. A future patch will
be changing the value of mdcr_el2 on activating traps, and this
ensures that its value will be restored.
No functional change intended.
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/include/asm/kvm_host.h | 5 ++++-
arch/arm64/include/asm/kvm_hyp.h | 2 +-
arch/arm64/kvm/hyp/include/hyp/switch.h | 6 +++++-
arch/arm64/kvm/hyp/nvhe/switch.c | 11 ++---------
arch/arm64/kvm/hyp/vhe/switch.c | 12 ++----------
arch/arm64/kvm/hyp/vhe/sysreg-sr.c | 2 +-
6 files changed, 15 insertions(+), 23 deletions(-)
@@ -287,10 +287,13 @@ struct kvm_vcpu_arch {/* Stage 2 paging state used by the hardware on next switch */structkvm_s2_mmu*hw_mmu;-/* HYP configuration */+/* Values of trap registers for the guest. */u64hcr_el2;u64mdcr_el2;+/* Values of trap registers for the host before guest entry. */+u64mdcr_el2_host;+/* Exception Information */structkvm_vcpu_fault_infofault;
@@ -91,17 +91,9 @@ void activate_traps_vhe_load(struct kvm_vcpu *vcpu)__activate_traps_common(vcpu);}-voiddeactivate_traps_vhe_put(void)+voiddeactivate_traps_vhe_put(structkvm_vcpu*vcpu){-u64mdcr_el2=read_sysreg(mdcr_el2);--mdcr_el2&=MDCR_EL2_HPMN_MASK|-MDCR_EL2_E2PB_MASK<<MDCR_EL2_E2PB_SHIFT|-MDCR_EL2_TPMS;--write_sysreg(mdcr_el2,mdcr_el2);--__deactivate_traps_common();+__deactivate_traps_common(vcpu);}/* Switch to the guest for VHE systems running in EL2 */
Track the baseline guest value for cptr_el2 in struct
kvm_vcpu_arch, similar to the other registers that control traps.
Use this value when setting cptr_el2 for the guest.
Currently this value is unchanged (CPTR_EL2_DEFAULT), but future
patches will set trapping bits based on features supported for
the guest.
No functional change intended.
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/include/asm/kvm_host.h | 1 +
arch/arm64/kvm/arm.c | 1 +
arch/arm64/kvm/hyp/nvhe/switch.c | 2 +-
3 files changed, 3 insertions(+), 1 deletion(-)
@@ -290,6 +290,7 @@ struct kvm_vcpu_arch {/* Values of trap registers for the guest. */u64hcr_el2;u64mdcr_el2;+u64cptr_el2;/* Values of trap registers for the host before guest entry. */u64mdcr_el2_host;
Add feature register flag definitions to clarify which features
might be supported.
Consolidate the various ID_AA64PFR0_ELx flags for all ELs.
No functional change intended.
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/include/asm/cpufeature.h | 4 ++--
arch/arm64/include/asm/sysreg.h | 12 ++++++++----
arch/arm64/kernel/cpufeature.c | 8 ++++----
3 files changed, 14 insertions(+), 10 deletions(-)
Add an array of pointers to handlers for various trap reasons in
nVHE code.
The current code selects how to fixup a guest on exit based on a
series of if/else statements. Future patches will also require
different handling for guest exists. Create an array of handlers
to consolidate them.
No functional change intended as the array isn't populated yet.
Acked-by: Will Deacon <will@kernel.org>
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/kvm/hyp/include/hyp/switch.h | 43 +++++++++++++++++++++++++
arch/arm64/kvm/hyp/nvhe/switch.c | 35 ++++++++++++++++++++
2 files changed, 78 insertions(+)
@@ -496,6 +536,9 @@ static inline bool fixup_guest_exit(struct kvm_vcpu *vcpu, u64 *exit_code)gotoguest;}+/* Check if there's an exit handler and allow it to handle the exit. */+if(kvm_hyp_handle_exit(vcpu))+gotoguest;exit:/* Return to the host kernel and handle the exit */returnfalse;
@@ -158,6 +158,41 @@ static void __pmu_switch_to_host(struct kvm_cpu_context *host_ctxt)write_sysreg(pmu->events_host,pmcntenset_el0);}+typedefint(*exit_handle_fn)(structkvm_vcpu*);++staticexit_handle_fnhyp_exit_handlers[]={+[0...ESR_ELx_EC_MAX]=NULL,+[ESR_ELx_EC_WFx]=NULL,+[ESR_ELx_EC_CP15_32]=NULL,+[ESR_ELx_EC_CP15_64]=NULL,+[ESR_ELx_EC_CP14_MR]=NULL,+[ESR_ELx_EC_CP14_LS]=NULL,+[ESR_ELx_EC_CP14_64]=NULL,+[ESR_ELx_EC_HVC32]=NULL,+[ESR_ELx_EC_SMC32]=NULL,+[ESR_ELx_EC_HVC64]=NULL,+[ESR_ELx_EC_SMC64]=NULL,+[ESR_ELx_EC_SYS64]=NULL,+[ESR_ELx_EC_SVE]=NULL,+[ESR_ELx_EC_IABT_LOW]=NULL,+[ESR_ELx_EC_DABT_LOW]=NULL,+[ESR_ELx_EC_SOFTSTP_LOW]=NULL,+[ESR_ELx_EC_WATCHPT_LOW]=NULL,+[ESR_ELx_EC_BREAKPT_LOW]=NULL,+[ESR_ELx_EC_BKPT32]=NULL,+[ESR_ELx_EC_BRK64]=NULL,+[ESR_ELx_EC_FP_ASIMD]=NULL,+[ESR_ELx_EC_PAC]=NULL,+};++exit_handle_fnkvm_get_nvhe_exit_handler(structkvm_vcpu*vcpu)+{+u32esr=kvm_vcpu_get_esr(vcpu);+u8esr_ec=ESR_ELx_EC(esr);++returnhyp_exit_handlers[esr_ec];+}+/* Switch to the guest for legacy non-VHE systems */int__kvm_vcpu_run(structkvm_vcpu*vcpu){
--
2.32.0.402.g57bb445576-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Move the sanitized copies of the CPU feature registers to the
recently created sys_regs.c. This consolidates all copies in a
more relevant file.
No functional change intended.
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 6 ------
arch/arm64/kvm/hyp/nvhe/sys_regs.c | 2 ++
2 files changed, 2 insertions(+), 6 deletions(-)
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 <redacted>
---
arch/arm64/kvm/hyp/include/hyp/switch.h | 3 ++
arch/arm64/kvm/hyp/nvhe/switch.c | 52 ++++++++++++++++++-------
2 files changed, 41 insertions(+), 14 deletions(-)
@@ -33,6 +33,9 @@externstructexception_table_entry__start___kvm_ex_table;externstructexception_table_entry__stop___kvm_ex_table;+intkvm_handle_pvm_sys64(structkvm_vcpu*vcpu);+intkvm_handle_pvm_restricted(structkvm_vcpu*vcpu);+/* Check whether the FP regs were dirtied while in the host-side run loop: */staticinlineboolupdate_fp_enabled(structkvm_vcpu*vcpu){
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 <redacted>
---
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
@@ -0,0 +1,443 @@+// SPDX-License-Identifier: GPL-2.0-only+/*+*Copyright(C)2021GoogleLLC+*Author:FuadTabba<tabba@google.com>+*/++#include<linux/kvm_host.h>++#include<asm/kvm_asm.h>+#include<asm/kvm_emulate.h>+#include<asm/kvm_fixed_config.h>+#include<asm/kvm_mmu.h>++#include<hyp/adjust_pc.h>++#include"../../sys_regs.h"++/*+*Copiesofthehost'sCPUfeaturesregistersholdingsanitizedvalues.+*/+u64id_aa64pfr0_el1_sys_val;+u64id_aa64pfr1_el1_sys_val;+u64id_aa64mmfr2_el1_sys_val;++/*+*Injectanunknown/undefinedexceptiontotheguest.+*/+staticvoidinject_undef(structkvm_vcpu*vcpu)+{+u32esr=(ESR_ELx_EC_UNKNOWN<<ESR_ELx_EC_SHIFT);++vcpu->arch.flags|=(KVM_ARM64_EXCEPT_AA64_EL1|+KVM_ARM64_EXCEPT_AA64_ELx_SYNC|+KVM_ARM64_PENDING_EXCEPTION);++__kvm_adjust_pc(vcpu);++write_sysreg_el1(esr,SYS_ESR);+write_sysreg_el1(read_sysreg_el2(SYS_ELR),SYS_ELR);+}++/*+*Accessorforundefinedaccesses.+*/+staticboolundef_access(structkvm_vcpu*vcpu,+structsys_reg_params*p,+conststructsys_reg_desc*r)+{+inject_undef(vcpu);+returnfalse;+}++/*+*Accessorsforfeatureregisters.+*+*Ifaccessisallowed,settheregvaltotheprotectedVM'sviewofthe+*registerandreturntrue.+*Otherwise,injectanundefinedexceptionandreturnfalse.+*/++/*+*Returnstheminimumfeaturesupportedandallowed.+*/+staticu64get_min_feature(u64feature,u64allowed_features,+u64supported_features)+{+constu64allowed_feature=FIELD_GET(feature,allowed_features);+constu64supported_feature=FIELD_GET(feature,supported_features);++returnmin(allowed_feature,supported_feature);+}++/* Accessor for ID_AA64PFR0_EL1. */+staticboolpvm_access_id_aa64pfr0(structkvm_vcpu*vcpu,+structsys_reg_params*p,+conststructsys_reg_desc*r)+{+conststructkvm*kvm=(conststructkvm*)kern_hyp_va(vcpu->kvm);+constu64feature_ids=PVM_ID_AA64PFR0_ALLOW;+u64set_mask=0;+u64clear_mask=0;++if(p->is_write)+returnundef_access(vcpu,p,r);++/* Get the RAS version allowed and supported */+clear_mask|=FEATURE(ID_AA64PFR0_RAS);+set_mask|=FIELD_PREP(FEATURE(ID_AA64PFR0_RAS),+get_min_feature(FEATURE(ID_AA64PFR0_RAS),+feature_ids,+id_aa64pfr0_el1_sys_val));++/* AArch32 guests: if not allowed then force guests to 64-bits only */+clear_mask|=FEATURE(ID_AA64PFR0_EL0)|FEATURE(ID_AA64PFR0_EL1)|+FEATURE(ID_AA64PFR0_EL2)|FEATURE(ID_AA64PFR0_EL3);++set_mask|=FIELD_PREP(FEATURE(ID_AA64PFR0_EL0),+get_min_feature(FEATURE(ID_AA64PFR0_EL0),+feature_ids,+id_aa64pfr0_el1_sys_val));+set_mask|=FIELD_PREP(FEATURE(ID_AA64PFR0_EL1),+get_min_feature(FEATURE(ID_AA64PFR0_EL1),+feature_ids,+id_aa64pfr0_el1_sys_val));+set_mask|=FIELD_PREP(FEATURE(ID_AA64PFR0_EL2),+get_min_feature(FEATURE(ID_AA64PFR0_EL2),+feature_ids,+id_aa64pfr0_el1_sys_val));+set_mask|=FIELD_PREP(FEATURE(ID_AA64PFR0_EL3),+get_min_feature(FEATURE(ID_AA64PFR0_EL3),+feature_ids,+id_aa64pfr0_el1_sys_val));++/* Spectre and Meltdown mitigation */+set_mask|=FIELD_PREP(FEATURE(ID_AA64PFR0_CSV2),+(u64)kvm->arch.pfr0_csv2);+set_mask|=FIELD_PREP(FEATURE(ID_AA64PFR0_CSV3),+(u64)kvm->arch.pfr0_csv3);++p->regval=(id_aa64pfr0_el1_sys_val&feature_ids&~clear_mask)|+set_mask;+returntrue;+}++/* Accessor for ID_AA64PFR1_EL1. */+staticboolpvm_access_id_aa64pfr1(structkvm_vcpu*vcpu,+structsys_reg_params*p,+conststructsys_reg_desc*r)+{+constu64feature_ids=PVM_ID_AA64PFR1_ALLOW;++if(p->is_write)+returnundef_access(vcpu,p,r);++p->regval=id_aa64pfr1_el1_sys_val&feature_ids;+returntrue;+}++/* Accessor for ID_AA64ZFR0_EL1. */+staticboolpvm_access_id_aa64zfr0(structkvm_vcpu*vcpu,+structsys_reg_params*p,+conststructsys_reg_desc*r)+{+if(p->is_write)+returnundef_access(vcpu,p,r);++/*+*NosupportforScalableVectors,therefore,pKVMhasnosanitized+*copyofthefeatureidregister.+*/+BUILD_BUG_ON(PVM_ID_AA64ZFR0_ALLOW!=0ULL);++p->regval=0;+returntrue;+}++/* Accessor for ID_AA64DFR0_EL1. */+staticboolpvm_access_id_aa64dfr0(structkvm_vcpu*vcpu,+structsys_reg_params*p,+conststructsys_reg_desc*r)+{+if(p->is_write)+returnundef_access(vcpu,p,r);++/*+*Nosupportfordebug,includingbreakpoints,andwatchpoints,+*therefore,pKVMhasnosanitizedcopyofthefeatureidregister.+*/+BUILD_BUG_ON(PVM_ID_AA64DFR0_ALLOW!=0ULL);++p->regval=0;+returntrue;+}++/*+*NorestrictionsonID_AA64ISAR1_EL1features,therefore,pKVMhasno+*sanitizedcopyofthefeatureidregisterandit'shandledbythehost.+*/+static_assert(PVM_ID_AA64ISAR1_ALLOW==~0ULL);++/* Accessor for ID_AA64MMFR0_EL1. */+staticboolpvm_access_id_aa64mmfr0(structkvm_vcpu*vcpu,+structsys_reg_params*p,+conststructsys_reg_desc*r)+{+constu64feature_ids=PVM_ID_AA64MMFR0_ALLOW;+u64set_mask=PVM_ID_AA64MMFR0_SET;++if(p->is_write)+returnundef_access(vcpu,p,r);++p->regval=(id_aa64mmfr0_el1_sys_val&feature_ids)|set_mask;+returntrue;+}++/* Accessor for ID_AA64MMFR1_EL1. */+staticboolpvm_access_id_aa64mmfr1(structkvm_vcpu*vcpu,+structsys_reg_params*p,+conststructsys_reg_desc*r)+{+constu64feature_ids=PVM_ID_AA64MMFR1_ALLOW;++if(p->is_write)+returnundef_access(vcpu,p,r);++p->regval=id_aa64mmfr1_el1_sys_val&feature_ids;+returntrue;+}++/* Accessor for ID_AA64MMFR2_EL1. */+staticboolpvm_access_id_aa64mmfr2(structkvm_vcpu*vcpu,+structsys_reg_params*p,+conststructsys_reg_desc*r)+{+constu64feature_ids=PVM_ID_AA64MMFR2_ALLOW;++if(p->is_write)+returnundef_access(vcpu,p,r);++p->regval=id_aa64mmfr2_el1_sys_val&feature_ids;+returntrue;+}++/*+*AccessorforAArch32ProcessorFeatureRegisters.+*+*Thevalueoftheseregistersis"unknown"accordingtothespecifAArch32+*isn'tsupported.+*/+staticboolpvm_access_id_aarch32(structkvm_vcpu*vcpu,+structsys_reg_params*p,+conststructsys_reg_desc*r)+{+if(p->is_write)+returnundef_access(vcpu,p,r);++/*+*NosupportforAArch32guests,therefore,pKVMhasnosanitizedcopy+*ofAArch32featureidregisters.+*/+BUILD_BUG_ON(FIELD_GET(FEATURE(ID_AA64PFR0_EL1),+PVM_ID_AA64PFR0_ALLOW)>ID_AA64PFR0_ELx_64BIT_ONLY);++/* Use 0 for architecturally "unknown" values. */+p->regval=0;+returntrue;+}++/* Mark the specified system register as an AArch32 feature register. */+#define AARCH32(REG) { SYS_DESC(REG), .access = pvm_access_id_aarch32 }++/* Mark the specified system register as not being handled in hyp. */+#define HOST_HANDLED(REG) { SYS_DESC(REG), .access = NULL }++/*+*Architectedsystemregisters.+*Important:MustbesortedascendingbyOp0,Op1,CRn,CRm,Op2+*+*NOTE:Anythingnotexplicitlylistedherewillbe*restrictedbydefault*,+*i.e.,itwillleadtoinjectinganexceptionintotheguest.+*/+staticconststructsys_reg_descpvm_sys_reg_descs[]={+/* Cache maintenance by set/way operations are restricted. */++/* Debug and Trace Registers are all restricted */++/* AArch64 mappings of the AArch32 ID registers */+/* CRm=1 */+AARCH32(SYS_ID_PFR0_EL1),+AARCH32(SYS_ID_PFR1_EL1),+AARCH32(SYS_ID_DFR0_EL1),+AARCH32(SYS_ID_AFR0_EL1),+AARCH32(SYS_ID_MMFR0_EL1),+AARCH32(SYS_ID_MMFR1_EL1),+AARCH32(SYS_ID_MMFR2_EL1),+AARCH32(SYS_ID_MMFR3_EL1),++/* CRm=2 */+AARCH32(SYS_ID_ISAR0_EL1),+AARCH32(SYS_ID_ISAR1_EL1),+AARCH32(SYS_ID_ISAR2_EL1),+AARCH32(SYS_ID_ISAR3_EL1),+AARCH32(SYS_ID_ISAR4_EL1),+AARCH32(SYS_ID_ISAR5_EL1),+AARCH32(SYS_ID_MMFR4_EL1),+AARCH32(SYS_ID_ISAR6_EL1),++/* CRm=3 */+AARCH32(SYS_MVFR0_EL1),+AARCH32(SYS_MVFR1_EL1),+AARCH32(SYS_MVFR2_EL1),+AARCH32(SYS_ID_PFR2_EL1),+AARCH32(SYS_ID_DFR1_EL1),+AARCH32(SYS_ID_MMFR5_EL1),++/* AArch64 ID registers */+/* CRm=4 */+{SYS_DESC(SYS_ID_AA64PFR0_EL1),.access=pvm_access_id_aa64pfr0},+{SYS_DESC(SYS_ID_AA64PFR1_EL1),.access=pvm_access_id_aa64pfr1},+{SYS_DESC(SYS_ID_AA64ZFR0_EL1),.access=pvm_access_id_aa64zfr0},+{SYS_DESC(SYS_ID_AA64DFR0_EL1),.access=pvm_access_id_aa64dfr0},+HOST_HANDLED(SYS_ID_AA64DFR1_EL1),+HOST_HANDLED(SYS_ID_AA64AFR0_EL1),+HOST_HANDLED(SYS_ID_AA64AFR1_EL1),+HOST_HANDLED(SYS_ID_AA64ISAR0_EL1),+HOST_HANDLED(SYS_ID_AA64ISAR1_EL1),+{SYS_DESC(SYS_ID_AA64MMFR0_EL1),.access=pvm_access_id_aa64mmfr0},+{SYS_DESC(SYS_ID_AA64MMFR1_EL1),.access=pvm_access_id_aa64mmfr1},+{SYS_DESC(SYS_ID_AA64MMFR2_EL1),.access=pvm_access_id_aa64mmfr2},++HOST_HANDLED(SYS_SCTLR_EL1),+HOST_HANDLED(SYS_ACTLR_EL1),+HOST_HANDLED(SYS_CPACR_EL1),++HOST_HANDLED(SYS_RGSR_EL1),+HOST_HANDLED(SYS_GCR_EL1),++/* Scalable Vector Registers are restricted. */++HOST_HANDLED(SYS_TTBR0_EL1),+HOST_HANDLED(SYS_TTBR1_EL1),+HOST_HANDLED(SYS_TCR_EL1),++HOST_HANDLED(SYS_APIAKEYLO_EL1),+HOST_HANDLED(SYS_APIAKEYHI_EL1),+HOST_HANDLED(SYS_APIBKEYLO_EL1),+HOST_HANDLED(SYS_APIBKEYHI_EL1),+HOST_HANDLED(SYS_APDAKEYLO_EL1),+HOST_HANDLED(SYS_APDAKEYHI_EL1),+HOST_HANDLED(SYS_APDBKEYLO_EL1),+HOST_HANDLED(SYS_APDBKEYHI_EL1),+HOST_HANDLED(SYS_APGAKEYLO_EL1),+HOST_HANDLED(SYS_APGAKEYHI_EL1),++HOST_HANDLED(SYS_AFSR0_EL1),+HOST_HANDLED(SYS_AFSR1_EL1),+HOST_HANDLED(SYS_ESR_EL1),++HOST_HANDLED(SYS_ERRIDR_EL1),+HOST_HANDLED(SYS_ERRSELR_EL1),+HOST_HANDLED(SYS_ERXFR_EL1),+HOST_HANDLED(SYS_ERXCTLR_EL1),+HOST_HANDLED(SYS_ERXSTATUS_EL1),+HOST_HANDLED(SYS_ERXADDR_EL1),+HOST_HANDLED(SYS_ERXMISC0_EL1),+HOST_HANDLED(SYS_ERXMISC1_EL1),++HOST_HANDLED(SYS_TFSR_EL1),+HOST_HANDLED(SYS_TFSRE0_EL1),++HOST_HANDLED(SYS_FAR_EL1),+HOST_HANDLED(SYS_PAR_EL1),++/* Performance Monitoring Registers are restricted. */++HOST_HANDLED(SYS_MAIR_EL1),+HOST_HANDLED(SYS_AMAIR_EL1),++/* Limited Ordering Regions Registers are restricted. */++HOST_HANDLED(SYS_VBAR_EL1),+HOST_HANDLED(SYS_DISR_EL1),++/* GIC CPU Interface registers are restricted. */++HOST_HANDLED(SYS_CONTEXTIDR_EL1),+HOST_HANDLED(SYS_TPIDR_EL1),++HOST_HANDLED(SYS_SCXTNUM_EL1),++HOST_HANDLED(SYS_CNTKCTL_EL1),++HOST_HANDLED(SYS_CCSIDR_EL1),+HOST_HANDLED(SYS_CLIDR_EL1),+HOST_HANDLED(SYS_CSSELR_EL1),+HOST_HANDLED(SYS_CTR_EL0),++/* Performance Monitoring Registers are restricted. */++HOST_HANDLED(SYS_TPIDR_EL0),+HOST_HANDLED(SYS_TPIDRRO_EL0),++HOST_HANDLED(SYS_SCXTNUM_EL0),++/* Activity Monitoring Registers are restricted. */++HOST_HANDLED(SYS_CNTP_TVAL_EL0),+HOST_HANDLED(SYS_CNTP_CTL_EL0),+HOST_HANDLED(SYS_CNTP_CVAL_EL0),++/* Performance Monitoring Registers are restricted. */++HOST_HANDLED(SYS_DACR32_EL2),+HOST_HANDLED(SYS_IFSR32_EL2),+HOST_HANDLED(SYS_FPEXC32_EL2),+};++/*+*HandlerforprotectedVMMSR,MRSorSysteminstructionexecutioninAArch64.+*+*Return1ifhandled,or0ifnot.+*/+intkvm_handle_pvm_sys64(structkvm_vcpu*vcpu)+{+conststructsys_reg_desc*r;+structsys_reg_paramsparams;+unsignedlongesr=kvm_vcpu_get_esr(vcpu);+intRt=kvm_vcpu_sys_get_rt(vcpu);++params=esr_sys64_to_params(esr);+params.regval=vcpu_get_reg(vcpu,Rt);++r=find_reg(¶ms,pvm_sys_reg_descs,ARRAY_SIZE(pvm_sys_reg_descs));++/* Undefined access (RESTRICTED). */+if(r==NULL){+inject_undef(vcpu);+return1;+}++/* Handled by the host (HOST_HANDLED) */+if(r->access==NULL)+return0;++/* Handled by hyp: skip instruction if instructed to do so. */+if(r->access(vcpu,¶ms,r))+__kvm_skip_instr(vcpu);++vcpu_set_reg(vcpu,Rt,params.regval);+return1;+}++/*+*HandlerforprotectedVMrestrictedexceptions.+*+*Injectanundefinedexceptionintotheguestandreturn1toindicatethat+*itwashandled.+*/+intkvm_handle_pvm_restricted(structkvm_vcpu*vcpu)+{+inject_undef(vcpu);+return1;+}
Protected KVM does not support protected AArch32 guests. However,
it is possible for the guest to force run AArch32, potentially
causing problems. Add an extra check so that if the hypervisor
catches the guest doing that, it can prevent the guest from
running again by resetting vcpu->arch.target and returning
ARM_EXCEPTION_IL.
Adapted from commit 22f553842b14 ("KVM: arm64: Handle Asymmetric
AArch32 systems")
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/kvm/hyp/include/hyp/switch.h | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
@@ -281,6 +283,65 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)returnr;}+staticintpkvm_check_extension(structkvm*kvm,longext,intkvm_cap)+{+intr;++switch(ext){+caseKVM_CAP_ARM_PSCI:+caseKVM_CAP_ARM_PSCI_0_2:+caseKVM_CAP_NR_VCPUS:+caseKVM_CAP_MAX_VCPUS:+caseKVM_CAP_MAX_VCPU_ID:+r=kvm_cap;+break;+caseKVM_CAP_ARM_EL1_32BIT:+r=kvm_cap&&+(FIELD_GET(FEATURE(ID_AA64PFR0_EL1),PVM_ID_AA64PFR0_ALLOW)>=+ID_AA64PFR0_ELx_32BIT_64BIT);+break;+caseKVM_CAP_GUEST_DEBUG_HW_BPS:+r=min(kvm_cap,kvm_arm_pkvm_get_max_brps());+break;+caseKVM_CAP_GUEST_DEBUG_HW_WPS:+r=min(kvm_cap,kvm_arm_pkvm_get_max_wrps());+break;+caseKVM_CAP_ARM_PMU_V3:+r=kvm_cap&&+FIELD_GET(FEATURE(ID_AA64DFR0_PMUVER),PVM_ID_AA64DFR0_ALLOW);+break;+caseKVM_CAP_ARM_SVE:+r=kvm_cap&&+FIELD_GET(FEATURE(ID_AA64PFR0_SVE),PVM_ID_AA64PFR0_ALLOW);+break;+caseKVM_CAP_ARM_PTRAUTH_ADDRESS:+r=kvm_cap&&+FIELD_GET(FEATURE(ID_AA64ISAR1_API),PVM_ID_AA64ISAR1_ALLOW)&&+FIELD_GET(FEATURE(ID_AA64ISAR1_APA),PVM_ID_AA64ISAR1_ALLOW);+break;+caseKVM_CAP_ARM_PTRAUTH_GENERIC:+r=kvm_cap&&+FIELD_GET(FEATURE(ID_AA64ISAR1_GPI),PVM_ID_AA64ISAR1_ALLOW)&&+FIELD_GET(FEATURE(ID_AA64ISAR1_GPA),PVM_ID_AA64ISAR1_ALLOW);+break;+default:+r=0;+break;+}++returnr;+}++intkvm_vm_ioctl_check_extension(structkvm*kvm,longext)+{+intr=kvm_check_extension(kvm,ext);++if(unlikely(kvm&&kvm_vm_is_protected(kvm)))+r=pkvm_check_extension(kvm,ext,r);++returnr;+}+longkvm_arch_dev_ioctl(structfile*filp,unsignedintioctl,unsignedlongarg){
From: Oliver Upton <hidden> Date: 2021-07-19 19:46:31
On Mon, Jul 19, 2021 at 9:04 AM Fuad Tabba [off-list ref] wrote:
Protected KVM does not support protected AArch32 guests. However,
it is possible for the guest to force run AArch32, potentially
causing problems. Add an extra check so that if the hypervisor
catches the guest doing that, it can prevent the guest from
running again by resetting vcpu->arch.target and returning
ARM_EXCEPTION_IL.
Adapted from commit 22f553842b14 ("KVM: arm64: Handle Asymmetric
AArch32 systems")
Signed-off-by: Fuad Tabba <redacted>
Would it make sense to document how we handle misbehaved guests, in
case a particular VMM wants to clean up the mess afterwards?
--
Thanks,
Oliver
From: Andrew Jones <hidden> Date: 2021-07-20 13:40:27
On Mon, Jul 19, 2021 at 05:03:36PM +0100, Fuad Tabba wrote:
quoted hunk
Refactor sys_regs.h and sys_regs.c to make it easier to reuse
common code. It will be used in nVHE in a later patch.
Note that the refactored code uses __inline_bsearch for find_reg
instead of bsearch to avoid copying the bsearch code for nVHE.
No functional change intended.
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/include/asm/sysreg.h | 3 +++
arch/arm64/kvm/sys_regs.c | 30 +-----------------------------
arch/arm64/kvm/sys_regs.h | 31 +++++++++++++++++++++++++++++++
3 files changed, 35 insertions(+), 29 deletions(-)
@@ -1153,6 +1153,9 @@#define ICH_VTR_A3V_SHIFT 21#define ICH_VTR_A3V_MASK (1 << ICH_VTR_A3V_SHIFT)+/* Extract the feature specified from the feature id register. */+#define FEATURE(x) (GENMASK_ULL(x##_SHIFT + 3, x##_SHIFT))
I think the comment would be better as
Create a mask for the feature bits of the specified feature.
And, I think a more specific name than FEATURE would be better. Maybe
FEATURE_MASK or even ARM64_FEATURE_MASK ?
Hi,
On Tue, Jul 20, 2021 at 2:38 PM Andrew Jones [off-list ref] wrote:
On Mon, Jul 19, 2021 at 05:03:36PM +0100, Fuad Tabba wrote:
quoted
Refactor sys_regs.h and sys_regs.c to make it easier to reuse
common code. It will be used in nVHE in a later patch.
Note that the refactored code uses __inline_bsearch for find_reg
instead of bsearch to avoid copying the bsearch code for nVHE.
No functional change intended.
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/include/asm/sysreg.h | 3 +++
arch/arm64/kvm/sys_regs.c | 30 +-----------------------------
arch/arm64/kvm/sys_regs.h | 31 +++++++++++++++++++++++++++++++
3 files changed, 35 insertions(+), 29 deletions(-)
From: Andrew Jones <hidden> Date: 2021-07-20 14:55:05
On Mon, Jul 19, 2021 at 05:03:37PM +0100, Fuad Tabba wrote:
On deactivating traps, restore the value of mdcr_el2 from the
newly created and preserved host value vcpu context, rather than
directly reading the hardware register.
Up until and including this patch the two values are the same,
i.e., the hardware register and the vcpu one. A future patch will
be changing the value of mdcr_el2 on activating traps, and this
ensures that its value will be restored.
No functional change intended.
I'm probably missing something, but I can't convince myself that the host
will end up with the same mdcr_el2 value after deactivating traps after
this patch as before. We clearly now restore whatever we had when
activating traps (presumably whatever we configured at init_el2_state
time), but is that equivalent to what we had before with the masking and
ORing that this patch drops?
Thanks,
drew
@@ -287,10 +287,13 @@ struct kvm_vcpu_arch {/* Stage 2 paging state used by the hardware on next switch */structkvm_s2_mmu*hw_mmu;-/* HYP configuration */+/* Values of trap registers for the guest. */u64hcr_el2;u64mdcr_el2;+/* Values of trap registers for the host before guest entry. */+u64mdcr_el2_host;+/* Exception Information */structkvm_vcpu_fault_infofault;
@@ -91,17 +91,9 @@ void activate_traps_vhe_load(struct kvm_vcpu *vcpu)__activate_traps_common(vcpu);}-voiddeactivate_traps_vhe_put(void)+voiddeactivate_traps_vhe_put(structkvm_vcpu*vcpu){-u64mdcr_el2=read_sysreg(mdcr_el2);--mdcr_el2&=MDCR_EL2_HPMN_MASK|-MDCR_EL2_E2PB_MASK<<MDCR_EL2_E2PB_SHIFT|-MDCR_EL2_TPMS;--write_sysreg(mdcr_el2,mdcr_el2);--__deactivate_traps_common();+__deactivate_traps_common(vcpu);}/* Switch to the guest for VHE systems running in EL2 */
Hi Drew,
On Tue, Jul 20, 2021 at 3:53 PM Andrew Jones [off-list ref] wrote:
On Mon, Jul 19, 2021 at 05:03:37PM +0100, Fuad Tabba wrote:
quoted
On deactivating traps, restore the value of mdcr_el2 from the
newly created and preserved host value vcpu context, rather than
directly reading the hardware register.
Up until and including this patch the two values are the same,
i.e., the hardware register and the vcpu one. A future patch will
be changing the value of mdcr_el2 on activating traps, and this
ensures that its value will be restored.
No functional change intended.
I'm probably missing something, but I can't convince myself that the host
will end up with the same mdcr_el2 value after deactivating traps after
this patch as before. We clearly now restore whatever we had when
activating traps (presumably whatever we configured at init_el2_state
time), but is that equivalent to what we had before with the masking and
ORing that this patch drops?
You're right. I thought that these were actually being initialized to
the same values, but having a closer look at the code the mdcr values
are not the same as pre-patch. I will fix this.
Thanks!
/fuad
@@ -287,10 +287,13 @@ struct kvm_vcpu_arch {/* Stage 2 paging state used by the hardware on next switch */structkvm_s2_mmu*hw_mmu;-/* HYP configuration */+/* Values of trap registers for the guest. */u64hcr_el2;u64mdcr_el2;+/* Values of trap registers for the host before guest entry. */+u64mdcr_el2_host;+/* Exception Information */structkvm_vcpu_fault_infofault;
@@ -91,17 +91,9 @@ void activate_traps_vhe_load(struct kvm_vcpu *vcpu)__activate_traps_common(vcpu);}-voiddeactivate_traps_vhe_put(void)+voiddeactivate_traps_vhe_put(structkvm_vcpu*vcpu){-u64mdcr_el2=read_sysreg(mdcr_el2);--mdcr_el2&=MDCR_EL2_HPMN_MASK|-MDCR_EL2_E2PB_MASK<<MDCR_EL2_E2PB_SHIFT|-MDCR_EL2_TPMS;--write_sysreg(mdcr_el2,mdcr_el2);--__deactivate_traps_common();+__deactivate_traps_common(vcpu);}/* Switch to the guest for VHE systems running in EL2 */
Hi Oliver,
On Mon, Jul 19, 2021 at 8:43 PM Oliver Upton [off-list ref] wrote:
On Mon, Jul 19, 2021 at 9:04 AM Fuad Tabba [off-list ref] wrote:
quoted
Protected KVM does not support protected AArch32 guests. However,
it is possible for the guest to force run AArch32, potentially
causing problems. Add an extra check so that if the hypervisor
catches the guest doing that, it can prevent the guest from
running again by resetting vcpu->arch.target and returning
ARM_EXCEPTION_IL.
Adapted from commit 22f553842b14 ("KVM: arm64: Handle Asymmetric
AArch32 systems")
Signed-off-by: Fuad Tabba <redacted>
Would it make sense to document how we handle misbehaved guests, in
case a particular VMM wants to clean up the mess afterwards?
I agree, especially since with this patch this could happen in more
than one place.
Thanks,
/fuad
From: Will Deacon <will@kernel.org> Date: 2021-08-03 15:34:01
On Mon, Jul 19, 2021 at 05:03:41PM +0100, Fuad Tabba wrote:
Add an array of pointers to handlers for various trap reasons in
nVHE code.
The current code selects how to fixup a guest on exit based on a
series of if/else statements. Future patches will also require
different handling for guest exists. Create an array of handlers
to consolidate them.
No functional change intended as the array isn't populated yet.
Acked-by: Will Deacon <will@kernel.org>
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/kvm/hyp/include/hyp/switch.h | 43 +++++++++++++++++++++++++
arch/arm64/kvm/hyp/nvhe/switch.c | 35 ++++++++++++++++++++
2 files changed, 78 insertions(+)
Definitely keep my Ack on this, but Clang just chucked out a warning due to:
Which leads to:
arch/arm64/kvm/hyp/nvhe/switch.c:189:15: warning: redefinition of typedef 'exit_handle_fn' is a C11 feature [-Wtypedef-redefinition]
typedef int (*exit_handle_fn)(struct kvm_vcpu *);
^
./arch/arm64/kvm/hyp/include/hyp/switch.h:416:15: note: previous definition is here
typedef int (*exit_handle_fn)(struct kvm_vcpu *);
^
1 warning generated.
So I guess just pick your favourite?
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Will Deacon <will@kernel.org> Date: 2021-08-12 08:47:39
On Wed, Jul 21, 2021 at 08:37:21AM +0100, Fuad Tabba wrote:
On Tue, Jul 20, 2021 at 3:53 PM Andrew Jones [off-list ref] wrote:
quoted
On Mon, Jul 19, 2021 at 05:03:37PM +0100, Fuad Tabba wrote:
quoted
On deactivating traps, restore the value of mdcr_el2 from the
newly created and preserved host value vcpu context, rather than
directly reading the hardware register.
Up until and including this patch the two values are the same,
i.e., the hardware register and the vcpu one. A future patch will
be changing the value of mdcr_el2 on activating traps, and this
ensures that its value will be restored.
No functional change intended.
I'm probably missing something, but I can't convince myself that the host
will end up with the same mdcr_el2 value after deactivating traps after
this patch as before. We clearly now restore whatever we had when
activating traps (presumably whatever we configured at init_el2_state
time), but is that equivalent to what we had before with the masking and
ORing that this patch drops?
You're right. I thought that these were actually being initialized to
the same values, but having a closer look at the code the mdcr values
are not the same as pre-patch. I will fix this.
Can you elaborate on the issue here, please? I was just looking at this
but aren't you now relying on __init_el2_debug to configure this, which
should be fine?
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Will Deacon <will@kernel.org> Date: 2021-08-12 09:01:50
On Mon, Jul 19, 2021 at 05:03:40PM +0100, Fuad Tabba wrote:
Add hardware configuration register bit definitions for HCR_EL2
and MDCR_EL2. Future patches toggle these hyp configuration
register bits to trap on certain accesses.
No functional change intended.
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/include/asm/kvm_arm.h | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
I checked all of these against the Arm ARM and they look correct to me:
Acked-by: Will Deacon <will@kernel.org>
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Will Deacon <will@kernel.org> Date: 2021-08-12 09:01:54
On Mon, Jul 19, 2021 at 05:03:32PM +0100, Fuad Tabba wrote:
Add a function to check whether a VM is protected (under pKVM).
Since the creation of protected VMs isn't enabled yet, this is a
placeholder that always returns false. The intention is for this
to become a check for protected VMs in the future (see Will's RFC
[*]).
No functional change intended.
Signed-off-by: Fuad Tabba <redacted>
[*] https://lore.kernel.org/kvmarm/20210603183347.1695-1-will@kernel.org/
You can make this a Link: tag.
Anyway, I think it makes lots of sense to decouple this from the user-ABI
series:
Acked-by: Will Deacon <will@kernel.org>
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
I'd usually be against these sorts of changes but given you're in the
area...
Acked-by: Will Deacon <will@kernel.org>
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Will Deacon <will@kernel.org> Date: 2021-08-12 09:02:13
On Mon, Jul 19, 2021 at 05:03:36PM +0100, Fuad Tabba wrote:
Refactor sys_regs.h and sys_regs.c to make it easier to reuse
common code. It will be used in nVHE in a later patch.
Note that the refactored code uses __inline_bsearch for find_reg
instead of bsearch to avoid copying the bsearch code for nVHE.
No functional change intended.
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/include/asm/sysreg.h | 3 +++
arch/arm64/kvm/sys_regs.c | 30 +-----------------------------
arch/arm64/kvm/sys_regs.h | 31 +++++++++++++++++++++++++++++++
3 files changed, 35 insertions(+), 29 deletions(-)
With the naming change suggested by Drew:
Acked-by: Will Deacon <will@kernel.org.
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Will Deacon <will@kernel.org> Date: 2021-08-12 09:02:35
On Mon, Jul 19, 2021 at 05:03:38PM +0100, Fuad Tabba wrote:
Track the baseline guest value for cptr_el2 in struct
kvm_vcpu_arch, similar to the other registers that control traps.
Use this value when setting cptr_el2 for the guest.
Currently this value is unchanged (CPTR_EL2_DEFAULT), but future
patches will set trapping bits based on features supported for
the guest.
No functional change intended.
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/include/asm/kvm_host.h | 1 +
arch/arm64/kvm/arm.c | 1 +
arch/arm64/kvm/hyp/nvhe/switch.c | 2 +-
3 files changed, 3 insertions(+), 1 deletion(-)
From: Will Deacon <will@kernel.org> Date: 2021-08-12 09:02:48
On Mon, Jul 19, 2021 at 05:03:39PM +0100, Fuad Tabba wrote:
quoted hunk
Add feature register flag definitions to clarify which features
might be supported.
Consolidate the various ID_AA64PFR0_ELx flags for all ELs.
No functional change intended.
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/include/asm/cpufeature.h | 4 ++--
arch/arm64/include/asm/sysreg.h | 12 ++++++++----
arch/arm64/kernel/cpufeature.c | 8 ++++----
3 files changed, 14 insertions(+), 10 deletions(-)
This doesn't correspond to an architectural definition afaict: the manual
says that any values other than 0, 1 or 2 are "reserved" so we should avoid
defining our own definitions here.
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Will,
On Thu, Aug 12, 2021 at 10:59 AM Will Deacon [off-list ref] wrote:
On Mon, Jul 19, 2021 at 05:03:39PM +0100, Fuad Tabba wrote:
quoted
Add feature register flag definitions to clarify which features
might be supported.
Consolidate the various ID_AA64PFR0_ELx flags for all ELs.
No functional change intended.
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/include/asm/cpufeature.h | 4 ++--
arch/arm64/include/asm/sysreg.h | 12 ++++++++----
arch/arm64/kernel/cpufeature.c | 8 ++++----
3 files changed, 14 insertions(+), 10 deletions(-)
This doesn't correspond to an architectural definition afaict: the manual
says that any values other than 0, 1 or 2 are "reserved" so we should avoid
defining our own definitions here.
I'll add a ID_AA64PFR0_RAS_V2 definition in that case and use it for
the checking later. That would achieve the same goal and I wouldn't be
adding definitions to the reserved area.
Cheers,
/fuad
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Will,
On Thu, Aug 12, 2021 at 10:59 AM Will Deacon [off-list ref] wrote:
On Mon, Jul 19, 2021 at 05:03:32PM +0100, Fuad Tabba wrote:
quoted
Add a function to check whether a VM is protected (under pKVM).
Since the creation of protected VMs isn't enabled yet, this is a
placeholder that always returns false. The intention is for this
to become a check for protected VMs in the future (see Will's RFC
[*]).
No functional change intended.
Signed-off-by: Fuad Tabba <redacted>
[*] https://lore.kernel.org/kvmarm/20210603183347.1695-1-will@kernel.org/
You can make this a Link: tag.
Of course. Thanks!
/fuad
Anyway, I think it makes lots of sense to decouple this from the user-ABI
series:
Acked-by: Will Deacon <will@kernel.org>
Will
Hi Will,
On Thu, Aug 12, 2021 at 10:46 AM Will Deacon [off-list ref] wrote:
On Wed, Jul 21, 2021 at 08:37:21AM +0100, Fuad Tabba wrote:
quoted
On Tue, Jul 20, 2021 at 3:53 PM Andrew Jones [off-list ref] wrote:
quoted
On Mon, Jul 19, 2021 at 05:03:37PM +0100, Fuad Tabba wrote:
quoted
On deactivating traps, restore the value of mdcr_el2 from the
newly created and preserved host value vcpu context, rather than
directly reading the hardware register.
Up until and including this patch the two values are the same,
i.e., the hardware register and the vcpu one. A future patch will
be changing the value of mdcr_el2 on activating traps, and this
ensures that its value will be restored.
No functional change intended.
I'm probably missing something, but I can't convince myself that the host
will end up with the same mdcr_el2 value after deactivating traps after
this patch as before. We clearly now restore whatever we had when
activating traps (presumably whatever we configured at init_el2_state
time), but is that equivalent to what we had before with the masking and
ORing that this patch drops?
You're right. I thought that these were actually being initialized to
the same values, but having a closer look at the code the mdcr values
are not the same as pre-patch. I will fix this.
Can you elaborate on the issue here, please? I was just looking at this
but aren't you now relying on __init_el2_debug to configure this, which
should be fine?
I *think* that it should be fine, but as Drew pointed out, the host
does not end up with the same mdcr_el2 value after the deactivation in
this patch as it did after deactivation before this patch. In my v4
(not sent out yet), I have fixed it to ensure that the host does end
up with the same value as the one before this patch. That should make
it easier to check that there's no functional change.
I'll look into it further, and if I can convince myself that there
aren't any issues and that this patch makes the code cleaner, I will
add it as a separate patch instead to make reviewing easier.
Thanks,
/fuad
From: Will Deacon <will@kernel.org> Date: 2021-08-12 09:49:34
On Mon, Jul 19, 2021 at 05:03:43PM +0100, Fuad Tabba wrote:
Move the sanitized copies of the CPU feature registers to the
recently created sys_regs.c. This consolidates all copies in a
more relevant file.
No functional change intended.
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 6 ------
arch/arm64/kvm/hyp/nvhe/sys_regs.c | 2 ++
2 files changed, 2 insertions(+), 6 deletions(-)
From: Will Deacon <will@kernel.org> Date: 2021-08-12 09:49:35
On Mon, Jul 19, 2021 at 05:03:42PM +0100, Fuad Tabba wrote:
quoted hunk
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 <redacted>
---
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
+ *
+ * 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., 0b1111),
+ * 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 :)
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 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
Perhaps expand on "support" here? E.g. "context-switching and trapping
support at EL2".
+ */
+#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.
+ */
I think the complexity of the debug architecture is a good reason to avoid
exposing it here, but I don't understand how providing breakpoints or
watchpoints to a guest could risk leaking guest data. What is the specific
threat here?
+#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.
s/confiruation/configuration/
However, I don't agree that this provides determinism since we're not
forcing any particular values, but rather filtering the values from the
host.
+ * - 40-bit IPA
This seems more about not supporting KVM_CAP_ARM_VM_IPA_SIZE for now.
+ * - 16-bit ASID
+ * - Mixed-endian
+ * - Distinction between Secure and Non-secure Memory
+ * - Mixed-endian at EL0 only
+ * - Non-context synchronizing exception entry and exit
These all seem to fall into the "cannot trap" category, so we just advertise
whatever we've got.
Why not, and can we actually prevent the guest from doing that?
+/*
+ * These features are chosen because they are supported by KVM and to limit the
+ * confiruation state space and make it more deterministic.
It's that typo again ;) But my comment from before still applies -- I don't
think an ALLOW mask adds hugely to the determinism.
+ * - Hardware translation table updates to Access flag and Dirty state
+ * - Number of VMID bits from CPU
+ * - Hierarchical Permission Disables
+ * - Privileged Access Never
+ * - SError interrupt exceptions from speculative reads
+ * - Enhanced Translation Synchronization
As before, I think this is a mixture of "trivial" and "cannot trap"
features.
+ */
+#define PVM_ID_AA64MMFR1_ALLOW (\
+ FEATURE(ID_AA64MMFR1_HADBS) | \
+ FEATURE(ID_AA64MMFR1_VMIDBITS) | \
+ FEATURE(ID_AA64MMFR1_HPD) | \
+ FEATURE(ID_AA64MMFR1_PAN) | \
+ FEATURE(ID_AA64MMFR1_SPECSEI) | \
+ FEATURE(ID_AA64MMFR1_ETS) \
+ )
+
+/*
+ * These features are chosen because they are supported by KVM and to limit the
+ * confiruation state space and make it more deterministic.
<same comment>
quoted hunk
+ * - Common not Private translations
+ * - User Access Override
+ * - IESB bit in the SCTLR_ELx registers
+ * - Unaligned single-copy atomicity and atomic functions
+ * - ESR_ELx.EC value on an exception by read access to feature ID space
+ * - TTL field in address operations.
+ * - Break-before-make sequences when changing translation block size
+ * - E0PDx mechanism
+ */
+#define PVM_ID_AA64MMFR2_ALLOW (\
+ FEATURE(ID_AA64MMFR2_CNP) | \
+ FEATURE(ID_AA64MMFR2_UAO) | \
+ FEATURE(ID_AA64MMFR2_IESB) | \
+ FEATURE(ID_AA64MMFR2_AT) | \
+ FEATURE(ID_AA64MMFR2_IDS) | \
+ FEATURE(ID_AA64MMFR2_TTL) | \
+ FEATURE(ID_AA64MMFR2_BBM) | \
+ FEATURE(ID_AA64MMFR2_E0PD) \
+ )
+
+/*
+ * Allow all features in this register because they are trivial to support, or
+ * are already supported by KVM:
+ * - LS64
+ * - XS
+ * - I8MM
+ * - DGB
+ * - BF16
+ * - SPECRES
+ * - SB
+ * - FRINTTS
+ * - PAuth
+ * - FPAC
+ * - LRCPC
+ * - FCMA
+ * - JSCVT
+ * - DPB
+ */
+#define PVM_ID_AA64ISAR1_ALLOW (~0ULL)
+
+#endif /* __ARM64_KVM_FIXED_CONFIG_H__ */
Careful here: this is an unsigned comparison, yet some fields are signed.
cpufeature.c uses the S_ARM64_FTR_BITS and ARM64_FTR_BITS to declare signed
and unsigned fields respectively.
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Will Deacon <will@kernel.org> Date: 2021-08-12 09:51:37
Hey Fuad,
On Thu, Aug 12, 2021 at 11:28:50AM +0200, Fuad Tabba wrote:
On Thu, Aug 12, 2021 at 10:46 AM Will Deacon [off-list ref] wrote:
quoted
On Wed, Jul 21, 2021 at 08:37:21AM +0100, Fuad Tabba wrote:
quoted
On Tue, Jul 20, 2021 at 3:53 PM Andrew Jones [off-list ref] wrote:
quoted
On Mon, Jul 19, 2021 at 05:03:37PM +0100, Fuad Tabba wrote:
quoted
On deactivating traps, restore the value of mdcr_el2 from the
newly created and preserved host value vcpu context, rather than
directly reading the hardware register.
Up until and including this patch the two values are the same,
i.e., the hardware register and the vcpu one. A future patch will
be changing the value of mdcr_el2 on activating traps, and this
ensures that its value will be restored.
No functional change intended.
I'm probably missing something, but I can't convince myself that the host
will end up with the same mdcr_el2 value after deactivating traps after
this patch as before. We clearly now restore whatever we had when
activating traps (presumably whatever we configured at init_el2_state
time), but is that equivalent to what we had before with the masking and
ORing that this patch drops?
You're right. I thought that these were actually being initialized to
the same values, but having a closer look at the code the mdcr values
are not the same as pre-patch. I will fix this.
Can you elaborate on the issue here, please? I was just looking at this
but aren't you now relying on __init_el2_debug to configure this, which
should be fine?
I *think* that it should be fine, but as Drew pointed out, the host
does not end up with the same mdcr_el2 value after the deactivation in
this patch as it did after deactivation before this patch. In my v4
(not sent out yet), I have fixed it to ensure that the host does end
up with the same value as the one before this patch. That should make
it easier to check that there's no functional change.
I'll look into it further, and if I can convince myself that there
aren't any issues and that this patch makes the code cleaner, I will
add it as a separate patch instead to make reviewing easier.
Cheers. I think the new code might actually be better, as things like
MDCR_EL2.E2PB are RES0 if SPE is not implemented. The init code takes care
to set those only if if probes SPE first, whereas the code you're removing
doesn't seem to check that.
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Will Deacon <will@kernel.org> Date: 2021-08-12 09:56:26
On Mon, Jul 19, 2021 at 05:03:44PM +0100, Fuad Tabba wrote:
quoted hunk
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 <redacted>
---
arch/arm64/kvm/hyp/include/hyp/switch.h | 3 ++
arch/arm64/kvm/hyp/nvhe/switch.c | 52 ++++++++++++++++++-------
2 files changed, 41 insertions(+), 14 deletions(-)
@@ -33,6 +33,9 @@externstructexception_table_entry__start___kvm_ex_table;externstructexception_table_entry__stop___kvm_ex_table;+intkvm_handle_pvm_sys64(structkvm_vcpu*vcpu);+intkvm_handle_pvm_restricted(structkvm_vcpu*vcpu);+/* Check whether the FP regs were dirtied while in the host-side run loop: */staticinlineboolupdate_fp_enabled(structkvm_vcpu*vcpu){
Why don't we move the kvm_vm_is_protected() check into
kvm_get_hyp_exit_handler() so we can avoid adding it to each handler
instead?
Either way:
Acked-by: Will Deacon <will@kernel.org>
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Will Deacon <will@kernel.org> Date: 2021-08-12 10:00:31
On Mon, Jul 19, 2021 at 05:03:45PM +0100, Fuad Tabba wrote:
quoted hunk
Protected KVM does not support protected AArch32 guests. However,
it is possible for the guest to force run AArch32, potentially
causing problems. Add an extra check so that if the hypervisor
catches the guest doing that, it can prevent the guest from
running again by resetting vcpu->arch.target and returning
ARM_EXCEPTION_IL.
Adapted from commit 22f553842b14 ("KVM: arm64: Handle Asymmetric
AArch32 systems")
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/kvm/hyp/include/hyp/switch.h | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
Would this be better off inside the nvhe-specific run loop? Seems like we
could elide fixup_guest_exit() altogether if we've detect that we're in
AArch32 state when we shouldn't be and it would keep the code off the shared
path.
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Will Deacon <will@kernel.org> Date: 2021-08-12 10:02:39
On Mon, Jul 19, 2021 at 05:03:46PM +0100, Fuad Tabba wrote:
Restrict protected VM capabilities based on the
fixed-configuration for protected VMs.
No functional change intended in current KVM-supported modes
(nVHE, VHE).
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/include/asm/kvm_fixed_config.h | 10 ++++
arch/arm64/kvm/arm.c | 63 ++++++++++++++++++++++-
arch/arm64/kvm/pkvm.c | 30 +++++++++++
3 files changed, 102 insertions(+), 1 deletion(-)
This patch looks good to me, but I'd be inclined to add this to the user-ABI
series given that it's really all user-facing and, without a functional
kvm_vm_is_protected(), isn't serving much purpose.
Cheers,
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Will,
On Thu, Aug 12, 2021 at 11:57 AM Will Deacon [off-list ref] wrote:
On Mon, Jul 19, 2021 at 05:03:45PM +0100, Fuad Tabba wrote:
quoted
Protected KVM does not support protected AArch32 guests. However,
it is possible for the guest to force run AArch32, potentially
causing problems. Add an extra check so that if the hypervisor
catches the guest doing that, it can prevent the guest from
running again by resetting vcpu->arch.target and returning
ARM_EXCEPTION_IL.
Adapted from commit 22f553842b14 ("KVM: arm64: Handle Asymmetric
AArch32 systems")
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/kvm/hyp/include/hyp/switch.h | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
Would this be better off inside the nvhe-specific run loop? Seems like we
could elide fixup_guest_exit() altogether if we've detect that we're in
AArch32 state when we shouldn't be and it would keep the code off the shared
path.
Yes, it makes more sense and would result in cleaner code to have it
there, especially in the future where there's likely to be a separate
run loop for protected VMs. I'll move it.
Thanks,
/fuad
Hi Will,
On Thu, Aug 12, 2021 at 11:59 AM Will Deacon [off-list ref] wrote:
On Mon, Jul 19, 2021 at 05:03:46PM +0100, Fuad Tabba wrote:
quoted
Restrict protected VM capabilities based on the
fixed-configuration for protected VMs.
No functional change intended in current KVM-supported modes
(nVHE, VHE).
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/include/asm/kvm_fixed_config.h | 10 ++++
arch/arm64/kvm/arm.c | 63 ++++++++++++++++++++++-
arch/arm64/kvm/pkvm.c | 30 +++++++++++
3 files changed, 102 insertions(+), 1 deletion(-)
This patch looks good to me, but I'd be inclined to add this to the user-ABI
series given that it's really all user-facing and, without a functional
kvm_vm_is_protected(), isn't serving much purpose.
Hi Will,
On Thu, Aug 12, 2021 at 11:46 AM Will Deacon [off-list ref] wrote:
On Mon, Jul 19, 2021 at 05:03:42PM +0100, Fuad Tabba wrote:
quoted
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 <redacted>
---
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
+ *
+ * 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., 0b1111),
+ * 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".
quoted
+ *
+ * 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.
quoted
+ * - are trivial, e.g., supporting the feature doesn't introduce or require the
+ * tracking of additional state
... in KVM.
Ack.
quoted
+ * - not trapable
s/not trapable/cannot be trapped/
Ack
quoted
+ */
+
+/*
+ * - 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
quoted
+ * - 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
quoted
+ * - RAS (v1)
+ * v1 doesn't require much additional support, but later versions do.
Be more specific?
Ack
quoted
+ * - 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 :)
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.
I agree. I've reworked it, and it simplifies the code and makes it
easier to read.
quoted
+ 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
Perhaps expand on "support" here? E.g. "context-switching and trapping
support at EL2".
Ack.
quoted
+ */
+#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.
+ */
I think the complexity of the debug architecture is a good reason to avoid
exposing it here, but I don't understand how providing breakpoints or
watchpoints to a guest could risk leaking guest data. What is the specific
threat here?
I mixed up the various debug and trace features here. Will fix the comment.
quoted
+#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.
s/confiruation/configuration/
However, I don't agree that this provides determinism since we're not
forcing any particular values, but rather filtering the values from the
host.
Ack
quoted
+ * - 40-bit IPA
This seems more about not supporting KVM_CAP_ARM_VM_IPA_SIZE for now.
quoted
+ * - 16-bit ASID
+ * - Mixed-endian
+ * - Distinction between Secure and Non-secure Memory
+ * - Mixed-endian at EL0 only
+ * - Non-context synchronizing exception entry and exit
These all seem to fall into the "cannot trap" category, so we just advertise
whatever we've got.
Why not, and can we actually prevent the guest from doing that?
We cannot prevent the guest from doing it. Initial reasoning was that
there isn't a clear use case for it, but since we cannot prevent the
guest from doing that, I'll unhide it.
quoted
+/*
+ * These features are chosen because they are supported by KVM and to limit the
+ * confiruation state space and make it more deterministic.
It's that typo again ;) But my comment from before still applies -- I don't
think an ALLOW mask adds hugely to the determinism.
Ack
quoted
+ * - Hardware translation table updates to Access flag and Dirty state
+ * - Number of VMID bits from CPU
+ * - Hierarchical Permission Disables
+ * - Privileged Access Never
+ * - SError interrupt exceptions from speculative reads
+ * - Enhanced Translation Synchronization
As before, I think this is a mixture of "trivial" and "cannot trap"
features.
Ack
quoted
+ */
+#define PVM_ID_AA64MMFR1_ALLOW (\
+ FEATURE(ID_AA64MMFR1_HADBS) | \
+ FEATURE(ID_AA64MMFR1_VMIDBITS) | \
+ FEATURE(ID_AA64MMFR1_HPD) | \
+ FEATURE(ID_AA64MMFR1_PAN) | \
+ FEATURE(ID_AA64MMFR1_SPECSEI) | \
+ FEATURE(ID_AA64MMFR1_ETS) \
+ )
+
+/*
+ * These features are chosen because they are supported by KVM and to limit the
+ * confiruation state space and make it more deterministic.
<same comment>
Ack
quoted
+ * - Common not Private translations
+ * - User Access Override
+ * - IESB bit in the SCTLR_ELx registers
+ * - Unaligned single-copy atomicity and atomic functions
+ * - ESR_ELx.EC value on an exception by read access to feature ID space
+ * - TTL field in address operations.
+ * - Break-before-make sequences when changing translation block size
+ * - E0PDx mechanism
+ */
+#define PVM_ID_AA64MMFR2_ALLOW (\
+ FEATURE(ID_AA64MMFR2_CNP) | \
+ FEATURE(ID_AA64MMFR2_UAO) | \
+ FEATURE(ID_AA64MMFR2_IESB) | \
+ FEATURE(ID_AA64MMFR2_AT) | \
+ FEATURE(ID_AA64MMFR2_IDS) | \
+ FEATURE(ID_AA64MMFR2_TTL) | \
+ FEATURE(ID_AA64MMFR2_BBM) | \
+ FEATURE(ID_AA64MMFR2_E0PD) \
+ )
+
+/*
+ * Allow all features in this register because they are trivial to support, or
+ * are already supported by KVM:
+ * - LS64
+ * - XS
+ * - I8MM
+ * - DGB
+ * - BF16
+ * - SPECRES
+ * - SB
+ * - FRINTTS
+ * - PAuth
+ * - FPAC
+ * - LRCPC
+ * - FCMA
+ * - JSCVT
+ * - DPB
+ */
+#define PVM_ID_AA64ISAR1_ALLOW (~0ULL)
+
+#endif /* __ARM64_KVM_FIXED_CONFIG_H__ */
Careful here: this is an unsigned comparison, yet some fields are signed.
cpufeature.c uses the S_ARM64_FTR_BITS and ARM64_FTR_BITS to declare signed
and unsigned fields respectively.
I completely missed that! It's described in "D13.1.3 Principles of the
ID scheme for fields in ID registers" or the Arm Architecture
Reference Manual. Fortunately, all of the features I'm working with
are unsigned. However, I will fix it in v4 to ensure that should we
add a signed feature we can clearly see that it needs to be handled
differently.
Thanks!
/fuad
Will
--
To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.