KVM: arm64: Fixup PC updates on exit to userspace

PATCH In review Marc Zyngier 3 participants 7 messages May 14, 2021

Subsystems (4)

Stable branch KVM arm64 KVM arm64

lore

Share & tools

lore

Marc Zyngier

author maintainer

2021-05-14 10:40 UTC #0 lore raw

v2 cover KVM: arm64: Fixup PC updates on exit to userspace

We recently moved anything related to PC updates into the guest entry code to help with the protected KVM effort. However, I missed a critical point: userspace needs to be able to observe state changes when the vcpu exits. Otherwise, live migration is a bit broken and vcpu reset can fail (as reported by Zenghui). Not good.

These two patches aim at fixing the above, and carry a Cc stable.

* From v1:
- Sanitized flag checking
- Added comment about relying on __kvm_adjust_pc() being preempt-safe
- Dropped superfluous includes

Marc Zyngier (2):
KVM: arm64: Move __adjust_pc out of line
KVM: arm64: Commit pending PC adjustemnts before returning to
userspace

 arch/arm64/include/asm/kvm_asm.h           |  3 +++
 arch/arm64/kvm/arm.c                       | 10 ++++++++++
 arch/arm64/kvm/hyp/exception.c             | 18 +++++++++++++++++-
 arch/arm64/kvm/hyp/include/hyp/adjust_pc.h | 18 ------------------
 arch/arm64/kvm/hyp/nvhe/hyp-main.c         |  8 ++++++++
 arch/arm64/kvm/hyp/nvhe/switch.c           |  3 +--
 arch/arm64/kvm/hyp/vhe/switch.c            |  3 +--
 7 files changed, 40 insertions(+), 23 deletions(-)

Signature · 1 line

2.29.2

Marc Zyngier

author maintainer

2021-05-14 10:40 UTC #1 lore raw

PATCH v2 1/2 KVM: arm64: Move __adjust_pc out of line

In order to make it easy to call __adjust_pc() from the EL1 code (in the case of nVHE), rename it to __kvm_adjust_pc() and move it out of line.

No expected functional change.

Signed-off-by Marc Zyngier <[email protected]>

Cc [email protected] # 5.11

---
 arch/arm64/include/asm/kvm_asm.h           |  2 ++
 arch/arm64/kvm/hyp/exception.c             | 18 +++++++++++++++++-
 arch/arm64/kvm/hyp/include/hyp/adjust_pc.h | 18 ------------------
 arch/arm64/kvm/hyp/nvhe/switch.c           |  3 +--
 arch/arm64/kvm/hyp/vhe/switch.c            |  3 +--
 5 files changed, 21 insertions(+), 23 deletions(-)

arch/arm64/include/asm/kvm_asm.h +2 -0

index cf8df032b9c3..d5b11037401d 100644
@@ -201,6 +201,8 @@ extern void __kvm_timer_set_cntvoff(u64 cntvoff);
 
 extern int __kvm_vcpu_run(struct kvm_vcpu *vcpu);
 
+extern void __kvm_adjust_pc(struct kvm_vcpu *vcpu);
+
 extern u64 __vgic_v3_get_gic_config(void);
 extern u64 __vgic_v3_read_vmcr(void);
 extern void __vgic_v3_write_vmcr(u32 vmcr);

arch/arm64/kvm/hyp/exception.c +17 -1

index 73629094f903..0812a496725f 100644
@@ -296,7 +296,7 @@ static void enter_exception32(struct kvm_vcpu *vcpu, u32 mode, u32 vect_offset)
 	*vcpu_pc(vcpu) = vect_offset;
 }
 
-void kvm_inject_exception(struct kvm_vcpu *vcpu)
+static void kvm_inject_exception(struct kvm_vcpu *vcpu)
 {
 	if (vcpu_el1_is_32bit(vcpu)) {
 		switch (vcpu->arch.flags & KVM_ARM64_EXCEPT_MASK) {

@@ -329,3 +329,19 @@ void kvm_inject_exception(struct kvm_vcpu *vcpu)
 		}
 	}
 }
