KVM: arm64: Fixup PC updates on exit to userspace

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           |  2 +-
 arch/arm64/kvm/hyp/vhe/switch.c            |  2 +-
 5 files changed, 21 insertions(+), 21 deletions(-)

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

index cf8df032b9c3..d5b11037401d 100644

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

+
 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 +1 -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) {

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

+ * 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 -1

index e9f6ea704d07..b8ac123c3419 100644
@@ -201,7 +201,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 +0 -0

index 7b8f7db5c1ed..3eafed0431f5 100644

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.

It does look to me like they're functionally identical. Minor comments below.

Quoted 17 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           |  2 +-
 arch/arm64/kvm/hyp/vhe/switch.c            |  2 +-
 5 files changed, 21 insertions(+), 21 deletions(-)

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

index cf8df032b9c3..d5b11037401d 100644
@@ -201,6 +201,8 @@ extern void __kvm_timer_set_cntvoff(u64 cntvoff);
 

arch/arm64/include/asm/kvm_asm.h extern void __kvm_timer_set_cntvoff(u64 cntvoff); · line 204

Quoted 3 lines

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

 extern int __kvm_vcpu_run(struct kvm_vcpu *vcpu);
 
+extern void __kvm_adjust_pc(struct kvm_vcpu *vcpu);

It looks pretty strange to have the file arch/arm64/kvm/hyp/include/hyp/adjust_pc.h, but the function __kvm_adjust_pc() in another header. I guess this was done because arch/arm64/kvm/arm.c will use the function in the next patch.

I was thinking that maybe renaming adjust_pc.h->skip_instr.h would make more sense, what do you think? I can send a patch on top of this series with the rename if you prefer.

Quoted 19 lines

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

+
 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 +1 -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) {

arch/arm64/kvm/hyp/exception.c void kvm_inject_exception(struct kvm_vcpu *vcpu) · line 334

Quoted 6 lines

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

@@ -329,3 +329,19 @@ void kvm_inject_exception(struct kvm_vcpu *vcpu)
 		}
 	}
 }
+
+/*
+ * Adjust the guest PC on entry, depending on flags provided by EL1

This is also called by the VHE code running at EL2, but the comment is reworded in the next patch, so it doesn't really matter, and keeping the diff a straight move makes it easier to read.

Quoted 70 lines

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

+ * 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 -1

index e9f6ea704d07..b8ac123c3419 100644
@@ -201,7 +201,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 +0 -0

index 7b8f7db5c1ed..3eafed0431f5 100644
@@ -132,7 +132,7 @@ static int __kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
 	__load_guest_stage2(vcpu->arch.hw_mmu);
 	__activate_traps(vcpu);
 

arch/arm64/kvm/hyp/vhe/switch.c static int __kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu) · line 135

Quoted 2 lines

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

-	__adjust_pc(vcpu);
+	__kvm_adjust_pc(vcpu);

With the function now moved to kvm_asm.h, the header include adjust_pc.h is not needed. Same for the nvhe version of switch.c.

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 +0 -0

index 1cb39c0803a4..d62a7041ebd1 100644

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

+		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();

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 2 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

The code seems to handle only the KVM_ARM64_PENDING_EXCEPTION flag. Is the "PC flag" a reference to the KVM_ARM64_INCREMENT_PC flag?

Quoted 2 lines

context is wrong, as userpsace doesn't have access to these flags

s/userpsace/userspace

Quoted 34 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..d62a7041ebd1 100644

arch/arm64/kvm/arm.c int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) · line 902

Quoted 6 lines

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

@@ -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

I'm going to assume "PC adjustment" means the KVM_ARM64_INCREMENT_PC flag. Please correct me if that's not true, but if that's the case, then the flag isn't handled below.

arch/arm64/kvm/arm.c int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) · line 907

Quoted 5 lines

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

+	 * adjustments in order to give userspace a consistent view of
+	 * the vcpu state.
+	 */
+	if (unlikely(vcpu->arch.flags & (KVM_ARM64_PENDING_EXCEPTION |
+					 KVM_ARM64_EXCEPT_MASK)))

The condition seems to suggest that it is valid to set KVM_ARM64_EXCEPT_{AA32,AA64}_* without setting KVM_ARM64_PENDING_EXCEPTION, which looks rather odd to me. Is that a valid use of the KVM_ARM64_EXCEPT_MASK bits? If it's not (the existing code always sets the exception type with the KVM_ARM64_PENDING_EXCEPTION), that I was thinking that checking only the KVM_ARM64_PENDING_EXCEPTION flag would make the intention clearer.

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 2 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

The code seems to handle only the KVM_ARM64_PENDING_EXCEPTION flag. Is the "PC flag" a reference to the KVM_ARM64_INCREMENT_PC flag?

No, it does handle both exception and PC increment, unless I have completely bodged something (entirely possible).

Quoted 48 lines

Quoted 2 lines

