[PATCH] Mask mxcsr according to cpu features.

[PATCH] Mask mxcsr according to cpu features.

[paubert]

[CC'ed to x86_64 and ia64 maintainers because they might have the 
same issues. For existing x86_64 processors, s/0xffbf/0xffff/ in 
arch/x86-64/ia32/{fpu32,ptrace32}.c might be sufficient]

With SSE2, mxcsr bit 6 is defined as controlling whether
denormals should be treated as zeroes or not. Setting it
no more causes an exception, but with the current code it 
would be cleared at every signal return which is a bit harsh.

The following patch fixes this (2.5, but easily ported to 2.4).

===== arch/i386/kernel/i387.c 1.16 vs edited =====
--- 1.16/arch/i386/kernel/i387.c	Wed Apr  9 05:45:37 2003
+++ edited/arch/i386/kernel/i387.c	Thu May  8 23:30:23 2003
@@ -25,6 +25,12 @@
 #define HAVE_HWFP 1
 #endif
 
+/* mxcsr bits 31-16 must be zero for security reasons,
+ * bit 6 depends on cpu features.
+ */
+#define MXCSR_MASK (cpu_has_sse2 ? 0xffff : 0xffbf)
+
+
 /*
  * The _current_ task is using the FPU for the first time
  * so initialize it and set the mxcsr to its default
@@ -208,7 +214,7 @@
 void set_fpu_mxcsr( struct task_struct *tsk, unsigned short mxcsr )
 {
 	if ( cpu_has_xmm ) {
-		tsk->thread.i387.fxsave.mxcsr = (mxcsr & 0xffbf);
+		tsk->thread.i387.fxsave.mxcsr = (mxcsr & MXCSR_MASK);
 	}
 }
 
@@ -356,8 +362,7 @@
 	clear_fpu( tsk );
 	err = __copy_from_user( &tsk->thread.i387.fxsave, &buf->_fxsr_env[0],
 				sizeof(struct i387_fxsave_struct) );
-	/* mxcsr bit 6 and 31-16 must be zero for security reasons */
-	tsk->thread.i387.fxsave.mxcsr &= 0xffbf;
+	tsk->thread.i387.fxsave.mxcsr &= MXCSR_MASK;
 	return err ? 1 : convert_fxsr_from_user( &tsk->thread.i387.fxsave, buf );
 }
 
