On 7/29/08, Avi Kivity <[EMAIL PROTECTED]> 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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html