[BUG] 2.5.65: Caching MSR_IA32_SYSENTER_CS kills dosemu

[BUG] 2.5.65: Caching MSR_IA32_SYSENTER_CS kills dosemu

[Wayne Whitney]

[This is a repost with a more descriptive subject.]

I run dosemu 1.0.2.1 on the 2.5.x kernels.  Upgrading from 2.5.64 to
2.5.65 (or 2.5.66) causes dosemu to no longer work:  it locks up the
machine shortly after I run it.  Alt-Sysrq still works OK.  I traced the
problem to the "Cache MSR_IA32_SYSENTER_CS value in the per-CPU TSS"
changeset introduced in 2.5.65-bk5, which I have included as a patch
below.  Backing this patch out from 2.5.65-bk5 makes dosemu work again.  
Any idea what is going on?

Cheers, Wayne


diff -ru linux-2.5.64-bk4/arch/i386/kernel/sysenter.c linux-2.5.64-bk5/arch/i386/kernel/sysenter.c
--- linux-2.5.64-bk4/arch/i386/kernel/sysenter.c	2003-03-26 21:06:54.000000000 -0800
+++ linux-2.5.64-bk5/arch/i386/kernel/sysenter.c	2003-03-26 13:55:08.000000000 -0800
@@ -40,6 +40,7 @@
 	int cpu = get_cpu();
 	struct tss_struct *tss = init_tss + cpu;
 
+	tss->ss1 = __KERNEL_CS;
 	wrmsr(MSR_IA32_SYSENTER_CS, __KERNEL_CS, 0);
 	wrmsr(MSR_IA32_SYSENTER_ESP, tss->esp0, 0);
 	wrmsr(MSR_IA32_SYSENTER_EIP, (unsigned long) sysenter_entry, 0);
diff -ru linux-2.5.64-bk4/arch/i386/kernel/vm86.c linux-2.5.64-bk5/arch/i386/kernel/vm86.c
--- linux-2.5.64-bk4/arch/i386/kernel/vm86.c	2003-03-26 21:06:54.000000000 -0800
+++ linux-2.5.64-bk5/arch/i386/kernel/vm86.c	2003-03-26 13:55:08.000000000 -0800
@@ -291,7 +291,7 @@
 
 	tss = init_tss + smp_processor_id();
 	tss->esp0 = tsk->thread.esp0 = (unsigned long) &info->VM86_TSS_ESP0;
-	disable_sysenter();
+	disable_sysenter(tss);
 
 	tsk->thread.screen_bitmap = info->screen_bitmap;
 	if (info->flags & VM86_SCREEN_BITMAP)
diff -ru linux-2.5.64-bk4/include/asm-i386/processor.h linux-2.5.64-bk5/include/asm-i386/processor.h
--- linux-2.5.64-bk4/include/asm-i386/processor.h	2003-03-26 21:06:55.000000000 -0800
+++ linux-2.5.64-bk5/include/asm-i386/processor.h	2003-03-26 13:55:09.000000000 -0800
@@ -347,7 +347,7 @@
 	unsigned long	esp0;
 	unsigned short	ss0,__ss0h;
 	unsigned long	esp1;
-	unsigned short	ss1,__ss1h;
+	unsigned short	ss1,__ss1h;	/* ss1 is used to cache MSR_IA32_SYSENTER_CS */
 	unsigned long	esp2;
 	unsigned short	ss2,__ss2h;
 	unsigned long	__cr3;
@@ -413,15 +413,20 @@
 {
 	tss->esp0 = esp0;
 	if (cpu_has_sep) {
-		wrmsr(MSR_IA32_SYSENTER_CS, __KERNEL_CS, 0);
+		if (tss->ss1 != __KERNEL_CS) {
+			tss->ss1 = __KERNEL_CS;
+			wrmsr(MSR_IA32_SYSENTER_CS, __KERNEL_CS, 0);
+		}
 		wrmsr(MSR_IA32_SYSENTER_ESP, esp0, 0);
 	}
 }
 
-static inline void disable_sysenter(void)
+static inline void disable_sysenter(struct tss_struct *tss)
 {
-	if (cpu_has_sep)  
+	if (cpu_has_sep)  {
+		tss->ss1 = 0;
 		wrmsr(MSR_IA32_SYSENTER_CS, 0, 0);
+	}
 }
 
 #define start_thread(regs, new_eip, new_esp) do {		\

Re: [BUG] 2.5.65: Caching MSR_IA32_SYSENTER_CS kills dosemu

[Linus Torvalds]

On Mon, 31 Mar 2003, Wayne Whitney wrote:
>
> I run dosemu 1.0.2.1 on the 2.5.x kernels.  Upgrading from 2.5.64 to
> 2.5.65 (or 2.5.66) causes dosemu to no longer work:  it locks up the
> machine shortly after I run it.

There appears to be at least one bug (that is longstanding but might be 
made worse by the MSR stuff). However, that one should matter only with 
preemption enabled. What's your configuration?

Also, do you actually have a new library that uses SYSENTER (ie recent 
redhat beta), and whct kind of CPU do you have?

		Linus