@@ -455,8 +460,7 @@
 	if ( cpu_has_fxsr ) {
 		__copy_from_user( &tsk->thread.i387.fxsave, buf,
 				  sizeof(struct user_fxsr_struct) );
-		/* mxcsr bit 6 and 31-16 must be zero for security reasons */
-		tsk->thread.i387.fxsave.mxcsr &= 0xffbf;
+		tsk->thread.i387.fxsave.mxcsr &= MXCSR_MASK;
 		return 0;
 	} else {
 		return -EIO;

	Gabriel

Re: [PATCH] Mask mxcsr according to cpu features.

[Andi Kleen]

On Fri, May 09, 2003 at 12:42:01AM +0000, paubert wrote:
> 
> [CC'ed to x86_64 and ia64 maintainers because they might have the 
> same issues. For existing x86_64 processors, s/0xffbf/0xffff/ in 
> arch/x86-64/ia32/{fpu32,ptrace32}.c might be sufficient]
> 
> With SSE2, mxcsr bit 6 is defined as controlling whether
> denormals should be treated as zeroes or not. Setting it
> no more causes an exception, but with the current code it 
> would be cleared at every signal return which is a bit harsh.
> 
> The following patch fixes this (2.5, but easily ported to 2.4).

x86-64 does it in a different way. It just handles the 
possible exception on FXRSTOR with an __ex_table handler.
With that all the mxcsr masking can be dropped.

It was already this way for 64bit programs, but the 32bit emulation
still masks. I'm not sure I can change that - in theory it could
break existing programs.

-Andi

Re: [PATCH] Mask mxcsr according to cpu features.

[paubert]

On Fri, May 09, 2003 at 04:24:17AM +0200, Andi Kleen wrote:
> On Fri, May 09, 2003 at 12:42:01AM +0000, paubert wrote:
> > 
> > [CC'ed to x86_64 and ia64 maintainers because they might have the 
> > same issues. For existing x86_64 processors, s/0xffbf/0xffff/ in 
> > arch/x86-64/ia32/{fpu32,ptrace32}.c might be sufficient]
> > 
> > With SSE2, mxcsr bit 6 is defined as controlling whether
> > denormals should be treated as zeroes or not. Setting it
> > no more causes an exception, but with the current code it 
> > would be cleared at every signal return which is a bit harsh.
> > 
> > The following patch fixes this (2.5, but easily ported to 2.4).
> 
> x86-64 does it in a different way. It just handles the 
> possible exception on FXRSTOR with an __ex_table handler.
> With that all the mxcsr masking can be dropped.
> 
> It was already this way for 64bit programs, 

I know, that's why I only listed files in the ia32 directory

> but the 32bit emulation still masks. I'm not sure I can 
> change that - in theory it could break existing programs.

I only ask you to change the mask to reflect what the hardware 
allows, not removing the masking, which could have more corner
cases side effects.

Clearing the DAZ bit of every 32 bit program as soon
as it receives a signal can't be right.

	Gabriel 

Re: [PATCH] Mask mxcsr according to cpu features.

[Andi Kleen]

> Clearing the DAZ bit of every 32 bit program as soon
> as it receives a signal can't be right.

No doubt, but changing could still break things. Perhaps there is some 
program that depends on this behaviour (sets the bit by mistake and is 
safed by some signal), and debugging such things is always
nasty. I try to keep the 32bit emulation bug-to-bug compatible to i386

-Andi

Re: [PATCH] Mask mxcsr according to cpu features.

[Philippe Elie]

paubert wrote:
> [CC'ed to x86_64 and ia64 maintainers because they might have the 
> same issues. For existing x86_64 processors, s/0xffbf/0xffff/ in 
> arch/x86-64/ia32/{fpu32,ptrace32}.c might be sufficient]
> 
> With SSE2, mxcsr bit 6 is defined as controlling whether
> denormals should be treated as zeroes or not. Setting it
> no more causes an exception, but with the current code it 
> would be cleared at every signal return which is a bit harsh.
> 
> The following patch fixes this (2.5, but easily ported to 2.4).
> 
> ===== arch/i386/kernel/i387.c 1.16 vs edited =====
> --- 1.16/arch/i386/kernel/i387.c	Wed Apr  9 05:45:37 2003
> +++ edited/arch/i386/kernel/i387.c	Thu May  8 23:30:23 2003
> @@ -25,6 +25,12 @@
>  #define HAVE_HWFP 1
>  #endif
>  
> +/* mxcsr bits 31-16 must be zero for security reasons,
> + * bit 6 depends on cpu features.
> + */
> +#define MXCSR_MASK (cpu_has_sse2 ? 0xffff : 0xffbf)
> +
> +

I don't think daz bit depend on sse2, it's a separate features

>  /*
>   * The _current_ task is using the FPU for the first time
>   * so initialize it and set the mxcsr to its default
> @@ -208,7 +214,7 @@
>  void set_fpu_mxcsr( struct task_struct *tsk, unsigned short mxcsr )
>  {
>  	if ( cpu_has_xmm ) {
> -		tsk->thread.i387.fxsave.mxcsr = (mxcsr & 0xffbf);
> +		tsk->thread.i387.fxsave.mxcsr = (mxcsr & MXCSR_MASK);


intel and x64 doc advocate to use

mxcsr &= tsk->thread.i387.fxsave.mxscr_mask
	? 0xffbf : tsk->thread.i387.fxsave.mxscr_mask;
tsk->thread.i387.fxsave.mxscr =  mxcsr;

with mxscr_mask the 16 upper bits of field currently named
mxscr in fxsave area. This bits was zeroed by previous processor
so this must be backward compatible.


Intel P4 Vol 1 11.6.6 "Guidelines for writing to MXSCR register"
AMD x64 Vol 2 11.4.4 "Saving and restoring Media and x87 processor"


regards,
Philippe Elie

Re: [PATCH] Mask mxcsr according to cpu features.

[paubert]

On Fri, May 09, 2003 at 06:32:00AM +0000, Philippe Elie wrote:
> paubert wrote:
> >[CC'ed to x86_64 and ia64 maintainers because they might have the 
> >same issues. For existing x86_64 processors, s/0xffbf/0xffff/ in 
> >arch/x86-64/ia32/{fpu32,ptrace32}.c might be sufficient]
> >
> >With SSE2, mxcsr bit 6 is defined as controlling whether
> >denormals should be treated as zeroes or not. Setting it
> >no more causes an exception, but with the current code it 
> >would be cleared at every signal return which is a bit harsh.
> >
> >The following patch fixes this (2.5, but easily ported to 2.4).
> >
> >===== arch/i386/kernel/i387.c 1.16 vs edited =====
> >--- 1.16/arch/i386/kernel/i387.c	Wed Apr  9 05:45:37 2003
> >+++ edited/arch/i386/kernel/i387.c	Thu May  8 23:30:23 2003
> >@@ -25,6 +25,12 @@
> > #define HAVE_HWFP 1
> > #endif
> > 
> >+/* mxcsr bits 31-16 must be zero for security reasons,
> >+ * bit 6 depends on cpu features.
> >+ */
> >+#define MXCSR_MASK (cpu_has_sse2 ? 0xffff : 0xffbf)
> >+
> >+
> 
> I don't think daz bit depend on sse2, it's a separate features

The doc I have state in several places:
"The denormals-are-zeros mode was introduced in the Pentium 4 processor 
with the SSE2 extensions."

Maybe I should download a newer doc from Intel. The one I have states
that DAZ is associated with sse2, and does not speak at _all_ of the 
MXCSR_MASK field (I have seen it in my x86_64 doc though).


> 
> > /*
> >  * The _current_ task is using the FPU for the first time
> >  * so initialize it and set the mxcsr to its default
> >@@ -208,7 +214,7 @@
> > void set_fpu_mxcsr( struct task_struct *tsk, unsigned short mxcsr )
> > {
> > 	if ( cpu_has_xmm ) {
> >-		tsk->thread.i387.fxsave.mxcsr = (mxcsr & 0xffbf);
> >+		tsk->thread.i387.fxsave.mxcsr = (mxcsr & MXCSR_MASK);
> 
> 
> intel and x64 doc advocate to use
> 
> mxcsr &= tsk->thread.i387.fxsave.mxscr_mask
> 	? 0xffbf : tsk->thread.i387.fxsave.mxscr_mask;
> tsk->thread.i387.fxsave.mxscr =  mxcsr;
> 
> with mxscr_mask the 16 upper bits of field currently named
> mxscr in fxsave area. This bits was zeroed by previous processor
> so this must be backward compatible.

So the question is, are there processors in the wild which have DAZ but
still clear the MXCSR_MASK field?

It's simply a matter of rewriting the MXCSR_MASK macro, but to avoid
a conditional, I'd rather have a global mxcsr_mask variable somewhere
with the cpu feature flags. 
 
	Gabriel


 
	Gabriel.

Re: [PATCH] Mask mxcsr according to cpu features.

[Philippe Elie]

paubert wrote:
> On Fri, May 09, 2003 at 06:32:00AM +0000, Philippe Elie wrote:
> 


>>>+/* mxcsr bits 31-16 must be zero for security reasons,
>>>+ * bit 6 depends on cpu features.
>>>+ */
>>>+#define MXCSR_MASK (cpu_has_sse2 ? 0xffff : 0xffbf)
>>>+
>>>+
>>
>>I don't think daz bit depend on sse2, it's a separate features
> 
> 
> The doc I have state in several places:
> "The denormals-are-zeros mode was introduced in the Pentium 4 processor 
> with the SSE2 extensions."
> 
> Maybe I should download a newer doc from Intel. The one I have states
> that DAZ is associated with sse2, and does not speak at _all_ of the 
> MXCSR_MASK field (I have seen it in my x86_64 doc though).

the doc is not very clear, I see this in 11.6.3

"The denormal-are-zeo flag in MXCSR was introduced in later Pentium
4 processor and in the Intel Xeon processor"


> So the question is, are there processors in the wild which have DAZ but
> still clear the MXCSR_MASK field?


Not for intel at least since they advocate to use the mask as
a "is this feature present" and use mask 0xFFBF if mask == 0
and since this bits was required to be zero I think it's safe.
The only problem we can get is an old processor which write non
zero but random bits in the 16 upper bits.


> It's simply a matter of rewriting the MXCSR_MASK macro, but to avoid
> a conditional, I'd rather have a global mxcsr_mask variable somewhere
> with the cpu feature flags. 


my documentation says to fxsave and get the features mask from
the mxcsr mask but to fall back to 0xffbf if mask == 0, quoting
docs 11.6.6:

1 setup a fxsave area
2 clear this area
3 fxsave in this area
4 if mxcsr == 0 use mask 0xffbf else use mxcsr mask

"All bits set to 0 in this mask indicate reserved bits in
the mxcsr register. Thus, if this mask value is AND'd with
a value to be written in the MXCSR register, the resulting
value will be assured of having all its reserved bits set
to 0, preventing the possibility of a general protection
exception being generated when the value is written to the
MXCSR register"

then the documentation show an example using this mechanism
for daz flag and add the following note:

"Note than all bits in the MXCSR_MASK value are set to 1 indicate
features supported byt the MXCSR register; so the bits in the
MXCSR_MASK can also treated as a features flags for identifying
processor capabilities"

regards,
Phil

Re: [PATCH] Mask mxcsr according to cpu features.

[paubert]

On Fri, May 09, 2003 at 04:33:37PM +0000, Philippe Elie wrote:
> paubert wrote:
> >On Fri, May 09, 2003 at 06:32:00AM +0000, Philippe Elie wrote:
> >
> 
> 
> >>>+/* mxcsr bits 31-16 must be zero for security reasons,
> >>>+ * bit 6 depends on cpu features.
> >>>+ */
> >>>+#define MXCSR_MASK (cpu_has_sse2 ? 0xffff : 0xffbf)
> >>>+
> >>>+
> >>
> >>I don't think daz bit depend on sse2, it's a separate features
> >
> >
> >The doc I have state in several places:
> >"The denormals-are-zeros mode was introduced in the Pentium 4 processor 
> >with the SSE2 extensions."
> >
> >Maybe I should download a newer doc from Intel. The one I have states
> >that DAZ is associated with sse2, and does not speak at _all_ of the 
> >MXCSR_MASK field (I have seen it in my x86_64 doc though).
> 
> the doc is not very clear, I see this in 11.6.3
> 
> "The denormal-are-zeo flag in MXCSR was introduced in later Pentium
> 4 processor and in the Intel Xeon processor"

Indeed, I have downloaded fresher docs and I have found the following:
"In earlier IA-32 processors and in some models of the Pentium 4
processor, this flag (bit 6) is reserved." (10.2.2)

Of course other sentences would imply that DAZ is associated
with SSE2 capability, but all Pentium 4 have SSE2, I believe.

Well, what a mess for a single bit! 

> Not for intel at least since they advocate to use the mask as
> a "is this feature present" and use mask 0xFFBF if mask == 0
> and since this bits was required to be zero I think it's safe.

AMD says more or less the same.

> The only problem we can get is an old processor which write non
> zero but random bits in the 16 upper bits.

I don't believe that there is any, but that maybe some which don't
write anything, hence the requirement for clearing the area in the
DAZ detection algorithm.

> 
> 
> >It's simply a matter of rewriting the MXCSR_MASK macro, but to avoid
> >a conditional, I'd rather have a global mxcsr_mask variable somewhere
> >with the cpu feature flags. 
> 
> 
> my documentation says to fxsave and get the features mask from
> the mxcsr mask but to fall back to 0xffbf if mask == 0, quoting
> docs 11.6.6:
> 
> 1 setup a fxsave area
> 2 clear this area
> 3 fxsave in this area
> 4 if mxcsr == 0 use mask 0xffbf else use mxcsr mask

Too expensive unless the mask is computed at boot time once and for 
all (thrashing half a kB  for a single 32 bit constant, sigh). I did 
not want to touch too many files in my patch, but it seems unavoidable. 

Now a last question, are there SMP systems in which one processor
supports DAZ and the other does not, just to complicate matters a
little more?

	Regards,
	Gabriel

Re: [PATCH] Mask mxcsr according to cpu features.

[Philippe Elie]

paubert wrote:
> On Fri, May 09, 2003 at 04:33:37PM +0000, Philippe Elie wrote:

>>The only problem we can get is an old processor which write non
>>zero but random bits in the 16 upper bits.
> 
> 
> I don't believe that there is any, but that maybe some which don't
> write anything, hence the requirement for clearing the area in the
> DAZ detection algorithm.

right


>>my documentation says to fxsave and get the features mask from
>>the mxcsr mask but to fall back to 0xffbf if mask == 0, quoting
>>docs 11.6.6:
>>
>>1 setup a fxsave area
>>2 clear this area
>>3 fxsave in this area
>>4 if mxcsr == 0 use mask 0xffbf else use mxcsr mask
> 
> 
> Too expensive unless the mask is computed at boot time once and for 

yeps,

> all (thrashing half a kB  for a single 32 bit constant, sigh). I did 

uh? you just need to fxsave on stack, extract the mask, the struct
is 512 bytes length, surely during kernel init 512 bytes stack
allocation is right

> not want to touch too many files in my patch, but it seems unavoidable. 


> Now a last question, are there SMP systems in which one processor
> supports DAZ and the other does not, just to complicate matters a
> little more?

Such system are not symetric. I don't think we must take care
about this theorical things and I'm pretty sure than mixing old
P4 and newers in a box can't work. Anyway even if it works
userspace program using DAZ will be not reliable since they can
run from time to time on cpu with DAZ then cpu w/o DAZ.

regards,
Phil

Re: [PATCH] [CFT] [RFC] Correct mxcsr handling (was: Mask mxcsr according to cpu features.)

[Gabriel Paubert]

On Fri, May 09, 2003 at 09:00:46PM +0000, Philippe Elie wrote:
> >I don't believe that there is any, but that maybe some which don't
> >write anything, hence the requirement for clearing the area in the
> >DAZ detection algorithm.
> 
> right

That's indeed the case on a Celeron/Coppermine I just tested
with a trivial application.
 
> uh? you just need to fxsave on stack, extract the mask, the struct
> is 512 bytes length, surely during kernel init 512 bytes stack
> allocation is right

Beware of the alignment requirement, in the following patch I just
allocated 512 bytes of initdata for this. Not elegant but safe.

> 
> >not want to touch too many files in my patch, but it seems unavoidable. 
> 
> 
> >Now a last question, are there SMP systems in which one processor
> >supports DAZ and the other does not, just to complicate matters a
> >little more?
> 
> Such system are not symetric. I don't think we must take care
> about this theorical things and I'm pretty sure than mixing old
> P4 and newers in a box can't work. Anyway even if it works
> userspace program using DAZ will be not reliable since they can
> run from time to time on cpu with DAZ then cpu w/o DAZ.

Hmmm, the (misnamed) boot_cpu_data flags are the intersection of 
all the cpu flags. So I did the same for mxcsr_mask.

Anyway, here is the second version of the patch. Unfortunately I could
not test it, only compile, and I'm going to be away from any machine 
I can play with until mid-June or so. It took me some time to
understand early boot, especially how cr4 OSFXSR, first set up through
check_bugs(!), propagated to other processors through the global
(and misnamed) mmu_cr4_features, and I might have missed other things.

I repeat, handle with care, I'm not sure at all that it won't eat
your data and there are things that I don't like about it: mostly
that boot_cpu_data should be reordered so that mxcsr_mask is close
to the capabilities. There are #defines of the offsets in this 
structure in head.S, changing this is far too dangerous for a patch 
that I can't test. 

     Regards,
     Gabriel

===== arch/i386/kernel/i387.c 1.16 vs edited =====
--- 1.16/arch/i386/kernel/i387.c	Wed Apr  9 05:45:37 2003
+++ edited/arch/i386/kernel/i387.c	Sun May 11 20:55:32 2003
@@ -208,7 +208,8 @@
 void set_fpu_mxcsr( struct task_struct *tsk, unsigned short mxcsr )
 {
 	if ( cpu_has_xmm ) {
-		tsk->thread.i387.fxsave.mxcsr = (mxcsr & 0xffbf);
+		tsk->thread.i387.fxsave.mxcsr = 
+		    mxcsr & boot_cpu_data.mxcsr_mask;
 	}
 }
 
@@ -356,8 +357,7 @@
 	clear_fpu( tsk );
 	err = __copy_from_user( &tsk->thread.i387.fxsave, &buf->_fxsr_env[0],
 				sizeof(struct i387_fxsave_struct) );
-	/* mxcsr bit 6 and 31-16 must be zero for security reasons */
-	tsk->thread.i387.fxsave.mxcsr &= 0xffbf;
+	tsk->thread.i387.fxsave.mxcsr &= boot_cpu_data.mxcsr_mask;
 	return err ? 1 : convert_fxsr_from_user( &tsk->thread.i387.fxsave, buf );
 }
 
