[PATCH] Use correct macros in raid code, not raw asm

[PATCH] Use correct macros in raid code, not raw asm

[Rusty Russell]

This make sure it's paravirtualized correctly when CONFIG_PARAVIRT=y.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff -r 4ff048622391 drivers/md/raid6x86.h
--- a/drivers/md/raid6x86.h	Thu Dec 28 16:52:54 2006 +1100
+++ b/drivers/md/raid6x86.h	Fri Dec 29 10:09:38 2006 +1100
@@ -75,13 +75,14 @@ static inline unsigned long raid6_get_fp
 	unsigned long cr0;
 
 	preempt_disable();
-	asm volatile("mov %%cr0,%0 ; clts" : "=r" (cr0));
+	cr0 = read_cr0();
+	clts();
 	return cr0;
 }
 
 static inline void raid6_put_fpu(unsigned long cr0)
 {
-	asm volatile("mov %0,%%cr0" : : "r" (cr0));
+	write_cr0(cr0);
 	preempt_enable();
 }
 

Re: [PATCH] Use correct macros in raid code, not raw asm

[Linus Torvalds]

On Fri, 29 Dec 2006, Rusty Russell wrote:
>
> This make sure it's paravirtualized correctly when CONFIG_PARAVIRT=y.

Why doesn't this code use "kernel_fpu_begin()" and "kernel_fpu_end()"?

The raid6 code is crap, and slower. It does "fsave/frstor" or movaps or 
other crud, and the thing is, it shouldn't. It should just do 
kernel_fpu_begin/end(), which does it all right, and avoids saving any 
state at all unless it's being used by the user RIGHT NOW.

		Linus

Re: [PATCH] Use correct macros in raid code, not raw asm

[Andrew Morton]

On Fri, 29 Dec 2006 10:34:21 +1100
Rusty Russell <rusty@rustcorp.com.au> wrote:

> This make sure it's paravirtualized correctly when CONFIG_PARAVIRT=y.
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> 
> diff -r 4ff048622391 drivers/md/raid6x86.h
> --- a/drivers/md/raid6x86.h	Thu Dec 28 16:52:54 2006 +1100
> +++ b/drivers/md/raid6x86.h	Fri Dec 29 10:09:38 2006 +1100
> @@ -75,13 +75,14 @@ static inline unsigned long raid6_get_fp
>  	unsigned long cr0;
>  
>  	preempt_disable();
> -	asm volatile("mov %%cr0,%0 ; clts" : "=r" (cr0));
> +	cr0 = read_cr0();
> +	clts();
>  	return cr0;
>  }
>  
>  static inline void raid6_put_fpu(unsigned long cr0)
>  {
> -	asm volatile("mov %0,%%cr0" : : "r" (cr0));
> +	write_cr0(cr0);
>  	preempt_enable();
>  }
>  

Perhaps we also need:

--- a/drivers/md/raid6x86.h~use-correct-macros-in-raid-code-not-raw-asm-include
+++ a/drivers/md/raid6x86.h
@@ -21,6 +21,8 @@
 
 #if defined(__i386__) || defined(__x86_64__)
 
+#include <asm/system.h>
+
 #ifdef __x86_64__
 
 typedef struct {
_

?

Re: [PATCH] Use correct macros in raid code, not raw asm

[Rusty Russell]

On Thu, 2006-12-28 at 15:56 -0800, Andrew Morton wrote:
> On Fri, 29 Dec 2006 10:34:21 +1100
> Rusty Russell <rusty@rustcorp.com.au> wrote:
> 
> > This make sure it's paravirtualized correctly when CONFIG_PARAVIRT=y.
> > 
> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > 
> > diff -r 4ff048622391 drivers/md/raid6x86.h
> > --- a/drivers/md/raid6x86.h	Thu Dec 28 16:52:54 2006 +1100
> > +++ b/drivers/md/raid6x86.h	Fri Dec 29 10:09:38 2006 +1100
> > @@ -75,13 +75,14 @@ static inline unsigned long raid6_get_fp
> >  	unsigned long cr0;
> >  
> >  	preempt_disable();
> > -	asm volatile("mov %%cr0,%0 ; clts" : "=r" (cr0));
> > +	cr0 = read_cr0();
> > +	clts();
> >  	return cr0;
> >  }
> >  
> >  static inline void raid6_put_fpu(unsigned long cr0)
> >  {
> > -	asm volatile("mov %0,%%cr0" : : "r" (cr0));
> > +	write_cr0(cr0);
> >  	preempt_enable();
> >  }
> >  
> 
> Perhaps we also need:
> 
> --- a/drivers/md/raid6x86.h~use-correct-macros-in-raid-code-not-raw-asm-include
> +++ a/drivers/md/raid6x86.h
> @@ -21,6 +21,8 @@
>  
>  #if defined(__i386__) || defined(__x86_64__)
>  
> +#include <asm/system.h>
> +

