[PATCH 0/7] Fix emulate_invalid_guest_state=0 part 2

[PATCH 0/7] Fix emulate_invalid_guest_state=0 part 2

[Gleb Natapov]

emulate_invalid_guest_state=0 still does not work since last patch of
my previous series was not applied. It causes kvm to emulate code that
previously kvm managed to run in vm86 and this, in turn, causes emulation
errors for instructions kvm does not know how to emulate. Patch 2 of
the series should fix that. Patch 3 is exactly same as 4/4 from previous
series. Other patches are clean-ups.


Gleb Natapov (7):
  KVM: VMX: cleanup rmode_segment_valid()
  KVM: VMX: relax check for CS register in rmode_segment_valid()
  KVM: VMX: return correct segment limit and flags for CS/SS registers
    in real mode
  KVM: VMX: use fix_rmode_seg() to fix all code/data segments
  KVM: VMX: remove redundant code from vmx_set_segment()
  KVM: VMX: clean-up vmx_set_segment()
  KVM: VMX: remove unneeded temporary variable from vmx_set_segment()

 arch/x86/kvm/vmx.c |   73 ++++++++++++++--------------------------------------
 1 file changed, 19 insertions(+), 54 deletions(-)

-- 
1.7.10.4

[PATCH 1/7] KVM: VMX: cleanup rmode_segment_valid()

[Gleb Natapov]

Set segment fields explicitly instead of using  binary operations.

No behaviour changes.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 arch/x86/kvm/vmx.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index d14bb12..4df3991 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3380,13 +3380,16 @@ static bool rmode_segment_valid(struct kvm_vcpu *vcpu, int seg)
 	u32 ar;
 
 	vmx_get_segment(vcpu, &var, seg);
+	var.dpl = 0x3;
+	var.g = 0;
+	var.db = 0;
 	ar = vmx_segment_access_rights(&var);
 
 	if (var.base != (var.selector << 4))
 		return false;
 	if (var.limit < 0xffff)
 		return false;
-	if (((ar | (3 << AR_DPL_SHIFT)) & ~(AR_G_MASK | AR_DB_MASK)) != 0xf3)
+	if (ar != 0xf3)
 		return false;
 
 	return true;
-- 
1.7.10.4

[PATCH 2/7] KVM: VMX: relax check for CS register in rmode_segment_valid()

[Gleb Natapov]

rmode_segment_valid() checks if segment descriptor can be used to enter
vm86 mode. VMX spec mandates that in vm86 mode CS register will be of
type data, not code. Lets allow guest entry with vm86 mode if the only
problem with CS register is incorrect type. Otherwise entire real mode
will be emulated.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 arch/x86/kvm/vmx.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 4df3991..acbe86f 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3383,6 +3383,8 @@ static bool rmode_segment_valid(struct kvm_vcpu *vcpu, int seg)
 	var.dpl = 0x3;
 	var.g = 0;
 	var.db = 0;
+	if (seg == VCPU_SREG_CS)
+		var.type = 0x3;
 	ar = vmx_segment_access_rights(&var);
 
 	if (var.base != (var.selector << 4))
-- 
1.7.10.4

[PATCH 3/7] KVM: VMX: return correct segment limit and flags for CS/SS registers in real mode

[Gleb Natapov]

VMX without unrestricted mode cannot virtualize real mode, so if
emulate_invalid_guest_state=0 kvm uses vm86 mode to approximate
it. Sometimes, when guest moves from protected mode to real mode, it
leaves segment descriptors in a state not suitable for use by vm86 mode
virtualization, so we keep shadow copy of segment descriptors for internal
use and load fake register to VMCS for guest entry to succeed. Till
now we kept shadow for all segments except SS and CS (for SS and CS we
returned parameters directly from VMCS), but since commit a5625189f6810
emulator enforces segment limits in real mode. This causes #GP during move
from protected mode to real mode when emulator fetches first instruction
after moving to real mode since it uses incorrect CS base and limit to
linearize the %rip. Fix by keeping shadow for SS and CS too.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 arch/x86/kvm/vmx.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index acbe86f..a01fbed 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2856,6 +2856,8 @@ static void enter_rmode(struct kvm_vcpu *vcpu)
 	vmx_get_segment(vcpu, &vmx->rmode.segs[VCPU_SREG_DS], VCPU_SREG_DS);
 	vmx_get_segment(vcpu, &vmx->rmode.segs[VCPU_SREG_FS], VCPU_SREG_FS);
 	vmx_get_segment(vcpu, &vmx->rmode.segs[VCPU_SREG_GS], VCPU_SREG_GS);