@@ -455,8 +455,7 @@
 	if ( cpu_has_fxsr ) {
 		__copy_from_user( &tsk->thread.i387.fxsave, buf,
 				  sizeof(struct user_fxsr_struct) );
-		/* mxcsr bit 6 and 31-16 must be zero for security reasons */
-		tsk->thread.i387.fxsave.mxcsr &= 0xffbf;
+		tsk->thread.i387.fxsave.mxcsr &= boot_cpu_data.mxcsr_mask;
 		return 0;
 	} else {
 		return -EIO;
===== arch/i386/kernel/cpu/common.c 1.21 vs edited =====
--- 1.21/arch/i386/kernel/cpu/common.c	Sun Mar 23 15:55:48 2003
+++ edited/arch/i386/kernel/cpu/common.c	Sun May 11 22:43:44 2003
@@ -262,6 +262,7 @@
 __setup("serialnumber", x86_serial_nr_setup);
 
 
+static struct i387_fxsave_struct fxsave_area __initdata;
 
 /*
  * This does the hard work of actually picking apart the CPU stuff...
@@ -324,6 +325,24 @@
 		clear_bit(X86_FEATURE_XMM, c->x86_capability);
 	}
 
+	if (test_bit(X86_FEATURE_XMM, c->x86_capability)) {
+		/* Setting OSFXSR may not be necessary */
+		unsigned cr0, cr4;
+		long mask;
+
+		memset(&fxsave_area, 0, sizeof fxsave_area);
+		cr0 = read_cr0();
+		cr4 = read_cr4();
+		write_cr4(cr4|X86_CR4_OSFXSR);
+		clts();
+		__asm__ __volatile__("fxsave %0": "=m" (fxsave_area));
+		write_cr4(cr4);
+		write_cr0(cr0);
+		mask = fxsave_area.mxcsr_mask;
+		mask = mask ? mask : 0xffbf;
+		c->mxcsr_mask = mask;
+	}
+
 	if (disable_pse)
 		clear_bit(X86_FEATURE_PSE, c->x86_capability);
 
