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

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

[Mohammed Gamal]

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

---

 arch/x86/kvm/vmx.c |  234 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 225 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index c4510fe..61da1e3 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -1,4 +1,4 @@
-/*
+t/
  * Kernel-based Virtual Machine driver for Linux
  *
  * This module enables machines with Intel VT-x extensions to run virtual
@@ -49,6 +49,9 @@ module_param(flexpriority_enabled, bool, 0);
 static int enable_ept = 1;
 module_param(enable_ept, bool, 0);
 
+static int emulate_invalid_guest_state = 0;
+module_param(emulate_invalid_guest_state, bool, 0);
+
 struct vmcs {
 	u32 revision_id;
 	u32 abort;
@@ -95,6 +98,7 @@ static inline struct vcpu_vmx *to_vmx(struct kvm_vcpu *vcpu)
 
 static int init_rmode(struct kvm *kvm);
 static u64 construct_eptp(unsigned long root_hpa);
+static int invalid_guest_state_handler(struct kvm_vcpu *vcpu);
 
 static DEFINE_PER_CPU(struct vmcs *, vmxarea);
 static DEFINE_PER_CPU(struct vmcs *, current_vmcs);
@@ -1275,6 +1279,177 @@ static __exit void hardware_unsetup(void)
 	free_kvm_area();
 }
 
+/*
+ * 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) {
+		/* Check segment limits */
+		if( (cs_limit != 0xffff) || (ds_limit != 0xffff) ||
+		    (ss_limit != 0xffff) || (es_limit != 0xffff) ||
+		    (fs_limit != 0xffff) || (gs_limit != 0xffff) )
+			return false;
+		
+		/* Check access rights */
+		if( (cs_ar != 0xf3) || (ds_ar != 0xf3) || (ss_ar != 0xf3) ||
+		    (es_ar != 0xf3) || (fs_ar != 0xf3) || (gs_ar != 0xf3) )
+			return false;
+	}
+	else { /* protected mode guest state checks */
+		
+		/* SS RPL bits must equal CS RPL bits */ 
+		if((cs & 0x03) != (ss & 0x03))
+			return false;
+		
+		/* Begin access rights bits check */
+		
+		/* Type field checks */
+		if(! (cs_ar & (AR_TYPE_ACCESSES_MASK|AR_TYPE_CODE_MASK)) )
+			return false;
+		if( ((ss_ar & AR_TYPE_MASK) != 3) || ((ss_ar & AR_TYPE_MASK) != 7) )
+			return false;
+		if(! ((ds_ar & AR_TYPE_ACCESSES_MASK) || (es_ar & AR_TYPE_ACCESSES_MASK) || 
+		      (fs_ar & AR_TYPE_ACCESSES_MASK) || (gs_ar & AR_TYPE_ACCESSES_MASK)) )
+			return false;
+		if(! (( (ds_ar & AR_TYPE_CODE_MASK) && (ds_ar & AR_TYPE_READABLE_MASK) ) ||
+		      ( (es_ar & AR_TYPE_CODE_MASK) && (es_ar & AR_TYPE_READABLE_MASK) ) ||
+		      ( (fs_ar & AR_TYPE_CODE_MASK) && (fs_ar & AR_TYPE_READABLE_MASK) ) ||
+		      ( (gs_ar & AR_TYPE_CODE_MASK) && (gs_ar & AR_TYPE_READABLE_MASK) )) )
+			return false;
+
+		/* S field checks */
+		if(! ((cs_ar & AR_S_MASK) || (ds_ar & AR_S_MASK) || (ss_ar & AR_S_MASK) ||
+		      (es_ar & AR_S_MASK) || (fs_ar & AR_S_MASK) || (gs_ar & AR_S_MASK)) )
+			return false;
+
+		/* DPL field checks */
+		if( ((cs_ar & AR_TYPE_MASK) <= 0xb) && ((cs_ar & AR_TYPE_MASK) >= 0x8) ) {
+			if(AR_DPL(cs_ar) != (cs & 0x03))
+				return false;
+		}
+		else if( ((cs_ar & AR_TYPE_MASK) <= 0xf) && ((cs_ar & AR_TYPE_MASK) >= 0xc) ) {
+			if(AR_DPL(cs_ar) > (cs & 0x03))
+				return false;
+		}
+
+		if(AR_DPL(ss_ar) != (ss & 0x03))
+			return false;
+
+		if((ds_ar & AR_TYPE_MASK) <= 0xb)
+			if(AR_DPL(ds_ar) < (ds & 0x03))
+				return false;
+		if((es_ar & AR_TYPE_MASK) <= 0xb)
+			if(AR_DPL(es_ar) < (es & 0x03))
+				return false;
+		if((fs_ar & AR_TYPE_MASK) <= 0xb)
+			if(AR_DPL(fs_ar) < (fs & 0x03))
+				return false;
+		if((gs_ar & AR_TYPE_MASK) <= 0xb)
+			if(AR_DPL(gs_ar) < (gs & 0x03))
+				return false; 
+
+		/* P field check */
+		if(! ((cs_ar & AR_P_MASK) || (ds_ar & AR_P_MASK) || (ss_ar & AR_P_MASK) ||
+		      (es_ar & AR_P_MASK) || (fs_ar & AR_P_MASK) || (gs_ar & AR_P_MASK)) )
+			return false;
+
+		/* Reserved fields check */
+		if( (cs_ar & AR_RESERVD_MASK) || (ds_ar & AR_RESERVD_MASK) || 
+		    (ss_ar & AR_RESERVD_MASK) || (es_ar & AR_RESERVD_MASK) ||
+		    (fs_ar & AR_RESERVD_MASK) || (gs_ar & AR_RESERVD_MASK) )
+			return false;
+
+		/* TODO: 
+ 		 * - Add checks on G and D/B fields
+ 		 * - Add checks on the unusable mask
+ 		 */
+	}
+
+	/* TR access rights bits checks */
+	if(tr_ar & AR_S_MASK)
+		return false;
+	if((tr_ar & AR_P_MASK) != 1)
+		return false;
+	if(tr_ar & AR_RESERVD_MASK)
+		return false;
+	if(tr_ar & AR_UNUSABLE_MASK)
+		return false;
+	if((tr_limit & 0x00000fff) != 0x00000fff) {
+		if(tr_ar & AR_G_MASK)
+			return false;
+	}
+	else if(tr_limit & 0xffff0000) {
+		if(!(tr_ar & AR_G_MASK))
+			return false;
+	}
+	
+	/* LDTR access right bits checks */
+	if(!(ldtr_ar & AR_TYPE_LDT))
+		return false;
+	if(ldtr_ar & AR_S_MASK)
+		return false;
+	if((ldtr_ar & AR_P_MASK) != 1)
+		return false;
+	if(ldtr_ar & AR_RESERVD_MASK)
+		return false;
+	if((ldtr_limit & 0x00000fff) != 0x00000fff) {
+		if(ldtr_ar & AR_G_MASK)
+			return false;
+	}
+	else if(ldtr_limit & 0xffff0000) {
+		if(ldtr_ar & AR_G_MASK)
+			return false;
+	}
+
+	/* TODO:
+ 	 * - Add checks on RIP
+ 	 * - Add checks on RFLAGS
+ 	 */
+ 	
+	return true;
+}
+
 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);
