| Message ID | 20200430144831.59194-3-dbrazdil@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Split off nVHE hyp code | expand |
Hi David, Quentin, On Thu, 30 Apr 2020 15:48:18 +0100, David Brazdil <dbrazdil@google.com> wrote: > > From: Quentin Perret <qperret@google.com> > > 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@google.com> > Signed-off-by: David Brazdil <dbrazdil@google.com> > --- > 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.
Hey Marc, On Thursday 07 May 2020 at 14:10:30 (+0100), Marc Zyngier wrote: > Hi David, Quentin, > > On Thu, 30 Apr 2020 15:48:18 +0100, > David Brazdil <dbrazdil@google.com> wrote: > > > > From: Quentin Perret <qperret@google.com> > > > > 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. Argh! Right... This should have been 'removing the /host/ code' :-) <snip> > > 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). This is essentially inspired from the various 'unistd.h' files we have in the kernel, e.g. include/uapi/asm-generic/unistd.h, which have exactly this type of construct. So, this was really just for consistency, but no strong opinion from me. > > > + > > +/* 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). Oh, OK. I assumed anything above HVC_STUB_HCALL_NR would be fine. But yes, this offset is really arbitrary, so if 0x1000 is better then that totally works. For my education, where is that 0x1000 coming from ? > 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! ;-) Note that the array's length is __KVM_HOST_HCALL_TABLE_IDX_SIZE, which doesn't include the offset, so we shouldn't be wasting memory here. > > + > > +/* 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. Right, and that again is inspired from the arm64 syscall table (see arch/arm64/kernel/sys.c). So the first intention was to keep things consistent. But again, no strong opinion :) > > 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. Ack, this isn't needed just yet so we should split that out. > > > 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. In fact David already has a nice patch that transforms the whole thing in a jump table, which is much nicer. I'll let him share the details :) > Another thing that could be improved is to keep the lm_alias offset > somewhere, saving one load per entry. Not a huge deal. Ack. > > + sub x0, x0, #__KVM_HOST_HCALL_BASE > > + add x0, x10, x0, lsl 3 > > The usual convention for immediate is to prefix them with #. Noted, thanks. > > + ldr x0, [x0] > > + ksym_hyp_va x0, x9 > > + > > /* > > * Perform the EL2 call > > */ > > - kern_hyp_va x0 > > do_el2_call > > > > eret > > -- > > 2.26.1 > > > > Thanks for the review! Quentin
Hi Quentin, On Thu, 07 May 2020 14:33:20 +0100, Quentin Perret <qperret@google.com> wrote: > > Hey Marc, > > On Thursday 07 May 2020 at 14:10:30 (+0100), Marc Zyngier wrote: > > Hi David, Quentin, [...] > > > +#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). > > This is essentially inspired from the various 'unistd.h' files we have > in the kernel, e.g. include/uapi/asm-generic/unistd.h, which have > exactly this type of construct. So, this was really just for consistency, > but no strong opinion from me. I think part of the of the reason for not using an enum in the syscall code is that part of is is (was?) used from assembly code. In our case, I'm pretty sure we won't go back to handling calls from asm any time soon, so a generated enum (and associated function pointer array) would be better. > > > > > > + > > > +/* 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). > > Oh, OK. I assumed anything above HVC_STUB_HCALL_NR would be fine. But > yes, this offset is really arbitrary, so if 0x1000 is better then that > totally works. For my education, where is that 0x1000 coming from ? We assumed that we wouldn't get a function pointer in the first 4kB, and documented as such in hyp.S. I would say that either we keep the current convention of leaving the first 4k function codes for the hyp-stubs, or we drop any sort of gap. Another thing to consider is that there is at least *one* external hypervisor (Jailhouse) that uses the stubs, so I'd like to make sure we don't break that (even if we made no promise whatsoever in that respect). > > > 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! ;-) > > Note that the array's length is __KVM_HOST_HCALL_TABLE_IDX_SIZE, which > doesn't include the offset, so we shouldn't be wasting memory here. Ah, you're right. I got confused between _SIZE and _END. > > > > + > > > +/* 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. > > Right, and that again is inspired from the arm64 syscall table (see > arch/arm64/kernel/sys.c). So the first intention was to keep things > consistent. But again, no strong opinion :) So let's try to build it with an enum instead, and see if it works. If it doesn't, at least we will know why. [...] > > > + /* 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. > > In fact David already has a nice patch that transforms the whole thing > in a jump table, which is much nicer. I'll let him share the details > :) Ah! Looking forward to reviewing it then! Thanks, M.
> > In fact David already has a nice patch that transforms the whole thing > > in a jump table, which is much nicer. I'll let him share the details > > :) > > Ah! Looking forward to reviewing it then! It's not actually that different. It still has the same header file, just uses the macros to generate a jump table rather than an array of function pointers. The main advantage being that we can avoid .hyp.text dependency on physvirt_offset. Feel free to have a look, branch 'topic/el2-obj-wip' at: https://android-kvm.googlesource.com/linux Perhaps this is not worth the trouble. We do hope to get to a point where the ABI between .text and .hyp.text is formalized, but in my mind that ABI is unlikely to be using this same hypcall path. On the other hand, I've played with preserving the function-pointer interface in the last couple of days and later in this series we do end up having to declare all of the hcall entry points (which now have two ELF symbols), so we end up with a similar table regardless, just with no IDs assigned. -David
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 + +/* 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) + +/* 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> +}; 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 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 + sub x0, x0, #__KVM_HOST_HCALL_BASE + add x0, x10, x0, lsl 3 + ldr x0, [x0] + ksym_hyp_va x0, x9 + /* * Perform the EL2 call */ - kern_hyp_va x0 do_el2_call eret