KVM: arm64: Provide guest support for GCS
Subsystems (5)
v21 cover KVM: arm64: Provide guest support for GCS
The arm64 Guarded Control Stack (GCS) feature provides support for hardware protected stacks of return addresses, intended to provide hardening against return oriented programming (ROP) attacks and to make it easier to gather call stacks for applications such as profiling.
When GCS is active a secondary stack called the Guarded Control Stack is maintained, protected with a memory attribute which means that it can only be written with specific GCS operations. The current GCS pointer can not be directly written to by userspace. When a BL is executed the value stored in LR is also pushed onto the GCS, and when a RET is executed the top of the GCS is popped and compared to LR with a fault being raised if the values do not match. GCS operations may only be performed on GCS pages, a data abort is generated if they are not.
The combination of hardware enforcement and lack of extra instructions in the function entry and exit paths should result in something which has less overhead and is more difficult to attack than a purely software implementation like clang's shadow stacks.
This series implements support for managing GCS for KVM guests.
---
Changes in v21:
- Drop "KVM: arm64: Fix FGT mapping for HFGITR_EL2.nGCSEPP" which was applied as 089e4f3c4862.
- Reject creation of guests with GCS without S1PIE, or S1PIE/S1POE
without TCR2. Add selftest coverage for this.
- Flip order of GCSPR_EL2 and GCSCR_EL2 in el2_reg in get-reg-list.
- Remove ineffective selftest cases with HFGITR_EL2.ERET set.
- Formatting and clarity tweaks.
- Link to v20: https://patch.msgid.link/[email protected]
Changes in v20:
- Rebase onto v7.3-rc1.
- Relax GCSB DSYNCs, remove them when loading the vCPU and move them to vcpu_put() on the outgoing path.
- Make kvm_check_illegal_exlock_returns() mode change checks more
similar to compute_exlock().
- Use kvm_has_gcs() in ctxt_has_gcs().
- Move some EL2 register saves under S1PIE checks.
- Link to v19: https://patch.msgid.link/[email protected]
Changes in v19:
- Fix accessor for GCSCR_EL2 in emulation.
- Correct locate_direct_register() for EL0 registers.
- Don't check for GCS without S1PIE when restoring vEL2.
- Move barriers to align better with nearby dsbs.
- Link to v18: https://patch.msgid.link/[email protected]
Changes in v18:
- Reinject all GCS exceptions from nested guests.
- Include GCSCRE0_EL1 and GCSPR_EL0 in the live CPU register accessors.
- Sort el2_regs in get-reg-list.
- Link to v17: https://patch.msgid.link/[email protected]
Changes in v17:
- Rebase onto v7.2-rc4.
- Fix mapping of HFGITR_EL2.nGCSEPP.
- Implement a test for NV emulation around EXLOCK.
- Fix NV exception emulation issues.
- Add GCS[PC]R_EL1 to locate_direct_register().
- Add barriers.
- Add ID_AA64PFR1_EL1.GCS to set_id_regs.
- Comment updates.
- Link to v16: https://lore.kernel.org/r/[email protected]
Changes in v16:
- Rebase onto v6.17-rc3.
- Also expose the feature to nested guests.
- Implement emulation of EXLOCK when returning from nested guests.
- Rename enter_exception_gcs() to compute_exlock().
- Move all ID_AA64PFR1_EL1 handling to the final kernel patch.
- Drop unneeded forwarding of GCS exceptions.
- Commit and cover message updates.
- Link to v15: https://lore.kernel.org/r/[email protected]
Changes in v15:
- Rebase onto v6.17-rc1.
- Link to v14: https://lore.kernel.org/r/[email protected]
Changes in v14:
- Rebase onto arm64/for-next/gcs which includes all the non-KVM support.
- Manage the fine grained traps for GCS instructions.
- Manage PSTATE.EXLOCK when delivering exceptions to KVM guests.
- Link to v13: https://lore.kernel.org/r/[email protected]
Changes in v13:
- Rebase onto v6.12-rc1.
- Allocate VM_HIGH_ARCH_6 since protection keys used all the existing
bits.
- Implement mm_release() and free transparently allocated GCSs there.
- Use bit 32 of AT_HWCAP for GCS due to AT_HWCAP2 being filled.
- Since we now only set GCSCRE0_EL1 on change ensure that it is
initialised with GCSPR_EL0 accessible to EL0.
- Fix OOM handling on thread copy.
- Link to v12: https://lore.kernel.org/r/[email protected]
Changes in v12:
- Clarify and simplify the signal handling code so we work with the register state.
- When checking for write aborts to shadow stack pages ensure the fault
is a data abort.
- Depend on !UPROBES.
- Comment cleanups.
- Link to v11: https://lore.kernel.org/r/[email protected]
Changes in v11:
- Remove the dependency on the addition of clone3() support for shadow stacks, rebasing onto v6.11-rc3.
- Make ID_AA64PFR1_EL1.GCS writeable in KVM.
- Hide GCS registers when GCS is not enabled for KVM guests.
- Require HCRX_EL2.GCSEn if booting at EL1.
- Require that GCSCR_EL1 and GCSCRE0_EL1 be initialised regardless of
if we boot at EL2 or EL1.
- Remove some stray use of bit 63 in signal cap tokens.
- Warn if we see a GCS with VM_SHARED.
- Remove rdundant check for VM_WRITE in fault handling.
- Cleanups and clarifications in the ABI document.
- Clean up and improve documentation of some sync placement.
- Only set the EL0 GCS mode if it's actually changed.
- Various minor fixes and tweaks.
- Link to v10: https://lore.kernel.org/r/[email protected]
Changes in v10:
- Fix issues with THP.
- Tighten up requirements for initialising GCSCR*.
- Only generate GCS signal frames for threads using GCS.
- Only context switch EL1 GCS registers if S1PIE is enabled.
- Move context switch of GCSCRE0_EL1 to EL0 context switch.
- Make GCS registers unconditionally visible to userspace.
- Use FHU infrastructure.
- Don't change writability of ID_AA64PFR1_EL1 for KVM.
- Remove unused arguments from alloc_gcs().
- Typo fixes.
- Link to v9: https://lore.kernel.org/r/[email protected]
Changes in v9:
- Rebase onto v6.10-rc3.
- Restructure and clarify memory management fault handling.
- Fix up basic-gcs for the latest clone3() changes.
- Convert to newly merged KVM ID register based feature configuration.
- Fixes for NV traps.
- Link to v8: https://lore.kernel.org/r/[email protected]
Changes in v8:
- Invalidate signal cap token on stack when consuming.
- Typo and other trivial fixes.
- Don't try to use process_vm_write() on GCS, it intentionally does not work.
- Fix leak of thread GCSs.
- Rebase onto latest clone3() series.
- Link to v7: https://lore.kernel.org/r/[email protected]
Changes in v7:
- Rebase onto v6.7-rc2 via the clone3() patch series.
- Change the token used to cap the stack during signal handling to be compatible with GCSPOPM.
- Fix flags for new page types.
- Fold in support for clone3().
- Replace copy_to_user_gcs() with put_user_gcs().
- Link to v6: https://lore.kernel.org/r/[email protected]
Changes in v6:
- Rebase onto v6.6-rc3.
- Add some more gcsb_dsync() barriers following spec clarifications.
- Due to ongoing discussion around clone()/clone3() I've not updated anything there, the behaviour is the same as on previous versions.
- Link to v5: https://lore.kernel.org/r/[email protected]
Changes in v5:
- Don't map any permissions for user GCSs, we always use EL0 accessors
or use a separate mapping of the page.
- Reduce the standard size of the GCS to RLIMIT_STACK/2.
- Enforce a PAGE_SIZE alignment requirement on map_shadow_stack().
- Clarifications and fixes to documentation.
- More tests.
- Link to v4: https://lore.kernel.org/r/[email protected]
Changes in v4:
- Implement flags for map_shadow_stack() allowing the cap and end of stack marker to be enabled independently or not at all.
- Relax size and alignment requirements for map_shadow_stack().
- Add more blurb explaining the advantages of hardware enforcement.
- Link to v3: https://lore.kernel.org/r/[email protected]
Changes in v3:
- Rebase onto v6.5-rc4.
- Add a GCS barrier on context switch.
- Add a GCS stress test.
- Link to v2: https://lore.kernel.org/r/[email protected]
Changes in v2:
- Rebase onto v6.5-rc3.
- Rework prctl() interface to allow each bit to be locked independently.
- map_shadow_stack() now places the cap token based on the size
requested by the caller not the actual space allocated.
- Mode changes other than enable via ptrace are now supported.
- Expand test coverage.
- Various smaller fixes and adjustments.
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Mark Brown (15):
arm64/gcs: Ensure FGTs for EL1 GCS instructions are disabled
KVM: arm64: Refuse to start a guest with S1PIE or S1POE but not TCR2
KVM: arm64: Manage GCS access and registers for guests
KVM: arm64: Ensure GCS memory effects are visible
KVM: arm64: Set PSTATE.EXLOCK when entering an exception
KVM: arm64: Validate GCS exception lock when emulating ERET
KVM: arm64: Forward GCS exceptions to nested guests
KVM: arm64: Enforce EXLOCK for SPSR and ELR
KVM: arm64: Allow GCS to be enabled for guests
KVM: selftests: arm64: Check that invalid feature combinations are rejected
KVM: selftests: arm64: Add GCS registers to get-reg-list
KVM: selftests: arm64: Add GCS to set_id_regs
KVM: selftests: arm64: Only restore SPSR_EL1 and ELR_EL1 if they change
tools: Synchronise the kernel esr.h
KVM: selftests: arm64: Add GCS EXLOCK exception emulation test
arch/arm64/include/asm/el2_setup.h | 5 + arch/arm64/include/asm/kvm_emulate.h | 11 + arch/arm64/include/asm/kvm_host.h | 14 + arch/arm64/include/asm/kvm_nested.h | 32 ++ arch/arm64/include/asm/vncr_mapping.h | 2 + arch/arm64/include/uapi/asm/ptrace.h | 1 + arch/arm64/kvm/arm.c | 7 + arch/arm64/kvm/emulate-nested.c | 5 +- arch/arm64/kvm/handle_exit.c | 18 +- arch/arm64/kvm/hyp/exception.c | 37 +++ arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h | 27 ++ arch/arm64/kvm/hyp/nvhe/hyp-main.c | 7 + arch/arm64/kvm/hyp/vhe/switch.c | 4 + arch/arm64/kvm/hyp/vhe/sysreg-sr.c | 10 + arch/arm64/kvm/nested.c | 3 +- arch/arm64/kvm/sys_regs.c | 113 ++++++- tools/arch/arm64/include/asm/esr.h | 113 ++++++- tools/testing/selftests/kvm/Makefile.kvm | 1 + tools/testing/selftests/kvm/arm64/gcs_exceptions.c | 336 +++++++++++++++++++++ tools/testing/selftests/kvm/arm64/get-reg-list.c | 12 + tools/testing/selftests/kvm/arm64/set_id_regs.c | 95 ++++++ tools/testing/selftests/kvm/lib/arm64/handlers.S | 10 + 22 files changed, 840 insertions(+), 23 deletions(-)
---
base-commit: fd73f4a6659897191fa0d40695fe370925dd3780
change-id: 20230303-arm64-gcs-e311ab0d8729
Best regards,
Signature · 1 line
Mark Brown <[email protected]>
PATCH v21 1/15 arm64/gcs: Ensure FGTs for EL1 GCS instructions are disabled
The initial EL2 setup for GCS did not include disabling of EL1 usage of GCS instructions, also disable these traps.
--- arch/arm64/include/asm/el2_setup.h | 5 +++++ 1 file changed, 5 insertions(+)
arch/arm64/include/asm/el2_setup.h +5 -0
@@ -393,6 +393,11 @@ orr x0, x0, #HFGRTR_EL2_nGCS_EL1_MASK orr x0, x0, #HFGRTR_EL2_nGCS_EL0_MASK +/* Disable traps of GCS instructions at EL1 */ + orr x2, x2, #HFGITR_EL2_nGCSEPP_MASK + orr x2, x2, #HFGITR_EL2_nGCSSTR_EL1_MASK + orr x2, x2, #HFGITR_EL2_nGCSPUSHM_EL1_MASK + .Lskip_gce_fgt_\@: .Lset_fgt_\@:
Signature · 1 line
2.47.3
PATCH v21 2/15 KVM: arm64: Refuse to start a guest with S1PIE or S1POE but not TCR2
In reply to Mark BrownSince there is an architectural dependency between the features as an optimisation we only context switch guest registers for FEAT_S1PIE and FEAT_S1POE if the guest also has FEAT_TCR2. We do not, however, enforce this as a requirement when starting a guest and only configure the traps for accessing the registers based on their individual features. This means that a VMM can configure a guest which can read and write the system registers for FEAT_S1PIE and FEAT_S1POE without the hypervisor updating the values of these registers for the guest.
Avoid this by refusing to create a guest with an affected configuration.
Rather than doing something data driven we open code the checks, I started doing something data driven but it was very clear that such code should be shared with the host kernel cpufeature code. Refactoring for that seemed like disproportionate effort and invasiveness for the context so is deferred for followup work.
--- arch/arm64/kvm/sys_regs.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)
arch/arm64/kvm/sys_regs.c +21 -0
@@ -5860,6 +5860,24 @@ void kvm_calculate_traps(struct kvm_vcpu *vcpu)mutex_unlock (&kvm->arch.config_lock); } +/* +* Some optimisations in fast paths would be broken by architecturally +* invalid feature combinations, reject those. +* +* This should share code with the host kernel cpufeature code, and +* make use of the MRS to generate dependencies. +*/ +static bool kvm_validate_id_regs (struct kvm *kvm) +{ +if (kvm_has_s1pie (kvm) && !kvm_has_tcr2 (kvm)) +return false; + +if (kvm_has_s1poe (kvm) && !kvm_has_tcr2 (kvm)) +return false; + +return true; +} +/* * Perform last adjustments to the ID registers that are implied by the * configuration outside of the ID regs themselves, as well as any
@@ -5928,6 +5946,9 @@ int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu)kvm_vgic_finalize_idregs (kvm); } +if (!kvm_validate_id_regs (vcpu->kvm)) +return -EINVAL ; +return 0 ; }
Signature · 1 line
2.47.3
[email protected]
AI reviewRe: [PATCH v21 02/15] KVM: arm64: Refuse to start a guest with S1PIE or S1POE but not TCR2
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Critical] kvm: arm64: TOCTOU race bypasses S1PIE/TCR2 ID register validation --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] kvm: arm64: TOCTOU race bypasses S1PIE/TCR2 ID register validation --
commit ecf3602b27fe0cb6f8ce76b1a8c6698e3dee08f4
Author: Mark Brown <[email protected]>
KVM: arm64: Refuse to start a guest with S1PIE or S1POE but not TCR2
This commit adds validation to reject starting a guest with FEAT_S1PIE or FEAT_S1POE enabled if FEAT_TCR2 is disabled, as the hypervisor requires TCR2 to properly context-switch these registers.
Quoted 4 lines
arch/arm64/kvm/sys_regs.c +0
-0
[ ... ]
arch/arm64/kvm/sys_regs.c int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu) · line 5951
Quoted 7 lines
arch/arm64/kvm/sys_regs.c +3
-0@@ -5928,6 +5946,9 @@ int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu)kvm_vgic_finalize_idregs (kvm); } +if (!kvm_validate_id_regs (vcpu->kvm)) +return -EINVAL ; +
[Severity: Critical]
Does this introduce a Time-Of-Check to Time-Of-Use (TOCTOU) regression that bypasses the S1PIE/TCR2 ID register check?
If Thread A executes the first KVM_RUN, it passes this kvm_validate_id_regs() check because the invalid configuration hasn't been set yet. The config_lock is then released.
Thread B could then execute KVM_SET_ONE_REG for the ID register, take config_lock, and inject the invalid S1PIE=1/TCR2=0 configuration via set_id_reg() in arch/arm64/kvm/sys_regs.c, bypassing the immutability check because KVM_ARCH_FLAG_HAS_RAN_ONCE is not yet set.
When Thread A proceeds to execute kvm_calculate_traps(), it reads the maliciously modified ID registers and disables the traps, before finally setting KVM_ARCH_FLAG_HAS_RAN_ONCE in kvm_arch_vcpu_run_pid_change() in arch/arm64/kvm/arm.c.
This regression allows KVM to configure traps based on S1PIE, disabling the trap for PIRE0_EL1, but skipping context-switching the register because TCR2 is disabled. Could this allow the guest to directly overwrite the host's physical PIRE0_EL1 register?
Quoted 2 lines
arch/arm64/kvm/sys_regs.c +0
-0return 0 ; }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Re: [PATCH v21 02/15] KVM: arm64: Refuse to start a guest with S1PIE or S1POE but not TCR2
Quoted 46 lines · On Wed, Sep 30, 2026 at 10:05:50PM +0000, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] kvm: arm64: TOCTOU race bypasses S1PIE/TCR2 ID register validation --commit
ecf3602b27fe0cb6f8ce76b1a8c6698e3dee08f4
Author: Mark Brown <[email protected]>KVM: arm64: Refuse to start a guest with S1PIE or S1POE but not TCR2
This commit adds validation to reject starting a guest with
FEAT_S1PIEorFEAT_S1POEenabled ifFEAT_TCR2is disabled, as the hypervisor requires TCR2 to properly context-switch these registers.Quoted 4 lines
arch/arm64/kvm/sys_regs.c +0
-0[ ... ]
arch/arm64/kvm/sys_regs.c int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu) · line 5951
Quoted 7 lines
arch/arm64/kvm/sys_regs.c +3
-0@@ -5928,6 +5946,9 @@ int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu)kvm_vgic_finalize_idregs (kvm); } +if (!kvm_validate_id_regs (vcpu->kvm)) +return -EINVAL ; +[Severity: Critical]
Does this introduce a Time-Of-Check to Time-Of-Use (TOCTOU) regression that bypasses the S1PIE/TCR2 ID register check?If Thread A executes the first
KVM_RUN, it passes thiskvm_validate_id_regs()check because the invalid configuration hasn't been set yet. Theconfig_lockis then released.Thread B could then execute
KVM_SET_ONE_REGfor the ID register, takeconfig_lock, and inject the invalid S1PIE=1/TCR2=0 configuration viaset_id_reg()inarch/arm64/kvm/sys_regs.c, bypassing the immutability check becauseKVM_ARCH_FLAG_HAS_RAN_ONCEis not yet set.When Thread A proceeds to execute
kvm_calculate_traps(), it reads the maliciously modified ID registers and disables the traps, before finally settingKVM_ARCH_FLAG_HAS_RAN_ONCEinkvm_arch_vcpu_run_pid_change()inarch/arm64/kvm/arm.c.
Quoted 4 lines
This regression allows KVM to configure traps based on S1PIE, disabling the trap for
PIRE0_EL1, but skipping context-switching the register because TCR2 is disabled. Could this allow the guest to directly overwrite the host's physicalPIRE0_EL1register?
Isn't this just a pre-existing issue and the one addressed by [0]?
https://lore.kernel.org/kvmarm/[email protected]/
I don't think it's related to patch 2/15 in any case.
Quoted 7 lines
arch/arm64/kvm/sys_regs.c int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu) · line 5953
Quoted 3 lines
arch/arm64/kvm/sys_regs.c +0
-0return 0 ; }--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
--
Cheers, Lorenzo
Re: [PATCH v21 02/15] KVM: arm64: Refuse to start a guest with S1PIE or S1POE but not TCR2
Quoted 7 lines · Lorenzo Stoakes (ARM)
Quoted 4 lines · On Wed, Sep 30, 2026 at 10:05:50PM +0000, [email protected] wrote:
This regression allows KVM to configure traps based on S1PIE, disabling the trap for
PIRE0_EL1, but skipping context-switching the register because TCR2 is disabled. Could this allow the guest to directly overwrite the host's physicalPIRE0_EL1register?
Quoted 5 lines
Isn't this just a pre-existing issue and the one addressed by [0]?
https://lore.kernel.org/kvmarm/[email protected]/
I don't think it's related to patch 2/15 in any case.
Yes, it should be due to that.
Re: [PATCH v21 02/15] KVM: arm64: Refuse to start a guest with S1PIE or S1POE but not TCR2
In reply to Mark BrownQuoted 2 lines · Mark Brown
Fixes 663abf04ee4d ("KVM: arm64: Make PIR{,E0}_EL1 save/restore conditional on FEAT_TCRX")Signed-off-by Mark Brown <[email protected]>
Great, this looks like the most sensible way of resolving the issue with userland being able to trigger a kernel oops due to the save/restore registers optimisations.
It's also entirely sensible to disallow completely broken configurations like this.
LGTM so:
arch/arm64/kvm/sys_regs.c int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu) · line 5951
Quoted 12 lines
arch/arm64/kvm/sys_regs.c +3
-0@@ -5928,6 +5946,9 @@ int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu)kvm_vgic_finalize_idregs (kvm); } +if (!kvm_validate_id_regs (vcpu->kvm)) +return -EINVAL ; +
Seems like the right place to put it as part of the finalisation process, run once per VM and guaranteeing the invariant so it's guaranteed that ctx_has_s1pie() -> ctxt_has_tcrx() and ctx_has_s1poe() -> ctxt_has_tcrx().
(Just working it through for my own understanding), tracing through it ends up at struct kvm_arch->id_regs[]:
kvm_has_tcr2()
-> kvm_has_feat()
-> __kvm_has_feat()
-> kvm_cmp_feat()
-> kvm_cmp_feat_unsigned()
-> get_idreg_field_unsigned()
-> kvm_read_vm_id_reg()
-> __vm_id_reg()
-> ka->id_regs[]Which will all have been populated by here. however failing at this point will stop KVM_ARCH_FLAG_HAS_RUN_ONCE from being set (also obviously your other series separately fixes the issue with KVM_ARCH_FLAG_FGU_INITIALIZED being set with !KVM_ARCH_FLAG_HAS_RUN_ONCE).
--
Cheers, Lorenzo
Marc Zyngier
maintainerRe: [PATCH v21 02/15] KVM: arm64: Refuse to start a guest with S1PIE or S1POE but not TCR2
In reply to Mark BrownQuoted 10 lines · Mark Brown
Since there is an architectural dependency between the features as an optimisation we only context switch guest registers for
FEAT_S1PIEandFEAT_S1POEif the guest also hasFEAT_TCR2. We do not, however, enforce this as a requirement when starting a guest and only configure the traps for accessing the registers based on their individual features. This means that a VMM can configure a guest which can read and write the system registers forFEAT_S1PIEandFEAT_S1POEwithout the hypervisor updating the values of these registers for the guest.
Quoted 7 lines
Avoid this by refusing to create a guest with an affected configuration.
Rather than doing something data driven we open code the checks, I started doing something data driven but it was very clear that such code should be shared with the host kernel cpufeature code. Refactoring for that seemed like disproportionate effort and invasiveness for the context so is deferred for followup work.
This *absolutely* needs to be data driven, and we're not going back to over two years ago. We already have most of what is needed in config.c, and it is only a matter of making sure that S1PxE is only enabled for the guest if TCR2 and ATS1A are also present. If that means additional sanitisation of the idregs when finalised, so be it.
If userspace decides to expose crap in the ID registers, that's its own problem, and we're not in the business of enforcing idiotic configurations. The only thing that matters is that the state that KVM deals with is consistent.
M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
Re: [PATCH v21 02/15] KVM: arm64: Refuse to start a guest with S1PIE or S1POE but not TCR2
Quoted 10 lines · Marc Zyngier
This *absolutely* needs to be data driven, and we're not going back to over two years ago. We already have most of what is needed in
config.c, and it is only a matter of making sure that S1PxE is only enabled for the guest if TCR2 and ATS1A are also present. If that means additional sanitisation of the idregs when finalised, so be it.If userspace decides to expose crap in the ID registers, that's its own problem, and we're not in the business of enforcing idiotic configurations. The only thing that matters is that the state that KVM deals with is consistent.
As I understand it: don't return an error for a configuration that doesn't make sense, enforce RES0 instead.
So I'm guessing something like the below against the series (+ obviously adjusting tests to reflect that)?
Cheers, Lorenzo
----8<----
From b10625d0147f74504141da1f0c837b3b2bdd3d37 Mon Sep 17 00:00:00 2001
From: "Lorenzo Stoakes (ARM)" <[email protected]>
Date: Wed, 14 Oct 2026 16:31:13 +0100
Subject: [PATCH] tmp
--- arch/arm64/include/asm/kvm_host.h | 7 ++++++- arch/arm64/kvm/config.c | 35 ++++++++++++++++++++++--------- arch/arm64/kvm/sys_regs.c | 24 --------------------- 3 files changed, 31 insertions(+), 35 deletions(-)
arch/arm64/include/asm/kvm_host.h +6 -1
@@ -1626,10 +1626,14 @@ void kvm_set_vm_id_reg(struct kvm *kvm, u32 reg, u64 val); (kvm_has_feat((k), ID_AA64MMFR3_EL1, TCRX, IMP))#define kvm_has_s1pie (k) \ - (kvm_has_feat((k), ID_AA64MMFR3_EL1, S1PIE, IMP)) + (kvm_has_tcr2(k) && \ + kvm_has_feat((k), ID_AA64ISAR2_EL1, ATS1A, IMP) && \ + kvm_has_feat((k), ID_AA64MMFR3_EL1, S1PIE, IMP))#define kvm_has_s1poe (k) \ (system_supports_poe() && \ + kvm_has_tcr2(k) && \ + kvm_has_feat((k), ID_AA64ISAR2_EL1, ATS1A, IMP) && \ kvm_has_feat((k), ID_AA64MMFR3_EL1, S1POE, IMP))#define kvm_has_ras (k) \
@@ -1640,6 +1644,7 @@ void kvm_set_vm_id_reg(struct kvm *kvm, u32 reg, u64 val);#define kvm_has_gcs (k) \ (system_supports_gcs() && \ + kvm_has_s1pie(k) && \ kvm_has_feat((k), ID_AA64PFR1_EL1, GCS, IMP))static inline bool kvm_arch_has_irq_bypass (void )
arch/arm64/kvm/config.c +25 -10
@@ -232,6 +232,21 @@ static bool not_feat_aa64el3(struct kvm *kvm)return !kvm_has_feat (kvm,FEAT_AA64EL3 ); } +static bool feat_s1pie (struct kvm *kvm) +{ +return kvm_has_s1pie (kvm); +} + +static bool feat_s1poe (struct kvm *kvm) +{ +return kvm_has_s1poe (kvm); +} + +static bool feat_gcs (struct kvm *kvm) +{ +return kvm_has_gcs (kvm); +} +static bool feat_nv2 (struct kvm *kvm) { return ((kvm_has_feat(kvm, ID_AA64MMFR4_EL1, NV_frac, NV2_ONLY) &&
@@ -393,17 +408,17 @@ static const struct reg_bits_to_feat_map hfgrtr_feat_map[] = {NEEDS_FEAT (HFGRTR_EL2_nS2POR_EL1,FEAT_S2POE ),NEEDS_FEAT (HFGRTR_EL2_nPOR_EL1 | HFGRTR_EL2_nPOR_EL0, -FEAT_S1POE ), + feat_s1poe),NEEDS_FEAT (HFGRTR_EL2_nPIR_EL1 | HFGRTR_EL2_nPIRE0_EL1, -FEAT_S1PIE ), + feat_s1pie),NEEDS_FEAT (HFGRTR_EL2_nRCWMASK_EL1,FEAT_THE ),NEEDS_FEAT (HFGRTR_EL2_nTPIDR2_EL0 | HFGRTR_EL2_nSMPRI_EL1,FEAT_SME ),NEEDS_FEAT (HFGRTR_EL2_nGCS_EL1 | HFGRTR_EL2_nGCS_EL0, -FEAT_GCS ), + feat_gcs),NEEDS_FEAT (HFGRTR_EL2_nACCDATA_EL1,FEAT_LS64_ACCDATA ), NEEDS_FEAT(HFGRTR_EL2_ERXADDR_EL1 | HFGRTR_EL2_ERXMISCn_EL1 |
@@ -474,17 +489,17 @@ static const struct reg_bits_to_feat_map hfgwtr_feat_map[] = {NEEDS_FEAT (HFGWTR_EL2_nS2POR_EL1,FEAT_S2POE ),NEEDS_FEAT (HFGWTR_EL2_nPOR_EL1 | HFGWTR_EL2_nPOR_EL0, -FEAT_S1POE ), + feat_s1poe),NEEDS_FEAT (HFGWTR_EL2_nPIR_EL1 | HFGWTR_EL2_nPIRE0_EL1, -FEAT_S1PIE ), + feat_s1pie),NEEDS_FEAT (HFGWTR_EL2_nRCWMASK_EL1,FEAT_THE ),NEEDS_FEAT (HFGWTR_EL2_nTPIDR2_EL0 | HFGWTR_EL2_nSMPRI_EL1,FEAT_SME ),NEEDS_FEAT (HFGWTR_EL2_nGCS_EL1 | HFGWTR_EL2_nGCS_EL0, -FEAT_GCS ), + feat_gcs),NEEDS_FEAT (HFGWTR_EL2_nACCDATA_EL1,FEAT_LS64_ACCDATA ), NEEDS_FEAT(HFGWTR_EL2_ERXADDR_EL1 | HFGWTR_EL2_ERXMISCn_EL1 |
@@ -674,7 +689,7 @@ static const struct reg_bits_to_feat_map hfgitr_feat_map[] = {NEEDS_FEAT (HFGITR_EL2_nGCSEPP | HFGITR_EL2_nGCSSTR_EL1 | HFGITR_EL2_nGCSPUSHM_EL1, -FEAT_GCS ), + feat_gcs),NEEDS_FEAT (HFGITR_EL2_nBRBIALL | HFGITR_EL2_nBRBINJ, FEAT_BRBE),
@@ -913,7 +928,7 @@ static const struct reg_bits_to_feat_map hcrx_feat_map[] = {NEEDS_FEAT (HCRX_EL2_SRMASKEn,FEAT_SRMASK ),NEEDS_FEAT (HCRX_EL2_PACMEn, feat_pauth_lr),NEEDS_FEAT (HCRX_EL2_EnFPM,FEAT_FPMR ), -NEEDS_FEAT (HCRX_EL2_GCSEn,FEAT_GCS ), +NEEDS_FEAT (HCRX_EL2_GCSEn, feat_gcs),NEEDS_FEAT (HCRX_EL2_EnIDCP128,FEAT_SYSREG128 ),NEEDS_FEAT (HCRX_EL2_EnSDERR, feat_aderr), NEEDS_FEAT(HCRX_EL2_TMEA, FEAT_DoubleFault2),
@@ -1062,8 +1077,8 @@ static const struct reg_bits_to_feat_map tcr2_el2_feat_map[] = {NEEDS_FEAT (TCR2_EL2_AIE ,FEAT_AIE ),NEEDS_FEAT (TCR2_EL2_POE |TCR2_EL2_E0POE , -FEAT_S1POE ), -NEEDS_FEAT (TCR2_EL2_PIE ,FEAT_S1PIE ), + feat_s1poe), +NEEDS_FEAT (TCR2_EL2_PIE , feat_s1pie),FORCE_RES0 (TCR2_EL2_RES0 ), FORCE_RES1(TCR2_EL2_RES1), };
arch/arm64/kvm/sys_regs.c +0 -24
@@ -5938,27 +5938,6 @@ void kvm_calculate_traps(struct kvm_vcpu *vcpu)mutex_unlock (&kvm->arch.config_lock); } -/* -* Some optimisations in fast paths would be broken by architecturally -* invalid feature combinations, reject those. -* -* This should share code with the host kernel cpufeature code, and -* make use of the MRS to generate dependencies. -*/ -static bool kvm_validate_id_regs (struct kvm *kvm) -{ -if (kvm_has_s1pie (kvm) && !kvm_has_tcr2 (kvm)) -return false; - -if (kvm_has_s1poe (kvm) && !kvm_has_tcr2 (kvm)) -return false; - -if (kvm_has_gcs (kvm) && !kvm_has_s1pie (kvm)) -return false; - -return true; -} -/* * Perform last adjustments to the ID registers that are implied by the * configuration outside of the ID regs themselves, as well as any
@@ -6027,9 +6006,6 @@ int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu)kvm_vgic_finalize_idregs (kvm); } -if (!kvm_validate_id_regs (vcpu->kvm)) -return -EINVAL ; -return 0 ; }
--
2.56.0
Re: [PATCH v21 02/15] KVM: arm64: Refuse to start a guest with S1PIE or S1POE but not TCR2
Quoted 7 lines · Lorenzo Stoakes (ARM)
Quoted 5 lines · Marc Zyngier
This *absolutely* needs to be data driven, and we're not going back to over two years ago. We already have most of what is needed in
config.c, and it is only a matter of making sure that S1PxE is only enabled for the guest if TCR2 and ATS1A are also present. If that means additional sanitisation of the idregs when finalised, so be it.
I do agree that's what we should have, I started down that route but as I noted in the commit log it felt like I was pulling in disproportionate refactoring given the existing gap.
Quoted 7 lines
Quoted 4 lines
If userspace decides to expose crap in the ID registers, that's its own problem, and we're not in the business of enforcing idiotic configurations. The only thing that matters is that the state that KVM deals with is consistent.
As I understand it: don't return an error for a configuration that doesn't make sense, enforce RES0 instead.
I went with erroring out there to make sure that we didn't make any mistakes with the ordering of our checks - I didn't want to accidentally end up with a situation where we look at the dependencies for feature A, decide that it's OK, then later check feature B which it depends on and turn it off due to it missing it's own prerequisite.
We can resolve that like you've done here by having the full dependency stack in every check but I worried that this would be error prone due to all the duplication, however looking at your code I think we can resolve that anyway, see blow.
Userspace really shouldn't be setting up guests where we need to make adjustments in the first place, and setting up guests isn't that hot a path so like Marc says so long as KVM protects itself we're fine.
arch/arm64/include/asm/kvm_host.h void kvm_set_vm_id_reg(struct kvm *kvm, u32 reg, u64 val); · line 1631
Quoted 5 lines
#define
kvm_has_s1pie(k)\arch/arm64/include/asm/kvm_host.h +3
-1- (kvm_has_feat ((k),ID_AA64MMFR3_EL1 ,S1PIE ,IMP )) + (kvm_has_tcr2 (k) && \ +kvm_has_feat ((k),ID_AA64ISAR2_EL1 ,ATS1A ,IMP ) && \ +kvm_has_feat ((k),ID_AA64MMFR3_EL1 ,S1PIE ,IMP ))
This will mean that every time we check for an individual feature throughout KVM we check for all the dependencies as well. A lot of the time that's not really going to matter but we have these checks in hot paths like context switches.
arch/arm64/kvm/config.c static bool not_feat_aa64el3(struct kvm *kvm) · line 238
Quoted 4 lines
arch/arm64/kvm/config.c +4
-0+static bool feat_s1pie (struct kvm *kvm) +{ +return kvm_has_s1pie (kvm); +}
We should put the dependencies in here where they're only checked during initialisation instead, that way the hot paths only need to look at the leaf features. We could also deal with the duplication concern by having the functions call each other rather than duplicating their contents:
static bool feat_gcs(struct kvm *kvm)
{
if (!feat_s1pie(kvm))
return false;
return kvm_has_gcs(kvm);
}(or whatever stylistic variation of that.)
This approach also needs the code for updating the ID registers since config.c doesn't do that at the moment. Some of the existing visibility limiting stuff (like for 32 bit ID registers without AArch32 or ID_AA64ZFR0_EL1 without SVE) could be moved over too.
TBH when I said in the commit log I was thinking of something more data driven I was thinking more of having the dependencies expressed as pure data, more like the way the test in set_id_regs is written. This also seems fine to me, the checks are similar. It's just much more invasive.
Re: [PATCH v21 02/15] KVM: arm64: Refuse to start a guest with S1PIE or S1POE but not TCR2
Quoted 66 lines · Mark Brown
Quoted 7 lines · Lorenzo Stoakes (ARM)
Quoted 5 lines · Marc Zyngier
This *absolutely* needs to be data driven, and we're not going back to over two years ago. We already have most of what is needed in
config.c, and it is only a matter of making sure that S1PxE is only enabled for the guest if TCR2 and ATS1A are also present. If that means additional sanitisation of the idregs when finalised, so be it.I do agree that's what we should have, I started down that route but as I noted in the commit log it felt like I was pulling in disproportionate refactoring given the existing gap.
Quoted 7 lines
Quoted 4 lines
If userspace decides to expose crap in the ID registers, that's its own problem, and we're not in the business of enforcing idiotic configurations. The only thing that matters is that the state that KVM deals with is consistent.
As I understand it: don't return an error for a configuration that doesn't make sense, enforce RES0 instead.
I went with erroring out there to make sure that we didn't make any mistakes with the ordering of our checks - I didn't want to accidentally end up with a situation where we look at the dependencies for feature A, decide that it's OK, then later check feature B which it depends on and turn it off due to it missing it's own prerequisite.
We can resolve that like you've done here by having the full dependency stack in every check but I worried that this would be error prone due to all the duplication, however looking at your code I think we can resolve that anyway, see blow.
Userspace really shouldn't be setting up guests where we need to make adjustments in the first place, and setting up guests isn't that hot a path so like Marc says so long as KVM protects itself we're fine.
arch/arm64/include/asm/kvm_host.h void kvm_set_vm_id_reg(struct kvm *kvm, u32 reg, u64 val); · line 1631
Quoted 5 lines
#define
kvm_has_s1pie(k)\arch/arm64/include/asm/kvm_host.h +3
-1- (kvm_has_feat ((k),ID_AA64MMFR3_EL1 ,S1PIE ,IMP )) + (kvm_has_tcr2 (k) && \ +kvm_has_feat ((k),ID_AA64ISAR2_EL1 ,ATS1A ,IMP ) && \ +kvm_has_feat ((k),ID_AA64MMFR3_EL1 ,S1PIE ,IMP ))This will mean that every time we check for an individual feature throughout KVM we check for all the dependencies as well. A lot of the time that's not really going to matter but we have these checks in hot paths like context switches.
arch/arm64/kvm/config.c static bool not_feat_aa64el3(struct kvm *kvm) · line 238
Quoted 4 lines
arch/arm64/kvm/config.c +4
-0+static bool feat_s1pie (struct kvm *kvm) +{ +return kvm_has_s1pie (kvm); +}We should put the dependencies in here where they're only checked during initialisation instead, that way the hot paths only need to look at the leaf features. We could also deal with the duplication concern by having the functions call each other rather than duplicating their contents:
static bool feat_gcs(struct kvm *kvm) { if (!feat_s1pie(kvm)) return false; return kvm_has_gcs(kvm); }
Quoted 6 lines
(or whatever stylistic variation of that.)
This approach also needs the code for updating the ID registers since
config.cdoesn't do that at the moment. Some of the existing visibility limiting stuff (like for 32 bit ID registers without AArch32 orID_AA64ZFR0_EL1without SVE) could be moved over too.
Hopefully Fuad + Marc's efforts [0] make all of this fairly easy :) as we now have a specific dependency mechanism, and with GCS added on respin should enforce all this correctly.
Quoted 5 lines
TBH when I said in the commit log I was thinking of something more data driven I was thinking more of having the dependencies expressed as pure data, more like the way the test in
set_id_regsis written. This also seems fine to me, the checks are similar. It's just much more invasive.
Indeed. Perhaps the config.c approach will make it easier to convert JSON -> code also?
--
Cheers, Lorenzo
Re: [PATCH v21 02/15] KVM: arm64: Refuse to start a guest with S1PIE or S1POE but not TCR2
Quoted 9 lines · Lorenzo Stoakes (ARM)
Quoted 4 lines · Mark Brown
TBH when I said in the commit log I was thinking of something more data driven I was thinking more of having the dependencies expressed as pure data, more like the way the test in
set_id_regsis written. This also seems fine to me, the checks are similar. It's just much more invasive.Indeed. Perhaps the
config.capproach will make it easier to convert JSON -> code also?
I'm not sure how much of a difference it makes given that there's still code for the actual dependency set.
PATCH v21 3/15 KVM: arm64: Manage GCS access and registers for guests
In reply to Mark BrownGCS introduces a number of system registers, on systems with GCS we need to context switch them and expose them to VMMs to allow guests to use GCS.
In order to allow guests to use GCS we also need to configure HCRX_EL2.GCSEn, if this is not set GCS instructions will be noops and CHKFEAT will report GCS as disabled.
--- arch/arm64/include/asm/kvm_emulate.h | 3 ++ arch/arm64/include/asm/kvm_host.h | 14 ++++++++++ arch/arm64/include/asm/vncr_mapping.h | 2 ++ arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h | 27 ++++++++++++++++++ arch/arm64/kvm/hyp/vhe/sysreg-sr.c | 10 +++++++ arch/arm64/kvm/sys_regs.c | 45 ++++++++++++++++++++++++++++++ 6 files changed, 101 insertions(+)
arch/arm64/include/asm/kvm_emulate.h +3 -0
@@ -712,6 +712,9 @@ static inline void vcpu_set_hcrx(struct kvm_vcpu *vcpu)if (kvm_has_feat (kvm,ID_AA64ISAR1_EL1 ,LS64 ,LS64_V )) vcpu->arch.hcrx_el2 |= HCRX_EL2_EnASR; +if (kvm_has_gcs (kvm)) + vcpu->arch.hcrx_el2 |= HCRX_EL2_GCSEn; +/* * NV3 is a host-specific extension, and we always use * it when present and that the guest uses NV. It may
arch/arm64/include/asm/kvm_host.h +14 -0
@@ -500,6 +500,10 @@ enum vcpu_sysreg {POR_EL0 ,/* Permission Overlay Register 0 (EL0) */ +/* Guarded Control Stack registers */ +GCSCRE0_EL1 ,/* Guarded Control Stack Control (EL0) */ +GCSPR_EL0 ,/* Guarded Control Stack Pointer (EL0) */ +/* FP/SIMD/SVE */ SVCR , FPMR,
@@ -520,6 +524,8 @@ enum vcpu_sysreg {PIRE0_EL2 ,/* Permission Indirection Register 0 (EL2) */ PIR_EL2 ,/* Permission Indirection Register 1 (EL2) */ POR_EL2 ,/* Permission Overlay Register 2 (EL2) */ +GCSCR_EL2 ,/* Guarded Control Stack Control Register (EL2) */ +GCSPR_EL2 ,/* Guarded Control Stack Pointer Register (EL2) */ SPSR_EL2 ,/* EL2 saved program status register */ ELR_EL2 ,/* EL2 exception link register */ AFSR0_EL2, /* Auxiliary Fault Status Register 0 (EL2) */
@@ -592,6 +598,10 @@ enum vcpu_sysreg {VNCR (VDISR_EL2 ),VNCR (VSESR_EL2 ), +/* Guarded Control Stack registers */ +VNCR (GCSPR_EL1 ),/* Guarded Control Stack Pointer (EL1) */ +VNCR (GCSCR_EL1 ),/* Guarded Control Stack Control (EL1) */ +VNCR (HFGRTR_EL2 ),VNCR (HFGWTR_EL2 ), VNCR(HFGITR_EL2),
@@ -1654,6 +1664,10 @@ void kvm_set_vm_id_reg(struct kvm *kvm, u32 reg, u64 val);#define kvm_has_sctlr2 (k) \ (kvm_has_feat((k), ID_AA64MMFR3_EL1, SCTLRX, IMP)) +#define kvm_has_gcs (k) \ + (system_supports_gcs() && \ + kvm_has_feat((k), ID_AA64PFR1_EL1, GCS, IMP)) +static inline bool kvm_arch_has_irq_bypass (void ) {return true;
arch/arm64/include/asm/vncr_mapping.h +2 -0
@@ -97,6 +97,8 @@#define VNCR_PMSNEVFR_EL1 0x850#define VNCR_PMSDSFR_EL1 0x858#define VNCR_TRFCR_EL1 0x880 +#define VNCR_GCSPR_EL1 0x8C0 +#define VNCR_GCSCR_EL1 0x8D0#define VNCR_MPAM1_EL1 0x900#define VNCR_MPAMHCR_EL2 0x930#define VNCR_MPAMVPMV_EL2 0x938
arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h +27 -0
@@ -17,6 +17,7 @@#include <asm/kvm_mmu.h> static inline bool ctxt_has_s1poe (struct kvm_cpu_context *ctxt); +static inline bool ctxt_has_gcs (struct kvm_cpu_context *ctxt);static inline struct kvm_vcpu *ctxt_to_vcpu (struct kvm_cpu_context *ctxt) {
@@ -67,6 +68,11 @@ static inline void __sysreg_save_user_state(struct kvm_cpu_context *ctxt) {ctxt_sys_reg (ctxt,TPIDR_EL0 ) =read_sysreg (tpidr_el0);ctxt_sys_reg (ctxt,TPIDRRO_EL0 ) =read_sysreg (tpidrro_el0); + +if (ctxt_has_gcs (ctxt)) { +ctxt_sys_reg (ctxt,GCSPR_EL0 ) =read_sysreg_s (SYS_GCSPR_EL0 ); +ctxt_sys_reg (ctxt,GCSCRE0_EL1 ) =read_sysreg_s (SYS_GCSCRE0_EL1 ); + } }static inline bool ctxt_has_mte (struct kvm_cpu_context *ctxt)
@@ -131,6 +137,13 @@ static inline bool ctxt_has_sctlr2(struct kvm_cpu_context *ctxt)return kvm_has_sctlr2 (kern_hyp_va (vcpu->kvm)); } +static inline bool ctxt_has_gcs (struct kvm_cpu_context *ctxt) +{ +struct kvm_vcpu *vcpu =ctxt_to_vcpu (ctxt); + +return kvm_has_gcs (kern_hyp_va (vcpu->kvm)); +} +static inline void __sysreg_save_el1_state (struct kvm_cpu_context *ctxt) {ctxt_sys_reg (ctxt,SCTLR_EL1 ) =read_sysreg_el1 (SYS_SCTLR );
@@ -144,6 +157,10 @@ static inline void __sysreg_save_el1_state(struct kvm_cpu_context *ctxt)if (ctxt_has_s1pie (ctxt)) {ctxt_sys_reg (ctxt,PIR_EL1 ) =read_sysreg_el1 (SYS_PIR );ctxt_sys_reg (ctxt,PIRE0_EL1 ) =read_sysreg_el1 (SYS_PIRE0 ); +if (ctxt_has_gcs (ctxt)) { +ctxt_sys_reg (ctxt,GCSPR_EL1 ) =read_sysreg_el1 (SYS_GCSPR ); +ctxt_sys_reg (ctxt,GCSCR_EL1 ) =read_sysreg_el1 (SYS_GCSCR ); + } }if (ctxt_has_s1poe (ctxt))
@@ -210,6 +227,11 @@ static inline void __sysreg_restore_user_state(struct kvm_cpu_context *ctxt) {write_sysreg (ctxt_sys_reg (ctxt,TPIDR_EL0 ), tpidr_el0);write_sysreg (ctxt_sys_reg (ctxt,TPIDRRO_EL0 ), tpidrro_el0); +if (ctxt_has_gcs (ctxt)) { +write_sysreg_s (ctxt_sys_reg (ctxt,GCSPR_EL0 ),SYS_GCSPR_EL0 ); +write_sysreg_s (ctxt_sys_reg (ctxt,GCSCRE0_EL1 ), +SYS_GCSCRE0_EL1 ); + } } static inline void __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt,
@@ -243,6 +265,11 @@ static inline void __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt,if (ctxt_has_s1pie (ctxt)) {write_sysreg_el1 (ctxt_sys_reg (ctxt,PIR_EL1 ),SYS_PIR );write_sysreg_el1 (ctxt_sys_reg (ctxt,PIRE0_EL1 ),SYS_PIRE0 ); + +if (ctxt_has_gcs (ctxt)) { +write_sysreg_el1 (ctxt_sys_reg (ctxt,GCSPR_EL1 ),SYS_GCSPR ); +write_sysreg_el1 (ctxt_sys_reg (ctxt,GCSCR_EL1 ),SYS_GCSCR ); + } }if (ctxt_has_s1poe (ctxt))
arch/arm64/kvm/hyp/vhe/sysreg-sr.c +10 -0
@@ -59,6 +59,9 @@ static void __sysreg_save_vel2_state(struct kvm_vcpu *vcpu)if (ctxt_has_s1pie (&vcpu->arch.ctxt)) {__vcpu_assign_sys_reg (vcpu,PIRE0_EL2 ,read_sysreg_el1 (SYS_PIRE0 ));__vcpu_assign_sys_reg (vcpu,PIR_EL2 ,read_sysreg_el1 (SYS_PIR )); + +if (ctxt_has_gcs (&vcpu->arch.ctxt)) +__vcpu_assign_sys_reg (vcpu,GCSCR_EL2 ,read_sysreg_el1 (SYS_GCSCR )); }if (ctxt_has_s1poe (&vcpu->arch.ctxt))
@@ -86,6 +89,8 @@ static void __sysreg_save_vel2_state(struct kvm_vcpu *vcpu)__vcpu_assign_sys_reg (vcpu,SP_EL2 ,read_sysreg (sp_el1));__vcpu_assign_sys_reg (vcpu,ELR_EL2 ,read_sysreg_el1 (SYS_ELR ));__vcpu_assign_sys_reg (vcpu,SPSR_EL2 ,read_sysreg_el1 (SYS_SPSR )); +if (ctxt_has_gcs (&vcpu->arch.ctxt)) +__vcpu_assign_sys_reg (vcpu,GCSPR_EL2 ,read_sysreg_el1 (SYS_GCSPR ));if (ctxt_has_sctlr2 (&vcpu->arch.ctxt))__vcpu_assign_sys_reg (vcpu,SCTLR2_EL2 ,read_sysreg_el1 (SYS_SCTLR2 ));
@@ -138,6 +143,11 @@ static void __sysreg_restore_vel2_state(struct kvm_vcpu *vcpu)if (ctxt_has_s1pie (&vcpu->arch.ctxt)) {write_sysreg_el1 (__vcpu_sys_reg (vcpu,PIR_EL2 ),SYS_PIR );write_sysreg_el1 (__vcpu_sys_reg (vcpu,PIRE0_EL2 ),SYS_PIRE0 ); + +if (ctxt_has_gcs (&vcpu->arch.ctxt)) { +write_sysreg_el1 (__vcpu_sys_reg (vcpu,GCSCR_EL2 ),SYS_GCSCR ); +write_sysreg_el1 (__vcpu_sys_reg (vcpu,GCSPR_EL2 ),SYS_GCSPR ); + } }if (ctxt_has_s1poe (&vcpu->arch.ctxt))
arch/arm64/kvm/sys_regs.c +45 -0
105 lines not on this page · raw message
Signature · 1 line
2.47.3
Re: [PATCH v21 03/15] KVM: arm64: Manage GCS access and registers for guests
Quoted 2 lines · Mark Brown
Reviewed-by Leonardo Bras <[email protected]>Signed-off-by Mark Brown <[email protected]>
With the preceding patch and the additional change below to extend the invariant to GCS as well, the issue with incorrect save/restore is resolved and this LGTM, so:
arch/arm64/kvm/sys_regs.c static bool kvm_validate_id_regs(struct kvm *kvm) · line 5921
Quoted 11 lines
arch/arm64/kvm/sys_regs.c +2
-0@@ -5875,6 +5917,9 @@ static bool kvm_validate_id_regs(struct kvm *kvm)if (kvm_has_s1poe (kvm) && !kvm_has_tcr2 (kvm))return false; +if (kvm_has_gcs (kvm) && !kvm_has_s1pie (kvm)) +return false;
OK nice this extends the invariant to cover GCS as well!
--
Cheers, Lorenzo
PATCH v21 4/15 KVM: arm64: Ensure GCS memory effects are visible
In reply to Mark BrownIn order to allow efficient implementation of GCS implementations are permitted to do GCS specific caching, with barriers implicit in stack switch operations and GCSB DSYNC as an explicit barrier. Since we do not use GCS in the hypervisor or host kernel the hypervisor cannot rely on any barriers being present in vCPU context switches other than those it explicitly inserts. Add explicit barriers when loading and saving vCPU state.
--- arch/arm64/kvm/arm.c | 7 +++++++ arch/arm64/kvm/hyp/nvhe/hyp-main.c | 7 +++++++ 2 files changed, 14 insertions(+)
arch/arm64/kvm/arm.c +7 -0
@@ -755,6 +755,13 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)vcpu_set_flag (vcpu,PKVM_HOST_STATE_DIRTY ); } +/* +* Ensure any GCS memory effects from the outgoing vCPU are +* visible elsewhere. +*/ +if (kvm_has_gcs (vcpu->kvm)) +gcsb_dsync (); +kvm_vcpu_put_debug (vcpu);kvm_arch_vcpu_put_fp (vcpu);if (has_vhe ())
arch/arm64/kvm/hyp/nvhe/hyp-main.c +7 -0
@@ -261,6 +261,13 @@ static void sync_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu)fpsimd_sve_sync (&hyp_vcpu->vcpu);sync_debug_state (hyp_vcpu); +/* +* Ensure any GCS memory effects from the outgoing vCPU are +* visible elsewhere even if the host skips syncing. +*/ +if (kvm_has_gcs (hyp_vcpu->vcpu.kvm)) +gcsb_dsync (); +if (pkvm_hyp_vcpu_is_protected (hyp_vcpu)) { host_vcpu->arch.ctxt = hyp_vcpu->vcpu.arch.ctxt; }else {
Signature · 1 line
2.47.3
PATCH v21 5/15 KVM: arm64: Set PSTATE.EXLOCK when entering an exception
In reply to Mark BrownAs per DDI 0487 R_WTXBY we need to manage PSTATE.EXLOCK when entering an exception, when the exception is entered from a lower EL the bit is cleared while if entering from the same EL it is set to GCSCR_ELx.EXLOCKEN. Implement this behaviour in enter_exception64().
--- arch/arm64/include/uapi/asm/ptrace.h | 1 + arch/arm64/kvm/hyp/exception.c | 37 ++++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+)
arch/arm64/include/uapi/asm/ptrace.h +1 -0
@@ -56,6 +56,7 @@#define PSR_C_BIT 0x20000000#define PSR_Z_BIT 0x40000000#define PSR_N_BIT 0x80000000 +#define PSR_EXLOCK_BIT 0x400000000#define PSR_BTYPE_SHIFT 10
arch/arm64/kvm/hyp/exception.c +37 -0
@@ -49,6 +49,37 @@ static void __vcpu_write_spsr_und(struct kvm_vcpu *vcpu, u64 val) vcpu->arch.ctxt.spsr_und = val; } +static bool set_exlock (struct kvm_vcpu *vcpu,unsigned long mode, +unsigned long target_mode) +{ +u64 gcscr; + +if (!kvm_has_gcs (kern_hyp_va (vcpu->kvm))) +return false; + +/* GCS can't be enabled for 32 bit */ +if (mode &PSR_MODE32_BIT ) +return false; + +/* When taking an exception to a higher EL EXLOCK is cleared. */ +if ((mode |PSR_MODE_THREAD_BIT ) != target_mode) +return false; + +/* +* When taking an exception to the same EL EXLOCK is set to +* the effective value of GCSCR_ELx.EXLOCKEN. +*/ +if (is_hyp_ctxt (vcpu)) + gcscr =vcpu_read_sys_reg (vcpu,GCSCR_EL2 ); +else + gcscr =vcpu_read_sys_reg (vcpu,GCSCR_EL1 ); + +if (gcscr & GCSCR_ELx_EXLOCKEN) +return true; + +return false; +} +/* * This performs the exception entry at a given EL (@target_mode), stashing PC * and PSTATE into ELR and SPSR respectively, and compute the new PC/PSTATE.
@@ -138,6 +169,12 @@ static void enter_exception64(struct kvm_vcpu *vcpu, unsigned long target_mode,// PSTATE.BTYPE is set to zero upon any exception to AArch64 // See ARM DDI 0487E.a, pages D1-2293 to D1-2294. +// PSTATE.EXLOCK is set to 0 upon any exception to a higher +// EL, or to GCSCR_ELx.EXLOCKEN for an exception to the same +// exception level. See ARM DDI 0487 R_WTXBY. +if (set_exlock (vcpu, mode, target_mode)) + new |=PSR_EXLOCK_BIT ; + new |=PSR_D_BIT ; new |=PSR_A_BIT ; new |=PSR_I_BIT ;
Signature · 1 line
2.47.3
Re: [PATCH v21 05/15] KVM: arm64: Set PSTATE.EXLOCK when entering an exception
Quoted 8 lines · Mark Brown
As per DDI 0487
R_WTXBYwe need to manage PSTATE.EXLOCK when entering an exception, when the exception is entered from a lower EL the bit is cleared while if entering from the same EL it is set to GCSCR_ELx.EXLOCKEN. Implement this behaviour inenter_exception64().Reviewed-by Yuan Yao <[email protected]>Reviewed-by Leonardo Bras <[email protected]>Signed-off-by Mark Brown <[email protected]>
I think this was just nits from v20, all LGTM so happy to re-tag:
--
Cheers, Lorenzo
PATCH v21 6/15 KVM: arm64: Validate GCS exception lock when emulating ERET
In reply to Mark BrownAs per DDI0487 R_TYTWB GCS adds an additional case where an illegal exception return can be generated. If all of:
- PSTATE.EXLOCK is 0.
- The EL is not being changed by the ERET.
- GCSCR_ELx.EXLOCKEN is 1.
are true then the return is illegal. Emulate this behaviour when emulating ERET for nested guests, while we're at it using the symbolic definition for EXLOCK in SPSR.
--- arch/arm64/include/asm/kvm_nested.h | 32 ++++++++++++++++++++++++++++++++ arch/arm64/kvm/emulate-nested.c | 5 ++++- arch/arm64/kvm/hyp/vhe/switch.c | 4 ++++ 3 files changed, 40 insertions(+), 1 deletion(-)
arch/arm64/include/asm/kvm_nested.h +32 -0
@@ -240,6 +240,38 @@ static inline bool kvm_auth_eretax(struct kvm_vcpu *vcpu, u64 *elr) }#endif +#ifdef CONFIG_ARM64_GCS +/* See IllegalExceptionReturn() pseudocode */ +static inline bool kvm_check_illegal_exlock_return (struct kvm_vcpu *vcpu, +u64 spsr) +{ +u64 pstate, cur_mode, target_mode; + +if (!kvm_has_gcs (vcpu->kvm)) +return false; + +if (vcpu->arch.ctxt.regs.pstate &PSR_EXLOCK_BIT ) +return false; + +/* Check the EL only, ignore thread mode */ + pstate = vcpu->arch.ctxt.regs.pstate; + cur_mode = (pstate &PSR_MODE_MASK ) |PSR_MODE_THREAD_BIT ; + target_mode = (spsr &PSR_MODE_MASK ) |PSR_MODE_THREAD_BIT ; + +if (cur_mode != target_mode) +return false; + +return vcpu_read_sys_reg (vcpu,GCSCR_EL2 ) & GCSCR_ELx_EXLOCKEN; +} + +#else +static inline bool kvm_check_illegal_exlock_return (struct kvm_vcpu *vcpu, +u64 spsr) +{ +return false; +} +#endif +#define KVM_NV_GUEST_MAP_SZ (KVM_PGTABLE_PROT_SW1 | KVM_PGTABLE_PROT_SW0)static inline u64 kvm_encode_nested_level (struct kvm_s2_trans *trans)
arch/arm64/kvm/emulate-nested.c +4 -1
@@ -2748,10 +2748,13 @@ static u64 kvm_check_illegal_exception_return(struct kvm_vcpu *vcpu, u64 spsr)* - trying to return to an illegal M value * - trying to return to a 32bit EL * - trying to return to EL1 with HCR_EL2.TGE set +* - GCSCR_ELx.EXLOCKEN is 1 and PSTATE.EXLOCK is 0 when attempting +* to return from ELx the same EL. */ if (mode == PSR_MODE_EL3t || mode == PSR_MODE_EL3h || mode ==0b00001 || (mode &BIT (1 )) || (spsr &PSR_MODE32_BIT ) || +kvm_check_illegal_exlock_return (vcpu, spsr) || (vcpu_el2_tge_is_set (vcpu) && (mode == PSR_MODE_EL1t || mode == PSR_MODE_EL1h))) {u64 mask;
@@ -2778,7 +2781,7 @@ static u64 kvm_check_illegal_exception_return(struct kvm_vcpu *vcpu, u64 spsr) mask =PSR_MODE_MASK |PSR_MODE32_BIT ;if (kvm_has_feat (vcpu->kvm,ID_AA64PFR1_EL1 ,GCS ,IMP )) - mask |=BIT_ULL (34 );/* PSTATE.EXLOCK */ + mask |=PSR_EXLOCK_BIT ; spsr |= *vcpu_cpsr (vcpu) & mask; spsr |=PSR_IL_BIT ;
arch/arm64/kvm/hyp/vhe/switch.c +4 -0
@@ -383,6 +383,10 @@ static bool kvm_hyp_handle_eret(struct kvm_vcpu *vcpu, u64 *exit_code)return false; } +/* Push GCS exception lock failures into the slow path */ +if (kvm_check_illegal_exlock_return (vcpu, spsr)) +return false; +/* If ERETAx fails, take the slow path */ if (esr_iss_is_eretax (esr)) {if (!(vcpu_has_ptrauth (vcpu) &&kvm_auth_eretax (vcpu, &elr)))
Signature · 1 line
2.47.3
Re: [PATCH v21 06/15] KVM: arm64: Validate GCS exception lock when emulating ERET
Quoted 6 lines · Mark Brown
As per DDI0487
R_TYTWBGCS adds an additional case where an illegal exception return can be generated. If all of:- PSTATE.EXLOCK is 0.
- The EL is not being changed by the ERET.
- GCSCR_ELx.EXLOCKEN is 1.
Ack can see from [0] in D.1.4.4.2 (M.d):
"If the Effective value of GCSCR_ELx.EXLOCKEN is 1 and PSTATE.EXLOCK is 0, the execution of an exception return instruction to return to the current Exception level ELx."
Quoted 7 lines
are true then the return is illegal. Emulate this behaviour when emulating ERET for nested guests, while we're at it using the symbolic definition for EXLOCK in SPSR.
Reviewed-by Leonardo Bras <[email protected]>Signed-off-by Mark Brown <[email protected]>
Some comments below. In general LGTM so:
arch/arm64/include/asm/kvm_nested.h static inline bool kvm_auth_eretax(struct kvm_vcpu *vcpu, u64 *elr) · line 244
Quoted 7 lines
arch/arm64/include/asm/kvm_nested.h +2
-0+#ifdef CONFIG_ARM64_GCS +/* See IllegalExceptionReturn() pseudocode */
Ack I see that at [1].
Quoted 10 lines
arch/arm64/include/asm/kvm_nested.h +10
-0+static inline bool kvm_check_illegal_exlock_return (struct kvm_vcpu *vcpu, +u64 spsr) +{ +u64 pstate, cur_mode, target_mode; + +if (!kvm_has_gcs (vcpu->kvm)) +return false; + +if (vcpu->arch.ctxt.regs.pstate &PSR_EXLOCK_BIT ) +return false;
arch/arm64/include/asm/kvm_nested.h static inline bool kvm_auth_eretax(struct kvm_vcpu *vcpu, u64 *elr) · line 264
Quoted 10 lines
arch/arm64/include/asm/kvm_nested.h +10
-0+ +/* Check the EL only, ignore thread mode */ + pstate = vcpu->arch.ctxt.regs.pstate; + cur_mode = (pstate &PSR_MODE_MASK ) |PSR_MODE_THREAD_BIT ; + target_mode = (spsr &PSR_MODE_MASK ) |PSR_MODE_THREAD_BIT ; + +if (cur_mode != target_mode) +return false; + +return vcpu_read_sys_reg (vcpu,GCSCR_EL2 ) & GCSCR_ELx_EXLOCKEN;
This seems identical, logically, to the psuedocode:
if (IsFeatureImplemented(FEAT_GCS) && PSTATE.EXLOCK == ‘0’ &&
PSTATE.EL == target && GetCurrentEXLOCKEN()) then
return TRUE;
end;
return FALSE;So LGTM.
arch/arm64/kvm/emulate-nested.c static u64 kvm_check_illegal_exception_return(struct kvm_vcpu *vcpu, u64 spsr) · line 2752
Quoted 10 lines
arch/arm64/kvm/emulate-nested.c +2
-0@@ -2748,10 +2748,13 @@ static u64 kvm_check_illegal_exception_return(struct kvm_vcpu *vcpu, u64 spsr)* - trying to return to an illegal M value * - trying to return to a 32bit EL * - trying to return to EL1 with HCR_EL2.TGE set +* - GCSCR_ELx.EXLOCKEN is 1 and PSTATE.EXLOCK is 0 when attempting +* to return from ELx the same EL.
This seems a little unclear, 'when attempting to return to the same EL' perhaps?
arch/arm64/kvm/emulate-nested.c static u64 kvm_check_illegal_exception_return(struct kvm_vcpu *vcpu, u64 spsr) · line 2783
Quoted 4 lines
arch/arm64/kvm/emulate-nested.c +0
-0@@ -2778,7 +2781,7 @@ static u64 kvm_check_illegal_exception_return(struct kvm_vcpu *vcpu, u64 spsr) mask =PSR_MODE_MASK |PSR_MODE32_BIT ;if (kvm_has_feat (vcpu->kvm,ID_AA64PFR1_EL1 ,GCS ,IMP ))
Could use kvm_has_gcs()?
arch/arm64/kvm/emulate-nested.c static u64 kvm_check_illegal_exception_return(struct kvm_vcpu *vcpu, u64 spsr) · line 2784
Quoted 2 lines
arch/arm64/kvm/emulate-nested.c +1
-1- mask |=BIT_ULL (34 );/* PSTATE.EXLOCK */ + mask |=PSR_EXLOCK_BIT ;
Ah nice to not hard code that any more!
arch/arm64/kvm/hyp/vhe/switch.c static bool kvm_hyp_handle_eret(struct kvm_vcpu *vcpu, u64 *exit_code) · line 389
Quoted 12 lines
arch/arm64/kvm/hyp/vhe/switch.c +4
-0@@ -383,6 +383,10 @@ static bool kvm_hyp_handle_eret(struct kvm_vcpu *vcpu, u64 *exit_code)return false; } +/* Push GCS exception lock failures into the slow path */ +if (kvm_check_illegal_exlock_return (vcpu, spsr)) +return false; +
Ah yeah because false -> slow path and illegal exceptions shouldn't be fast path :)
--
Cheers, Lorenzo
PATCH v21 7/15 KVM: arm64: Forward GCS exceptions to nested guests
In reply to Mark BrownFor nested guests where HFGITR_EL2.nGCSSTR_EL1 is clear or when there are L2 GCS data check exceptions we need to forward the exception to the guest. Add handling to do so.
--- arch/arm64/kvm/handle_exit.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)
arch/arm64/kvm/handle_exit.c +15 -3
@@ -304,12 +304,24 @@ static int handle_svc(struct kvm_vcpu *vcpu)return 1 ; } +/* +* We might get GCS exceptions that need to be forwarded to the +* hypervisor when a nested guest has HFGITR_EL2.nGCSSTR_EL1 clear, or +* for a GCS data check exception for a L2 guest. +*/ static int kvm_handle_gcs (struct kvm_vcpu *vcpu) { -/* We don't expect GCS, so treat it with contempt */ -if (kvm_has_feat (vcpu->kvm,ID_AA64PFR1_EL1 ,GCS ,IMP )) -WARN_ON_ONCE (1 ); +if (!kvm_has_gcs (vcpu->kvm)) { +kvm_inject_undefined (vcpu); +return 1 ; + } +if (vcpu_has_nv (vcpu)) { +kvm_inject_nested_sync (vcpu,kvm_vcpu_get_esr (vcpu)); +return 1 ; + } + +WARN_ON_ONCE (1 );kvm_inject_undefined (vcpu);return 1 ; }
Signature · 1 line
2.47.3
Re: [PATCH v21 07/15] KVM: arm64: Forward GCS exceptions to nested guests
Quoted 1 line · Mark Brown
For nested guests where HFGITR_EL2.nGCSSTR_EL1 is clear or when there are
I see at [0] that, if HFGITR_EL2.nGCSSTR_EL1 is clear:
"If EL2 is implemented and enabled in the current Security state, and either EL3 is not implemented or SCR_EL3.FGTEn == 1, then execution at EL1 using AArch64 of any of the specified instructions generates a GCS exception with EC syndrome value 0x2D, unless the instruction generates a higher priority exception."
For:
GCSSTR.
GCSSTTR when PSTATE.UAO is 1.
GCSSTTR when the Effective value of HCR_EL2.{NV, NV1} is {1, 1}.
So privileged GCS stores.
Quoted 5 lines
L2 GCS data check exceptions we need to forward the exception to the guest. Add handling to do so.
Reviewed-by Leonardo Bras <[email protected]>Signed-off-by Mark Brown <[email protected]>
(A bunch of commentary here, just reasoning out loud for my own understanding.)
All LGTM, so:
arch/arm64/kvm/handle_exit.c static int handle_svc(struct kvm_vcpu *vcpu) · line 311
Quoted 10 lines
arch/arm64/kvm/handle_exit.c +5
-0+/* +* We might get GCS exceptions that need to be forwarded to the +* hypervisor when a nested guest has HFGITR_EL2.nGCSSTR_EL1 clear, or +* for a GCS data check exception for a L2 guest. +*/
OK so I see this is used as a handler in:
static exit_handle_fn arm_exit_handlers[] = {
...
[ESR_ELx_EC_GCS] = kvm_handle_gcs,
};And as per the arm ref page, 0x2D == ESR_ELx_EC_GCS.
And it's always something being injected, so returning 1 (handled) is fine.
arch/arm64/kvm/handle_exit.c static int handle_svc(struct kvm_vcpu *vcpu) · line 317
Quoted 9 lines
static int
kvm_handle_gcs(struct kvm_vcpu *vcpu)
{arch/arm64/kvm/handle_exit.c +4
-3-/* We don't expect GCS, so treat it with contempt */ -if (kvm_has_feat (vcpu->kvm,ID_AA64PFR1_EL1 ,GCS ,IMP )) -WARN_ON_ONCE (1 ); +if (!kvm_has_gcs (vcpu->kvm)) { +kvm_inject_undefined (vcpu); +return 1 ; + }
OK so make it not-a-kernel-warning if unsupported...
arch/arm64/kvm/handle_exit.c static int handle_svc(struct kvm_vcpu *vcpu) · line 320
Quoted 3 lines
arch/arm64/kvm/handle_exit.c +2
-0+if (vcpu_has_nv (vcpu)) { +kvm_inject_nested_sync (vcpu,kvm_vcpu_get_esr (vcpu));
...And this is the L0 at physical EL2 -> vEL2 (in physical EL1) injecting a synchronous exception.
So it seems to me the key point of the patch is to forward to nested guests, hence the subject line :>)
arch/arm64/kvm/handle_exit.c static int handle_svc(struct kvm_vcpu *vcpu) · line 324
Quoted 4 lines
arch/arm64/kvm/handle_exit.c +4
-0+return 1 ; + } + +WARN_ON_ONCE (1 );
And this is an 'impossible' case because it should have been handled natively otherwise. So it's a bug in KVM if it ever happened.
--
Cheers, Lorenzo
PATCH v21 8/15 KVM: arm64: Enforce EXLOCK for SPSR and ELR
In reply to Mark BrownAs per I_CFFNS and the pseudocode for the SPSR_ELx and ELR_ELx registers a GCS exception with ExType 1 is generated for attempts to write to those registers when both GCSCR_ELx.EXLOCKEN and PSTATE.EXLOCK are set. Ensure that this is enforced for guests if access to these registers from the guest is handled by the hypervisor.
--- arch/arm64/include/asm/kvm_emulate.h | 8 +++++++ arch/arm64/kvm/sys_regs.c | 42 ++++++++++++++++++++++++++++++++---- 2 files changed, 46 insertions(+), 4 deletions(-)
arch/arm64/include/asm/kvm_emulate.h +8 -0
@@ -81,6 +81,14 @@ int kvm_inject_nested_irq(struct kvm_vcpu *vcpu);int kvm_inject_nested_sea (struct kvm_vcpu *vcpu,bool iabt,u64 addr);int kvm_inject_nested_serror (struct kvm_vcpu *vcpu,u64 esr); +static inline void kvm_inject_exlock (struct kvm_vcpu *vcpu) +{ +u64 esr =FIELD_PREP (ESR_ELx_EC_MASK, ESR_ELx_EC_GCS) | ESR_ELx_IL | +FIELD_PREP (ESR_ELx_ExType_MASK, ESR_ELx_ExType_EXLOCK); + +kvm_inject_sync (vcpu, esr); +} +static inline void kvm_inject_nested_sve_trap (struct kvm_vcpu *vcpu) { u64 esr = FIELD_PREP(ESR_ELx_EC_MASK, ESR_ELx_EC_SVE) |
arch/arm64/kvm/sys_regs.c +38 -4
62 lines not on this page · raw message
Signature · 1 line
2.47.3
Re: [PATCH v21 08/15] KVM: arm64: Enforce EXLOCK for SPSR and ELR
Quoted 3 lines · Mark Brown
As per
I_CFFNSand the pseudocode for theSPSR_ELxandELR_ELxregisters a GCS exception with ExType 1 is generated for attempts to write to those registers when both GCSCR_ELx.EXLOCKEN and PSTATE.EXLOCK are set.
From [4] I see I_CFFNS is defined as:
"When an MSR instruction would write to the relevant ELR_ELx or SPSR_ELx for the current Exception level ELy, the Effective value of GCSCR_ELy.EXLOCKEN and PSTATE.EXLOCK may prevent the write."
So the lock applies to writes to the current EL's own ELR/SPSR rather than to any ELR_ELx/SPSR_ELx the current EL can write to?
Quoted 34 lines
Ensure that this is enforced for guests if access to these registers from the guest is handled by the hypervisor.
Reviewed-by Leonardo Bras <[email protected]>Signed-off-by Mark Brown <[email protected]>--- arch/arm64/include/asm/kvm_emulate.h | 8 +++++++ arch/arm64/kvm/sys_regs.c | 42 ++++++++++++++++++++++++++++++++---- 2 files changed, 46 insertions(+), 4 deletions(-)arch/arm64/include/asm/kvm_emulate.h +8
-0@@ -81,6 +81,14 @@ int kvm_inject_nested_irq(struct kvm_vcpu *vcpu);int kvm_inject_nested_sea (struct kvm_vcpu *vcpu,bool iabt,u64 addr);int kvm_inject_nested_serror (struct kvm_vcpu *vcpu,u64 esr); +static inline void kvm_inject_exlock (struct kvm_vcpu *vcpu) +{ +u64 esr =FIELD_PREP (ESR_ELx_EC_MASK, ESR_ELx_EC_GCS) | ESR_ELx_IL | +FIELD_PREP (ESR_ELx_ExType_MASK, ESR_ELx_ExType_EXLOCK); + +kvm_inject_sync (vcpu, esr); +} +static inline void kvm_inject_nested_sve_trap (struct kvm_vcpu *vcpu) { u64 esr = FIELD_PREP(ESR_ELx_EC_MASK, ESR_ELx_EC_SVE) |arch/arm64/kvm/sys_regs.c +0
-0
arch/arm64/kvm/sys_regs.c static bool access_sp_el1(struct kvm_vcpu *vcpu, · line 2869
Quoted 4 lines
arch/arm64/kvm/sys_regs.c +1
-0@@ -2866,14 +2866,42 @@ static bool access_sp_el1(struct kvm_vcpu *vcpu,return true; } +static inline bool sysregs_exlocked (struct kvm_vcpu *vcpu)
So if this returns true, an EXLOCKException is raised, otherwise SPSR_EL1 is set (based on caller below).
From [0] the pseudocode for MSR SPSR_EL1, <Xt> is:
elsif PSTATE.EL == EL2 then
if IsFeatureImplemented(FEAT_GCS) && GetCurrentEXLOCKEN() && !Halted() && PSTATE.EXLOCK == '1' && ELIsInHost(EL2) then
EXLOCKException();
elsif ELIsInHost(EL2) then
SPSR_EL2() = X{64}(t);
else
SPSR_EL1() = X{64}(t);
end;Reading [2] and [3], ELIsInHost(EL2) is equivalent to vcpu_el2_e2h_is_set(), but the code doesn't check this anywhere?
The calling function only ever writes to SPSR_EL1, which implies !ELIsInHost() (I guess because if it is true the writes are just done directly to registers?)
But if that's always false, then an exception can never be raised due to the first condition being false, and then the whole thing collapses to SPSR_EL1() = X{64}(t).
So PSTATE.EL == EL2 is ~ I gather to is_hyp_ctxt() (guest's view of its own EL) and thus this would be equivalent to:
if (is_hyp_ctxt())
return false;
Wouldn't it?
But for the PSTATE.EL == EL1 branch (what the hardware runs for a vEL2 guest), also from [0]:
if IsFeatureImplemented(FEAT_GCS) && GetCurrentEXLOCKEN() && !Halted() && PSTATE.EXLOCK == '1' && !(EffectiveHCR_EL2_NVx() IN {'x11'}) then
EXLOCKException();
elsif IsFeatureImplemented(FEAT_S1POE2) && EffectiveHCR_EL2_NVx() IN {'x11'} && FGDTState.nH == '1' then
AArch64_FGDTSystemAccessTrap(EL1, 0x18);
elsif EffectiveHCR_EL2_NVx() == '011' then
AArch64_SystemAccessTrap(EL2, 0x18);
elsif EffectiveHCR_EL2_NVx() IN {'111'} then
NVMem(0x160) = X{64}(t);
else
SPSR_EL1() = X{64}(t);
end;EffectiveHCR_EL2_NVx() (see [5]) obtains the nested virt configuration based on HCR flags.
In __compute_hcr():
static u64 __compute_hcr(struct kvm_vcpu *vcpu)
{
...
if (is_hyp_ctxt(vcpu)) {
...
hcr |= HCR_NV | HCR_NV2 | HCR_AT | HCR_TTLB;
if (!vcpu_el2_e2h_is_set(vcpu))
hcr |= HCR_NV1;
...
}
...
}So for a vEL2 guest this is either '101' (guest E2H=1), or '111' (guest E2H=0).
So that leaves us with:
if IsFeatureImplemented(FEAT_GCS) && GetCurrentEXLOCKEN() && !Halted() && PSTATE.EXLOCK == '1' && !(EffectiveHCR_EL2_NVx() IN {'x11'}) then
EXLOCKException();
elsif EffectiveHCR_EL2_NVx() IN {'111'} then
NVMem(0x160) = X{64}(t);
else
SPSR_EL1() = X{64}(t);And from include/asm/vncr_mapping.h:
#define VNCR_SPSR_EL1 0x160
So that's the VNCR write that the hardware does.
So for '101' the hardware does the EXLOCK check and the write itself, for '111' the check is skipped and the write goes to the VNCR page.
In both cases, nothing traps to KVM (only '011', NV1 without NV2 would - but KVM never configures that as it requires NV2).
So... TL;DR is, shouldn't this function look like:
static inline bool sysregs_exlocked(struct kvm_vcpu *vcpu)
{
if (!kvm_has_gcs(vcpu->kvm))
return false;
if (!(vcpu->arch.ctxt.regs.pstate & PSR_EXLOCK_BIT))
return false;
if (is_hyp_ctxt(vcpu))
return false;
return vcpu_read_sys_reg(vcpu, GCSCR_EL1) & GCSCR_ELx_EXLOCKEN;
}?
arch/arm64/kvm/sys_regs.c static bool access_sp_el1(struct kvm_vcpu *vcpu, · line 2882
Quoted 13 lines
arch/arm64/kvm/sys_regs.c +13
-0+{ +u64 gcscr; + +if (!kvm_has_gcs (vcpu->kvm)) +return false; + +if (!(vcpu->arch.ctxt.regs.pstate &PSR_EXLOCK_BIT )) +return false; + +/* +* Note that the EXLOCKEN for the running EL is checked +* regardless of the register written to. +*/
This seems to contradict [4] - the register written to is what decides whether the lock applies?
arch/arm64/kvm/sys_regs.c static bool access_sp_el1(struct kvm_vcpu *vcpu, · line 2886
Quoted 4 lines
arch/arm64/kvm/sys_regs.c +4
-0+if (is_hyp_ctxt (vcpu)) + gcscr =vcpu_read_sys_reg (vcpu,GCSCR_EL2 ); +else + gcscr =vcpu_read_sys_reg (vcpu,GCSCR_EL1 );
GetCurrentEXLOCKEN() is defined in [1] as:
...
case PSTATE.EL of
...
when EL1 =>
return GCSCR_EL1().EXLOCKEN == ‘1’;
when EL2 =>
return GCSCR_EL2().EXLOCKEN == ‘1’;
...
end;And is_hyp_ctxt() -> PSTATE.EL == 2, else -> PSTATE.EL == 1 so this matches up with:
arch/arm64/kvm/sys_regs.c static bool access_sp_el1(struct kvm_vcpu *vcpu, · line 2888
Quoted 2 lines
arch/arm64/kvm/sys_regs.c +2
-0+ +return gcscr & GCSCR_ELx_EXLOCKEN;
Where this is the .EXLOCKEN == '1' bit.
arch/arm64/kvm/sys_regs.c static bool access_sp_el1(struct kvm_vcpu *vcpu, · line 2900
Quoted 13 lines
arch/arm64/kvm/sys_regs.c +8
-1+} +static bool access_elr (struct kvm_vcpu *vcpu,struct sys_reg_params *p,const struct sys_reg_desc *r) { -if (p->is_write) +if (p->is_write) { +if (sysregs_exlocked (vcpu)) { +kvm_inject_exlock (vcpu); +return false; + } +
(I assume ELR mirrors SPSR but I haven't checked).
Quoted 29 lines
arch/arm64/kvm/sys_regs.c +10
-4vcpu_write_sys_reg (vcpu, p->regval,ELR_EL1 ); - else + }else { p->regval =vcpu_read_sys_reg (vcpu,ELR_EL1 ); + }return true; }@@ -2882,10 +2910,16 @@ static bool access_spsr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, const struct sys_reg_desc *r) { -if (p->is_write) +if (p->is_write) { +if (sysregs_exlocked (vcpu)) { +kvm_inject_exlock (vcpu); +return false; + } +__vcpu_assign_sys_reg (vcpu,SPSR_EL1 , p->regval); -else + }else { p->regval =__vcpu_sys_reg (vcpu,SPSR_EL1 ); + }return true; } --
Quoted 3 lines
2.47.3
[0]:https://support.arm.com/documentation/111107/2026-09/AArch64-Registers/SPSR-EL1--Saved-Program-Status-Register--EL1-?lang=en [1]:https://support.arm.com/documentation/ddi0487/md/-Part-J-Architectural-Pseudocode/-Chapter-J1-A-profile-Architecture-Pseudocode/-J1-2-Pseudocode-for-AArch64-operation/-J1-2-267-GetCurrentEXLOCKEN [2]:https://support.arm.com/documentation/ddi0487/md/-Part-J-Architectural-Pseudocode/-Chapter-J1-A-profile-Architecture-Pseudocode/-J1-4-Shared-pseudocode/-J1-4-552-ELIsInHost?lang=en [3]:https://support.arm.com/documentation/ddi0487/md/-Part-J-Architectural-Pseudocode/-Chapter-J1-A-profile-Architecture-Pseudocode/-J1-4-Shared-pseudocode/-J1-4-558-EffectiveHCR-EL2-E2H?lang=en [4]:https://support.arm.com/documentation/ddi0487/md/-Part-D-The-AArch64-System-Level-Architecture/-Chapter-D11-The-Guarded-Control-Stack/-D11-4-Exception-returns/-D11-4-1-Pushing-and-popping-exception-return-state?lang=en [5]:https://support.arm.com/documentation/ddi0487/md/-Part-J-Architectural-Pseudocode/-Chapter-J1-A-profile-Architecture-Pseudocode/-J1-4-Shared-pseudocode/-J1-4-559-EffectiveHCR-EL2-NVx?lang=en
--
Cheers, Lorenzo
Re: [PATCH v21 08/15] KVM: arm64: Enforce EXLOCK for SPSR and ELR
Quoted 5 lines · Lorenzo Stoakes (ARM)
Quoted 3 lines · Mark Brown
As per
I_CFFNSand the pseudocode for theSPSR_ELxandELR_ELxregisters a GCS exception with ExType 1 is generated for attempts to write to those registers when both GCSCR_ELx.EXLOCKEN and PSTATE.EXLOCK are set.
Quoted 8 lines
From [4] I see
I_CFFNSis defined as:"When an MSR instruction would write to the relevant
ELR_ELxorSPSR_ELxfor the current Exception level ELy, the Effective value of GCSCR_ELy.EXLOCKEN and PSTATE.EXLOCK may prevent the write."So the lock applies to writes to the current EL's own ELR/SPSR rather than to any ELR_ELx/SPSR_ELx the current EL can write to?
VHE and NV complicate things so "own" isn't just the same ELx, but my text above definitely oversimplifies too much and so is wrong.
For example refering to the pseudocode for ELR_EL1 and SPSR_EL1 we see in the MSR handling:
elsif PSTATE.EL == EL2 then
if IsFeatureImplemented(FEAT_GCS) && GetCurrentEXLOCKEN() && !Halted() && PSTATE.EXLOCK == '1' && ELIsInHost(EL2) then
EXLOCKException();and note the use of ELx for ELR/SPSR and ELy for the current exception level and GCSCR in I_CFFNS (ie, ELx vs ELy). My interpretation here is that the use of "the relevant" rather than just using ELx throughout is an effort to cover the complications resulting from VHE and NV. With the above pseudocode writes to the EL1 register from EL2 are also covered when we're in host mode - the fact that we're in host mode makes the EL1 access relevant.
I'll reword what I've written in the commit log, like I say it's wrong.
Quoted 15 lines
So... TL;DR is, shouldn't this function look like:
static inline bool
sysregs_exlocked(struct kvm_vcpu *vcpu){ if (!kvm_has_gcs(vcpu->kvm)) return false; if (!(vcpu->arch.ctxt.regs.pstate & PSR_EXLOCK_BIT)) return false; if (is_hyp_ctxt(vcpu)) return false; return vcpu_read_sys_reg(vcpu, GCSCR_EL1) & GCSCR_ELx_EXLOCKEN; }
I think so, but I'll check through again.
The confusions you identified in the bit of your mail above this were the result of me doing some but on all of the simplifications. I was trying to make things clearer by including some code that couldn't run so it was more obvious that things correspond to the pseudocode, but really that shouldn't have had any simplifications in it - we should either have all the simplifications or none of them. I'll add more comments instead.
Quoted 7 lines
arch/arm64/kvm/sys_regs.c static bool access_sp_el1(struct kvm_vcpu *vcpu, · line 2882
Quoted 4 lines
arch/arm64/kvm/sys_regs.c +4
-0+/* +* Note that the EXLOCKEN for the running EL is checked +* regardless of the register written to. +*/ This seems to contradict [4] - the register written to is what decides whether the lock applies?
[4] is section D11.4.1 of DDI0487 M.d, containing rule I_CFFNS discussed above. My intent there is to express that if EXLOCK exceptions might be generated we check EXLOCKEN for the running EL, not one influenced by the written register. Some combinations of register, EL and system state do not generate exceptions but those that do use the current EL's EXLOCKEN rather than an _ELx register using GCSCR_ELx.EXLOCKEN.
Re: [PATCH v21 08/15] KVM: arm64: Enforce EXLOCK for SPSR and ELR
Quoted 12 lines · Mark Brown
Quoted 10 lines · Lorenzo Stoakes (ARM)
Quoted 3 lines · Mark Brown
As per
I_CFFNSand the pseudocode for theSPSR_ELxandELR_ELxregisters a GCS exception with ExType 1 is generated for attempts to write to those registers when both GCSCR_ELx.EXLOCKEN and PSTATE.EXLOCK are set.From [4] I see
I_CFFNSis defined as:"When an MSR instruction would write to the relevant
ELR_ELxorSPSR_ELxfor the current Exception level ELy, the Effective value of GCSCR_ELy.EXLOCKEN and PSTATE.EXLOCK may prevent the write."
Quoted 5 lines
Quoted 2 lines
So the lock applies to writes to the current EL's own ELR/SPSR rather than to any ELR_ELx/SPSR_ELx the current EL can write to?
VHE and NV complicate things so "own" isn't just the same ELx, but my text above definitely oversimplifies too much and so is wrong.
It's all incredibly tricky and I felt my brain melting out of my head going through the psuedocode/definition yesterday so it's understandable :)
Quoted 8 lines
For example refering to the pseudocode for
ELR_EL1andSPSR_EL1we see in the MSR handling:elsif PSTATE.EL == EL2 then if IsFeatureImplemented(FEAT_GCS) && GetCurrentEXLOCKEN() && !Halted() && PSTATE.EXLOCK == '1' && ELIsInHost(EL2) then EXLOCKException();
Quoted 7 lines
and note the use of ELx for ELR/SPSR and ELy for the current exception level and GCSCR in
I_CFFNS(ie, ELx vs ELy). My interpretation here is that the use of "the relevant" rather than just using ELx throughout is an effort to cover the complications resulting from VHE and NV. With the above pseudocode writes to the EL1 register from EL2 are also covered when we're in host mode - the fact that we're in host mode makes the EL1 access relevant.
Yeah agreed.
Quoted 2 lines
I'll reword what I've written in the commit log, like I say it's wrong.
Ack thanks!
Quoted 11 lines
Quoted 11 lines
So... TL;DR is, shouldn't this function look like:
static inline bool
sysregs_exlocked(struct kvm_vcpu *vcpu){ if (!kvm_has_gcs(vcpu->kvm)) return false; if (!(vcpu->arch.ctxt.regs.pstate & PSR_EXLOCK_BIT)) return false;
Quoted 7 lines
Quoted 5 lines
if (is_hyp_ctxt(vcpu)) return false; return vcpu_read_sys_reg(vcpu, GCSCR_EL1) & GCSCR_ELx_EXLOCKEN; }I think so, but I'll check through again.
Thanks!
Quoted 8 lines
The confusions you identified in the bit of your mail above this were the result of me doing some but on all of the simplifications. I was trying to make things clearer by including some code that couldn't run so it was more obvious that things correspond to the pseudocode, but really that shouldn't have had any simplifications in it - we should either have all the simplifications or none of them. I'll add more comments instead.
Yeah it's all very complicated, unfortunately I think, the psuedocode had me stumped a lot when I was reading it...
Definitely agree on all-or-nothing. If we did go the 'all' route, then we'd need to put in the ELIsInHost(EL2) conditional too
i.e. vcpu_el2_e2h_is_set() as well.
But I think simplifying that is also valid, as the resultant function above is a lot easier to reason about and avoids people having to figure out that certain bits are irrelevant (though comments could square that off too!)
Quoted 9 lines
Quoted 8 lines
arch/arm64/kvm/sys_regs.c static bool access_sp_el1(struct kvm_vcpu *vcpu, · line 2882
Quoted 5 lines
arch/arm64/kvm/sys_regs.c +4
-0+/* +* Note that the EXLOCKEN for the running EL is checked +* regardless of the register written to. +*/ This seems to contradict [4] - the register written to is what decides whether the lock applies?
Quoted 6 lines
[4] is section D11.4.1 of DDI0487 M.d, containing rule
I_CFFNSdiscussed above. My intent there is to express that if EXLOCK exceptions might be generated we check EXLOCKEN for the running EL, not one influenced by the written register. Some combinations of register, EL and system state do not generate exceptions but those that do use the current EL's EXLOCKEN rather than an_ELxregister using GCSCR_ELx.EXLOCKEN.
Yeah that's clear thanks!
IOW - the register/El/state decides _whether_ an exception can be raised, the running EL's EXLOCKEN is what is consulted _when_ it can be raised.
--
Cheers, Lorenzo
PATCH v21 9/15 KVM: arm64: Allow GCS to be enabled for guests
In reply to Mark BrownNow that required functionality for GCS is in place expose ID_AA64PFR1_EL1.GCS, allowing guests to be given the feature.
--- arch/arm64/kvm/nested.c | 3 +-- arch/arm64/kvm/sys_regs.c | 5 +++-- 2 files changed, 4 insertions(+), 4 deletions(-)
arch/arm64/kvm/nested.c +1 -2
@@ -1702,11 +1702,10 @@ u64 limit_nv_id_reg(struct kvm *kvm, u32 reg, u64 val)break ;case SYS_ID_AA64PFR1_EL1 : -/* Only support BTI, SSBS, CSV2_frac */ +/* Only support GCS, BTI, SSBS, CSV2_frac */ val &= ~(ID_AA64PFR1_EL1_PFAR | ID_AA64PFR1_EL1_MTEX | ID_AA64PFR1_EL1_THE | - ID_AA64PFR1_EL1_GCS | ID_AA64PFR1_EL1_MTE_frac | ID_AA64PFR1_EL1_NMI | ID_AA64PFR1_EL1_SME |
arch/arm64/kvm/sys_regs.c +3 -2
@@ -2193,10 +2193,12 @@ static u64 sanitise_id_aa64pfr1_el1(const struct kvm_vcpu *vcpu, u64 val) SYS_FIELD_GET(ID_AA64PFR0_EL1, RAS, pfr0) == ID_AA64PFR0_EL1_RAS_IMP)) val &= ~ID_AA64PFR1_EL1_RAS_frac; +if (!system_supports_gcs ()) + val &= ~ID_AA64PFR1_EL1_GCS ; + val &= ~ID_AA64PFR1_EL1_SME ; val &= ~ID_AA64PFR1_EL1_RNDR_trap; val &= ~ID_AA64PFR1_EL1_NMI ; - val &= ~ID_AA64PFR1_EL1_GCS ; val &= ~ID_AA64PFR1_EL1_THE ; val &= ~ID_AA64PFR1_EL1_MTEX ; val &= ~ID_AA64PFR1_EL1_PFAR ;
@@ -3444,7 +3446,6 @@ static const struct sys_reg_desc sys_reg_descs[] = { ~(ID_AA64PFR1_EL1_PFAR | ID_AA64PFR1_EL1_MTEX | ID_AA64PFR1_EL1_THE | - ID_AA64PFR1_EL1_GCS | ID_AA64PFR1_EL1_MTE_frac | ID_AA64PFR1_EL1_NMI | ID_AA64PFR1_EL1_RNDR_trap |
Signature · 1 line
2.47.3
Re: [PATCH v21 09/15] KVM: arm64: Allow GCS to be enabled for guests
Quoted 5 lines · Mark Brown
Now that required functionality for GCS is in place expose ID_AA64PFR1_EL1.GCS, allowing guests to be given the feature.
Reviewed-by Leonardo Bras <[email protected]>Signed-off-by Mark Brown <[email protected]>
This looks straightforward and sensible so:
Quoted 50 lines
--- arch/arm64/kvm/nested.c | 3 +-- arch/arm64/kvm/sys_regs.c | 5 +++-- 2 files changed, 4 insertions(+), 4 deletions(-)arch/arm64/kvm/nested.c +1
-2@@ -1702,11 +1702,10 @@ u64 limit_nv_id_reg(struct kvm *kvm, u32 reg, u64 val)break ;case SYS_ID_AA64PFR1_EL1 : -/* Only support BTI, SSBS, CSV2_frac */ +/* Only support GCS, BTI, SSBS, CSV2_frac */ val &= ~(ID_AA64PFR1_EL1_PFAR | ID_AA64PFR1_EL1_MTEX | ID_AA64PFR1_EL1_THE | - ID_AA64PFR1_EL1_GCS | ID_AA64PFR1_EL1_MTE_frac | ID_AA64PFR1_EL1_NMI | ID_AA64PFR1_EL1_SME |arch/arm64/kvm/sys_regs.c +3
-3@@ -2193,10 +2193,12 @@ static u64 sanitise_id_aa64pfr1_el1(const struct kvm_vcpu *vcpu, u64 val) SYS_FIELD_GET(ID_AA64PFR0_EL1, RAS, pfr0) == ID_AA64PFR0_EL1_RAS_IMP)) val &= ~ID_AA64PFR1_EL1_RAS_frac; +if (!system_supports_gcs ()) + val &= ~ID_AA64PFR1_EL1_GCS ; + val &= ~ID_AA64PFR1_EL1_SME ; val &= ~ID_AA64PFR1_EL1_RNDR_trap; val &= ~ID_AA64PFR1_EL1_NMI ; - val &= ~ID_AA64PFR1_EL1_GCS ; val &= ~ID_AA64PFR1_EL1_THE ; val &= ~ID_AA64PFR1_EL1_MTEX ; val &= ~ID_AA64PFR1_EL1_PFAR ;@@ -3444,7 +3446,6 @@ static const struct sys_reg_desc sys_reg_descs[] = { ~(ID_AA64PFR1_EL1_PFAR | ID_AA64PFR1_EL1_MTEX | ID_AA64PFR1_EL1_THE | - ID_AA64PFR1_EL1_GCS | ID_AA64PFR1_EL1_MTE_frac | ID_AA64PFR1_EL1_NMI | ID_AA64PFR1_EL1_RNDR_trap | --
Quoted 3 lines
2.47.3
--
Cheers, Lorenzo
PATCH v21 10/15 KVM: selftests: arm64: Check that invalid feature combinations are rejected
In reply to Mark BrownIn order to optimise fast paths KVM explicitly rejects configurations with S1PIE or S1POE but not TCR2, add coverage of this in the set_id_regs test. We have a list of invalid configurations, for each of them we try to run a VM and fail the test if it succeeds. We do feature detection by validating that we can write the fields with failing values.
Since this misfiring can disrupt some of the other tests due to the kernel refusing to start guests we run the new tests first, improving diagnostics in the failing case.
--- tools/testing/selftests/kvm/arm64/set_id_regs.c | 87 +++++++++++++++++++++++++ 1 file changed, 87 insertions(+)
tools/testing/selftests/kvm/arm64/set_id_regs.c +87 -0
@@ -803,6 +803,89 @@ static void test_reset_preserves_id_regs(struct kvm_vcpu *vcpu)ksft_test_result_pass ("%s\n" , __func__); } +struct reg_ftr_val { +u64 reg; +u64 mask; +u64 val; +}; + +#define REG_FTR_VAL (r, f, v) \ + { .reg = ARM64_SYS_REG(sys_reg_Op0(SYS_ ## r), \ + sys_reg_Op1(SYS_ ## r), \ + sys_reg_CRn(SYS_ ## r), \ + sys_reg_CRm(SYS_ ## r), \ + sys_reg_Op2(SYS_ ## r)), \ + .mask = r ## _ ## f ## _MASK, \ + .val = (r ## _ ## f ## _ ## v << r ## _ ## f ## _SHIFT) } + +static const struct reg_ftr_val s1pie_no_tcr2[] = { +REG_FTR_VAL (ID_AA64MMFR3_EL1 ,TCRX ,NI ), +REG_FTR_VAL (ID_AA64MMFR3_EL1 ,S1PIE ,IMP ), + { } +}; + +static const struct reg_ftr_val s1poe_no_tcr2[] = { +REG_FTR_VAL (ID_AA64MMFR3_EL1 ,TCRX ,NI ), +REG_FTR_VAL (ID_AA64MMFR3_EL1 ,S1POE ,IMP ), + { } +}; + +struct ftr_config { +const char *name; +const struct reg_ftr_val *regs; +}; + +static const struct ftr_config invalid_configs[] = { + { .name ="S1PIE without TCRX" , .regs = s1pie_no_tcr2 }, + { .name ="S1POE without TCRX" , .regs = s1poe_no_tcr2 }, +}; + +static void test_invalid_config (const struct ftr_config *config) +{ +struct kvm_vcpu *vcpu; +struct kvm_vm *vm; +const struct reg_ftr_val *field; +u64 val; +int ret; + + vm =vm_create (1 ); +vm_enable_cap (vm,KVM_CAP_ARM_WRITABLE_IMP_ID_REGS ,0 ); + vcpu =vm_vcpu_add (vm,0 , guest_code); +kvm_arch_vm_finalize_vcpus (vm); + +/* +* If we don't manage to set any of the fields assume the +* system does not support the feature and skip the test. +*/ +for (field = config->regs; field->reg; field++) { + val =vcpu_get_reg (vcpu, field->reg); + val &= ~field->mask; + val |= field->val; +__vcpu_set_reg (vcpu, field->reg, val); + +if (vcpu_get_reg (vcpu, field->reg) != val) { +ksft_print_msg ("Test setup not supported\n" ); +ksft_test_result_skip ("refuse %s\n" , config->name); + goto out; + } + } + + ret =_vcpu_run (vcpu); +ksft_test_result (ret <0 && errno ==EINVAL ,"refuse %s\n" , + config->name); +out: +kvm_vm_free (vm); +} + +static void test_invalid_configs (void ) +{ +int i; + +for (i =0 ; i <ARRAY_SIZE (invalid_configs); i++) { +test_invalid_config (&invalid_configs[i]); + } +} +int main (void ) {struct kvm_vcpu *vcpu;
@@ -829,12 +912,16 @@ int main(void)ksft_print_header (); test_cnt =3 +MPAM_IDREG_TEST +MTE_IDREG_TEST ; + test_cnt +=ARRAY_SIZE (invalid_configs);for (i =0 ; i <ARRAY_SIZE (test_regs); i++)for (j =0 ; test_regs[i].ftr_bits[j].type !=FTR_END ; j++) test_cnt++;ksft_set_plan (test_cnt); +/* Do this first in case a break interferes with other tests */ +test_invalid_configs (); +test_vm_ftr_id_regs (vcpu, aarch64_only);test_vcpu_ftr_id_regs (vcpu);test_vcpu_non_ftr_id_regs (vcpu);
Signature · 1 line
2.47.3
Re: [PATCH v21 10/15] KVM: selftests: arm64: Check that invalid feature combinations are rejected
Quoted 9 lines · Mark Brown
In order to optimise fast paths KVM explicitly rejects configurations with S1PIE or S1POE but not TCR2, add coverage of this in the
set_id_regstest. We have a list of invalid configurations, for each of them we try to run a VM and fail the test if it succeeds. We do feature detection by validating that we can write the fields with failing values.Since this misfiring can disrupt some of the other tests due to the kernel refusing to start guests we run the new tests first, improving diagnostics in the failing case.
Ah so this reflects the changes done to enforce the arch-valid s1pie/poe -> tcr requirement it seems.
Quoted 2 lines
Signed-off-by Mark Brown <[email protected]>
Everything seems sensible, so:
Quoted 83 lines
--- tools/testing/selftests/kvm/arm64/set_id_regs.c | 87 +++++++++++++++++++++++++ 1 file changed, 87 insertions(+)tools/testing/selftests/kvm/arm64/set_id_regs.c +71
-0@@ -803,6 +803,89 @@ static void test_reset_preserves_id_regs(struct kvm_vcpu *vcpu)ksft_test_result_pass ("%s\n" , __func__); } +struct reg_ftr_val { +u64 reg; +u64 mask; +u64 val; +}; + +#define REG_FTR_VAL (r, f, v) \ + { .reg = ARM64_SYS_REG(sys_reg_Op0(SYS_ ## r), \ + sys_reg_Op1(SYS_ ## r), \ + sys_reg_CRn(SYS_ ## r), \ + sys_reg_CRm(SYS_ ## r), \ + sys_reg_Op2(SYS_ ## r)), \ + .mask = r ## _ ## f ## _MASK, \ + .val = (r ## _ ## f ## _ ## v << r ## _ ## f ## _SHIFT) } + +static const struct reg_ftr_val s1pie_no_tcr2[] = { +REG_FTR_VAL (ID_AA64MMFR3_EL1 ,TCRX ,NI ), +REG_FTR_VAL (ID_AA64MMFR3_EL1 ,S1PIE ,IMP ), + { } +}; + +static const struct reg_ftr_val s1poe_no_tcr2[] = { +REG_FTR_VAL (ID_AA64MMFR3_EL1 ,TCRX ,NI ), +REG_FTR_VAL (ID_AA64MMFR3_EL1 ,S1POE ,IMP ), + { } +}; + +struct ftr_config { +const char *name; +const struct reg_ftr_val *regs; +}; + +static const struct ftr_config invalid_configs[] = { + { .name ="S1PIE without TCRX" , .regs = s1pie_no_tcr2 }, + { .name ="S1POE without TCRX" , .regs = s1poe_no_tcr2 }, +}; + +static void test_invalid_config (const struct ftr_config *config) +{ +struct kvm_vcpu *vcpu; +struct kvm_vm *vm; +const struct reg_ftr_val *field; +u64 val; +int ret; + + vm =vm_create (1 ); +vm_enable_cap (vm,KVM_CAP_ARM_WRITABLE_IMP_ID_REGS ,0 ); + vcpu =vm_vcpu_add (vm,0 , guest_code); +kvm_arch_vm_finalize_vcpus (vm); + +/* +* If we don't manage to set any of the fields assume the +* system does not support the feature and skip the test. +*/ +for (field = config->regs; field->reg; field++) { + val =vcpu_get_reg (vcpu, field->reg); + val &= ~field->mask; + val |= field->val; +__vcpu_set_reg (vcpu, field->reg, val); + +if (vcpu_get_reg (vcpu, field->reg) != val) { +ksft_print_msg ("Test setup not supported\n" ); +ksft_test_result_skip ("refuse %s\n" , config->name); + goto out; + } + } + + ret =_vcpu_run (vcpu); +ksft_test_result (ret <0 && errno ==EINVAL ,"refuse %s\n" , + config->name); +out:
tools/testing/selftests/kvm/arm64/set_id_regs.c static void test_reset_preserves_id_regs(struct kvm_vcpu *vcpu) · line 886
Quoted 10 lines
tools/testing/selftests/kvm/arm64/set_id_regs.c +10
-0+kvm_vm_free (vm); +} + +static void test_invalid_configs (void ) +{ +int i; + +for (i =0 ; i <ARRAY_SIZE (invalid_configs); i++) { +test_invalid_config (&invalid_configs[i]); + }
VERY NITTY: This is insanely pedantic and I don't mind too much _really_ but in theory should be no {}'s :)
Quoted 24 lines
tools/testing/selftests/kvm/arm64/set_id_regs.c +6
-1+} +int main (void ) {struct kvm_vcpu *vcpu;@@ -829,12 +912,16 @@ int main(void)ksft_print_header (); test_cnt =3 +MPAM_IDREG_TEST +MTE_IDREG_TEST ; + test_cnt +=ARRAY_SIZE (invalid_configs);for (i =0 ; i <ARRAY_SIZE (test_regs); i++)for (j =0 ; test_regs[i].ftr_bits[j].type !=FTR_END ; j++) test_cnt++;ksft_set_plan (test_cnt); +/* Do this first in case a break interferes with other tests */ +test_invalid_configs (); +test_vm_ftr_id_regs (vcpu, aarch64_only);test_vcpu_ftr_id_regs (vcpu);test_vcpu_non_ftr_id_regs (vcpu); --
Quoted 3 lines
2.47.3
--
Cheers, Lorenzo
PATCH v21 11/15 KVM: selftests: arm64: Add GCS registers to get-reg-list
In reply to Mark BrownGCS adds new registers GCSCR_ELx, GCSCRE0_EL1 and GCSPR_ELx. Add these to those validated by get-reg-list.
--- tools/testing/selftests/kvm/arm64/get-reg-list.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
tools/testing/selftests/kvm/arm64/get-reg-list.c +12 -0
@@ -42,6 +42,12 @@ struct feature_id_reg {static struct feature_id_reg feat_id_regs[] = {REG_FEAT (TCR2_EL1 ,ID_AA64MMFR3_EL1 ,TCRX ,IMP ),REG_FEAT (TCR2_EL2 ,ID_AA64MMFR3_EL1 ,TCRX ,IMP ), +REG_FEAT (GCSPR_EL0 ,ID_AA64PFR1_EL1 ,GCS ,IMP ), +REG_FEAT (GCSPR_EL1 ,ID_AA64PFR1_EL1 ,GCS ,IMP ), +REG_FEAT (GCSPR_EL2 ,ID_AA64PFR1_EL1 ,GCS ,IMP ), +REG_FEAT (GCSCRE0_EL1 ,ID_AA64PFR1_EL1 ,GCS ,IMP ), +REG_FEAT (GCSCR_EL1 ,ID_AA64PFR1_EL1 ,GCS ,IMP ), +REG_FEAT (GCSCR_EL2 ,ID_AA64PFR1_EL1 ,GCS ,IMP ),REG_FEAT (PIRE0_EL1 ,ID_AA64MMFR3_EL1 ,S1PIE ,IMP ),REG_FEAT (PIRE0_EL2 ,ID_AA64MMFR3_EL1 ,S1PIE ,IMP ),REG_FEAT (PIR_EL1 ,ID_AA64MMFR3_EL1 ,S1PIE ,IMP ),
@@ -503,6 +509,9 @@ static __u64 base_regs[] = {ARM64_SYS_REG (3 ,0 ,2 ,0 ,1 ),/* TTBR1_EL1 */ ARM64_SYS_REG (3 ,0 ,2 ,0 ,2 ),/* TCR_EL1 */ ARM64_SYS_REG (3 ,0 ,2 ,0 ,3 ),/* TCR2_EL1 */ +ARM64_SYS_REG (3 ,0 ,2 ,5 ,0 ),/* GCSCR_EL1 */ +ARM64_SYS_REG (3 ,0 ,2 ,5 ,1 ),/* GCSPR_EL1 */ +ARM64_SYS_REG (3 ,0 ,2 ,5 ,2 ),/* GCSCRE0_EL1 */ ARM64_SYS_REG (3 ,0 ,5 ,1 ,0 ),/* AFSR0_EL1 */ ARM64_SYS_REG (3 ,0 ,5 ,1 ,1 ),/* AFSR1_EL1 */ ARM64_SYS_REG(3, 0, 5, 2, 0), /* ESR_EL1 */
@@ -519,6 +528,7 @@ static __u64 base_regs[] = {ARM64_SYS_REG (3 ,0 ,13 ,0 ,4 ),/* TPIDR_EL1 */ ARM64_SYS_REG (3 ,0 ,14 ,1 ,0 ),/* CNTKCTL_EL1 */ ARM64_SYS_REG (3 ,2 ,0 ,0 ,0 ),/* CSSELR_EL1 */ +ARM64_SYS_REG (3 ,3 ,2 ,5 ,1 ),/* GCSPR_EL0 */ ARM64_SYS_REG (3 ,3 ,10 ,2 ,4 ),/* POR_EL0 */ ARM64_SYS_REG (3 ,3 ,13 ,0 ,2 ),/* TPIDR_EL0 */ ARM64_SYS_REG(3, 3, 13, 0, 3), /* TPIDRRO_EL0 */
@@ -740,6 +750,8 @@ static __u64 el2_regs[] = {SYS_REG (VTTBR_EL2 ),SYS_REG (VTCR_EL2 ),SYS_REG (VNCR_EL2 ), +SYS_REG (GCSCR_EL2 ), +SYS_REG (GCSPR_EL2 ),SYS_REG (HDFGRTR2_EL2 ),SYS_REG (HDFGWTR2_EL2 ), SYS_REG(HFGRTR2_EL2),
Signature · 1 line
2.47.3
Re: [PATCH v21 11/15] KVM: selftests: arm64: Add GCS registers to get-reg-list
Quoted 5 lines · Mark Brown
GCS adds new registers
GCSCR_ELx,GCSCRE0_EL1andGCSPR_ELx. Add these to those validated by get-reg-list.Reviewed-by Thiago Jung Bauermann <[email protected]>Signed-off-by Mark Brown <[email protected]>
This looks obviously correct (TM) to me so:
--
Cheers, Lorenzo
PATCH v21 12/15 KVM: selftests: arm64: Add GCS to set_id_regs
In reply to Mark BrownNow that GCS is configurable via the ID registers update set_id_regs to cover ID_AA64PFR1_EL1.GCS.
--- tools/testing/selftests/kvm/arm64/set_id_regs.c | 8 ++++++++ 1 file changed, 8 insertions(+)
tools/testing/selftests/kvm/arm64/set_id_regs.c +8 -0
@@ -154,6 +154,7 @@ static const struct reg_ftr_bits ftr_id_aa64pfr0_el1[] = {static const struct reg_ftr_bits ftr_id_aa64pfr1_el1[] = {REG_FTR_BITS (FTR_LOWER_SAFE ,ID_AA64PFR1_EL1 ,DF2 ,0 ), +REG_FTR_BITS (FTR_LOWER_SAFE ,ID_AA64PFR1_EL1 ,GCS ,0 ),REG_FTR_BITS (FTR_LOWER_SAFE ,ID_AA64PFR1_EL1 , CSV2_frac,0 ),REG_FTR_BITS (FTR_LOWER_SAFE ,ID_AA64PFR1_EL1 ,SSBS ,ID_AA64PFR1_EL1_SSBS_NI ),REG_FTR_BITS (FTR_LOWER_SAFE ,ID_AA64PFR1_EL1 ,BT ,0 ),
@@ -818,6 +819,12 @@ struct reg_ftr_val { .mask = r ## _ ## f ## _MASK, \ .val = (r ## _ ## f ## _ ## v << r ## _ ## f ## _SHIFT) } +static const struct reg_ftr_val gcs_no_s1pie[] = { +REG_FTR_VAL (ID_AA64PFR1_EL1 ,GCS ,IMP ), +REG_FTR_VAL (ID_AA64MMFR3_EL1 ,S1PIE ,NI ), + { } +}; +static const struct reg_ftr_val s1pie_no_tcr2[] = {REG_FTR_VAL (ID_AA64MMFR3_EL1 ,TCRX ,NI ),REG_FTR_VAL (ID_AA64MMFR3_EL1 ,S1PIE ,IMP ),
@@ -836,6 +843,7 @@ struct ftr_config { };static const struct ftr_config invalid_configs[] = { + { .name ="GCS without S1PIE" , .regs = gcs_no_s1pie }, { .name ="S1PIE without TCRX" , .regs = s1pie_no_tcr2 }, { .name ="S1POE without TCRX" , .regs = s1poe_no_tcr2 }, };
Signature · 1 line
2.47.3
Re: [PATCH v21 12/15] KVM: selftests: arm64: Add GCS to set_id_regs
Quoted 2 lines · Mark Brown
Now that GCS is configurable via the ID registers update
set_id_regsto cover ID_AA64PFR1_EL1.GCS.
Should maybe also mention adding the GCS-without-S1PIE test?
Quoted 2 lines
Signed-off-by Mark Brown <[email protected]>
Looks obviously correct (TM) to me too so:
Quoted 39 lines
--- tools/testing/selftests/kvm/arm64/set_id_regs.c | 8 ++++++++ 1 file changed, 8 insertions(+)tools/testing/selftests/kvm/arm64/set_id_regs.c +8
-1@@ -154,6 +154,7 @@ static const struct reg_ftr_bits ftr_id_aa64pfr0_el1[] = {static const struct reg_ftr_bits ftr_id_aa64pfr1_el1[] = {REG_FTR_BITS (FTR_LOWER_SAFE ,ID_AA64PFR1_EL1 ,DF2 ,0 ), +REG_FTR_BITS (FTR_LOWER_SAFE ,ID_AA64PFR1_EL1 ,GCS ,0 ),REG_FTR_BITS (FTR_LOWER_SAFE ,ID_AA64PFR1_EL1 , CSV2_frac,0 ),REG_FTR_BITS (FTR_LOWER_SAFE ,ID_AA64PFR1_EL1 ,SSBS ,ID_AA64PFR1_EL1_SSBS_NI ),REG_FTR_BITS (FTR_LOWER_SAFE ,ID_AA64PFR1_EL1 ,BT ,0 ),@@ -818,6 +819,12 @@ struct reg_ftr_val { .mask = r ## _ ## f ## _MASK, \ .val = (r ## _ ## f ## _ ## v << r ## _ ## f ## _SHIFT) } +static const struct reg_ftr_val gcs_no_s1pie[] = { +REG_FTR_VAL (ID_AA64PFR1_EL1 ,GCS ,IMP ), +REG_FTR_VAL (ID_AA64MMFR3_EL1 ,S1PIE ,NI ), + { } +}; +static const struct reg_ftr_val s1pie_no_tcr2[] = {REG_FTR_VAL (ID_AA64MMFR3_EL1 ,TCRX ,NI ),REG_FTR_VAL (ID_AA64MMFR3_EL1 ,S1PIE ,IMP ),@@ -836,6 +843,7 @@ struct ftr_config { };static const struct ftr_config invalid_configs[] = { + { .name ="GCS without S1PIE" , .regs = gcs_no_s1pie }, { .name ="S1PIE without TCRX" , .regs = s1pie_no_tcr2 }, { .name ="S1POE without TCRX" , .regs = s1poe_no_tcr2 }, }; --
Quoted 3 lines
2.47.3
--
Cheers, Lorenzo
Re: [PATCH v21 12/15] KVM: selftests: arm64: Add GCS to set_id_regs
Quoted 6 lines · Lorenzo Stoakes (ARM)
Quoted 2 lines · Mark Brown
Now that GCS is configurable via the ID registers update
set_id_regsto cover ID_AA64PFR1_EL1.GCS.Should maybe also mention adding the GCS-without-S1PIE test?
I'd have said that was part of the coverage the test now has for bitfields TBH.
Re: [PATCH v21 12/15] KVM: selftests: arm64: Add GCS to set_id_regs
Quoted 10 lines · Mark Brown
Quoted 6 lines · Lorenzo Stoakes (ARM)
Quoted 2 lines · Mark Brown
Now that GCS is configurable via the ID registers update
set_id_regsto cover ID_AA64PFR1_EL1.GCS.Should maybe also mention adding the GCS-without-S1PIE test?
I'd have said that was part of the coverage the test now has for bitfields TBH.
Fair enough, all LGTM at any rate! :)
--
Cheers, Lorenzo
PATCH v21 13/15 KVM: selftests: arm64: Only restore SPSR_EL1 and ELR_EL1 if they change
In reply to Mark BrownWhen EXLOCK is enabled for ELx writes to SPSR_ELx and ELR_ELx generate GCS exceptions. Currently the selftest helpers unconditionally reload the saved values for these registers regardless of if the handler didn't change them. This gets in the way of EXLOCK testing, making it hard to generate contexts which will generate an invalid ERET since we never get as far as doing an exception return while EXLOCK is enabled.
In order to facilitate testing of nested virtualisation emulation of EXLOCK change our reloads into read/modify/write cycles, suppressing the writes when they would not change the value in the register.
--- tools/testing/selftests/kvm/lib/arm64/handlers.S | 10 ++++++++++ 1 file changed, 10 insertions(+)
tools/testing/selftests/kvm/lib/arm64/handlers.S +10 -0
@@ -33,8 +33,18 @@ .macro restore_registers ldp x1, x2, [sp, #16 * 16] /* PC, PSTATE */ + + /* Suppress redundant writes to support GCS exception lock testing */ + mrs x3, elr_el1 + cmp x1, x3 + beq .Lskip_elr_\@ msr elr_el1, x1 +.Lskip_elr_\@: + mrs x3, spsr_el1 + cmp x2, x3 + beq .Lskip_spsr_\@ msr spsr_el1, x2 +.Lskip_spsr_\@: /* sp is not restored */ ldp x30, xzr, [sp, #16 * 15] /* x30, SP */
Signature · 1 line
2.47.3
Re: [PATCH v21 13/15] KVM: selftests: arm64: Only restore SPSR_EL1 and ELR_EL1 if they change
Quoted 6 lines · Mark Brown
When EXLOCK is enabled for ELx writes to
SPSR_ELxandELR_ELxgenerate GCS exceptions. Currently the selftest helpers unconditionally reload the saved values for these registers regardless of if the handler didn't change them. This gets in the way of EXLOCK testing, making it hard to generate contexts which will generate an invalid ERET since we never get as far as doing an exception return while EXLOCK is enabled.
Sounds reasonable.
Quoted 4 lines
In order to facilitate testing of nested virtualisation emulation of EXLOCK change our reloads into read/modify/write cycles, suppressing the writes when they would not change the value in the register.
Also sounds sensible.
Quoted 3 lines
Reviewed-by Leonardo Bras <[email protected]>Signed-off-by Mark Brown <[email protected]>
Also looks obviously correct (TM) to me so:
Quoted 12 lines
--- tools/testing/selftests/kvm/lib/arm64/handlers.S | 10 ++++++++++ 1 file changed, 10 insertions(+)tools/testing/selftests/kvm/lib/arm64/handlers.S +0
-0@@ -33,8 +33,18 @@ .macro restore_registers ldp x1, x2, [sp, #16 * 16] /* PC, PSTATE */
tools/testing/selftests/kvm/lib/arm64/handlers.S line 45
Quoted 10 lines
tools/testing/selftests/kvm/lib/arm64/handlers.S +9
-0+ + /* Suppress redundant writes to support GCS exception lock testing */ + mrs x3, elr_el1 + cmp x1, x3 + beq .Lskip_elr_\@ msr elr_el1, x1 +.Lskip_elr_\@: + mrs x3, spsr_el1 + cmp x2, x3 + beq .Lskip_spsr_\@
Even as an arm64 assembly novice I can see what this is doing :)
Quoted 10 lines
tools/testing/selftests/kvm/lib/arm64/handlers.S +1
-1msr spsr_el1, x2 +.Lskip_spsr_\@: /* sp is not restored */ ldp x30, xzr, [sp, #16 * 15] /* x30, SP */ --2.47.3
--
Cheers, Lorenzo
PATCH v21 14/15 tools: Synchronise the kernel esr.h
In reply to Mark BrownTo pick up definitions for GCS.
--- tools/arch/arm64/include/asm/esr.h | 113 +++++++++++++++++++++++++++++++++---- 1 file changed, 102 insertions(+), 11 deletions(-)
tools/arch/arm64/include/asm/esr.h +102 -11
@@ -19,7 +19,8 @@#define ESR_ELx_EC_FP_ASIMD UL(0x07)#define ESR_ELx_EC_CP10_ID UL(0x08)/* EL2 only */ #define ESR_ELx_EC_PAC UL(0x09)/* EL2 and above */ -/* Unallocated EC: 0x0A - 0x0B */ +#define ESR_ELx_EC_OTHER UL(0x0A) +/* Unallocated EC: 0x0B */ #define ESR_ELx_EC_CP14_64 UL(0x0C)#define ESR_ELx_EC_BTI UL(0x0D)#define ESR_ELx_EC_ILL UL(0x0E)
@@ -50,7 +51,8 @@#define ESR_ELx_EC_FP_EXC32 UL(0x28)/* Unallocated EC: 0x29 - 0x2B */ #define ESR_ELx_EC_FP_EXC64 UL(0x2C) -/* Unallocated EC: 0x2D - 0x2E */ +#define ESR_ELx_EC_GCS UL(0x2D) +/* Unallocated EC: 0x2E */ #define ESR_ELx_EC_SERROR UL(0x2F)#define ESR_ELx_EC_BREAKPT_LOW UL(0x30)#define ESR_ELx_EC_BREAKPT_CUR UL(0x31)
@@ -97,6 +99,8 @@#define ESR_ELx_AET_CE (UL(6) << ESR_ELx_AET_SHIFT)/* Shared ISS field definitions for Data/Instruction aborts */ +#define ESR_ELx_VNCR_SHIFT (13) +#define ESR_ELx_VNCR (UL(1) << ESR_ELx_VNCR_SHIFT)#define ESR_ELx_SET_SHIFT (11)#define ESR_ELx_SET_MASK (UL(3) << ESR_ELx_SET_SHIFT)#define ESR_ELx_FnV_SHIFT (10)
@@ -119,6 +123,16 @@#define ESR_ELx_FSC_SEA_TTW (n) (0x14 + (n))#define ESR_ELx_FSC_SECC (0x18)#define ESR_ELx_FSC_SECC_TTW (n) (0x1c + (n)) +#define ESR_ELx_FSC_EXCL_ATOMIC (0x35) +#define ESR_ELx_FSC_ADDRSZ (0x00) + +/* +* Annoyingly, the negative levels for Address size faults aren't laid out +* contiguously (or in the desired order) +*/ +#define ESR_ELx_FSC_ADDRSZ_nL (n) ((n) == -1 ? 0x25 : 0x2C) +#define ESR_ELx_FSC_ADDRSZ_L (n) ((n) < 0 ? ESR_ELx_FSC_ADDRSZ_nL(n) : \ + (ESR_ELx_FSC_ADDRSZ + (n)))/* Status codes for individual page table levels */ #define ESR_ELx_FSC_ACCESS_L (n) (ESR_ELx_FSC_ACCESS + (n))
@@ -141,12 +155,12 @@#define ESR_ELx_SF (UL(1) << ESR_ELx_SF_SHIFT)#define ESR_ELx_AR_SHIFT (14)#define ESR_ELx_AR (UL(1) << ESR_ELx_AR_SHIFT) -#define ESR_ELx_VNCR_SHIFT (13) -#define ESR_ELx_VNCR (UL(1) << ESR_ELx_VNCR_SHIFT)#define ESR_ELx_CM_SHIFT (8)#define ESR_ELx_CM (UL(1) << ESR_ELx_CM_SHIFT)/* ISS2 field definitions for Data Aborts */ +#define ESR_ELx_HDBSSF_SHIFT (11) +#define ESR_ELx_HDBSSF (UL(1) << ESR_ELx_HDBSSF_SHIFT)#define ESR_ELx_TnD_SHIFT (10)#define ESR_ELx_TnD (UL(1) << ESR_ELx_TnD_SHIFT)#define ESR_ELx_TagAccess_SHIFT (9)
@@ -161,8 +175,6 @@#define ESR_ELx_Xs_MASK (GENMASK_ULL(4, 0))/* ISS field definitions for exceptions taken in to Hyp */ -#define ESR_ELx_FSC_ADDRSZ (0x00) -#define ESR_ELx_FSC_ADDRSZ_L (n) (ESR_ELx_FSC_ADDRSZ + (n))#define ESR_ELx_CV (UL(1) << 24)#define ESR_ELx_COND_SHIFT (20)#define ESR_ELx_COND_MASK (UL(0xF) << ESR_ELx_COND_SHIFT)
@@ -174,6 +186,13 @@#define ESR_ELx_WFx_ISS_WFE (UL(1) << 0)#define ESR_ELx_xVC_IMM_MASK ((UL(1) << 16) - 1) +/* ISS definitions for LD64B/ST64B/{T,P}SBCSYNC instructions */ +#define ESR_ELx_ISS_OTHER_ST64BV (0) +#define ESR_ELx_ISS_OTHER_ST64BV0 (1) +#define ESR_ELx_ISS_OTHER_LDST64B (2) +#define ESR_ELx_ISS_OTHER_TSBCSYNC (3) +#define ESR_ELx_ISS_OTHER_PSBCSYNC (4) +#define DISR_EL1_IDS (UL(1) << 24)/* * DISR_EL1 and ESR_ELx share the bottom 13 bits, but the RES0 bits may mean
@@ -371,12 +390,14 @@/* * ISS values for SME traps */ +#define ESR_ELx_SME_ISS_SMTC_MASK GENMASK(2, 0) +#define ESR_ELx_SME_ISS_SMTC (esr) ((esr) & ESR_ELx_SME_ISS_SMTC_MASK) -#define ESR_ELx_SME_ISS_SME_DISABLED 0 -#define ESR_ELx_SME_ISS_ILL 1 -#define ESR_ELx_SME_ISS_SM_DISABLED 2 -#define ESR_ELx_SME_ISS_ZA_DISABLED 3 -#define ESR_ELx_SME_ISS_ZT_DISABLED 4 +#define ESR_ELx_SME_ISS_SMTC_SME_DISABLED 0 +#define ESR_ELx_SME_ISS_SMTC_ILL 1 +#define ESR_ELx_SME_ISS_SMTC_SM_DISABLED 2 +#define ESR_ELx_SME_ISS_SMTC_ZA_DISABLED 3 +#define ESR_ELx_SME_ISS_SMTC_ZT_DISABLED 4/* ISS field definitions for MOPS exceptions */ #define ESR_ELx_MOPS_ISS_MEM_INST (UL(1) << 24)
@@ -387,6 +408,31 @@#define ESR_ELx_MOPS_ISS_SRCREG (esr) (((esr) & (UL(0x1f) << 5)) >> 5)#define ESR_ELx_MOPS_ISS_SIZEREG (esr) (((esr) & (UL(0x1f) << 0)) >> 0) +/* ISS field definitions for GCS */ +#define ESR_ELx_ExType_SHIFT (20) +#define ESR_ELx_ExType_MASK GENMASK(23, 20) +#define ESR_ELx_Raddr_SHIFT (10) +#define ESR_ELx_Raddr_MASK GENMASK(14, 10) +#define ESR_ELx_Rn_SHIFT (5) +#define ESR_ELx_Rn_MASK GENMASK(9, 5) +#define ESR_ELx_Rvalue_SHIFT 5 +#define ESR_ELx_Rvalue_MASK GENMASK(9, 5) +#define ESR_ELx_IT_SHIFT (0) +#define ESR_ELx_IT_MASK GENMASK(4, 0) + +#define ESR_ELx_ExType_DATA_CHECK 0 +#define ESR_ELx_ExType_EXLOCK 1 +#define ESR_ELx_ExType_STR 2 + +#define ESR_ELx_IT_RET 0 +#define ESR_ELx_IT_GCSPOPM 1 +#define ESR_ELx_IT_RET_KEYA 2 +#define ESR_ELx_IT_RET_KEYB 3 +#define ESR_ELx_IT_GCSSS1 4 +#define ESR_ELx_IT_GCSSS2 5 +#define ESR_ELx_IT_GCSPOPCX 6 +#define ESR_ELx_IT_GCSPOPX 7 +#ifndef __ASSEMBLER__#include <asm/types.h>
@@ -408,6 +454,11 @@ static inline bool esr_is_cfi_brk(unsigned long esr) (esr_brk_comment (esr) & ~CFI_BRK_IMM_MASK ) ==CFI_BRK_IMM_BASE ; } +static inline bool esr_is_ubsan_brk (unsigned long esr) +{ +return (esr_brk_comment (esr) & ~UBSAN_BRK_MASK ) ==UBSAN_BRK_IMM ; +} +static inline bool esr_fsc_is_translation_fault (unsigned long esr) { esr = esr & ESR_ELx_FSC;
@@ -439,6 +490,46 @@ static inline bool esr_fsc_is_access_flag_fault(unsigned long esr) (esr ==ESR_ELx_FSC_ACCESS_L (0 )); } +static inline bool esr_fsc_is_excl_atomic_fault (unsigned long esr) +{ + esr = esr & ESR_ELx_FSC; + +return esr == ESR_ELx_FSC_EXCL_ATOMIC; +} + +static inline bool esr_fsc_is_addr_sz_fault (unsigned long esr) +{ + esr &= ESR_ELx_FSC; + +return (esr ==ESR_ELx_FSC_ADDRSZ_L (3 )) || + (esr ==ESR_ELx_FSC_ADDRSZ_L (2 )) || + (esr ==ESR_ELx_FSC_ADDRSZ_L (1 )) || + (esr ==ESR_ELx_FSC_ADDRSZ_L (0 )) || + (esr ==ESR_ELx_FSC_ADDRSZ_L (-1 )); +} + +static inline bool esr_fsc_is_sea_ttw (unsigned long esr) +{ + esr = esr & ESR_ELx_FSC; + +return (esr ==ESR_ELx_FSC_SEA_TTW (3 )) || + (esr ==ESR_ELx_FSC_SEA_TTW (2 )) || + (esr ==ESR_ELx_FSC_SEA_TTW (1 )) || + (esr ==ESR_ELx_FSC_SEA_TTW (0 )) || + (esr ==ESR_ELx_FSC_SEA_TTW (-1 )); +} + +static inline bool esr_fsc_is_secc_ttw (unsigned long esr) +{ + esr = esr & ESR_ELx_FSC; + +return (esr ==ESR_ELx_FSC_SECC_TTW (3 )) || + (esr ==ESR_ELx_FSC_SECC_TTW (2 )) || + (esr ==ESR_ELx_FSC_SECC_TTW (1 )) || + (esr ==ESR_ELx_FSC_SECC_TTW (0 )) || + (esr ==ESR_ELx_FSC_SECC_TTW (-1 )); +} +/* Indicate whether ESR.EC==0x1A is for an ERETAx instruction */ static inline bool esr_iss_is_eretax (unsigned long esr) {
Signature · 1 line
2.47.3
[email protected]
AI reviewRe: [PATCH v21 14/15] tools: Synchronise the kernel esr.h
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] tools: incorrect mapping of ESR_ELx_FSC_ADDRSZ_nL for level -1 --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] tools: incorrect mapping of ESR_ELx_FSC_ADDRSZ_nL for level -1 --
commit 29c713adc7b9f4531e116d3021bff33ab6d602a4
Author: Mark Brown <[email protected]>
tools: Synchronise the kernel esr.h
This commit synchronizes the tools version of the arm64 esr.h header with the main kernel header to pick up definitions for GCS.
Quoted 4 lines
tools/arch/arm64/include/asm/esr.h +0
-0
[ ... ]
tools/arch/arm64/include/asm/esr.h line 133
Quoted 12 lines
tools/arch/arm64/include/asm/esr.h +8
-0@@ -119,6 +123,16 @@#define ESR_ELx_FSC_SEA_TTW (n) (0x14 + (n))#define ESR_ELx_FSC_SECC (0x18)#define ESR_ELx_FSC_SECC_TTW (n) (0x1c + (n)) +#define ESR_ELx_FSC_EXCL_ATOMIC (0x35) +#define ESR_ELx_FSC_ADDRSZ (0x00) + +/* +* Annoyingly, the negative levels for Address size faults aren't laid out +* contiguously (or in the desired order) +*/ +#define ESR_ELx_FSC_ADDRSZ_nL (n) ((n) == -1 ? 0x25 : 0x2C)
[Severity: Medium]
Does this ESR_ELx_FSC_ADDRSZ_nL macro incorrectly map level -1 Address Size faults to 0x25 instead of 0x29?
If tools/arch/arm64/include/asm/esr.h is meant to be synchronized with the kernel header, diverging here causes user-space tools (such as KVM selftests or perf) to misdecode or incorrectly inject Address Size faults for level -1. Any tool compiling against this header and invoking ESR_ELx_FSC_ADDRSZ_L(-1) would receive 0x25 rather than the correct hardware encoding 0x29.
Quoted 5 lines
tools/arch/arm64/include/asm/esr.h +2
-0+#define ESR_ELx_FSC_ADDRSZ_L (n) ((n) < 0 ? ESR_ELx_FSC_ADDRSZ_nL(n) : \ + (ESR_ELx_FSC_ADDRSZ + (n)))/* Status codes for individual page table levels */ #define ESR_ELx_FSC_ACCESS_L (n) (ESR_ELx_FSC_ACCESS + (n))
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=14
Re: [PATCH v21 14/15] tools: Synchronise the kernel esr.h
In reply to Mark BrownQuoted 4 lines · Mark Brown
To pick up definitions for GCS.
Reviewed-by Leonardo Bras <[email protected]>Signed-off-by Mark Brown <[email protected]>
This seems to be out of date, with your series applied to next:
$ diff -u arch/arm64/include/asm/esr.h tools/arch/arm64/include/asm/esr.h --- arch/arm64/include/asm/esr.h 2026-10-01 17:43:56.186852562 +0100 +++ tools/arch/arm64/include/asm/esr.h 2026-10-01 17:43:56.194056154 +0100 @@ -7,7 +7,6 @@ #ifndef __ASM_ESR_H #define __ASM_ESR_H
-#include <asm/memory.h>
#include <asm/sysreg.h>
#define ESR_ELx_EC_UNKNOWN UL(0x00)
@@ -131,7 +130,7 @@
* Annoyingly, the negative levels for Address size faults aren't laid out
* contiguously (or in the desired order)
*/
-#define ESR_ELx_FSC_ADDRSZ_nL(n) ((n) == -1 ? 0x29 : 0x2C)
+#define ESR_ELx_FSC_ADDRSZ_nL(n) ((n) == -1 ? 0x25 : 0x2C)
#define ESR_ELx_FSC_ADDRSZ_L(n) ((n) < 0 ? ESR_ELx_FSC_ADDRSZ_nL(n) : \
(ESR_ELx_FSC_ADDRSZ + (n)))
@@ -437,35 +436,6 @@
#ifndef __ASSEMBLER__
#include <asm/types.h>
-static __always_inline bool esr_trap_is_iabt(unsigned long esr) -{
- return ESR_ELx_EC(esr) == ESR_ELx_EC_IABT_LOW;
-}
-
-/* Always check for S1PTW *before* using this. */ -static __always_inline bool esr_dabt_is_write(unsigned long esr) -{
- return esr & ESR_ELx_WNR;
-}
-
-static __always_inline bool esr_dabt_is_cm(unsigned long esr) -{
- return esr & ESR_ELx_CM;
-}
-
-static __always_inline bool esr_abt_is_sea(unsigned long esr) -{
- switch (esr & ESR_ELx_FSC) {
- case ESR_ELx_FSC_EXTABT:
- case ESR_ELx_FSC_SEA_TTW(-1) ... ESR_ELx_FSC_SEA_TTW(3):
- case ESR_ELx_FSC_SECC:
- case ESR_ELx_FSC_SECC_TTW(-1) ... ESR_ELx_FSC_SECC_TTW(3):
- return true;
- default:
- return false;
- }
-}
-
static inline unsigned long esr_brk_comment(unsigned long esr) {
return esr & ESR_ELx_BRK64_ISS_COMMENT_MASK;
--
Cheers, Lorenzo
Re: [PATCH v21 14/15] tools: Synchronise the kernel esr.h
Quoted 7 lines · Lorenzo Stoakes (ARM)
Quoted 4 lines · Mark Brown
To pick up definitions for GCS.
Reviewed-by Leonardo Bras <[email protected]>Signed-off-by Mark Brown <[email protected]>This seems to be out of date, with your series applied to next:
Yeah, though some of those are changes that are in -next only so possibly slightly premature. I'll resync with mainline, really this shouldn't be a manual sync :/
Re: [PATCH v21 14/15] tools: Synchronise the kernel esr.h
Quoted 4 lines · Mark Brown
Quoted 3 lines · Lorenzo Stoakes (ARM)
Quoted 2 lines · Mark Brown
To pick up definitions for GCS.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
Reviewed-by Leonardo Bras <[email protected]>Signed-off-by Mark Brown <[email protected]>This seems to be out of date, with your series applied to next:
Yeah, though some of those are changes that are in -next only so possibly slightly premature. I'll resync with mainline, really this shouldn't be a manual sync :/
Thanks! + Yeah agreed, it's definitely a pain.
--
Cheers, Lorenzo
PATCH v21 15/15 KVM: selftests: arm64: Add GCS EXLOCK exception emulation test
In reply to Mark BrownWhen running in vEL2 ERET is emulated, this means that we must emulate the behaviour of GCSCR_EL2.EXLOCKEN. Add a test that validates the behaviour.
As per DDI0487 R_WTXBY PSTATE.EXLOCK is set to the value of GCSCR_ELx.EXLOCKEN when taking an exception to the same exception level. As per DDI0487 R_TYTWB if on exception return to the same EL GCSCR_ELx.EXLOCKEN is 1 and PSTATE.EXLOCK is not 1 then the exception return is illegal.
We nest handling of two UDFs to allow us to both observe and modify PSTATE.EXLOCK, checking that expected exceptions are generated for ERET. We also repeat the UDF tests having enabled fine grained traps for ERET to try to ensure that the slow paths are taken.
We also attempt to write to both SPSR_ELx and ELR_ELx, again using a UDF handler to control PSTATE.EXLOCK. We do all this both with and without having enabled EXLOCKEN, though only for code running at EL2 due to framework limitations.
For completeness we also inject a data abort from EL0 to EL2 with GCSCR_EL2.EXLOCKEN set, ensuring that PSTATE.EXLOCK is not set.
--- tools/testing/selftests/kvm/Makefile.kvm | 1 + tools/testing/selftests/kvm/arm64/gcs_exceptions.c | 336 +++++++++++++++++++++ 2 files changed, 337 insertions(+)
tools/testing/selftests/kvm/Makefile.kvm +1 -0
@@ -179,6 +179,7 @@ TEST_GEN_PROGS_arm64 += arm64/hello_el2 TEST_GEN_PROGS_arm64 += arm64/host_sve TEST_GEN_PROGS_arm64 += arm64/hypercalls TEST_GEN_PROGS_arm64 += arm64/external_aborts +TEST_GEN_PROGS_arm64 += arm64/gcs_exceptions TEST_GEN_PROGS_arm64 += arm64/mmio_sign_ext TEST_GEN_PROGS_arm64 += arm64/page_fault_test TEST_GEN_PROGS_arm64 += arm64/psci_test
tools/testing/selftests/kvm/arm64/gcs_exceptions.c new file +336 -0
336 lines not on this page · raw message
Signature · 1 line
2.47.3
[email protected]
AI reviewRe: [PATCH v21 15/15] KVM: selftests: arm64: Add GCS EXLOCK exception emulation test
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] kselftest/arm64: missing fine-grained trap tests mentioned in commit message --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] kselftest/arm64: missing fine-grained trap tests mentioned in commit message --
commit c63cb70f3a95d5f56914bee1163a8ae38fd9251f
Author: Mark Brown <[email protected]>
KVM: selftests: arm64: Add GCS EXLOCK exception emulation test
This commit adds a new selftest to validate the emulation of GCSCR_EL2.EXLOCKEN behavior during exception return (ERET) for KVM on arm64. It checks that expected exceptions are generated and verifies system register interactions.
Quoted 4 lines
We nest handling of two UDFs to allow us to both observe and modify PSTATE.EXLOCK, checking that expected exceptions are generated for ERET. We also repeat the UDF tests having enabled fine grained traps for ERET to try to ensure that the slow paths are taken.
[ ... ]
Quoted 3 lines
tools/testing/selftests/kvm/arm64/gcs_exceptions.c new file +0
-0
[ ... ]
Quoted 12 lines
tools/testing/selftests/kvm/arm64/gcs_exceptions.c new file +12
-0+static void guest_code (void ) +{ +GUEST_ASSERT_EQ (get_current_el (),2 ); + +/* +* Try straightforwardly generating an exception without and +* with EXLOCK. +*/ +test_udf_exception (false); +test_sysreg (UDF_ELR_EL1 , false); +test_sysreg (UDF_ELR_EL2 , false); +test_sysreg (UDF_SPSR_EL1 , false);
tools/testing/selftests/kvm/arm64/gcs_exceptions.c line 252
Quoted 10 lines
tools/testing/selftests/kvm/arm64/gcs_exceptions.c new file +10
-0+test_sysreg (UDF_SPSR_EL2 , false); + +test_udf_exception (true); +test_sysreg (UDF_ELR_EL1 , true); +test_sysreg (UDF_ELR_EL2 , true); +test_sysreg (UDF_SPSR_EL1 , true); +test_sysreg (UDF_SPSR_EL2 , true); + +GUEST_DONE (); +}
[Severity: Medium]
Is the fine-grained trap testing missing here? The commit message explicitly states that the UDF tests are repeated with fine-grained traps enabled for ERET to ensure slow paths are taken.
However, looking at the guest_code() implementation in tools/testing/selftests/kvm/arm64/gcs_exceptions.c, the tests are only executed with the enable_exlock parameter set to false and then true. There does not appear to be any logic to configure fine-grained trap registers (such as HFGITR_EL2) or an additional pass to repeat these UDF tests with those traps active.
Does the test need an update to match the commit message, or is this handled in a different way?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=15
Re: [PATCH v21 15/15] KVM: selftests: arm64: Add GCS EXLOCK exception emulation test
In reply to Mark BrownQuoted 20 lines · Mark Brown
When running in vEL2 ERET is emulated, this means that we must emulate the behaviour of GCSCR_EL2.EXLOCKEN. Add a test that validates the behaviour.
As per DDI0487
R_WTXBYPSTATE.EXLOCK is set to the value of GCSCR_ELx.EXLOCKEN when taking an exception to the same exception level. As per DDI0487R_TYTWBif on exception return to the same EL GCSCR_ELx.EXLOCKEN is 1 and PSTATE.EXLOCK is not 1 then the exception return is illegal.We nest handling of two UDFs to allow us to both observe and modify PSTATE.EXLOCK, checking that expected exceptions are generated for ERET. We also repeat the UDF tests having enabled fine grained traps for ERET to try to ensure that the slow paths are taken.
We also attempt to write to both
SPSR_ELxandELR_ELx, again using a UDF handler to control PSTATE.EXLOCK. We do all this both with and without having enabled EXLOCKEN, though only for code running at EL2 due to framework limitations.
Quoted 5 lines
For completeness we also inject a data abort from EL0 to EL2 with GCSCR_EL2.EXLOCKEN set, ensuring that PSTATE.EXLOCK is not set.
Signed-off-by Mark Brown <[email protected]>Reviewed-by Leonardo Bras <[email protected]>
This all seems reasonable test-wise, but I wonder if the discussion around 8/15 might alter some of what's tested here?
In that - if it's only hardware that's doing the locked check then the SPSR (and probably ELR?) bits might only be exercising that rather than KVM stuff?
It might only need a comment update though :)
--
Cheers, Lorenzo