@@ -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;
+		}
+	}
 }
 
 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);
 
-	fix_rmode_seg(VCPU_SREG_ES, &vcpu->arch.rmode.es);
-	fix_rmode_seg(VCPU_SREG_DS, &vcpu->arch.rmode.ds);
-	fix_rmode_seg(VCPU_SREG_GS, &vcpu->arch.rmode.gs);
-	fix_rmode_seg(VCPU_SREG_FS, &vcpu->arch.rmode.fs);
+	if(!emulate_invalid_guest_state) {
+		fix_rmode_seg(VCPU_SREG_ES, &vcpu->arch.rmode.es);
+		fix_rmode_seg(VCPU_SREG_DS, &vcpu->arch.rmode.ds);
+		fix_rmode_seg(VCPU_SREG_GS, &vcpu->arch.rmode.gs);
+		fix_rmode_seg(VCPU_SREG_FS, &vcpu->arch.rmode.fs);
+	}
 
 	kvm_mmu_reset_context(vcpu);
 	init_rmode(vcpu->kvm);
+	
+	if(emulate_invalid_guest_state) {
+		while(!guest_state_valid(vcpu)) {
+			if(!invalid_guest_state_handler(vcpu))
+				break;
+		}
+	}
 }
 
 #ifdef CONFIG_X86_64
@@ -2708,6 +2901,29 @@ static int handle_nmi_window(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 	return 1;
 }
 