+
+/*
+ * Adjust the guest PC on entry, depending on flags provided by EL1
+ * for the purpose of emulation (MMIO, sysreg) or exception injection.
+ */
+void __kvm_adjust_pc(struct kvm_vcpu *vcpu)
+{
+	if (vcpu->arch.flags & KVM_ARM64_PENDING_EXCEPTION) {
+		kvm_inject_exception(vcpu);
+		vcpu->arch.flags &= ~(KVM_ARM64_PENDING_EXCEPTION |
+				      KVM_ARM64_EXCEPT_MASK);
+	} else 	if (vcpu->arch.flags & KVM_ARM64_INCREMENT_PC) {
+		kvm_skip_instr(vcpu);
+		vcpu->arch.flags &= ~KVM_ARM64_INCREMENT_PC;
+	}
+}

arch/arm64/kvm/hyp/include/hyp/adjust_pc.h +0 ~~-18~~

index 61716359035d..4fdfeabefeb4 100644
@@ -13,8 +13,6 @@
 #include <asm/kvm_emulate.h>
 #include <asm/kvm_host.h>
 
-void kvm_inject_exception(struct kvm_vcpu *vcpu);
-
 static inline void kvm_skip_instr(struct kvm_vcpu *vcpu)
 {
 	if (vcpu_mode_is_32bit(vcpu)) {

@@ -43,22 +41,6 @@ static inline void __kvm_skip_instr(struct kvm_vcpu *vcpu)
 	write_sysreg_el2(*vcpu_pc(vcpu), SYS_ELR);
 }
 
-/*
- * Adjust the guest PC on entry, depending on flags provided by EL1
- * for the purpose of emulation (MMIO, sysreg) or exception injection.
- */
-static inline void __adjust_pc(struct kvm_vcpu *vcpu)
-{
-	if (vcpu->arch.flags & KVM_ARM64_PENDING_EXCEPTION) {
-		kvm_inject_exception(vcpu);
-		vcpu->arch.flags &= ~(KVM_ARM64_PENDING_EXCEPTION |
-				      KVM_ARM64_EXCEPT_MASK);
-	} else 	if (vcpu->arch.flags & KVM_ARM64_INCREMENT_PC) {
-		kvm_skip_instr(vcpu);
-		vcpu->arch.flags &= ~KVM_ARM64_INCREMENT_PC;
-	}
-}
-
 /*
  * Skip an instruction while host sysregs are live.
  * Assumes host is always 64-bit.

arch/arm64/kvm/hyp/nvhe/switch.c +1 -2

index e9f6ea704d07..f7af9688c1f7 100644
@@ -4,7 +4,6 @@
  * Author: Marc Zyngier <[email protected]>
  */
 
-#include <hyp/adjust_pc.h>
 #include <hyp/switch.h>
 #include <hyp/sysreg-sr.h>

@@ -201,7 +200,7 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu)
 	 */
 	__debug_save_host_buffers_nvhe(vcpu);
 
-	__adjust_pc(vcpu);
+	__kvm_adjust_pc(vcpu);
 
 	/*
 	 * We must restore the 32-bit state before the sysregs, thanks

arch/arm64/kvm/hyp/vhe/switch.c +1 -2

index 7b8f7db5c1ed..b3229924d243 100644
@@ -4,7 +4,6 @@
  * Author: Marc Zyngier <[email protected]>
  */
 
-#include <hyp/adjust_pc.h>
 #include <hyp/switch.h>
 
 #include <linux/arm-smccc.h>

@@ -132,7 +131,7 @@ static int __kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
 	__load_guest_stage2(vcpu->arch.hw_mmu);
 	__activate_traps(vcpu);
 
-	__adjust_pc(vcpu);
+	__kvm_adjust_pc(vcpu);
 
 	sysreg_restore_guest_state_vhe(guest_ctxt);
 	__debug_switch_to_guest(vcpu);

