[PATCH] ARM: add get_user() support for 8 byte types

[Rob Clark]

On Mon, Nov 12, 2012 at 8:38 AM, Will Deacon <will.deacon at arm.com> wrote:
> On Mon, Nov 12, 2012 at 01:46:57PM +0000, Rob Clark wrote:
>> On Mon, Nov 12, 2012 at 4:46 AM, Will Deacon <will.deacon at arm.com> wrote:
>> > On Fri, Nov 09, 2012 at 09:17:33PM +0000, Rob Clark wrote:
>> >> @@ -122,22 +124,35 @@ extern int __get_user_4(void *);
>> >>       ({                                                              \
>> >>               unsigned long __limit = current_thread_info()->addr_limit - 1; \
>> >>               register const typeof(*(p)) __user *__p asm("r0") = (p);\
>> >> -             register unsigned long __r2 asm("r2");                  \
>> >>               register unsigned long __l asm("r1") = __limit;         \
>> >>               register int __e asm("r0");                             \
>> >>               switch (sizeof(*(__p))) {                               \
>> >> -             case 1:                                                 \
>> >> +             case 1: {                                               \
>> >> +                     register unsigned long __r2 asm("r2");          \
>> >>                       __get_user_x(__r2, __p, __e, __l, 1);           \
>> >> +                     x = (typeof(*(p))) __r2;                        \
>> >>                       break;                                          \
>> >> -             case 2:                                                 \
>> >> +             }                                                       \
>> >> +             case 2: {                                               \
>> >> +                     register unsigned long __r2 asm("r2");          \
>> >>                       __get_user_x(__r2, __p, __e, __l, 2);           \
>> >> +                     x = (typeof(*(p))) __r2;                        \
>> >>                       break;                                          \
>> >> -             case 4:                                                 \
>> >> +             }                                                       \
>> >> +             case 4: {                                               \
>> >> +                     register unsigned long __r2 asm("r2");          \
>> >>                       __get_user_x(__r2, __p, __e, __l, 4);           \
>> >> +                     x = (typeof(*(p))) __r2;                        \
>> >> +                     break;                                          \
>> >> +             }                                                       \
>> >> +             case 8: {                                               \
>> >> +                     register unsigned long long __r2 asm("r2");     \
>> >
>> > Does this matter? For EABI, we'll pass in (r2, r3) and it's all handcrafted
>> > asm, so the compiler shouldn't care much. For OABI, I think you may have to
>> > do some more work to get the two words where you want them.
>>
>> Is the question whether the compiler is guaranteed to allocate r2 and
>> r3 in all cases?  I'm not quite sure, I confess to usually trying to
>> avoid inline asm.  But from looking at the disassembly (for little
>> endian EABI build) it seemed to do the right thing.
>
> I can't recall how OABI represents 64-bit values and particularly whether this
> differs between little and big-endian, so I wondered whether you may have to
> do some marshalling when you assign x. However, a few quick experiments with
> GCC suggest that the register representation matches EABI in regards to word
> ordering (it just doesn't require an even base register), although it would
> be good to find this written down somewhere...

yeah, I was kinda hoping that someone a bit closer to the compiler
would speak up here :-)

>> The only other idea I had was to explicitly declare two 'unsigned
>> long's and then shift them into a 64bit x, although I'm open to
>> suggestions if there is a better way.
>
> Can't you just use register unsigned long long for all cases? Even better,
> follow what put_user does and use typeof(*(p))?

typeof(*(p) was my first try but:

  register typeof(*(p)) __r2 asm("r2");

gives me the error:

  error: read-only variable ‘__r2’ used as ‘asm’ output

I guess because 'const' ends up being part of the typeof *p?  I
suppose I could do typeof(x) instead

BR,
-R

>> >> diff --git a/arch/arm/lib/getuser.S b/arch/arm/lib/getuser.S
>> >> index 9b06bb4..d05285c 100644
>> >> --- a/arch/arm/lib/getuser.S
>> >> +++ b/arch/arm/lib/getuser.S
>> >> @@ -18,7 +18,7 @@
>> >>   * Inputs:   r0 contains the address
>> >>   *           r1 contains the address limit, which must be preserved
>> >>   * Outputs:  r0 is the error code
>> >> - *           r2 contains the zero-extended value
>> >> + *           r2, r3 contains the zero-extended value
>> >>   *           lr corrupted
>> >>   *
>> >>   * No other registers must be altered.  (see <asm/uaccess.h>
>> >> @@ -66,6 +66,19 @@ ENTRY(__get_user_4)
>> >>       mov     pc, lr
>> >>  ENDPROC(__get_user_4)
>> >>
>> >> +ENTRY(__get_user_8)
>> >> +     check_uaccess r0, 4, r1, r2, __get_user_bad
>> >
>> > Shouldn't you be passing 8 here, so that we validate the correct range?
>>
>> yes, sorry, I'll fix that
>>
>> >> +#ifdef CONFIG_THUMB2_KERNEL
>> >> +5: TUSER(ldr)        r2, [r0]
>> >> +6: TUSER(ldr)        r3, [r0, #4]
>> >> +#else
>> >> +5: TUSER(ldr)        r2, [r0], #4
>> >> +6: TUSER(ldr)        r3, [r0]
>> >> +#endif
>> >
>> > This doesn't work for EABI big-endian systems.
>>
>> Hmm, is that true?  Wouldn't put_user() then have the same problem?
>
> I dug up the PCS and it seems that we arrange the two halves of the
> doubleword to match the ldm/stm memory representation for EABI, so sorry
> for the confusion.
>
>> I guess __ARMEB__ is the flag for big-endian?
>
> That's the thing defined by the compiler, yes.
>
> Will
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html