[PATCH 1/1] X86: use explicit register name for get/put_user

[PATCH 1/1] X86: use explicit register name for get/put_user

[Jiri Slaby]

I couldn't find any traces of documentation for the behavior used
in current get/put_user implementation. After the merge of
32/64-bit version there is back register reference ("0") to the
output register ("a"). The output is retval of int type, but
the value in the input may be a long on 64-bit.

I don't know if this could ever cause any problems, but changing
the input to an explicit register ("a") makes it cleaner in my eyes.
There is no need of the back reference.

Change this for both get and put_user.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
---
 arch/x86/include/asm/uaccess.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index abd3e0e..35f483b 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -122,7 +122,7 @@ extern int __get_user_bad(void);
 #define __get_user_x(size, ret, x, ptr)		      \
 	asm volatile("call __get_user_" #size	      \
 		     : "=a" (ret), "=d" (x)	      \
-		     : "0" (ptr))		      \
+		     : "a" (ptr))		      \
 
 /* Careful: we have to cast the result to the type of the pointer
  * for sign reasons */
@@ -181,7 +181,7 @@ extern int __get_user_bad(void);
 
 #define __put_user_x(size, x, ptr, __ret_pu)			\
 	asm volatile("call __put_user_" #size : "=a" (__ret_pu)	\
-		     : "0" ((typeof(*(ptr)))(x)), "c" (ptr) : "ebx")
+		     : "a" ((typeof(*(ptr)))(x)), "c" (ptr) : "ebx")
 
 
 
-- 
1.6.5.3

Re: [PATCH 1/1] X86: use explicit register name for get/put_user

[H. Peter Anvin]

On 12/06/2009 01:30 AM, Jiri Slaby wrote:
> 
> I don't know if this could ever cause any problems, but changing
> the input to an explicit register ("a") makes it cleaner in my eyes.
> There is no need of the back reference.
> 

It can't: the backreference refers to only the information that is in
the register constraint, not to anything else.  I really would prefer
avoiding any changes to working code that aren't justified, simply
because every time we change an asm() we risk tickling a new obscure bug
in some old version of gcc.

As such,

Nacked-by: H. Peter Anvin <hpa@zytor.com>

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH 1/1] X86: use explicit register name for get/put_user

[Jiri Slaby]

On 12/06/2009 08:11 PM, H. Peter Anvin wrote:
> On 12/06/2009 01:30 AM, Jiri Slaby wrote:
>>
>> I don't know if this could ever cause any problems, but changing
>> the input to an explicit register ("a") makes it cleaner in my eyes.
>> There is no need of the back reference.
>>
> 
> It can't: the backreference refers to only the information that is in
> the register constraint, not to anything else.

Is this documented somewhere? Or do we rely on an undocumented feature?
I mean it doesn't refer only to the constraint but also to a concrete
register allocation. As far as I understand it (from the gcc 4.4
documentation), if one does
 "insn %0" : "=r" (out) : "0" (in)
the "0" constraint corresponds to the concrete register allocated for
out, not to any register (which is the constraint "r").

In the document they write only about the "same location" occupied by in
and out, nothing is said about size (and hence I think we cannot
mismatch size of operands). And I couldn't find any other
restrictions/documentation about inline assembly, hence the patch,
because nothing assured me this cannot change in the future.

Now I tried different compilers (clang, llvm-gcc) and they choke on that:
$ cat c.c
void x(void)
{
        unsigned long in;
        int out;
        asm("insn %0" : "=r" (out) : "0" (in));
}
$ clang c.c -S -o -
c.c:5:36: error: unsupported inline asm: input with type 'unsigned long'
      matching output with type 'int'
        asm("insn %0" : "=r" (out) : "0" (in));
                              ~~~         ^~
1 diagnostic generated.
$ llvm-gcc c.c -S -o -
c.c: In function 'x':
c.c:5: error: unsupported inline asm: input constraint with a matching
output constraint of incompatible type!

thanks for the review,
-- 
js

Re: [PATCH 1/1] X86: use explicit register name for get/put_user

[H. Peter Anvin]

On 12/07/2009 04:37 AM, Jiri Slaby wrote:
> Is this documented somewhere? Or do we rely on an undocumented feature?
> I mean it doesn't refer only to the constraint but also to a concrete
> register allocation. As far as I understand it (from the gcc 4.4
> documentation), if one does
>  "insn %0" : "=r" (out) : "0" (in)
> the "0" constraint corresponds to the concrete register allocated for
> out, not to any register (which is the constraint "r").

Yes, but it only corresponds to the information that is conveyed in the
register selection.

> In the document they write only about the "same location" occupied by in
> and out, nothing is said about size (and hence I think we cannot
> mismatch size of operands). And I couldn't find any other
> restrictions/documentation about inline assembly, hence the patch,
> because nothing assured me this cannot change in the future.

There is almost no documentation at all; some of the little
documentation there is is in comments in the source code.  To a first
order of approximation, asm() is defined by behavior, not by a written
spec.  Trying to play language lawyer with the little bit that is
written down is pointless -- the gcc people have been more than happy to
break asm() between releases regardless of what is and is not written down.

> Now I tried different compilers (clang, llvm-gcc) and they choke on that:
> $ cat c.c
> void x(void)
> {
>         unsigned long in;
>         int out;
>         asm("insn %0" : "=r" (out) : "0" (in));
> }
> $ clang c.c -S -o -
> c.c:5:36: error: unsupported inline asm: input with type 'unsigned long'
>       matching output with type 'int'
>         asm("insn %0" : "=r" (out) : "0" (in));
>                               ~~~         ^~
> 1 diagnostic generated.
> $ llvm-gcc c.c -S -o -
> c.c: In function 'x':
> c.c:5: error: unsupported inline asm: input constraint with a matching
> output constraint of incompatible type!
> 
> thanks for the review,

gcc is the standard for gcc-style asm()... if they don't comply, that a
bug...

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH 1/1] X86: use explicit register name for get/put_user

[Jiri Slaby]

On 12/07/2009 07:35 PM, H. Peter Anvin wrote:
> On 12/07/2009 04:37 AM, Jiri Slaby wrote:
>> In the document they write only about the "same location" occupied by in
>> and out, nothing is said about size (and hence I think we cannot
>> mismatch size of operands). And I couldn't find any other
>> restrictions/documentation about inline assembly, hence the patch,
>> because nothing assured me this cannot change in the future.
> 
> There is almost no documentation at all; some of the little
> documentation there is is in comments in the source code.  To a first
> order of approximation, asm() is defined by behavior, not by a written
> spec.  Trying to play language lawyer with the little bit that is
> written down is pointless -- the gcc people have been more than happy to
> break asm() between releases regardless of what is and is not written down.

Ok, thanks for the explanation.

-- 
js