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 simple testcase here:
#include <stdio.h>
#include <string.h>
#define HYPERVISOR_INFO 0x40000000
#define CPUID(idx, eax, ebx, ecx, edx) \
asm volatile ( \
"ud2a; .ascii \"kvm\"; 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");
}
v3 -> v4:
* forwarding emulation failure to userspace
v2 -> v3:
* fix compile warning
v1 -> v2:
* update patch descriptions
* move handle_ud to x86.c, shared by vmx and svm
* the parameter is in kvm module
* rename parameter to force_emulation_prefix
Cc: Paolo Bonzini <[email protected]>
Cc: Radim Krčmář <[email protected]>
Cc: Andrew Cooper <[email protected]>
Cc: Konrad Rzeszutek Wilk <[email protected]>
Cc: Liran Alon <[email protected]>
Wanpeng Li (2):
KVM: X86: Introduce handle_ud()
KVM: X86: Add Force Emulation Prefix for "emulate the next instruction"
arch/x86/kvm/svm.c | 9 +--------
arch/x86/kvm/vmx.c | 10 ++--------
arch/x86/kvm/x86.c | 31 +++++++++++++++++++++++++++++++
arch/x86/kvm/x86.h | 2 ++
4 files changed, 36 insertions(+), 16 deletions(-)
--
2.7.4
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]>
Reviewed-by: Liran Alon <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: Radim Krčmář <[email protected]>
Cc: Andrew Cooper <[email protected]>
Cc: Konrad Rzeszutek Wilk <[email protected]>
Cc: Liran Alon <[email protected]>
Signed-off-by: Wanpeng Li <[email protected]>
---
arch/x86/kvm/svm.c | 9 +--------
arch/x86/kvm/vmx.c | 10 ++--------
arch/x86/kvm/x86.c | 13 +++++++++++++
arch/x86/kvm/x86.h | 2 ++
4 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index f66fc2e..e0a3f56 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2676,14 +2676,7 @@ static int bp_interception(struct vcpu_svm *svm)
static int ud_interception(struct vcpu_svm *svm)
{
- int er;
-
- er = emulate_instruction(&svm->vcpu, EMULTYPE_TRAP_UD);
- if (er == EMULATE_USER_EXIT)
- return 0;
- if (er != EMULATE_DONE)
- kvm_queue_exception(&svm->vcpu, UD_VECTOR);
- return 1;
+ return handle_ud(&svm->vcpu);
}
static int ac_interception(struct vcpu_svm *svm)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index b2f8a70..0f11243 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -6436,14 +6436,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)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 7d9a444..1eb495e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4840,6 +4840,19 @@ int kvm_write_guest_virt_system(struct x86_emulate_ctxt *ctxt,
}
EXPORT_SYMBOL_GPL(kvm_write_guest_virt_system);
+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;
+}
+EXPORT_SYMBOL_GPL(handle_ud);
+
static int vcpu_is_mmio_gpa(struct kvm_vcpu *vcpu, unsigned long gva,
gpa_t gpa, bool write)
{
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 1e86174..7d35ce6 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -255,6 +255,8 @@ int kvm_write_guest_virt_system(struct x86_emulate_ctxt *ctxt,
gva_t addr, void *val, unsigned int bytes,
struct x86_exception *exception);
+int handle_ud(struct kvm_vcpu *vcpu);
+
void kvm_vcpu_mtrr_init(struct kvm_vcpu *vcpu);
u8 kvm_mtrr_get_guest_memory_type(struct kvm_vcpu *vcpu, gfn_t gfn);
bool kvm_mtrr_valid(struct kvm_vcpu *vcpu, u32 msr, u64 data);
--
2.7.4
From: Wanpeng Li <[email protected]>
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 simple testcase here:
#include <stdio.h>
#include <string.h>
#define HYPERVISOR_INFO 0x40000000
#define CPUID(idx, eax, ebx, ecx, edx) \
asm volatile (\
"ud2a; .ascii \"kvm\"; 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]>
Reviewed-by: Radim Krčmář <[email protected]>
Reviewed-by: Liran Alon <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: Radim Krčmář <[email protected]>
Cc: Andrew Cooper <[email protected]>
Cc: Konrad Rzeszutek Wilk <[email protected]>
Cc: Liran Alon <[email protected]>
Signed-off-by: Wanpeng Li <[email protected]>
---
arch/x86/kvm/x86.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1eb495e..a55ecef 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -146,6 +146,9 @@ bool __read_mostly enable_vmware_backdoor = false;
module_param(enable_vmware_backdoor, bool, S_IRUGO);
EXPORT_SYMBOL_GPL(enable_vmware_backdoor);
+static bool __read_mostly force_emulation_prefix = false;
+module_param(force_emulation_prefix, bool, S_IRUGO);
+
#define KVM_NR_SHARED_MSRS 16
struct kvm_shared_msrs_global {
@@ -4844,6 +4847,21 @@ int handle_ud(struct kvm_vcpu *vcpu)
{
enum emulation_result er;
+ if (force_emulation_prefix) {
+ char sig[5]; /* ud2; .ascii "kvm" */
+ struct x86_exception e;
+
+ if (kvm_read_guest_virt(&vcpu->arch.emulate_ctxt,
+ kvm_get_linear_rip(vcpu), sig, sizeof(sig), &e))
+ goto emulate_ud;
+
+ if (memcmp(sig, "\xf\xbkvm", sizeof(sig)) == 0) {
+ kvm_rip_write(vcpu, kvm_rip_read(vcpu) + sizeof(sig));
+ return emulate_instruction(vcpu, 0) == EMULATE_DONE;
+ }
+ }
+
+emulate_ud:
er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
if (er == EMULATE_USER_EXIT)
return 0;
--
2.7.4
On 04.04.2018 01:28, 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]>
> Reviewed-by: Liran Alon <[email protected]>
> Cc: Paolo Bonzini <[email protected]>
> Cc: Radim Krčmář <[email protected]>
> Cc: Andrew Cooper <[email protected]>
> Cc: Konrad Rzeszutek Wilk <[email protected]>
> Cc: Liran Alon <[email protected]>
> Signed-off-by: Wanpeng Li <[email protected]>
> ---
> arch/x86/kvm/svm.c | 9 +--------
> arch/x86/kvm/vmx.c | 10 ++--------
> arch/x86/kvm/x86.c | 13 +++++++++++++
> arch/x86/kvm/x86.h | 2 ++
> 4 files changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index f66fc2e..e0a3f56 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2676,14 +2676,7 @@ static int bp_interception(struct vcpu_svm *svm)
>
> static int ud_interception(struct vcpu_svm *svm)
> {
> - int er;
> -
> - er = emulate_instruction(&svm->vcpu, EMULTYPE_TRAP_UD);
> - if (er == EMULATE_USER_EXIT)
> - return 0;
> - if (er != EMULATE_DONE)
> - kvm_queue_exception(&svm->vcpu, UD_VECTOR);
> - return 1;
> + return handle_ud(&svm->vcpu);
> }
>
> static int ac_interception(struct vcpu_svm *svm)
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index b2f8a70..0f11243 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -6436,14 +6436,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);
(maybe different on this branch) isn't "er" now unused?
>
> error_code = 0;
> if (intr_info & INTR_INFO_DELIVER_CODE_MASK)
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 7d9a444..1eb495e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4840,6 +4840,19 @@ int kvm_write_guest_virt_system(struct x86_emulate_ctxt *ctxt,
> }
> EXPORT_SYMBOL_GPL(kvm_write_guest_virt_system);
>
> +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;
I would now actually prefer
if (er == EMULATE_DONE)
return 1 ...
Anyhow,
Reviewed-by: David Hildenbrand <[email protected]>
--
Thanks,
David / dhildenb
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 1eb495e..a55ecef 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -146,6 +146,9 @@ bool __read_mostly enable_vmware_backdoor = false;
> module_param(enable_vmware_backdoor, bool, S_IRUGO);
> EXPORT_SYMBOL_GPL(enable_vmware_backdoor);
>
> +static bool __read_mostly force_emulation_prefix = false;
> +module_param(force_emulation_prefix, bool, S_IRUGO);
> +
> #define KVM_NR_SHARED_MSRS 16
>
> struct kvm_shared_msrs_global {
> @@ -4844,6 +4847,21 @@ int handle_ud(struct kvm_vcpu *vcpu)
> {
> enum emulation_result er;
>
> + if (force_emulation_prefix) {
> + char sig[5]; /* ud2; .ascii "kvm" */
> + struct x86_exception e;
> +
> + if (kvm_read_guest_virt(&vcpu->arch.emulate_ctxt,
> + kvm_get_linear_rip(vcpu), sig, sizeof(sig), &e))
> + goto emulate_ud;
> +
> + if (memcmp(sig, "\xf\xbkvm", sizeof(sig)) == 0) {
> + kvm_rip_write(vcpu, kvm_rip_read(vcpu) + sizeof(sig));
> + return emulate_instruction(vcpu, 0) == EMULATE_DONE;
What if we would have an invalid instruction here? Shouldn't you handle
the emulate_instruction() like below?
(e.g. keep a variable with the emulation type (0 vs EMULTYPE_TRAP_UD)
and reuse emulate_ud below)
> + }
> + }
> +
> +emulate_ud:
> er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
> if (er == EMULATE_USER_EXIT)
> return 0;
>
--
Thanks,
David / dhildenb
2018-04-04 19:54 GMT+08:00 David Hildenbrand <[email protected]>:
> On 04.04.2018 01:28, 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]>
>> Reviewed-by: Liran Alon <[email protected]>
>> Cc: Paolo Bonzini <[email protected]>
>> Cc: Radim Krčmář <[email protected]>
>> Cc: Andrew Cooper <[email protected]>
>> Cc: Konrad Rzeszutek Wilk <[email protected]>
>> Cc: Liran Alon <[email protected]>
>> Signed-off-by: Wanpeng Li <[email protected]>
>> ---
>> arch/x86/kvm/svm.c | 9 +--------
>> arch/x86/kvm/vmx.c | 10 ++--------
>> arch/x86/kvm/x86.c | 13 +++++++++++++
>> arch/x86/kvm/x86.h | 2 ++
>> 4 files changed, 18 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index f66fc2e..e0a3f56 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -2676,14 +2676,7 @@ static int bp_interception(struct vcpu_svm *svm)
>>
>> static int ud_interception(struct vcpu_svm *svm)
>> {
>> - int er;
>> -
>> - er = emulate_instruction(&svm->vcpu, EMULTYPE_TRAP_UD);
>> - if (er == EMULATE_USER_EXIT)
>> - return 0;
>> - if (er != EMULATE_DONE)
>> - kvm_queue_exception(&svm->vcpu, UD_VECTOR);
>> - return 1;
>> + return handle_ud(&svm->vcpu);
>> }
>>
>> static int ac_interception(struct vcpu_svm *svm)
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index b2f8a70..0f11243 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -6436,14 +6436,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);
>
> (maybe different on this branch) isn't "er" now unused?
Hmm, It is used in other place of the function handle_exception.
>
>>
>> error_code = 0;
>> if (intr_info & INTR_INFO_DELIVER_CODE_MASK)
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 7d9a444..1eb495e 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -4840,6 +4840,19 @@ int kvm_write_guest_virt_system(struct x86_emulate_ctxt *ctxt,
>> }
>> EXPORT_SYMBOL_GPL(kvm_write_guest_virt_system);
>>
>> +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;
>
> I would now actually prefer
>
> if (er == EMULATE_DONE)
> return 1 ...
Keep the original one I think.
Regards,
Wanpeng Li
2018-04-04 19:59 GMT+08:00 David Hildenbrand <[email protected]>:
>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 1eb495e..a55ecef 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -146,6 +146,9 @@ bool __read_mostly enable_vmware_backdoor = false;
>> module_param(enable_vmware_backdoor, bool, S_IRUGO);
>> EXPORT_SYMBOL_GPL(enable_vmware_backdoor);
>>
>> +static bool __read_mostly force_emulation_prefix = false;
>> +module_param(force_emulation_prefix, bool, S_IRUGO);
>> +
>> #define KVM_NR_SHARED_MSRS 16
>>
>> struct kvm_shared_msrs_global {
>> @@ -4844,6 +4847,21 @@ int handle_ud(struct kvm_vcpu *vcpu)
>> {
>> enum emulation_result er;
>>
>> + if (force_emulation_prefix) {
>> + char sig[5]; /* ud2; .ascii "kvm" */
>> + struct x86_exception e;
>> +
>> + if (kvm_read_guest_virt(&vcpu->arch.emulate_ctxt,
>> + kvm_get_linear_rip(vcpu), sig, sizeof(sig), &e))
>> + goto emulate_ud;
>> +
>> + if (memcmp(sig, "\xf\xbkvm", sizeof(sig)) == 0) {
>> + kvm_rip_write(vcpu, kvm_rip_read(vcpu) + sizeof(sig));
>> + return emulate_instruction(vcpu, 0) == EMULATE_DONE;
>
> What if we would have an invalid instruction here? Shouldn't you handle
> the emulate_instruction() like below?
> (e.g. keep a variable with the emulation type (0 vs EMULTYPE_TRAP_UD)
> and reuse emulate_ud below)
emulate_instruction(vcpu, 0) can handle invalid instruction.
Regards,
Wanpeng Li
On 04/04/2018 15:35, Wanpeng Li wrote:
> 2018-04-04 19:59 GMT+08:00 David Hildenbrand <[email protected]>:
>>
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index 1eb495e..a55ecef 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -146,6 +146,9 @@ bool __read_mostly enable_vmware_backdoor = false;
>>> module_param(enable_vmware_backdoor, bool, S_IRUGO);
>>> EXPORT_SYMBOL_GPL(enable_vmware_backdoor);
>>>
>>> +static bool __read_mostly force_emulation_prefix = false;
>>> +module_param(force_emulation_prefix, bool, S_IRUGO);
>>> +
>>> #define KVM_NR_SHARED_MSRS 16
>>>
>>> struct kvm_shared_msrs_global {
>>> @@ -4844,6 +4847,21 @@ int handle_ud(struct kvm_vcpu *vcpu)
>>> {
>>> enum emulation_result er;
>>>
>>> + if (force_emulation_prefix) {
>>> + char sig[5]; /* ud2; .ascii "kvm" */
>>> + struct x86_exception e;
>>> +
>>> + if (kvm_read_guest_virt(&vcpu->arch.emulate_ctxt,
>>> + kvm_get_linear_rip(vcpu), sig, sizeof(sig), &e))
>>> + goto emulate_ud;
>>> +
>>> + if (memcmp(sig, "\xf\xbkvm", sizeof(sig)) == 0) {
>>> + kvm_rip_write(vcpu, kvm_rip_read(vcpu) + sizeof(sig));
>>> + return emulate_instruction(vcpu, 0) == EMULATE_DONE;
>>
>> What if we would have an invalid instruction here? Shouldn't you handle
>> the emulate_instruction() like below?
>> (e.g. keep a variable with the emulation type (0 vs EMULTYPE_TRAP_UD)
>> and reuse emulate_ud below)
>
> emulate_instruction(vcpu, 0) can handle invalid instruction.
But David's observation is still better because your code doesn't handle
usermode exits. I've fixed this up.
Paolo
On 04/04/2018 13:54, David Hildenbrand wrote:
>> +{
>> + 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;
> I would now actually prefer
>
> if (er == EMULATE_DONE)
> return 1 ...
Why? The return statement would be duplicated.
Paolo
On 04.04.2018 19:12, Paolo Bonzini wrote:
> On 04/04/2018 13:54, David Hildenbrand wrote:
>>> +{
>>> + 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;
>> I would now actually prefer
>>
>> if (er == EMULATE_DONE)
>> return 1 ...
>
> Why? The return statement would be duplicated.
>
> Paolo
>
I was talking about two equality checks vs. 1 equality and 1 inequality
check.
er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
if (er == EMULATE_USER_EXIT)
return 0;
else if (er == EMULATE_DONE)
return 1;
return kvm_queue_exception(vcpu, UD_VECTOR);
--
Thanks,
David / dhildenb
2018-04-05 1:09 GMT+08:00 Paolo Bonzini <[email protected]>:
> On 04/04/2018 15:35, Wanpeng Li wrote:
>> 2018-04-04 19:59 GMT+08:00 David Hildenbrand <[email protected]>:
>>>
>>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>>> index 1eb495e..a55ecef 100644
>>>> --- a/arch/x86/kvm/x86.c
>>>> +++ b/arch/x86/kvm/x86.c
>>>> @@ -146,6 +146,9 @@ bool __read_mostly enable_vmware_backdoor = false;
>>>> module_param(enable_vmware_backdoor, bool, S_IRUGO);
>>>> EXPORT_SYMBOL_GPL(enable_vmware_backdoor);
>>>>
>>>> +static bool __read_mostly force_emulation_prefix = false;
>>>> +module_param(force_emulation_prefix, bool, S_IRUGO);
>>>> +
>>>> #define KVM_NR_SHARED_MSRS 16
>>>>
>>>> struct kvm_shared_msrs_global {
>>>> @@ -4844,6 +4847,21 @@ int handle_ud(struct kvm_vcpu *vcpu)
>>>> {
>>>> enum emulation_result er;
>>>>
>>>> + if (force_emulation_prefix) {
>>>> + char sig[5]; /* ud2; .ascii "kvm" */
>>>> + struct x86_exception e;
>>>> +
>>>> + if (kvm_read_guest_virt(&vcpu->arch.emulate_ctxt,
>>>> + kvm_get_linear_rip(vcpu), sig, sizeof(sig), &e))
>>>> + goto emulate_ud;
>>>> +
>>>> + if (memcmp(sig, "\xf\xbkvm", sizeof(sig)) == 0) {
>>>> + kvm_rip_write(vcpu, kvm_rip_read(vcpu) + sizeof(sig));
>>>> + return emulate_instruction(vcpu, 0) == EMULATE_DONE;
>>>
>>> What if we would have an invalid instruction here? Shouldn't you handle
>>> the emulate_instruction() like below?
>>> (e.g. keep a variable with the emulation type (0 vs EMULTYPE_TRAP_UD)
>>> and reuse emulate_ud below)
>>
>> emulate_instruction(vcpu, 0) can handle invalid instruction.
>
> But David's observation is still better because your code doesn't handle usermode exits.
My code handles it, return emulate_instruction(vcpu, 0) ==
EMULATE_DONE, it will return 0 since EMULATE_USER_EXIT == EMULATE_DONE
fails.
> I've fixed this up.
Thanks. The codes similar to my v3 but more beauty. :) I change to
this view since Radim's comments to v3
https://www.spinics.net/lists/kvm/msg166999.html
Regards,
Wanpeng Li
On 05/04/2018 02:04, Wanpeng Li wrote:
>>> emulate_instruction(vcpu, 0) can handle invalid instruction.
>> But David's observation is still better because your code doesn't handle usermode exits.
> My code handles it, return emulate_instruction(vcpu, 0) ==
> EMULATE_DONE, it will return 0 since EMULATE_USER_EXIT == EMULATE_DONE
> fails.
>
>> I've fixed this up.
> Thanks. The codes similar to my v3 but more beauty. :) I change to
> this view since Radim's comments to v3
> https://www.spinics.net/lists/kvm/msg166999.html
And after I actually woke up I think I disagree with Radim. Tests can
trap the #UD to test emulation at CPL0 and skip or fail the test for
instructions unknown to the emulator. It's much better than sending an
emulation failure to userspace, which would abort the guest.
Paolo