Signature · 1 line

2.29.2

Alexandru Elisei

Reviewed-by on v2 1/2

2021-05-14 14:06 UTC #2 lore raw

Re: [PATCH v2 1/2] KVM: arm64: Move __adjust_pc out of line

Hi Marc,

Quoted 5 lines · Marc Zyngier

In order to make it easy to call __adjust_pc() from the EL1 code (in the case of nVHE), rename it to __kvm_adjust_pc() and move it out of line.
No expected functional change.

Looks good to me. Ran the kvm-unit-tests test selftest-vectors-kernel, which goes out of its way to trigger an undefined exception, compiled for arm and arm64, under VHE, nVHE and protected modes on an odroid-c4, everything worked as expected:

Reviewed-by Alexandru Elisei <[email protected]>

Thanks,

Alex

Quoted 135 lines

Signed-off-by Marc Zyngier <[email protected]>

Cc [email protected] # 5.11

---
 arch/arm64/include/asm/kvm_asm.h           |  2 ++
 arch/arm64/kvm/hyp/exception.c             | 18 +++++++++++++++++-
 arch/arm64/kvm/hyp/include/hyp/adjust_pc.h | 18 ------------------
 arch/arm64/kvm/hyp/nvhe/switch.c           |  3 +--
 arch/arm64/kvm/hyp/vhe/switch.c            |  3 +--
 5 files changed, 21 insertions(+), 23 deletions(-)

arch/arm64/include/asm/kvm_asm.h +2 -0

index cf8df032b9c3..d5b11037401d 100644
@@ -201,6 +201,8 @@ extern void __kvm_timer_set_cntvoff(u64 cntvoff);
 
 extern int __kvm_vcpu_run(struct kvm_vcpu *vcpu);
 
+extern void __kvm_adjust_pc(struct kvm_vcpu *vcpu);
+
 extern u64 __vgic_v3_get_gic_config(void);
 extern u64 __vgic_v3_read_vmcr(void);
 extern void __vgic_v3_write_vmcr(u32 vmcr);

arch/arm64/kvm/hyp/exception.c +17 -1

index 73629094f903..0812a496725f 100644
@@ -296,7 +296,7 @@ static void enter_exception32(struct kvm_vcpu *vcpu, u32 mode, u32 vect_offset)
 	*vcpu_pc(vcpu) = vect_offset;
 }
 
-void kvm_inject_exception(struct kvm_vcpu *vcpu)
+static void kvm_inject_exception(struct kvm_vcpu *vcpu)
 {
 	if (vcpu_el1_is_32bit(vcpu)) {
 		switch (vcpu->arch.flags & KVM_ARM64_EXCEPT_MASK) {

@@ -329,3 +329,19 @@ void kvm_inject_exception(struct kvm_vcpu *vcpu)
 		}
 	}
 }
+
+/*
+ * Adjust the guest PC on entry, depending on flags provided by EL1
+ * for the purpose of emulation (MMIO, sysreg) or exception injection.
+ */
+void __kvm_adjust_pc(struct kvm_vcpu *vcpu)
+{
+	if (vcpu->arch.flags & KVM_ARM64_PENDING_EXCEPTION) {
+		kvm_inject_exception(vcpu);
+		vcpu->arch.flags &= ~(KVM_ARM64_PENDING_EXCEPTION |
+				      KVM_ARM64_EXCEPT_MASK);
+	} else 	if (vcpu->arch.flags & KVM_ARM64_INCREMENT_PC) {
+		kvm_skip_instr(vcpu);
+		vcpu->arch.flags &= ~KVM_ARM64_INCREMENT_PC;
+	}
+}

arch/arm64/kvm/hyp/include/hyp/adjust_pc.h +0 ~~-18~~

index 61716359035d..4fdfeabefeb4 100644
@@ -13,8 +13,6 @@
 #include <asm/kvm_emulate.h>
 #include <asm/kvm_host.h>
 
