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.