[patch 0/3] partially fix Windows reboot-via-triple-fault

[patch 0/3] partially fix Windows reboot-via-triple-fault

[Marcelo Tosatti]

The following patchset fixes task switch problems seen on installation
of SMP Windows (2000, 2003 and supposedly XP).

Windows 2003 reboots fine, but crashes during initialization (separate
problem though, also happens with UP installation or with new qemu-kvm
instance). XP not tested yet.

Windows 2000 is now able to reboot, but crashes early after initialization:

(triple fault generated, reboot)

SIPI to vcpu 1 vector 0x10
SIPI to vcpu 2 vector 0x10
SIPI to vcpu 3 vector 0x10
handle_exception: unexpected, vectoring info 0x80000202 intr info 0x80000b0d
handle_exception: unexpected, vectoring info 0x80000202 intr info 0x80000b0d
pending exception: not handled yet
pending exception: not handled yet

The task switch is initiated via task-gate-on-IDT from an NMI interrupt,
so apparently some state is not properly cleanup up. Ideas?

Anyway, this changes are supposed to be a step in the right direction.


-- 

[patch 1/3] KVM: task switch: segment base is linear address

[Marcelo Tosatti]

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

The segment base is always a linear address, so translate before 
accessing guest memory.

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

Index: kvm/arch/x86/kvm/x86.c
===================================================================
--- kvm.orig/arch/x86/kvm/x86.c
+++ kvm/arch/x86/kvm/x86.c
@@ -3234,6 +3234,7 @@ static void get_segment_descritptor_dtab
 static int load_guest_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector,
 					 struct desc_struct *seg_desc)
 {
+	gpa_t gpa;
 	struct descriptor_table dtable;
 	u16 index = selector >> 3;
 
@@ -3243,13 +3244,16 @@ static int load_guest_segment_descriptor
 		kvm_queue_exception_e(vcpu, GP_VECTOR, selector & 0xfffc);
 		return 1;
 	}
