* [RFC][PATCH] VMX: Add enhanced mechanism to detect vmentry failures
@ 2008-07-28 21:37 Mohammed Gamal
2008-07-29 14:31 ` Avi Kivity
0 siblings, 1 reply; 3+ messages in thread
From: Mohammed Gamal @ 2008-07-28 21:37 UTC (permalink / raw)
To: kvm; +Cc: avi, guillaume.thouvenin, laurent.vivier, riel
This patch is *not* meant to be merged. This patch fixes the random
crashes with gfxboot and it doesn't crash anymore at random
instructions.
It mainly does two things:
1- It handles all possible exit reasons before exiting for VMX failures
2- It handles vmentry failures avoiding external interrupts
However, while this patch allows booting FreeDOS with HIMEM with no
problems. It does occasionally crash with gfxboot at RIP 6e29, looking
at the gfxboot code the instructions causing the crash is as follows:
00006e10 <switch_to_pm_20>:
6e10: 66 b8 20 00 mov $0x20,%ax
6e14: 8e d8 mov %eax,%ds
6e16: 8c d0 mov %ss,%eax
6e18: 81 e4 ff ff 00 00 and $0xffff,%esp
6e1e: c1 e0 04 shl $0x4,%eax
6e21: 01 c4 add %eax,%esp
6e23: 66 b8 08 00 mov $0x8,%ax
6e27: 8e d0 mov %eax,%ss
6e29: 8e c0 mov %eax,%es
6e2b: 8e e0 mov %eax,%fs
6e2d: 8e e8 mov %eax,%gs
6e2f: 58 pop %eax
6e30: 66 9d popfw
6e32: 66 c3 retw
So apparently to fix the problem we need to add other guest state checks
-namely for ES, FS, GS- to invalid_guest_state().
Signed-off-by: Guillaume Thouvenin <guillaume.thouvenin@ext.bull.net>
Signed-off-by: Laurent Vivier <laurent.vivier@bull.net>
Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com>
---
arch/x86/kvm/vmx.c | 116 +++++++++++++++++++++++++++++++++++++++++---
arch/x86/kvm/vmx.h | 3 +
include/asm-x86/kvm_host.h | 1 +
3 files changed, 112 insertions(+), 8 deletions(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index c4510fe..b438f94 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -1316,7 +1316,8 @@ static void enter_pmode(struct kvm_vcpu *vcpu)
fix_pmode_dataseg(VCPU_SREG_GS, &vcpu->arch.rmode.gs);
fix_pmode_dataseg(VCPU_SREG_FS, &vcpu->arch.rmode.fs);
- vmcs_write16(GUEST_SS_SELECTOR, 0);
+ if (vcpu->arch.rmode_failed)
+ vmcs_write16(GUEST_SS_SELECTOR, 0);
vmcs_write32(GUEST_SS_AR_BYTES, 0x93);
vmcs_write16(GUEST_CS_SELECTOR,
@@ -2708,6 +2709,93 @@ static int handle_nmi_window(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
return 1;
}
+static int invalid_guest_state(struct kvm_vcpu *vcpu,
+ struct kvm_run *kvm_run, u32 failure_reason)
+{
+ u16 ss, cs;
+ u8 opcodes[4];
+ unsigned long rip = kvm_rip_read(vcpu);
+ unsigned long rip_linear;
+
+ ss = vmcs_read16(GUEST_SS_SELECTOR);
+ cs = vmcs_read16(GUEST_CS_SELECTOR);
+
+ if ((ss & 0x03) != (cs & 0x03)) {
+ int err;
+ rip_linear = rip + vmx_get_segment_base(vcpu, VCPU_SREG_CS);
+ emulator_read_std(rip_linear, (void *)opcodes, 4, vcpu);
+ err = emulate_instruction(vcpu, kvm_run, 0, 0, 0);
+ switch (err) {
+ case EMULATE_DONE:
+ return 1;
+ case EMULATE_DO_MMIO:
+ printk(KERN_INFO "mmio?\n");
+ return 0;
+ default:
+ /* HACK: If we can not emulate the instruction
+ * we write a sane value on SS to pass sanity
+ * checks. The good thing to do is to emulate the
+ * instruction */
+ kvm_report_emulation_failure(vcpu, "vmentry failure");
+ printk(KERN_INFO " => Quit real mode emulation\n");
+ vcpu->arch.rmode_failed = 1;
+ vmcs_write16(GUEST_SS_SELECTOR, 0);
+ return 1;
+ }
+ }
+
+ kvm_run->exit_reason = KVM_EXIT_UNKNOWN;
+ kvm_run->hw.hardware_exit_reason = failure_reason;
+ printk(KERN_INFO "Failed to handle invalid guest state\n");
+ return 0;
+}
+
+/*
+ * Should be replaced with exit handlers for each individual case
+ */
+static int handle_vmentry_failure(struct kvm_vcpu *vcpu,
+ struct kvm_run *kvm_run,
+ u32 failure_reason)
+{
+ unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
+ switch (failure_reason) {
+ case EXIT_REASON_INVALID_GUEST_STATE:
+ return invalid_guest_state(vcpu, kvm_run, failure_reason);
+ case EXIT_REASON_MSR_LOADING:
+ printk("VMentry failure caused by MSR entry %ld loading.\n",
+ exit_qualification);
+ printk(" ... Not handled\n");
+ break;
+ case EXIT_REASON_MACHINE_CHECK:
+ printk("VMentry failure caused by machine check.\n");
+ printk(" ... Not handled\n");
+ break;
+ default:
+ printk("reason not known yet!\n");
+ break;
+ }
+ return 0;
+}
+
+static int handle_invalid_guest_state(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
+{
+ int rc;
+ u32 exit_reason = vmcs_read32(VM_EXIT_REASON);
+
+ /*
+ * Disable interrupts to avoid occasional vmexits while
+ * handling vmentry failures
+ */
+ spin_lock_irq(&vmx_vpid_lock);
+ if(exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY)
+ exit_reason &= ~VMX_EXIT_REASONS_FAILED_VMENTRY;
+
+ rc = invalid_guest_state(vcpu, kvm_run, exit_reason);
+ spin_unlock_irq(&vmx_vpid_lock);
+
+ return rc;
+}
+
/*
* The exit handlers return 1 if the exit was handled fully and guest execution
* may resume. Otherwise they set the kvm_run parameter to indicate what needs
@@ -2733,6 +2821,7 @@ static int (*kvm_vmx_exit_handlers[])(struct kvm_vcpu *vcpu,
[EXIT_REASON_WBINVD] = handle_wbinvd,
[EXIT_REASON_TASK_SWITCH] = handle_task_switch,
[EXIT_REASON_EPT_VIOLATION] = handle_ept_violation,
+ [EXIT_REASON_INVALID_GUEST_STATE] = handle_invalid_guest_state,
};
static const int kvm_vmx_max_exit_handlers =
@@ -2758,21 +2847,32 @@ static int kvm_handle_exit(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
ept_load_pdptrs(vcpu);
}
- if (unlikely(vmx->fail)) {
- kvm_run->exit_reason = KVM_EXIT_FAIL_ENTRY;
- kvm_run->fail_entry.hardware_entry_failure_reason
- = vmcs_read32(VM_INSTRUCTION_ERROR);
- return 0;
- }
-
if ((vectoring_info & VECTORING_INFO_VALID_MASK) &&
(exit_reason != EXIT_REASON_EXCEPTION_NMI &&
exit_reason != EXIT_REASON_EPT_VIOLATION))
printk(KERN_WARNING "%s: unexpected, valid vectoring info and "
"exit reason is 0x%x\n", __func__, exit_reason);
+
+ /*
+ * Instead of using handle_vmentry_failure(), just clear
+ * the vmentry failure bit and leave it to the exit handlers
+ * to deal with the specific exit reason.
+ * The exit handlers other than invalid guest state handler
+ * will be added later.
+ */
+ if ((exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY))
+ exit_reason &= ~VMX_EXIT_REASONS_FAILED_VMENTRY;
+
+
+ /* Handle all possible exits first, handle failure later. */
if (exit_reason < kvm_vmx_max_exit_handlers
&& kvm_vmx_exit_handlers[exit_reason])
return kvm_vmx_exit_handlers[exit_reason](vcpu, kvm_run);
+ else if(unlikely(vmx->fail)) {
+ kvm_run->exit_reason = KVM_EXIT_FAIL_ENTRY;
+ kvm_run->fail_entry.hardware_entry_failure_reason
+ = vmcs_read32(VM_INSTRUCTION_ERROR);
+ }
else {
kvm_run->exit_reason = KVM_EXIT_UNKNOWN;
kvm_run->hw.hardware_exit_reason = exit_reason;
diff --git a/arch/x86/kvm/vmx.h b/arch/x86/kvm/vmx.h
index 0c22e5f..cf8b771 100644
--- a/arch/x86/kvm/vmx.h
+++ b/arch/x86/kvm/vmx.h
@@ -239,7 +239,10 @@ enum vmcs_field {
#define EXIT_REASON_IO_INSTRUCTION 30
#define EXIT_REASON_MSR_READ 31
#define EXIT_REASON_MSR_WRITE 32
+#define EXIT_REASON_INVALID_GUEST_STATE 33
+#define EXIT_REASON_MSR_LOADING 34
#define EXIT_REASON_MWAIT_INSTRUCTION 36
+#define EXIT_REASON_MACHINE_CHECK 41
#define EXIT_REASON_TPR_BELOW_THRESHOLD 43
#define EXIT_REASON_APIC_ACCESS 44
#define EXIT_REASON_EPT_VIOLATION 48
diff --git a/include/asm-x86/kvm_host.h b/include/asm-x86/kvm_host.h
index 0b6b996..422d7c2 100644
--- a/include/asm-x86/kvm_host.h
+++ b/include/asm-x86/kvm_host.h
@@ -294,6 +294,7 @@ struct kvm_vcpu_arch {
} tr, es, ds, fs, gs;
} rmode;
int halt_request; /* real mode on Intel only */
+ int rmode_failed;
int cpuid_nent;
struct kvm_cpuid_entry2 cpuid_entries[KVM_MAX_CPUID_ENTRIES];
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [RFC][PATCH] VMX: Add enhanced mechanism to detect vmentry failures
2008-07-28 21:37 [RFC][PATCH] VMX: Add enhanced mechanism to detect vmentry failures Mohammed Gamal
@ 2008-07-29 14:31 ` Avi Kivity
2008-07-29 15:06 ` Mohammed Gamal
0 siblings, 1 reply; 3+ messages in thread
From: Avi Kivity @ 2008-07-29 14:31 UTC (permalink / raw)
To: Mohammed Gamal; +Cc: kvm, guillaume.thouvenin, laurent.vivier, riel
Mohammed Gamal wrote:
> This patch is *not* meant to be merged. This patch fixes the random
> crashes with gfxboot and it doesn't crash anymore at random
> instructions.
>
> It mainly does two things:
> 1- It handles all possible exit reasons before exiting for VMX failures
> 2- It handles vmentry failures avoiding external interrupts
>
> However, while this patch allows booting FreeDOS with HIMEM with no
> problems. It does occasionally crash with gfxboot at RIP 6e29, looking
> at the gfxboot code the instructions causing the crash is as follows:
>
> 00006e10 <switch_to_pm_20>:
> 6e10: 66 b8 20 00 mov $0x20,%ax
> 6e14: 8e d8 mov %eax,%ds
> 6e16: 8c d0 mov %ss,%eax
> 6e18: 81 e4 ff ff 00 00 and $0xffff,%esp
> 6e1e: c1 e0 04 shl $0x4,%eax
> 6e21: 01 c4 add %eax,%esp
> 6e23: 66 b8 08 00 mov $0x8,%ax
> 6e27: 8e d0 mov %eax,%ss
> 6e29: 8e c0 mov %eax,%es
> 6e2b: 8e e0 mov %eax,%fs
> 6e2d: 8e e8 mov %eax,%gs
> 6e2f: 58 pop %eax
> 6e30: 66 9d popfw
> 6e32: 66 c3 retw
>
> So apparently to fix the problem we need to add other guest state checks
> -namely for ES, FS, GS- to invalid_guest_state().
>
> @ -2708,6 +2709,93 @@ static int handle_nmi_window(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
> return 1;
> }
>
> +static int invalid_guest_state(struct kvm_vcpu *vcpu,
> + struct kvm_run *kvm_run, u32 failure_reason)
> +{
> + u16 ss, cs;
> + u8 opcodes[4];
> + unsigned long rip = kvm_rip_read(vcpu);
> + unsigned long rip_linear;
> +
> + ss = vmcs_read16(GUEST_SS_SELECTOR);
> + cs = vmcs_read16(GUEST_CS_SELECTOR);
> +
> + if ((ss & 0x03) != (cs & 0x03)) {
>
Please separate into a predicate function that checks various conditions
for invalid guest state (for example, segment base != segment selector
<< 4, or segment limit != 64K-1, in real mode). We can then use the
predicate to switch in and out of emulation mode without attempting entry.
> + int err;
> + rip_linear = rip + vmx_get_segment_base(vcpu, VCPU_SREG_CS);
> + emulator_read_std(rip_linear, (void *)opcodes, 4, vcpu);
> + err = emulate_instruction(vcpu, kvm_run, 0, 0, 0);
> + switch (err) {
> + case EMULATE_DONE:
> + return 1;
> + case EMULATE_DO_MMIO:
> + printk(KERN_INFO "mmio?\n");
>
It would be nice to handle mmio correctly in this case.
> + return 0;
> + default:
> + /* HACK: If we can not emulate the instruction
> + * we write a sane value on SS to pass sanity
> + * checks. The good thing to do is to emulate the
> + * instruction */
> + kvm_report_emulation_failure(vcpu, "vmentry failure");
> + printk(KERN_INFO " => Quit real mode emulation\n");
> + vcpu->arch.rmode_failed = 1;
> + vmcs_write16(GUEST_SS_SELECTOR, 0);
> + return 1;
>
Does this actually happen? If so, we can simply fix/add the
intstructions which fail.
> + }
>
> static const int kvm_vmx_max_exit_handlers =
> @@ -2758,21 +2847,32 @@ static int kvm_handle_exit(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
> ept_load_pdptrs(vcpu);
> }
>
> - if (unlikely(vmx->fail)) {
> - kvm_run->exit_reason = KVM_EXIT_FAIL_ENTRY;
> - kvm_run->fail_entry.hardware_entry_failure_reason
> - = vmcs_read32(VM_INSTRUCTION_ERROR);
> - return 0;
> - }
> -
> if ((vectoring_info & VECTORING_INFO_VALID_MASK) &&
> (exit_reason != EXIT_REASON_EXCEPTION_NMI &&
> exit_reason != EXIT_REASON_EPT_VIOLATION))
> printk(KERN_WARNING "%s: unexpected, valid vectoring info and "
> "exit reason is 0x%x\n", __func__, exit_reason);
>
Is vectoring_info valid here? I'd think not. We could run into trouble
if interrupts are injected.
This is a tricky area. Perhaps we should never attempt entry if the
guest state is invalid, simply to avoid this potential mess. On entry
into real mode (or rather, on any mode switch) check
invalid_guest_state() and keep emulating until it isn't.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RFC][PATCH] VMX: Add enhanced mechanism to detect vmentry failures
2008-07-29 14:31 ` Avi Kivity
@ 2008-07-29 15:06 ` Mohammed Gamal
0 siblings, 0 replies; 3+ messages in thread
From: Mohammed Gamal @ 2008-07-29 15:06 UTC (permalink / raw)
To: Avi Kivity; +Cc: kvm, guillaume.thouvenin, laurent.vivier, riel
On 7/29/08, Avi Kivity <avi@qumranet.com> wrote:
> Mohammed Gamal wrote:
> >
> > This patch is *not* meant to be merged. This patch fixes the random
> > crashes with gfxboot and it doesn't crash anymore at random
> > instructions.
> >
> > It mainly does two things:
> > 1- It handles all possible exit reasons before exiting for VMX failures
> > 2- It handles vmentry failures avoiding external interrupts
> >
> > However, while this patch allows booting FreeDOS with HIMEM with no
> > problems. It does occasionally crash with gfxboot at RIP 6e29, looking
> > at the gfxboot code the instructions causing the crash is as follows:
> >
> > 00006e10 <switch_to_pm_20>:
> > 6e10: 66 b8 20 00 mov $0x20,%ax
> > 6e14: 8e d8 mov %eax,%ds
> > 6e16: 8c d0 mov %ss,%eax
> > 6e18: 81 e4 ff ff 00 00 and $0xffff,%esp
> > 6e1e: c1 e0 04 shl $0x4,%eax
> > 6e21: 01 c4 add %eax,%esp
> > 6e23: 66 b8 08 00 mov $0x8,%ax
> > 6e27: 8e d0 mov %eax,%ss
> > 6e29: 8e c0 mov %eax,%es
> > 6e2b: 8e e0 mov %eax,%fs
> > 6e2d: 8e e8 mov %eax,%gs
> > 6e2f: 58 pop %eax
> > 6e30: 66 9d popfw
> > 6e32: 66 c3 retw
> >
> > So apparently to fix the problem we need to add other guest state checks
> > -namely for ES, FS, GS- to invalid_guest_state().
> >
> > @ -2708,6 +2709,93 @@ static int handle_nmi_window(struct kvm_vcpu *vcpu,
> struct kvm_run *kvm_run)
> > return 1;
> > }
> > +static int invalid_guest_state(struct kvm_vcpu *vcpu,
> > + struct kvm_run *kvm_run, u32 failure_reason)
> > +{
> > + u16 ss, cs;
> > + u8 opcodes[4];
> > + unsigned long rip = kvm_rip_read(vcpu);
> > + unsigned long rip_linear;
> > +
> > + ss = vmcs_read16(GUEST_SS_SELECTOR);
> > + cs = vmcs_read16(GUEST_CS_SELECTOR);
> > +
> > + if ((ss & 0x03) != (cs & 0x03)) {
> >
>
> Please separate into a predicate function that checks various conditions for
> invalid guest state (for example, segment base != segment selector << 4, or
> segment limit != 64K-1, in real mode). We can then use the predicate to
> switch in and out of emulation mode without attempting entry.
>
This is exactly what I was thinking, but since this patch is just
based on Guillaume Thouvenin's older patch, I though I'd do that after
getting comments on this one.
> > + int err;
> > + rip_linear = rip + vmx_get_segment_base(vcpu,
> VCPU_SREG_CS);
> > + emulator_read_std(rip_linear, (void *)opcodes, 4, vcpu);
> > + err = emulate_instruction(vcpu, kvm_run, 0, 0, 0);
> > + switch (err) {
> > + case EMULATE_DONE:
> > + return 1;
> > + case EMULATE_DO_MMIO:
> > + printk(KERN_INFO "mmio?\n");
> >
> >
>
> It would be nice to handle mmio correctly in this case.
Will look into that, but my main focus for now is to get gfxboot to
work correctly.
> > + return 0;
> > + default:
> > + /* HACK: If we can not emulate the
> instruction
> > + * we write a sane value on SS to pass
> sanity
> > + * checks. The good thing to do is to
> emulate the
> > + * instruction */
> > +
> kvm_report_emulation_failure(vcpu, "vmentry failure");
> > + printk(KERN_INFO " => Quit real mode
> emulation\n");
> > + vcpu->arch.rmode_failed = 1;
> > +
> vmcs_write16(GUEST_SS_SELECTOR, 0);
> > + return 1;
> >
> >
>
> Does this actually happen? If so, we can simply fix/add the intstructions
> which fail.
>
I didn't encounter any instructions except 'nop', which I sent a patch
to add along time ago.
> > + }
> > static const int kvm_vmx_max_exit_handlers =
> > @@ -2758,21 +2847,32 @@ static int kvm_handle_exit(struct kvm_run
> *kvm_run, struct kvm_vcpu *vcpu)
> > ept_load_pdptrs(vcpu);
> > }
> > - if (unlikely(vmx->fail)) {
> > - kvm_run->exit_reason = KVM_EXIT_FAIL_ENTRY;
> > -
> kvm_run->fail_entry.hardware_entry_failure_reason
> > - =
> vmcs_read32(VM_INSTRUCTION_ERROR);
> > - return 0;
> > - }
> > -
> > if ((vectoring_info & VECTORING_INFO_VALID_MASK) &&
> > (exit_reason != EXIT_REASON_EXCEPTION_NMI &&
> > exit_reason != EXIT_REASON_EPT_VIOLATION))
> > printk(KERN_WARNING "%s: unexpected, valid vectoring info
> and "
> > "exit reason is 0x%x\n", __func__, exit_reason);
> >
> >
>
> Is vectoring_info valid here? I'd think not. We could run into trouble if
> interrupts are injected.
>
dmesg didn't show anything so I guess it's fine, I didn't check
vectoring_info value though.
> This is a tricky area. Perhaps we should never attempt entry if the guest
> state is invalid, simply to avoid this potential mess. On entry into real
> mode (or rather, on any mode switch) check invalid_guest_state() and keep
> emulating until it isn't.
>
This sounds alot neater than trying to handle vmentry failures.
Perhaps I should try doing it this way.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2008-07-29 15:06 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-07-28 21:37 [RFC][PATCH] VMX: Add enhanced mechanism to detect vmentry failures Mohammed Gamal
2008-07-29 14:31 ` Avi Kivity
2008-07-29 15:06 ` Mohammed Gamal
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox