Re: [RFC][PATCH] VMX: Invalid guest state emulation

[Avi Kivity <avi@qumranet.com>]

Mohammed Gamal wrote:
> This patch aims to allow emulation whenever guest state is not valid for VMX operation. This usually happens in mode switches with guests such as 
> older versions of gfxboot and FreeDOS with HIMEM. 
>
> The patch aims to address this issue, it introduces the following:
>
> - A function that invokes the x86 emulator when the guest state is not valid (borrowed from Guillaume Thouvenin's real mode patches)
> - A function that checks that guest register state is VMX compliant
> - A module parameter that enables these operations. It is disabled by default, in order not to intervene with KVM's normal operation
>   

>  
> +/*
> + * Check if guest state is valid. Returns true if valid, false if
> + * not.
> + * We assume that registers are always usable
> + */
> +static bool guest_state_valid(struct kvm_vcpu *vcpu)
> +{
> +	u16 cs,ds,ss,es,fs,gs,tr,ldtr;
> +	u64 cs_limit, ds_limit, ss_limit, es_limit, fs_limit, gs_limit;
> +	u64 tr_limit, ldtr_limit;
> +	u32 cs_ar, ds_ar, ss_ar, es_ar, fs_ar, gs_ar, tr_ar, ldtr_ar;
> +	
> +	cs = vmcs_read16(GUEST_CS_SELECTOR);
> +	ds = vmcs_read16(GUEST_DS_SELECTOR);
> +	ss = vmcs_read16(GUEST_SS_SELECTOR);
> +	es = vmcs_read16(GUEST_ES_SELECTOR);
> +	fs = vmcs_read16(GUEST_FS_SELECTOR);
> +	gs = vmcs_read16(GUEST_GS_SELECTOR);
> +	tr = vmcs_read16(GUEST_TR_SELECTOR);
> +	ldtr = vmcs_read16(GUEST_LDTR_SELECTOR);
> +
> +	cs_limit = vmcs_readl(GUEST_SS_LIMIT);
> +	ds_limit = vmcs_readl(GUEST_DS_LIMIT);
> +	ss_limit = vmcs_readl(GUEST_SS_LIMIT);
> +	es_limit = vmcs_readl(GUEST_ES_LIMIT);
> +	fs_limit = vmcs_readl(GUEST_FS_LIMIT);
> +	gs_limit = vmcs_readl(GUEST_GS_LIMIT);
> +	tr_limit = vmcs_readl(GUEST_TR_LIMIT);
> +
> +	cs_ar = vmcs_read32(GUEST_CS_AR_BYTES);
> +	ds_ar = vmcs_read32(GUEST_DS_AR_BYTES);
> +	ss_ar = vmcs_read32(GUEST_SS_AR_BYTES);
> +	es_ar = vmcs_read32(GUEST_ES_AR_BYTES);
> +	fs_ar = vmcs_read32(GUEST_FS_AR_BYTES);
> +	gs_ar = vmcs_read32(GUEST_GS_AR_BYTES);
> +	tr_ar = vmcs_read32(GUEST_TR_AR_BYTES);
> +	ldtr_ar = vmcs_read32(GUEST_LDTR_AR_BYTES);
> +
> +	if(tr & 0x02)	/* TI = 1 */
> +		return false;
> +
> +	if(ldtr & 0x02) /* TI = 1 */
> +		return false;
> +
> +	/* vm86 mode guest state checks */
> +	if(vcpu->arch.rmode.active) {
>   

Better to check cr0 here.

> +		/* Check segment limits */
> +		if( (cs_limit != 0xffff) || (ds_limit != 0xffff) ||
> +		    (ss_limit != 0xffff) || (es_limit != 0xffff) ||
> +		    (fs_limit != 0xffff) || (gs_limit != 0xffff) )
> +			return false;
>   

Would be nice to get code reuse, here and below (i.e. 
data_segment_valid() for ds,es..gs.).  Also, vmx_get_segment() will make 
the code tidier.

> +
>  static void fix_pmode_dataseg(int seg, struct kvm_save_segment *save)
>  {
>  	struct kvm_vmx_segment_field *sf = &kvm_vmx_segment_fields[seg];
> @@ -1311,10 +1486,12 @@ static void enter_pmode(struct kvm_vcpu *vcpu)
>  
>  	update_exception_bitmap(vcpu);
>  
> -	fix_pmode_dataseg(VCPU_SREG_ES, &vcpu->arch.rmode.es);
> -	fix_pmode_dataseg(VCPU_SREG_DS, &vcpu->arch.rmode.ds);
> -	fix_pmode_dataseg(VCPU_SREG_GS, &vcpu->arch.rmode.gs);
> -	fix_pmode_dataseg(VCPU_SREG_FS, &vcpu->arch.rmode.fs);
> +	if(!emulate_invalid_guest_state) {
> +		fix_pmode_dataseg(VCPU_SREG_ES, &vcpu->arch.rmode.es);
> +		fix_pmode_dataseg(VCPU_SREG_DS, &vcpu->arch.rmode.ds);
> +		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);
>  	vmcs_write32(GUEST_SS_AR_BYTES, 0x93);
>   

These (and the other hacks below) need also to be qualified with 
emulate_invalid_guest_state.

> @@ -1322,6 +1499,13 @@ static void enter_pmode(struct kvm_vcpu *vcpu)
>  	vmcs_write16(GUEST_CS_SELECTOR,
>  		     vmcs_read16(GUEST_CS_SELECTOR) & ~SELECTOR_RPL_MASK);
>  	vmcs_write32(GUEST_CS_AR_BYTES, 0x9b);
> +
> +	if(emulate_invalid_guest_state) {
> +		while(!guest_state_valid(vcpu)) {
> +			if(!invalid_guest_state_handler(vcpu))
> +				break;
> +		}
> +	}
>   

This can drive the host into an infinite loop.  Instead, I suggest 
having enter_rmode() and enter_pmode() set a vmx->mode_transition flag.  
On guest entry, if (mode_transition && emulate_invalid_guest_state), 
emulate_instruction() instead of entering vmx.

>  }
>  
>  static gva_t rmode_tss_base(struct kvm *kvm)
> @@ -1383,13 +1567,22 @@ static void enter_rmode(struct kvm_vcpu *vcpu)
>  		vmcs_writel(GUEST_CS_BASE, 0xf0000);
>  	vmcs_write16(GUEST_CS_SELECTOR, vmcs_readl(GUEST_CS_BASE) >> 4);
>   

Also needs to be qualified with emulate_invalid_guest_state.

Overall, looks very promising.  We can start with this, then change it 
to be the default, and finally rip off the old code.

Note you have some coding style violations (need space after if or while).

-- 
error compiling committee.c: too many arguments to function