----
===== arch/i386/kernel/vm86.c 1.21 vs edited =====
--- 1.21/arch/i386/kernel/vm86.c	Sun Mar  9 18:49:37 2003
+++ edited/arch/i386/kernel/vm86.c	Mon Mar 31 11:23:50 2003
@@ -289,9 +289,10 @@
 	asm volatile("movl %%fs,%0":"=m" (tsk->thread.saved_fs));
 	asm volatile("movl %%gs,%0":"=m" (tsk->thread.saved_gs));
 
-	tss = init_tss + smp_processor_id();
+	tss = init_tss + get_cpu();
 	tss->esp0 = tsk->thread.esp0 = (unsigned long) &info->VM86_TSS_ESP0;
 	disable_sysenter(tss);
+	put_cpu();
 
 	tsk->thread.screen_bitmap = info->screen_bitmap;
 	if (info->flags & VM86_SCREEN_BITMAP)

Re: [BUG] 2.5.65: Caching MSR_IA32_SYSENTER_CS kills dosemu

[Wayne Whitney]

On Mon, 31 Mar 2003, Linus Torvalds wrote:

> On Mon, 31 Mar 2003, Wayne Whitney wrote:
> >
> > I run dosemu 1.0.2.1 on the 2.5.x kernels.  Upgrading from 2.5.64 to
> > 2.5.65 (or 2.5.66) causes dosemu to no longer work:  it locks up the
> > machine shortly after I run it.
> 
> There appears to be at least one bug (that is longstanding but might be 
> made worse by the MSR stuff). However, that one should matter only with 
> preemption enabled. What's your configuration?

UP with preempt.  2.5.66 with the patch you sent still locks up.  I should
mention that I am running two copies of a hacked XFree86 on two different
sets of KVM hardware, but that doesn't require any kernel patches (well, a
small one to the input layer).

> Also, do you actually have a new library that uses SYSENTER (ie recent 
> redhat beta), and whct kind of CPU do you have?

Well, I have glibc-2.3.1-51, from RedHat Rawhide February 20, so it sounds
like that uses SYSENTER.  The CPU is an Althon XP, stepping 6-6-2.

Cheers, Wayne

Re: [BUG] 2.5.65: Caching MSR_IA32_SYSENTER_CS kills dosemu

[Linus Torvalds]

On Mon, 31 Mar 2003, Wayne Whitney wrote:
> 
> UP with preempt.  2.5.66 with the patch you sent still locks up.  I should
> mention that I am running two copies of a hacked XFree86 on two different
> sets of KVM hardware, but that doesn't require any kernel patches (well, a
> small one to the input layer).

Can you check dosemu in text mode to see if the kernel prints out
anything. I realize that not many things are relevant in text mode, but I
have this memory of dosemu at least historically supporting it. Maybe not
any more.

		Linus

Re: [BUG] 2.5.65: Caching MSR_IA32_SYSENTER_CS kills dosemu

[Wayne Whitney]

On Mon, 31 Mar 2003, Linus Torvalds wrote:

> Can you check dosemu in text mode to see if the kernel prints out
> anything. I realize that not many things are relevant in text mode, but
> I have this memory of dosemu at least historically supporting it. Maybe
> not any more.

OK, running dosemu at the console does not cause a lock up.  More
specifically:  just running dosemu doesn't cause a lock up ever.  To get
it to lock up, I have to run this game I play.  Under X, just running the
game and waiting at the game's main menu causes the lock up in a couple
minutes.  At the console, the game's graphics mode is not supported, so I
get a blank screen.  But if I run the game blind and wait at the main
menu, there is never a lock up.

Also, it seems that turning off preempt makes the problem go away.  At
least, I haven't got any lockups yet.

Cheers, Wayne

Re: [BUG] 2.5.65: Caching MSR_IA32_SYSENTER_CS kills dosemu

[Linus Torvalds]

On Mon, 31 Mar 2003, Wayne Whitney wrote:
> 
> Also, it seems that turning off preempt makes the problem go away.  At
> least, I haven't got any lockups yet.

Can you test this patch? It turns out that "get_cpu()/put_cpu()" are not 
enough - on UP they don't actually disable preemption, since the CPU 
number itself is perfectly stable at 0, of course.

But we need to disable preemption while we re-load esp0, since if we
schedule at that point our esp0 cache in the TSS structures will become
out-of-sync with the actual value of the CPU esp0.

You'd obviously have to re-enable preemption to test this, please..

			Linus

---
===== arch/i386/kernel/vm86.c 1.22 vs edited =====
--- 1.22/arch/i386/kernel/vm86.c	Mon Mar 31 14:30:01 2003
+++ edited/arch/i386/kernel/vm86.c	Tue Apr  1 13:25:28 2003
@@ -113,10 +113,14 @@
 		printk("vm86: could not access userspace vm86_info\n");
 		do_exit(SIGSEGV);
 	}
+
+	preempt_disable();
 	tss = init_tss + smp_processor_id();
 	current->thread.esp0 = current->thread.saved_esp0;
 	load_esp0(tss, current->thread.esp0);
 	current->thread.saved_esp0 = 0;
