[08/15] nios2: Use Linux kABI for syscall return
diff mbox series

Message ID 20200210192038.23588-8-adhemerval.zanella@linaro.org
State New
Headers show
Series
  • [01/15] powerpc: Consolidate Linux syscall definition
Related show

Commit Message

Adhemerval Zanella Feb. 10, 2020, 7:20 p.m. UTC
It changes the nios INTERNAL_SYSCALL_RAW macro to return a negative
value instead of 'r2' register value on 'err' macro argument.

The macro INTERNAL_SYSCALL_DECL is no longer required, and the
INTERNAL_SYSCALL_ERROR_P follows the other Linux kABIS.

Checked with a build against nios2-linux-gnu.
---
 sysdeps/unix/sysv/linux/nios2/sysdep.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Florian Weimer Feb. 11, 2020, 11:20 a.m. UTC | #1
* Adhemerval Zanella:

> +#define INTERNAL_SYSCALL_ERROR_P(val, err) \
> +  ((unsigned long) (val) >= (unsigned long) -4095)

This should use unsigned long int.  Rest looks okay.

Thanks,
Florian
Andreas Schwab Feb. 11, 2020, 11:50 a.m. UTC | #2
On Feb 10 2020, Adhemerval Zanella wrote:

> diff --git a/sysdeps/unix/sysv/linux/nios2/sysdep.h b/sysdeps/unix/sysv/linux/nios2/sysdep.h
> index b02730bd23..eab888df32 100644
> --- a/sysdeps/unix/sysv/linux/nios2/sysdep.h
> +++ b/sysdeps/unix/sysv/linux/nios2/sysdep.h
> @@ -157,13 +157,14 @@
>       (int) result_var; })
>  
>  #undef INTERNAL_SYSCALL_DECL
> -#define INTERNAL_SYSCALL_DECL(err) unsigned int err __attribute__((unused))
> +#define INTERNAL_SYSCALL_DECL(err) do { } while (0)
>  
>  #undef INTERNAL_SYSCALL_ERROR_P
> -#define INTERNAL_SYSCALL_ERROR_P(val, err) ((void) (val), (unsigned int) (err))
> +#define INTERNAL_SYSCALL_ERROR_P(val, err) \
> +  ((unsigned long) (val) >= (unsigned long) -4095)

Perhaps -4095UL instead?

Andreas.
Adhemerval Zanella Feb. 11, 2020, 7:09 p.m. UTC | #3
On 11/02/2020 08:20, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> +#define INTERNAL_SYSCALL_ERROR_P(val, err) \
>> +  ((unsigned long) (val) >= (unsigned long) -4095)
> 
> This should use unsigned long int.  Rest looks okay.
> 
> Thanks,
> Florian
> 

I didn't bother because this snippet will be removed by the
'linux: Consolidate INLINE_SYSCALL' patch in this set, but you
are correct.

I have changed to:
 
#define INTERNAL_SYSCALL_ERROR_P(val, err) \
  ((unsigned long) (val) > -4096UL)

(which is the one I used on consolidation).
Vineet Gupta Feb. 19, 2020, 9:40 p.m. UTC | #4
On 2/10/20 11:20 AM, Adhemerval Zanella wrote:
> It changes the nios INTERNAL_SYSCALL_RAW macro to return a negative
> value instead of 'r2' register value on 'err' macro argument.
> 
> The macro INTERNAL_SYSCALL_DECL is no longer required, and the
> INTERNAL_SYSCALL_ERROR_P follows the other Linux kABIS.
> 
> Checked with a build against nios2-linux-gnu.
> ---
>  sysdeps/unix/sysv/linux/nios2/sysdep.h | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/nios2/sysdep.h b/sysdeps/unix/sysv/linux/nios2/sysdep.h
> index b02730bd23..eab888df32 100644
> --- a/sysdeps/unix/sysv/linux/nios2/sysdep.h
> +++ b/sysdeps/unix/sysv/linux/nios2/sysdep.h
> @@ -157,13 +157,14 @@
>       (int) result_var; })
>  
>  #undef INTERNAL_SYSCALL_DECL
> -#define INTERNAL_SYSCALL_DECL(err) unsigned int err __attribute__((unused))
> +#define INTERNAL_SYSCALL_DECL(err) do { } while (0)
>  
>  #undef INTERNAL_SYSCALL_ERROR_P
> -#define INTERNAL_SYSCALL_ERROR_P(val, err) ((void) (val), (unsigned int) (err))
> +#define INTERNAL_SYSCALL_ERROR_P(val, err) \
> +  ((unsigned long) (val) >= (unsigned long) -4095)
>  
>  #undef INTERNAL_SYSCALL_ERRNO
> -#define INTERNAL_SYSCALL_ERRNO(val, err)   ((void) (err), val)
> +#define INTERNAL_SYSCALL_ERRNO(val, err)     (-(val))
>  
>  #undef INTERNAL_SYSCALL_RAW
>  #define INTERNAL_SYSCALL_RAW(name, err, nr, args...)            \
> @@ -180,8 +181,7 @@
>                       : "+r" (_r2), "=r" (_err)                  \
>                       : ASM_ARGS_##nr				\
>                       : __SYSCALL_CLOBBERS);                     \
> -       _sys_result = _r2;                                       \
> -       err = _err;                                              \
> +       _sys_result = _err != 0 ? -_r2 : -_r2;                   \

