On 24-11-2023 08:02 pm, Marc Zyngier wrote:
On Fri, 24 Nov 2023 13:22:22 +0000,
Ganapatrao Kulkarni <gankulkarni@xxxxxxxxxxxxxxxxxxxxxx> wrote:
How is this value possible if the write to HCR_EL2 has taken place?
When do you sample this?
I am not sure how and where it got set. I think, whatever it is set,
it is due to false return of vcpu_el2_e2h_is_set(). Need to
understand/debug.
The vhcr_el2 value I have shared is traced along with hcr in function
__activate_traps/__compute_hcr.
Here's my hunch:
The guest boots with E2H=0, because we don't advertise anything else
on your HW. So we run with NV1=1 until we try to *upgrade* to VHE. NV2
means that HCR_EL2 is writable (to memory) without a trap. But we're
still running with NV1=1.
Subsequently, we access a sysreg that should never trap for a VHE
guest, but we're with the wrong config. Bad things happen.
Unfortunately, NV2 is pretty much incompatible with E2H being updated,
because it cannot perform the changes that this would result into at
the point where they should happen. We can try and do a best effort
handling, but you can always trick it.
Anyway, can you see if the hack below helps? I'm not keen on it at
all, but this would be a good data point.
Thanks Marc, this diff fixes the issue.
Just wondering what is changed w.r.t to L1 handling from V10 to V11 that
it requires this trick?
Also why this was not seen on your platform, is it E2H0 enabled?
M.
From c4b856221661393b884cbf673d100faaa8dc018a Mon Sep 17 00:00:00 2001
From: Marc Zyngier <maz@xxxxxxxxxx>
Date: Fri, 26 May 2023 12:16:05 +0100
Subject: [PATCH] KVM: arm64: Opportunistically track HCR_EL2.E2H being flipped
Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
---
arch/arm64/include/asm/kvm_host.h | 9 +++++++--
arch/arm64/kvm/hyp/include/hyp/switch.h | 13 +++++++++++++
arch/arm64/kvm/hyp/vhe/switch.c | 10 ++++++++--
3 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index c91f607e989d..d45ef41de5fb 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -655,6 +655,9 @@ struct kvm_vcpu_arch {
/* State flags for kernel bookkeeping, unused by the hypervisor code */
u8 sflags;
+ /* Bookkeeping flags for NV */
+ u8 nvflags;
+
/*
* Don't run the guest (internal implementation need).
*
@@ -858,8 +861,6 @@ struct kvm_vcpu_arch {
#define DEBUG_STATE_SAVE_SPE __vcpu_single_flag(iflags, BIT(5))
/* Save TRBE context if active */
#define DEBUG_STATE_SAVE_TRBE __vcpu_single_flag(iflags, BIT(6))
-/* vcpu running in HYP context */
-#define VCPU_HYP_CONTEXT __vcpu_single_flag(iflags, BIT(7))
/* SVE enabled for host EL0 */
#define HOST_SVE_ENABLED __vcpu_single_flag(sflags, BIT(0))
@@ -878,6 +879,10 @@ struct kvm_vcpu_arch {
/* WFI instruction trapped */
#define IN_WFI __vcpu_single_flag(sflags, BIT(7))
+/* vcpu running in HYP context */
+#define VCPU_HYP_CONTEXT __vcpu_single_flag(nvflags, BIT(0))
+/* vcpu entered with HCR_EL2.E2H set */
+#define VCPU_HCR_E2H __vcpu_single_flag(nvflags, BIT(1))
/* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
#define vcpu_sve_pffr(vcpu) (kern_hyp_va((vcpu)->arch.sve_state) + \
diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
index aed2ea35082c..9c1346116d61 100644
--- a/arch/arm64/kvm/hyp/include/hyp/switch.h
+++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
@@ -669,6 +669,19 @@ static inline bool fixup_guest_exit(struct kvm_vcpu *vcpu, u64 *exit_code)
*/
synchronize_vcpu_pstate(vcpu, exit_code);
+ if (vcpu_has_nv(vcpu) &&
+ (!!vcpu_get_flag(vcpu, VCPU_HCR_E2H) ^ vcpu_el2_e2h_is_set(vcpu))) {
+ if (vcpu_el2_e2h_is_set(vcpu)) {
+ sysreg_clear_set(hcr_el2, HCR_NV1, 0);
+ vcpu_set_flag(vcpu, VCPU_HCR_E2H);
+ } else {
+ sysreg_clear_set(hcr_el2, 0, HCR_NV1);
+ vcpu_clear_flag(vcpu, VCPU_HCR_E2H);
+ }
+
+ return true;
+ }
+
/*
* Check whether we want to repaint the state one way or
* another.
diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c
index 8d1e9d1adabe..395aaa06f358 100644
--- a/arch/arm64/kvm/hyp/vhe/switch.c
+++ b/arch/arm64/kvm/hyp/vhe/switch.c
@@ -447,10 +447,16 @@ static int __kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
sysreg_restore_guest_state_vhe(guest_ctxt);
__debug_switch_to_guest(vcpu);
- if (is_hyp_ctxt(vcpu))
+ if (is_hyp_ctxt(vcpu)) {
+ if (vcpu_el2_e2h_is_set(vcpu))
+ vcpu_set_flag(vcpu, VCPU_HCR_E2H);
+ else
+ vcpu_clear_flag(vcpu, VCPU_HCR_E2H);
+
vcpu_set_flag(vcpu, VCPU_HYP_CONTEXT);
- else
+ } else {
vcpu_clear_flag(vcpu, VCPU_HYP_CONTEXT);
+ }
do {
/* Jump in the fire! */
Thanks,
Ganapat
