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

login
register
mail settings
Submitter Adhemerval Zanella Netto
Date Feb. 10, 2020, 7:20 p.m.
Message ID <20200210192038.23588-2-adhemerval.zanella@linaro.org>
Download mbox | patch
Permalink /patch/37868/
State New
Headers show

Comments

Adhemerval Zanella Netto - Feb. 10, 2020, 7:20 p.m.
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(-)
Florian Weimer - Feb. 11, 2020, 11:18 a.m.
* 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 Netto - Feb. 11, 2020, 12:14 p.m.
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.
* 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 Netto - Feb. 11, 2020, 1:31 p.m.
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.
* 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.
* 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 Netto - Feb. 12, 2020, 1:24 p.m.
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.
>

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