+	vmx_get_segment(vcpu, &vmx->rmode.segs[VCPU_SREG_SS], VCPU_SREG_SS);
+	vmx_get_segment(vcpu, &vmx->rmode.segs[VCPU_SREG_CS], VCPU_SREG_CS);
 
 	vmx->emulation_required = 1;
 	vmx->rmode.vm86_active = 1;
@@ -3171,10 +3173,7 @@ static void vmx_get_segment(struct kvm_vcpu *vcpu,
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	u32 ar;
 
-	if (vmx->rmode.vm86_active
-	    && (seg == VCPU_SREG_TR || seg == VCPU_SREG_ES
-		|| seg == VCPU_SREG_DS || seg == VCPU_SREG_FS
-		|| seg == VCPU_SREG_GS)) {
+	if (vmx->rmode.vm86_active && seg != VCPU_SREG_LDTR) {
 		*var = vmx->rmode.segs[seg];
 		if (seg == VCPU_SREG_TR
 		    || var->selector == vmx_read_guest_seg_selector(vmx, seg))
-- 
1.7.10.4

[PATCH 4/7] KVM: VMX: use fix_rmode_seg() to fix all code/data segments

[Gleb Natapov]

The code for SS and CS does the same thing fix_rmode_seg() is doing.
Use it instead of hand crafted code.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 arch/x86/kvm/vmx.c |   26 ++------------------------
 1 file changed, 2 insertions(+), 24 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index a01fbed..35cafa9 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3315,30 +3315,8 @@ static void vmx_set_segment(struct kvm_vcpu *vcpu,
 	 * unrestricted guest like Westmere to older host that don't have
 	 * unrestricted guest like Nehelem.
 	 */
-	if (vmx->rmode.vm86_active) {
-		switch (seg) {
-		case VCPU_SREG_CS:
-			vmcs_write32(GUEST_CS_AR_BYTES, 0xf3);
-			vmcs_write32(GUEST_CS_LIMIT, 0xffff);
-			if (vmcs_readl(GUEST_CS_BASE) == 0xffff0000)
-				vmcs_writel(GUEST_CS_BASE, 0xf0000);
-			vmcs_write16(GUEST_CS_SELECTOR,
-				     vmcs_readl(GUEST_CS_BASE) >> 4);
-			break;
-		case VCPU_SREG_ES:
-		case VCPU_SREG_DS:
-		case VCPU_SREG_GS:
-		case VCPU_SREG_FS:
-			fix_rmode_seg(seg, &vmx->rmode.segs[seg]);
-			break;
-		case VCPU_SREG_SS:
-			vmcs_write16(GUEST_SS_SELECTOR,
-				     vmcs_readl(GUEST_SS_BASE) >> 4);
-			vmcs_write32(GUEST_SS_LIMIT, 0xffff);
-			vmcs_write32(GUEST_SS_AR_BYTES, 0xf3);
-			break;
-		}
-	}
+	if (vmx->rmode.vm86_active && var->s)
+		fix_rmode_seg(seg, &vmx->rmode.segs[seg]);
 }
 
 static void vmx_get_cs_db_l_bits(struct kvm_vcpu *vcpu, int *db, int *l)
-- 
1.7.10.4

[PATCH 5/7] KVM: VMX: remove redundant code from vmx_set_segment()

[Gleb Natapov]

Segment descriptor's base is fixed by call to fix_rmode_seg(). Not need
to do it twice.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 arch/x86/kvm/vmx.c |   12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 35cafa9..ff049e7 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3268,7 +3268,7 @@ static void vmx_set_segment(struct kvm_vcpu *vcpu,
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	const struct kvm_vmx_segment_field *sf = &kvm_vmx_segment_fields[seg];
-	u32 ar;
+	u32 ar = 0;
 
 	vmx_segment_cache_clear(vmx);
 
@@ -3280,15 +3280,9 @@ static void vmx_set_segment(struct kvm_vcpu *vcpu,
 	vmcs_writel(sf->base, var->base);
 	vmcs_write32(sf->limit, var->limit);
 	vmcs_write16(sf->selector, var->selector);
-	if (vmx->rmode.vm86_active && var->s) {
+	if (vmx->rmode.vm86_active && var->s)
 		vmx->rmode.segs[seg] = *var;
-		/*
-		 * Hack real-mode segments into vm86 compatibility.
-		 */
-		if (var->base == 0xffff0000 && var->selector == 0xf000)
-			vmcs_writel(sf->base, 0xf0000);
-		ar = 0xf3;
-	} else
+	else
 		ar = vmx_segment_access_rights(var);
 
 	/*
-- 
1.7.10.4

[PATCH 6/7] KVM: VMX: clean-up vmx_set_segment()

[Gleb Natapov]

Move all vm86_active logic into one place.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 arch/x86/kvm/vmx.c |   27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index ff049e7..193697e 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3271,19 +3271,21 @@ static void vmx_set_segment(struct kvm_vcpu *vcpu,
 	u32 ar = 0;
 
 	vmx_segment_cache_clear(vmx);
+	__clear_bit(VCPU_EXREG_CPL, (ulong *)&vcpu->arch.regs_avail);
 
-	if (vmx->rmode.vm86_active && seg == VCPU_SREG_TR) {
-		vmcs_write16(sf->selector, var->selector);
-		vmx->rmode.segs[VCPU_SREG_TR] = *var;
+	if (vmx->rmode.vm86_active && seg != VCPU_SREG_LDTR) {
+		vmx->rmode.segs[seg] = *var;
+		if (seg == VCPU_SREG_TR)
+			vmcs_write16(sf->selector, var->selector);
+		else if (var->s)
+			fix_rmode_seg(seg, &vmx->rmode.segs[seg]);
 		return;
 	}
+
 	vmcs_writel(sf->base, var->base);
 	vmcs_write32(sf->limit, var->limit);
 	vmcs_write16(sf->selector, var->selector);
-	if (vmx->rmode.vm86_active && var->s)
-		vmx->rmode.segs[seg] = *var;
-	else
-		ar = vmx_segment_access_rights(var);
+	ar = vmx_segment_access_rights(var);
 
 	/*
 	 *   Fix the "Accessed" bit in AR field of segment registers for older
@@ -3300,17 +3302,6 @@ static void vmx_set_segment(struct kvm_vcpu *vcpu,
 		ar |= 0x1; /* Accessed */
 
 	vmcs_write32(sf->ar_bytes, ar);
-	__clear_bit(VCPU_EXREG_CPL, (ulong *)&vcpu->arch.regs_avail);
-
-	/*
-	 * Fix segments for real mode guest in hosts that don't have
-	 * "unrestricted_mode" or it was disabled.
-	 * This is done to allow migration of the guests from hosts with
-	 * unrestricted guest like Westmere to older host that don't have
-	 * unrestricted guest like Nehelem.
-	 */
-	if (vmx->rmode.vm86_active && var->s)
-		fix_rmode_seg(seg, &vmx->rmode.segs[seg]);
 }
 
 static void vmx_get_cs_db_l_bits(struct kvm_vcpu *vcpu, int *db, int *l)
-- 
1.7.10.4

[PATCH 7/7] KVM: VMX: remove unneeded temporary variable from vmx_set_segment()

[Gleb Natapov]

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 arch/x86/kvm/vmx.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 193697e..44fb146 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3268,7 +3268,6 @@ static void vmx_set_segment(struct kvm_vcpu *vcpu,
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	const struct kvm_vmx_segment_field *sf = &kvm_vmx_segment_fields[seg];
-	u32 ar = 0;
 
 	vmx_segment_cache_clear(vmx);
 	__clear_bit(VCPU_EXREG_CPL, (ulong *)&vcpu->arch.regs_avail);
@@ -3285,7 +3284,6 @@ static void vmx_set_segment(struct kvm_vcpu *vcpu,
 	vmcs_writel(sf->base, var->base);
 	vmcs_write32(sf->limit, var->limit);
 	vmcs_write16(sf->selector, var->selector);
-	ar = vmx_segment_access_rights(var);
 
 	/*
 	 *   Fix the "Accessed" bit in AR field of segment registers for older
@@ -3299,9 +3297,9 @@ static void vmx_set_segment(struct kvm_vcpu *vcpu,
 	 * kvm hack.
 	 */
 	if (enable_unrestricted_guest && (seg != VCPU_SREG_LDTR))
-		ar |= 0x1; /* Accessed */
+		var->type |= 0x1; /* Accessed */
 
-	vmcs_write32(sf->ar_bytes, ar);
+	vmcs_write32(sf->ar_bytes, vmx_segment_access_rights(var));
 }
 
 static void vmx_get_cs_db_l_bits(struct kvm_vcpu *vcpu, int *db, int *l)
-- 
1.7.10.4

Re: [PATCH 2/7] KVM: VMX: relax check for CS register in rmode_segment_valid()

[Marcelo Tosatti]

On Wed, Dec 12, 2012 at 07:10:50PM +0200, Gleb Natapov wrote:
> rmode_segment_valid() checks if segment descriptor can be used to enter
> vm86 mode. VMX spec mandates that in vm86 mode CS register will be of
> type data, not code. Lets allow guest entry with vm86 mode if the only
> problem with CS register is incorrect type. Otherwise entire real mode
> will be emulated.
> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> ---
>  arch/x86/kvm/vmx.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 4df3991..acbe86f 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -3383,6 +3383,8 @@ static bool rmode_segment_valid(struct kvm_vcpu *vcpu, int seg)
>  	var.dpl = 0x3;
>  	var.g = 0;
>  	var.db = 0;
> +	if (seg == VCPU_SREG_CS)
> +		var.type = 0x3;
>  	ar = vmx_segment_access_rights(&var);
>  
>  	if (var.base != (var.selector << 4))
> -- 
> 1.7.10.4

But with emulate_invalid_guest_state=1, segments are not fixed on 
transition to real mode. So this patch can result in 
invalid guest state entry failure.

Does this defeat the purpose of emulate_invalid_guest_state=1?

Re: [PATCH 0/7] Fix emulate_invalid_guest_state=0 part 2

[Marcelo Tosatti]

On Wed, Dec 12, 2012 at 07:10:48PM +0200, Gleb Natapov wrote:
> emulate_invalid_guest_state=0 still does not work since last patch of
> my previous series was not applied. It causes kvm to emulate code that
> previously kvm managed to run in vm86 and this, in turn, causes emulation
> errors for instructions kvm does not know how to emulate. Patch 2 of
> the series should fix that. Patch 3 is exactly same as 4/4 from previous
> series. Other patches are clean-ups.

Can't see how guest_state_valid() can affect
emulate_invalid_guest_state=0.

For the cleanups, Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>.

Re: [PATCH 2/7] KVM: VMX: relax check for CS register in rmode_segment_valid()

[Gleb Natapov]

On Fri, Dec 21, 2012 at 09:17:16PM -0200, Marcelo Tosatti wrote:
> On Wed, Dec 12, 2012 at 07:10:50PM +0200, Gleb Natapov wrote:
> > rmode_segment_valid() checks if segment descriptor can be used to enter
> > vm86 mode. VMX spec mandates that in vm86 mode CS register will be of
> > type data, not code. Lets allow guest entry with vm86 mode if the only
> > problem with CS register is incorrect type. Otherwise entire real mode
> > will be emulated.
> > 
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > ---
> >  arch/x86/kvm/vmx.c |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index 4df3991..acbe86f 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -3383,6 +3383,8 @@ static bool rmode_segment_valid(struct kvm_vcpu *vcpu, int seg)
> >  	var.dpl = 0x3;
> >  	var.g = 0;
> >  	var.db = 0;
> > +	if (seg == VCPU_SREG_CS)
> > +		var.type = 0x3;
> >  	ar = vmx_segment_access_rights(&var);
> >  
> >  	if (var.base != (var.selector << 4))
> > -- 
> > 1.7.10.4
> 
> But with emulate_invalid_guest_state=1, segments are not fixed on 
> transition to real mode. So this patch can result in 
> invalid guest state entry failure.
> 
At the point of this patch emulate_invalid_guest_state=1 is broken and
sate is fixed unconditionally during the call to vmx_set_segment(). See
big switch at the end of the function there. Note that here
rmode_segment_valid() does not check register properly anyway. It ignores
db value and allow guest entry with limit > 0xffff.  My patches fixes
this. After the patches are applied rmode_segment_valid() still relax
dpl and CS.type check:

	var.dpl = 0x3;
  	if (seg == VCPU_SREG_CS)
   		var.type = 0x3;

but fix_rmode_seg() unconditionally does exactly same:

        var.dpl = 0x3;
        if (seg == VCPU_SREG_CS)
                var.type = 0x3;

> Does this defeat the purpose of emulate_invalid_guest_state=1?
No since changing dpl to 3 and segment register type to 3 does not
affect instruction execution in vm86 mode, so entering vcpu in vm86
mode or fully emulate all instruction will yield exactly same result
no matter what instructions are executed. If we do not relax this check
in rmode_segment_valid() all real mode will be fully emulated because
no guest ever configure CS register to data type and set DPL of all
segments to 3 before entering real mode.

--
			Gleb.

Re: [PATCH 0/7] Fix emulate_invalid_guest_state=0 part 2

[Gleb Natapov]

On Fri, Dec 21, 2012 at 09:55:29PM -0200, Marcelo Tosatti wrote:
> On Wed, Dec 12, 2012 at 07:10:48PM +0200, Gleb Natapov wrote:
> > emulate_invalid_guest_state=0 still does not work since last patch of
> > my previous series was not applied. It causes kvm to emulate code that
> > previously kvm managed to run in vm86 and this, in turn, causes emulation
> > errors for instructions kvm does not know how to emulate. Patch 2 of
> > the series should fix that. Patch 3 is exactly same as 4/4 from previous
> > series. Other patches are clean-ups.
> 
> Can't see how guest_state_valid() can affect
> emulate_invalid_guest_state=0.
> 
Patch 3/7 fixes it. Patch 2/7 is needed for 3/7 to not break
emulate_invalid_guest_state=1.

> For the cleanups, Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>.

--
			Gleb.

Re: [PATCH 2/7] KVM: VMX: relax check for CS register in rmode_segment_valid()

[Marcelo Tosatti]

On Sat, Dec 22, 2012 at 09:02:41AM +0200, Gleb Natapov wrote:
> On Fri, Dec 21, 2012 at 09:17:16PM -0200, Marcelo Tosatti wrote:
> > On Wed, Dec 12, 2012 at 07:10:50PM +0200, Gleb Natapov wrote:
> > > rmode_segment_valid() checks if segment descriptor can be used to enter
> > > vm86 mode. VMX spec mandates that in vm86 mode CS register will be of
> > > type data, not code. Lets allow guest entry with vm86 mode if the only
> > > problem with CS register is incorrect type. Otherwise entire real mode
> > > will be emulated.
> > > 
> > > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > > ---
> > >  arch/x86/kvm/vmx.c |    2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > > index 4df3991..acbe86f 100644
> > > --- a/arch/x86/kvm/vmx.c
> > > +++ b/arch/x86/kvm/vmx.c
> > > @@ -3383,6 +3383,8 @@ static bool rmode_segment_valid(struct kvm_vcpu *vcpu, int seg)
> > >  	var.dpl = 0x3;
> > >  	var.g = 0;
> > >  	var.db = 0;
> > > +	if (seg == VCPU_SREG_CS)
> > > +		var.type = 0x3;
> > >  	ar = vmx_segment_access_rights(&var);
> > >  
> > >  	if (var.base != (var.selector << 4))
> > > -- 
> > > 1.7.10.4
> > 
> > But with emulate_invalid_guest_state=1, segments are not fixed on 
> > transition to real mode. So this patch can result in 
> > invalid guest state entry failure.
> > 
> At the point of this patch emulate_invalid_guest_state=1 is broken and
> sate is fixed unconditionally during the call to vmx_set_segment().

Err, i meant:

"But with emulate_invalid_guest_state=1, CS segment is not fixed
on transition to real mode. So this patch can result in
invalid guest state entry failure."

Which is true, see enter_rmode. So unless i am missing something,
the patch opens the possibility for an invalid vm-entry which was 
covered before.

Should you force vmx_set_segment(CS) with emulate_invalid_guest_state=1?
(then it would be fixed unconditionally).

> See
> big switch at the end of the function there. Note that here
> rmode_segment_valid() does not check register properly anyway. It ignores
> db value and allow guest entry with limit > 0xffff.  My patches fixes
> this. After the patches are applied rmode_segment_valid() still relax
> dpl and CS.type check:
> 
> 	var.dpl = 0x3;
>   	if (seg == VCPU_SREG_CS)
>    		var.type = 0x3;
> 
> but fix_rmode_seg() unconditionally does exactly same:
> 
>         var.dpl = 0x3;
>         if (seg == VCPU_SREG_CS)
>                 var.type = 0x3;
> 
> > Does this defeat the purpose of emulate_invalid_guest_state=1?
> No since changing dpl to 3 and segment register type to 3 does not
> affect instruction execution in vm86 mode, so entering vcpu in vm86
> mode or fully emulate all instruction will yield exactly same result
> no matter what instructions are executed. If we do not relax this check
> in rmode_segment_valid() all real mode will be fully emulated because
> no guest ever configure CS register to data type and set DPL of all
> segments to 3 before entering real mode.
> 
> --
> 			Gleb.

OK, makes sense. See comment above about not fixing CS unconditionally
on eigs=1.

Re: [PATCH 2/7] KVM: VMX: relax check for CS register in rmode_segment_valid()

[Marcelo Tosatti]

On Sat, Dec 22, 2012 at 12:55:43PM -0200, Marcelo Tosatti wrote:
> On Sat, Dec 22, 2012 at 09:02:41AM +0200, Gleb Natapov wrote:
> > On Fri, Dec 21, 2012 at 09:17:16PM -0200, Marcelo Tosatti wrote:
> > > On Wed, Dec 12, 2012 at 07:10:50PM +0200, Gleb Natapov wrote:
> > > > rmode_segment_valid() checks if segment descriptor can be used to enter
> > > > vm86 mode. VMX spec mandates that in vm86 mode CS register will be of
> > > > type data, not code. Lets allow guest entry with vm86 mode if the only
> > > > problem with CS register is incorrect type. Otherwise entire real mode
> > > > will be emulated.
> > > > 
> > > > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > > > ---
> > > >  arch/x86/kvm/vmx.c |    2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > > > index 4df3991..acbe86f 100644
> > > > --- a/arch/x86/kvm/vmx.c
> > > > +++ b/arch/x86/kvm/vmx.c
> > > > @@ -3383,6 +3383,8 @@ static bool rmode_segment_valid(struct kvm_vcpu *vcpu, int seg)
> > > >  	var.dpl = 0x3;
> > > >  	var.g = 0;
> > > >  	var.db = 0;
> > > > +	if (seg == VCPU_SREG_CS)
> > > > +		var.type = 0x3;
> > > >  	ar = vmx_segment_access_rights(&var);
> > > >  
> > > >  	if (var.base != (var.selector << 4))
> > > > -- 
> > > > 1.7.10.4
> > > 
> > > But with emulate_invalid_guest_state=1, segments are not fixed on 
> > > transition to real mode. So this patch can result in 
> > > invalid guest state entry failure.
> > > 
> > At the point of this patch emulate_invalid_guest_state=1 is broken and
> > sate is fixed unconditionally during the call to vmx_set_segment().
> 
> Err, i meant:
> 
> "But with emulate_invalid_guest_state=1, CS segment is not fixed
> on transition to real mode. So this patch can result in
> invalid guest state entry failure."
> 
> Which is true, see enter_rmode. So unless i am missing something,
> the patch opens the possibility for an invalid vm-entry which was 
> covered before.
> 
> Should you force vmx_set_segment(CS) with emulate_invalid_guest_state=1?
> (then it would be fixed unconditionally).

Oh, patch 3 fixes that, sorry. ACK.