Hi,
Changes since v3 [1]:
- Redid calculating restricted values of feature register fields, ensuring that
the code distinguishes between unsigned and (potentially in the future)
signed fields (Will)
- Refactoring and fixes (Drew, Will)
- More documentation and comments (Oliver, Will)
- Dropped patch "Restrict protected VM capabilities", since it should come with
or after the user ABI series for pKVM (Will)
- Carried Will's acks
Changes since v2 [2]:
- 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 [3]
- Rebase on 5.14-rc2
- Carried Will's acks
Changes since v1 [4]:
- 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) [5].
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 [6].
Cheers,
/fuad
[1] https://lore.kernel.org/kvmarm/20210719160346.609914-1-tabba@google.com/
[2] https://lore.kernel.org/kvmarm/20210615133950.693489-1-tabba@google.com/
[3] https://lore.kernel.org/kvmarm/20210603183347.1695-1-will@kernel.org/
[4] https://lore.kernel.org/kvmarm/20210608141141.997398-1-tabba@google.com/
[5] 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
[6] https://android-kvm.googlesource.com/linux/+/refs/heads/tabba/el2_fixed_feature_v4
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: Keep mdcr_el2's value as set by __init_el2_debug
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
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 | 170 +++++++++
arch/arm64/include/asm/kvm_host.h | 15 +-
arch/arm64/include/asm/kvm_hyp.h | 5 +-
arch/arm64/include/asm/sysreg.h | 17 +-
arch/arm64/kernel/cpufeature.c | 8 +-
arch/arm64/kvm/Makefile | 2 +-
arch/arm64/kvm/arm.c | 12 +
arch/arm64/kvm/debug.c | 2 +-
arch/arm64/kvm/hyp/include/hyp/switch.h | 52 ++-
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 | 87 ++++-
arch/arm64/kvm/hyp/nvhe/sys_regs.c | 432 ++++++++++++++++++++++
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 | 185 +++++++++
arch/arm64/kvm/sys_regs.c | 64 +---
arch/arm64/kvm/sys_regs.h | 31 ++
23 files changed, 1059 insertions(+), 109 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: c500bee1c5b2f1d59b1081ac879d73268ab0ff17
--
2.33.0.rc1.237.g0d66db33f3-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.
Acked-by: Will Deacon <will@kernel.org>
Signed-off-by: Fuad Tabba <redacted>
Link: 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.
Acked-by: Will Deacon <will@kernel.org>
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/include/asm/sysreg.h | 5 +++
arch/arm64/kvm/sys_regs.c | 60 +++++++++------------------------
arch/arm64/kvm/sys_regs.h | 31 +++++++++++++++++
3 files changed, 52 insertions(+), 44 deletions(-)
This breaks an existing trace in debug.c::kvm_arm_setup_mdcr_el2():
trace_kvm_arm_set_dreg32("MDCR_EL2", vcpu->arch.mdcr_el2);
which expects a 32bit value. I guess we could add an equivalent 64bit
version, or silently upgrade the tracepoint to take a 64bit value.
None of them are good solutions, but hey, something has to give...
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
__init_el2_debug configures mdcr_el2 at initialization based on,
among other things, available hardware support. Trap deactivation
doesn't check that, so keep the initial value.
No functional change intended. However, the value of mdcr_el2
might be different after deactivating traps than it was before
this patch.
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/kvm/hyp/nvhe/switch.c | 4 ----
arch/arm64/kvm/hyp/vhe/switch.c | 4 ----
2 files changed, 8 deletions(-)
From: Will Deacon <will@kernel.org> Date: 2021-08-18 13:20:31
On Tue, Aug 17, 2021 at 09:11:26AM +0100, Fuad Tabba wrote:
__init_el2_debug configures mdcr_el2 at initialization based on,
among other things, available hardware support. Trap deactivation
doesn't check that, so keep the initial value.
No functional change intended. However, the value of mdcr_el2
might be different after deactivating traps than it was before
this patch.
I think this sentence is very confusing, so I'd remove it. I also don't
think it's correct, as the EL2 initialisation code only manipulates the
bits which are being masked here.
So with that sentence removed:
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
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 | 13 +++++--------
arch/arm64/kvm/hyp/vhe/switch.c | 14 +++++---------
arch/arm64/kvm/hyp/vhe/sysreg-sr.c | 2 +-
6 files changed, 21 insertions(+), 21 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,13 @@ 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);+vcpu->arch.mdcr_el2_host&=MDCR_EL2_HPMN_MASK|+MDCR_EL2_E2PB_MASK<<MDCR_EL2_E2PB_SHIFT|+MDCR_EL2_TPMS;-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 */
From: Will Deacon <will@kernel.org> Date: 2021-08-18 13:15:47
On Tue, Aug 17, 2021 at 09:11:25AM +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.
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 | 13 +++++--------
arch/arm64/kvm/hyp/vhe/switch.c | 14 +++++---------
arch/arm64/kvm/hyp/vhe/sysreg-sr.c | 2 +-
6 files changed, 21 insertions(+), 21 deletions(-)
From: Marc Zyngier <maz@kernel.org> Date: 2021-08-18 14:44:14
On Tue, 17 Aug 2021 09:11:25 +0100,
Fuad Tabba [off-list ref] wrote:
quoted hunk
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 | 13 +++++--------
arch/arm64/kvm/hyp/vhe/switch.c | 14 +++++---------
arch/arm64/kvm/hyp/vhe/sysreg-sr.c | 2 +-
6 files changed, 21 insertions(+), 21 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;
This probably should then eventually replace the per-CPU copy of
mdcr_el2 that lives in debug.c, shouldn't it?
quoted hunk
+
/* Exception Information */
struct kvm_vcpu_fault_info fault;
FWIW, I found this whole sequence massively confusing, and it is only
when I came to patch #7 that the various pieces did come together.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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.
Acked-by: Will Deacon <will@kernel.org>
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(-)
From: Will Deacon <will@kernel.org> Date: 2021-08-18 13:24:07
On Tue, Aug 17, 2021 at 09:11:28AM +0100, Fuad Tabba wrote:
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(-)
Thanks, looks better now:
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
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.
Acked-by: Will Deacon <will@kernel.org>
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/include/asm/kvm_arm.h | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
From: Marc Zyngier <maz@kernel.org> Date: 2021-08-18 15:17:57
On Tue, 17 Aug 2021 09:11:29 +0100,
Fuad Tabba [off-list ref] wrote:
quoted hunk
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.
Acked-by: Will Deacon <will@kernel.org>
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/include/asm/kvm_arm.h | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 | 33 +++++++++++++++++++
2 files changed, 76 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,39 @@ static void __pmu_switch_to_host(struct kvm_cpu_context *host_ctxt)write_sysreg(pmu->events_host,pmcntenset_el0);}+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.33.0.rc1.237.g0d66db33f3-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Marc Zyngier <maz@kernel.org> Date: 2021-08-18 16:47:55
On Tue, 17 Aug 2021 09:11:30 +0100,
Fuad Tabba [off-list ref] wrote:
quoted hunk
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 | 33 +++++++++++++++++++
2 files changed, 76 insertions(+)
+
+exit_handle_fn kvm_get_nvhe_exit_handler(struct kvm_vcpu *vcpu);
+
+static exit_handle_fn kvm_get_hyp_exit_handler(struct kvm_vcpu *vcpu)
+{
+ return is_nvhe_hyp_code() ? kvm_get_nvhe_exit_handler(vcpu) : NULL;
+}
+
+/*
+ * Allow the hypervisor to handle the exit with an exit handler if it has one.
+ *
+ * Returns true if the hypervisor handled the exit, and control should go back
+ * to the guest, or false if it hasn't.
+ */
+static bool kvm_hyp_handle_exit(struct kvm_vcpu *vcpu)
+{
+ bool is_handled = false;
... which you then implicitly cast as a bool.
+ exit_handle_fn exit_handler = kvm_get_hyp_exit_handler(vcpu);
+
+ if (exit_handler) {
+ /*
+ * There's limited vcpu context here since it's not synced yet.
+ * Ensure that relevant vcpu context that might be used by the
+ * exit_handler is in sync before it's called and if handled.
+ */
+ *vcpu_pc(vcpu) = read_sysreg_el2(SYS_ELR);
+ *vcpu_cpsr(vcpu) = read_sysreg_el2(SYS_SPSR);
+
+ is_handled = exit_handler(vcpu);
What does 'is_handled' mean here? By definition, any trap *must* be
handled, one way or another. By the look of it, what you really mean
is something like "I have updated the vcpu state and you'd better
reload it". Is that what it means?
All these functions really should be marked inline. Have you checked
how this expands on VHE? I think some compilers could be pretty
unhappy about the undefined symbol in kvm_get_hyp_exit_handler().
It is also unfortunate that we get a bunch of tests for various
flavours of traps (FP, PAuth, page faults...), only to hit yet another
decoding tree. Is there a way we could use this infrastructure for
everything?
quoted hunk
+
/*
* Return true when we were able to fixup the guest exit and should return to
* the guest, false when we should restore the host state and return to the
@@ -496,6 +536,9 @@ static inline bool fixup_guest_exit(struct kvm_vcpu *vcpu, u64 *exit_code) goto guest; }+ /* Check if there's an exit handler and allow it to handle the exit. */+ if (kvm_hyp_handle_exit(vcpu))+ goto guest; exit: /* Return to the host kernel and handle the exit */ return false;
You can safely drop all these and only keep the top one for now. This
will also keep the idiot robot at bay for until the next patch... ;-)
+};
+
+exit_handle_fn kvm_get_nvhe_exit_handler(struct kvm_vcpu *vcpu)
+{
+ u32 esr = kvm_vcpu_get_esr(vcpu);
+ u8 esr_ec = ESR_ELx_EC(esr);
+
+ return hyp_exit_handlers[esr_ec];
+}
+
/* Switch to the guest for legacy non-VHE systems */
int __kvm_vcpu_run(struct kvm_vcpu *vcpu)
{
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Marc Zyngier <maz@kernel.org> Date: 2021-08-19 14:37:52
Hi Fuad,
On Wed, 18 Aug 2021 17:45:50 +0100,
Marc Zyngier [off-list ref] wrote:
On Tue, 17 Aug 2021 09:11:30 +0100,
Fuad Tabba [off-list ref] wrote:
quoted
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 | 33 +++++++++++++++++++
2 files changed, 76 insertions(+)
+
+exit_handle_fn kvm_get_nvhe_exit_handler(struct kvm_vcpu *vcpu);
+
+static exit_handle_fn kvm_get_hyp_exit_handler(struct kvm_vcpu *vcpu)
+{
+ return is_nvhe_hyp_code() ? kvm_get_nvhe_exit_handler(vcpu) : NULL;
+}
+
+/*
+ * Allow the hypervisor to handle the exit with an exit handler if it has one.
+ *
+ * Returns true if the hypervisor handled the exit, and control should go back
+ * to the guest, or false if it hasn't.
+ */
+static bool kvm_hyp_handle_exit(struct kvm_vcpu *vcpu)
+{
+ bool is_handled = false;
... which you then implicitly cast as a bool.
quoted
+ exit_handle_fn exit_handler = kvm_get_hyp_exit_handler(vcpu);
+
+ if (exit_handler) {
+ /*
+ * There's limited vcpu context here since it's not synced yet.
+ * Ensure that relevant vcpu context that might be used by the
+ * exit_handler is in sync before it's called and if handled.
+ */
+ *vcpu_pc(vcpu) = read_sysreg_el2(SYS_ELR);
+ *vcpu_cpsr(vcpu) = read_sysreg_el2(SYS_SPSR);
+
+ is_handled = exit_handler(vcpu);
What does 'is_handled' mean here? By definition, any trap *must* be
handled, one way or another. By the look of it, what you really mean
is something like "I have updated the vcpu state and you'd better
reload it". Is that what it means?
All these functions really should be marked inline. Have you checked
how this expands on VHE? I think some compilers could be pretty
unhappy about the undefined symbol in kvm_get_hyp_exit_handler().
It is also unfortunate that we get a bunch of tests for various
flavours of traps (FP, PAuth, page faults...), only to hit yet another
decoding tree. Is there a way we could use this infrastructure for
everything?
I realised that I wasn't very forthcoming here. I've decided to put
the code where my mouth is and pushed out a branch [1] with your first
10 patches, followed by my own take on this particular problem. It
compiles, and even managed to boot a Debian guest on a nVHE box.
As you can see, most of the early exit handling is now moved to
specific handlers, unifying the handling. For the protected mode, you
can provide your own handler array (just hack
kvm_get_exit_handler_array() to return something else), which will do
the right thing as long as you call into the existing handlers first.
When it comes to the ELR/SPSR handling, it is better left to the
individual handlers (which we already do in some cases, see how we
skip instructions, for example).
Please let me know what you think.
Thanks,
M.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/pkvm-fixed-features
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Marc,
On Thu, Aug 19, 2021 at 3:36 PM Marc Zyngier [off-list ref] wrote:
Hi Fuad,
On Wed, 18 Aug 2021 17:45:50 +0100,
Marc Zyngier [off-list ref] wrote:
quoted
On Tue, 17 Aug 2021 09:11:30 +0100,
Fuad Tabba [off-list ref] wrote:
quoted
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 | 33 +++++++++++++++++++
2 files changed, 76 insertions(+)
+
+exit_handle_fn kvm_get_nvhe_exit_handler(struct kvm_vcpu *vcpu);
+
+static exit_handle_fn kvm_get_hyp_exit_handler(struct kvm_vcpu *vcpu)
+{
+ return is_nvhe_hyp_code() ? kvm_get_nvhe_exit_handler(vcpu) : NULL;
+}
+
+/*
+ * Allow the hypervisor to handle the exit with an exit handler if it has one.
+ *
+ * Returns true if the hypervisor handled the exit, and control should go back
+ * to the guest, or false if it hasn't.
+ */
+static bool kvm_hyp_handle_exit(struct kvm_vcpu *vcpu)
+{
+ bool is_handled = false;
... which you then implicitly cast as a bool.
quoted
+ exit_handle_fn exit_handler = kvm_get_hyp_exit_handler(vcpu);
+
+ if (exit_handler) {
+ /*
+ * There's limited vcpu context here since it's not synced yet.
+ * Ensure that relevant vcpu context that might be used by the
+ * exit_handler is in sync before it's called and if handled.
+ */
+ *vcpu_pc(vcpu) = read_sysreg_el2(SYS_ELR);
+ *vcpu_cpsr(vcpu) = read_sysreg_el2(SYS_SPSR);
+
+ is_handled = exit_handler(vcpu);
What does 'is_handled' mean here? By definition, any trap *must* be
handled, one way or another. By the look of it, what you really mean
is something like "I have updated the vcpu state and you'd better
reload it". Is that what it means?
All these functions really should be marked inline. Have you checked
how this expands on VHE? I think some compilers could be pretty
unhappy about the undefined symbol in kvm_get_hyp_exit_handler().
It is also unfortunate that we get a bunch of tests for various
flavours of traps (FP, PAuth, page faults...), only to hit yet another
decoding tree. Is there a way we could use this infrastructure for
everything?
I realised that I wasn't very forthcoming here. I've decided to put
the code where my mouth is and pushed out a branch [1] with your first
10 patches, followed by my own take on this particular problem. It
compiles, and even managed to boot a Debian guest on a nVHE box.
As you can see, most of the early exit handling is now moved to
specific handlers, unifying the handling. For the protected mode, you
can provide your own handler array (just hack
kvm_get_exit_handler_array() to return something else), which will do
the right thing as long as you call into the existing handlers first.
When it comes to the ELR/SPSR handling, it is better left to the
individual handlers (which we already do in some cases, see how we
skip instructions, for example).
Please let me know what you think.
Thanks a lot for this and sorry for being late to reply. I've been travelling.
I think that your proposal looks great. All handling is consolidated
now and handling for protected VMs can just be added on top. There are
some small issues with what parameters we need (e.g., passing struct
kvm to kvm_get_exit_handler_array), but I will sort them out and
submit them in the next round.
Cheers,
/fuad
From: Marc Zyngier <maz@kernel.org> Date: 2021-08-23 12:12:50
Hi Fuad,
On Mon, 23 Aug 2021 11:21:05 +0100,
Fuad Tabba [off-list ref] wrote:
Hi Marc,
On Thu, Aug 19, 2021 at 3:36 PM Marc Zyngier [off-list ref] wrote:
quoted
I realised that I wasn't very forthcoming here. I've decided to put
the code where my mouth is and pushed out a branch [1] with your first
10 patches, followed by my own take on this particular problem. It
compiles, and even managed to boot a Debian guest on a nVHE box.
As you can see, most of the early exit handling is now moved to
specific handlers, unifying the handling. For the protected mode, you
can provide your own handler array (just hack
kvm_get_exit_handler_array() to return something else), which will do
the right thing as long as you call into the existing handlers first.
When it comes to the ELR/SPSR handling, it is better left to the
individual handlers (which we already do in some cases, see how we
skip instructions, for example).
Please let me know what you think.
Thanks a lot for this and sorry for being late to reply. I've been
travelling.
No worries, it should be me who apologies for getting to this that late.
I think that your proposal looks great. All handling is consolidated
now and handling for protected VMs can just be added on top. There are
some small issues with what parameters we need (e.g., passing struct
kvm to kvm_get_exit_handler_array), but I will sort them out and
submit them in the next round.
OK. Please base these changes on top of the three patches in my
branch, which I will update with actual commit messages.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 | 170 +++++++++
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 | 430 ++++++++++++++++++++++
arch/arm64/kvm/pkvm.c | 185 ++++++++++
8 files changed, 803 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,430 @@+// 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.+*/++/*+*Returnstherestrictedfeaturesvaluesofthefeatureregisterbasedonthe+*limitationsinrestrict_fields.+*Note:Useonlyforunsignedfeaturefieldvalues.+*/+staticu64get_restricted_features_unsigned(u64sys_reg_val,+u64restrict_fields)+{+u64value=0UL;+u64mask=GENMASK_ULL(ARM64_FEATURE_FIELD_BITS-1,0);++/*+*AccordingtotheArmArchitectureReferenceManual,featurefields+*useincreasingvaluestoindicateincreasesinfunctionality.+*Iterateovertherestrictedfeaturefieldsandcalculatetheminimum+*unsignedvaluebetweentheonesupportedbythesystem,andwhatthe+*valueisbeingrestrictedto.+*/+while(sys_reg_val&&restrict_fields){+value|=min(sys_reg_val&mask,restrict_fields&mask);+sys_reg_val&=~mask;+restrict_fields&=~mask;+mask<<=ARM64_FEATURE_FIELD_BITS;+}++returnvalue;+}++/* 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);+u64set_mask=0;++if(p->is_write)+returnundef_access(vcpu,p,r);++set_mask|=get_restricted_features_unsigned(id_aa64pfr0_el1_sys_val,+PVM_ID_AA64PFR0_RESTRICT_UNSIGNED);++/* Spectre and Meltdown mitigation in KVM */+set_mask|=FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2),+(u64)kvm->arch.pfr0_csv2);+set_mask|=FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3),+(u64)kvm->arch.pfr0_csv3);++p->regval=(id_aa64pfr0_el1_sys_val&PVM_ID_AA64PFR0_ALLOW)|+set_mask;+returntrue;+}++/* Accessor for ID_AA64PFR1_EL1. */+staticboolpvm_access_id_aa64pfr1(structkvm_vcpu*vcpu,+structsys_reg_params*p,+conststructsys_reg_desc*r)+{+if(p->is_write)+returnundef_access(vcpu,p,r);++p->regval=id_aa64pfr1_el1_sys_val&PVM_ID_AA64PFR1_ALLOW;+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+*sanitizedcopyofthefeatureidregisteranditishandledbythehost.+*/+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)+{+u64set_mask=0;++if(p->is_write)+returnundef_access(vcpu,p,r);++set_mask|=get_restricted_features_unsigned(id_aa64mmfr0_el1_sys_val,+PVM_ID_AA64MMFR0_RESTRICT_UNSIGNED);++p->regval=(id_aa64mmfr0_el1_sys_val&PVM_ID_AA64MMFR0_ALLOW)|+set_mask;+returntrue;+}++/* Accessor for ID_AA64MMFR1_EL1. */+staticboolpvm_access_id_aa64mmfr1(structkvm_vcpu*vcpu,+structsys_reg_params*p,+conststructsys_reg_desc*r)+{+if(p->is_write)+returnundef_access(vcpu,p,r);++p->regval=id_aa64mmfr1_el1_sys_val&PVM_ID_AA64MMFR1_ALLOW;+returntrue;+}++/* Accessor for ID_AA64MMFR2_EL1. */+staticboolpvm_access_id_aa64mmfr2(structkvm_vcpu*vcpu,+structsys_reg_params*p,+conststructsys_reg_desc*r)+{+if(p->is_write)+returnundef_access(vcpu,p,r);++p->regval=id_aa64mmfr2_el1_sys_val&PVM_ID_AA64MMFR2_ALLOW;+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(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1),+PVM_ID_AA64PFR0_RESTRICT_UNSIGNED)>+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;+}
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.
Acked-by: Will Deacon <will@kernel.org>
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.
Acked-by: Will Deacon <will@kernel.org>
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/kvm/hyp/include/hyp/switch.h | 3 +++
arch/arm64/kvm/hyp/nvhe/switch.c | 34 ++++++++++++++-----------
2 files changed, 22 insertions(+), 15 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){
@@ -188,7 +188,11 @@ exit_handle_fn kvm_get_nvhe_exit_handler(struct kvm_vcpu *vcpu)u32esr=kvm_vcpu_get_esr(vcpu);u8esr_ec=ESR_ELx_EC(esr);-returnhyp_exit_handlers[esr_ec];+/* For now, only protected VMs have exit handlers. */+if(unlikely(kvm_vm_is_protected(kern_hyp_va(vcpu->kvm))))+returnhyp_exit_handlers[esr_ec];+else+returnNULL;}/* Switch to the guest for legacy non-VHE systems */
--
2.33.0.rc1.237.g0d66db33f3-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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.
If this were to happen, The VMM can try and fix it by re-
initializing the vcpu with KVM_ARM_VCPU_INIT, however, this is
likely not possible for protected VMs.
Adapted from commit 22f553842b14 ("KVM: arm64: Handle Asymmetric
AArch32 systems")
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/kvm/hyp/nvhe/switch.c | 37 ++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
@@ -195,6 +196,39 @@ exit_handle_fn kvm_get_nvhe_exit_handler(struct kvm_vcpu *vcpu)returnNULL;}+/*+*Someguests(e.g.,protectedVMs)mightnotbeallowedtoruninAArch32.The+*checkbelowisbasedontheoneinkvm_arch_vcpu_ioctl_run().+*TheARMv8architecturedoesnotgivethehypervisoramechanismtopreventa+*guestfromdroppingtoAArch32EL0ifimplementedbytheCPU.Ifthe+*hypervisorspotsaguestinsuchastateensureitishandled,anddon't+*trustthehosttospotorfixit.+*+*Returnstrueifthecheckpassedandtheguestrunloopcancontinue,or+*falseiftheguestshouldexittothehost.+*/+staticboolcheck_aarch32_guest(structkvm_vcpu*vcpu,u64*exit_code)+{+if(kvm_vm_is_protected(kern_hyp_va(vcpu->kvm))&&+vcpu_mode_is_32bit(vcpu)&&+FIELD_GET(ARM64_FEATURE_MASK(ID_AA64PFR0_EL0),+PVM_ID_AA64PFR0_RESTRICT_UNSIGNED)<+ID_AA64PFR0_ELx_32BIT_64BIT){+/*+*Aswehavecaughttheguestred-handed,decidethatitisn't+*fitforpurposeanymorebymakingthevcpuinvalid.TheVMM+*cantryandfixitbyre-initializingthevcpuwith+*KVM_ARM_VCPU_INIT,however,thisislikelynotpossiblefor+*protectedVMs.+*/+vcpu->arch.target=-1;+*exit_code=ARM_EXCEPTION_IL;+returnfalse;+}++returntrue;+}+/* Switch to the guest for legacy non-VHE systems */int__kvm_vcpu_run(structkvm_vcpu*vcpu){
@@ -255,6 +289,9 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu)/* Jump in the fire! */exit_code=__guest_enter(vcpu);+if(unlikely(!check_aarch32_guest(vcpu,&exit_code)))+break;+/* And we're baaack! */}while(fixup_guest_exit(vcpu,&exit_code));
--
2.33.0.rc1.237.g0d66db33f3-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Oliver Upton <hidden> Date: 2021-08-19 08:12:26
Hi Fuad,
On Tue, Aug 17, 2021 at 1:12 AM Fuad Tabba [off-list ref] 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.
If this were to happen, The VMM can try and fix it by re-
initializing the vcpu with KVM_ARM_VCPU_INIT, however, this is
likely not possible for protected VMs.
Adapted from commit 22f553842b14 ("KVM: arm64: Handle Asymmetric
AArch32 systems")
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/kvm/hyp/nvhe/switch.c | 37 ++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
It may be more readable to initialize a local variable with this
feature check, i.e:
bool aarch32_allowed = FIELD_GET(...) == ID_AA64PFR0_ELx_32BIT_64BIT;
and then:
if (kvm_vm_is_protected(kvm) && vcpu_mode_is_32bit(vcpu) &&
!aarch32_allowed) {
quoted hunk
+ /*
+ * As we have caught the guest red-handed, decide that it isn't
+ * fit for purpose anymore by making the vcpu invalid. The VMM
+ * can try and fix it by re-initializing the vcpu with
+ * KVM_ARM_VCPU_INIT, however, this is likely not possible for
+ * protected VMs.
+ */
+ vcpu->arch.target = -1;
+ *exit_code = ARM_EXCEPTION_IL;
+ return false;
+ }
+
+ return true;
+}
+
/* Switch to the guest for legacy non-VHE systems */
int __kvm_vcpu_run(struct kvm_vcpu *vcpu)
{
@@ -255,6 +289,9 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu) /* Jump in the fire! */ exit_code = __guest_enter(vcpu);+ if (unlikely(!check_aarch32_guest(vcpu, &exit_code)))+ break;+ /* And we're baaack! */ } while (fixup_guest_exit(vcpu, &exit_code));--
Hi Oliver,
On Thu, Aug 19, 2021 at 9:10 AM Oliver Upton [off-list ref] wrote:
Hi Fuad,
On Tue, Aug 17, 2021 at 1:12 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.
If this were to happen, The VMM can try and fix it by re-
initializing the vcpu with KVM_ARM_VCPU_INIT, however, this is
likely not possible for protected VMs.
Adapted from commit 22f553842b14 ("KVM: arm64: Handle Asymmetric
AArch32 systems")
Signed-off-by: Fuad Tabba <redacted>
---
arch/arm64/kvm/hyp/nvhe/switch.c | 37 ++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
It may be more readable to initialize a local variable with this
feature check, i.e:
bool aarch32_allowed = FIELD_GET(...) == ID_AA64PFR0_ELx_32BIT_64BIT;
and then:
if (kvm_vm_is_protected(kvm) && vcpu_mode_is_32bit(vcpu) &&
!aarch32_allowed) {
I agree.
Thanks,
/fuad
quoted
+ /*
+ * As we have caught the guest red-handed, decide that it isn't
+ * fit for purpose anymore by making the vcpu invalid. The VMM
+ * can try and fix it by re-initializing the vcpu with
+ * KVM_ARM_VCPU_INIT, however, this is likely not possible for
+ * protected VMs.
+ */
+ vcpu->arch.target = -1;
+ *exit_code = ARM_EXCEPTION_IL;
+ return false;
+ }
+
+ return true;
+}
+
/* Switch to the guest for legacy non-VHE systems */
int __kvm_vcpu_run(struct kvm_vcpu *vcpu)
{
@@ -255,6 +289,9 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu) /* Jump in the fire! */ exit_code = __guest_enter(vcpu);+ if (unlikely(!check_aarch32_guest(vcpu, &exit_code)))+ break;+ /* And we're baaack! */ } while (fixup_guest_exit(vcpu, &exit_code));--
From: Marc Zyngier <maz@kernel.org> Date: 2021-08-20 10:36:46
On Tue, 17 Aug 2021 09:11:19 +0100, Fuad Tabba wrote:
Changes since v3 [1]:
- Redid calculating restricted values of feature register fields, ensuring that
the code distinguishes between unsigned and (potentially in the future)
signed fields (Will)
- Refactoring and fixes (Drew, Will)
- More documentation and comments (Oliver, Will)
- Dropped patch "Restrict protected VM capabilities", since it should come with
or after the user ABI series for pKVM (Will)
- Carried Will's acks
[...]
I've taken the first 10 patches of this series in order to
progress it. I also stashed a fixlet on top to address the
tracepoint issue.
Hopefully we can resolve the rest of the issues quickly.
[01/15] KVM: arm64: placeholder to check if VM is protected
commit: 2ea7f655800b00b109951f22539fe2025add210b
[02/15] KVM: arm64: Remove trailing whitespace in comment
commit: e6bc555c96990046d680ff92c8e2e7b6b43b509f
[03/15] KVM: arm64: MDCR_EL2 is a 64-bit register
commit: d6c850dd6ce9ce4b410142a600d8c34dc041d860
[04/15] KVM: arm64: Fix names of config register fields
commit: dabb1667d8573302712a75530cccfee8f3ffff84
[05/15] KVM: arm64: Refactor sys_regs.h,c for nVHE reuse
commit: f76f89e2f73d93720cfcad7fb7b24d022b2846bf
[06/15] KVM: arm64: Restore mdcr_el2 from vcpu
commit: 1460b4b25fde52cbee746c11a4b1d3185f2e2847
[07/15] KVM: arm64: Keep mdcr_el2's value as set by __init_el2_debug
commit: 12849badc6d2456f15f8f2c93037628d5176810b
[08/15] KVM: arm64: Track value of cptr_el2 in struct kvm_vcpu_arch
commit: cd496228fd8de2e82b6636d3d89105631ea2b69c
[09/15] KVM: arm64: Add feature register flag definitions
commit: 95b54c3e4c92b9185b15c83e8baab9ba312195f6
[10/15] KVM: arm64: Add config register bit definitions
commit: 2d701243b9f231b5d7f9a8cb81870650d3eb32bc
Cheers,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Marc,
On Fri, Aug 20, 2021 at 11:34 AM Marc Zyngier [off-list ref] wrote:
On Tue, 17 Aug 2021 09:11:19 +0100, Fuad Tabba wrote:
quoted
Changes since v3 [1]:
- Redid calculating restricted values of feature register fields, ensuring that
the code distinguishes between unsigned and (potentially in the future)
signed fields (Will)
- Refactoring and fixes (Drew, Will)
- More documentation and comments (Oliver, Will)
- Dropped patch "Restrict protected VM capabilities", since it should come with
or after the user ABI series for pKVM (Will)
- Carried Will's acks
[...]
I've taken the first 10 patches of this series in order to
progress it. I also stashed a fixlet on top to address the
tracepoint issue.
Hopefully we can resolve the rest of the issues quickly.
Thanks. I am working on a patch series with the remaining patches to
address the issues. Stay tuned :)
Cheers,
/fuad
[01/15] KVM: arm64: placeholder to check if VM is protected
commit: 2ea7f655800b00b109951f22539fe2025add210b
[02/15] KVM: arm64: Remove trailing whitespace in comment
commit: e6bc555c96990046d680ff92c8e2e7b6b43b509f
[03/15] KVM: arm64: MDCR_EL2 is a 64-bit register
commit: d6c850dd6ce9ce4b410142a600d8c34dc041d860
[04/15] KVM: arm64: Fix names of config register fields
commit: dabb1667d8573302712a75530cccfee8f3ffff84
[05/15] KVM: arm64: Refactor sys_regs.h,c for nVHE reuse
commit: f76f89e2f73d93720cfcad7fb7b24d022b2846bf
[06/15] KVM: arm64: Restore mdcr_el2 from vcpu
commit: 1460b4b25fde52cbee746c11a4b1d3185f2e2847
[07/15] KVM: arm64: Keep mdcr_el2's value as set by __init_el2_debug
commit: 12849badc6d2456f15f8f2c93037628d5176810b
[08/15] KVM: arm64: Track value of cptr_el2 in struct kvm_vcpu_arch
commit: cd496228fd8de2e82b6636d3d89105631ea2b69c
[09/15] KVM: arm64: Add feature register flag definitions
commit: 95b54c3e4c92b9185b15c83e8baab9ba312195f6
[10/15] KVM: arm64: Add config register bit definitions
commit: 2d701243b9f231b5d7f9a8cb81870650d3eb32bc
Cheers,
M.
--
Without deviation from the norm, progress is not possible.
On Fri, Aug 20, 2021 at 11:34 AM Marc Zyngier [off-list ref] wrote:
On Tue, 17 Aug 2021 09:11:19 +0100, Fuad Tabba wrote:
quoted
Changes since v3 [1]:
- Redid calculating restricted values of feature register fields, ensuring that
the code distinguishes between unsigned and (potentially in the future)
signed fields (Will)
- Refactoring and fixes (Drew, Will)
- More documentation and comments (Oliver, Will)
- Dropped patch "Restrict protected VM capabilities", since it should come with
or after the user ABI series for pKVM (Will)
- Carried Will's acks
[...]
I've taken the first 10 patches of this series in order to
progress it. I also stashed a fixlet on top to address the
tracepoint issue.
Hopefully we can resolve the rest of the issues quickly.
[01/15] KVM: arm64: placeholder to check if VM is protected
commit: 2ea7f655800b00b109951f22539fe2025add210b
[02/15] KVM: arm64: Remove trailing whitespace in comment
commit: e6bc555c96990046d680ff92c8e2e7b6b43b509f
[03/15] KVM: arm64: MDCR_EL2 is a 64-bit register
commit: d6c850dd6ce9ce4b410142a600d8c34dc041d860
[04/15] KVM: arm64: Fix names of config register fields
commit: dabb1667d8573302712a75530cccfee8f3ffff84
[05/15] KVM: arm64: Refactor sys_regs.h,c for nVHE reuse
commit: f76f89e2f73d93720cfcad7fb7b24d022b2846bf
[06/15] KVM: arm64: Restore mdcr_el2 from vcpu
commit: 1460b4b25fde52cbee746c11a4b1d3185f2e2847
[07/15] KVM: arm64: Keep mdcr_el2's value as set by __init_el2_debug
commit: 12849badc6d2456f15f8f2c93037628d5176810b
[08/15] KVM: arm64: Track value of cptr_el2 in struct kvm_vcpu_arch
commit: cd496228fd8de2e82b6636d3d89105631ea2b69c
[09/15] KVM: arm64: Add feature register flag definitions
commit: 95b54c3e4c92b9185b15c83e8baab9ba312195f6
[10/15] KVM: arm64: Add config register bit definitions
commit: 2d701243b9f231b5d7f9a8cb81870650d3eb32bc
Cheers,
M.
--
Without deviation from the norm, progress is not possible.