-void kvm_inject_exception(struct kvm_vcpu *vcpu);
-
 static inline void kvm_skip_instr(struct kvm_vcpu *vcpu)
 {
 	if (vcpu_mode_is_32bit(vcpu)) {

@@ -43,22 +41,6 @@ static inline void __kvm_skip_instr(struct kvm_vcpu *vcpu)
 	write_sysreg_el2(*vcpu_pc(vcpu), SYS_ELR);
 }
 
-/*
- * Adjust the guest PC on entry, depending on flags provided by EL1
- * for the purpose of emulation (MMIO, sysreg) or exception injection.
- */
-static inline void __adjust_pc(struct kvm_vcpu *vcpu)
-{
-	if (vcpu->arch.flags & KVM_ARM64_PENDING_EXCEPTION) {
-		kvm_inject_exception(vcpu);
-		vcpu->arch.flags &= ~(KVM_ARM64_PENDING_EXCEPTION |
-				      KVM_ARM64_EXCEPT_MASK);
-	} else 	if (vcpu->arch.flags & KVM_ARM64_INCREMENT_PC) {
-		kvm_skip_instr(vcpu);
-		vcpu->arch.flags &= ~KVM_ARM64_INCREMENT_PC;
-	}
-}
-
 /*
  * Skip an instruction while host sysregs are live.
  * Assumes host is always 64-bit.

arch/arm64/kvm/hyp/nvhe/switch.c +1 -2

index e9f6ea704d07..f7af9688c1f7 100644
@@ -4,7 +4,6 @@
  * Author: Marc Zyngier <[email protected]>
  */
 
-#include <hyp/adjust_pc.h>
 #include <hyp/switch.h>
 #include <hyp/sysreg-sr.h>

@@ -201,7 +200,7 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu)
 	 */
 	__debug_save_host_buffers_nvhe(vcpu);
 
-	__adjust_pc(vcpu);
+	__kvm_adjust_pc(vcpu);
 
 	/*
 	 * We must restore the 32-bit state before the sysregs, thanks

arch/arm64/kvm/hyp/vhe/switch.c +1 -2

index 7b8f7db5c1ed..b3229924d243 100644
@@ -4,7 +4,6 @@
  * Author: Marc Zyngier <[email protected]>
  */
 
-#include <hyp/adjust_pc.h>
 #include <hyp/switch.h>
 
 #include <linux/arm-smccc.h>

@@ -132,7 +131,7 @@ static int __kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
 	__load_guest_stage2(vcpu->arch.hw_mmu);
 	__activate_traps(vcpu);
 
-	__adjust_pc(vcpu);
+	__kvm_adjust_pc(vcpu);
 
 	sysreg_restore_guest_state_vhe(guest_ctxt);
 	__debug_switch_to_guest(vcpu);

Marc Zyngier

author maintainer

2021-05-14 10:40 UTC #3 lore raw

PATCH v2 2/2 KVM: arm64: Commit pending PC adjustemnts before returning to userspace

In reply to Marc Zyngier

KVM currently updates PC (and the corresponding exception state) using a two phase approach: first by setting a set of flags, then by converting these flags into a state update when the vcpu is about to enter the guest.

However, this creates a disconnect with userspace if the vcpu thread returns there with any exception/PC flag set. In this case, the exposed context is wrong, as userpsace doesn't have access to these flags (they aren't architectural). It also means that these flags are preserved across a reset, which isn't expected.

To solve this problem, force an explicit synchronisation of the exception state on vcpu exit to userspace. As an optimisation for nVHE systems, only perform this when there is something pending.

Reported-by Zenghui Yu <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

Cc [email protected] # 5.11

---
 arch/arm64/include/asm/kvm_asm.h   |  1 +
 arch/arm64/kvm/arm.c               | 10 ++++++++++
 arch/arm64/kvm/hyp/exception.c     |  4 ++--
 arch/arm64/kvm/hyp/nvhe/hyp-main.c |  8 ++++++++
 4 files changed, 21 insertions(+), 2 deletions(-)

arch/arm64/include/asm/kvm_asm.h +1 -0

index d5b11037401d..5e9b33cbac51 100644
@@ -63,6 +63,7 @@
 #define __KVM_HOST_SMCCC_FUNC___pkvm_cpu_set_vector		18
 #define __KVM_HOST_SMCCC_FUNC___pkvm_prot_finalize		19
 #define __KVM_HOST_SMCCC_FUNC___pkvm_mark_hyp			20
+#define __KVM_HOST_SMCCC_FUNC___kvm_adjust_pc			21
 
 #ifndef __ASSEMBLY__

arch/arm64/kvm/arm.c +10 -0

index 1cb39c0803a4..c4fe2b71f429 100644
@@ -897,6 +897,16 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
 
 	kvm_sigset_deactivate(vcpu);
 
+	/*
+	 * In the unlikely event that we are returning to userspace
+	 * with pending exceptions or PC adjustment, commit these
+	 * adjustments in order to give userspace a consistent view of
+	 * the vcpu state.
+	 */
+	if (unlikely(vcpu->arch.flags & (KVM_ARM64_PENDING_EXCEPTION |
+					 KVM_ARM64_INCREMENT_PC)))
+		kvm_call_hyp(__kvm_adjust_pc, vcpu);
+
 	vcpu_put(vcpu);
 	return ret;
 }

arch/arm64/kvm/hyp/exception.c +2 -2

index 0812a496725f..11541b94b328 100644
@@ -331,8 +331,8 @@ static void kvm_inject_exception(struct kvm_vcpu *vcpu)
 }
 
 /*
- * Adjust the guest PC on entry, depending on flags provided by EL1
- * for the purpose of emulation (MMIO, sysreg) or exception injection.
+ * Adjust the guest PC (and potentially exception state) depending on
+ * flags provided by the emulation code.
  */
 void __kvm_adjust_pc(struct kvm_vcpu *vcpu)
 {

arch/arm64/kvm/hyp/nvhe/hyp-main.c +8 -0

index f36420a80474..1632f001f4ed 100644
@@ -28,6 +28,13 @@ static void handle___kvm_vcpu_run(struct kvm_cpu_context *host_ctxt)
 	cpu_reg(host_ctxt, 1) =  __kvm_vcpu_run(kern_hyp_va(vcpu));
 }
 
