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@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.
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
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