The code looks like it's designed to be included from userspace for
testing; as it compiles without this include (and has no other
includes), I chose not to add it.

Linus makes a good point, but someone who actually knows the code
should, y'know, test it and stuff...

Rusty.

Re: [PATCH] Use correct macros in raid code, not raw asm

[Andi Kleen]

> 
> The code looks like it's designed to be included from userspace for
> testing; as it compiles without this include (and has no other
> includes), I chose not to add it.
> 
> Linus makes a good point, but someone who actually knows the code
> should, y'know, test it and stuff...

It should use kernel_fpu_begin() imho. If someone wants to test
it in user space again they can add dummy definitions of that
to their user space  header.

-Andi

Re: [PATCH] Use correct macros in raid code, not raw asm

[H. Peter Anvin]

Andi Kleen wrote:
>> The code looks like it's designed to be included from userspace for
>> testing; as it compiles without this include (and has no other
>> includes), I chose not to add it.
>>
>> Linus makes a good point, but someone who actually knows the code
>> should, y'know, test it and stuff...
> 
> It should use kernel_fpu_begin() imho. If someone wants to test
> it in user space again they can add dummy definitions of that
> to their user space  header.

I hadn't seen this thread until now, when Neil pointed me to the thread.

Using kernel_fpu_begin() ... kernel_fpu_end() is probably indeed the 
best option.

	-hpa

Re: [PATCH] Use correct macros in raid code, not raw asm

[Neil Brown]

On Thursday February 8, hpa@zytor.com wrote:
> Andi Kleen wrote:
> > 
> > It should use kernel_fpu_begin() imho. If someone wants to test
> > it in user space again they can add dummy definitions of that
> > to their user space  header.
> 
> I hadn't seen this thread until now, when Neil pointed me to the thread.
> 
> Using kernel_fpu_begin() ... kernel_fpu_end() is probably indeed the 
> best option.
> 