+static void handle___kvm_adjust_pc(struct kvm_cpu_context *host_ctxt)
+{
+	DECLARE_REG(struct kvm_vcpu *, vcpu, host_ctxt, 1);
+
+	__kvm_adjust_pc(kern_hyp_va(vcpu));
+}
+
 static void handle___kvm_flush_vm_context(struct kvm_cpu_context *host_ctxt)
 {
 	__kvm_flush_vm_context();

@@ -170,6 +177,7 @@ typedef void (*hcall_t)(struct kvm_cpu_context *);
 
 static const hcall_t host_hcall[] = {
 	HANDLE_FUNC(__kvm_vcpu_run),
+	HANDLE_FUNC(__kvm_adjust_pc),
 	HANDLE_FUNC(__kvm_flush_vm_context),
 	HANDLE_FUNC(__kvm_tlb_flush_vmid_ipa),
 	HANDLE_FUNC(__kvm_tlb_flush_vmid),

Signature · 1 line

2.29.2

Alexandru Elisei

Reviewed-by on v2 2/2

2021-05-14 14:07 UTC #4 lore raw

Re: [PATCH v2 2/2] KVM: arm64: Commit pending PC adjustemnts before returning to userspace

Hi Marc,

Quoted 8 lines · Marc Zyngier

KVM currently updates PC (and the corresponding exception state) using a two phase approach: first by setting a set of flags, then by converting these flags into a state update when the vcpu is about to enter the guest.
However, this creates a disconnect with userspace if the vcpu thread returns there with any exception/PC flag set. In this case, the exposed context is wrong, as userpsace doesn't have access to these flags

Nitpick: s/userpsace/userspace

Quoted 84 lines

(they aren't architectural). It also means that these flags are preserved across a reset, which isn't expected.

To solve this problem, force an explicit synchronisation of the exception state on vcpu exit to userspace. As an optimisation for nVHE systems, only perform this when there is something pending.

Reported-by Zenghui Yu <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

Cc [email protected] # 5.11

---
 arch/arm64/include/asm/kvm_asm.h   |  1 +
 arch/arm64/kvm/arm.c               | 10 ++++++++++
 arch/arm64/kvm/hyp/exception.c     |  4 ++--
 arch/arm64/kvm/hyp/nvhe/hyp-main.c |  8 ++++++++
 4 files changed, 21 insertions(+), 2 deletions(-)

arch/arm64/include/asm/kvm_asm.h +1 -0

index d5b11037401d..5e9b33cbac51 100644
@@ -63,6 +63,7 @@
 #define __KVM_HOST_SMCCC_FUNC___pkvm_cpu_set_vector		18
 #define __KVM_HOST_SMCCC_FUNC___pkvm_prot_finalize		19
 #define __KVM_HOST_SMCCC_FUNC___pkvm_mark_hyp			20
+#define __KVM_HOST_SMCCC_FUNC___kvm_adjust_pc			21
 
 #ifndef __ASSEMBLY__

arch/arm64/kvm/arm.c +10 -0

index 1cb39c0803a4..c4fe2b71f429 100644
@@ -897,6 +897,16 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
 
 	kvm_sigset_deactivate(vcpu);
 
+	/*
+	 * In the unlikely event that we are returning to userspace
+	 * with pending exceptions or PC adjustment, commit these
+	 * adjustments in order to give userspace a consistent view of
+	 * the vcpu state.
+	 */
+	if (unlikely(vcpu->arch.flags & (KVM_ARM64_PENDING_EXCEPTION |
+					 KVM_ARM64_INCREMENT_PC)))
+		kvm_call_hyp(__kvm_adjust_pc, vcpu);
+
 	vcpu_put(vcpu);
 	return ret;
 }

arch/arm64/kvm/hyp/exception.c +2 -2

index 0812a496725f..11541b94b328 100644
@@ -331,8 +331,8 @@ static void kvm_inject_exception(struct kvm_vcpu *vcpu)
 }
 
 /*
- * Adjust the guest PC on entry, depending on flags provided by EL1
- * for the purpose of emulation (MMIO, sysreg) or exception injection.
+ * Adjust the guest PC (and potentially exception state) depending on
+ * flags provided by the emulation code.
  */
 void __kvm_adjust_pc(struct kvm_vcpu *vcpu)
 {

arch/arm64/kvm/hyp/nvhe/hyp-main.c +7 -0

index f36420a80474..1632f001f4ed 100644
@@ -28,6 +28,13 @@ static void handle___kvm_vcpu_run(struct kvm_cpu_context *host_ctxt)
 	cpu_reg(host_ctxt, 1) =  __kvm_vcpu_run(kern_hyp_va(vcpu));
 }
 
+static void handle___kvm_adjust_pc(struct kvm_cpu_context *host_ctxt)
+{
+	DECLARE_REG(struct kvm_vcpu *, vcpu, host_ctxt, 1);
+
+	__kvm_adjust_pc(kern_hyp_va(vcpu));
+}
+
 static void handle___kvm_flush_vm_context(struct kvm_cpu_context *host_ctxt)
 {
 	__kvm_flush_vm_context();

arch/arm64/kvm/hyp/nvhe/hyp-main.c typedef void (*hcall_t)(struct kvm_cpu_context *); · line 183

Quoted 7 lines

arch/arm64/kvm/hyp/nvhe/hyp-main.c +1 -0

@@ -170,6 +177,7 @@ typedef void (*hcall_t)(struct kvm_cpu_context *);
 
 static const hcall_t host_hcall[] = {
 	HANDLE_FUNC(__kvm_vcpu_run),
+	HANDLE_FUNC(__kvm_adjust_pc),
 	HANDLE_FUNC(__kvm_flush_vm_context),
 	HANDLE_FUNC(__kvm_tlb_flush_vmid_ipa),
 	HANDLE_FUNC(__kvm_tlb_flush_vmid),

I'm guessing that the comment mentioned in the cover letter should be in this patch, right? Or is the changelog from the cover letter stale?

Regardless, I trust your judgement regarding the comment and the patch looks correct:

Reviewed-by Alexandru Elisei <[email protected]>

Thanks,

Alex

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Marc Zyngier

author maintainer

2021-05-14 14:21 UTC #5 lore raw

Re: [PATCH v2 2/2] KVM: arm64: Commit pending PC adjustemnts before returning to userspace

Quoted 8 lines · Alexandru Elisei

Hi Marc,
Quoted 5 lines · Marc Zyngier
KVM currently updates PC (and the corresponding exception state) using a two phase approach: first by setting a set of flags, then by converting these flags into a state update when the vcpu is about to enter the guest.

Quoted 5 lines

Quoted 3 lines
However, this creates a disconnect with userspace if the vcpu thread returns there with any exception/PC flag set. In this case, the exposed context is wrong, as userpsace doesn't have access to these flags
Nitpick: s/userpsace/userspace

Ah, forgot that one.

Quoted 37 lines

Quoted 37 lines
(they aren't architectural). It also means that these flags are preserved across a reset, which isn't expected.
To solve this problem, force an explicit synchronisation of the exception state on vcpu exit to userspace. As an optimisation for nVHE systems, only perform this when there is something pending.
Reported-by Zenghui Yu <[email protected]>
Signed-off-by Marc Zyngier <[email protected]>
Cc [email protected] # 5.11
---
 arch/arm64/include/asm/kvm_asm.h   |  1 +
 arch/arm64/kvm/arm.c               | 10 ++++++++++
 arch/arm64/kvm/hyp/exception.c     |  4 ++--
 arch/arm64/kvm/hyp/nvhe/hyp-main.c |  8 ++++++++
 4 files changed, 21 insertions(+), 2 deletions(-)
arch/arm64/include/asm/kvm_asm.h +1 -0
index d5b11037401d..5e9b33cbac51 100644
@@ -63,6 +63,7 @@
 #define __KVM_HOST_SMCCC_FUNC___pkvm_cpu_set_vector		18
 #define __KVM_HOST_SMCCC_FUNC___pkvm_prot_finalize		19
 #define __KVM_HOST_SMCCC_FUNC___pkvm_mark_hyp			20
+#define __KVM_HOST_SMCCC_FUNC___kvm_adjust_pc			21
 
 #ifndef __ASSEMBLY__
 
arch/arm64/kvm/arm.c +0 -0
index 1cb39c0803a4..c4fe2b71f429 100644
@@ -897,6 +897,16 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu 
 *vcpu)
 

Quoted 7 lines

arch/arm64/kvm/arm.c int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu · line 905

Quoted 7 lines

arch/arm64/kvm/arm.c +5 -0

 	kvm_sigset_deactivate(vcpu);
 
+	/*
+	 * In the unlikely event that we are returning to userspace
+	 * with pending exceptions or PC adjustment, commit these
+	 * adjustments in order to give userspace a consistent view of
+	 * the vcpu state.

I didn't managed to commit this:

arch/arm64/kvm/arm.c +2 -1

index c4fe2b71f429..1126eae27400 100644
@@ -901,7 +901,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
         * In the unlikely event that we are returning to userspace
         * with pending exceptions or PC adjustment, commit these
         * adjustments in order to give userspace a consistent view of
-        * the vcpu state.
+        * the vcpu state. Note that this relies on __kvm_adjust_pc()
+        * being preempt-safe on VHE.
         */
        if (unlikely(vcpu->arch.flags & (KVM_ARM64_PENDING_EXCEPTION |
                                         KVM_ARM64_INCREMENT_PC)))

I promise I'll fold that in!

Quoted 57 lines

Quoted 46 lines

arch/arm64/kvm/arm.c +5 -0

+	 */
+	if (unlikely(vcpu->arch.flags & (KVM_ARM64_PENDING_EXCEPTION |
+					 KVM_ARM64_INCREMENT_PC)))
+		kvm_call_hyp(__kvm_adjust_pc, vcpu);
+
 	vcpu_put(vcpu);
 	return ret;

}

arch/arm64/kvm/hyp/exception.c +0 -2

index 0812a496725f..11541b94b328 100644
@@ -331,8 +331,8 @@ static void kvm_inject_exception(struct kvm_vcpu 
 *vcpu)
 }
 
 /*
- * Adjust the guest PC on entry, depending on flags provided by EL1
- * for the purpose of emulation (MMIO, sysreg) or exception

injection.
+ * Adjust the guest PC (and potentially exception state) depending on
+ * flags provided by the emulation code.
*/
void __kvm_adjust_pc(struct kvm_vcpu *vcpu)
{

arch/arm64/kvm/hyp/nvhe/hyp-main.c +7 -0

index f36420a80474..1632f001f4ed 100644
@@ -28,6 +28,13 @@ static void handle___kvm_vcpu_run(struct 
 kvm_cpu_context *host_ctxt)
 	cpu_reg(host_ctxt, 1) =  __kvm_vcpu_run(kern_hyp_va(vcpu));
 }
 
+static void handle___kvm_adjust_pc(struct kvm_cpu_context *host_ctxt)
+{
+	DECLARE_REG(struct kvm_vcpu *, vcpu, host_ctxt, 1);
+
+	__kvm_adjust_pc(kern_hyp_va(vcpu));
+}
+
 static void handle___kvm_flush_vm_context(struct kvm_cpu_context 
 *host_ctxt)

{

arch/arm64/kvm/hyp/nvhe/hyp-main.c +0 -0

 	__kvm_flush_vm_context();

arch/arm64/kvm/hyp/nvhe/hyp-main.c typedef void (*hcall_t)(struct kvm_cpu_context *); · line 183

Quoted 7 lines

arch/arm64/kvm/hyp/nvhe/hyp-main.c +1 -0

@@ -170,6 +177,7 @@ typedef void (*hcall_t)(struct kvm_cpu_context *);
 
 static const hcall_t host_hcall[] = {
 	HANDLE_FUNC(__kvm_vcpu_run),
+	HANDLE_FUNC(__kvm_adjust_pc),
 	HANDLE_FUNC(__kvm_flush_vm_context),
 	HANDLE_FUNC(__kvm_tlb_flush_vmid_ipa),
 	HANDLE_FUNC(__kvm_tlb_flush_vmid),

I'm guessing that the comment mentioned in the cover letter should be in this patch, right? Or is the changelog from the cover letter stale?

Quoted 4 lines

Regardless, I trust your judgement regarding the comment and the patch looks correct:
Reviewed-by Alexandru Elisei <[email protected]>

Thanks,

M.

Signature · 1 line

Jazz is not dead. It just smells funny...

Zenghui Yu

reviewer Reviewed-by Tested-by on the v2 series

2021-05-14 12:32 UTC #6 lore raw

In reply to Marc Zyngier

Hi Marc,

Quoted 11 lines · Marc Zyngier

We recently moved anything related to PC updates into the guest entry code to help with the protected KVM effort. However, I missed a critical point: userspace needs to be able to observe state changes when the vcpu exits. Otherwise, live migration is a bit broken and vcpu reset can fail (as reported by Zenghui). Not good.
These two patches aim at fixing the above, and carry a Cc stable.
* From v1:
- Sanitized flag checking
- Added comment about relying on __kvm_adjust_pc() being preempt-safe

I had a try but failed to find the added comment ;-). Regardless,

Reviewed-by Zenghui Yu <[email protected]>

Tested-by Zenghui Yu <[email protected]>