This patchset 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.
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");
}
Cc: Paolo Bonzini <[email protected]>
Cc: Radim Krčmář <[email protected]>
Cc: Andrew Cooper <[email protected]>
Wanpeng Li (2):
KVM: VMX: Introduce handle_ud()
KVM: VMX: Add Force Emulation Prefix for "emulate the next instruction"
arch/x86/kvm/vmx.c | 38 ++++++++++++++++++++++++++++++--------
1 file changed, 30 insertions(+), 8 deletions(-)
--
2.7.4
From: Wanpeng Li <[email protected]>
Introduce handle_ud() to handle invalid opcode, this function will be
used by later patches.
Cc: Paolo Bonzini <[email protected]>
Cc: Radim Krčmář <[email protected]>
Cc: Andrew Cooper <[email protected]>
Signed-off-by: Wanpeng Li <[email protected]>
---
arch/x86/kvm/vmx.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 9bc05f5..0f99833 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -6215,6 +6215,18 @@ static int handle_machine_check(struct kvm_vcpu *vcpu)
return 1;
}
+static int handle_ud(struct kvm_vcpu *vcpu)
+{
+ enum emulation_result er;
+
+ er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
+ if (er == EMULATE_USER_EXIT)
+ return 0;
+ if (er != EMULATE_DONE)
+ kvm_queue_exception(vcpu, UD_VECTOR);
+ return 1;
+}
+
static int handle_exception(struct kvm_vcpu *vcpu)
{
struct vcpu_vmx *vmx = to_vmx(vcpu);
@@ -6233,14 +6245,8 @@ static int handle_exception(struct kvm_vcpu *vcpu)
if (is_nmi(intr_info))
return 1; /* already handled by vmx_vcpu_run() */
- if (is_invalid_opcode(intr_info)) {
- er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
- if (er == EMULATE_USER_EXIT)
- return 0;
- if (er != EMULATE_DONE)
- kvm_queue_exception(vcpu, UD_VECTOR);
- return 1;
- }
+ if (is_invalid_opcode(intr_info))
+ return handle_ud(vcpu);
error_code = 0;
if (intr_info & INTR_INFO_DELIVER_CODE_MASK)
--
2.7.4
From: Wanpeng Li <[email protected]>
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.
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 <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: Radim Krčmář <[email protected]>
Cc: Andrew Cooper <[email protected]>
Signed-off-by: Wanpeng Li <[email protected]>
---
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);
+
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
On Mon, Mar 26, 2018 at 07:12:14PM -0700, Wanpeng Li wrote:
> From: Wanpeng Li <[email protected]>
>
> Introduce handle_ud() to handle invalid opcode, this function will be
> used by later patches.
>
Reviewed-by: Konrad Rzeszutek Wilk <[email protected]>
Thank you!
> Cc: Paolo Bonzini <[email protected]>
> Cc: Radim Krčmář <[email protected]>
> Cc: Andrew Cooper <[email protected]>
> Signed-off-by: Wanpeng Li <[email protected]>
> ---
> arch/x86/kvm/vmx.c | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 9bc05f5..0f99833 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -6215,6 +6215,18 @@ static int handle_machine_check(struct kvm_vcpu *vcpu)
> return 1;
> }
>
> +static int handle_ud(struct kvm_vcpu *vcpu)
> +{
> + enum emulation_result er;
> +
> + er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
> + if (er == EMULATE_USER_EXIT)
> + return 0;
> + if (er != EMULATE_DONE)
> + kvm_queue_exception(vcpu, UD_VECTOR);
> + return 1;
> +}
> +
> static int handle_exception(struct kvm_vcpu *vcpu)
> {
> struct vcpu_vmx *vmx = to_vmx(vcpu);
> @@ -6233,14 +6245,8 @@ static int handle_exception(struct kvm_vcpu *vcpu)
> if (is_nmi(intr_info))
> return 1; /* already handled by vmx_vcpu_run() */
>
> - if (is_invalid_opcode(intr_info)) {
> - er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
> - if (er == EMULATE_USER_EXIT)
> - return 0;
> - if (er != EMULATE_DONE)
> - kvm_queue_exception(vcpu, UD_VECTOR);
> - return 1;
> - }
> + if (is_invalid_opcode(intr_info))
> + return handle_ud(vcpu);
>
> error_code = 0;
> if (intr_info & INTR_INFO_DELIVER_CODE_MASK)
> --
> 2.7.4
>
On Mon, Mar 26, 2018 at 07:12:15PM -0700, Wanpeng Li wrote:
> From: Wanpeng Li <[email protected]>
>
> 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.
Can you expand a bit ? Why do you want this in KVM in the first place?
Should this be controlled by a boolean parameter?
>
> 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 <[email protected]>
> Cc: Paolo Bonzini <[email protected]>
> Cc: Radim Krčmář <[email protected]>
> Cc: Andrew Cooper <[email protected]>
> Signed-off-by: Wanpeng Li <[email protected]>
> ---
> 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);
> +
> 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;
Don't you want to do = { };
to memset it?
> +
> + 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
>
On Tue, Mar 27, 2018 at 12:40:20AM -0400, Konrad Rzeszutek Wilk wrote:
> On Mon, Mar 26, 2018 at 07:12:15PM -0700, Wanpeng Li wrote:
> > From: Wanpeng Li <[email protected]>
> >
> > 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.
>
> Can you expand a bit ? Why do you want this in KVM in the first place?
> Should this be controlled by a boolean parameter?
.. per guest. That is instead of a global one, have a per guest one?
2018-03-27 12:55 GMT+08:00 Konrad Rzeszutek Wilk <[email protected]>:
> On Tue, Mar 27, 2018 at 12:40:20AM -0400, Konrad Rzeszutek Wilk wrote:
>> On Mon, Mar 26, 2018 at 07:12:15PM -0700, Wanpeng Li wrote:
>> > From: Wanpeng Li <[email protected]>
>> >
>> > 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.
>>
>> Can you expand a bit ? Why do you want this in KVM in the first place?
Please refer to the original discussion(Force Emulation Prefix part).
https://lkml.org/lkml/2018/3/22/220
>> Should this be controlled by a boolean parameter?
>
> .. per guest. That is instead of a global one, have a per guest one?
As Paolo pointed out offline:
> Testing without the hacks being done by emulator.flat (TLB mismatch between instructions and data).
I think a global module is enough for testing.
Regards,
Wanpeng Li
2018-03-27 12:40 GMT+08:00 Konrad Rzeszutek Wilk <[email protected]>:
> On Mon, Mar 26, 2018 at 07:12:15PM -0700, Wanpeng Li wrote:
>> From: Wanpeng Li <[email protected]>
>>
>> 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.
>
> Can you expand a bit ? Why do you want this in KVM in the first place?
> Should this be controlled by a boolean parameter?
>>
>> 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 <[email protected]>
>> Cc: Paolo Bonzini <[email protected]>
>> Cc: Radim Krčmář <[email protected]>
>> Cc: Andrew Cooper <[email protected]>
>> Signed-off-by: Wanpeng Li <[email protected]>
>> ---
>> 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);
>> +
>> 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;
>
> Don't you want to do = { };
> to memset it?
sig buffer will be filled by insns which are fetched from guest
memory, so I think not memset is fine.
Regards,
Wanpeng Li
On Tue, Mar 27, 2018 at 01:03:15PM +0800, Wanpeng Li wrote:
> 2018-03-27 12:55 GMT+08:00 Konrad Rzeszutek Wilk <[email protected]>:
> > On Tue, Mar 27, 2018 at 12:40:20AM -0400, Konrad Rzeszutek Wilk wrote:
> >> On Mon, Mar 26, 2018 at 07:12:15PM -0700, Wanpeng Li wrote:
> >> > From: Wanpeng Li <[email protected]>
> >> >
> >> > 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.
> >>
> >> Can you expand a bit ? Why do you want this in KVM in the first place?
>
> Please refer to the original discussion(Force Emulation Prefix part).
> https://lkml.org/lkml/2018/3/22/220
That really should be part massaged in this patch as part of the description.
>
> >> Should this be controlled by a boolean parameter?
> >
> > .. per guest. That is instead of a global one, have a per guest one?
>
> As Paolo pointed out offline:
> > Testing without the hacks being done by emulator.flat (TLB mismatch between instructions and data).
And also this above.
> I think a global module is enough for testing.
If so, perhaps have it wrapped with #ifdef DEBUG?
No need to put code gadgets that won't be utilized 99% of time.
>
> Regards,
> Wanpeng Li
On 27/03/2018 07:18, Konrad Rzeszutek Wilk wrote:
>> I think a global module is enough for testing.
> If so, perhaps have it wrapped with #ifdef DEBUG?
>
> No need to put code gadgets that won't be utilized 99% of time.
It is going to be used a lot in testing, actually. There are quite a
few emulate.c bugfixes that are hard to test (see for example the
syscall eflags.tf bug that can currently be tested only on Intel) and
having something like this can only improve our coverage.
#ifdef DEBUG is almost always a bad idea, and though I agree that the
commit messages can be improved, I think the module parameter is the way
to go.
Wanpeng, can you also add it to svm.c?
Thanks,
Paolo
2018-03-27 15:25 GMT+08:00 Paolo Bonzini <[email protected]>:
> On 27/03/2018 07:18, Konrad Rzeszutek Wilk wrote:
>>> I think a global module is enough for testing.
>> If so, perhaps have it wrapped with #ifdef DEBUG?
>>
>> No need to put code gadgets that won't be utilized 99% of time.
>
> It is going to be used a lot in testing, actually. There are quite a
> few emulate.c bugfixes that are hard to test (see for example the
> syscall eflags.tf bug that can currently be tested only on Intel) and
> having something like this can only improve our coverage.
>
> #ifdef DEBUG is almost always a bad idea, and though I agree that the
> commit messages can be improved, I think the module parameter is the way
> to go.
>
> Wanpeng, can you also add it to svm.c?
Yeah, thanks for the review. :)
Regards,
Wanpeng Li
----- [email protected] wrote:
> From: Wanpeng Li <[email protected]>
>
> Introduce handle_ud() to handle invalid opcode, this function will be
>
> used by later patches.
>
> Cc: Paolo Bonzini <[email protected]>
> Cc: Radim Krčmář <[email protected]>
> Cc: Andrew Cooper <[email protected]>
> Signed-off-by: Wanpeng Li <[email protected]>
> ---
> arch/x86/kvm/vmx.c | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 9bc05f5..0f99833 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -6215,6 +6215,18 @@ static int handle_machine_check(struct kvm_vcpu
> *vcpu)
> return 1;
> }
>
> +static int handle_ud(struct kvm_vcpu *vcpu)
> +{
> + enum emulation_result er;
> +
> + er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
> + if (er == EMULATE_USER_EXIT)
> + return 0;
> + if (er != EMULATE_DONE)
> + kvm_queue_exception(vcpu, UD_VECTOR);
> + return 1;
> +}
> +
> static int handle_exception(struct kvm_vcpu *vcpu)
> {
> struct vcpu_vmx *vmx = to_vmx(vcpu);
> @@ -6233,14 +6245,8 @@ static int handle_exception(struct kvm_vcpu
> *vcpu)
> if (is_nmi(intr_info))
> return 1; /* already handled by vmx_vcpu_run() */
>
> - if (is_invalid_opcode(intr_info)) {
> - er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
> - if (er == EMULATE_USER_EXIT)
> - return 0;
> - if (er != EMULATE_DONE)
> - kvm_queue_exception(vcpu, UD_VECTOR);
> - return 1;
> - }
> + if (is_invalid_opcode(intr_info))
> + return handle_ud(vcpu);
>
> error_code = 0;
> if (intr_info & INTR_INFO_DELIVER_CODE_MASK)
> --
> 2.7.4
Reviewed-By: Liran Alon <[email protected]>
----- [email protected] wrote:
> From: Wanpeng Li <[email protected]>
>
> 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.
>
> 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 <[email protected]>
> Cc: Paolo Bonzini <[email protected]>
> Cc: Radim Krčmář <[email protected]>
> Cc: Andrew Cooper <[email protected]>
> Signed-off-by: Wanpeng Li <[email protected]>
> ---
> 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
On 27/03/2018 09:52, Liran Alon wrote:
>> +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
Agreed.
> 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)
If you can move handle_ud to x86.c, then it makes sense to have the
module parameter in the kvm module. I haven't checked.
Otherwise you would have to EXPORT_SYMBOL_GPL the variable; in the end
it's just a debugging tool, so it'd be simpler to just add it separately
to kvm_intel and kvm_amd.
Paolo
----- [email protected] wrote:
> On 27/03/2018 09:52, Liran Alon wrote:
> > 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)
>
> If you can move handle_ud to x86.c, then it makes sense to have the
> module parameter in the kvm module. I haven't checked.
I don't see a reason why you couldn't do that.
>
> Otherwise you would have to EXPORT_SYMBOL_GPL the variable; in the
This is what I did for enable_vmware_backdoor module parameter.
I think this is what should be done in this case as-well.
> end
> it's just a debugging tool, so it'd be simpler to just add it
> separately
> to kvm_intel and kvm_amd.
I agree it's just a debugging tool. But no reason for it to be used differently
when running tests on Intel CPU vs. AMD CPU.
I think the effort to fix this is low.
>
> Paolo
2018-03-27 15:52 GMT+08:00 Liran Alon <[email protected]>:
>
> ----- [email protected] wrote:
>
>> From: Wanpeng Li <[email protected]>
>>
>> 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 <[email protected]>
>> Cc: Paolo Bonzini <[email protected]>
>> Cc: Radim Krčmář <[email protected]>
>> Cc: Andrew Cooper <[email protected]>
>> Signed-off-by: Wanpeng Li <[email protected]>
>> ---
>> 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
What you are essentially trying to do is create a PV interface to access
the x86 emulator.
Why not use a simple hypercall (VMCALL) to accomplish this instead of
inventing yet another PV method?
Something like “KVM_HC_EMULATE_NEXT_INSTRUCTION” in kvm_emulate_hypercall
should do the trick (however it needs to be placed before the check for
CPL>0 so that user mode code can test the emulator too).
Nikita
> On 27 Mar 2018, at 11:26, Liran Alon <[email protected]> wrote:
>
>
> ----- [email protected] wrote:
>
>> On 27/03/2018 09:52, Liran Alon wrote:
>>> 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)
>>
>> If you can move handle_ud to x86.c, then it makes sense to have the
>> module parameter in the kvm module. I haven't checked.
>
> I don't see a reason why you couldn't do that.
>
>>
>> Otherwise you would have to EXPORT_SYMBOL_GPL the variable; in the
>
> This is what I did for enable_vmware_backdoor module parameter.
> I think this is what should be done in this case as-well.
>
>> end
>> it's just a debugging tool, so it'd be simpler to just add it
>> separately
>> to kvm_intel and kvm_amd.
>
> I agree it's just a debugging tool. But no reason for it to be used differently
> when running tests on Intel CPU vs. AMD CPU.
> I think the effort to fix this is low.
>
>>
>> Paolo
>
>
On 27/03/2018 11:05, Nikita Leshenko wrote:
> What you are essentially trying to do is create a PV interface to access
> the x86 emulator.
> Why not use a simple hypercall (VMCALL) to accomplish this instead of
> inventing yet another PV method?
Because hypercalls force you to use %rax for the hypercall number.
Paolo
> Something like “KVM_HC_EMULATE_NEXT_INSTRUCTION” in kvm_emulate_hypercall
> should do the trick (however it needs to be placed before the check for
> CPL>0 so that user mode code can test the emulator too).
>
> Nikita