+static int invalid_guest_state_handler(struct kvm_vcpu *vcpu)
+{
+	u8 opcodes[4];
+	unsigned long rip = kvm_rip_read(vcpu);
+	unsigned long rip_linear;
+	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, NULL, 0, 0, 0);
+		
+	switch (err) {
+		case EMULATE_DONE:
+			return 1;
+		case EMULATE_DO_MMIO:
+			printk(KERN_INFO "mmio?\n");
+			return 0;
+		default:
+			kvm_report_emulation_failure(vcpu, "vmentry failure");
+			return 0;
+	}
+}
+
 /*
  * 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

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

[Mohammed Gamal]

[This resend adds the sign-offs and fixes a typo that was in the previous patch]

This patch aims to allow emulation whenever guest state is not valid for VMX operation, which occurs while trying to emulate big real mode on 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

The next step needed in order to correctly emulate real mode would be to add more instructions in the x86 emulator whenever needed.

Signed-off-by: Laurent Vivier <laurent.vivier@bull.net>
Signed-off-by: Guillaume Thouvenin <guillaume.thouvenin@ext.bull.net>
Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com>
---
 arch/x86/kvm/vmx.c |  232 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 224 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index c4510fe..2b5dd68 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -49,6 +49,9 @@ module_param(flexpriority_enabled, bool, 0);
 static int enable_ept = 1;
 module_param(enable_ept, bool, 0);
 
+static int emulate_invalid_guest_state = 0;
+module_param(emulate_invalid_guest_state, bool, 0);
+
 struct vmcs {
 	u32 revision_id;
 	u32 abort;
@@ -95,6 +98,7 @@ static inline struct vcpu_vmx *to_vmx(struct kvm_vcpu *vcpu)
 
 static int init_rmode(struct kvm *kvm);
 static u64 construct_eptp(unsigned long root_hpa);
+static int invalid_guest_state_handler(struct kvm_vcpu *vcpu);
 
 static DEFINE_PER_CPU(struct vmcs *, vmxarea);
 static DEFINE_PER_CPU(struct vmcs *, current_vmcs);
@@ -1275,6 +1279,177 @@ static __exit void hardware_unsetup(void)
 	free_kvm_area();
 }
 
+/*
+ * 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) {
+		/* Check segment limits */
+		if( (cs_limit != 0xffff) || (ds_limit != 0xffff) ||
+		    (ss_limit != 0xffff) || (es_limit != 0xffff) ||
+		    (fs_limit != 0xffff) || (gs_limit != 0xffff) )
+			return false;
+		
+		/* Check access rights */
+		if( (cs_ar != 0xf3) || (ds_ar != 0xf3) || (ss_ar != 0xf3) ||
+		    (es_ar != 0xf3) || (fs_ar != 0xf3) || (gs_ar != 0xf3) )
+			return false;
+	}
+	else { /* protected mode guest state checks */
+		
+		/* SS RPL bits must equal CS RPL bits */ 
+		if((cs & 0x03) != (ss & 0x03))
+			return false;
+		
+		/* Begin access rights bits check */
+		
+		/* Type field checks */
+		if(! (cs_ar & (AR_TYPE_ACCESSES_MASK|AR_TYPE_CODE_MASK)) )
+			return false;
+		if( ((ss_ar & AR_TYPE_MASK) != 3) || ((ss_ar & AR_TYPE_MASK) != 7) )
+			return false;
+		if(! ((ds_ar & AR_TYPE_ACCESSES_MASK) || (es_ar & AR_TYPE_ACCESSES_MASK) || 
+		      (fs_ar & AR_TYPE_ACCESSES_MASK) || (gs_ar & AR_TYPE_ACCESSES_MASK)) )
+			return false;
+		if(! (( (ds_ar & AR_TYPE_CODE_MASK) && (ds_ar & AR_TYPE_READABLE_MASK) ) ||
+		      ( (es_ar & AR_TYPE_CODE_MASK) && (es_ar & AR_TYPE_READABLE_MASK) ) ||
+		      ( (fs_ar & AR_TYPE_CODE_MASK) && (fs_ar & AR_TYPE_READABLE_MASK) ) ||
+		      ( (gs_ar & AR_TYPE_CODE_MASK) && (gs_ar & AR_TYPE_READABLE_MASK) )) )
+			return false;
+
+		/* S field checks */
+		if(! ((cs_ar & AR_S_MASK) || (ds_ar & AR_S_MASK) || (ss_ar & AR_S_MASK) ||
+		      (es_ar & AR_S_MASK) || (fs_ar & AR_S_MASK) || (gs_ar & AR_S_MASK)) )
+			return false;
+
+		/* DPL field checks */
+		if( ((cs_ar & AR_TYPE_MASK) <= 0xb) && ((cs_ar & AR_TYPE_MASK) >= 0x8) ) {
+			if(AR_DPL(cs_ar) != (cs & 0x03))
+				return false;
+		}
+		else if( ((cs_ar & AR_TYPE_MASK) <= 0xf) && ((cs_ar & AR_TYPE_MASK) >= 0xc) ) {
+			if(AR_DPL(cs_ar) > (cs & 0x03))
+				return false;
+		}
+
+		if(AR_DPL(ss_ar) != (ss & 0x03))
+			return false;
+
+		if((ds_ar & AR_TYPE_MASK) <= 0xb)
+			if(AR_DPL(ds_ar) < (ds & 0x03))
+				return false;
+		if((es_ar & AR_TYPE_MASK) <= 0xb)
+			if(AR_DPL(es_ar) < (es & 0x03))
+				return false;
+		if((fs_ar & AR_TYPE_MASK) <= 0xb)
+			if(AR_DPL(fs_ar) < (fs & 0x03))
+				return false;
+		if((gs_ar & AR_TYPE_MASK) <= 0xb)
+			if(AR_DPL(gs_ar) < (gs & 0x03))
+				return false; 
+
+		/* P field check */
+		if(! ((cs_ar & AR_P_MASK) || (ds_ar & AR_P_MASK) || (ss_ar & AR_P_MASK) ||
+		      (es_ar & AR_P_MASK) || (fs_ar & AR_P_MASK) || (gs_ar & AR_P_MASK)) )
+			return false;
+
+		/* Reserved fields check */
+		if( (cs_ar & AR_RESERVD_MASK) || (ds_ar & AR_RESERVD_MASK) || 
+		    (ss_ar & AR_RESERVD_MASK) || (es_ar & AR_RESERVD_MASK) ||
+		    (fs_ar & AR_RESERVD_MASK) || (gs_ar & AR_RESERVD_MASK) )
+			return false;
+
+		/* TODO: 
+ 		 * - Add checks on G and D/B fields
+ 		 * - Add checks on the unusable mask
+ 		 */
+	}
+
+	/* TR access rights bits checks */
+	if(tr_ar & AR_S_MASK)
+		return false;
+	if((tr_ar & AR_P_MASK) != 1)
+		return false;
+	if(tr_ar & AR_RESERVD_MASK)
+		return false;
+	if(tr_ar & AR_UNUSABLE_MASK)
+		return false;
+	if((tr_limit & 0x00000fff) != 0x00000fff) {
+		if(tr_ar & AR_G_MASK)
+			return false;
+	}
+	else if(tr_limit & 0xffff0000) {
+		if(!(tr_ar & AR_G_MASK))
+			return false;
+	}
+	
+	/* LDTR access right bits checks */
+	if(!(ldtr_ar & AR_TYPE_LDT))
+		return false;
+	if(ldtr_ar & AR_S_MASK)
+		return false;
+	if((ldtr_ar & AR_P_MASK) != 1)
+		return false;
+	if(ldtr_ar & AR_RESERVD_MASK)
+		return false;
+	if((ldtr_limit & 0x00000fff) != 0x00000fff) {
+		if(ldtr_ar & AR_G_MASK)
+			return false;
+	}
+	else if(ldtr_limit & 0xffff0000) {
+		if(ldtr_ar & AR_G_MASK)
+			return false;
+	}
+
+	/* TODO:
+ 	 * - Add checks on RIP
+ 	 * - Add checks on RFLAGS
+ 	 */
+ 	
+	return true;
+}
+
 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);
