x86: fix rdrand asm()

x86: fix rdrand asm()

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 778 bytes --]

Just learned the hard way that at least for non-volatile asm()s gcc
indeed does what the documentation says: It may move it across jumps
(i.e. ahead of the cpu_has() check). While the documentation claims
that this can also happen for volatile asm()s, if that was the case
we'd have many more problems in our code (and e,g, Linux would too).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/include/asm-x86/random.h
+++ b/xen/include/asm-x86/random.h
@@ -8,7 +8,7 @@ static inline unsigned int arch_get_rand
     unsigned int val = 0;
 
     if ( cpu_has(&current_cpu_data, X86_FEATURE_RDRAND) )
-        asm ( ".byte 0x0f,0xc7,0xf0" : "+a" (val) );
+        __asm__ __volatile__ ( ".byte 0x0f,0xc7,0xf0" : "+a" (val) );
 
     return val;
 }




[-- Attachment #2: x86-rdrand-asm-volatile.patch --]
[-- Type: text/plain, Size: 797 bytes --]

x86: fix rdrand asm()

Just learned the hard way that at least for non-volatile asm()s gcc
indeed does what the documentation says: It may move it across jumps
(i.e. ahead of the cpu_has() check). While the documentation claims
that this can also happen for volatile asm()s, if that was the case
we'd have many more problems in our code (and e,g, Linux would too).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/include/asm-x86/random.h
+++ b/xen/include/asm-x86/random.h
@@ -8,7 +8,7 @@ static inline unsigned int arch_get_rand
     unsigned int val = 0;
 
     if ( cpu_has(&current_cpu_data, X86_FEATURE_RDRAND) )
-        asm ( ".byte 0x0f,0xc7,0xf0" : "+a" (val) );
+        __asm__ __volatile__ ( ".byte 0x0f,0xc7,0xf0" : "+a" (val) );
 
     return val;
 }

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: x86: fix rdrand asm()

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 1179 bytes --]

On 25/09/13 16:46, Jan Beulich wrote:
> Just learned the hard way that at least for non-volatile asm()s gcc
> indeed does what the documentation says: It may move it across jumps
> (i.e. ahead of the cpu_has() check). While the documentation claims
> that this can also happen for volatile asm()s, if that was the case
> we'd have many more problems in our code (and e,g, Linux would too).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/include/asm-x86/random.h
> +++ b/xen/include/asm-x86/random.h
> @@ -8,7 +8,7 @@ static inline unsigned int arch_get_rand
>      unsigned int val = 0;
>  
>      if ( cpu_has(&current_cpu_data, X86_FEATURE_RDRAND) )
> -        asm ( ".byte 0x0f,0xc7,0xf0" : "+a" (val) );
> +        __asm__ __volatile__ ( ".byte 0x0f,0xc7,0xf0" : "+a" (val) );

Any reason for using the double underscore versions?  They have
identical meanings, and the prevailing style does appear to be without.

Either way,

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>  
>      return val;
>  }
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 2114 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: x86: fix rdrand asm()

[Jan Beulich]

>>> On 25.09.13 at 17:52, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 25/09/13 16:46, Jan Beulich wrote:
>> Just learned the hard way that at least for non-volatile asm()s gcc
>> indeed does what the documentation says: It may move it across jumps
>> (i.e. ahead of the cpu_has() check). While the documentation claims
>> that this can also happen for volatile asm()s, if that was the case
>> we'd have many more problems in our code (and e,g, Linux would too).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/include/asm-x86/random.h
>> +++ b/xen/include/asm-x86/random.h
>> @@ -8,7 +8,7 @@ static inline unsigned int arch_get_rand
>>      unsigned int val = 0;
>>  
>>      if ( cpu_has(&current_cpu_data, X86_FEATURE_RDRAND) )
>> -        asm ( ".byte 0x0f,0xc7,0xf0" : "+a" (val) );
>> +        __asm__ __volatile__ ( ".byte 0x0f,0xc7,0xf0" : "+a" (val) );
> 
> Any reason for using the double underscore versions?  They have
> identical meanings, and the prevailing style does appear to be without.

In header I prefer to use the name space safe variant with
the underscores, while in .c files I'd use the ones without. From
a strict POV that ought to be necessary for public headers only
(where we don't have any asm()s anyway), but I'd like to do it
this way as long as we're not really consistent with this
throughout the tree.

Jan

Re: x86: fix rdrand asm()

[Keir Fraser]

On 25/09/2013 16:46, "Jan Beulich" <JBeulich@suse.com> wrote:

> Just learned the hard way that at least for non-volatile asm()s gcc
> indeed does what the documentation says: It may move it across jumps
> (i.e. ahead of the cpu_has() check). While the documentation claims
> that this can also happen for volatile asm()s, if that was the case
> we'd have many more problems in our code (and e,g, Linux would too).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/include/asm-x86/random.h
> +++ b/xen/include/asm-x86/random.h
> @@ -8,7 +8,7 @@ static inline unsigned int arch_get_rand
>      unsigned int val = 0;
>  
>      if ( cpu_has(&current_cpu_data, X86_FEATURE_RDRAND) )
> -        asm ( ".byte 0x0f,0xc7,0xf0" : "+a" (val) );
> +        __asm__ __volatile__ ( ".byte 0x0f,0xc7,0xf0" : "+a" (val) );

Although not consistently applied, we use 'asm volatile' rather than
'__asm__ __volatile__' generally. So we should here. Apart from that
Acked-by: Keir Fraser <keir@xen.org>

>      return val;
>  }
> 
> 
>