diff mbox series

[02/15] powerpc: Use Linux kABI for syscall return

Message ID 20200210192038.23588-2-adhemerval.zanella@linaro.org
State Accepted
Commit e26b8a008ae6f45f6aa2dd56f6d6ce13f181df9f
Headers show
Series [01/15] powerpc: Consolidate Linux syscall definition | expand

Commit Message

Adhemerval Zanella Feb. 10, 2020, 7:20 p.m. UTC 
It changes the powerpc INTERNAL_VSYSCALL_CALL and INTERNAL_SYSCALL_NCS
to return a negative value instead of returning the CR 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 on powerpc64-linux-gnu, powerpc64le-linux-gnu, and
powerpc-linux-gnu-power4.
---
 sysdeps/unix/sysv/linux/powerpc/sysdep.h | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

-- 
2.17.1

Comments

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

> diff --git a/sysdeps/unix/sysv/linux/powerpc/sysdep.h b/sysdeps/unix/sysv/linux/powerpc/sysdep.h

> index 01c26be24b..abdcfd4a63 100644

> --- a/sysdeps/unix/sysv/linux/powerpc/sysdep.h

> +++ b/sysdeps/unix/sysv/linux/powerpc/sysdep.h

> @@ -60,9 +60,8 @@

>         : "+r" (r0), "+r" (r3), "+r" (r4), "+r" (r5),  "+r" (r6),        \

>           "+r" (r7), "+r" (r8)						\

>         : : "r9", "r10", "r11", "r12", "cr0", "ctr", "lr", "memory");	\

> -    err = (long int) r0;						\

>      __asm__ __volatile__ ("" : "=r" (rval) : "r" (r3));		        \

> -    rval;								\

> +    (long int) r0 & (1 << 28) ? -rval : rval;				\

>    })

>  

>  #define INTERNAL_VSYSCALL_CALL(funcptr, err, nr, args...)		\

> @@ -110,21 +109,20 @@

>         : ASM_INPUT_##nr							\

>         : "r9", "r10", "r11", "r12",					\

>           "cr0", "ctr", "memory");					\

> -	  err = r0;  \

> -    r3;  \

> +    r0 & (1 << 28) ? -r3 : r3;						\

>    })

>  #define INTERNAL_SYSCALL(name, err, nr, args...)			\

>    INTERNAL_SYSCALL_NCS (__NR_##name, err, nr, args)

>  

>  #undef INTERNAL_SYSCALL_DECL

> -#define INTERNAL_SYSCALL_DECL(err) long 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), __builtin_expect ((err) & (1 << 28), 0))

> +  ((unsigned long) (val) >= (unsigned long) -4095)

>  

>  #undef INTERNAL_SYSCALL_ERRNO

> -#define INTERNAL_SYSCALL_ERRNO(val, err)     (val)

> +#define INTERNAL_SYSCALL_ERRNO(val, err)     (-(val))

>  

>  #if defined(__PPC64__) || defined(__powerpc64__)

>  # define SYSCALL_ARG_SIZE 8


What's the baseline for this patch?

Thanks,
Florian
Adhemerval Zanella Feb. 11, 2020, 12:14 p.m. UTC | #2 
On 11/02/2020 08:18, Florian Weimer wrote:
> * Adhemerval Zanella:

> 

>> diff --git a/sysdeps/unix/sysv/linux/powerpc/sysdep.h b/sysdeps/unix/sysv/linux/powerpc/sysdep.h

>> index 01c26be24b..abdcfd4a63 100644

>> --- a/sysdeps/unix/sysv/linux/powerpc/sysdep.h

>> +++ b/sysdeps/unix/sysv/linux/powerpc/sysdep.h

>> @@ -60,9 +60,8 @@

>>         : "+r" (r0), "+r" (r3), "+r" (r4), "+r" (r5),  "+r" (r6),        \

>>           "+r" (r7), "+r" (r8)						\

>>         : : "r9", "r10", "r11", "r12", "cr0", "ctr", "lr", "memory");	\

>> -    err = (long int) r0;						\

>>      __asm__ __volatile__ ("" : "=r" (rval) : "r" (r3));		        \

>> -    rval;								\

>> +    (long int) r0 & (1 << 28) ? -rval : rval;				\

>>    })

>>  

>>  #define INTERNAL_VSYSCALL_CALL(funcptr, err, nr, args...)		\

