[patch 0/3] task switch fixes

[patch 0/3] task switch fixes

[Marcelo Tosatti]

Some more fixes for the task switch emulation.

> I think the problem is in seg_desct_to_kvm_desct() (besides the extra
> T's).  It copies the limit from the descriptor directly to the kvm_segment
> structure.

You're right.

After fixing that 2003 Server task switches successfully to an EIP that
contains junk, a few UD's are injected and then a GP, which BSOD's
asking for a reboot.

All task switch state is valid, can't find anything that would generate
any exception. And even if it did, #GP and #TS are handled with a BSOD.

Xen has this special case for when the TSS's first 104 bytes cross a page
boundary (docs mention this should be avoided since processor uses the
physical addresses as base), but not the case with 2003.

XP sets CR3 with invalid bits. Xen simply resets the guest in that case,
KVM could do the same.


-- 

[patch 1/3] KVM: task switch: translate guest segment limit to virt-extension byte granular field

[Marcelo Tosatti]

[-- Attachment #1: vmx-seg-limti --]
[-- Type: text/plain, Size: 995 bytes --]

If 'g' is one then limit is 4kb granular.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: kvm-vmx-checks/arch/x86/kvm/x86.c
===================================================================
--- kvm-vmx-checks.orig/arch/x86/kvm/x86.c
+++ kvm-vmx-checks/arch/x86/kvm/x86.c
@@ -3195,6 +3195,10 @@ static void seg_desct_to_kvm_desct(struc
 	kvm_desct->base |= seg_desc->base2 << 24;
 	kvm_desct->limit = seg_desc->limit0;
 	kvm_desct->limit |= seg_desc->limit << 16;
+	if (seg_desc->g) {
+		kvm_desct->limit <<= 12;
+		kvm_desct->limit |= 0xfff;
+	}
 	kvm_desct->selector = selector;
 	kvm_desct->type = seg_desc->type;
 	kvm_desct->present = seg_desc->p;
@@ -3222,8 +3226,12 @@ static void get_segment_descritptor_dtab
 
 		if (kvm_seg.unusable)
 			dtable->limit = 0;
-		else
-			dtable->limit = kvm_seg.limit;
+		else {
+			if (kvm_seg.g)
+				dtable->limit = kvm_seg.limit >> 12;
+			else
+				dtable->limit = kvm_seg.limit;
+		}
 		dtable->base = kvm_seg.base;
 	}
 	else

-- 

[patch 2/3] KVM: task switch: check task busy state

[Marcelo Tosatti]

[-- Attachment #1: task-switch-checks --]
[-- Type: text/plain, Size: 1433 bytes --]

Checks that the new task is available (call, jump, exception, or interrupt) or 
busy (IRET return). Generate GP# or TS# otherwise.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: kvm-vmx-checks/arch/x86/kvm/x86.c
===================================================================
--- kvm-vmx-checks.orig/arch/x86/kvm/x86.c
+++ kvm-vmx-checks/arch/x86/kvm/x86.c
@@ -3519,6 +3519,11 @@ int kvm_task_switch(struct kvm_vcpu *vcp
 	if (load_guest_segment_descriptor(vcpu, old_tss_sel, &cseg_desc))
 		goto out;
 
+	if (!nseg_desc.p || (nseg_desc.limit0 | nseg_desc.limit << 16) < 0x67) {
+		kvm_queue_exception_e(vcpu, TS_VECTOR, tss_selector & 0xfffc);
+		return 1;
+	}
+
 	if (reason != TASK_SWITCH_IRET) {
 		int cpl;
 
@@ -3527,12 +3532,19 @@ int kvm_task_switch(struct kvm_vcpu *vcp
 			kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
 			return 1;
 		}
+		if (nseg_desc.type & (1 << 1)) {
+			kvm_queue_exception_e(vcpu, GP_VECTOR,
+					      old_tss_sel & 0xfffc);
+			return 1;
+		}
+	} else {
+		if (!(cseg_desc.type & (1 << 1))) {
+			kvm_queue_exception_e(vcpu, TS_VECTOR,
+					      tss_selector & 0xfffc);
+			return 1;
+		}
 	}
 
-	if (!nseg_desc.p || (nseg_desc.limit0 | nseg_desc.limit << 16) < 0x67) {
-		kvm_queue_exception_e(vcpu, TS_VECTOR, tss_selector & 0xfffc);
-		return 1;
-	}
 
 	if (reason == TASK_SWITCH_IRET || reason == TASK_SWITCH_JMP) {
 		cseg_desc.type &= ~(1 << 1); //clear the B flag

-- 

[patch 3/3] KVM: task switch: check for segment base translation failure

[Marcelo Tosatti]

[-- Attachment #1: task-switch-checks-2 --]
[-- Type: text/plain, Size: 2768 bytes --]

Subject says it all.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: kvm-vmx-checks/arch/x86/kvm/x86.c
===================================================================
--- kvm-vmx-checks.orig/arch/x86/kvm/x86.c
+++ kvm-vmx-checks/arch/x86/kvm/x86.c
@@ -3253,6 +3253,8 @@ static int load_guest_segment_descriptor
 		return 1;
 	}
 	gpa = vcpu->arch.mmu.gva_to_gpa(vcpu, dtable.base);
+	if (gpa == UNMAPPED_GVA)
+		return 1;
 	gpa += index * 8;
 	return kvm_read_guest(vcpu->kvm, gpa, seg_desc, 8);
 }
@@ -3270,11 +3272,13 @@ static int save_guest_segment_descriptor
 	if (dtable.limit < index * 8 + 7)
 		return 1;
 	gpa = vcpu->arch.mmu.gva_to_gpa(vcpu, dtable.base);
+	if (gpa == UNMAPPED_GVA)
+		return 1;
 	gpa += index * 8;
 	return kvm_write_guest(vcpu->kvm, gpa, seg_desc, 8);
 }
 
-static u32 get_tss_base_addr(struct kvm_vcpu *vcpu,
+static gpa_t get_tss_base_addr(struct kvm_vcpu *vcpu,
 			     struct desc_struct *seg_desc)
 {
 	u32 base_addr;
@@ -3446,8 +3450,13 @@ static int kvm_task_switch_16(struct kvm
 		       struct desc_struct *nseg_desc)
 {
 	struct tss_segment_16 tss_segment_16;
+	gpa_t tss_base;
 	int ret = 0;
 
+	tss_base = get_tss_base_addr(vcpu, nseg_desc);
+	if (tss_base == UNMAPPED_GVA)
+		goto out;
+
 	if (kvm_read_guest(vcpu->kvm, old_tss_base, &tss_segment_16,
 			   sizeof tss_segment_16))
 		goto out;
@@ -3458,8 +3467,8 @@ static int kvm_task_switch_16(struct kvm
 			    sizeof tss_segment_16))
 		goto out;
 
-	if (kvm_read_guest(vcpu->kvm, get_tss_base_addr(vcpu, nseg_desc),
-			   &tss_segment_16, sizeof tss_segment_16))
+	if (kvm_read_guest(vcpu->kvm, tss_base, &tss_segment_16,
+			   sizeof tss_segment_16))
 		goto out;
 
 	if (load_state_from_tss16(vcpu, &tss_segment_16))
@@ -3475,8 +3484,13 @@ static int kvm_task_switch_32(struct kvm
 		       struct desc_struct *nseg_desc)
 {
 	struct tss_segment_32 tss_segment_32;
+	gpa_t tss_base;
 	int ret = 0;
 
+	tss_base = get_tss_base_addr(vcpu, nseg_desc);
+	if (tss_base == UNMAPPED_GVA)
+		goto out;
+
 	if (kvm_read_guest(vcpu->kvm, old_tss_base, &tss_segment_32,
 			   sizeof tss_segment_32))
 		goto out;
@@ -3487,7 +3501,7 @@ static int kvm_task_switch_32(struct kvm
 			    sizeof tss_segment_32))
 		goto out;
 
-	if (kvm_read_guest(vcpu->kvm, get_tss_base_addr(vcpu, nseg_desc),
+	if (kvm_read_guest(vcpu->kvm, tss_base,
 			   &tss_segment_32, sizeof tss_segment_32))
 		goto out;
 
@@ -3509,6 +3523,8 @@ int kvm_task_switch(struct kvm_vcpu *vcp
 	u16 old_tss_sel = get_segment_selector(vcpu, VCPU_SREG_TR);
 
 	old_tss_base = vcpu->arch.mmu.gva_to_gpa(vcpu, old_tss_base);
+	if (old_tss_base == UNMAPPED_GVA)
+		return 1;
 
 	/* FIXME: Handle errors. Failure to read either TSS or their
  	 * descriptors should generate a pagefault.

-- 

Re: [patch 1/3] KVM: task switch: translate guest segment limit to virt-extension byte granular field

[Avi Kivity]

Marcelo Tosatti wrote:
> If 'g' is one then limit is 4kb granular.
>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>
> Index: kvm-vmx-checks/arch/x86/kvm/x86.c
> ===================================================================
> --- kvm-vmx-checks.orig/arch/x86/kvm/x86.c
> +++ kvm-vmx-checks/arch/x86/kvm/x86.c
> @@ -3195,6 +3195,10 @@ static void seg_desct_to_kvm_desct(struc
>  	kvm_desct->base |= seg_desc->base2 << 24;
>  	kvm_desct->limit = seg_desc->limit0;
>  	kvm_desct->limit |= seg_desc->limit << 16;
> +	if (seg_desc->g) {
> +		kvm_desct->limit <<= 12;
> +		kvm_desct->limit |= 0xfff;
> +	}
>  	kvm_desct->selector = selector;
>  	kvm_desct->type = seg_desc->type;
>  	kvm_desct->present = seg_desc->p;
>   

This looks good,

> @@ -3222,8 +3226,12 @@ static void get_segment_descritptor_dtab
>  
>  		if (kvm_seg.unusable)
>  			dtable->limit = 0;
> -		else
> -			dtable->limit = kvm_seg.limit;
> +		else {
> +			if (kvm_seg.g)
> +				dtable->limit = kvm_seg.limit >> 12;
> +			else
> +				dtable->limit = kvm_seg.limit;
> +		}
>  		dtable->base = kvm_seg.base;
>  	

But this doesn't.  As far as I can tell, users of 
get_segment_descritptor_dtable() expect a normalized limit (always in 
bytes).

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

Re: [patch 3/3] KVM: task switch: check for segment base translation failure

[Avi Kivity]

Marcelo Tosatti wrote:
> Subject says it all.
>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>
> Index: kvm-vmx-checks/arch/x86/kvm/x86.c
> ===================================================================
> --- kvm-vmx-checks.orig/arch/x86/kvm/x86.c
> +++ kvm-vmx-checks/arch/x86/kvm/x86.c
> @@ -3253,6 +3253,8 @@ static int load_guest_segment_descriptor
>  		return 1;
>  	}
>  	gpa = vcpu->arch.mmu.gva_to_gpa(vcpu, dtable.base);
> +	if (gpa == UNMAPPED_GVA)
> +		return 1;
>  	gpa += index * 8;
>  	return kvm_read_guest(vcpu->kvm, gpa, seg_desc, 8);
>  }
>   

This is wrong; if the descriptor table is long enough, the first page 
could be unmapped but the page(s) containing the segment could be mapped 
(and nothing guarantees the mapping is contiguous).

We need to translate dtable.base + index * 8.

What we really need is kvm_read_guest_virt() to take care of all of 
these things.  The emulator callbacks come fairly close.

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

Re: [patch 1/3] KVM: task switch: translate guest segment limit to virt-extension byte granular field

[Marcelo Tosatti]

On Sun, Jul 20, 2008 at 12:22:07PM +0300, Avi Kivity wrote:
> Marcelo Tosatti wrote:
>> If 'g' is one then limit is 4kb granular.
>>
>> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>>
>> Index: kvm-vmx-checks/arch/x86/kvm/x86.c
>> ===================================================================
>> --- kvm-vmx-checks.orig/arch/x86/kvm/x86.c
>> +++ kvm-vmx-checks/arch/x86/kvm/x86.c
>> @@ -3195,6 +3195,10 @@ static void seg_desct_to_kvm_desct(struc
>>  	kvm_desct->base |= seg_desc->base2 << 24;
>>  	kvm_desct->limit = seg_desc->limit0;
>>  	kvm_desct->limit |= seg_desc->limit << 16;
>> +	if (seg_desc->g) {
>> +		kvm_desct->limit <<= 12;
>> +		kvm_desct->limit |= 0xfff;
>> +	}
>>  	kvm_desct->selector = selector;
>>  	kvm_desct->type = seg_desc->type;
>>  	kvm_desct->present = seg_desc->p;
>>   
>
> This looks good,
>
>> @@ -3222,8 +3226,12 @@ static void get_segment_descritptor_dtab
>>   		if (kvm_seg.unusable)
>>  			dtable->limit = 0;
>> -		else
>> -			dtable->limit = kvm_seg.limit;
>> +		else {
>> +			if (kvm_seg.g)
>> +				dtable->limit = kvm_seg.limit >> 12;
>> +			else
>> +				dtable->limit = kvm_seg.limit;
>> +		}
>>  		dtable->base = kvm_seg.base;
>>  	
>
> But this doesn't.  As far as I can tell, users of  
> get_segment_descritptor_dtable() expect a normalized limit (always in  
> bytes).

Ouch, yes. Here's it:


KVM: task switch: translate guest segment limit to virt-extension byte granular field

If 'g' is one then limit is 4kb granular.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: kvm-vmx-checks/arch/x86/kvm/x86.c
===================================================================
--- kvm-vmx-checks.orig/arch/x86/kvm/x86.c
+++ kvm-vmx-checks/arch/x86/kvm/x86.c
@@ -3195,6 +3195,10 @@ static void seg_desct_to_kvm_desct(struc
 	kvm_desct->base |= seg_desc->base2 << 24;
 	kvm_desct->limit = seg_desc->limit0;
 	kvm_desct->limit |= seg_desc->limit << 16;
+	if (seg_desc->g) {
+		kvm_desct->limit <<= 12;
+		kvm_desct->limit |= 0xfff;
+	}
 	kvm_desct->selector = selector;
 	kvm_desct->type = seg_desc->type;
 	kvm_desct->present = seg_desc->p;

Re: [patch 1/3] KVM: task switch: translate guest segment limit to virt-extension byte granular field

[Avi Kivity]

Marcelo Tosatti wrote:
> On Sun, Jul 20, 2008 at 12:22:07PM +0300, Avi Kivity wrote:
>   
>> Marcelo Tosatti wrote:
>>     
>>> If 'g' is one then limit is 4kb granular.
>>>
>>> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>>>
>>> Index: kvm-vmx-checks/arch/x86/kvm/x86.c
>>> ===================================================================
>>> --- kvm-vmx-checks.orig/arch/x86/kvm/x86.c
>>> +++ kvm-vmx-checks/arch/x86/kvm/x86.c
>>> @@ -3195,6 +3195,10 @@ static void seg_desct_to_kvm_desct(struc
>>>  	kvm_desct->base |= seg_desc->base2 << 24;
>>>  	kvm_desct->limit = seg_desc->limit0;
>>>  	kvm_desct->limit |= seg_desc->limit << 16;
>>> +	if (seg_desc->g) {
>>> +		kvm_desct->limit <<= 12;
>>> +		kvm_desct->limit |= 0xfff;
>>> +	}
>>>  	kvm_desct->selector = selector;
>>>  	kvm_desct->type = seg_desc->type;
>>>  	kvm_desct->present = seg_desc->p;
>>>   
>>>       
>> This looks good,
>>
>>     
>>> @@ -3222,8 +3226,12 @@ static void get_segment_descritptor_dtab
>>>   		if (kvm_seg.unusable)
>>>  			dtable->limit = 0;
>>> -		else
>>> -			dtable->limit = kvm_seg.limit;
>>> +		else {
>>> +			if (kvm_seg.g)
>>> +				dtable->limit = kvm_seg.limit >> 12;
>>> +			else
>>> +				dtable->limit = kvm_seg.limit;
>>> +		}
>>>  		dtable->base = kvm_seg.base;
>>>  	
>>>       
>> But this doesn't.  As far as I can tell, users of  
>> get_segment_descritptor_dtable() expect a normalized limit (always in  
>> bytes).
>>     
>
> Ouch, yes. Here's it:
>
>
> KVM: task switch: translate guest segment limit to virt-extension byte granular field
>
> If 'g' is one then limit is 4kb granular.
>
>   

I already merged this bit (I guess I forgot to ack); aren't you reading 
kvm-commits@?

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