Is there a typo here ? both cases seem to be -ve

>       }                                                          \
>       (int) _sys_result; })
>  
>
Adhemerval Zanella Feb. 20, 2020, 1:14 p.m. UTC | #5
On 19/02/2020 18:40, Vineet Gupta wrote:
> On 2/10/20 11:20 AM, Adhemerval Zanella wrote:
>> It changes the nios INTERNAL_SYSCALL_RAW macro to return a negative
>> value instead of 'r2' register value on 'err' macro argument.
>>
>> The macro INTERNAL_SYSCALL_DECL is no longer required, and the
>> INTERNAL_SYSCALL_ERROR_P follows the other Linux kABIS.
>>
>> Checked with a build against nios2-linux-gnu.
>> ---
>>  sysdeps/unix/sysv/linux/nios2/sysdep.h | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/sysdeps/unix/sysv/linux/nios2/sysdep.h b/sysdeps/unix/sysv/linux/nios2/sysdep.h
>> index b02730bd23..eab888df32 100644
>> --- a/sysdeps/unix/sysv/linux/nios2/sysdep.h
>> +++ b/sysdeps/unix/sysv/linux/nios2/sysdep.h
>> @@ -157,13 +157,14 @@
>>       (int) result_var; })
>>  
>>  #undef INTERNAL_SYSCALL_DECL
>> -#define INTERNAL_SYSCALL_DECL(err) unsigned int err __attribute__((unused))
>> +#define INTERNAL_SYSCALL_DECL(err) do { } while (0)
>>  
>>  #undef INTERNAL_SYSCALL_ERROR_P
>> -#define INTERNAL_SYSCALL_ERROR_P(val, err) ((void) (val), (unsigned int) (err))
>> +#define INTERNAL_SYSCALL_ERROR_P(val, err) \
>> +  ((unsigned long) (val) >= (unsigned long) -4095)
>>  
>>  #undef INTERNAL_SYSCALL_ERRNO
>> -#define INTERNAL_SYSCALL_ERRNO(val, err)   ((void) (err), val)
>> +#define INTERNAL_SYSCALL_ERRNO(val, err)     (-(val))
>>  
>>  #undef INTERNAL_SYSCALL_RAW
>>  #define INTERNAL_SYSCALL_RAW(name, err, nr, args...)            \
>> @@ -180,8 +181,7 @@
>>                       : "+r" (_r2), "=r" (_err)                  \
>>                       : ASM_ARGS_##nr				\
>>                       : __SYSCALL_CLOBBERS);                     \
>> -       _sys_result = _r2;                                       \
>> -       err = _err;                                              \
>> +       _sys_result = _err != 0 ? -_r2 : -_r2;                   \
> 
> Is there a typo here ? both cases seem to be -ve

It is, thanks for catching it. I have pushed b790c8c2ed to fix and
double checked nios2 syscall handling (arch/nios2/kernel/entry.S:205)
to certify that the modification does follow nios2 kABI.
Vineet Gupta Feb. 20, 2020, 8:39 p.m. UTC | #6
Hi Adhemerval,

On 2/20/20 5:14 AM, Adhemerval Zanella wrote:
> 
> 
> On 19/02/2020 18:40, Vineet Gupta wrote:
>> On 2/10/20 11:20 AM, Adhemerval Zanella wrote:
>>> It changes the nios INTERNAL_SYSCALL_RAW macro to return a negative
>>> value instead of 'r2' register value on 'err' macro argument.
>>>
>>> The macro INTERNAL_SYSCALL_DECL is no longer required, and the
>>> INTERNAL_SYSCALL_ERROR_P follows the other Linux kABIS.
>>>
>>> Checked with a build against nios2-linux-gnu.
>>> ---
>>>  sysdeps/unix/sysv/linux/nios2/sysdep.h | 10 +++++-----
>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/sysdeps/unix/sysv/linux/nios2/sysdep.h b/sysdeps/unix/sysv/linux/nios2/sysdep.h
>>> index b02730bd23..eab888df32 100644
>>> --- a/sysdeps/unix/sysv/linux/nios2/sysdep.h
>>> +++ b/sysdeps/unix/sysv/linux/nios2/sysdep.h
>>> @@ -157,13 +157,14 @@
>>>       (int) result_var; })
>>>  
>>>  #undef INTERNAL_SYSCALL_DECL
>>> -#define INTERNAL_SYSCALL_DECL(err) unsigned int err __attribute__((unused))
>>> +#define INTERNAL_SYSCALL_DECL(err) do { } while (0)
>>>  
>>>  #undef INTERNAL_SYSCALL_ERROR_P
>>> -#define INTERNAL_SYSCALL_ERROR_P(val, err) ((void) (val), (unsigned int) (err))
>>> +#define INTERNAL_SYSCALL_ERROR_P(val, err) \
>>> +  ((unsigned long) (val) >= (unsigned long) -4095)
>>>  
>>>  #undef INTERNAL_SYSCALL_ERRNO
>>> -#define INTERNAL_SYSCALL_ERRNO(val, err)   ((void) (err), val)
>>> +#define INTERNAL_SYSCALL_ERRNO(val, err)     (-(val))
>>>  
>>>  #undef INTERNAL_SYSCALL_RAW
>>>  #define INTERNAL_SYSCALL_RAW(name, err, nr, args...)            \
>>> @@ -180,8 +181,7 @@
>>>                       : "+r" (_r2), "=r" (_err)                  \
>>>                       : ASM_ARGS_##nr				\
>>>                       : __SYSCALL_CLOBBERS);                     \
>>> -       _sys_result = _r2;                                       \
>>> -       err = _err;                                              \
>>> +       _sys_result = _err != 0 ? -_r2 : -_r2;                   \
>>
>> Is there a typo here ? both cases seem to be -ve
> 
> It is, thanks for catching it. I have pushed b790c8c2ed to fix and
> double checked nios2 syscall handling (arch/nios2/kernel/entry.S:205)
> to certify that the modification does follow nios2 kABI.