So does this look right (no, I haven't compiled it yet)

NeilBrown


### Diffstat output
 ./drivers/md/raid6x86.h |   56 ++++++++++--------------------------------------
 1 file changed, 12 insertions(+), 44 deletions(-)

diff .prev/drivers/md/raid6x86.h ./drivers/md/raid6x86.h
--- .prev/drivers/md/raid6x86.h	2007-02-09 12:30:32.000000000 +1100
+++ ./drivers/md/raid6x86.h	2007-02-09 12:36:01.000000000 +1100
@@ -25,20 +25,17 @@
 
 typedef struct {
 	unsigned int fsave[27];
-	unsigned long cr0;
 } raid6_mmx_save_t __attribute__((aligned(16)));
 
 /* N.B.: For SSE we only save %xmm0-%xmm7 even for x86-64, since
    the code doesn't know about the additional x86-64 registers */
 typedef struct {
 	unsigned int sarea[8*4+2];
-	unsigned long cr0;
 } raid6_sse_save_t __attribute__((aligned(16)));
 
 /* This is for x86-64-specific code which uses all 16 XMM registers */
 typedef struct {
 	unsigned int sarea[16*4+2];
-	unsigned long cr0;
 } raid6_sse16_save_t __attribute__((aligned(16)));
 
 /* On x86-64 the stack *SHOULD* be 16-byte aligned, but currently this
@@ -50,7 +47,6 @@ typedef struct {
 
 typedef struct {
 	unsigned int fsave[27];
-	unsigned long cr0;
 } raid6_mmx_save_t;
 
 /* On i386, the stack is only 8-byte aligned, but SSE requires 16-byte
@@ -58,7 +54,6 @@ typedef struct {
    a properly-sized area correctly.  */
 typedef struct {
 	unsigned int sarea[8*4+3];
-	unsigned long cr0;
 } raid6_sse_save_t;
 
 /* Find the 16-byte aligned save area */
@@ -66,56 +61,29 @@ typedef struct {
 
 #endif
 
-#ifdef __KERNEL__ /* Real code */
-
-/* Note: %cr0 is 32 bits on i386 and 64 bits on x86-64 */
-
-static inline unsigned long raid6_get_fpu(void)
-{
-	unsigned long cr0;
-
-	preempt_disable();
-	asm volatile("mov %%cr0,%0 ; clts" : "=r" (cr0));
-	return cr0;
-}
-
-static inline void raid6_put_fpu(unsigned long cr0)
-{
-	asm volatile("mov %0,%%cr0" : : "r" (cr0));
-	preempt_enable();
-}
-
-#else /* Dummy code for user space testing */
-
-static inline unsigned long raid6_get_fpu(void)
-{
-	return 0xf00ba6;
-}
-
-static inline void raid6_put_fpu(unsigned long cr0)
-{
-	(void)cr0;
-}
-
+#ifndef __KERNEL__
+/* for user-space testing */
+#define kernel_fpu_begin()
+#define kernel_fpu_end();
 #endif
 
 static inline void raid6_before_mmx(raid6_mmx_save_t *s)
 {
-	s->cr0 = raid6_get_fpu();
+	kernel_fpu_begin();
 	asm volatile("fsave %0 ; fwait" : "=m" (s->fsave[0]));
 }
 
 static inline void raid6_after_mmx(raid6_mmx_save_t *s)
 {
 	asm volatile("frstor %0" : : "m" (s->fsave[0]));
-	raid6_put_fpu(s->cr0);
+	kernel_fpu_end();
 }
 
 static inline void raid6_before_sse(raid6_sse_save_t *s)
 {
 	unsigned int *rsa = SAREA(s);
 
-	s->cr0 = raid6_get_fpu();
+	kernel_fpu_begin();
 
 	asm volatile("movaps %%xmm0,%0" : "=m" (rsa[0]));
 	asm volatile("movaps %%xmm1,%0" : "=m" (rsa[4]));
@@ -140,14 +108,14 @@ static inline void raid6_after_sse(raid6
 	asm volatile("movaps %0,%%xmm6" : : "m" (rsa[24]));
 	asm volatile("movaps %0,%%xmm7" : : "m" (rsa[28]));
 
-	raid6_put_fpu(s->cr0);
+	kernel_fpu_end();
 }
 
 static inline void raid6_before_sse2(raid6_sse_save_t *s)
 {
 	unsigned int *rsa = SAREA(s);
 
-	s->cr0 = raid6_get_fpu();
+	kernel_fpu_begin();
 
 	asm volatile("movdqa %%xmm0,%0" : "=m" (rsa[0]));
 	asm volatile("movdqa %%xmm1,%0" : "=m" (rsa[4]));
@@ -172,7 +140,7 @@ static inline void raid6_after_sse2(raid
 	asm volatile("movdqa %0,%%xmm6" : : "m" (rsa[24]));
 	asm volatile("movdqa %0,%%xmm7" : : "m" (rsa[28]));
 
-	raid6_put_fpu(s->cr0);
+	kernel_fpu_end();
 }
 
 #ifdef __x86_64__
@@ -181,7 +149,7 @@ static inline void raid6_before_sse16(ra
 {
 	unsigned int *rsa = SAREA(s);
 
-	s->cr0 = raid6_get_fpu();
+	kernel_fpu_begin();
 
 	asm volatile("movdqa %%xmm0,%0" : "=m" (rsa[0]));
 	asm volatile("movdqa %%xmm1,%0" : "=m" (rsa[4]));
@@ -222,7 +190,7 @@ static inline void raid6_after_sse16(rai
 	asm volatile("movdqa %0,%%xmm14" : : "m" (rsa[56]));
 	asm volatile("movdqa %0,%%xmm15" : : "m" (rsa[60]));
 
-	raid6_put_fpu(s->cr0);
+	kernel_fpu_end();
 }
 
 #endif /* __x86_64__ */

Re: [PATCH] Use correct macros in raid code, not raw asm

[H. Peter Anvin]

Neil Brown wrote:
> 
> So does this look right (no, I haven't compiled it yet)
> 

No, the whole raid6_*_save_t should be removed.  I'll try to have a 
patch for you later.

	-hpa