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
