Re: [PATCH] sh: Implement __get_user_u64() required for 64-bit get_user()

[John Paul Adrian Glaubitz]

Hi Geert!

Thanks a lot for the explanation!

On 5/31/20 12:43 PM, Geert Uytterhoeven wrote:
>> Hmm, this change is not the case for __put_user_asm() vs. __put_user_u64().
>> But I have to admit, I don't know what the part below "3:\n\t" is for.
> 
> It's part of the exception handling, in case the passed (userspace) pointer
> points to an inaccessible address, and triggers an exception.
> 
> For an invalid store, nothing is done, besides returning -EFAULT.
> Hence there's no "mov #0, %1\n\t" in the put_user case.

I have replaced it with two individual mov's now as suggested since I now
understand what's happening here.

> For an invalid load, the data is replaced by zero, and -EFAULT is returned.
> 
>> +__asm__ __volatile__( \
>> +       "1:\n\t" \
>> +       "mov.l  %2,%R1\n\t" \
>> +       "mov.l  %T2,%S1\n\t" \
>> +       "2:\n" \
> 
> (reordering the two sections for easier explanation)
> 
>> +       ".section       __ex_table,\"a\"\n\t" \
>> +       ".long  1b, 3b\n\t" \
> 
> In case an exception happens for the instruction at 1b, jump to 3b.
> 
> Note that the m68k version has two entries here: one for each half of
> the 64-bit access[*].
> I don't know if that is really needed (and thus SH needs it, too), or if
> the exception code handles subsequent instructions automatically.

Hmm. I assume this is something one of the SH maintainers or Yutaka Niibe
can answer.

>> +       ".section       .fixup,\"ax\"\n" \
>> +       "3:\n\t" \
>> +       "mov    #0, %1\n\t" \
> 
> Return zero instead of the data at the (invalid) address.

Makes sense.

>> +       "mov.l  4f, %0\n\t" \
>> +       "jmp    @%0\n\t" \
> 
> Resume at 2b.
> Remember: branch delay slot, so the instruction below is executed first!

I didn't even know that SH has delay slots.

>> +       " mov   %3, %0\n\t" \
> 
> Set err to -EFAULT.

Yes.

>> +       ".balign        4\n" \
>> +       "4:     .long   2b\n\t" \
>> +       ".previous\n" \
> 
>> +       ".previous" \
>> +       :"=&r" (err), "=&r" (x) \
>> +       :"m" (__m(addr)), "i" (-EFAULT), "0" (err)); })
> 
> [*] arch/m68k/include/asm/uaccess_mm.h
> 
>                         "1:     "MOVES".l       (%2)+,%1\n"             \
>                         "2:     "MOVES".l       (%2),%R1\n"             \
> 
>                         "       .section __ex_table,\"a\"\n"            \
>                         "       .align  4\n"                            \
>                         "       .long   1b,10b\n"                       \
>                         "       .long   2b,10b\n"                       \
> 

Hmm. I'll wait for more feedback whether need to do the same as on m68k here.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@xxxxxxxxxx
`. `'   Freie Universitaet Berlin - glaubitz@xxxxxxxxxxxxxxxxxxx
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913