[PATCH] When switching to a vm8086 task, load segments as 16-bit

[PATCH] When switching to a vm8086 task, load segments as 16-bit

[Anthony Liguori]

According to 16.2.5 in the SDM, eflags.vm in the tss is consulted before loading
and new segments.  If eflags.vm == 1, then the segments are treated as 16-bit
segments.

This fixes an invalid vmentry failure in a custom OS that was happening after
a task switch into vm8086 mode.  Since the segments were being mistakenly
treated as 32-bit, we loaded garbage state.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 arch/x86/kvm/x86.c |   23 +++++++++++++++++------
 1 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6263991..48b1be9 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4050,6 +4050,16 @@ static void save_state_to_tss32(struct kvm_vcpu *vcpu,
 	tss->ldt_selector = get_segment_selector(vcpu, VCPU_SREG_LDTR);
 }
 
+static int load_seg_from_tss32(struct kvm_vcpu *vcpu,
+                               struct tss_segment_32 *tss,
+			       u16 selector, int type_bits, int seg)
+{
+	if ((tss->eflags & X86_EFLAGS_VM))
+		return kvm_load_realmode_segment(vcpu, selector, seg);
+
+	return kvm_load_segment_descriptor(vcpu, selector, type_bits, seg);
+}
+
 static int load_state_from_tss32(struct kvm_vcpu *vcpu,
 				  struct tss_segment_32 *tss)
 {
@@ -4070,23 +4080,24 @@ static int load_state_from_tss32(struct kvm_vcpu *vcpu,
 	if (kvm_load_segment_descriptor(vcpu, tss->ldt_selector, 0, VCPU_SREG_LDTR))
 		return 1;
 
-	if (kvm_load_segment_descriptor(vcpu, tss->es, 1, VCPU_SREG_ES))
+	if (load_seg_from_tss32(vcpu, tss, tss->es, 1, VCPU_SREG_ES))
 		return 1;
 
-	if (kvm_load_segment_descriptor(vcpu, tss->cs, 9, VCPU_SREG_CS))
+	if (load_seg_from_tss32(vcpu, tss, tss->cs, 9, VCPU_SREG_CS))
 		return 1;
 
-	if (kvm_load_segment_descriptor(vcpu, tss->ss, 1, VCPU_SREG_SS))
+	if (load_seg_from_tss32(vcpu, tss, tss->ss, 1, VCPU_SREG_SS))
 		return 1;
 
-	if (kvm_load_segment_descriptor(vcpu, tss->ds, 1, VCPU_SREG_DS))
+	if (load_seg_from_tss32(vcpu, tss, tss->ds, 1, VCPU_SREG_DS))
 		return 1;
 
-	if (kvm_load_segment_descriptor(vcpu, tss->fs, 1, VCPU_SREG_FS))
+	if (load_seg_from_tss32(vcpu, tss, tss->fs, 1, VCPU_SREG_FS))
 		return 1;
 
-	if (kvm_load_segment_descriptor(vcpu, tss->gs, 1, VCPU_SREG_GS))
+	if (load_seg_from_tss32(vcpu, tss, tss->gs, 1, VCPU_SREG_GS))
 		return 1;
+
 	return 0;
 }
 
-- 
1.6.2.5

Re: [PATCH] When switching to a vm8086 task, load segments as 16-bit

[Avi Kivity]

On 08/11/2009 04:00 AM, Anthony Liguori wrote:
> According to 16.2.5 in the SDM, eflags.vm in the tss is consulted before loading
> and new segments.  If eflags.vm == 1, then the segments are treated as 16-bit
> segments.
>
> This fixes an invalid vmentry failure in a custom OS that was happening after
> a task switch into vm8086 mode.  Since the segments were being mistakenly
> treated as 32-bit, we loaded garbage state.
>
> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
> ---
>   arch/x86/kvm/x86.c |   23 +++++++++++++++++------
>   1 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 6263991..48b1be9 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4050,6 +4050,16 @@ static void save_state_to_tss32(struct kvm_vcpu *vcpu,
>   	tss->ldt_selector = get_segment_selector(vcpu, VCPU_SREG_LDTR);
>   }
>
> +static int load_seg_from_tss32(struct kvm_vcpu *vcpu,
> +                               struct tss_segment_32 *tss,
> +			       u16 selector, int type_bits, int seg)
> +{
> +	if ((tss->eflags&  X86_EFLAGS_VM))
> +		return kvm_load_realmode_segment(vcpu, selector, seg);
> +
> +	return kvm_load_segment_descriptor(vcpu, selector, type_bits, seg);
> +}
> +
>    

kvm_load_segment_descriptor() can already load plain segments:

     if (!(vcpu->arch.cr0 & X86_CR0_PE))
         return kvm_load_realmode_segment(vcpu, selector, seg);

so we can simplify the patch a bit by extending the check.

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

Re: [PATCH] When switching to a vm8086 task, load segments as 16-bit

[Anthony Liguori]

Avi Kivity wrote:
> On 08/11/2009 04:00 AM, Anthony Liguori wrote:
>
> kvm_load_segment_descriptor() can already load plain segments:
>
>     if (!(vcpu->arch.cr0 & X86_CR0_PE))
>         return kvm_load_realmode_segment(vcpu, selector, seg);
>
> so we can simplify the patch a bit by extending the check.

I went down that road but you need to check the to-be-switched-into tss, 
which isn't available directly from vcpu so we would have to either pass 
the tss as a parameter to the function or at least a flag indicating 
that we're going into vm8086 mode.

There are other callers of kvm_load_segment_descriptor that would need 
updating and it already takes a few extra parameters.  That's why I went 
down this road.

Happy to change though if you'd prefer a different approach.

-- 
Regards,

Anthony Liguori

Re: [PATCH] When switching to a vm8086 task, load segments as 16-bit

[Avi Kivity]

On 08/11/2009 07:45 PM, Anthony Liguori wrote:
> Avi Kivity wrote:
>> On 08/11/2009 04:00 AM, Anthony Liguori wrote:
>>
>> kvm_load_segment_descriptor() can already load plain segments:
>>
>>     if (!(vcpu->arch.cr0 & X86_CR0_PE))
>>         return kvm_load_realmode_segment(vcpu, selector, seg);
>>
>> so we can simplify the patch a bit by extending the check.
>
> I went down that road but you need to check the to-be-switched-into 
> tss, which isn't available directly from vcpu so we would have to 
> either pass the tss as a parameter to the function or at least a flag 
> indicating that we're going into vm8086 mode.
>

eflags is already updated at this point, so no need for an additional 
parameter.

> There are other callers of kvm_load_segment_descriptor that would need 
> updating and it already takes a few extra parameters.  That's why I 
> went down this road.

The other callers are a good reason to do it in 
kvm_load_segment_descriptor(), so they don't have to perform the check 
themselves.  In fact with this change your patch will fix a few more bugs.

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