Hi Marc, On 5/10/21 10:49 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
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? > context is wrong, as userpsace doesn't have access to these flags 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 <[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(-) > > 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..d62a7041ebd1 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 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. > + * 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. Thanks, Alex > + 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), _______________________________________________ kvmarm mailing list [email protected] https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