context is wrong, as userpsace doesn't have access to these flags

s/userpsace/userspace

Quoted 34 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..d62a7041ebd1 100644

arch/arm64/kvm/arm.c int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) · line 902

Quoted 6 lines

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

@@ -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

I'm going to assume "PC adjustment" means the KVM_ARM64_INCREMENT_PC flag. Please correct me if that's not true, but if that's the case, then the flag isn't handled below.

Quoted 13 lines

arch/arm64/kvm/arm.c int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) · line 907

Quoted 5 lines

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

+	 * adjustments in order to give userspace a consistent view of
+	 * the vcpu state.
+	 */
+	if (unlikely(vcpu->arch.flags & (KVM_ARM64_PENDING_EXCEPTION |
+					 KVM_ARM64_EXCEPT_MASK)))

The condition seems to suggest that it is valid to set KVM_ARM64_EXCEPT_{AA32,AA64}_* without setting KVM_ARM64_PENDING_EXCEPTION, which looks rather odd to me. Is that a valid use of the KVM_ARM64_EXCEPT_MASK bits? If it's not (the existing code always sets the exception type with the KVM_ARM64_PENDING_EXCEPTION), that I was thinking that checking only the KVM_ARM64_PENDING_EXCEPTION flag would make the intention clearer.

No, you are missing this (subtle) comment in kvm_host.h:

<quote>
/*
* Overlaps with KVM_ARM64_EXCEPT_MASK on purpose so that it can't be
* set together with an exception...
*/
#define KVM_ARM64_INCREMENT_PC (1 << 9) /* Increment PC */
</quote>

So (KVM_ARM64_PENDING_EXCEPTION | KVM_ARM64_EXCEPT_MASK) checks for *both* an exception and a PC increment.

Hi Marc,

Quoted 10 lines · Marc Zyngier

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 7 lines

Quoted 5 lines

Quoted 2 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

The code seems to handle only the KVM_ARM64_PENDING_EXCEPTION flag. Is the "PC flag" a reference to the KVM_ARM64_INCREMENT_PC flag?

No, it does handle both exception and PC increment, unless I have completely bodged something (entirely possible).

The message is correct, my bad.

Quoted 71 lines

Quoted 48 lines

Quoted 2 lines

context is wrong, as userpsace doesn't have access to these flags

s/userpsace/userspace

Quoted 34 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..d62a7041ebd1 100644

arch/arm64/kvm/arm.c int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) · line 902

Quoted 6 lines

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

@@ -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

I'm going to assume "PC adjustment" means the KVM_ARM64_INCREMENT_PC flag. Please correct me if that's not true, but if that's the case, then the flag isn't handled below.

Quoted 13 lines

arch/arm64/kvm/arm.c int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) · line 907

Quoted 5 lines

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

+	 * adjustments in order to give userspace a consistent view of
+	 * the vcpu state.
+	 */
+	if (unlikely(vcpu->arch.flags & (KVM_ARM64_PENDING_EXCEPTION |
+					 KVM_ARM64_EXCEPT_MASK)))

The condition seems to suggest that it is valid to set KVM_ARM64_EXCEPT_{AA32,AA64}_* without setting KVM_ARM64_PENDING_EXCEPTION, which looks rather odd to me. Is that a valid use of the KVM_ARM64_EXCEPT_MASK bits? If it's not (the existing code always sets the exception type with the KVM_ARM64_PENDING_EXCEPTION), that I was thinking that checking only the KVM_ARM64_PENDING_EXCEPTION flag would make the intention clearer.

No, you are missing this (subtle) comment in kvm_host.h:

<quote>
/*
* Overlaps with KVM_ARM64_EXCEPT_MASK on purpose so that it can't be
* set together with an exception...
*/
#define KVM_ARM64_INCREMENT_PC (1 << 9) /* Increment PC */
</quote>

Quoted 2 lines

So (KVM_ARM64_PENDING_EXCEPTION | KVM_ARM64_EXCEPT_MASK) checks for *both* an exception and a PC increment.

Then how about explicitly checking for the KVM_ARM64_PENDING_EXCEPTION and KVM_ARM64_INCREMENT_PC flags, like it's done in __kvm_adjust_pc? That would certainly make the code easier to understand, as it's not immediately obvious that the EXCEPT mask includes the INCREMENT_PC flag.

Quoted 92 lines · Alexandru Elisei

Hi Marc,

Quoted 10 lines · Marc Zyngier

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 7 lines

Quoted 5 lines

Quoted 2 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

The code seems to handle only the KVM_ARM64_PENDING_EXCEPTION flag. Is the "PC flag" a reference to the KVM_ARM64_INCREMENT_PC flag?

No, it does handle both exception and PC increment, unless I have completely bodged something (entirely possible).

The message is correct, my bad.

Quoted 70 lines

Quoted 47 lines

Quoted 1 line

context is wrong, as userpsace doesn't have access to these flags

s/userpsace/userspace