@@ -357,6 +376,7 @@
 		/* AND the already accumulated flags with these */
 		for ( i = 0 ; i < NCAPINTS ; i++ )
 			boot_cpu_data.x86_capability[i] &= c->x86_capability[i];
+		boot_cpu_data.mxcsr_mask &= c->mxcsr_mask;
 	}
 
 	/* Init Machine Check Exception if available. */
===== include/asm-i386/processor.h 1.50 vs edited =====
--- 1.50/include/asm-i386/processor.h	Fri May  9 21:24:03 2003
+++ edited/include/asm-i386/processor.h	Sun May 11 20:53:22 2003
@@ -63,6 +63,7 @@
 	int	f00f_bug;
 	int	coma_bug;
 	unsigned long loops_per_jiffy;
+	long    mxcsr_mask;
 } __attribute__((__aligned__(SMP_CACHE_BYTES)));
 
 #define X86_VENDOR_INTEL 0
@@ -320,7 +321,7 @@
 	long	foo;
 	long	fos;
 	long	mxcsr;
-	long	reserved;
+	long	mxcsr_mask;
 	long	st_space[32];	/* 8*16 bytes for each FP-reg = 128 bytes */
 	long	xmm_space[32];	/* 8*16 bytes for each XMM-reg = 128 bytes */
 	long	padding[56];