[PATCH v2 0/2] KVM: arm64: Fixup PC updates on exit to userspace

[PATCH v2 0/2] KVM: arm64: Fixup PC updates on exit to userspace

[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
  - 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(-)

-- 
2.29.2

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

[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.

Signed-off-by: Marc Zyngier <maz@kernel.org>
Cc: stable@vger.kernel.org # 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(-)

diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
index cf8df032b9c3..d5b11037401d 100644
--- a/arch/arm64/include/asm/kvm_asm.h
+++ b/arch/arm64/include/asm/kvm_asm.h
@@ -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);
diff --git a/arch/arm64/kvm/hyp/exception.c b/arch/arm64/kvm/hyp/exception.c
index 73629094f903..0812a496725f 100644
--- a/arch/arm64/kvm/hyp/exception.c
+++ b/arch/arm64/kvm/hyp/exception.c
@@ -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;
+	}
+}
diff --git a/arch/arm64/kvm/hyp/include/hyp/adjust_pc.h b/arch/arm64/kvm/hyp/include/hyp/adjust_pc.h
index 61716359035d..4fdfeabefeb4 100644
--- a/arch/arm64/kvm/hyp/include/hyp/adjust_pc.h
+++ b/arch/arm64/kvm/hyp/include/hyp/adjust_pc.h
@@ -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.
diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c
index e9f6ea704d07..f7af9688c1f7 100644
--- a/arch/arm64/kvm/hyp/nvhe/switch.c
+++ b/arch/arm64/kvm/hyp/nvhe/switch.c
@@ -4,7 +4,6 @@
  * Author: Marc Zyngier <marc.zyngier@arm.com>
  */
 
-#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
diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c
index 7b8f7db5c1ed..b3229924d243 100644
--- a/arch/arm64/kvm/hyp/vhe/switch.c
+++ b/arch/arm64/kvm/hyp/vhe/switch.c
@@ -4,7 +4,6 @@
  * Author: Marc Zyngier <marc.zyngier@arm.com>
  */
 
-#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);
-- 
2.29.2

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

[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 <yuzenghui@huawei.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Cc: stable@vger.kernel.org # 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(-)

diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
index d5b11037401d..5e9b33cbac51 100644
--- a/arch/arm64/include/asm/kvm_asm.h
+++ b/arch/arm64/include/asm/kvm_asm.h
@@ -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__
 
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 1cb39c0803a4..c4fe2b71f429 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -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;
 }
diff --git a/arch/arm64/kvm/hyp/exception.c b/arch/arm64/kvm/hyp/exception.c
index 0812a496725f..11541b94b328 100644
--- a/arch/arm64/kvm/hyp/exception.c
+++ b/arch/arm64/kvm/hyp/exception.c
@@ -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)
 {
diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
index f36420a80474..1632f001f4ed 100644
--- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
+++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
@@ -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),
-- 
2.29.2

Re: [PATCH v2 0/2] KVM: arm64: Fixup PC updates on exit to userspace

[Zenghui Yu]

Hi Marc,

On 2021/5/14 18:40, Marc Zyngier wrote:
> 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 <yuzenghui@huawei.com>
Tested-by: Zenghui Yu <yuzenghui@huawei.com>

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

[Alexandru Elisei]

Hi Marc,

On 5/14/21 11:40 AM, Marc Zyngier wrote:
> 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 <alexandru.elisei@arm.com>

Thanks,

Alex

>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Cc: stable@vger.kernel.org # 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(-)
>
> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> index cf8df032b9c3..d5b11037401d 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -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);
> diff --git a/arch/arm64/kvm/hyp/exception.c b/arch/arm64/kvm/hyp/exception.c
> index 73629094f903..0812a496725f 100644
> --- a/arch/arm64/kvm/hyp/exception.c
> +++ b/arch/arm64/kvm/hyp/exception.c
> @@ -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;
> +	}
> +}
> diff --git a/arch/arm64/kvm/hyp/include/hyp/adjust_pc.h b/arch/arm64/kvm/hyp/include/hyp/adjust_pc.h
> index 61716359035d..4fdfeabefeb4 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/adjust_pc.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/adjust_pc.h
> @@ -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.
> diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c
> index e9f6ea704d07..f7af9688c1f7 100644
> --- a/arch/arm64/kvm/hyp/nvhe/switch.c
> +++ b/arch/arm64/kvm/hyp/nvhe/switch.c
> @@ -4,7 +4,6 @@
>   * Author: Marc Zyngier <marc.zyngier@arm.com>
>   */
>  
> -#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
> diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c
> index 7b8f7db5c1ed..b3229924d243 100644
> --- a/arch/arm64/kvm/hyp/vhe/switch.c
> +++ b/arch/arm64/kvm/hyp/vhe/switch.c
> @@ -4,7 +4,6 @@
>   * Author: Marc Zyngier <marc.zyngier@arm.com>
>   */
>  
> -#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);

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

[Alexandru Elisei]

Hi Marc,

On 5/14/21 11:40 AM, Marc Zyngier wrote:
> 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

> (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 <yuzenghui@huawei.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Cc: stable@vger.kernel.org # 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(-)
>
> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> index d5b11037401d..5e9b33cbac51 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -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__
>  
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 1cb39c0803a4..c4fe2b71f429 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -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;
>  }
> diff --git a/arch/arm64/kvm/hyp/exception.c b/arch/arm64/kvm/hyp/exception.c
> index 0812a496725f..11541b94b328 100644
> --- a/arch/arm64/kvm/hyp/exception.c
> +++ b/arch/arm64/kvm/hyp/exception.c
> @@ -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)
>  {
> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> index f36420a80474..1632f001f4ed 100644
> --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> @@ -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),

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 <alexandru.elisei@arm.com>

Thanks,

Alex

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

[Marc Zyngier]

On 2021-05-14 15:08, Alexandru Elisei wrote:
> Hi Marc,
> 
> On 5/14/21 11:40 AM, Marc Zyngier wrote:
>> 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

Ah, forgot that one.

> 
>> (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 <yuzenghui@huawei.com>
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> Cc: stable@vger.kernel.org # 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(-)
>> 
>> diff --git a/arch/arm64/include/asm/kvm_asm.h 
>> b/arch/arm64/include/asm/kvm_asm.h
>> index d5b11037401d..5e9b33cbac51 100644
>> --- a/arch/arm64/include/asm/kvm_asm.h
>> +++ b/arch/arm64/include/asm/kvm_asm.h
>> @@ -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__
>> 
>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>> index 1cb39c0803a4..c4fe2b71f429 100644
>> --- a/arch/arm64/kvm/arm.c
>> +++ b/arch/arm64/kvm/arm.c
>> @@ -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.

I didn't managed to commit this:

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index c4fe2b71f429..1126eae27400 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -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!

>> +	 */
>> +	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;
>>  }
>> diff --git a/arch/arm64/kvm/hyp/exception.c 
>> b/arch/arm64/kvm/hyp/exception.c
>> index 0812a496725f..11541b94b328 100644
>> --- a/arch/arm64/kvm/hyp/exception.c
>> +++ b/arch/arm64/kvm/hyp/exception.c
>> @@ -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)
>>  {
>> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c 
>> b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
>> index f36420a80474..1632f001f4ed 100644
>> --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
>> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
>> @@ -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),
> 
> 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 <alexandru.elisei@arm.com>

Thanks,

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