Re: [RFC 05/33] KVM: x86: hyper-v: Introduce VTL call/return prologues in hypercall page

[Nicolas Saenz Julienne <nsaenz@amazon.com>]

On Tue Nov 28, 2023 at 7:08 AM UTC, Maxim Levitsky wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On Wed, 2023-11-08 at 11:17 +0000, Nicolas Saenz Julienne wrote:
> > VTL call/return hypercalls have their own entry points in the hypercall
> > page because they don't follow normal hyper-v hypercall conventions.
> > Move the VTL call/return control input into ECX/RAX and set the
> > hypercall code into EAX/RCX before calling the hypercall instruction in
> > order to be able to use the Hyper-V hypercall entry function.
> >
> > Guests can read an emulated code page offsets register to know the
> > offsets into the hypercall page for the VTL call/return entries.
> >
> > Signed-off-by: Nicolas Saenz Julienne <nsaenz@amazon.com>
> >
> > ---
> >
> > My tree has the additional patch, we're still trying to understand under
> > what conditions Windows expects the offset to be fixed.
> >
> > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> > index 54f7f36a89bf..9f2ea8c34447 100644
> > --- a/arch/x86/kvm/hyperv.c
> > +++ b/arch/x86/kvm/hyperv.c
> > @@ -294,6 +294,7 @@ static int patch_hypercall_page(struct kvm_vcpu *vcpu, u64 data)
> >
> >         /* VTL call/return entries */
> >         if (!kvm_xen_hypercall_enabled(kvm) && kvm_hv_vsm_enabled(kvm)) {
> > +               i = 22;
> >  #ifdef CONFIG_X86_64
> >                 if (is_64_bit_mode(vcpu)) {
> >                         /*
> > ---
> >  arch/x86/include/asm/kvm_host.h   |  2 +
> >  arch/x86/kvm/hyperv.c             | 78 ++++++++++++++++++++++++++++++-
> >  include/asm-generic/hyperv-tlfs.h | 11 +++++
> >  3 files changed, 90 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index a2f224f95404..00cd21b09f8c 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1105,6 +1105,8 @@ struct kvm_hv {
> >       u64 hv_tsc_emulation_status;
> >       u64 hv_invtsc_control;
> >
> > +     union hv_register_vsm_code_page_offsets vsm_code_page_offsets;
> > +
> >       /* How many vCPUs have VP index != vCPU index */
> >       atomic_t num_mismatched_vp_indexes;
> >
> > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> > index 78d053042667..d4b1b53ea63d 100644
> > --- a/arch/x86/kvm/hyperv.c
> > +++ b/arch/x86/kvm/hyperv.c
> > @@ -259,7 +259,8 @@ static void synic_exit(struct kvm_vcpu_hv_synic *synic, u32 msr)
> >  static int patch_hypercall_page(struct kvm_vcpu *vcpu, u64 data)
> >  {
> >       struct kvm *kvm = vcpu->kvm;
> > -     u8 instructions[9];
> > +     struct kvm_hv *hv = to_kvm_hv(kvm);
> > +     u8 instructions[0x30];
> >       int i = 0;
> >       u64 addr;
> >
> > @@ -285,6 +286,81 @@ static int patch_hypercall_page(struct kvm_vcpu *vcpu, u64 data)
> >       /* ret */
> >       ((unsigned char *)instructions)[i++] = 0xc3;
> >
> > +     /* VTL call/return entries */
> > +     if (!kvm_xen_hypercall_enabled(kvm) && kvm_hv_vsm_enabled(kvm)) {
> > +#ifdef CONFIG_X86_64
> > +             if (is_64_bit_mode(vcpu)) {
> > +                     /*
> > +                      * VTL call 64-bit entry prologue:
> > +                      *      mov %rcx, %rax
> > +                      *      mov $0x11, %ecx
> > +                      *      jmp 0:
>
> This isn't really 'jmp 0' as I first wondered but actually backward jump 32 bytes back (if I did the calculation correctly).
> This is very dangerous because code that was before can change and in fact I don't think that this
> offset is even correct now, and on top of that it depends on support for xen hypercalls as well.

You're absolutely right. The offset is wrong as is, and the overall
approach might break in the future.

Another solution is to explicitly do the vmcall and avoid any jumps.
This seems to be what Hyper-V does:
https://hal.science/hal-03117362/document (Figure 8).

> This can be fixed by calculating the offset in runtime, however I am thinking:
>
>
> Since userspace will have to be aware of the offsets in this page, and since
> pretty much everything else is done in userspace, it might make sense to create
> the hypercall page in the userspace.
>
> In fact, the fact that KVM currently overwrites the guest page, is a violation of
> the HV spec.
>
> It's more correct regardless of VTL to do userspace vm exit and let the userspace put a memslot ("overlay")
> over the address, and put whatever userspace wants there, including the above code.

I agree we should be on the safe side and fully implement overlays. That
said, I suspect they are not actually necessary in practice (with or
without VSM support).

> Then we won't need the new ioctl as well.
>
> To support this I think that we can add a userspace msr filter on the HV_X64_MSR_HYPERCALL,
> although I am not 100% sure if a userspace msr filter overrides the in-kernel msr handling.

I thought about it at the time. It's not that simple though, we should
still let KVM set the hypercall bytecode, and other quirks like the Xen
one. Additionally, we have no way of knowing where they are going to be
located. We could do something like this, but it's not pretty:

  - Exit to user-space on HV_X64_MSR_HYPERCALL (it overrides the msr
    handling).
  - Setup the overlay.
  - Call HV_X64_MSR_HYPERCALL from user-space so KVM writes its share of
    the hypercall page.
  - Copy the VSM parts from user-space in an area we know to be safe.

We could maybe introduce and extension CAP that provides a safe offset
in the hypercall page for user-space to use?

Nicolas