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

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

Commit Message

Adhemerval Zanella Feb. 10, 2020, 7:20 p.m.
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(-)

-- 
2.17.1

Comments

Florian Weimer Feb. 11, 2020, 11:20 a.m. | #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. | #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.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Adhemerval Zanella Feb. 11, 2020, 7:09 p.m. | #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. | #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. | #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. | #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. | #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. | #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 --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; })