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

[Marc Zyngier]

Hi Quentin,

On Thu, 07 May 2020 14:33:20 +0100,
Quentin Perret <qperret@xxxxxxxxxx> 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.

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