Actually the reason I spotted it was trying to replicate similar changes in ARC
port and it seems to be hosed now. It is quite likely a snaufu at my end, but I
don't quite understand the new logic.

Consider brk syscall which does

     __curbrk = (void *) INTERNAL_SYSCALL_CALL (brk, addr);

Through a maze of defines this ends up calling INTERNAL_SYSCALL_RAW which seems be
unconditionally converting !0 value (success) into -ve and returning it. So won't
 it convert a legit brk address return into a -ve and save in __curbrk.

Am I not following this correctly ?

Thx,
-Vineet
Vineet Gupta Feb. 20, 2020, 9:04 p.m. UTC | #7
On 2/20/20 12:39 PM, Vineet Gupta wrote:
> Am I not following this correctly ?

Oh never mind, they use 2 seperate regs to convey syscall result and error, so
your code is right.
Adhemerval Zanella Feb. 27, 2020, 5:49 p.m. UTC | #8
On 20/02/2020 18:04, Vineet Gupta wrote:
> 
> Through a maze of defines this ends up calling INTERNAL_SYSCALL_RAW which seems be
> unconditionally converting !0 value (success) into -ve and returning it. So won't
>  it convert a legit brk address return into a -ve and save in __curbrk.


> On 2/20/20 12:39 PM, Vineet Gupta wrote:
>> Am I not following this correctly ?
> 
> Oh never mind, they use 2 seperate regs to convey syscall result and error, so
> your code is right.
> 

One of my goals is to disentangle the {INTERNAL,INLINE}_SYSCALL macros
to consolidate their definitions and move the arch-specific bits to 
inline functions instead of macros.  Another one is to remove the
requirement to define similar assembly macros to be used by the 
auto-generated one.

The idea is an architecture will just need to define a set of
inline_syscall{0-6} functions.

Patch
diff mbox series

diff --git a/sysdeps/unix/sysv/linux/nios2/sysdep.h b/sysdeps/unix/sysv/linux/nios2/sysdep.h
index b02730bd23..eab888df32 100644
--- a/sysdeps/unix/sysv/linux/nios2/sysdep.h
+++ b/sysdeps/unix/sysv/linux/nios2/sysdep.h
@@ -157,13 +157,14 @@ 
      (int) result_var; })
 
 #undef INTERNAL_SYSCALL_DECL
-#define INTERNAL_SYSCALL_DECL(err) unsigned int err __attribute__((unused))
+#define INTERNAL_SYSCALL_DECL(err) do { } while (0)
 
 #undef INTERNAL_SYSCALL_ERROR_P
-#define INTERNAL_SYSCALL_ERROR_P(val, err) ((void) (val), (unsigned int) (err))
+#define INTERNAL_SYSCALL_ERROR_P(val, err) \
+  ((unsigned long) (val) >= (unsigned long) -4095)
 
 #undef INTERNAL_SYSCALL_ERRNO
-#define INTERNAL_SYSCALL_ERRNO(val, err)   ((void) (err), val)
+#define INTERNAL_SYSCALL_ERRNO(val, err)     (-(val))
 
 #undef INTERNAL_SYSCALL_RAW
 #define INTERNAL_SYSCALL_RAW(name, err, nr, args...)            \
@@ -180,8 +181,7 @@ 
                      : "+r" (_r2), "=r" (_err)                  \
                      : ASM_ARGS_##nr				\
                      : __SYSCALL_CLOBBERS);                     \
-       _sys_result = _r2;                                       \
-       err = _err;                                              \
+       _sys_result = _err != 0 ? -_r2 : -_r2;                   \
      }                                                          \
      (int) _sys_result; })