>> @@ -110,21 +109,20 @@

>>         : ASM_INPUT_##nr							\

>>         : "r9", "r10", "r11", "r12",					\

>>           "cr0", "ctr", "memory");					\

>> -	  err = r0;  \

>> -    r3;  \

>> +    r0 & (1 << 28) ? -r3 : r3;						\

>>    })

>>  #define INTERNAL_SYSCALL(name, err, nr, args...)			\

>>    INTERNAL_SYSCALL_NCS (__NR_##name, err, nr, args)

>>  

>>  #undef INTERNAL_SYSCALL_DECL

>> -#define INTERNAL_SYSCALL_DECL(err) long 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), __builtin_expect ((err) & (1 << 28), 0))

>> +  ((unsigned long) (val) >= (unsigned long) -4095)

>>  

>>  #undef INTERNAL_SYSCALL_ERRNO

>> -#define INTERNAL_SYSCALL_ERRNO(val, err)     (val)

>> +#define INTERNAL_SYSCALL_ERRNO(val, err)     (-(val))

>>  

>>  #if defined(__PPC64__) || defined(__powerpc64__)

>>  # define SYSCALL_ARG_SIZE 8

> 

> What's the baseline for this patch?


To simplify the Linux syscall handling on all architectures by using the
already set kABI interface (where returns values from
0xfffffffffffff000 to 0xffffffffffffffff indicates an error). The idea
is initially to consolidate the INLINE_SYSCALL macro and remove the
INTERNAL_SYSCALL_DECL macro.

This refactoring is an initial one, my long-term goal is twofold:

  1. Remove the assembly macros to define syscall and only use the
     C interface. It simplifies ports, requires less hackery to handle
     all its subtitles in C generations (static/pic/etc), and most likely
     would play nice on a possible LTO build.

  2. Rework the syscall interfaces to use static inline instead of
     macros. It will avoid the argument handling that led to the
     subtle BZ#25523 bug and it defines a proper kABI interface.
Florian Weimer Feb. 11, 2020, 12:31 p.m. UTC | #3 
* Adhemerval Zanella:

> On 11/02/2020 08:18, Florian Weimer wrote:

>> * Adhemerval Zanella:

>> 

>>> diff --git a/sysdeps/unix/sysv/linux/powerpc/sysdep.h b/sysdeps/unix/sysv/linux/powerpc/sysdep.h

>>> index 01c26be24b..abdcfd4a63 100644

>>> --- a/sysdeps/unix/sysv/linux/powerpc/sysdep.h

>>> +++ b/sysdeps/unix/sysv/linux/powerpc/sysdep.h

>>> @@ -60,9 +60,8 @@

>>>         : "+r" (r0), "+r" (r3), "+r" (r4), "+r" (r5),  "+r" (r6),        \

>>>           "+r" (r7), "+r" (r8)						\

>>>         : : "r9", "r10", "r11", "r12", "cr0", "ctr", "lr", "memory");	\

>>> -    err = (long int) r0;						\

>>>      __asm__ __volatile__ ("" : "=r" (rval) : "r" (r3));		        \

>>> -    rval;								\

>>> +    (long int) r0 & (1 << 28) ? -rval : rval;				\

>>>    })

>>>  

>>>  #define INTERNAL_VSYSCALL_CALL(funcptr, err, nr, args...)		\

>>> @@ -110,21 +109,20 @@

>>>         : ASM_INPUT_##nr							\

>>>         : "r9", "r10", "r11", "r12",					\

>>>           "cr0", "ctr", "memory");					\

>>> -	  err = r0;  \

>>> -    r3;  \

>>> +    r0 & (1 << 28) ? -r3 : r3;						\

>>>    })

>>>  #define INTERNAL_SYSCALL(name, err, nr, args...)			\

