Re: [PATCH 02/15] arm64: kvm: Formalize host-hyp hypcall ABI

[Marc Zyngier]

Hi David, Quentin,

On Thu, 30 Apr 2020 15:48:18 +0100,
David Brazdil <dbrazdil@xxxxxxxxxx> wrote:
> 
> From: Quentin Perret <qperret@xxxxxxxxxx>
> 
> In preparation for removing the hyp code from the TCB, convert the current

nit: Unless we have a different interpretation of the TCB acronym, I
think you want to remove anything *but* the EL2 code from the TCB.

> EL1 - EL2 KVM ABI to use hypercall numbers in lieu of direct function pointers.
> While this in itself does not provide any isolation, it is a first step towards
> having a properly defined EL2 ABI.
> 
> The implementation is based on a kvm_hcall_table which holds function pointers
> to the hyp_text functions corresponding to each hypercall. This is highly
> inspired from the arm64 sys_call_table, the main difference being the lack of
> hcall wrappers at this stage.
> 
> Signed-off-by: Quentin Perret <qperret@xxxxxxxxxx>
> Signed-off-by: David Brazdil <dbrazdil@xxxxxxxxxx>
> ---
>  arch/arm64/include/asm/kvm_host.h            | 20 ++++++-
>  arch/arm64/include/asm/kvm_host_hypercalls.h | 62 ++++++++++++++++++++
>  arch/arm64/kvm/hyp/Makefile                  |  2 +-
>  arch/arm64/kvm/hyp/host_hypercall.c          | 22 +++++++
>  arch/arm64/kvm/hyp/hyp-entry.S               | 36 +++++++++++-
>  5 files changed, 137 insertions(+), 5 deletions(-)
>  create mode 100644 arch/arm64/include/asm/kvm_host_hypercalls.h
>  create mode 100644 arch/arm64/kvm/hyp/host_hypercall.c
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index e61143d6602d..5dec3b06f2b7 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -24,6 +24,7 @@
>  #include <asm/fpsimd.h>
>  #include <asm/kvm.h>
>  #include <asm/kvm_asm.h>
> +#include <asm/kvm_host_hypercalls.h>
>  #include <asm/thread_info.h>
>  #include <asm/virt.h>
>  
> @@ -447,10 +448,25 @@ int kvm_test_age_hva(struct kvm *kvm, unsigned long hva);
>  void kvm_arm_halt_guest(struct kvm *kvm);
>  void kvm_arm_resume_guest(struct kvm *kvm);
>  
> -#define kvm_call_hyp_nvhe(hypfn, ...) \
> -	__kvm_call_hyp((unsigned long)kvm_ksym_ref(hypfn), ##__VA_ARGS__)
> +/*
> + * Call the hypervisor via HVC. The hcall parameter must be the name of
> + * a hypercall as defined in <asm/kvm_host_hypercall.h>.
> + *
> + * Hypercalls take at most 6 parameters.
> + */
> +#define kvm_call_hyp_nvhe(hcall, ...) \
> +	__kvm_call_hyp(KVM_HOST_HCALL_NR(hcall), ##__VA_ARGS__)
>  
>  /*
> + * u64 kvm_call_hyp(hcall, ...);
> + *
> + * Call the hypervisor. The hcall parameter must be the name of a hypercall
> + * defined in <asm/kvm_host_hypercall.h>. In the VHE case, this will be
> + * translated into a direct function call.
> + *
> + * A hcall value of less than 0xfff has a special meaning and is used to
> + * implement hyp stubs in the same way as in arch/arm64/kernel/hyp_stub.S.
> + *
>   * The couple of isb() below are there to guarantee the same behaviour
>   * on VHE as on !VHE, where the eret to EL1 acts as a context
>   * synchronization event.
> diff --git a/arch/arm64/include/asm/kvm_host_hypercalls.h b/arch/arm64/include/asm/kvm_host_hypercalls.h
> new file mode 100644
> index 000000000000..af8ad505d816
> --- /dev/null
> +++ b/arch/arm64/include/asm/kvm_host_hypercalls.h
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2020 Google, inc
> + */
> +
> +#ifndef __KVM_HOST_HCALL
> +#define __KVM_HOST_HCALL(x)
> +#endif
> +
> +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_enable_ssbs		0
> +__KVM_HOST_HCALL(__kvm_enable_ssbs)
> +
> +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_get_mdcr_el2		1
> +__KVM_HOST_HCALL(__kvm_get_mdcr_el2)
> +
> +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_timer_set_cntvoff	2
> +__KVM_HOST_HCALL(__kvm_timer_set_cntvoff)
> +
> +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_tlb_flush_local_vmid	3
> +__KVM_HOST_HCALL(__kvm_tlb_flush_local_vmid)
> +
> +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_flush_vm_context	4
> +__KVM_HOST_HCALL(__kvm_flush_vm_context)
> +
> +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_vcpu_run_nvhe		5
> +__KVM_HOST_HCALL(__kvm_vcpu_run_nvhe)
> +
> +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_tlb_flush_vmid		6
> +__KVM_HOST_HCALL(__kvm_tlb_flush_vmid)
> +
> +#define __KVM_HOST_HCALL_TABLE_IDX___kvm_tlb_flush_vmid_ipa	7
> +__KVM_HOST_HCALL(__kvm_tlb_flush_vmid_ipa)
> +
> +#define __KVM_HOST_HCALL_TABLE_IDX___vgic_v3_init_lrs		8
> +__KVM_HOST_HCALL(__vgic_v3_init_lrs)
> +
> +#define __KVM_HOST_HCALL_TABLE_IDX___vgic_v3_get_ich_vtr_el2	9
> +__KVM_HOST_HCALL(__vgic_v3_get_ich_vtr_el2)
> +
> +#define __KVM_HOST_HCALL_TABLE_IDX___vgic_v3_write_vmcr		10
> +__KVM_HOST_HCALL(__vgic_v3_write_vmcr)
> +
> +#define __KVM_HOST_HCALL_TABLE_IDX___vgic_v3_restore_aprs	11
> +__KVM_HOST_HCALL(__vgic_v3_restore_aprs)
> +
> +#define __KVM_HOST_HCALL_TABLE_IDX___vgic_v3_read_vmcr		12
> +__KVM_HOST_HCALL(__vgic_v3_read_vmcr)
> +
> +#define __KVM_HOST_HCALL_TABLE_IDX___vgic_v3_save_aprs		13
> +__KVM_HOST_HCALL(__vgic_v3_save_aprs)
> +
> +#define __KVM_HOST_HCALL_TABLE_IDX_SIZE				14

This whole thing screams "enum" into my ears. Having to maintain these
as #defines feels like a pain, specially if the numbers are never used
in assembly code. (and for that, we have asm-offset.h).

> +
> +/* XXX - Arbitrary offset for KVM-related hypercalls */
> +#define __KVM_HOST_HCALL_BASE_SHIFT 8
> +#define __KVM_HOST_HCALL_BASE (1ULL << __KVM_HOST_HCALL_BASE_SHIFT)
> +#define __KVM_HOST_HCALL_END (__KVM_HOST_HCALL_BASE + \
> +			      __KVM_HOST_HCALL_TABLE_IDX_SIZE)

I don't really get what is this offset for. It is too small to be used
to skip the stub hypercalls (you'd need to start at 0x1000). Given
that you store the whole thing in an array, you're wasting some
memory. I'm sure you have a good story for it though! ;-)

> +
> +/* Hypercall number = kvm offset + table idx */
> +#define KVM_HOST_HCALL_NR(name) (__KVM_HOST_HCALL_TABLE_IDX_##name + \
> +				 __KVM_HOST_HCALL_BASE)
> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
> index 8c9880783839..29e2b2cd2fbc 100644
> --- a/arch/arm64/kvm/hyp/Makefile
> +++ b/arch/arm64/kvm/hyp/Makefile
> @@ -9,7 +9,7 @@ ccflags-y += -fno-stack-protector -DDISABLE_BRANCH_PROFILING \
>  obj-$(CONFIG_KVM) += hyp.o
>  
>  hyp-y := vgic-v3-sr.o timer-sr.o aarch32.o vgic-v2-cpuif-proxy.o sysreg-sr.o \
> -	 debug-sr.o entry.o switch.o fpsimd.o tlb.o hyp-entry.o
> +	 debug-sr.o entry.o switch.o fpsimd.o tlb.o host_hypercall.o hyp-entry.o
>  
>  # KVM code is run at a different exception code with a different map, so
>  # compiler instrumentation that inserts callbacks or checks into the code may
> diff --git a/arch/arm64/kvm/hyp/host_hypercall.c b/arch/arm64/kvm/hyp/host_hypercall.c
> new file mode 100644
> index 000000000000..6b31310f36a8
> --- /dev/null
> +++ b/arch/arm64/kvm/hyp/host_hypercall.c
> @@ -0,0 +1,22 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2020 Google, inc
> + */
> +
> +#include <asm/kvm_hyp.h>
> +
> +typedef long (*kvm_hcall_fn_t)(void);
> +
> +static long __hyp_text kvm_hc_ni(void)
> +{
> +       return -ENOSYS;
> +}
> +
> +#undef __KVM_HOST_HCALL
> +#define __KVM_HOST_HCALL(name) \
> +	[__KVM_HOST_HCALL_TABLE_IDX_##name] = (long (*)(void))name,
> +
> +const kvm_hcall_fn_t kvm_hcall_table[__KVM_HOST_HCALL_TABLE_IDX_SIZE] = {
> +	[0 ... __KVM_HOST_HCALL_TABLE_IDX_SIZE-1] = kvm_hc_ni,
> +#include <asm/kvm_host_hypercalls.h>
> +};

Cunning. At the same time, if you can do this once, you can do it
twice, and generating the __KVM_HOST_HCALL_TABLE_IDX_* as an enum
should be pretty easy, and could live in its own include file.

> diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
> index c2a13ab3c471..1a51a0958504 100644
> --- a/arch/arm64/kvm/hyp/hyp-entry.S
> +++ b/arch/arm64/kvm/hyp/hyp-entry.S
> @@ -13,6 +13,7 @@
>  #include <asm/kvm_arm.h>
>  #include <asm/kvm_asm.h>
>  #include <asm/kvm_mmu.h>
> +#include <asm/kvm_host_hypercalls.h>
>  #include <asm/mmu.h>
>  
>  	.text
> @@ -21,17 +22,26 @@
>  .macro do_el2_call
>  	/*
>  	 * Shuffle the parameters before calling the function
> -	 * pointed to in x0. Assumes parameters in x[1,2,3].
> +	 * pointed to in x0. Assumes parameters in x[1,2,3,4,5,6].
>  	 */
>  	str	lr, [sp, #-16]!
>  	mov	lr, x0
>  	mov	x0, x1
>  	mov	x1, x2
>  	mov	x2, x3
> +	mov	x3, x4
> +	mov	x4, x5
> +	mov	x5, x6

Has any function changed prototype as part of this patch that they'd
require this change? It doesn't look like it. I guess this should be
part of another patch.

>  	blr	lr
>  	ldr	lr, [sp], #16
>  .endm
>  
> +/* kern_hyp_va(lm_alias(ksym)) */
> +.macro ksym_hyp_va ksym, lm_offset
> +	sub	\ksym, \ksym, \lm_offset
> +	kern_hyp_va	\ksym
> +.endm
> +
>  el1_sync:				// Guest trapped into EL2
>  
>  	mrs	x0, esr_el2
> @@ -66,10 +76,32 @@ el1_sync:				// Guest trapped into EL2
>  	br	x5
>  
>  1:
> +	/* Check if the hcall number is valid */
> +	cmp	x0, #__KVM_HOST_HCALL_BASE
> +	b.lt	2f
> +	cmp	x0, #__KVM_HOST_HCALL_END
> +	b.lt	3f
> +2:
> +	mov	x0, -ENOSYS
> +	eret
> +
> +3:
> +	/* Compute lm_alias() offset in x9 */
> +	ldr_l	x9, kimage_voffset
> +	ldr_l	x10, physvirt_offset
> +	add	x9, x9, x10
> +
> +	/* Find hcall function pointer in the table */
> +	ldr	x10, =kvm_hcall_table
> +	ksym_hyp_va	x10, x9

Can't kvm_hcall_table be referenced with adr or adr_l? It'd certainly
be nice to avoid the extra load for something that is essentially a
build-time constant.

Another thing that could be improved is to keep the lm_alias offset
somewhere, saving one load per entry. Not a huge deal.

> +	sub	x0, x0, #__KVM_HOST_HCALL_BASE
> +	add	x0, x10, x0, lsl 3

The usual convention for immediate is to prefix them with #.

> +	ldr	x0, [x0]
> +	ksym_hyp_va	x0, x9
> +
>  	/*
>  	 * Perform the EL2 call
>  	 */
> -	kern_hyp_va	x0
>  	do_el2_call
>  
>  	eret
> -- 
> 2.26.1
> 
> 

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.