[patch] use __asm__ and __volatile__ in asm-x86_64/msr.h

[patch] use __asm__ and __volatile__ in asm-x86_64/msr.h

[Mike Frysinger]

Most of the macros in msr.h already use __asm__ and __volatile__, this patch
brings the rest into line.  This is needed since these get exported into
userspace (need to use strict __asm__ vs asm when gcc compiles with GNU
extensions turned off).

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
diff --git a/include/asm-x86_64/msr.h b/include/asm-x86_64/msr.h
index d5c55b8..ed15d2b 100644
--- a/include/asm-x86_64/msr.h
+++ b/include/asm-x86_64/msr.h
@@ -33,7 +33,7 @@
 
 /* wrmsr with exception handling */
 #define wrmsr_safe(msr,a,b) ({ int ret__;			\
-	asm volatile("2: wrmsr ; xorl %0,%0\n"			\
+	__asm__ __volatile__("2: wrmsr ; xorl %0,%0\n"			\
 		     "1:\n\t"					\
 		     ".section .fixup,\"ax\"\n\t"		\
 		     "3:  movl %4,%0 ; jmp 1b\n\t"		\
@@ -50,7 +50,7 @@
 
 #define rdmsr_safe(msr,a,b) \
 	({ int ret__;						\
-	  asm volatile ("1:       rdmsr\n"			\
+	  __asm__ __volatile__ ("1:       rdmsr\n"			\
                       "2:\n"					\
                       ".section .fixup,\"ax\"\n"		\
                       "3:       movl %4,%0\n"			\
@@ -70,17 +70,17 @@
      __asm__ __volatile__ ("rdtsc" : "=a" (low) : : "edx")
 
 #define rdtscp(low,high,aux) \
-     asm volatile (".byte 0x0f,0x01,0xf9" : "=a" (low), "=d" (high), "=c" (aux))
+     __asm__ __volatile__ (".byte 0x0f,0x01,0xf9" : "=a" (low), "=d" (high), "=c" (aux))
 
 #define rdtscll(val) do { \
      unsigned int __a,__d; \
-     asm volatile("rdtsc" : "=a" (__a), "=d" (__d)); \
+     __asm__ __volatile__("rdtsc" : "=a" (__a), "=d" (__d)); \
      (val) = ((unsigned long)__a) | (((unsigned long)__d)<<32); \
 } while(0)
 
 #define rdtscpll(val, aux) do { \
      unsigned long __a, __d; \
-     asm volatile (".byte 0x0f,0x01,0xf9" : "=a" (__a), "=d" (__d), "=c" (aux)); \
+     __asm__ __volatile__ (".byte 0x0f,0x01,0xf9" : "=a" (__a), "=d" (__d), "=c" (aux)); \
      (val) = (__d << 32) | __a; \
 } while (0)
 

Re: [patch] use __asm__ and __volatile__ in asm-x86_64/msr.h

[H. Peter Anvin]

Mike Frysinger wrote:
> Most of the macros in msr.h already use __asm__ and __volatile__, this patch
> brings the rest into line.  This is needed since these get exported into
> userspace (need to use strict __asm__ vs asm when gcc compiles with GNU
> extensions turned off).
> ---
> diff --git a/include/asm-x86_64/msr.h b/include/asm-x86_64/msr.h
> index d5c55b8..ed15d2b 100644
> --- a/include/asm-x86_64/msr.h
> +++ b/include/asm-x86_64/msr.h
> @@ -33,7 +33,7 @@
>  
>  /* wrmsr with exception handling */
>  #define wrmsr_safe(msr,a,b) ({ int ret__;			\
> -	asm volatile("2: wrmsr ; xorl %0,%0\n"			\
> +	__asm__ __volatile__("2: wrmsr ; xorl %0,%0\n"			\
>  		     "1:\n\t"					\
>  		     ".section .fixup,\"ax\"\n\t"		\
>  		     "3:  movl %4,%0 ; jmp 1b\n\t"		\
> @@ -50,7 +50,7 @@
>  
>  #define rdmsr_safe(msr,a,b) \
>  	({ int ret__;						\
> -	  asm volatile ("1:       rdmsr\n"			\
> +	  __asm__ __volatile__ ("1:       rdmsr\n"			\
>                        "2:\n"					\
>                        ".section .fixup,\"ax\"\n"		\
>                        "3:       movl %4,%0\n"			\

NAK.

This is wrong for two reasons:

- MSRs are not accessible in user space.
- .section .fixup is not applicable to user space.

Anything MSR-related should be protected by __KERNEL__ instead.

This NAK doesn't apply to rdtsc* and rdpmc* changes, however, as those
instructions are legitimate in userspace.

	-hpa

Re: [patch] use __asm__ and __volatile__ in asm-x86_64/msr.h

[Mike Frysinger]

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

On Monday 18 June 2007, H. Peter Anvin wrote:
> Mike Frysinger wrote:
> > Most of the macros in msr.h already use __asm__ and __volatile__, this
> > patch brings the rest into line.  This is needed since these get exported
> > into userspace (need to use strict __asm__ vs asm when gcc compiles with
> > GNU extensions turned off).
> > ---
> > diff --git a/include/asm-x86_64/msr.h b/include/asm-x86_64/msr.h
> > index d5c55b8..ed15d2b 100644
> > --- a/include/asm-x86_64/msr.h
> > +++ b/include/asm-x86_64/msr.h
> > @@ -33,7 +33,7 @@
> >
> >  /* wrmsr with exception handling */
> >  #define wrmsr_safe(msr,a,b) ({ int ret__;			\
> > -	asm volatile("2: wrmsr ; xorl %0,%0\n"			\
> > +	__asm__ __volatile__("2: wrmsr ; xorl %0,%0\n"			\
> >  		     "1:\n\t"					\
> >  		     ".section .fixup,\"ax\"\n\t"		\
> >  		     "3:  movl %4,%0 ; jmp 1b\n\t"		\
> > @@ -50,7 +50,7 @@
> >
> >  #define rdmsr_safe(msr,a,b) \
> >  	({ int ret__;						\
> > -	  asm volatile ("1:       rdmsr\n"			\
> > +	  __asm__ __volatile__ ("1:       rdmsr\n"			\
> >                        "2:\n"					\
> >                        ".section .fixup,\"ax\"\n"		\
> >                        "3:       movl %4,%0\n"			\
>
> NAK.
>
> This is wrong for two reasons:
>
> - MSRs are not accessible in user space.
> - .section .fixup is not applicable to user space.
>
> Anything MSR-related should be protected by __KERNEL__ instead.
>
> This NAK doesn't apply to rdtsc* and rdpmc* changes, however, as those
> instructions are legitimate in userspace.

then someone needs to audit msr.h for this because currently it lacks 
__KERNEL__ of any kind yet is exported completely to userspace ... i dont 
know anything about this instructions so that someone != me

this is similar to what i heard last time i tried to fixup msr.h for 
userspace, but the file still hasnt been updated ...
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: [patch] use __asm__ and __volatile__ in asm-x86_64/msr.h

[Christoph Hellwig]

On Mon, Jun 18, 2007 at 10:02:21AM -0400, Mike Frysinger wrote:
> > This is wrong for two reasons:
> >
> > - MSRs are not accessible in user space.
> > - .section .fixup is not applicable to user space.
> >
> > Anything MSR-related should be protected by __KERNEL__ instead.
> >
> > This NAK doesn't apply to rdtsc* and rdpmc* changes, however, as those
> > instructions are legitimate in userspace.
> 
> then someone needs to audit msr.h for this because currently it lacks 
> __KERNEL__ of any kind yet is exported completely to userspace ... i dont 
> know anything about this instructions so that someone != me
> 
> this is similar to what i heard last time i tried to fixup msr.h for 
> userspace, but the file still hasnt been updated ...

asm-i386/msr.h should not be exported to userspace at all, it contains
nothing but kernel-internal helpers.

Re: [patch] use __asm__ and __volatile__ in asm-x86_64/msr.h

[Andi Kleen]

> asm-i386/msr.h should not be exported to userspace at all, it contains
> nothing but kernel-internal helpers.

Actually rdtsc and rdtscll and potentially rdpmc which is in there can be very 
useful in user space if you know what you're doing. Unfortunately a lot of its users don't,
but not having the include probably won't stop them either.

-Andi

Re: [patch] use __asm__ and __volatile__ in asm-x86_64/msr.h

[H. Peter Anvin]

Andi Kleen wrote:
>> asm-i386/msr.h should not be exported to userspace at all, it contains
>> nothing but kernel-internal helpers.
> 
> Actually rdtsc and rdtscll and potentially rdpmc which is in there can be very 
> useful in user space if you know what you're doing. Unfortunately a lot of its users don't,
> but not having the include probably won't stop them either.

More likely, people will just re-implement them incorrectly.

However, the rdtsc() definition in the kernel is weird (and removable, I
think there are no more users -- I have it removed in my MSR driver
rewrite tree which I need to get off my arse and push.)  Most users
would expect the rdtscll() functionality with the rdtsc() name.

	-hpa

Re: [patch] use __asm__ and __volatile__ in asm-x86_64/msr.h

[Mike Frysinger]

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

On Wednesday 20 June 2007, H. Peter Anvin wrote:
> Andi Kleen wrote:
> >> asm-i386/msr.h should not be exported to userspace at all, it contains
> >> nothing but kernel-internal helpers.
> >
> > Actually rdtsc and rdtscll and potentially rdpmc which is in there can be
> > very useful in user space if you know what you're doing. Unfortunately a
> > lot of its users don't, but not having the include probably won't stop
> > them either.
>
> More likely, people will just re-implement them incorrectly.
>
> However, the rdtsc() definition in the kernel is weird (and removable, I
> think there are no more users -- I have it removed in my MSR driver
> rewrite tree which I need to get off my arse and push.)  Most users
> would expect the rdtscll() functionality with the rdtsc() name.

does that mean you'll also take care of cleaning up msr.h ?  or do i need to 
post another patch ?
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: [patch] use __asm__ and __volatile__ in asm-x86_64/msr.h

[H. Peter Anvin]

Mike Frysinger wrote:
> On Wednesday 20 June 2007, H. Peter Anvin wrote:
>> Andi Kleen wrote:
>>>> asm-i386/msr.h should not be exported to userspace at all, it contains
>>>> nothing but kernel-internal helpers.
>>> Actually rdtsc and rdtscll and potentially rdpmc which is in there can be
>>> very useful in user space if you know what you're doing. Unfortunately a
>>> lot of its users don't, but not having the include probably won't stop
>>> them either.
>> More likely, people will just re-implement them incorrectly.
>>
>> However, the rdtsc() definition in the kernel is weird (and removable, I
>> think there are no more users -- I have it removed in my MSR driver
>> rewrite tree which I need to get off my arse and push.)  Most users
>> would expect the rdtscll() functionality with the rdtsc() name.
> 
> does that mean you'll also take care of cleaning up msr.h ?  or do i need to 
> post another patch ?

I'll put that on my list.

	-hpa