>>>    INTERNAL_SYSCALL_NCS (__NR_##name, err, nr, args)

>>>  

>>>  #undef INTERNAL_SYSCALL_DECL

>>> -#define INTERNAL_SYSCALL_DECL(err) long 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), __builtin_expect ((err) & (1 << 28), 0))

>>> +  ((unsigned long) (val) >= (unsigned long) -4095)

>>>  

>>>  #undef INTERNAL_SYSCALL_ERRNO

>>> -#define INTERNAL_SYSCALL_ERRNO(val, err)     (val)

>>> +#define INTERNAL_SYSCALL_ERRNO(val, err)     (-(val))

>>>  

>>>  #if defined(__PPC64__) || defined(__powerpc64__)

>>>  # define SYSCALL_ARG_SIZE 8

>> 

>> What's the baseline for this patch?

>

> To simplify the Linux syscall handling on all architectures by using the

> already set kABI interface (where returns values from

> 0xfffffffffffff000 to 0xffffffffffffffff indicates an error). The idea

> is initially to consolidate the INLINE_SYSCALL macro and remove the

> INTERNAL_SYSCALL_DECL macro.

>

> This refactoring is an initial one, my long-term goal is twofold:

>

>   1. Remove the assembly macros to define syscall and only use the

>      C interface. It simplifies ports, requires less hackery to handle

>      all its subtitles in C generations (static/pic/etc), and most likely

>      would play nice on a possible LTO build.

>

>   2. Rework the syscall interfaces to use static inline instead of

>      macros. It will avoid the argument handling that led to the

>      subtle BZ#25523 bug and it defines a proper kABI interface.


I meant that the patch doesn't seem to be against master.

I don't have the object 01c26be24b locally.

Thanks,
Florian
Adhemerval Zanella Feb. 11, 2020, 1:31 p.m. UTC | #4 
On 11/02/2020 09:31, Florian Weimer wrote:
> * Adhemerval Zanella:

> 

>> On 11/02/2020 08:18, Florian Weimer wrote:

>>> * Adhemerval Zanella:

>>>

>>>> diff --git a/sysdeps/unix/sysv/linux/powerpc/sysdep.h b/sysdeps/unix/sysv/linux/powerpc/sysdep.h

>>>> index 01c26be24b..abdcfd4a63 100644

>>>> --- a/sysdeps/unix/sysv/linux/powerpc/sysdep.h

>>>> +++ b/sysdeps/unix/sysv/linux/powerpc/sysdep.h

>>>> @@ -60,9 +60,8 @@

>>>>         : "+r" (r0), "+r" (r3), "+r" (r4), "+r" (r5),  "+r" (r6),        \

>>>>           "+r" (r7), "+r" (r8)						\

>>>>         : : "r9", "r10", "r11", "r12", "cr0", "ctr", "lr", "memory");	\

>>>> -    err = (long int) r0;						\

>>>>      __asm__ __volatile__ ("" : "=r" (rval) : "r" (r3));		        \

>>>> -    rval;								\

>>>> +    (long int) r0 & (1 << 28) ? -rval : rval;				\

>>>>    })

>>>>  

>>>>  #define INTERNAL_VSYSCALL_CALL(funcptr, err, nr, args...)		\

>>>> @@ -110,21 +109,20 @@

>>>>         : ASM_INPUT_##nr							\

>>>>         : "r9", "r10", "r11", "r12",					\

>>>>           "cr0", "ctr", "memory");					\

>>>> -	  err = r0;  \

>>>> -    r3;  \

>>>> +    r0 & (1 << 28) ? -r3 : r3;						\

>>>>    })

>>>>  #define INTERNAL_SYSCALL(name, err, nr, args...)			\