Quoted 34 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..d62a7041ebd1 100644

arch/arm64/kvm/arm.c int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) · line 902

Quoted 6 lines

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

@@ -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

I'm going to assume "PC adjustment" means the KVM_ARM64_INCREMENT_PC flag. Please correct me if that's not true, but if that's the case, then the flag isn't handled below.

Quoted 13 lines

arch/arm64/kvm/arm.c int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) · line 907

Quoted 5 lines

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

+	 * adjustments in order to give userspace a consistent view of
+	 * the vcpu state.
+	 */
+	if (unlikely(vcpu->arch.flags & (KVM_ARM64_PENDING_EXCEPTION |
+					 KVM_ARM64_EXCEPT_MASK)))

The condition seems to suggest that it is valid to set KVM_ARM64_EXCEPT_{AA32,AA64}_* without setting KVM_ARM64_PENDING_EXCEPTION, which looks rather odd to me. Is that a valid use of the KVM_ARM64_EXCEPT_MASK bits? If it's not (the existing code always sets the exception type with the KVM_ARM64_PENDING_EXCEPTION), that I was thinking that checking only the KVM_ARM64_PENDING_EXCEPTION flag would make the intention clearer.

No, you are missing this (subtle) comment in kvm_host.h:

<quote>
/*
* Overlaps with KVM_ARM64_EXCEPT_MASK on purpose so that it can't be
* set together with an exception...
*/
#define KVM_ARM64_INCREMENT_PC (1 << 9) /* Increment PC */
</quote>

Quoted 7 lines

Quoted 2 lines

So (KVM_ARM64_PENDING_EXCEPTION | KVM_ARM64_EXCEPT_MASK) checks for *both* an exception and a PC increment.

Then how about explicitly checking for the KVM_ARM64_PENDING_EXCEPTION and KVM_ARM64_INCREMENT_PC flags, like it's done in __kvm_adjust_pc? That would certainly make the code easier to understand, as it's not immediately obvious that the EXCEPT mask includes the INCREMENT_PC flag.

Fair enough. I'll fix that in v2.

Another thing I wondered about: we now rely on __kvm_adjust_pc() to be preemption safe. That's always the case in nVHE (we're at EL2), but VHE can be preempted at any point. The code we call is preemption safe, but it takes some effort to be convinced of it.

Do you have a good suggestion on how to express this requirement? I could throw a preempt_disable()/enable() at the call for the sake of being in the same context between VHE and nVHE, but that's not strictly necessary for now.

KVM: arm64: Commit pending PC adjustemnts before returning to userspace

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.

Hi Marc,

Quoted 1 line

KVM: arm64: Commit pending PC adjustemnts before returning to userspace

s/adjustments/adjustments

Quoted 12 lines

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.

Quoted 3 lines

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.

I've tested this with a few nvhe and vhe tests that exercise both __kvm_adjust_pc call paths (__kvm_vcpu_run and kvm_arch_vcpu_ioctl_run), and the tests ran as expected. I'll do the same for v2 when you send it out.

Hi Fuad,

Quoted 6 lines · Fuad Tabba

Hi Marc,

Quoted 1 line

KVM: arm64: Commit pending PC adjustemnts before returning to userspace

s/adjustments/adjustments

Looks like Gmail refuses to let you mimic my spelling mistakes! :D

Quoted 18 lines · Marc Zyngier

Quoted 15 lines

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.

Quoted 4 lines

I've tested this with a few nvhe and vhe tests that exercise both __kvm_adjust_pc call paths (__kvm_vcpu_run and kvm_arch_vcpu_ioctl_run), and the tests ran as expected. I'll do the same for v2 when you send it out.

Ah, that's interesting. Do you have tests that actually fail when hitting this bug? Given that this is pretty subtle, it'd be good to have a way to make sure it doesn't crop up again.

Thanks,

        M.

--
Without deviation from the norm, progress is not possible.

Hi Marc,

Quoted 37 lines · Marc Zyngier

Hi Fuad,

Quoted 6 lines · Fuad Tabba

Hi Marc,

Quoted 1 line

KVM: arm64: Commit pending PC adjustemnts before returning to userspace

s/adjustments/adjustments

Looks like Gmail refuses to let you mimic my spelling mistakes! :D

Quoted 22 lines · Marc Zyngier

Quoted 12 lines

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.

Quoted 3 lines

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.

I've tested this with a few nvhe and vhe tests that exercise both __kvm_adjust_pc call paths (__kvm_vcpu_run and kvm_arch_vcpu_ioctl_run), and the tests ran as expected. I'll do the same for v2 when you send it out.

Quoted 3 lines

Ah, that's interesting. Do you have tests that actually fail when hitting this bug? Given that this is pretty subtle, it'd be good to have a way to make sure it doesn't crop up again.

Nothing that fails, just code that generates exceptions or emulates instructions at various points. That said, I think it should be straightforward to write a selftest for this. I'll give it a go.