+	preempt_enable();
+
 	loadsegment(fs, current->thread.saved_fs);
 	loadsegment(gs, current->thread.saved_gs);
 	ret = KVM86->regs32;
@@ -289,10 +293,11 @@
 	asm volatile("movl %%fs,%0":"=m" (tsk->thread.saved_fs));
 	asm volatile("movl %%gs,%0":"=m" (tsk->thread.saved_gs));
 
-	tss = init_tss + get_cpu();
+	preempt_disable();
+	tss = init_tss + smp_processor_id();
 	tss->esp0 = tsk->thread.esp0 = (unsigned long) &info->VM86_TSS_ESP0;
 	disable_sysenter(tss);
-	put_cpu();
+	preempt_enable();
 
 	tsk->thread.screen_bitmap = info->screen_bitmap;
 	if (info->flags & VM86_SCREEN_BITMAP)

Re: [BUG] 2.5.65: Caching MSR_IA32_SYSENTER_CS kills dosemu

[Robert Love]

On Tue, 2003-04-01 at 16:28, Linus Torvalds wrote:

> Can you test this patch? It turns out that "get_cpu()/put_cpu()" are not 
> enough - on UP they don't actually disable preemption, since the CPU 
> number itself is perfectly stable at 0, of course.

Actually, do they do disable preemption - if they do not, something is
broken.

Because, even on UP, preemption can lead to a race over a variable that
has no locking because its per-CPU.  But it would need locking
otherwise, and thus we do need to disable preemption.  I.e., per-CPU
vars are only safe on SMP because we assume the other processors won't
touch them.  If we start preempting, even on UP, we have problems.

So something else is amiss here...

	Robert Love

Re: [BUG] 2.5.65: Caching MSR_IA32_SYSENTER_CS kills dosemu

[Linus Torvalds]

On 1 Apr 2003, Robert Love wrote:
> 
> Actually, do they do disable preemption - if they do not, something is
> broken.

Ok, I was too lazy to check. Anyway, the test-patch is worth trying, since
one of the areas fixed had no pre-emption protection regardless (ie it
used just a plain "smp_processor_id()"), and the patch should be
technically equivalent (just uglier) to a proper get_cpu().

> Because, even on UP, preemption can lead to a race over a variable that
> has no locking because its per-CPU.  But it would need locking
> otherwise, and thus we do need to disable preemption.

Yes, that was certainly the case here.

		Linus

Re: [BUG] 2.5.65: Caching MSR_IA32_SYSENTER_CS kills dosemu

[Robert Love]

On Tue, 2003-04-01 at 18:13, Linus Torvalds wrote:

> Ok, I was too lazy to check. Anyway, the test-patch is worth trying, since
> one of the areas fixed had no pre-emption protection regardless (ie it
> used just a plain "smp_processor_id()"), and the patch should be
> technically equivalent (just uglier) to a proper get_cpu().

Yah, I just looked, and I noticed that too.

Whether or not this fixes it for Wayne, I think the right thing to do is
replace the first smp_processor_id() with get_cpu(), too.

	Robert Love

Re: [BUG] 2.5.65: Caching MSR_IA32_SYSENTER_CS kills dosemu

[Wayne Whitney]

On Tue, 1 Apr 2003, Linus Torvalds wrote:

> Can you test this patch? 

This patch (on top of the previous one) does the trick for me.  Thanks!

Wayne

> ===== arch/i386/kernel/vm86.c 1.22 vs edited =====
> --- 1.22/arch/i386/kernel/vm86.c	Mon Mar 31 14:30:01 2003
> +++ edited/arch/i386/kernel/vm86.c	Tue Apr  1 13:25:28 2003
> @@ -113,10 +113,14 @@
>  		printk("vm86: could not access userspace vm86_info\n");
>  		do_exit(SIGSEGV);
>  	}
> +
> +	preempt_disable();
>  	tss = init_tss + smp_processor_id();
>  	current->thread.esp0 = current->thread.saved_esp0;
>  	load_esp0(tss, current->thread.esp0);
>  	current->thread.saved_esp0 = 0;
> +	preempt_enable();
> +
>  	loadsegment(fs, current->thread.saved_fs);
>  	loadsegment(gs, current->thread.saved_gs);
>  	ret = KVM86->regs32;
> @@ -289,10 +293,11 @@
>  	asm volatile("movl %%fs,%0":"=m" (tsk->thread.saved_fs));
>  	asm volatile("movl %%gs,%0":"=m" (tsk->thread.saved_gs));
>  
> -	tss = init_tss + get_cpu();
> +	preempt_disable();
> +	tss = init_tss + smp_processor_id();
>  	tss->esp0 = tsk->thread.esp0 = (unsigned long) &info->VM86_TSS_ESP0;
>  	disable_sysenter(tss);
> -	put_cpu();
> +	preempt_enable();
>  
>  	tsk->thread.screen_bitmap = info->screen_bitmap;
>  	if (info->flags & VM86_SCREEN_BITMAP)