>>>>    INTERNAL_SYSCALL_NCS (__NR_##name, err, nr, args)

>>>>  

>>>>  #undef INTERNAL_SYSCALL_DECL

>>>> -#define INTERNAL_SYSCALL_DECL(err) long 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), __builtin_expect ((err) & (1 << 28), 0))

>>>> +  ((unsigned long) (val) >= (unsigned long) -4095)

>>>>  

>>>>  #undef INTERNAL_SYSCALL_ERRNO

>>>> -#define INTERNAL_SYSCALL_ERRNO(val, err)     (val)

>>>> +#define INTERNAL_SYSCALL_ERRNO(val, err)     (-(val))

>>>>  

>>>>  #if defined(__PPC64__) || defined(__powerpc64__)

>>>>  # define SYSCALL_ARG_SIZE 8

>>>

>>> What's the baseline for this patch?

>>

>> To simplify the Linux syscall handling on all architectures by using the

>> already set kABI interface (where returns values from

>> 0xfffffffffffff000 to 0xffffffffffffffff indicates an error). The idea

>> is initially to consolidate the INLINE_SYSCALL macro and remove the

>> INTERNAL_SYSCALL_DECL macro.

>>

>> This refactoring is an initial one, my long-term goal is twofold:

>>

>>   1. Remove the assembly macros to define syscall and only use the

>>      C interface. It simplifies ports, requires less hackery to handle

>>      all its subtitles in C generations (static/pic/etc), and most likely

>>      would play nice on a possible LTO build.

>>

>>   2. Rework the syscall interfaces to use static inline instead of

>>      macros. It will avoid the argument handling that led to the

>>      subtle BZ#25523 bug and it defines a proper kABI interface.

> 

> I meant that the patch doesn't seem to be against master.


Hum I just rebase against master (eb948facd8) and it does apply.  Why
do you think it does not seem to be apply against master?

> 

> I don't have the object 01c26be24b locally.

> 

> Thanks,

> Florian

>
Florian Weimer Feb. 11, 2020, 7:45 p.m. UTC | #5 
* Adhemerval Zanella:

> It changes the powerpc INTERNAL_VSYSCALL_CALL and INTERNAL_SYSCALL_NCS

> to return a negative value instead of returning the CR value on 'err'

> macro argument.


value on?  This part is unclear to me.

> The macro INTERNAL_SYSCALL_DECL is no longer required, and the

> INTERNAL_SYSCALL_ERROR_P follows the other Linux kABIS.


See my other comment about wording here.

The patch looks reasonable to me.
Florian Weimer Feb. 11, 2020, 7:45 p.m. UTC | #6 
* Adhemerval Zanella:

> Hum I just rebase against master (eb948facd8) and it does apply.  Why

> do you think it does not seem to be apply against master?


Never mind, found it, looks like I have trouble reading email today.
Adhemerval Zanella Feb. 12, 2020, 1:24 p.m. UTC | #7 
On 11/02/2020 16:45, Florian Weimer wrote:
> * Adhemerval Zanella:

> 

>> It changes the powerpc INTERNAL_VSYSCALL_CALL and INTERNAL_SYSCALL_NCS

>> to return a negative value instead of returning the CR value on 'err'

>> macro argument.

> 

> value on?  This part is unclear to me.

> 

>> The macro INTERNAL_SYSCALL_DECL is no longer required, and the

>> INTERNAL_SYSCALL_ERROR_P follows the other Linux kABIS.

> 

> See my other comment about wording here.


Ack, I changed based on sparc comments.

> 

> The patch looks reasonable to me.

>
diff mbox series

Patch

diff --git a/sysdeps/unix/sysv/linux/powerpc/sysdep.h b/sysdeps/unix/sysv/linux/powerpc/sysdep.h
index 01c26be24b..abdcfd4a63 100644
--- a/sysdeps/unix/sysv/linux/powerpc/sysdep.h
+++ b/sysdeps/unix/sysv/linux/powerpc/sysdep.h
@@ -60,9 +60,8 @@ 
        : "+r" (r0), "+r" (r3), "+r" (r4), "+r" (r5),  "+r" (r6),        \
          "+r" (r7), "+r" (r8)						\
        : : "r9", "r10", "r11", "r12", "cr0", "ctr", "lr", "memory");	\
-    err = (long int) r0;						\
     __asm__ __volatile__ ("" : "=r" (rval) : "r" (r3));		        \
-    rval;								\
+    (long int) r0 & (1 << 28) ? -rval : rval;				\
   })
 
 #define INTERNAL_VSYSCALL_CALL(funcptr, err, nr, args...)		\
@@ -110,21 +109,20 @@ 
        : ASM_INPUT_##nr							\
        : "r9", "r10", "r11", "r12",					\
          "cr0", "ctr", "memory");					\
-	  err = r0;  \
-    r3;  \
+    r0 & (1 << 28) ? -r3 : r3;						\
   })
 #define INTERNAL_SYSCALL(name, err, nr, args...)			\
   INTERNAL_SYSCALL_NCS (__NR_##name, err, nr, args)
 
 #undef INTERNAL_SYSCALL_DECL
-#define INTERNAL_SYSCALL_DECL(err) long 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), __builtin_expect ((err) & (1 << 28), 0))
+  ((unsigned long) (val) >= (unsigned long) -4095)
 
 #undef INTERNAL_SYSCALL_ERRNO
-#define INTERNAL_SYSCALL_ERRNO(val, err)     (val)
+#define INTERNAL_SYSCALL_ERRNO(val, err)     (-(val))
 
 #if defined(__PPC64__) || defined(__powerpc64__)
 # define SYSCALL_ARG_SIZE 8