@@ -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;
+		}
+	}
 }
 
 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);
 
-	fix_rmode_seg(VCPU_SREG_ES, &vcpu->arch.rmode.es);
-	fix_rmode_seg(VCPU_SREG_DS, &vcpu->arch.rmode.ds);
-	fix_rmode_seg(VCPU_SREG_GS, &vcpu->arch.rmode.gs);
-	fix_rmode_seg(VCPU_SREG_FS, &vcpu->arch.rmode.fs);
+	if(!emulate_invalid_guest_state) {
+		fix_rmode_seg(VCPU_SREG_ES, &vcpu->arch.rmode.es);
+		fix_rmode_seg(VCPU_SREG_DS, &vcpu->arch.rmode.ds);
+		fix_rmode_seg(VCPU_SREG_GS, &vcpu->arch.rmode.gs);
+		fix_rmode_seg(VCPU_SREG_FS, &vcpu->arch.rmode.fs);
+	}
 
 	kvm_mmu_reset_context(vcpu);
 	init_rmode(vcpu->kvm);
+	
+	if(emulate_invalid_guest_state) {
+		while(!guest_state_valid(vcpu)) {
+			if(!invalid_guest_state_handler(vcpu))
+				break;
+		}
+	}
 }
 
 #ifdef CONFIG_X86_64
