From: Andrew Scull <ascull@google.com> To: Marc Zyngier <maz@kernel.org> Cc: kernel-team@android.com, catalin.marinas@arm.com, linux-arm-kernel@lists.infradead.org, Sudeep Holla <sudeep.holla@arm.com>, will@kernel.org, kvmarm@lists.cs.columbia.edu Subject: Re: [PATCH v3 06/18] KVM: arm64: nVHE: Use separate vector for the host Date: Tue, 8 Sep 2020 11:29:38 +0100 [thread overview] Message-ID: <20200908102938.GB3268721@google.com> (raw) In-Reply-To: <87y2lllsu1.wl-maz@kernel.org> On Mon, Sep 07, 2020 at 12:38:46PM +0100, Marc Zyngier wrote: > Hi Andrew, > > On Thu, 03 Sep 2020 14:52:55 +0100, > Andrew Scull <ascull@google.com> wrote: > > > > The host is treated differently from the guests when an exception is > > taken so introduce a separate vector that is specialized for the host. > > This also allows the nVHE specific code to move out of hyp-entry.S and > > into nvhe/host.S. > > > > The host is only expected to make HVC calls and anything else is > > considered invalid and results in a panic. > > > > Hyp initialization is now passed the vector that is used for the host > > and it is swapped for the guest vector during the context switch. > > > > Signed-off-by: Andrew Scull <ascull@google.com> > > --- > > arch/arm64/include/asm/kvm_asm.h | 2 + > > arch/arm64/kernel/image-vars.h | 1 + > > arch/arm64/kvm/arm.c | 11 +++- > > arch/arm64/kvm/hyp/hyp-entry.S | 66 -------------------- > > arch/arm64/kvm/hyp/nvhe/Makefile | 2 +- > > arch/arm64/kvm/hyp/nvhe/host.S | 104 +++++++++++++++++++++++++++++++ > > arch/arm64/kvm/hyp/nvhe/switch.c | 3 + > > 7 files changed, 121 insertions(+), 68 deletions(-) > > create mode 100644 arch/arm64/kvm/hyp/nvhe/host.S > > > > diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h > > index 6f9c4162a764..34ec1b558219 100644 > > --- a/arch/arm64/include/asm/kvm_asm.h > > +++ b/arch/arm64/include/asm/kvm_asm.h > > @@ -98,10 +98,12 @@ struct kvm_vcpu; > > struct kvm_s2_mmu; > > > > DECLARE_KVM_NVHE_SYM(__kvm_hyp_init); > > +DECLARE_KVM_NVHE_SYM(__kvm_hyp_host_vector); > > DECLARE_KVM_HYP_SYM(__kvm_hyp_vector); > > > > #ifndef __KVM_NVHE_HYPERVISOR__ > > #define __kvm_hyp_init CHOOSE_NVHE_SYM(__kvm_hyp_init) > > +#define __kvm_hyp_host_vector CHOOSE_NVHE_SYM(__kvm_hyp_host_vector) > > #define __kvm_hyp_vector CHOOSE_HYP_SYM(__kvm_hyp_vector) > > #endif > > > > diff --git a/arch/arm64/kernel/image-vars.h b/arch/arm64/kernel/image-vars.h > > index 8982b68289b7..54bb0eb34b0f 100644 > > --- a/arch/arm64/kernel/image-vars.h > > +++ b/arch/arm64/kernel/image-vars.h > > @@ -71,6 +71,7 @@ KVM_NVHE_ALIAS(kvm_update_va_mask); > > /* Global kernel state accessed by nVHE hyp code. */ > > KVM_NVHE_ALIAS(arm64_ssbd_callback_required); > > KVM_NVHE_ALIAS(kvm_host_data); > > +KVM_NVHE_ALIAS(kvm_hyp_vector); > > KVM_NVHE_ALIAS(kvm_vgic_global_state); > > > > /* Kernel constant needed to compute idmap addresses. */ > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > > index 77fc856ea513..b6442c6be5ad 100644 > > --- a/arch/arm64/kvm/arm.c > > +++ b/arch/arm64/kvm/arm.c > > @@ -1277,7 +1277,7 @@ static void cpu_init_hyp_mode(void) > > > > pgd_ptr = kvm_mmu_get_httbr(); > > hyp_stack_ptr = __this_cpu_read(kvm_arm_hyp_stack_page) + PAGE_SIZE; > > - vector_ptr = __this_cpu_read(kvm_hyp_vector); > > + vector_ptr = (unsigned long)kern_hyp_va(kvm_ksym_ref(__kvm_hyp_host_vector)); > > > > /* > > * Call initialization code, and switch to the full blown HYP code. > > @@ -1542,6 +1542,7 @@ static int init_hyp_mode(void) > > > > for_each_possible_cpu(cpu) { > > struct kvm_host_data *cpu_data; > > + unsigned long *vector; > > > > cpu_data = per_cpu_ptr(&kvm_host_data, cpu); > > err = create_hyp_mappings(cpu_data, cpu_data + 1, PAGE_HYP); > > @@ -1550,6 +1551,14 @@ static int init_hyp_mode(void) > > kvm_err("Cannot map host CPU state: %d\n", err); > > goto out_err; > > } > > + > > + vector = per_cpu_ptr(&kvm_hyp_vector, cpu); > > + err = create_hyp_mappings(vector, vector + 1, PAGE_HYP); > > + > > + if (err) { > > + kvm_err("Cannot map hyp guest vector address\n"); > > + goto out_err; > > + } > > } > > > > err = hyp_map_aux_data(); > > diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S > > index 9cb3fbca5d79..f92489250dfc 100644 > > --- a/arch/arm64/kvm/hyp/hyp-entry.S > > +++ b/arch/arm64/kvm/hyp/hyp-entry.S > > @@ -12,7 +12,6 @@ > > #include <asm/cpufeature.h> > > #include <asm/kvm_arm.h> > > #include <asm/kvm_asm.h> > > -#include <asm/kvm_mmu.h> > > #include <asm/mmu.h> > > > > .macro save_caller_saved_regs_vect > > @@ -41,20 +40,6 @@ > > > > .text > > > > -.macro do_el2_call > > - /* > > - * Shuffle the parameters before calling the function > > - * pointed to in x0. Assumes parameters in x[1,2,3]. > > - */ > > - str lr, [sp, #-16]! > > - mov lr, x0 > > - mov x0, x1 > > - mov x1, x2 > > - mov x2, x3 > > - blr lr > > - ldr lr, [sp], #16 > > -.endm > > - > > el1_sync: // Guest trapped into EL2 > > > > mrs x0, esr_el2 > > @@ -63,44 +48,6 @@ el1_sync: // Guest trapped into EL2 > > ccmp x0, #ESR_ELx_EC_HVC32, #4, ne > > b.ne el1_trap > > > > -#ifdef __KVM_NVHE_HYPERVISOR__ > > - mrs x1, vttbr_el2 // If vttbr is valid, the guest > > - cbnz x1, el1_hvc_guest // called HVC > > - > > - /* Here, we're pretty sure the host called HVC. */ > > - ldp x0, x1, [sp], #16 > > - > > - /* Check for a stub HVC call */ > > - cmp x0, #HVC_STUB_HCALL_NR > > - b.hs 1f > > - > > - /* > > - * Compute the idmap address of __kvm_handle_stub_hvc and > > - * jump there. Since we use kimage_voffset, do not use the > > - * HYP VA for __kvm_handle_stub_hvc, but the kernel VA instead > > - * (by loading it from the constant pool). > > - * > > - * Preserve x0-x4, which may contain stub parameters. > > - */ > > - ldr x5, =__kvm_handle_stub_hvc > > - ldr_l x6, kimage_voffset > > - > > - /* x5 = __pa(x5) */ > > - sub x5, x5, x6 > > - br x5 > > - > > -1: > > - /* > > - * Perform the EL2 call > > - */ > > - kern_hyp_va x0 > > - do_el2_call > > - > > - eret > > - sb > > -#endif /* __KVM_NVHE_HYPERVISOR__ */ > > - > > -el1_hvc_guest: > > /* > > * Fastest possible path for ARM_SMCCC_ARCH_WORKAROUND_1. > > * The workaround has already been applied on the host, > > @@ -198,18 +145,6 @@ el2_error: > > eret > > sb > > > > -#ifdef __KVM_NVHE_HYPERVISOR__ > > -SYM_FUNC_START(__hyp_do_panic) > > - mov lr, #(PSR_F_BIT | PSR_I_BIT | PSR_A_BIT | PSR_D_BIT |\ > > - PSR_MODE_EL1h) > > - msr spsr_el2, lr > > - ldr lr, =panic > > - msr elr_el2, lr > > - eret > > - sb > > -SYM_FUNC_END(__hyp_do_panic) > > -#endif > > - > > .macro invalid_vector label, target = hyp_panic > > .align 2 > > SYM_CODE_START(\label) > > @@ -222,7 +157,6 @@ SYM_CODE_END(\label) > > invalid_vector el2t_irq_invalid > > invalid_vector el2t_fiq_invalid > > invalid_vector el2t_error_invalid > > - invalid_vector el2h_sync_invalid > > invalid_vector el2h_irq_invalid > > invalid_vector el2h_fiq_invalid > > invalid_vector el1_fiq_invalid > > diff --git a/arch/arm64/kvm/hyp/nvhe/Makefile b/arch/arm64/kvm/hyp/nvhe/Makefile > > index aef76487edc2..ddf98eb07b9d 100644 > > --- a/arch/arm64/kvm/hyp/nvhe/Makefile > > +++ b/arch/arm64/kvm/hyp/nvhe/Makefile > > @@ -6,7 +6,7 @@ > > asflags-y := -D__KVM_NVHE_HYPERVISOR__ > > ccflags-y := -D__KVM_NVHE_HYPERVISOR__ > > > > -obj-y := timer-sr.o sysreg-sr.o debug-sr.o switch.o tlb.o hyp-init.o > > +obj-y := timer-sr.o sysreg-sr.o debug-sr.o switch.o tlb.o hyp-init.o host.o > > obj-y += ../vgic-v3-sr.o ../aarch32.o ../vgic-v2-cpuif-proxy.o ../entry.o \ > > ../fpsimd.o ../hyp-entry.o > > > > diff --git a/arch/arm64/kvm/hyp/nvhe/host.S b/arch/arm64/kvm/hyp/nvhe/host.S > > new file mode 100644 > > index 000000000000..9c96b9a3b71d > > --- /dev/null > > +++ b/arch/arm64/kvm/hyp/nvhe/host.S > > @@ -0,0 +1,104 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > +/* > > + * Copyright (C) 2020 - Google Inc > > + * Author: Andrew Scull <ascull@google.com> > > + */ > > + > > +#include <linux/linkage.h> > > + > > +#include <asm/assembler.h> > > +#include <asm/kvm_asm.h> > > +#include <asm/kvm_mmu.h> > > + > > + .text > > + > > +SYM_FUNC_START(__hyp_do_panic) > > + mov lr, #(PSR_F_BIT | PSR_I_BIT | PSR_A_BIT | PSR_D_BIT |\ > > + PSR_MODE_EL1h) > > + msr spsr_el2, lr > > + ldr lr, =panic > > + msr elr_el2, lr > > + eret > > + sb > > +SYM_FUNC_END(__hyp_do_panic) > > + > > +.macro valid_host_el1_sync_vect > > Do we actually need the "valid_" prefix? The invalid stuff is prefixed > as invalid already. Something like el1_sync_host would be good enough > IMHO. It's just a name; dropped the prefix. > > + .align 7 > > + esb > > + stp x0, x1, [sp, #-16]! > > + > > + mrs x0, esr_el2 > > + lsr x0, x0, #ESR_ELx_EC_SHIFT > > + cmp x0, #ESR_ELx_EC_HVC64 > > + b.ne hyp_panic > > + > > + ldp x0, x1, [sp], #16 > > You probably want to restore x0/x1 before branching to hyp_panic. At > least your stack pointer will be correct, and x0/x1 may contain > interesting stuff (not that it matters much at the moment, but I'm > hopeful that at some point we will have a better panic handling). Right, I had that in a later patch but it belongs here. Moved the LDP between the CMP and branch. .align 7 esb stp x0, x1, [sp, #-16]! mrs x0, esr_el2 lsr x0, x0, #ESR_ELx_EC_SHIFT cmp x0, #ESR_ELx_EC_HVC64 ldp x0, x1, [sp], #16 b.ne hyp_panic > > + > > + /* Check for a stub HVC call */ > > + cmp x0, #HVC_STUB_HCALL_NR > > + b.hs 1f > > + > > + /* > > + * Compute the idmap address of __kvm_handle_stub_hvc and > > + * jump there. Since we use kimage_voffset, do not use the > > + * HYP VA for __kvm_handle_stub_hvc, but the kernel VA instead > > + * (by loading it from the constant pool). > > + * > > + * Preserve x0-x4, which may contain stub parameters. > > + */ > > + ldr x5, =__kvm_handle_stub_hvc > > + ldr_l x6, kimage_voffset > > + > > + /* x5 = __pa(x5) */ > > + sub x5, x5, x6 > > + br x5 > > + > > +1: > > + /* > > + * Shuffle the parameters before calling the function > > + * pointed to in x0. Assumes parameters in x[1,2,3]. > > + */ > > + kern_hyp_va x0 > > + str lr, [sp, #-16]! > > + mov lr, x0 > > + mov x0, x1 > > + mov x1, x2 > > + mov x2, x3 > > + blr lr > > + ldr lr, [sp], #16 > > + > > + eret > > + sb > > Please add some checks to ensure that this macro never grows past 128 > bytes, which is all you can fit in a single vector entry. Something > like > > .org valid_host_el1_sync_vect + 0x80 > > should do the trick (the assembler will shout at you if you move the > pointer backward in the case the macro becomes too long). Gave it an explicit measurement check. +.macro host_el1_sync_vect + .align 7 +.L__vect_start\@: + esb + stp x0, x1, [sp, #-16]! ... + eret + sb +.L__vect_end\@: +.if ((.L__vect_end\@ - .L__vect_start\@) > 0x80) + .error "host_el1_sync_vect larger than vector entry" +.endif +.endm > > +.endm > > + > > +.macro invalid_host_vect > > + .align 7 > > + b hyp_panic > > +.endm > > + > > +/* > > + * CONFIG_KVM_INDIRECT_VECTORS is not applied to the host vector because the > > nit: s/vector/vectors/ Done. > > + * host already knows the address of hyp by virtue of loading it there. > > I find this comment a bit confusing (and I'm easily confused). How > about: > > "... because the host knows about the EL2 vectors already, and there is > no point in hiding them"? Done. > > + */ > > + .align 11 > > +SYM_CODE_START(__kvm_hyp_host_vector) > > + invalid_host_vect // Synchronous EL2t > > + invalid_host_vect // IRQ EL2t > > + invalid_host_vect // FIQ EL2t > > + invalid_host_vect // Error EL2t > > + > > + invalid_host_vect // Synchronous EL2h > > + invalid_host_vect // IRQ EL2h > > + invalid_host_vect // FIQ EL2h > > + invalid_host_vect // Error EL2h > > + > > + valid_host_el1_sync_vect // Synchronous 64-bit EL1 > > + invalid_host_vect // IRQ 64-bit EL1 > > + invalid_host_vect // FIQ 64-bit EL1 > > + invalid_host_vect // Error 64-bit EL1 > > + > > + invalid_host_vect // Synchronous 32-bit EL1 > > + invalid_host_vect // IRQ 32-bit EL1 > > + invalid_host_vect // FIQ 32-bit EL1 > > + invalid_host_vect // Error 32-bit EL1 > > +SYM_CODE_END(__kvm_hyp_host_vector) > > diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c > > index 1e8a31b7c94c..1ab773bb60ca 100644 > > --- a/arch/arm64/kvm/hyp/nvhe/switch.c > > +++ b/arch/arm64/kvm/hyp/nvhe/switch.c > > @@ -42,6 +42,7 @@ static void __activate_traps(struct kvm_vcpu *vcpu) > > } > > > > write_sysreg(val, cptr_el2); > > + write_sysreg(__hyp_this_cpu_read(kvm_hyp_vector), vbar_el2); > > There is still the pending question of whether this requires extra > synchronisation, but at least this becomes a problem common to both > VHE and nVHE. It does require an ISB to make the vectors change here and now or else, up until the ERET, either vector could be active. I've tried to keep this in mind so that both the host and guest vectors can handle this transition period. > > > > if (cpus_have_final_cap(ARM64_WORKAROUND_SPECULATIVE_AT)) { > > struct kvm_cpu_context *ctxt = &vcpu->arch.ctxt; > > @@ -60,6 +61,7 @@ static void __activate_traps(struct kvm_vcpu *vcpu) > > > > static void __deactivate_traps(struct kvm_vcpu *vcpu) > > { > > + extern char __kvm_hyp_host_vector[]; > > u64 mdcr_el2; > > > > ___deactivate_traps(vcpu); > > @@ -91,6 +93,7 @@ static void __deactivate_traps(struct kvm_vcpu *vcpu) > > write_sysreg(mdcr_el2, mdcr_el2); > > write_sysreg(HCR_HOST_NVHE_FLAGS, hcr_el2); > > write_sysreg(CPTR_EL2_DEFAULT, cptr_el2); > > + write_sysreg(__kvm_hyp_host_vector, vbar_el2); > > } > > > > static void __load_host_stage2(void) > > -- > > 2.28.0.402.g5ffc5be6b7-goog > > > > > > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible. _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
WARNING: multiple messages have this Message-ID (diff)
From: Andrew Scull <ascull@google.com> To: Marc Zyngier <maz@kernel.org> Cc: kernel-team@android.com, suzuki.poulose@arm.com, catalin.marinas@arm.com, james.morse@arm.com, linux-arm-kernel@lists.infradead.org, Sudeep Holla <sudeep.holla@arm.com>, will@kernel.org, kvmarm@lists.cs.columbia.edu, julien.thierry.kdev@gmail.com Subject: Re: [PATCH v3 06/18] KVM: arm64: nVHE: Use separate vector for the host Date: Tue, 8 Sep 2020 11:29:38 +0100 [thread overview] Message-ID: <20200908102938.GB3268721@google.com> (raw) In-Reply-To: <87y2lllsu1.wl-maz@kernel.org> On Mon, Sep 07, 2020 at 12:38:46PM +0100, Marc Zyngier wrote: > Hi Andrew, > > On Thu, 03 Sep 2020 14:52:55 +0100, > Andrew Scull <ascull@google.com> wrote: > > > > The host is treated differently from the guests when an exception is > > taken so introduce a separate vector that is specialized for the host. > > This also allows the nVHE specific code to move out of hyp-entry.S and > > into nvhe/host.S. > > > > The host is only expected to make HVC calls and anything else is > > considered invalid and results in a panic. > > > > Hyp initialization is now passed the vector that is used for the host > > and it is swapped for the guest vector during the context switch. > > > > Signed-off-by: Andrew Scull <ascull@google.com> > > --- > > arch/arm64/include/asm/kvm_asm.h | 2 + > > arch/arm64/kernel/image-vars.h | 1 + > > arch/arm64/kvm/arm.c | 11 +++- > > arch/arm64/kvm/hyp/hyp-entry.S | 66 -------------------- > > arch/arm64/kvm/hyp/nvhe/Makefile | 2 +- > > arch/arm64/kvm/hyp/nvhe/host.S | 104 +++++++++++++++++++++++++++++++ > > arch/arm64/kvm/hyp/nvhe/switch.c | 3 + > > 7 files changed, 121 insertions(+), 68 deletions(-) > > create mode 100644 arch/arm64/kvm/hyp/nvhe/host.S > > > > diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h > > index 6f9c4162a764..34ec1b558219 100644 > > --- a/arch/arm64/include/asm/kvm_asm.h > > +++ b/arch/arm64/include/asm/kvm_asm.h > > @@ -98,10 +98,12 @@ struct kvm_vcpu; > > struct kvm_s2_mmu; > > > > DECLARE_KVM_NVHE_SYM(__kvm_hyp_init); > > +DECLARE_KVM_NVHE_SYM(__kvm_hyp_host_vector); > > DECLARE_KVM_HYP_SYM(__kvm_hyp_vector); > > > > #ifndef __KVM_NVHE_HYPERVISOR__ > > #define __kvm_hyp_init CHOOSE_NVHE_SYM(__kvm_hyp_init) > > +#define __kvm_hyp_host_vector CHOOSE_NVHE_SYM(__kvm_hyp_host_vector) > > #define __kvm_hyp_vector CHOOSE_HYP_SYM(__kvm_hyp_vector) > > #endif > > > > diff --git a/arch/arm64/kernel/image-vars.h b/arch/arm64/kernel/image-vars.h > > index 8982b68289b7..54bb0eb34b0f 100644 > > --- a/arch/arm64/kernel/image-vars.h > > +++ b/arch/arm64/kernel/image-vars.h > > @@ -71,6 +71,7 @@ KVM_NVHE_ALIAS(kvm_update_va_mask); > > /* Global kernel state accessed by nVHE hyp code. */ > > KVM_NVHE_ALIAS(arm64_ssbd_callback_required); > > KVM_NVHE_ALIAS(kvm_host_data); > > +KVM_NVHE_ALIAS(kvm_hyp_vector); > > KVM_NVHE_ALIAS(kvm_vgic_global_state); > > > > /* Kernel constant needed to compute idmap addresses. */ > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > > index 77fc856ea513..b6442c6be5ad 100644 > > --- a/arch/arm64/kvm/arm.c > > +++ b/arch/arm64/kvm/arm.c > > @@ -1277,7 +1277,7 @@ static void cpu_init_hyp_mode(void) > > > > pgd_ptr = kvm_mmu_get_httbr(); > > hyp_stack_ptr = __this_cpu_read(kvm_arm_hyp_stack_page) + PAGE_SIZE; > > - vector_ptr = __this_cpu_read(kvm_hyp_vector); > > + vector_ptr = (unsigned long)kern_hyp_va(kvm_ksym_ref(__kvm_hyp_host_vector)); > > > > /* > > * Call initialization code, and switch to the full blown HYP code. > > @@ -1542,6 +1542,7 @@ static int init_hyp_mode(void) > > > > for_each_possible_cpu(cpu) { > > struct kvm_host_data *cpu_data; > > + unsigned long *vector; > > > > cpu_data = per_cpu_ptr(&kvm_host_data, cpu); > > err = create_hyp_mappings(cpu_data, cpu_data + 1, PAGE_HYP); > > @@ -1550,6 +1551,14 @@ static int init_hyp_mode(void) > > kvm_err("Cannot map host CPU state: %d\n", err); > > goto out_err; > > } > > + > > + vector = per_cpu_ptr(&kvm_hyp_vector, cpu); > > + err = create_hyp_mappings(vector, vector + 1, PAGE_HYP); > > + > > + if (err) { > > + kvm_err("Cannot map hyp guest vector address\n"); > > + goto out_err; > > + } > > } > > > > err = hyp_map_aux_data(); > > diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S > > index 9cb3fbca5d79..f92489250dfc 100644 > > --- a/arch/arm64/kvm/hyp/hyp-entry.S > > +++ b/arch/arm64/kvm/hyp/hyp-entry.S > > @@ -12,7 +12,6 @@ > > #include <asm/cpufeature.h> > > #include <asm/kvm_arm.h> > > #include <asm/kvm_asm.h> > > -#include <asm/kvm_mmu.h> > > #include <asm/mmu.h> > > > > .macro save_caller_saved_regs_vect > > @@ -41,20 +40,6 @@ > > > > .text > > > > -.macro do_el2_call > > - /* > > - * Shuffle the parameters before calling the function > > - * pointed to in x0. Assumes parameters in x[1,2,3]. > > - */ > > - str lr, [sp, #-16]! > > - mov lr, x0 > > - mov x0, x1 > > - mov x1, x2 > > - mov x2, x3 > > - blr lr > > - ldr lr, [sp], #16 > > -.endm > > - > > el1_sync: // Guest trapped into EL2 > > > > mrs x0, esr_el2 > > @@ -63,44 +48,6 @@ el1_sync: // Guest trapped into EL2 > > ccmp x0, #ESR_ELx_EC_HVC32, #4, ne > > b.ne el1_trap > > > > -#ifdef __KVM_NVHE_HYPERVISOR__ > > - mrs x1, vttbr_el2 // If vttbr is valid, the guest > > - cbnz x1, el1_hvc_guest // called HVC > > - > > - /* Here, we're pretty sure the host called HVC. */ > > - ldp x0, x1, [sp], #16 > > - > > - /* Check for a stub HVC call */ > > - cmp x0, #HVC_STUB_HCALL_NR > > - b.hs 1f > > - > > - /* > > - * Compute the idmap address of __kvm_handle_stub_hvc and > > - * jump there. Since we use kimage_voffset, do not use the > > - * HYP VA for __kvm_handle_stub_hvc, but the kernel VA instead > > - * (by loading it from the constant pool). > > - * > > - * Preserve x0-x4, which may contain stub parameters. > > - */ > > - ldr x5, =__kvm_handle_stub_hvc > > - ldr_l x6, kimage_voffset > > - > > - /* x5 = __pa(x5) */ > > - sub x5, x5, x6 > > - br x5 > > - > > -1: > > - /* > > - * Perform the EL2 call > > - */ > > - kern_hyp_va x0 > > - do_el2_call > > - > > - eret > > - sb > > -#endif /* __KVM_NVHE_HYPERVISOR__ */ > > - > > -el1_hvc_guest: > > /* > > * Fastest possible path for ARM_SMCCC_ARCH_WORKAROUND_1. > > * The workaround has already been applied on the host, > > @@ -198,18 +145,6 @@ el2_error: > > eret > > sb > > > > -#ifdef __KVM_NVHE_HYPERVISOR__ > > -SYM_FUNC_START(__hyp_do_panic) > > - mov lr, #(PSR_F_BIT | PSR_I_BIT | PSR_A_BIT | PSR_D_BIT |\ > > - PSR_MODE_EL1h) > > - msr spsr_el2, lr > > - ldr lr, =panic > > - msr elr_el2, lr > > - eret > > - sb > > -SYM_FUNC_END(__hyp_do_panic) > > -#endif > > - > > .macro invalid_vector label, target = hyp_panic > > .align 2 > > SYM_CODE_START(\label) > > @@ -222,7 +157,6 @@ SYM_CODE_END(\label) > > invalid_vector el2t_irq_invalid > > invalid_vector el2t_fiq_invalid > > invalid_vector el2t_error_invalid > > - invalid_vector el2h_sync_invalid > > invalid_vector el2h_irq_invalid > > invalid_vector el2h_fiq_invalid > > invalid_vector el1_fiq_invalid > > diff --git a/arch/arm64/kvm/hyp/nvhe/Makefile b/arch/arm64/kvm/hyp/nvhe/Makefile > > index aef76487edc2..ddf98eb07b9d 100644 > > --- a/arch/arm64/kvm/hyp/nvhe/Makefile > > +++ b/arch/arm64/kvm/hyp/nvhe/Makefile > > @@ -6,7 +6,7 @@ > > asflags-y := -D__KVM_NVHE_HYPERVISOR__ > > ccflags-y := -D__KVM_NVHE_HYPERVISOR__ > > > > -obj-y := timer-sr.o sysreg-sr.o debug-sr.o switch.o tlb.o hyp-init.o > > +obj-y := timer-sr.o sysreg-sr.o debug-sr.o switch.o tlb.o hyp-init.o host.o > > obj-y += ../vgic-v3-sr.o ../aarch32.o ../vgic-v2-cpuif-proxy.o ../entry.o \ > > ../fpsimd.o ../hyp-entry.o > > > > diff --git a/arch/arm64/kvm/hyp/nvhe/host.S b/arch/arm64/kvm/hyp/nvhe/host.S > > new file mode 100644 > > index 000000000000..9c96b9a3b71d > > --- /dev/null > > +++ b/arch/arm64/kvm/hyp/nvhe/host.S > > @@ -0,0 +1,104 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > +/* > > + * Copyright (C) 2020 - Google Inc > > + * Author: Andrew Scull <ascull@google.com> > > + */ > > + > > +#include <linux/linkage.h> > > + > > +#include <asm/assembler.h> > > +#include <asm/kvm_asm.h> > > +#include <asm/kvm_mmu.h> > > + > > + .text > > + > > +SYM_FUNC_START(__hyp_do_panic) > > + mov lr, #(PSR_F_BIT | PSR_I_BIT | PSR_A_BIT | PSR_D_BIT |\ > > + PSR_MODE_EL1h) > > + msr spsr_el2, lr > > + ldr lr, =panic > > + msr elr_el2, lr > > + eret > > + sb > > +SYM_FUNC_END(__hyp_do_panic) > > + > > +.macro valid_host_el1_sync_vect > > Do we actually need the "valid_" prefix? The invalid stuff is prefixed > as invalid already. Something like el1_sync_host would be good enough > IMHO. It's just a name; dropped the prefix. > > + .align 7 > > + esb > > + stp x0, x1, [sp, #-16]! > > + > > + mrs x0, esr_el2 > > + lsr x0, x0, #ESR_ELx_EC_SHIFT > > + cmp x0, #ESR_ELx_EC_HVC64 > > + b.ne hyp_panic > > + > > + ldp x0, x1, [sp], #16 > > You probably want to restore x0/x1 before branching to hyp_panic. At > least your stack pointer will be correct, and x0/x1 may contain > interesting stuff (not that it matters much at the moment, but I'm > hopeful that at some point we will have a better panic handling). Right, I had that in a later patch but it belongs here. Moved the LDP between the CMP and branch. .align 7 esb stp x0, x1, [sp, #-16]! mrs x0, esr_el2 lsr x0, x0, #ESR_ELx_EC_SHIFT cmp x0, #ESR_ELx_EC_HVC64 ldp x0, x1, [sp], #16 b.ne hyp_panic > > + > > + /* Check for a stub HVC call */ > > + cmp x0, #HVC_STUB_HCALL_NR > > + b.hs 1f > > + > > + /* > > + * Compute the idmap address of __kvm_handle_stub_hvc and > > + * jump there. Since we use kimage_voffset, do not use the > > + * HYP VA for __kvm_handle_stub_hvc, but the kernel VA instead > > + * (by loading it from the constant pool). > > + * > > + * Preserve x0-x4, which may contain stub parameters. > > + */ > > + ldr x5, =__kvm_handle_stub_hvc > > + ldr_l x6, kimage_voffset > > + > > + /* x5 = __pa(x5) */ > > + sub x5, x5, x6 > > + br x5 > > + > > +1: > > + /* > > + * Shuffle the parameters before calling the function > > + * pointed to in x0. Assumes parameters in x[1,2,3]. > > + */ > > + kern_hyp_va x0 > > + str lr, [sp, #-16]! > > + mov lr, x0 > > + mov x0, x1 > > + mov x1, x2 > > + mov x2, x3 > > + blr lr > > + ldr lr, [sp], #16 > > + > > + eret > > + sb > > Please add some checks to ensure that this macro never grows past 128 > bytes, which is all you can fit in a single vector entry. Something > like > > .org valid_host_el1_sync_vect + 0x80 > > should do the trick (the assembler will shout at you if you move the > pointer backward in the case the macro becomes too long). Gave it an explicit measurement check. +.macro host_el1_sync_vect + .align 7 +.L__vect_start\@: + esb + stp x0, x1, [sp, #-16]! ... + eret + sb +.L__vect_end\@: +.if ((.L__vect_end\@ - .L__vect_start\@) > 0x80) + .error "host_el1_sync_vect larger than vector entry" +.endif +.endm > > +.endm > > + > > +.macro invalid_host_vect > > + .align 7 > > + b hyp_panic > > +.endm > > + > > +/* > > + * CONFIG_KVM_INDIRECT_VECTORS is not applied to the host vector because the > > nit: s/vector/vectors/ Done. > > + * host already knows the address of hyp by virtue of loading it there. > > I find this comment a bit confusing (and I'm easily confused). How > about: > > "... because the host knows about the EL2 vectors already, and there is > no point in hiding them"? Done. > > + */ > > + .align 11 > > +SYM_CODE_START(__kvm_hyp_host_vector) > > + invalid_host_vect // Synchronous EL2t > > + invalid_host_vect // IRQ EL2t > > + invalid_host_vect // FIQ EL2t > > + invalid_host_vect // Error EL2t > > + > > + invalid_host_vect // Synchronous EL2h > > + invalid_host_vect // IRQ EL2h > > + invalid_host_vect // FIQ EL2h > > + invalid_host_vect // Error EL2h > > + > > + valid_host_el1_sync_vect // Synchronous 64-bit EL1 > > + invalid_host_vect // IRQ 64-bit EL1 > > + invalid_host_vect // FIQ 64-bit EL1 > > + invalid_host_vect // Error 64-bit EL1 > > + > > + invalid_host_vect // Synchronous 32-bit EL1 > > + invalid_host_vect // IRQ 32-bit EL1 > > + invalid_host_vect // FIQ 32-bit EL1 > > + invalid_host_vect // Error 32-bit EL1 > > +SYM_CODE_END(__kvm_hyp_host_vector) > > diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c > > index 1e8a31b7c94c..1ab773bb60ca 100644 > > --- a/arch/arm64/kvm/hyp/nvhe/switch.c > > +++ b/arch/arm64/kvm/hyp/nvhe/switch.c > > @@ -42,6 +42,7 @@ static void __activate_traps(struct kvm_vcpu *vcpu) > > } > > > > write_sysreg(val, cptr_el2); > > + write_sysreg(__hyp_this_cpu_read(kvm_hyp_vector), vbar_el2); > > There is still the pending question of whether this requires extra > synchronisation, but at least this becomes a problem common to both > VHE and nVHE. It does require an ISB to make the vectors change here and now or else, up until the ERET, either vector could be active. I've tried to keep this in mind so that both the host and guest vectors can handle this transition period. > > > > if (cpus_have_final_cap(ARM64_WORKAROUND_SPECULATIVE_AT)) { > > struct kvm_cpu_context *ctxt = &vcpu->arch.ctxt; > > @@ -60,6 +61,7 @@ static void __activate_traps(struct kvm_vcpu *vcpu) > > > > static void __deactivate_traps(struct kvm_vcpu *vcpu) > > { > > + extern char __kvm_hyp_host_vector[]; > > u64 mdcr_el2; > > > > ___deactivate_traps(vcpu); > > @@ -91,6 +93,7 @@ static void __deactivate_traps(struct kvm_vcpu *vcpu) > > write_sysreg(mdcr_el2, mdcr_el2); > > write_sysreg(HCR_HOST_NVHE_FLAGS, hcr_el2); > > write_sysreg(CPTR_EL2_DEFAULT, cptr_el2); > > + write_sysreg(__kvm_hyp_host_vector, vbar_el2); > > } > > > > static void __load_host_stage2(void) > > -- > > 2.28.0.402.g5ffc5be6b7-goog > > > > > > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-09-08 10:29 UTC|newest] Thread overview: 72+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-09-03 13:52 [PATCH v3 00/18] Introduce separate nVHE hyp context Andrew Scull 2020-09-03 13:52 ` Andrew Scull 2020-09-03 13:52 ` [PATCH v3 01/18] KVM: arm64: Remove __activate_vm wrapper Andrew Scull 2020-09-03 13:52 ` Andrew Scull 2020-09-03 13:52 ` [PATCH v3 02/18] KVM: arm64: Remove hyp_panic arguments Andrew Scull 2020-09-03 13:52 ` Andrew Scull 2020-09-07 10:21 ` Marc Zyngier 2020-09-07 10:21 ` Marc Zyngier 2020-09-03 13:52 ` [PATCH v3 03/18] KVM: arm64: Remove kvm_host_data_t typedef Andrew Scull 2020-09-03 13:52 ` Andrew Scull 2020-09-03 13:52 ` [PATCH v3 04/18] KVM: arm64: Restrict symbol aliasing to outside nVHE Andrew Scull 2020-09-03 13:52 ` Andrew Scull 2020-09-07 10:38 ` Marc Zyngier 2020-09-07 10:38 ` Marc Zyngier 2020-09-08 10:13 ` Andrew Scull 2020-09-08 10:13 ` Andrew Scull 2020-09-03 13:52 ` [PATCH v3 05/18] KVM: arm64: Save chosen hyp vector to a percpu variable Andrew Scull 2020-09-03 13:52 ` Andrew Scull 2020-09-03 13:52 ` [PATCH v3 06/18] KVM: arm64: nVHE: Use separate vector for the host Andrew Scull 2020-09-03 13:52 ` Andrew Scull 2020-09-07 11:38 ` Marc Zyngier 2020-09-07 11:38 ` Marc Zyngier 2020-09-08 10:29 ` Andrew Scull [this message] 2020-09-08 10:29 ` Andrew Scull 2020-09-03 13:52 ` [PATCH v3 07/18] KVM: arm64: nVHE: Don't consume host SErrors with ESB Andrew Scull 2020-09-03 13:52 ` Andrew Scull 2020-09-07 11:46 ` Marc Zyngier 2020-09-07 11:46 ` Marc Zyngier 2020-09-03 13:52 ` [PATCH v3 08/18] KVM: arm64: Introduce hyp context Andrew Scull 2020-09-03 13:52 ` Andrew Scull 2020-09-07 13:29 ` Marc Zyngier 2020-09-07 13:29 ` Marc Zyngier 2020-09-08 10:52 ` Andrew Scull 2020-09-08 10:52 ` Andrew Scull 2020-09-03 13:52 ` [PATCH v3 09/18] KVM: arm64: Update context references from host to hyp Andrew Scull 2020-09-03 13:52 ` Andrew Scull 2020-09-03 13:52 ` [PATCH v3 10/18] KVM: arm64: Restore hyp when panicking in guest context Andrew Scull 2020-09-03 13:52 ` Andrew Scull 2020-09-03 13:53 ` [PATCH v3 11/18] KVM: arm64: Share context save and restore macros Andrew Scull 2020-09-03 13:53 ` Andrew Scull 2020-09-03 13:53 ` [PATCH v3 12/18] KVM: arm64: nVHE: Switch to hyp context for EL2 Andrew Scull 2020-09-03 13:53 ` Andrew Scull 2020-09-07 13:02 ` Marc Zyngier 2020-09-07 13:02 ` Marc Zyngier 2020-09-08 10:42 ` Andrew Scull 2020-09-08 10:42 ` Andrew Scull 2020-09-03 13:53 ` [PATCH v3 13/18] KVM: arm64: nVHE: Handle hyp panics Andrew Scull 2020-09-03 13:53 ` Andrew Scull 2020-09-07 13:24 ` Marc Zyngier 2020-09-07 13:24 ` Marc Zyngier 2020-09-03 13:53 ` [PATCH v3 14/18] smccc: Cast arguments to unsigned long Andrew Scull 2020-09-03 13:53 ` Andrew Scull 2020-09-07 13:33 ` Marc Zyngier 2020-09-07 13:33 ` Marc Zyngier 2020-09-08 10:58 ` Andrew Scull 2020-09-08 10:58 ` Andrew Scull 2020-09-03 13:53 ` [PATCH v3 15/18] KVM: arm64: nVHE: Pass pointers consistently to hyp-init Andrew Scull 2020-09-03 13:53 ` Andrew Scull 2020-09-03 13:53 ` [PATCH v3 16/18] KVM: arm64: nVHE: Migrate hyp interface to SMCCC Andrew Scull 2020-09-03 13:53 ` Andrew Scull 2020-09-07 13:47 ` Marc Zyngier 2020-09-07 13:47 ` Marc Zyngier 2020-09-07 14:20 ` Marc Zyngier 2020-09-07 14:20 ` Marc Zyngier 2020-09-08 11:02 ` Andrew Scull 2020-09-08 11:02 ` Andrew Scull 2020-09-09 8:30 ` Andrew Scull 2020-09-09 8:30 ` Andrew Scull 2020-09-03 13:53 ` [PATCH v3 17/18] KVM: arm64: nVHE: Migrate hyp-init " Andrew Scull 2020-09-03 13:53 ` Andrew Scull 2020-09-03 13:53 ` [PATCH v3 18/18] KVM: arm64: nVHE: Fix pointers during SMCCC convertion Andrew Scull 2020-09-03 13:53 ` Andrew Scull
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200908102938.GB3268721@google.com \ --to=ascull@google.com \ --cc=catalin.marinas@arm.com \ --cc=kernel-team@android.com \ --cc=kvmarm@lists.cs.columbia.edu \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=maz@kernel.org \ --cc=sudeep.holla@arm.com \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.