Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
Message-ID: <76d08635-59da-4399-bc35-f99a00236725@default>
Date:   Tue, 27 Mar 2018 00:52:41 -0700 (PDT)
From:   Liran Alon <liran.alon@oracle.com>
To:     <kernellwp@gmail.com>
Cc:     <rkrcmar@redhat.com>, <pbonzini@redhat.com>,
        <linux-kernel@vger.kernel.org>, <andrew.cooper3@citrix.com>,
        <kvm@vger.kernel.org>
Subject: Re: [PATCH 2/2] KVM: VMX: Add Force Emulation Prefix for "emulate the
 next instruction"
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


----- kernellwp@gmail.com wrote:

> From: Wanpeng Li <wanpengli@tencent.com>
>=20
> This patch introduces a Force Emulation Prefix (ud2a; .ascii "kvm")
> for=20
> "emulate the next instruction", the codes will be executed by emulator
>=20
> instead of processor, for testing purposes.

I think this should be better explained in commit message.
We should explain that there is no easy way to force KVM to run an
instruction through the emulator (by design as that will expose the
x86 emulator as a significant attack-surface).
However, we do wish to expose the x86 emulator in case we are testing it
(e.g. via kvm-unit-tests). Therefore, this patch adds a "force emulation pr=
efix"
that is designed to raise #UD which KVM will trap and it's #UD exit-handler=
 will
match "force emulation prefix" to run instruction after prefix by the x86 e=
mulator.
To not expose the x86 emulator by default, we add a module parameter that s=
hould be
off by default.

> =20
> A testcase here:
>=20
> #include <stdio.h>
> #include <string.h>
>   =20
> #define HYPERVISOR_INFO 0x40000000
>   =20
> #define CPUID(idx, eax, ebx, ecx, edx)\
>     asm volatile (\
>     "ud2a; .ascii \"kvm\"; 1: cpuid" \
>     :"=3Db" (*ebx), "=3Da" (*eax),"=3Dc" (*ecx), "=3Dd" (*edx)\
>         :"0"(idx) ); =20
>   =20
> void main() =20
> { =20
> =09unsigned int eax,ebx,ecx,edx; =20
> =09char string[13]; =20
>   =20
> =09CPUID(HYPERVISOR_INFO, &eax, &ebx, &ecx, &edx); =20
> =09*(unsigned int *)(string+0) =3D ebx; =20
> =09*(unsigned int *)(string+4) =3D ecx; =20
> =09*(unsigned int *)(string+8) =3D edx; =20
>   =20
> =09string[12] =3D 0; =20
> =09if (strncmp(string, "KVMKVMKVM\0\0\0",12) =3D=3D 0)
> =09=09printf("kvm guest\n"); =20
> =09else =20
> =09=09printf("bare hardware\n"); =20
> }
>=20
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Kr=C4=8Dm=C3=A1=C5=99 <rkrcmar@redhat.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> ---
>  arch/x86/kvm/vmx.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>=20
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 0f99833..90abed8 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -108,6 +108,9 @@ module_param_named(enable_shadow_vmcs,
> enable_shadow_vmcs, bool, S_IRUGO);
>  static bool __read_mostly nested =3D 0;
>  module_param(nested, bool, S_IRUGO);
> =20
> +static bool __read_mostly fep =3D 0;
> +module_param(fep, bool, S_IRUGO);

I think this module parameter should have a better name...
Why not "emulation_prefix" or "enable_emulation_prefix"?
This short names just confuse the average user.
It makes him think it is some kind of Intel VT-x technology
that he isn't aware of :P

In addition, I think this module parameter should be in kvm module
(not kvm_intel) and you should add similar logic to kvm_amd module (SVM)

> +
>  static u64 __read_mostly host_xss;
> =20
>  static bool __read_mostly enable_pml =3D 1;
> @@ -6218,8 +6221,21 @@ static int handle_machine_check(struct kvm_vcpu
> *vcpu)
>  static int handle_ud(struct kvm_vcpu *vcpu)
>  {
>  =09enum emulation_result er;
> +=09int emulation_type =3D EMULTYPE_TRAP_UD;
> +
> +=09if (fep) {
> +=09=09char sig[5]; /* ud2; .ascii "kvm" */
> +=09=09struct x86_exception e;
> +
> +=09=09kvm_read_guest_virt(&vcpu->arch.emulate_ctxt,
> +=09=09=09=09kvm_get_linear_rip(vcpu), sig, sizeof(sig), &e);
> +=09=09if (memcmp(sig, "\xf\xbkvm", sizeof(sig)) =3D=3D 0) {
> +=09=09=09emulation_type =3D 0;
> +=09=09=09kvm_rip_write(vcpu, kvm_rip_read(vcpu) + sizeof(sig));
> +=09=09}
> +=09}
> =20
> -=09er =3D emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
> +=09er =3D emulate_instruction(vcpu, emulation_type);
>  =09if (er =3D=3D EMULATE_USER_EXIT)
>  =09=09return 0;
>  =09if (er !=3D EMULATE_DONE)
> --=20
> 2.7.4