@@ -2708,6 +2901,29 @@ static int handle_nmi_window(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 	return 1;
 }
 
+static int invalid_guest_state_handler(struct kvm_vcpu *vcpu)
+{
+	u8 opcodes[4];
+	unsigned long rip = kvm_rip_read(vcpu);
+	unsigned long rip_linear;
+	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, NULL, 0, 0, 0);
+		
+	switch (err) {
+		case EMULATE_DONE:
+			return 1;
+		case EMULATE_DO_MMIO:
+			printk(KERN_INFO "mmio?\n");
+			return 0;
+		default:
+			kvm_report_emulation_failure(vcpu, "vmentry failure");
+			return 0;
+	}
+}
+
 /*
  * 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

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

[Guillaume Thouvenin]

On Sun, 3 Aug 2008 16:26:01 +0300
Mohammed Gamal <m.gamal005@gmail.com> wrote:

> [This resend adds the sign-offs and fixes a typo that was in the previous patch]
> 
> This patch aims to allow emulation whenever guest state is not valid for VMX operation, which occurs while trying to emulate big real mode on guests 
> such as older versions of gfxboot and FreeDOS with HIMEM.

I tried the patch with an openSUSE-10.3 and I got the following failure:

[~/work/kvm.git/kvm]$ /home/guill/local/kvm-userspace.git/bin/qemu-system-x86_64 -hda /images/disk/hda_10G.qcow2 -cdrom /images/iso/openSUSE-10.3-GM-x86_64-mini.iso -boot d
unhandled vm exit: 0x80000021 vcpu_id 0
rax 000000000000f002 rbx 000000000000d8d1 rcx 0000000000000000 rdx 0000000000000402
rsi 00000000ffff0000 rdi 0000000000080000 rsp 000000000000fffc rbp 0000000000008271
r8  0000000000000000 r9  0000000000000000 r10 0000000000000000 r11 0000000000000000
r12 0000000000000000 r13 0000000000000000 r14 0000000000000000 r15 0000000000000000
rip 000000000000b1ec rflags 00023046
cs f000 (000f0000/0000ffff p 1 dpl 3 db 0 s 1 type 3 l 0 g 0 avl 0)
ds 0000 (00000000/00000000 p 0 dpl 0 db 0 s 0 type 0 l 0 g 0 avl 0)
es 0000 (00000000/00000000 p 0 dpl 0 db 0 s 0 type 0 l 0 g 0 avl 0)
ss 0000 (00000000/00000000 p 0 dpl 0 db 0 s 0 type 0 l 0 g 0 avl 0)
fs 0000 (00000000/00000000 p 0 dpl 0 db 0 s 0 type 0 l 0 g 0 avl 0)
gs 0000 (00000000/00000000 p 0 dpl 0 db 0 s 0 type 0 l 0 g 0 avl 0)
tr 0000 (fffbd000/00002088 p 1 dpl 0 db 0 s 0 type b l 0 g 0 avl 0)
ldt 0000 (00000000/0000ffff p 1 dpl 0 db 0 s 0 type 2 l 0 g 0 avl 0)
gdt fb1f2/30
idt 0/3ff
cr0 10 cr2 0 cr3 0 cr4 0 cr8 0 efer 0
Aborted

and also:

[ 3379.569530] emulation failed (vmentry failure) rip fff0 c0 20 d4 8b
[ 3380.426135] emulation failed (vmentry failure) rip fb180 fc be 00 00
[ 3380.495821] emulation failed (vmentry failure) rip b1e9 e6 92 c3 30

Best regards,
Guillaume

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

[Mohammed Gamal]

On Mon, Aug 4, 2008 at 11:48 AM, Guillaume Thouvenin
<guillaume.thouvenin@ext.bull.net> wrote:
> On Sun, 3 Aug 2008 16:26:01 +0300
> Mohammed Gamal <m.gamal005@gmail.com> wrote:
>
>> [This resend adds the sign-offs and fixes a typo that was in the previous patch]
>>
>> This patch aims to allow emulation whenever guest state is not valid for VMX operation, which occurs while trying to emulate big real mode on guests
>> such as older versions of gfxboot and FreeDOS with HIMEM.
>
> I tried the patch with an openSUSE-10.3 and I got the following failure:
>
> [~/work/kvm.git/kvm]$ /home/guill/local/kvm-userspace.git/bin/qemu-system-x86_64 -hda /images/disk/hda_10G.qcow2 -cdrom /images/iso/openSUSE-10.3-GM-x86_64-mini.iso -boot d
> unhandled vm exit: 0x80000021 vcpu_id 0
> rax 000000000000f002 rbx 000000000000d8d1 rcx 0000000000000000 rdx 0000000000000402
> rsi 00000000ffff0000 rdi 0000000000080000 rsp 000000000000fffc rbp 0000000000008271
> r8  0000000000000000 r9  0000000000000000 r10 0000000000000000 r11 0000000000000000
> r12 0000000000000000 r13 0000000000000000 r14 0000000000000000 r15 0000000000000000
> rip 000000000000b1ec rflags 00023046
> cs f000 (000f0000/0000ffff p 1 dpl 3 db 0 s 1 type 3 l 0 g 0 avl 0)
> ds 0000 (00000000/00000000 p 0 dpl 0 db 0 s 0 type 0 l 0 g 0 avl 0)
> es 0000 (00000000/00000000 p 0 dpl 0 db 0 s 0 type 0 l 0 g 0 avl 0)
> ss 0000 (00000000/00000000 p 0 dpl 0 db 0 s 0 type 0 l 0 g 0 avl 0)
> fs 0000 (00000000/00000000 p 0 dpl 0 db 0 s 0 type 0 l 0 g 0 avl 0)
> gs 0000 (00000000/00000000 p 0 dpl 0 db 0 s 0 type 0 l 0 g 0 avl 0)
> tr 0000 (fffbd000/00002088 p 1 dpl 0 db 0 s 0 type b l 0 g 0 avl 0)
> ldt 0000 (00000000/0000ffff p 1 dpl 0 db 0 s 0 type 2 l 0 g 0 avl 0)
> gdt fb1f2/30
> idt 0/3ff
> cr0 10 cr2 0 cr3 0 cr4 0 cr8 0 efer 0
> Aborted
>
> and also:
>
> [ 3379.569530] emulation failed (vmentry failure) rip fff0 c0 20 d4 8b
> [ 3380.426135] emulation failed (vmentry failure) rip fb180 fc be 00 00
> [ 3380.495821] emulation failed (vmentry failure) rip b1e9 e6 92 c3 30
>
> Best regards,
> Guillaume
>

I'm getting the same here too. This is because the BIOS jumps between
16-bit protected mode and real mode, causing the VM to be in an
invalid guest state and thereby invoking the emulator which will fail
when encountering an instruction it doesn't include yet. What's needed
to do is add these instructions causing the failure to the x86
emulator.

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

[Avi Kivity]

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

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

[Mohammed Gamal]

On Sun, Aug 10, 2008 at 11:09 AM, Avi Kivity <avi@qumranet.com> wrote:
> 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.

Why? when cr0.PE bit is cleared, enter_rmode() is called and
vcpu->arch.rmode.active is set, or do you mean it should be checked in
addition?

>
>> +               /* 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.

I think the function can be further broken down into smaller functions
each checking for a certain segment parameter (e.g.
segment_limit_valid(), segment_ar_valid(), rip_valid() ...etc.) and
then call all these functions in guest_state_valid().

>
>> +
>>  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.
>

I've already done that, I'll update my patch to take your suggestions
in consideration.

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

[Avi Kivity]

Mohammed Gamal wrote:
> On Sun, Aug 10, 2008 at 11:09 AM, Avi Kivity <avi@qumranet.com> wrote:
>   
>> 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)
>>> +{
>>>       

...

>>> +       /* vm86 mode guest state checks */
>>> +       if(vcpu->arch.rmode.active) {
>>>
>>>       
>> Better to check cr0 here.
>>     
>
> Why? when cr0.PE bit is cleared, enter_rmode() is called and
> vcpu->arch.rmode.active is set, or do you mean it should be checked in
> addition?
>
>   

No, instead.  If we check cr0 then the function only depends on guest 
state and is therefore easier to understand.

Eventually rmode.active will go away.

>>> +               /* 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.
>>     
>
> I think the function can be further broken down into smaller functions
> each checking for a certain segment parameter (e.g.
> segment_limit_valid(), segment_ar_valid(), rip_valid() ...etc.) and
> then call all these functions in guest_state_valid().
>
>   

Sure.


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