2018-03-27 15:52 GMT+08:00 Liran Alon <liran.alon@xxxxxxxxxx>: > > ----- kernellwp@xxxxxxxxx wrote: > >> From: Wanpeng Li <wanpengli@xxxxxxxxxxx> >> >> This patch introduces a Force Emulation Prefix (ud2a; .ascii "kvm") >> for >> "emulate the next instruction", the codes will be executed by emulator >> >> 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 prefix" > 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 emulator. > To not expose the x86 emulator by default, we add a module parameter that should be > off by default. This commit message looks good, I'm too lazy to write a new one, will reference. :) Regards, Wanpeng Li > >> >> A testcase here: >> >> #include <stdio.h> >> #include <string.h> >> >> #define HYPERVISOR_INFO 0x40000000 >> >> #define CPUID(idx, eax, ebx, ecx, edx)\ >> asm volatile (\ >> "ud2a; .ascii \"kvm\"; 1: cpuid" \ >> :"=b" (*ebx), "=a" (*eax),"=c" (*ecx), "=d" (*edx)\ >> :"0"(idx) ); >> >> void main() >> { >> unsigned int eax,ebx,ecx,edx; >> char string[13]; >> >> CPUID(HYPERVISOR_INFO, &eax, &ebx, &ecx, &edx); >> *(unsigned int *)(string+0) = ebx; >> *(unsigned int *)(string+4) = ecx; >> *(unsigned int *)(string+8) = edx; >> >> string[12] = 0; >> if (strncmp(string, "KVMKVMKVM\0\0\0",12) == 0) >> printf("kvm guest\n"); >> else >> printf("bare hardware\n"); >> } >> >> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> >> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx> >> Cc: Radim Krčmář <rkrcmar@xxxxxxxxxx> >> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> >> Signed-off-by: Wanpeng Li <wanpengli@xxxxxxxxxxx> >> --- >> arch/x86/kvm/vmx.c | 18 +++++++++++++++++- >> 1 file changed, 17 insertions(+), 1 deletion(-) >> >> 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 = 0; >> module_param(nested, bool, S_IRUGO); >> >> +static bool __read_mostly fep = 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; >> >> static bool __read_mostly enable_pml = 1; >> @@ -6218,8 +6221,21 @@ static int handle_machine_check(struct kvm_vcpu >> *vcpu) >> static int handle_ud(struct kvm_vcpu *vcpu) >> { >> enum emulation_result er; >> + int emulation_type = EMULTYPE_TRAP_UD; >> + >> + if (fep) { >> + char sig[5]; /* ud2; .ascii "kvm" */ >> + struct x86_exception e; >> + >> + kvm_read_guest_virt(&vcpu->arch.emulate_ctxt, >> + kvm_get_linear_rip(vcpu), sig, sizeof(sig), &e); >> + if (memcmp(sig, "\xf\xbkvm", sizeof(sig)) == 0) { >> + emulation_type = 0; >> + kvm_rip_write(vcpu, kvm_rip_read(vcpu) + sizeof(sig)); >> + } >> + } >> >> - er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD); >> + er = emulate_instruction(vcpu, emulation_type); >> if (er == EMULATE_USER_EXIT) >> return 0; >> if (er != EMULATE_DONE) >> -- >> 2.7.4