Re: [RFC][PATCH] VMX: Add enhanced mechanism to detect vmentry failures

[Avi Kivity <avi@qumranet.com>]

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