[PATCH v3 06/18] KVM: arm64: nVHE: Use separate vector for the host

[Andrew Scull]

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