-	return kvm_read_guest(vcpu->kvm, dtable.base + index * 8, seg_desc, 8);
+	gpa = vcpu->arch.mmu.gva_to_gpa(vcpu, dtable.base);
+	gpa += index * 8;
+	return kvm_read_guest(vcpu->kvm, gpa, seg_desc, 8);
 }
 
 /* allowed just for 8 bytes segments */
 static int save_guest_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector,
 					 struct desc_struct *seg_desc)
 {
+	gpa_t gpa;
 	struct descriptor_table dtable;
 	u16 index = selector >> 3;
 
@@ -3257,7 +3261,9 @@ static int save_guest_segment_descriptor
 
 	if (dtable.limit < index * 8 + 7)
 		return 1;
-	return kvm_write_guest(vcpu->kvm, dtable.base + index * 8, seg_desc, 8);
+	gpa = vcpu->arch.mmu.gva_to_gpa(vcpu, dtable.base);
+	gpa += index * 8;
+	return kvm_write_guest(vcpu->kvm, gpa, seg_desc, 8);
 }
 
 static u32 get_tss_base_addr(struct kvm_vcpu *vcpu,
@@ -3269,7 +3275,7 @@ static u32 get_tss_base_addr(struct kvm_
 	base_addr |= (seg_desc->base1 << 16);
 	base_addr |= (seg_desc->base2 << 24);
 
-	return base_addr;
+	return vcpu->arch.mmu.gva_to_gpa(vcpu, base_addr);
 }
 
 static int load_tss_segment32(struct kvm_vcpu *vcpu,

-- 

[patch 2/3] KVM: task switch: use seg regs provided by subarch instead of reading from GDT

[Marcelo Tosatti]

[-- Attachment #1: task-switch-use-virt-seg-info --]
[-- Type: text/plain, Size: 5414 bytes --]

There is no guarantee that the old TSS descriptor in the GDT contains
the proper base address. This is the case for Windows installation's
reboot-via-triplefault.

Use guest registers instead. Also translate the address properly.

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

Index: kvm/arch/x86/kvm/x86.c
===================================================================
--- kvm.orig/arch/x86/kvm/x86.c
+++ kvm/arch/x86/kvm/x86.c
@@ -3278,54 +3278,6 @@ static u32 get_tss_base_addr(struct kvm_
 	return vcpu->arch.mmu.gva_to_gpa(vcpu, base_addr);
 }
 
-static int load_tss_segment32(struct kvm_vcpu *vcpu,
-			      struct desc_struct *seg_desc,
-			      struct tss_segment_32 *tss)
-{
-	u32 base_addr;
-
-	base_addr = get_tss_base_addr(vcpu, seg_desc);
-
-	return kvm_read_guest(vcpu->kvm, base_addr, tss,
-			      sizeof(struct tss_segment_32));
-}
-
-static int save_tss_segment32(struct kvm_vcpu *vcpu,
-			      struct desc_struct *seg_desc,
-			      struct tss_segment_32 *tss)
-{
-	u32 base_addr;
-
-	base_addr = get_tss_base_addr(vcpu, seg_desc);
-
-	return kvm_write_guest(vcpu->kvm, base_addr, tss,
-			       sizeof(struct tss_segment_32));
-}
-
-static int load_tss_segment16(struct kvm_vcpu *vcpu,
-			      struct desc_struct *seg_desc,
-			      struct tss_segment_16 *tss)
-{
-	u32 base_addr;
-
-	base_addr = get_tss_base_addr(vcpu, seg_desc);
-
-	return kvm_read_guest(vcpu->kvm, base_addr, tss,
-			      sizeof(struct tss_segment_16));
-}
-
-static int save_tss_segment16(struct kvm_vcpu *vcpu,
-			      struct desc_struct *seg_desc,
-			      struct tss_segment_16 *tss)
-{
-	u32 base_addr;
-
-	base_addr = get_tss_base_addr(vcpu, seg_desc);
-
-	return kvm_write_guest(vcpu->kvm, base_addr, tss,
-			       sizeof(struct tss_segment_16));
-}
-
 static u16 get_segment_selector(struct kvm_vcpu *vcpu, int seg)
 {
 	struct kvm_segment kvm_seg;
@@ -3482,20 +3434,26 @@ static int load_state_from_tss16(struct 
 }
 
 static int kvm_task_switch_16(struct kvm_vcpu *vcpu, u16 tss_selector,
-		       struct desc_struct *cseg_desc,
+		       u32 old_tss_base,
 		       struct desc_struct *nseg_desc)
 {
 	struct tss_segment_16 tss_segment_16;
 	int ret = 0;
 
-	if (load_tss_segment16(vcpu, cseg_desc, &tss_segment_16))
+	if (kvm_read_guest(vcpu->kvm, old_tss_base, &tss_segment_16,
+			   sizeof tss_segment_16))
 		goto out;
 
 	save_state_to_tss16(vcpu, &tss_segment_16);
-	save_tss_segment16(vcpu, cseg_desc, &tss_segment_16);
 
-	if (load_tss_segment16(vcpu, nseg_desc, &tss_segment_16))
+	if (kvm_write_guest(vcpu->kvm, old_tss_base, &tss_segment_16,
+			    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))
+		goto out;
+
 	if (load_state_from_tss16(vcpu, &tss_segment_16))
 		goto out;
 
@@ -3505,20 +3463,26 @@ out:
 }
 
 static int kvm_task_switch_32(struct kvm_vcpu *vcpu, u16 tss_selector,
-		       struct desc_struct *cseg_desc,
+		       u32 old_tss_base,
 		       struct desc_struct *nseg_desc)
 {
 	struct tss_segment_32 tss_segment_32;
 	int ret = 0;
 
-	if (load_tss_segment32(vcpu, cseg_desc, &tss_segment_32))
+	if (kvm_read_guest(vcpu->kvm, old_tss_base, &tss_segment_32,
+			   sizeof tss_segment_32))
 		goto out;
 
 	save_state_to_tss32(vcpu, &tss_segment_32);
-	save_tss_segment32(vcpu, cseg_desc, &tss_segment_32);
 
-	if (load_tss_segment32(vcpu, nseg_desc, &tss_segment_32))
+	if (kvm_write_guest(vcpu->kvm, old_tss_base, &tss_segment_32,
+			    sizeof tss_segment_32))
+		goto out;
+
+	if (kvm_read_guest(vcpu->kvm, get_tss_base_addr(vcpu, nseg_desc),
+			   &tss_segment_32, sizeof tss_segment_32))
 		goto out;
+
 	if (load_state_from_tss32(vcpu, &tss_segment_32))
 		goto out;
 
@@ -3533,16 +3497,20 @@ int kvm_task_switch(struct kvm_vcpu *vcp
 	struct desc_struct cseg_desc;
 	struct desc_struct nseg_desc;
 	int ret = 0;
+	u32 old_tss_base = get_segment_base(vcpu, VCPU_SREG_TR);
+	u16 old_tss_sel = get_segment_selector(vcpu, VCPU_SREG_TR);
 
-	kvm_get_segment(vcpu, &tr_seg, VCPU_SREG_TR);
+	old_tss_base = vcpu->arch.mmu.gva_to_gpa(vcpu, old_tss_base);
 
+	/* FIXME: Handle errors. Failure to read either TSS or their
+ 	 * descriptors should generate a pagefault.
+ 	 */
 	if (load_guest_segment_descriptor(vcpu, tss_selector, &nseg_desc))
 		goto out;
 
-	if (load_guest_segment_descriptor(vcpu, tr_seg.selector, &cseg_desc))
+	if (load_guest_segment_descriptor(vcpu, old_tss_sel, &cseg_desc))
 		goto out;
 
-
 	if (reason != TASK_SWITCH_IRET) {
 		int cpl;
 
@@ -3560,8 +3528,7 @@ int kvm_task_switch(struct kvm_vcpu *vcp
 
 	if (reason == TASK_SWITCH_IRET || reason == TASK_SWITCH_JMP) {
 		cseg_desc.type &= ~(1 << 1); //clear the B flag
-		save_guest_segment_descriptor(vcpu, tr_seg.selector,
-					      &cseg_desc);
+		save_guest_segment_descriptor(vcpu, old_tss_sel, &cseg_desc);
 	}
 
 	if (reason == TASK_SWITCH_IRET) {
@@ -3572,10 +3539,10 @@ int kvm_task_switch(struct kvm_vcpu *vcp
 	kvm_x86_ops->skip_emulated_instruction(vcpu);
 
 	if (nseg_desc.type & 8)
-		ret = kvm_task_switch_32(vcpu, tss_selector, &cseg_desc,
+		ret = kvm_task_switch_32(vcpu, tss_selector, old_tss_base,
 					 &nseg_desc);
 	else
-		ret = kvm_task_switch_16(vcpu, tss_selector, &cseg_desc,
+		ret = kvm_task_switch_16(vcpu, tss_selector, old_tss_base,
 					 &nseg_desc);
 
 	if (reason == TASK_SWITCH_CALL || reason == TASK_SWITCH_GATE) {

-- 

[patch 3/3] KVM: VMX: handle segment limit granularity special case in software

[Marcelo Tosatti]

[-- Attachment #1: vmx-seg-limit-bits-0-11-zeroed --]
[-- Type: text/plain, Size: 1157 bytes --]

As the comment in the diff mentions, VMX does not accept any bit in
the range 11:0 of ES,CS,FS,GS,SS segment registers limit field to 
be zero with the granulity bit set to one.

So clear granularity and adjust the limit accordingly. 

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

Index: kvm/arch/x86/kvm/vmx.c
===================================================================
--- kvm.orig/arch/x86/kvm/vmx.c
+++ kvm/arch/x86/kvm/vmx.c
@@ -1665,6 +1665,22 @@ static void vmx_set_segment(struct kvm_v
 		return;
 	}
 	vmcs_writel(sf->base, var->base);
+
+	/*
+ 	 * section 22.3.1.2:
+ 	 * - If any bit in the limit field in the range 11:0 is 0, G must be 0.
+ 	 * - If any bit in the limit field in the range 31:20 is 1, G must be 1.
+ 	 */
+	if (!vcpu->arch.rmode.active && !var->unusable &&
+	     seg != VCPU_SREG_TR && seg != VCPU_SREG_LDTR) {
+#define SEG_MASK ((1 << 12)-1)
+		if (var->g && (var->limit & SEG_MASK) != SEG_MASK) {
+			var->g = 0;
+			var->limit <<= 12;
+			var->limit |= SEG_MASK;
+		}
+	}
+
 	vmcs_write32(sf->limit, var->limit);
 	vmcs_write16(sf->selector, var->selector);
 	if (vcpu->arch.rmode.active && var->s) {

-- 

Re: [patch 3/3] KVM: VMX: handle segment limit granularity special case in software

[Avi Kivity]

Marcelo Tosatti wrote:
> As the comment in the diff mentions, VMX does not accept any bit in
> the range 11:0 of ES,CS,FS,GS,SS segment registers limit field to 
> be zero with the granulity bit set to one.
>
> So clear granularity and adjust the limit accordingly. 
>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>
> Index: kvm/arch/x86/kvm/vmx.c
> ===================================================================
> --- kvm.orig/arch/x86/kvm/vmx.c
> +++ kvm/arch/x86/kvm/vmx.c
> @@ -1665,6 +1665,22 @@ static void vmx_set_segment(struct kvm_v
>  		return;
>  	}
>  	vmcs_writel(sf->base, var->base);
> +
> +	/*
> + 	 * section 22.3.1.2:
> + 	 * - If any bit in the limit field in the range 11:0 is 0, G must be 0.
> + 	 * - If any bit in the limit field in the range 31:20 is 1, G must be 1.
> + 	 */
> +	if (!vcpu->arch.rmode.active && !var->unusable &&
> +	     seg != VCPU_SREG_TR && seg != VCPU_SREG_LDTR) {
> +#define SEG_MASK ((1 << 12)-1)
> +		if (var->g && (var->limit & SEG_MASK) != SEG_MASK) {
> +			var->g = 0;
> +			var->limit <<= 12;
> +			var->limit |= SEG_MASK;
> +		}
> +	}
> +

Both kvm_segment::limit and vmx's GUEST_xS_LIMIT are normalized (always 
in bytes), so I don't see why you are modifying var->limit (which is an 
input parameter!)


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

Re: [patch 3/3] KVM: VMX: handle segment limit granularity special case in software

[Marcelo Tosatti]

On Thu, Jul 17, 2008 at 01:03:57PM +0300, Avi Kivity wrote:
> Marcelo Tosatti wrote:
>> As the comment in the diff mentions, VMX does not accept any bit in
>> the range 11:0 of ES,CS,FS,GS,SS segment registers limit field to be 
>> zero with the granulity bit set to one.
>>
>> So clear granularity and adjust the limit accordingly. 
>>
>> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>>
>> Index: kvm/arch/x86/kvm/vmx.c
>> ===================================================================
>> --- kvm.orig/arch/x86/kvm/vmx.c
>> +++ kvm/arch/x86/kvm/vmx.c
>> @@ -1665,6 +1665,22 @@ static void vmx_set_segment(struct kvm_v
>>  		return;
>>  	}
>>  	vmcs_writel(sf->base, var->base);
>> +
>> +	/*
>> + 	 * section 22.3.1.2:
>> + 	 * - If any bit in the limit field in the range 11:0 is 0, G must be 0.
>> + 	 * - If any bit in the limit field in the range 31:20 is 1, G must be 1.
>> + 	 */
>> +	if (!vcpu->arch.rmode.active && !var->unusable &&
>> +	     seg != VCPU_SREG_TR && seg != VCPU_SREG_LDTR) {
>> +#define SEG_MASK ((1 << 12)-1)
>> +		if (var->g && (var->limit & SEG_MASK) != SEG_MASK) {
>> +			var->g = 0;
>> +			var->limit <<= 12;
>> +			var->limit |= SEG_MASK;
>> +		}
>> +	}
>> +
>
> Both kvm_segment::limit and vmx's GUEST_xS_LIMIT are normalized (always  
> in bytes), so I don't see why you are modifying var->limit (which is an  
> input parameter!)


The problem is the Windows new TSS's FS segment:

unhandled vm exit: 0x80000021 vcpu_id 2
rax 0000000000000000 rbx 0000000000000000 rcx 0000000000000000 rdx
0000000000000000
rsi 0000000000000000 rdi 0000000000000000 rsp 00000000fd6b73c0 rbp
0000000000000000
r8  0000000000000000 r9  0000000000000000 r10 0000000000000000 r11
0000000000000000
r12 0000000000000000 r13 0000000000000000 r14 0000000000000000 r15
0000000000000000
rip 000000008088ab72 rflags 00004002
cs 0008 (00000000/000fffff p 1 dpl 0 db 1 s 1 type b l 0 g 1 avl 0)
ds 0023 (00000000/000fffff p 1 dpl 3 db 1 s 1 type 3 l 0 g 1 avl 0)
es 0023 (00000000/000fffff p 1 dpl 3 db 1 s 1 type 3 l 0 g 1 avl 0)
ss 0010 (00000000/000fffff p 1 dpl 0 db 1 s 1 type 3 l 0 g 1 avl 0)
fs 0030 (fffffffffd6b1000/00000001 p 1 dpl 0 db 1 s 1 type 3 l 0 g 1 avl
                          ^^^^^^^                                ^^^

"section 22.3.1.2:
 - If any bit in the limit field in the range 11:0 is 0, G must be 0."

So this patch fixes that particular issue by setting G to 0 (G=1 ignores
the 12 least significant bits of the offset when comparing the address
against the segment limit), then shifts left the limit by 12, and sets
those 12 bits.

I don't understand what you mean by "vmx's GUEST_sX_LIMIT are
normalized".

Do you have a better suggestion on how to deal with this? Or is it
supposed to by handled somewhere already?

Thanks

Re: [patch 3/3] KVM: VMX: handle segment limit granularity special case in software

[Avi Kivity]

Marcelo Tosatti wrote:
> On Thu, Jul 17, 2008 at 01:03:57PM +0300, Avi Kivity wrote:
>   
>> Marcelo Tosatti wrote:
>>     
>>> As the comment in the diff mentions, VMX does not accept any bit in
>>> the range 11:0 of ES,CS,FS,GS,SS segment registers limit field to be 
>>> zero with the granulity bit set to one.
>>>
>>> So clear granularity and adjust the limit accordingly. 
>>>
>>> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>>>
>>> Index: kvm/arch/x86/kvm/vmx.c
>>> ===================================================================
>>> --- kvm.orig/arch/x86/kvm/vmx.c
>>> +++ kvm/arch/x86/kvm/vmx.c
>>> @@ -1665,6 +1665,22 @@ static void vmx_set_segment(struct kvm_v
>>>  		return;
>>>  	}
>>>  	vmcs_writel(sf->base, var->base);
>>> +
>>> +	/*
>>> + 	 * section 22.3.1.2:
>>> + 	 * - If any bit in the limit field in the range 11:0 is 0, G must be 0.
>>> + 	 * - If any bit in the limit field in the range 31:20 is 1, G must be 1.
>>> + 	 */
>>> +	if (!vcpu->arch.rmode.active && !var->unusable &&
>>> +	     seg != VCPU_SREG_TR && seg != VCPU_SREG_LDTR) {
>>> +#define SEG_MASK ((1 << 12)-1)
>>> +		if (var->g && (var->limit & SEG_MASK) != SEG_MASK) {
>>> +			var->g = 0;
>>> +			var->limit <<= 12;
>>> +			var->limit |= SEG_MASK;
>>> +		}
>>> +	}
>>> +
>>>       
>> Both kvm_segment::limit and vmx's GUEST_xS_LIMIT are normalized (always  
>> in bytes), so I don't see why you are modifying var->limit (which is an  
>> input parameter!)
>>     
>
>
> The problem is the Windows new TSS's FS segment:
>
> unhandled vm exit: 0x80000021 vcpu_id 2
> rax 0000000000000000 rbx 0000000000000000 rcx 0000000000000000 rdx
> 0000000000000000
> rsi 0000000000000000 rdi 0000000000000000 rsp 00000000fd6b73c0 rbp
> 0000000000000000
> r8  0000000000000000 r9  0000000000000000 r10 0000000000000000 r11
> 0000000000000000
> r12 0000000000000000 r13 0000000000000000 r14 0000000000000000 r15
> 0000000000000000
> rip 000000008088ab72 rflags 00004002
> cs 0008 (00000000/000fffff p 1 dpl 0 db 1 s 1 type b l 0 g 1 avl 0)
> ds 0023 (00000000/000fffff p 1 dpl 3 db 1 s 1 type 3 l 0 g 1 avl 0)
> es 0023 (00000000/000fffff p 1 dpl 3 db 1 s 1 type 3 l 0 g 1 avl 0)
> ss 0010 (00000000/000fffff p 1 dpl 0 db 1 s 1 type 3 l 0 g 1 avl 0)
> fs 0030 (fffffffffd6b1000/00000001 p 1 dpl 0 db 1 s 1 type 3 l 0 g 1 avl
>                           ^^^^^^^                                ^^^
>
> "section 22.3.1.2:
>  - If any bit in the limit field in the range 11:0 is 0, G must be 0."
>
> So this patch fixes that particular issue by setting G to 0 (G=1 ignores
> the 12 least significant bits of the offset when comparing the address
> against the segment limit), then shifts left the limit by 12, and sets
> those 12 bits.
>
> I don't understand what you mean by "vmx's GUEST_sX_LIMIT are
> normalized".
>
>   

I meant, they are always in bytes.  In a descriptor, the limit is in 
bytes or pages, depending on the g bit.

> Do you have a better suggestion on how to deal with this? Or is it
> supposed to by handled somewhere already?

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.

Most likely a simple

    if (seg_desc->g)
        kvm_desct->limit <<= 12;

will suffice.

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

Re: [patch 0/3] partially fix Windows reboot-via-triple-fault

[Avi Kivity]

Marcelo Tosatti wrote:
> The following patchset fixes task switch problems seen on installation
> of SMP Windows (2000, 2003 and supposedly XP).
>
> Windows 2003 reboots fine, but crashes during initialization (separate
> problem though, also happens with UP installation or with new qemu-kvm
> instance). XP not tested yet.
>
> Windows 2000 is now able to reboot, but crashes early after initialization:
>
> (triple fault generated, reboot)
>
> SIPI to vcpu 1 vector 0x10
> SIPI to vcpu 2 vector 0x10
> SIPI to vcpu 3 vector 0x10
> handle_exception: unexpected, vectoring info 0x80000202 intr info 0x80000b0d
> handle_exception: unexpected, vectoring info 0x80000202 intr info 0x80000b0d
> pending exception: not handled yet
> pending exception: not handled yet
>
> The task switch is initiated via task-gate-on-IDT from an NMI interrupt,
> so apparently some state is not properly cleanup up. Ideas?
>
> Anyway, this changes are supposed to be a step in the right direction.
>   
   
Applied the first two patched; see comments on the third.

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