* [PATCH v4 01/20] powerpc: Remove asmlinkage from syscall handler definitions
2022-08-24 2:05 [PATCH v4 00/20] powerpc: Syscall wrapper and register clearing Rohan McLure
@ 2022-08-24 2:05 ` Rohan McLure
2022-08-25 7:04 ` Andrew Donnellan
2022-09-12 8:20 ` Nicholas Piggin
2022-08-24 2:05 ` [PATCH v4 02/20] powerpc: Use generic fallocate compatibility syscall Rohan McLure
` (19 subsequent siblings)
20 siblings, 2 replies; 53+ messages in thread
From: Rohan McLure @ 2022-08-24 2:05 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Rohan McLure
The asmlinkage macro has no special meaning in powerpc, and prior to
this patch is used sporadically on some syscall handler definitions. On
architectures that do not define asmlinkage, it resolves to extern "C"
for C++ compilers and a nop otherwise. The current invocations of
asmlinkage provide far from complete support for C++ toolchains, and so
the macro serves no purpose in powerpc.
Remove all invocations of asmlinkage in arch/powerpc. These incidentally
only occur in syscall definitions and prototypes.
Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
V2 -> V3: new patch
---
arch/powerpc/include/asm/syscalls.h | 16 ++++++++--------
arch/powerpc/kernel/sys_ppc32.c | 8 ++++----
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/include/asm/syscalls.h b/arch/powerpc/include/asm/syscalls.h
index a2b13e55254f..21c2faaa2957 100644
--- a/arch/powerpc/include/asm/syscalls.h
+++ b/arch/powerpc/include/asm/syscalls.h
@@ -10,14 +10,14 @@
struct rtas_args;
-asmlinkage long sys_mmap(unsigned long addr, size_t len,
- unsigned long prot, unsigned long flags,
- unsigned long fd, off_t offset);
-asmlinkage long sys_mmap2(unsigned long addr, size_t len,
- unsigned long prot, unsigned long flags,
- unsigned long fd, unsigned long pgoff);
-asmlinkage long ppc64_personality(unsigned long personality);
-asmlinkage long sys_rtas(struct rtas_args __user *uargs);
+long sys_mmap(unsigned long addr, size_t len,
+ unsigned long prot, unsigned long flags,
+ unsigned long fd, off_t offset);
+long sys_mmap2(unsigned long addr, size_t len,
+ unsigned long prot, unsigned long flags,
+ unsigned long fd, unsigned long pgoff);
+long ppc64_personality(unsigned long personality);
+long sys_rtas(struct rtas_args __user *uargs);
int ppc_select(int n, fd_set __user *inp, fd_set __user *outp,
fd_set __user *exp, struct __kernel_old_timeval __user *tvp);
long ppc_fadvise64_64(int fd, int advice, u32 offset_high, u32 offset_low,
diff --git a/arch/powerpc/kernel/sys_ppc32.c b/arch/powerpc/kernel/sys_ppc32.c
index 16ff0399a257..f4edcc9489fb 100644
--- a/arch/powerpc/kernel/sys_ppc32.c
+++ b/arch/powerpc/kernel/sys_ppc32.c
@@ -85,20 +85,20 @@ compat_ssize_t compat_sys_readahead(int fd, u32 r4, u32 offset1, u32 offset2, u3
return ksys_readahead(fd, merge_64(offset1, offset2), count);
}
-asmlinkage int compat_sys_truncate64(const char __user * path, u32 reg4,
+int compat_sys_truncate64(const char __user * path, u32 reg4,
unsigned long len1, unsigned long len2)
{
return ksys_truncate(path, merge_64(len1, len2));
}
-asmlinkage long compat_sys_fallocate(int fd, int mode, u32 offset1, u32 offset2,
+long compat_sys_fallocate(int fd, int mode, u32 offset1, u32 offset2,
u32 len1, u32 len2)
{
return ksys_fallocate(fd, mode, ((loff_t)offset1 << 32) | offset2,
merge_64(len1, len2));
}
-asmlinkage int compat_sys_ftruncate64(unsigned int fd, u32 reg4, unsigned long len1,
+int compat_sys_ftruncate64(unsigned int fd, u32 reg4, unsigned long len1,
unsigned long len2)
{
return ksys_ftruncate(fd, merge_64(len1, len2));
@@ -111,7 +111,7 @@ long ppc32_fadvise64(int fd, u32 unused, u32 offset1, u32 offset2,
advice);
}
-asmlinkage long compat_sys_sync_file_range2(int fd, unsigned int flags,
+long compat_sys_sync_file_range2(int fd, unsigned int flags,
unsigned offset1, unsigned offset2,
unsigned nbytes1, unsigned nbytes2)
{
--
2.34.1
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v4 01/20] powerpc: Remove asmlinkage from syscall handler definitions
2022-08-24 2:05 ` [PATCH v4 01/20] powerpc: Remove asmlinkage from syscall handler definitions Rohan McLure
@ 2022-08-25 7:04 ` Andrew Donnellan
2022-09-12 8:20 ` Nicholas Piggin
1 sibling, 0 replies; 53+ messages in thread
From: Andrew Donnellan @ 2022-08-25 7:04 UTC (permalink / raw)
To: Rohan McLure, linuxppc-dev
On Wed, 2022-08-24 at 12:05 +1000, Rohan McLure wrote:
> The asmlinkage macro has no special meaning in powerpc, and prior to
> this patch is used sporadically on some syscall handler definitions.
> On
> architectures that do not define asmlinkage, it resolves to extern
> "C"
> for C++ compilers and a nop otherwise. The current invocations of
> asmlinkage provide far from complete support for C++ toolchains, and
> so
> the macro serves no purpose in powerpc.
>
> Remove all invocations of asmlinkage in arch/powerpc. These
> incidentally
> only occur in syscall definitions and prototypes.
>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
This does indeed get rid of every reference to asmlinkage in
arch/powerpc.
Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>
> ---
> V2 -> V3: new patch
> ---
> arch/powerpc/include/asm/syscalls.h | 16 ++++++++--------
> arch/powerpc/kernel/sys_ppc32.c | 8 ++++----
> 2 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/syscalls.h
> b/arch/powerpc/include/asm/syscalls.h
> index a2b13e55254f..21c2faaa2957 100644
> --- a/arch/powerpc/include/asm/syscalls.h
> +++ b/arch/powerpc/include/asm/syscalls.h
> @@ -10,14 +10,14 @@
>
> struct rtas_args;
>
> -asmlinkage long sys_mmap(unsigned long addr, size_t len,
> - unsigned long prot, unsigned long flags,
> - unsigned long fd, off_t offset);
> -asmlinkage long sys_mmap2(unsigned long addr, size_t len,
> - unsigned long prot, unsigned long flags,
> - unsigned long fd, unsigned long pgoff);
> -asmlinkage long ppc64_personality(unsigned long personality);
> -asmlinkage long sys_rtas(struct rtas_args __user *uargs);
> +long sys_mmap(unsigned long addr, size_t len,
> + unsigned long prot, unsigned long flags,
> + unsigned long fd, off_t offset);
> +long sys_mmap2(unsigned long addr, size_t len,
> + unsigned long prot, unsigned long flags,
> + unsigned long fd, unsigned long pgoff);
> +long ppc64_personality(unsigned long personality);
> +long sys_rtas(struct rtas_args __user *uargs);
> int ppc_select(int n, fd_set __user *inp, fd_set __user *outp,
> fd_set __user *exp, struct __kernel_old_timeval __user
> *tvp);
> long ppc_fadvise64_64(int fd, int advice, u32 offset_high, u32
> offset_low,
> diff --git a/arch/powerpc/kernel/sys_ppc32.c
> b/arch/powerpc/kernel/sys_ppc32.c
> index 16ff0399a257..f4edcc9489fb 100644
> --- a/arch/powerpc/kernel/sys_ppc32.c
> +++ b/arch/powerpc/kernel/sys_ppc32.c
> @@ -85,20 +85,20 @@ compat_ssize_t compat_sys_readahead(int fd, u32
> r4, u32 offset1, u32 offset2, u3
> return ksys_readahead(fd, merge_64(offset1, offset2), count);
> }
>
> -asmlinkage int compat_sys_truncate64(const char __user * path, u32
> reg4,
> +int compat_sys_truncate64(const char __user * path, u32 reg4,
> unsigned long len1, unsigned long
> len2)
> {
> return ksys_truncate(path, merge_64(len1, len2));
> }
>
> -asmlinkage long compat_sys_fallocate(int fd, int mode, u32 offset1,
> u32 offset2,
> +long compat_sys_fallocate(int fd, int mode, u32 offset1, u32
> offset2,
> u32 len1, u32 len2)
> {
> return ksys_fallocate(fd, mode, ((loff_t)offset1 << 32) |
> offset2,
> merge_64(len1, len2));
> }
>
> -asmlinkage int compat_sys_ftruncate64(unsigned int fd, u32 reg4,
> unsigned long len1,
> +int compat_sys_ftruncate64(unsigned int fd, u32 reg4, unsigned long
> len1,
> unsigned long len2)
> {
> return ksys_ftruncate(fd, merge_64(len1, len2));
> @@ -111,7 +111,7 @@ long ppc32_fadvise64(int fd, u32 unused, u32
> offset1, u32 offset2,
> advice);
> }
>
> -asmlinkage long compat_sys_sync_file_range2(int fd, unsigned int
> flags,
> +long compat_sys_sync_file_range2(int fd, unsigned int flags,
> unsigned offset1, unsigned
> offset2,
> unsigned nbytes1, unsigned
> nbytes2)
> {
--
Andrew Donnellan OzLabs, ADL Canberra
ajd@linux.ibm.com IBM Australia Limited
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 01/20] powerpc: Remove asmlinkage from syscall handler definitions
2022-08-24 2:05 ` [PATCH v4 01/20] powerpc: Remove asmlinkage from syscall handler definitions Rohan McLure
2022-08-25 7:04 ` Andrew Donnellan
@ 2022-09-12 8:20 ` Nicholas Piggin
1 sibling, 0 replies; 53+ messages in thread
From: Nicholas Piggin @ 2022-09-12 8:20 UTC (permalink / raw)
To: Rohan McLure, linuxppc-dev
On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
> The asmlinkage macro has no special meaning in powerpc, and prior to
> this patch is used sporadically on some syscall handler definitions. On
> architectures that do not define asmlinkage, it resolves to extern "C"
> for C++ compilers and a nop otherwise. The current invocations of
> asmlinkage provide far from complete support for C++ toolchains, and so
> the macro serves no purpose in powerpc.
>
> Remove all invocations of asmlinkage in arch/powerpc. These incidentally
> only occur in syscall definitions and prototypes.
>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 02/20] powerpc: Use generic fallocate compatibility syscall
2022-08-24 2:05 [PATCH v4 00/20] powerpc: Syscall wrapper and register clearing Rohan McLure
2022-08-24 2:05 ` [PATCH v4 01/20] powerpc: Remove asmlinkage from syscall handler definitions Rohan McLure
@ 2022-08-24 2:05 ` Rohan McLure
2022-09-12 8:38 ` Nicholas Piggin
2022-08-24 2:05 ` [PATCH v4 03/20] powerpc/32: Remove powerpc select specialisation Rohan McLure
` (18 subsequent siblings)
20 siblings, 1 reply; 53+ messages in thread
From: Rohan McLure @ 2022-08-24 2:05 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Rohan McLure, Arnd Bergmann
The powerpc fallocate compat syscall handler is identical to the
generic implementation provided by commit 59c10c52f573f ("riscv:
compat: syscall: Add compat_sys_call_table implementation"), and as
such can be removed in favour of the generic implementation.
A future patch series will replace more architecture-defined syscall
handlers with generic implementations, dependent on introducing generic
implementations that are compatible with powerpc and arm's parameter
reorderings.
Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
V1 -> V2: Remove arch-specific fallocate handler.
V2 -> V3: Remove generic fallocate prototype. Move to beginning of
series.
---
arch/powerpc/include/asm/compat.h | 5 +++++
arch/powerpc/include/asm/syscalls.h | 2 --
arch/powerpc/include/asm/unistd.h | 1 +
3 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/include/asm/compat.h b/arch/powerpc/include/asm/compat.h
index dda4091fd012..f20caae3f019 100644
--- a/arch/powerpc/include/asm/compat.h
+++ b/arch/powerpc/include/asm/compat.h
@@ -16,6 +16,11 @@ typedef u16 compat_ipc_pid_t;
#include <asm-generic/compat.h>
#ifdef __BIG_ENDIAN__
+#define compat_arg_u64(name) u32 name##_hi, u32 name##_lo
+#define compat_arg_u64_dual(name) u32, name##_hi, u32, name##_lo
+#define compat_arg_u64_glue(name) (((u64)name##_lo & 0xffffffffUL) | \
+ ((u64)name##_hi << 32))
+
#define COMPAT_UTS_MACHINE "ppc\0\0"
#else
#define COMPAT_UTS_MACHINE "ppcle\0\0"
diff --git a/arch/powerpc/include/asm/syscalls.h b/arch/powerpc/include/asm/syscalls.h
index 21c2faaa2957..675a8f5ec3ca 100644
--- a/arch/powerpc/include/asm/syscalls.h
+++ b/arch/powerpc/include/asm/syscalls.h
@@ -39,8 +39,6 @@ compat_ssize_t compat_sys_readahead(int fd, u32 r4, u32 offset1, u32 offset2, u3
int compat_sys_truncate64(const char __user *path, u32 reg4,
unsigned long len1, unsigned long len2);
-long compat_sys_fallocate(int fd, int mode, u32 offset1, u32 offset2, u32 len1, u32 len2);
-
int compat_sys_ftruncate64(unsigned int fd, u32 reg4, unsigned long len1,
unsigned long len2);
diff --git a/arch/powerpc/include/asm/unistd.h b/arch/powerpc/include/asm/unistd.h
index b1129b4ef57d..659a996c75aa 100644
--- a/arch/powerpc/include/asm/unistd.h
+++ b/arch/powerpc/include/asm/unistd.h
@@ -45,6 +45,7 @@
#define __ARCH_WANT_SYS_UTIME
#define __ARCH_WANT_SYS_NEWFSTATAT
#define __ARCH_WANT_COMPAT_STAT
+#define __ARCH_WANT_COMPAT_FALLOCATE
#define __ARCH_WANT_COMPAT_SYS_SENDFILE
#endif
#define __ARCH_WANT_SYS_FORK
--
2.34.1
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v4 02/20] powerpc: Use generic fallocate compatibility syscall
2022-08-24 2:05 ` [PATCH v4 02/20] powerpc: Use generic fallocate compatibility syscall Rohan McLure
@ 2022-09-12 8:38 ` Nicholas Piggin
2022-09-12 9:57 ` Arnd Bergmann
0 siblings, 1 reply; 53+ messages in thread
From: Nicholas Piggin @ 2022-09-12 8:38 UTC (permalink / raw)
To: Rohan McLure, linuxppc-dev; +Cc: Arnd Bergmann
On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
> The powerpc fallocate compat syscall handler is identical to the
> generic implementation provided by commit 59c10c52f573f ("riscv:
> compat: syscall: Add compat_sys_call_table implementation"), and as
> such can be removed in favour of the generic implementation.
>
> A future patch series will replace more architecture-defined syscall
> handlers with generic implementations, dependent on introducing generic
> implementations that are compatible with powerpc and arm's parameter
> reorderings.
>
> Reported-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> V1 -> V2: Remove arch-specific fallocate handler.
> V2 -> V3: Remove generic fallocate prototype. Move to beginning of
> series.
> ---
> arch/powerpc/include/asm/compat.h | 5 +++++
> arch/powerpc/include/asm/syscalls.h | 2 --
> arch/powerpc/include/asm/unistd.h | 1 +
> 3 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/compat.h b/arch/powerpc/include/asm/compat.h
> index dda4091fd012..f20caae3f019 100644
> --- a/arch/powerpc/include/asm/compat.h
> +++ b/arch/powerpc/include/asm/compat.h
> @@ -16,6 +16,11 @@ typedef u16 compat_ipc_pid_t;
> #include <asm-generic/compat.h>
>
> #ifdef __BIG_ENDIAN__
> +#define compat_arg_u64(name) u32 name##_hi, u32 name##_lo
> +#define compat_arg_u64_dual(name) u32, name##_hi, u32, name##_lo
> +#define compat_arg_u64_glue(name) (((u64)name##_lo & 0xffffffffUL) | \
> + ((u64)name##_hi << 32))
Is there a reason not to put this in asm-generic/compat.h?
Possibly you want to put this with the other compat definitions and
above the asm-generic include. The generic header expects the arch to
include it after defining what it wants to override.
Not sure why x_lo gets cast from u32 to u64 and masked before the |
there, but generic code does the same so this isn't the place to
change it.
Thanks,
Nick
> +
> #define COMPAT_UTS_MACHINE "ppc\0\0"
> #else
> #define COMPAT_UTS_MACHINE "ppcle\0\0"
> diff --git a/arch/powerpc/include/asm/syscalls.h b/arch/powerpc/include/asm/syscalls.h
> index 21c2faaa2957..675a8f5ec3ca 100644
> --- a/arch/powerpc/include/asm/syscalls.h
> +++ b/arch/powerpc/include/asm/syscalls.h
> @@ -39,8 +39,6 @@ compat_ssize_t compat_sys_readahead(int fd, u32 r4, u32 offset1, u32 offset2, u3
> int compat_sys_truncate64(const char __user *path, u32 reg4,
> unsigned long len1, unsigned long len2);
>
> -long compat_sys_fallocate(int fd, int mode, u32 offset1, u32 offset2, u32 len1, u32 len2);
> -
> int compat_sys_ftruncate64(unsigned int fd, u32 reg4, unsigned long len1,
> unsigned long len2);
>
> diff --git a/arch/powerpc/include/asm/unistd.h b/arch/powerpc/include/asm/unistd.h
> index b1129b4ef57d..659a996c75aa 100644
> --- a/arch/powerpc/include/asm/unistd.h
> +++ b/arch/powerpc/include/asm/unistd.h
> @@ -45,6 +45,7 @@
> #define __ARCH_WANT_SYS_UTIME
> #define __ARCH_WANT_SYS_NEWFSTATAT
> #define __ARCH_WANT_COMPAT_STAT
> +#define __ARCH_WANT_COMPAT_FALLOCATE
> #define __ARCH_WANT_COMPAT_SYS_SENDFILE
> #endif
> #define __ARCH_WANT_SYS_FORK
> --
> 2.34.1
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 02/20] powerpc: Use generic fallocate compatibility syscall
2022-09-12 8:38 ` Nicholas Piggin
@ 2022-09-12 9:57 ` Arnd Bergmann
2022-09-12 11:00 ` Christophe Leroy
0 siblings, 1 reply; 53+ messages in thread
From: Arnd Bergmann @ 2022-09-12 9:57 UTC (permalink / raw)
To: Nicholas Piggin, Rohan McLure, linuxppc-dev
On Mon, Sep 12, 2022, at 10:38 AM, Nicholas Piggin wrote:
> On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
>> The powerpc fallocate compat syscall handler is identical to the
>> generic implementation provided by commit 59c10c52f573f ("riscv:
>> compat: syscall: Add compat_sys_call_table implementation"), and as
>> such can be removed in favour of the generic implementation.
>>
>> A future patch series will replace more architecture-defined syscall
>> handlers with generic implementations, dependent on introducing generic
>> implementations that are compatible with powerpc and arm's parameter
>> reorderings.
>>
>> Reported-by: Arnd Bergmann <arnd@arndb.de>
>> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
>> ---
>> V1 -> V2: Remove arch-specific fallocate handler.
>> V2 -> V3: Remove generic fallocate prototype. Move to beginning of
>> series.
>> ---
>> @@ -16,6 +16,11 @@ typedef u16 compat_ipc_pid_t;
>> #include <asm-generic/compat.h>
>>
>> #ifdef __BIG_ENDIAN__
>> +#define compat_arg_u64(name) u32 name##_hi, u32 name##_lo
>> +#define compat_arg_u64_dual(name) u32, name##_hi, u32, name##_lo
>> +#define compat_arg_u64_glue(name) (((u64)name##_lo & 0xffffffffUL) | \
>> + ((u64)name##_hi << 32))
>
> Is there a reason not to put this in asm-generic/compat.h?
>
> Possibly you want to put this with the other compat definitions and
> above the asm-generic include. The generic header expects the arch to
> include it after defining what it wants to override.
Yes, makes sense. I think the riscv people added this to asm-generic,
they tried to do only the minimal parts.
In theory, any architecture could have its own calling conventions
for each syscall and have them in the opposite order for one
endianess. I checked the seven non-generic implementations of the
sys_fallocate() syscall and all except powerpc have the same
ABI as the generic one.
The powerpc difference is that in little-endian mode, only
the 'len' argument is swapped but the 'offset' argument is
still high/low:
long compat_sys_fallocate(int fd, int mode, u32 offset1, u32 offset2,
u32 len1, u32 len2)
{
return ksys_fallocate(fd, mode, ((loff_t)offset1 << 32) | offset2,
merge_64(len1, len2));
}
It's probably best to first fix this by using merge_64(offset1,
offset2) and allow that patch to be backported to stable kernels,
before changing it over to the generic code in a separate patch
within that series.
A related issue seems to exist in ppc_fadvise64_64(), which
uses the wrong argument order on ppc32le compat tasks, in addition
to having at least three different calling conventions across
architectures.
Arnd
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 02/20] powerpc: Use generic fallocate compatibility syscall
2022-09-12 9:57 ` Arnd Bergmann
@ 2022-09-12 11:00 ` Christophe Leroy
2022-09-12 11:07 ` Arnd Bergmann
0 siblings, 1 reply; 53+ messages in thread
From: Christophe Leroy @ 2022-09-12 11:00 UTC (permalink / raw)
To: Arnd Bergmann, Nicholas Piggin, Rohan McLure, linuxppc-dev
Le 12/09/2022 à 11:57, Arnd Bergmann a écrit :
> On Mon, Sep 12, 2022, at 10:38 AM, Nicholas Piggin wrote:
>> On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
>>> The powerpc fallocate compat syscall handler is identical to the
>>> generic implementation provided by commit 59c10c52f573f ("riscv:
>>> compat: syscall: Add compat_sys_call_table implementation"), and as
>>> such can be removed in favour of the generic implementation.
>>>
>>> A future patch series will replace more architecture-defined syscall
>>> handlers with generic implementations, dependent on introducing generic
>>> implementations that are compatible with powerpc and arm's parameter
>>> reorderings.
>>>
>>> Reported-by: Arnd Bergmann <arnd@arndb.de>
>>> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
>>> ---
>>> V1 -> V2: Remove arch-specific fallocate handler.
>>> V2 -> V3: Remove generic fallocate prototype. Move to beginning of
>>> series.
>>> ---
>
>>> @@ -16,6 +16,11 @@ typedef u16 compat_ipc_pid_t;
>>> #include <asm-generic/compat.h>
>>>
>>> #ifdef __BIG_ENDIAN__
>>> +#define compat_arg_u64(name) u32 name##_hi, u32 name##_lo
>>> +#define compat_arg_u64_dual(name) u32, name##_hi, u32, name##_lo
>>> +#define compat_arg_u64_glue(name) (((u64)name##_lo & 0xffffffffUL) | \
>>> + ((u64)name##_hi << 32))
>>
>> Is there a reason not to put this in asm-generic/compat.h?
>>
>> Possibly you want to put this with the other compat definitions and
>> above the asm-generic include. The generic header expects the arch to
>> include it after defining what it wants to override.
>
> Yes, makes sense. I think the riscv people added this to asm-generic,
> they tried to do only the minimal parts.
>
> In theory, any architecture could have its own calling conventions
> for each syscall and have them in the opposite order for one
> endianess. I checked the seven non-generic implementations of the
> sys_fallocate() syscall and all except powerpc have the same
> ABI as the generic one.
>
> The powerpc difference is that in little-endian mode, only
> the 'len' argument is swapped but the 'offset' argument is
> still high/low:
>
> long compat_sys_fallocate(int fd, int mode, u32 offset1, u32 offset2,
> u32 len1, u32 len2)
> {
> return ksys_fallocate(fd, mode, ((loff_t)offset1 << 32) | offset2,
> merge_64(len1, len2));
> }
>
> It's probably best to first fix this by using merge_64(offset1,
> offset2) and allow that patch to be backported to stable kernels,
> before changing it over to the generic code in a separate patch
> within that series.
>
> A related issue seems to exist in ppc_fadvise64_64(), which
> uses the wrong argument order on ppc32le compat tasks, in addition
> to having at least three different calling conventions across
> architectures.
Do ppc32le exist at all ?
Native ppc32 is be only, and I'm not aware that ppc64 is able to run
ppc32le compat tasks.
Christophe
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 02/20] powerpc: Use generic fallocate compatibility syscall
2022-09-12 11:00 ` Christophe Leroy
@ 2022-09-12 11:07 ` Arnd Bergmann
0 siblings, 0 replies; 53+ messages in thread
From: Arnd Bergmann @ 2022-09-12 11:07 UTC (permalink / raw)
To: Christophe Leroy, Nicholas Piggin, Rohan McLure, linuxppc-dev
On Mon, Sep 12, 2022, at 1:00 PM, Christophe Leroy wrote:
> Le 12/09/2022 à 11:57, Arnd Bergmann a écrit :
>> On Mon, Sep 12, 2022, at 10:38 AM, Nicholas Piggin wrote:
>>
>> The powerpc difference is that in little-endian mode, only
>> the 'len' argument is swapped but the 'offset' argument is
>> still high/low:
>>
>> long compat_sys_fallocate(int fd, int mode, u32 offset1, u32 offset2,
>> u32 len1, u32 len2)
>> {
>> return ksys_fallocate(fd, mode, ((loff_t)offset1 << 32) | offset2,
>> merge_64(len1, len2));
>> }
>>
>> It's probably best to first fix this by using merge_64(offset1,
>> offset2) and allow that patch to be backported to stable kernels,
>> before changing it over to the generic code in a separate patch
>> within that series.
>>
>> A related issue seems to exist in ppc_fadvise64_64(), which
>> uses the wrong argument order on ppc32le compat tasks, in addition
>> to having at least three different calling conventions across
>> architectures.
>
> Do ppc32le exist at all ?
>
> Native ppc32 is be only, and I'm not aware that ppc64 is able to run
> ppc32le compat tasks.
I'm not aware of anyone using it, but commit 6e944aed8859
("powerpc/64: Make COMPAT user-selectable disabled on littleendian
by default.") added support to the kernel, and commit
57f48b4b74e7 ("powerpc/compat_sys: swap hi/lo parts of 64-bit
syscall args in LE mode") changed the compat syscall helpers
to pass 64-bit arguments correctly but apparently got fallocate()
and fadvise64_64() wrong.
With Rohan's series, we use generic implementation, which
is the sensible ABI but the change is technically an ABI
change for ppc32le, and it makes sense to split the change
that fixes the behavior from the cleanup.
Arnd
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 03/20] powerpc/32: Remove powerpc select specialisation
2022-08-24 2:05 [PATCH v4 00/20] powerpc: Syscall wrapper and register clearing Rohan McLure
2022-08-24 2:05 ` [PATCH v4 01/20] powerpc: Remove asmlinkage from syscall handler definitions Rohan McLure
2022-08-24 2:05 ` [PATCH v4 02/20] powerpc: Use generic fallocate compatibility syscall Rohan McLure
@ 2022-08-24 2:05 ` Rohan McLure
2022-09-12 9:03 ` Nicholas Piggin
2022-08-24 2:05 ` [PATCH v4 04/20] powerpc: Provide do_ppc64_personality helper Rohan McLure
` (17 subsequent siblings)
20 siblings, 1 reply; 53+ messages in thread
From: Rohan McLure @ 2022-08-24 2:05 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Rohan McLure
Syscall #82 has been implemented for 32-bit platforms in a unique way on
powerpc systems. This hack will in effect guess whether the caller is
expecting new select semantics or old select semantics. It does so via a
guess, based off the first parameter. In new select, this parameter
represents the length of a user-memory array of file descriptors, and in
old select this is a pointer to an arguments structure.
The heuristic simply interprets sufficiently large values of its first
parameter as being a call to old select. The following is a discussion
on how this syscall should be handled.
Link: https://lore.kernel.org/lkml/13737de5-0eb7-e881-9af0-163b0d29a1a0@csgroup.eu/
As discussed in this thread, the existence of such a hack suggests that for
whatever powerpc binaries may predate glibc, it is most likely that they
would have taken use of the old select semantics. x86 and arm64 both
implement this syscall with oldselect semantics.
Remove the powerpc implementation, and update syscall.tbl to refer to emit
a reference to sys_old_select for 32-bit binaries, in keeping with how
other architectures support syscall #82.
Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
V1 -> V2: Remove arch-specific select handler
V2 -> V3: Remove ppc_old_select prototype in <asm/syscalls.h>. Move to
earlier in patch series
---
arch/powerpc/include/asm/syscalls.h | 2 --
arch/powerpc/kernel/syscalls.c | 17 -----------------
arch/powerpc/kernel/syscalls/syscall.tbl | 2 +-
.../arch/powerpc/entry/syscalls/syscall.tbl | 2 +-
4 files changed, 2 insertions(+), 21 deletions(-)
diff --git a/arch/powerpc/include/asm/syscalls.h b/arch/powerpc/include/asm/syscalls.h
index 675a8f5ec3ca..739498c358a1 100644
--- a/arch/powerpc/include/asm/syscalls.h
+++ b/arch/powerpc/include/asm/syscalls.h
@@ -18,8 +18,6 @@ long sys_mmap2(unsigned long addr, size_t len,
unsigned long fd, unsigned long pgoff);
long ppc64_personality(unsigned long personality);
long sys_rtas(struct rtas_args __user *uargs);
-int ppc_select(int n, fd_set __user *inp, fd_set __user *outp,
- fd_set __user *exp, struct __kernel_old_timeval __user *tvp);
long ppc_fadvise64_64(int fd, int advice, u32 offset_high, u32 offset_low,
u32 len_high, u32 len_low);
diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c
index fc999140bc27..ef5896bee818 100644
--- a/arch/powerpc/kernel/syscalls.c
+++ b/arch/powerpc/kernel/syscalls.c
@@ -63,23 +63,6 @@ SYSCALL_DEFINE6(mmap, unsigned long, addr, size_t, len,
return do_mmap2(addr, len, prot, flags, fd, offset, PAGE_SHIFT);
}
-#ifdef CONFIG_PPC32
-/*
- * Due to some executables calling the wrong select we sometimes
- * get wrong args. This determines how the args are being passed
- * (a single ptr to them all args passed) then calls
- * sys_select() with the appropriate args. -- Cort
- */
-int
-ppc_select(int n, fd_set __user *inp, fd_set __user *outp, fd_set __user *exp, struct __kernel_old_timeval __user *tvp)
-{
- if ((unsigned long)n >= 4096)
- return sys_old_select((void __user *)n);
-
- return sys_select(n, inp, outp, exp, tvp);
-}
-#endif
-
#ifdef CONFIG_PPC64
long ppc64_personality(unsigned long personality)
{
diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
index 2600b4237292..4cbbb810ae10 100644
--- a/arch/powerpc/kernel/syscalls/syscall.tbl
+++ b/arch/powerpc/kernel/syscalls/syscall.tbl
@@ -110,7 +110,7 @@
79 common settimeofday sys_settimeofday compat_sys_settimeofday
80 common getgroups sys_getgroups
81 common setgroups sys_setgroups
-82 32 select ppc_select sys_ni_syscall
+82 32 select sys_old_select sys_ni_syscall
82 64 select sys_ni_syscall
82 spu select sys_ni_syscall
83 common symlink sys_symlink
diff --git a/tools/perf/arch/powerpc/entry/syscalls/syscall.tbl b/tools/perf/arch/powerpc/entry/syscalls/syscall.tbl
index 2600b4237292..4cbbb810ae10 100644
--- a/tools/perf/arch/powerpc/entry/syscalls/syscall.tbl
+++ b/tools/perf/arch/powerpc/entry/syscalls/syscall.tbl
@@ -110,7 +110,7 @@
79 common settimeofday sys_settimeofday compat_sys_settimeofday
80 common getgroups sys_getgroups
81 common setgroups sys_setgroups
-82 32 select ppc_select sys_ni_syscall
+82 32 select sys_old_select sys_ni_syscall
82 64 select sys_ni_syscall
82 spu select sys_ni_syscall
83 common symlink sys_symlink
--
2.34.1
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v4 03/20] powerpc/32: Remove powerpc select specialisation
2022-08-24 2:05 ` [PATCH v4 03/20] powerpc/32: Remove powerpc select specialisation Rohan McLure
@ 2022-09-12 9:03 ` Nicholas Piggin
2022-09-15 4:36 ` Rohan McLure
0 siblings, 1 reply; 53+ messages in thread
From: Nicholas Piggin @ 2022-09-12 9:03 UTC (permalink / raw)
To: Rohan McLure, linuxppc-dev
On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
> Syscall #82 has been implemented for 32-bit platforms in a unique way on
> powerpc systems. This hack will in effect guess whether the caller is
> expecting new select semantics or old select semantics. It does so via a
> guess, based off the first parameter. In new select, this parameter
> represents the length of a user-memory array of file descriptors, and in
> old select this is a pointer to an arguments structure.
>
> The heuristic simply interprets sufficiently large values of its first
> parameter as being a call to old select. The following is a discussion
> on how this syscall should be handled.
>
> Link: https://lore.kernel.org/lkml/13737de5-0eb7-e881-9af0-163b0d29a1a0@csgroup.eu/
Seems okay to me, probably Christophe needs to ack it.
Should some of that history be included directly in this changelog?
Should ppc64 compat be added back too, if this is being updated instead
of removed? I don't know much about compat but it seems odd not provide
it (considering it's just using compat_sys_old_select, isn't it?
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
> As discussed in this thread, the existence of such a hack suggests that for
> whatever powerpc binaries may predate glibc, it is most likely that they
> would have taken use of the old select semantics. x86 and arm64 both
> implement this syscall with oldselect semantics.
>
> Remove the powerpc implementation, and update syscall.tbl to refer to emit
> a reference to sys_old_select for 32-bit binaries, in keeping with how
> other architectures support syscall #82.
>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> V1 -> V2: Remove arch-specific select handler
> V2 -> V3: Remove ppc_old_select prototype in <asm/syscalls.h>. Move to
> earlier in patch series
> ---
> arch/powerpc/include/asm/syscalls.h | 2 --
> arch/powerpc/kernel/syscalls.c | 17 -----------------
> arch/powerpc/kernel/syscalls/syscall.tbl | 2 +-
> .../arch/powerpc/entry/syscalls/syscall.tbl | 2 +-
> 4 files changed, 2 insertions(+), 21 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/syscalls.h b/arch/powerpc/include/asm/syscalls.h
> index 675a8f5ec3ca..739498c358a1 100644
> --- a/arch/powerpc/include/asm/syscalls.h
> +++ b/arch/powerpc/include/asm/syscalls.h
> @@ -18,8 +18,6 @@ long sys_mmap2(unsigned long addr, size_t len,
> unsigned long fd, unsigned long pgoff);
> long ppc64_personality(unsigned long personality);
> long sys_rtas(struct rtas_args __user *uargs);
> -int ppc_select(int n, fd_set __user *inp, fd_set __user *outp,
> - fd_set __user *exp, struct __kernel_old_timeval __user *tvp);
> long ppc_fadvise64_64(int fd, int advice, u32 offset_high, u32 offset_low,
> u32 len_high, u32 len_low);
>
> diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c
> index fc999140bc27..ef5896bee818 100644
> --- a/arch/powerpc/kernel/syscalls.c
> +++ b/arch/powerpc/kernel/syscalls.c
> @@ -63,23 +63,6 @@ SYSCALL_DEFINE6(mmap, unsigned long, addr, size_t, len,
> return do_mmap2(addr, len, prot, flags, fd, offset, PAGE_SHIFT);
> }
>
> -#ifdef CONFIG_PPC32
> -/*
> - * Due to some executables calling the wrong select we sometimes
> - * get wrong args. This determines how the args are being passed
> - * (a single ptr to them all args passed) then calls
> - * sys_select() with the appropriate args. -- Cort
> - */
> -int
> -ppc_select(int n, fd_set __user *inp, fd_set __user *outp, fd_set __user *exp, struct __kernel_old_timeval __user *tvp)
> -{
> - if ((unsigned long)n >= 4096)
> - return sys_old_select((void __user *)n);
> -
> - return sys_select(n, inp, outp, exp, tvp);
> -}
> -#endif
> -
> #ifdef CONFIG_PPC64
> long ppc64_personality(unsigned long personality)
> {
> diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
> index 2600b4237292..4cbbb810ae10 100644
> --- a/arch/powerpc/kernel/syscalls/syscall.tbl
> +++ b/arch/powerpc/kernel/syscalls/syscall.tbl
> @@ -110,7 +110,7 @@
> 79 common settimeofday sys_settimeofday compat_sys_settimeofday
> 80 common getgroups sys_getgroups
> 81 common setgroups sys_setgroups
> -82 32 select ppc_select sys_ni_syscall
> +82 32 select sys_old_select sys_ni_syscall
> 82 64 select sys_ni_syscall
> 82 spu select sys_ni_syscall
> 83 common symlink sys_symlink
> diff --git a/tools/perf/arch/powerpc/entry/syscalls/syscall.tbl b/tools/perf/arch/powerpc/entry/syscalls/syscall.tbl
> index 2600b4237292..4cbbb810ae10 100644
> --- a/tools/perf/arch/powerpc/entry/syscalls/syscall.tbl
> +++ b/tools/perf/arch/powerpc/entry/syscalls/syscall.tbl
> @@ -110,7 +110,7 @@
> 79 common settimeofday sys_settimeofday compat_sys_settimeofday
> 80 common getgroups sys_getgroups
> 81 common setgroups sys_setgroups
> -82 32 select ppc_select sys_ni_syscall
> +82 32 select sys_old_select sys_ni_syscall
> 82 64 select sys_ni_syscall
> 82 spu select sys_ni_syscall
> 83 common symlink sys_symlink
> --
> 2.34.1
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 03/20] powerpc/32: Remove powerpc select specialisation
2022-09-12 9:03 ` Nicholas Piggin
@ 2022-09-15 4:36 ` Rohan McLure
0 siblings, 0 replies; 53+ messages in thread
From: Rohan McLure @ 2022-09-15 4:36 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linuxppc-dev
> On 12 Sep 2022, at 7:03 pm, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
>> Syscall #82 has been implemented for 32-bit platforms in a unique way on
>> powerpc systems. This hack will in effect guess whether the caller is
>> expecting new select semantics or old select semantics. It does so via a
>> guess, based off the first parameter. In new select, this parameter
>> represents the length of a user-memory array of file descriptors, and in
>> old select this is a pointer to an arguments structure.
>>
>> The heuristic simply interprets sufficiently large values of its first
>> parameter as being a call to old select. The following is a discussion
>> on how this syscall should be handled.
>>
>> Link: https://lore.kernel.org/lkml/13737de5-0eb7-e881-9af0-163b0d29a1a0@csgroup.eu/
>
> Seems okay to me, probably Christophe needs to ack it.
> Should some of that history be included directly in this changelog?
>
> Should ppc64 compat be added back too, if this is being updated instead
> of removed? I don't know much about compat but it seems odd not provide
> it (considering it's just using compat_sys_old_select, isn't it?
That would make sense to me. I’ll put that in syscall.tbl.
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
>>
>> As discussed in this thread, the existence of such a hack suggests that for
>> whatever powerpc binaries may predate glibc, it is most likely that they
>> would have taken use of the old select semantics. x86 and arm64 both
>> implement this syscall with oldselect semantics.
>>
>> Remove the powerpc implementation, and update syscall.tbl to refer to emit
>> a reference to sys_old_select for 32-bit binaries, in keeping with how
>> other architectures support syscall #82.
>>
>> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
>> ---
>> V1 -> V2: Remove arch-specific select handler
>> V2 -> V3: Remove ppc_old_select prototype in <asm/syscalls.h>. Move to
>> earlier in patch series
>> ---
>> arch/powerpc/include/asm/syscalls.h | 2 --
>> arch/powerpc/kernel/syscalls.c | 17 -----------------
>> arch/powerpc/kernel/syscalls/syscall.tbl | 2 +-
>> .../arch/powerpc/entry/syscalls/syscall.tbl | 2 +-
>> 4 files changed, 2 insertions(+), 21 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/syscalls.h b/arch/powerpc/include/asm/syscalls.h
>> index 675a8f5ec3ca..739498c358a1 100644
>> --- a/arch/powerpc/include/asm/syscalls.h
>> +++ b/arch/powerpc/include/asm/syscalls.h
>> @@ -18,8 +18,6 @@ long sys_mmap2(unsigned long addr, size_t len,
>> unsigned long fd, unsigned long pgoff);
>> long ppc64_personality(unsigned long personality);
>> long sys_rtas(struct rtas_args __user *uargs);
>> -int ppc_select(int n, fd_set __user *inp, fd_set __user *outp,
>> - fd_set __user *exp, struct __kernel_old_timeval __user *tvp);
>> long ppc_fadvise64_64(int fd, int advice, u32 offset_high, u32 offset_low,
>> u32 len_high, u32 len_low);
>>
>> diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c
>> index fc999140bc27..ef5896bee818 100644
>> --- a/arch/powerpc/kernel/syscalls.c
>> +++ b/arch/powerpc/kernel/syscalls.c
>> @@ -63,23 +63,6 @@ SYSCALL_DEFINE6(mmap, unsigned long, addr, size_t, len,
>> return do_mmap2(addr, len, prot, flags, fd, offset, PAGE_SHIFT);
>> }
>>
>> -#ifdef CONFIG_PPC32
>> -/*
>> - * Due to some executables calling the wrong select we sometimes
>> - * get wrong args. This determines how the args are being passed
>> - * (a single ptr to them all args passed) then calls
>> - * sys_select() with the appropriate args. -- Cort
>> - */
>> -int
>> -ppc_select(int n, fd_set __user *inp, fd_set __user *outp, fd_set __user *exp, struct __kernel_old_timeval __user *tvp)
>> -{
>> - if ((unsigned long)n >= 4096)
>> - return sys_old_select((void __user *)n);
>> -
>> - return sys_select(n, inp, outp, exp, tvp);
>> -}
>> -#endif
>> -
>> #ifdef CONFIG_PPC64
>> long ppc64_personality(unsigned long personality)
>> {
>> diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
>> index 2600b4237292..4cbbb810ae10 100644
>> --- a/arch/powerpc/kernel/syscalls/syscall.tbl
>> +++ b/arch/powerpc/kernel/syscalls/syscall.tbl
>> @@ -110,7 +110,7 @@
>> 79 common settimeofday sys_settimeofday compat_sys_settimeofday
>> 80 common getgroups sys_getgroups
>> 81 common setgroups sys_setgroups
>> -82 32 select ppc_select sys_ni_syscall
>> +82 32 select sys_old_select sys_ni_syscall
>> 82 64 select sys_ni_syscall
>> 82 spu select sys_ni_syscall
>> 83 common symlink sys_symlink
>> diff --git a/tools/perf/arch/powerpc/entry/syscalls/syscall.tbl b/tools/perf/arch/powerpc/entry/syscalls/syscall.tbl
>> index 2600b4237292..4cbbb810ae10 100644
>> --- a/tools/perf/arch/powerpc/entry/syscalls/syscall.tbl
>> +++ b/tools/perf/arch/powerpc/entry/syscalls/syscall.tbl
>> @@ -110,7 +110,7 @@
>> 79 common settimeofday sys_settimeofday compat_sys_settimeofday
>> 80 common getgroups sys_getgroups
>> 81 common setgroups sys_setgroups
>> -82 32 select ppc_select sys_ni_syscall
>> +82 32 select sys_old_select sys_ni_syscall
>> 82 64 select sys_ni_syscall
>> 82 spu select sys_ni_syscall
>> 83 common symlink sys_symlink
>> --
>> 2.34.1
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 04/20] powerpc: Provide do_ppc64_personality helper
2022-08-24 2:05 [PATCH v4 00/20] powerpc: Syscall wrapper and register clearing Rohan McLure
` (2 preceding siblings ...)
2022-08-24 2:05 ` [PATCH v4 03/20] powerpc/32: Remove powerpc select specialisation Rohan McLure
@ 2022-08-24 2:05 ` Rohan McLure
2022-09-12 9:26 ` Nicholas Piggin
2022-08-24 2:05 ` [PATCH v4 05/20] powerpc: Remove direct call to personality syscall handler Rohan McLure
` (16 subsequent siblings)
20 siblings, 1 reply; 53+ messages in thread
From: Rohan McLure @ 2022-08-24 2:05 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Rohan McLure
Avoid duplication in future patch that will define the ppc64_personality
syscall handler in terms of the SYSCALL_DEFINE and COMPAT_SYSCALL_DEFINE
macros, by extracting the common body of ppc64_personality into a helper
function.
Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
V2 -> V3: New commit.
---
arch/powerpc/kernel/syscalls.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c
index ef5896bee818..9f29e451e2de 100644
--- a/arch/powerpc/kernel/syscalls.c
+++ b/arch/powerpc/kernel/syscalls.c
@@ -64,7 +64,7 @@ SYSCALL_DEFINE6(mmap, unsigned long, addr, size_t, len,
}
#ifdef CONFIG_PPC64
-long ppc64_personality(unsigned long personality)
+static inline long do_ppc64_personality(unsigned long personality)
{
long ret;
@@ -76,6 +76,10 @@ long ppc64_personality(unsigned long personality)
ret = (ret & ~PER_MASK) | PER_LINUX;
return ret;
}
+long ppc64_personality(unsigned long personality)
+{
+ return do_ppc64_personality(personality);
+}
#endif
long ppc_fadvise64_64(int fd, int advice, u32 offset_high, u32 offset_low,
--
2.34.1
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v4 04/20] powerpc: Provide do_ppc64_personality helper
2022-08-24 2:05 ` [PATCH v4 04/20] powerpc: Provide do_ppc64_personality helper Rohan McLure
@ 2022-09-12 9:26 ` Nicholas Piggin
0 siblings, 0 replies; 53+ messages in thread
From: Nicholas Piggin @ 2022-09-12 9:26 UTC (permalink / raw)
To: Rohan McLure, linuxppc-dev
On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
> Avoid duplication in future patch that will define the ppc64_personality
> syscall handler in terms of the SYSCALL_DEFINE and COMPAT_SYSCALL_DEFINE
> macros, by extracting the common body of ppc64_personality into a helper
> function.
>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> V2 -> V3: New commit.
> ---
> arch/powerpc/kernel/syscalls.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c
> index ef5896bee818..9f29e451e2de 100644
> --- a/arch/powerpc/kernel/syscalls.c
> +++ b/arch/powerpc/kernel/syscalls.c
> @@ -64,7 +64,7 @@ SYSCALL_DEFINE6(mmap, unsigned long, addr, size_t, len,
> }
>
> #ifdef CONFIG_PPC64
> -long ppc64_personality(unsigned long personality)
> +static inline long do_ppc64_personality(unsigned long personality)
> {
> long ret;
>
If this was merged in patch 7 it would just include the above, right?
I don't really mind if you prefer to split it this way though and make
fewer changes in patch 7.
Could you drop the 'inline'? I don't think it's necessary for modern
compliers. do_mmap2 could drop it too.
I did have a question about the duplication in patch 7, but assuming
you still need this patch
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
> long ppc_fadvise64_64(int fd, int advice, u32 offset_high, u32 offset_low,
> --
> 2.34.1
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 05/20] powerpc: Remove direct call to personality syscall handler
2022-08-24 2:05 [PATCH v4 00/20] powerpc: Syscall wrapper and register clearing Rohan McLure
` (3 preceding siblings ...)
2022-08-24 2:05 ` [PATCH v4 04/20] powerpc: Provide do_ppc64_personality helper Rohan McLure
@ 2022-08-24 2:05 ` Rohan McLure
2022-09-12 9:42 ` Nicholas Piggin
2022-08-24 2:05 ` [PATCH v4 06/20] powerpc: Remove direct call to mmap2 syscall handlers Rohan McLure
` (15 subsequent siblings)
20 siblings, 1 reply; 53+ messages in thread
From: Rohan McLure @ 2022-08-24 2:05 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Rohan McLure
Syscall handlers should not be invoked internally by their symbol names,
as these symbols defined by the architecture-defined SYSCALL_DEFINE
macro. Fortunately, in the case of ppc64_personality, its call to
sys_personality can be replaced with an invocation to the
equivalent ksys_personality inline helper in <linux/syscalls.h>.
Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
V1 -> V2: Use inline helper to deduplicate bodies in compat/regular
implementations.
V3 -> V4: Move to be applied before syscall wrapper.
---
arch/powerpc/kernel/syscalls.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c
index 9f29e451e2de..b8461128c8f7 100644
--- a/arch/powerpc/kernel/syscalls.c
+++ b/arch/powerpc/kernel/syscalls.c
@@ -71,7 +71,7 @@ static inline long do_ppc64_personality(unsigned long personality)
if (personality(current->personality) == PER_LINUX32
&& personality(personality) == PER_LINUX)
personality = (personality & ~PER_MASK) | PER_LINUX32;
- ret = sys_personality(personality);
+ ret = ksys_personality(personality);
if (personality(ret) == PER_LINUX32)
ret = (ret & ~PER_MASK) | PER_LINUX;
return ret;
--
2.34.1
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v4 05/20] powerpc: Remove direct call to personality syscall handler
2022-08-24 2:05 ` [PATCH v4 05/20] powerpc: Remove direct call to personality syscall handler Rohan McLure
@ 2022-09-12 9:42 ` Nicholas Piggin
0 siblings, 0 replies; 53+ messages in thread
From: Nicholas Piggin @ 2022-09-12 9:42 UTC (permalink / raw)
To: Rohan McLure, linuxppc-dev
On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
> Syscall handlers should not be invoked internally by their symbol names,
> as these symbols defined by the architecture-defined SYSCALL_DEFINE
> macro. Fortunately, in the case of ppc64_personality, its call to
> sys_personality can be replaced with an invocation to the
> equivalent ksys_personality inline helper in <linux/syscalls.h>.
Huh. I wonder why sys_personality doesn't just call ksys_personality
too. Anyway this looks good.
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
Several of your steps like this one look like they apply to other archs
as well. You might consider ccing linux-arch for some of these.
>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> V1 -> V2: Use inline helper to deduplicate bodies in compat/regular
> implementations.
> V3 -> V4: Move to be applied before syscall wrapper.
> ---
> arch/powerpc/kernel/syscalls.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c
> index 9f29e451e2de..b8461128c8f7 100644
> --- a/arch/powerpc/kernel/syscalls.c
> +++ b/arch/powerpc/kernel/syscalls.c
> @@ -71,7 +71,7 @@ static inline long do_ppc64_personality(unsigned long personality)
> if (personality(current->personality) == PER_LINUX32
> && personality(personality) == PER_LINUX)
> personality = (personality & ~PER_MASK) | PER_LINUX32;
> - ret = sys_personality(personality);
> + ret = ksys_personality(personality);
> if (personality(ret) == PER_LINUX32)
> ret = (ret & ~PER_MASK) | PER_LINUX;
> return ret;
> --
> 2.34.1
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 06/20] powerpc: Remove direct call to mmap2 syscall handlers
2022-08-24 2:05 [PATCH v4 00/20] powerpc: Syscall wrapper and register clearing Rohan McLure
` (4 preceding siblings ...)
2022-08-24 2:05 ` [PATCH v4 05/20] powerpc: Remove direct call to personality syscall handler Rohan McLure
@ 2022-08-24 2:05 ` Rohan McLure
2022-09-12 9:47 ` Nicholas Piggin
2022-08-24 2:05 ` [PATCH v4 07/20] powerpc: Adopt SYSCALL_DEFINE for arch-specific " Rohan McLure
` (14 subsequent siblings)
20 siblings, 1 reply; 53+ messages in thread
From: Rohan McLure @ 2022-08-24 2:05 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Rohan McLure
Syscall handlers should not be invoked internally by their symbol names,
as these symbols defined by the architecture-defined SYSCALL_DEFINE
macro. Move the compatibility syscall definition for mmap2 to
syscalls.c, so that all mmap implementations can share an inline helper
function, as is done with the personality handlers.
Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
V1 -> V2: Move mmap2 compat implementation to asm/kernel/syscalls.c.
V3 -> V4: Move to be applied before syscall wrapper introduced.
---
arch/powerpc/kernel/sys_ppc32.c | 9 ---------
arch/powerpc/kernel/syscalls.c | 11 +++++++++++
2 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/kernel/sys_ppc32.c b/arch/powerpc/kernel/sys_ppc32.c
index f4edcc9489fb..bc6491ed6454 100644
--- a/arch/powerpc/kernel/sys_ppc32.c
+++ b/arch/powerpc/kernel/sys_ppc32.c
@@ -25,7 +25,6 @@
#include <linux/poll.h>
#include <linux/personality.h>
#include <linux/stat.h>
-#include <linux/mman.h>
#include <linux/in.h>
#include <linux/syscalls.h>
#include <linux/unistd.h>
@@ -48,14 +47,6 @@
#include <asm/syscalls.h>
#include <asm/switch_to.h>
-unsigned long compat_sys_mmap2(unsigned long addr, size_t len,
- unsigned long prot, unsigned long flags,
- unsigned long fd, unsigned long pgoff)
-{
- /* This should remain 12 even if PAGE_SIZE changes */
- return sys_mmap(addr, len, prot, flags, fd, pgoff << 12);
-}
-
/*
* long long munging:
* The 32 bit ABI passes long longs in an odd even register pair.
diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c
index b8461128c8f7..32fadf3c2cd3 100644
--- a/arch/powerpc/kernel/syscalls.c
+++ b/arch/powerpc/kernel/syscalls.c
@@ -56,6 +56,17 @@ SYSCALL_DEFINE6(mmap2, unsigned long, addr, size_t, len,
return do_mmap2(addr, len, prot, flags, fd, pgoff, PAGE_SHIFT-12);
}
+#ifdef CONFIG_COMPAT
+COMPAT_SYSCALL_DEFINE6(mmap2,
+ unsigned long, addr, size_t, len,
+ unsigned long, prot, unsigned long, flags,
+ unsigned long, fd, unsigned long, pgoff)
+{
+ /* This should remain 12 even if PAGE_SIZE changes */
+ return do_mmap2(addr, len, prot, flags, fd, pgoff << 12, PAGE_SHIFT-12);
+}
+#endif
+
SYSCALL_DEFINE6(mmap, unsigned long, addr, size_t, len,
unsigned long, prot, unsigned long, flags,
unsigned long, fd, off_t, offset)
--
2.34.1
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v4 06/20] powerpc: Remove direct call to mmap2 syscall handlers
2022-08-24 2:05 ` [PATCH v4 06/20] powerpc: Remove direct call to mmap2 syscall handlers Rohan McLure
@ 2022-09-12 9:47 ` Nicholas Piggin
2022-09-15 5:06 ` Rohan McLure
0 siblings, 1 reply; 53+ messages in thread
From: Nicholas Piggin @ 2022-09-12 9:47 UTC (permalink / raw)
To: Rohan McLure, linuxppc-dev
On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
> Syscall handlers should not be invoked internally by their symbol names,
> as these symbols defined by the architecture-defined SYSCALL_DEFINE
> macro. Move the compatibility syscall definition for mmap2 to
> syscalls.c, so that all mmap implementations can share an inline helper
> function, as is done with the personality handlers.
>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
Is there any point to keping sys_ppc32.c at all? Might as well move them
all to syscall.c IMO.
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> V1 -> V2: Move mmap2 compat implementation to asm/kernel/syscalls.c.
> V3 -> V4: Move to be applied before syscall wrapper introduced.
> ---
> arch/powerpc/kernel/sys_ppc32.c | 9 ---------
> arch/powerpc/kernel/syscalls.c | 11 +++++++++++
> 2 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/arch/powerpc/kernel/sys_ppc32.c b/arch/powerpc/kernel/sys_ppc32.c
> index f4edcc9489fb..bc6491ed6454 100644
> --- a/arch/powerpc/kernel/sys_ppc32.c
> +++ b/arch/powerpc/kernel/sys_ppc32.c
> @@ -25,7 +25,6 @@
> #include <linux/poll.h>
> #include <linux/personality.h>
> #include <linux/stat.h>
> -#include <linux/mman.h>
> #include <linux/in.h>
> #include <linux/syscalls.h>
> #include <linux/unistd.h>
> @@ -48,14 +47,6 @@
> #include <asm/syscalls.h>
> #include <asm/switch_to.h>
>
> -unsigned long compat_sys_mmap2(unsigned long addr, size_t len,
> - unsigned long prot, unsigned long flags,
> - unsigned long fd, unsigned long pgoff)
> -{
> - /* This should remain 12 even if PAGE_SIZE changes */
> - return sys_mmap(addr, len, prot, flags, fd, pgoff << 12);
> -}
> -
> /*
> * long long munging:
> * The 32 bit ABI passes long longs in an odd even register pair.
> diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c
> index b8461128c8f7..32fadf3c2cd3 100644
> --- a/arch/powerpc/kernel/syscalls.c
> +++ b/arch/powerpc/kernel/syscalls.c
> @@ -56,6 +56,17 @@ SYSCALL_DEFINE6(mmap2, unsigned long, addr, size_t, len,
> return do_mmap2(addr, len, prot, flags, fd, pgoff, PAGE_SHIFT-12);
> }
>
> +#ifdef CONFIG_COMPAT
> +COMPAT_SYSCALL_DEFINE6(mmap2,
> + unsigned long, addr, size_t, len,
> + unsigned long, prot, unsigned long, flags,
> + unsigned long, fd, unsigned long, pgoff)
> +{
> + /* This should remain 12 even if PAGE_SIZE changes */
> + return do_mmap2(addr, len, prot, flags, fd, pgoff << 12, PAGE_SHIFT-12);
> +}
> +#endif
> +
> SYSCALL_DEFINE6(mmap, unsigned long, addr, size_t, len,
> unsigned long, prot, unsigned long, flags,
> unsigned long, fd, off_t, offset)
> --
> 2.34.1
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 06/20] powerpc: Remove direct call to mmap2 syscall handlers
2022-09-12 9:47 ` Nicholas Piggin
@ 2022-09-15 5:06 ` Rohan McLure
0 siblings, 0 replies; 53+ messages in thread
From: Rohan McLure @ 2022-09-15 5:06 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linuxppc-dev
> On 12 Sep 2022, at 7:47 pm, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
>> Syscall handlers should not be invoked internally by their symbol names,
>> as these symbols defined by the architecture-defined SYSCALL_DEFINE
>> macro. Move the compatibility syscall definition for mmap2 to
>> syscalls.c, so that all mmap implementations can share an inline helper
>> function, as is done with the personality handlers.
>>
>> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
>
> Is there any point to keping sys_ppc32.c at all? Might as well move them
> all to syscall.c IMO.
Currently serves as a fairly arbitrary distinginction between compat calls
and others, noting that a compat variant of personality is in syscalls.c.
May as well get rid of it.
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
>> ---
>> V1 -> V2: Move mmap2 compat implementation to asm/kernel/syscalls.c.
>> V3 -> V4: Move to be applied before syscall wrapper introduced.
>> ---
>> arch/powerpc/kernel/sys_ppc32.c | 9 ---------
>> arch/powerpc/kernel/syscalls.c | 11 +++++++++++
>> 2 files changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/sys_ppc32.c b/arch/powerpc/kernel/sys_ppc32.c
>> index f4edcc9489fb..bc6491ed6454 100644
>> --- a/arch/powerpc/kernel/sys_ppc32.c
>> +++ b/arch/powerpc/kernel/sys_ppc32.c
>> @@ -25,7 +25,6 @@
>> #include <linux/poll.h>
>> #include <linux/personality.h>
>> #include <linux/stat.h>
>> -#include <linux/mman.h>
>> #include <linux/in.h>
>> #include <linux/syscalls.h>
>> #include <linux/unistd.h>
>> @@ -48,14 +47,6 @@
>> #include <asm/syscalls.h>
>> #include <asm/switch_to.h>
>>
>> -unsigned long compat_sys_mmap2(unsigned long addr, size_t len,
>> - unsigned long prot, unsigned long flags,
>> - unsigned long fd, unsigned long pgoff)
>> -{
>> - /* This should remain 12 even if PAGE_SIZE changes */
>> - return sys_mmap(addr, len, prot, flags, fd, pgoff << 12);
>> -}
>> -
>> /*
>> * long long munging:
>> * The 32 bit ABI passes long longs in an odd even register pair.
>> diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c
>> index b8461128c8f7..32fadf3c2cd3 100644
>> --- a/arch/powerpc/kernel/syscalls.c
>> +++ b/arch/powerpc/kernel/syscalls.c
>> @@ -56,6 +56,17 @@ SYSCALL_DEFINE6(mmap2, unsigned long, addr, size_t, len,
>> return do_mmap2(addr, len, prot, flags, fd, pgoff, PAGE_SHIFT-12);
>> }
>>
>> +#ifdef CONFIG_COMPAT
>> +COMPAT_SYSCALL_DEFINE6(mmap2,
>> + unsigned long, addr, size_t, len,
>> + unsigned long, prot, unsigned long, flags,
>> + unsigned long, fd, unsigned long, pgoff)
>> +{
>> + /* This should remain 12 even if PAGE_SIZE changes */
>> + return do_mmap2(addr, len, prot, flags, fd, pgoff << 12, PAGE_SHIFT-12);
>> +}
>> +#endif
>> +
>> SYSCALL_DEFINE6(mmap, unsigned long, addr, size_t, len,
>> unsigned long, prot, unsigned long, flags,
>> unsigned long, fd, off_t, offset)
>> --
>> 2.34.1
>
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 07/20] powerpc: Adopt SYSCALL_DEFINE for arch-specific syscall handlers
2022-08-24 2:05 [PATCH v4 00/20] powerpc: Syscall wrapper and register clearing Rohan McLure
` (5 preceding siblings ...)
2022-08-24 2:05 ` [PATCH v4 06/20] powerpc: Remove direct call to mmap2 syscall handlers Rohan McLure
@ 2022-08-24 2:05 ` Rohan McLure
2022-09-12 10:04 ` Nicholas Piggin
2022-08-24 2:05 ` [PATCH v4 08/20] powerpc: Include all arch-specific syscall prototypes Rohan McLure
` (13 subsequent siblings)
20 siblings, 1 reply; 53+ messages in thread
From: Rohan McLure @ 2022-08-24 2:05 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Rohan McLure
Arch-specific implementations of syscall handlers are currently used
over generic implementations for the following reasons:
1. Semantics unique to powerpc
2. Compatibility syscalls require 'argument padding' to comply with
64-bit argument convention in ELF32 abi.
3. Parameter types or order is different in other architectures.
These syscall handlers have been defined prior to this patch series
without invoking the SYSCALL_DEFINE or COMPAT_SYSCALL_DEFINE macros with
custom input and output types. We remove every such direct definition in
favour of the aforementioned macros.
Also update syscalls.tbl in order to refer to the symbol names generated
by each of these macros. Since ppc64_personality can be called by both
64 bit and 32 bit binaries through compatibility, we must generate both
both compat_sys_ and sys_ symbols for this handler.
A number of architectures including arm and powerpc agree on an
alternative argument order and numbering for most of these arch-specific
handlers. A future patch series may allow for asm/unistd.h to signal
through its defines that a generic implementation of these syscall
handlers with the correct calling convention be omitted, through the
__ARCH_WANT_COMPAT_SYS_... convention.
Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
V1 -> V2: All syscall handlers wrapped by this macro.
V2 -> V3: Move creation of do_ppc64_personality helper to prior patch.
V3 -> V4: Fix parenthesis alignment. Don't emit sys_*** symbols.
---
arch/powerpc/include/asm/syscalls.h | 10 ++--
arch/powerpc/kernel/sys_ppc32.c | 45 ++++++++++--------
arch/powerpc/kernel/syscalls.c | 17 +++++--
arch/powerpc/kernel/syscalls/syscall.tbl | 22 ++++-----
.../arch/powerpc/entry/syscalls/syscall.tbl | 22 ++++-----
5 files changed, 64 insertions(+), 52 deletions(-)
diff --git a/arch/powerpc/include/asm/syscalls.h b/arch/powerpc/include/asm/syscalls.h
index 739498c358a1..3e3aff0835a6 100644
--- a/arch/powerpc/include/asm/syscalls.h
+++ b/arch/powerpc/include/asm/syscalls.h
@@ -16,10 +16,10 @@ long sys_mmap(unsigned long addr, size_t len,
long sys_mmap2(unsigned long addr, size_t len,
unsigned long prot, unsigned long flags,
unsigned long fd, unsigned long pgoff);
-long ppc64_personality(unsigned long personality);
+long sys_ppc64_personality(unsigned long personality);
long sys_rtas(struct rtas_args __user *uargs);
-long ppc_fadvise64_64(int fd, int advice, u32 offset_high, u32 offset_low,
- u32 len_high, u32 len_low);
+long sys_ppc_fadvise64_64(int fd, int advice, u32 offset_high, u32 offset_low,
+ u32 len_high, u32 len_low);
#ifdef CONFIG_COMPAT
unsigned long compat_sys_mmap2(unsigned long addr, size_t len,
@@ -40,8 +40,8 @@ int compat_sys_truncate64(const char __user *path, u32 reg4,
int compat_sys_ftruncate64(unsigned int fd, u32 reg4, unsigned long len1,
unsigned long len2);
-long ppc32_fadvise64(int fd, u32 unused, u32 offset1, u32 offset2,
- size_t len, int advice);
+long compat_sys_ppc32_fadvise64(int fd, u32 unused, u32 offset1, u32 offset2,
+ size_t len, int advice);
long compat_sys_sync_file_range2(int fd, unsigned int flags,
unsigned int offset1, unsigned int offset2,
diff --git a/arch/powerpc/kernel/sys_ppc32.c b/arch/powerpc/kernel/sys_ppc32.c
index bc6491ed6454..dd9039671227 100644
--- a/arch/powerpc/kernel/sys_ppc32.c
+++ b/arch/powerpc/kernel/sys_ppc32.c
@@ -59,52 +59,55 @@
#define merge_64(high, low) ((u64)high << 32) | low
#endif
-compat_ssize_t compat_sys_pread64(unsigned int fd, char __user *ubuf, compat_size_t count,
- u32 reg6, u32 pos1, u32 pos2)
+COMPAT_SYSCALL_DEFINE6(ppc_pread64,
+ unsigned int, fd,
+ char __user *, ubuf, compat_size_t, count,
+ u32, reg6, u32, pos1, u32, pos2)
{
return ksys_pread64(fd, ubuf, count, merge_64(pos1, pos2));
}
-compat_ssize_t compat_sys_pwrite64(unsigned int fd, const char __user *ubuf, compat_size_t count,
- u32 reg6, u32 pos1, u32 pos2)
+COMPAT_SYSCALL_DEFINE6(ppc_pwrite64,
+ unsigned int, fd,
+ const char __user *, ubuf, compat_size_t, count,
+ u32, reg6, u32, pos1, u32, pos2)
{
return ksys_pwrite64(fd, ubuf, count, merge_64(pos1, pos2));
}
-compat_ssize_t compat_sys_readahead(int fd, u32 r4, u32 offset1, u32 offset2, u32 count)
+COMPAT_SYSCALL_DEFINE5(ppc_readahead,
+ int, fd, u32, r4,
+ u32, offset1, u32, offset2, u32, count)
{
return ksys_readahead(fd, merge_64(offset1, offset2), count);
}
-int compat_sys_truncate64(const char __user * path, u32 reg4,
- unsigned long len1, unsigned long len2)
+COMPAT_SYSCALL_DEFINE4(ppc_truncate64,
+ const char __user *, path, u32, reg4,
+ unsigned long, len1, unsigned long, len2)
{
return ksys_truncate(path, merge_64(len1, len2));
}
-long compat_sys_fallocate(int fd, int mode, u32 offset1, u32 offset2,
- u32 len1, u32 len2)
-{
- return ksys_fallocate(fd, mode, ((loff_t)offset1 << 32) | offset2,
- merge_64(len1, len2));
-}
-
-int compat_sys_ftruncate64(unsigned int fd, u32 reg4, unsigned long len1,
- unsigned long len2)
+COMPAT_SYSCALL_DEFINE4(ppc_ftruncate64,
+ unsigned int, fd, u32, reg4,
+ unsigned long, len1, unsigned long, len2)
{
return ksys_ftruncate(fd, merge_64(len1, len2));
}
-long ppc32_fadvise64(int fd, u32 unused, u32 offset1, u32 offset2,
- size_t len, int advice)
+COMPAT_SYSCALL_DEFINE6(ppc32_fadvise64,
+ int, fd, u32, unused, u32, offset1, u32, offset2,
+ size_t, len, int, advice)
{
return ksys_fadvise64_64(fd, merge_64(offset1, offset2), len,
advice);
}
-long compat_sys_sync_file_range2(int fd, unsigned int flags,
- unsigned offset1, unsigned offset2,
- unsigned nbytes1, unsigned nbytes2)
+COMPAT_SYSCALL_DEFINE6(ppc_sync_file_range2,
+ int, fd, unsigned int, flags,
+ unsigned int, offset1, unsigned int, offset2,
+ unsigned int, nbytes1, unsigned int, nbytes2)
{
loff_t offset = merge_64(offset1, offset2);
loff_t nbytes = merge_64(nbytes1, nbytes2);
diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c
index 32fadf3c2cd3..2d4c62e5bac7 100644
--- a/arch/powerpc/kernel/syscalls.c
+++ b/arch/powerpc/kernel/syscalls.c
@@ -87,14 +87,23 @@ static inline long do_ppc64_personality(unsigned long personality)
ret = (ret & ~PER_MASK) | PER_LINUX;
return ret;
}
-long ppc64_personality(unsigned long personality)
+
+SYSCALL_DEFINE1(ppc64_personality, unsigned long, personality)
{
return do_ppc64_personality(personality);
}
-#endif
-long ppc_fadvise64_64(int fd, int advice, u32 offset_high, u32 offset_low,
- u32 len_high, u32 len_low)
+#ifdef CONFIG_COMPAT
+COMPAT_SYSCALL_DEFINE1(ppc64_personality, unsigned long, personality)
+{
+ return do_ppc64_personality(personality);
+}
+#endif /* CONFIG_COMPAT */
+#endif /* CONFIG_PPC64 */
+
+SYSCALL_DEFINE6(ppc_fadvise64_64,
+ int, fd, int, advice, u32, offset_high, u32, offset_low,
+ u32, len_high, u32, len_low)
{
return ksys_fadvise64_64(fd, (u64)offset_high << 32 | offset_low,
(u64)len_high << 32 | len_low, advice);
diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
index 4cbbb810ae10..b4c970c9c6b1 100644
--- a/arch/powerpc/kernel/syscalls/syscall.tbl
+++ b/arch/powerpc/kernel/syscalls/syscall.tbl
@@ -178,9 +178,9 @@
133 common fchdir sys_fchdir
134 common bdflush sys_ni_syscall
135 common sysfs sys_sysfs
-136 32 personality sys_personality ppc64_personality
-136 64 personality ppc64_personality
-136 spu personality ppc64_personality
+136 32 personality sys_personality compat_sys_ppc64_personality
+136 64 personality sys_ppc64_personality
+136 spu personality sys_ppc64_personality
137 common afs_syscall sys_ni_syscall
138 common setfsuid sys_setfsuid
139 common setfsgid sys_setfsgid
@@ -228,8 +228,8 @@
176 64 rt_sigtimedwait sys_rt_sigtimedwait
177 nospu rt_sigqueueinfo sys_rt_sigqueueinfo compat_sys_rt_sigqueueinfo
178 nospu rt_sigsuspend sys_rt_sigsuspend compat_sys_rt_sigsuspend
-179 common pread64 sys_pread64 compat_sys_pread64
-180 common pwrite64 sys_pwrite64 compat_sys_pwrite64
+179 common pread64 sys_pread64 compat_sys_ppc_pread64
+180 common pwrite64 sys_pwrite64 compat_sys_ppc_pwrite64
181 common chown sys_chown
182 common getcwd sys_getcwd
183 common capget sys_capget
@@ -242,10 +242,10 @@
188 common putpmsg sys_ni_syscall
189 nospu vfork sys_vfork
190 common ugetrlimit sys_getrlimit compat_sys_getrlimit
-191 common readahead sys_readahead compat_sys_readahead
+191 common readahead sys_readahead compat_sys_ppc_readahead
192 32 mmap2 sys_mmap2 compat_sys_mmap2
-193 32 truncate64 sys_truncate64 compat_sys_truncate64
-194 32 ftruncate64 sys_ftruncate64 compat_sys_ftruncate64
+193 32 truncate64 sys_truncate64 compat_sys_ppc_truncate64
+194 32 ftruncate64 sys_ftruncate64 compat_sys_ppc_ftruncate64
195 32 stat64 sys_stat64
196 32 lstat64 sys_lstat64
197 32 fstat64 sys_fstat64
@@ -288,7 +288,7 @@
230 common io_submit sys_io_submit compat_sys_io_submit
231 common io_cancel sys_io_cancel
232 nospu set_tid_address sys_set_tid_address
-233 common fadvise64 sys_fadvise64 ppc32_fadvise64
+233 common fadvise64 sys_fadvise64 compat_sys_ppc32_fadvise64
234 nospu exit_group sys_exit_group
235 nospu lookup_dcookie sys_lookup_dcookie compat_sys_lookup_dcookie
236 common epoll_create sys_epoll_create
@@ -323,7 +323,7 @@
251 spu utimes sys_utimes
252 common statfs64 sys_statfs64 compat_sys_statfs64
253 common fstatfs64 sys_fstatfs64 compat_sys_fstatfs64
-254 32 fadvise64_64 ppc_fadvise64_64
+254 32 fadvise64_64 sys_ppc_fadvise64_64
254 spu fadvise64_64 sys_ni_syscall
255 common rtas sys_rtas
256 32 sys_debug_setcontext sys_debug_setcontext sys_ni_syscall
@@ -390,7 +390,7 @@
305 common signalfd sys_signalfd compat_sys_signalfd
306 common timerfd_create sys_timerfd_create
307 common eventfd sys_eventfd
-308 common sync_file_range2 sys_sync_file_range2 compat_sys_sync_file_range2
+308 common sync_file_range2 sys_sync_file_range2 compat_sys_ppc_sync_file_range2
309 nospu fallocate sys_fallocate compat_sys_fallocate
310 nospu subpage_prot sys_subpage_prot
311 32 timerfd_settime sys_timerfd_settime32
diff --git a/tools/perf/arch/powerpc/entry/syscalls/syscall.tbl b/tools/perf/arch/powerpc/entry/syscalls/syscall.tbl
index 4cbbb810ae10..b4c970c9c6b1 100644
--- a/tools/perf/arch/powerpc/entry/syscalls/syscall.tbl
+++ b/tools/perf/arch/powerpc/entry/syscalls/syscall.tbl
@@ -178,9 +178,9 @@
133 common fchdir sys_fchdir
134 common bdflush sys_ni_syscall
135 common sysfs sys_sysfs
-136 32 personality sys_personality ppc64_personality
-136 64 personality ppc64_personality
-136 spu personality ppc64_personality
+136 32 personality sys_personality compat_sys_ppc64_personality
+136 64 personality sys_ppc64_personality
+136 spu personality sys_ppc64_personality
137 common afs_syscall sys_ni_syscall
138 common setfsuid sys_setfsuid
139 common setfsgid sys_setfsgid
@@ -228,8 +228,8 @@
176 64 rt_sigtimedwait sys_rt_sigtimedwait
177 nospu rt_sigqueueinfo sys_rt_sigqueueinfo compat_sys_rt_sigqueueinfo
178 nospu rt_sigsuspend sys_rt_sigsuspend compat_sys_rt_sigsuspend
-179 common pread64 sys_pread64 compat_sys_pread64
-180 common pwrite64 sys_pwrite64 compat_sys_pwrite64
+179 common pread64 sys_pread64 compat_sys_ppc_pread64
+180 common pwrite64 sys_pwrite64 compat_sys_ppc_pwrite64
181 common chown sys_chown
182 common getcwd sys_getcwd
183 common capget sys_capget
@@ -242,10 +242,10 @@
188 common putpmsg sys_ni_syscall
189 nospu vfork sys_vfork
190 common ugetrlimit sys_getrlimit compat_sys_getrlimit
-191 common readahead sys_readahead compat_sys_readahead
+191 common readahead sys_readahead compat_sys_ppc_readahead
192 32 mmap2 sys_mmap2 compat_sys_mmap2
-193 32 truncate64 sys_truncate64 compat_sys_truncate64
-194 32 ftruncate64 sys_ftruncate64 compat_sys_ftruncate64
+193 32 truncate64 sys_truncate64 compat_sys_ppc_truncate64
+194 32 ftruncate64 sys_ftruncate64 compat_sys_ppc_ftruncate64
195 32 stat64 sys_stat64
196 32 lstat64 sys_lstat64
197 32 fstat64 sys_fstat64
@@ -288,7 +288,7 @@
230 common io_submit sys_io_submit compat_sys_io_submit
231 common io_cancel sys_io_cancel
232 nospu set_tid_address sys_set_tid_address
-233 common fadvise64 sys_fadvise64 ppc32_fadvise64
+233 common fadvise64 sys_fadvise64 compat_sys_ppc32_fadvise64
234 nospu exit_group sys_exit_group
235 nospu lookup_dcookie sys_lookup_dcookie compat_sys_lookup_dcookie
236 common epoll_create sys_epoll_create
@@ -323,7 +323,7 @@
251 spu utimes sys_utimes
252 common statfs64 sys_statfs64 compat_sys_statfs64
253 common fstatfs64 sys_fstatfs64 compat_sys_fstatfs64
-254 32 fadvise64_64 ppc_fadvise64_64
+254 32 fadvise64_64 sys_ppc_fadvise64_64
254 spu fadvise64_64 sys_ni_syscall
255 common rtas sys_rtas
256 32 sys_debug_setcontext sys_debug_setcontext sys_ni_syscall
@@ -390,7 +390,7 @@
305 common signalfd sys_signalfd compat_sys_signalfd
306 common timerfd_create sys_timerfd_create
307 common eventfd sys_eventfd
-308 common sync_file_range2 sys_sync_file_range2 compat_sys_sync_file_range2
+308 common sync_file_range2 sys_sync_file_range2 compat_sys_ppc_sync_file_range2
309 nospu fallocate sys_fallocate compat_sys_fallocate
310 nospu subpage_prot sys_subpage_prot
311 32 timerfd_settime sys_timerfd_settime32
--
2.34.1
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v4 07/20] powerpc: Adopt SYSCALL_DEFINE for arch-specific syscall handlers
2022-08-24 2:05 ` [PATCH v4 07/20] powerpc: Adopt SYSCALL_DEFINE for arch-specific " Rohan McLure
@ 2022-09-12 10:04 ` Nicholas Piggin
0 siblings, 0 replies; 53+ messages in thread
From: Nicholas Piggin @ 2022-09-12 10:04 UTC (permalink / raw)
To: Rohan McLure, linuxppc-dev
On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
> Arch-specific implementations of syscall handlers are currently used
> over generic implementations for the following reasons:
>
> 1. Semantics unique to powerpc
> 2. Compatibility syscalls require 'argument padding' to comply with
> 64-bit argument convention in ELF32 abi.
> 3. Parameter types or order is different in other architectures.
>
> These syscall handlers have been defined prior to this patch series
> without invoking the SYSCALL_DEFINE or COMPAT_SYSCALL_DEFINE macros with
> custom input and output types. We remove every such direct definition in
> favour of the aforementioned macros.
And what about sys_fallocate, where did that go?
>
> Also update syscalls.tbl in order to refer to the symbol names generated
> by each of these macros. Since ppc64_personality can be called by both
> 64 bit and 32 bit binaries through compatibility, we must generate both
> both compat_sys_ and sys_ symbols for this handler.
Actually I don't have a concern about this patch, I thoughtthere was
something odd going on with ppc64_personality but I misread, it all
looks pretty good.
>
> A number of architectures including arm and powerpc agree on an
> alternative argument order and numbering for most of these arch-specific
> handlers. A future patch series may allow for asm/unistd.h to signal
> through its defines that a generic implementation of these syscall
> handlers with the correct calling convention be omitted, through the
> __ARCH_WANT_COMPAT_SYS_... convention.
I'm on the fence about including this in changelog. arm developers
might not see it, for example. Putting it in include/linux/compat.h
as a comment might be better.
If you keep it in the changelog, maybe preface with something like
aside, side note, in future or something so that's clear it's not part
of the current patch.
Thanks,
Nick
>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> V1 -> V2: All syscall handlers wrapped by this macro.
> V2 -> V3: Move creation of do_ppc64_personality helper to prior patch.
> V3 -> V4: Fix parenthesis alignment. Don't emit sys_*** symbols.
> ---
> arch/powerpc/include/asm/syscalls.h | 10 ++--
> arch/powerpc/kernel/sys_ppc32.c | 45 ++++++++++--------
> arch/powerpc/kernel/syscalls.c | 17 +++++--
> arch/powerpc/kernel/syscalls/syscall.tbl | 22 ++++-----
> .../arch/powerpc/entry/syscalls/syscall.tbl | 22 ++++-----
> 5 files changed, 64 insertions(+), 52 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/syscalls.h b/arch/powerpc/include/asm/syscalls.h
> index 739498c358a1..3e3aff0835a6 100644
> --- a/arch/powerpc/include/asm/syscalls.h
> +++ b/arch/powerpc/include/asm/syscalls.h
> @@ -16,10 +16,10 @@ long sys_mmap(unsigned long addr, size_t len,
> long sys_mmap2(unsigned long addr, size_t len,
> unsigned long prot, unsigned long flags,
> unsigned long fd, unsigned long pgoff);
> -long ppc64_personality(unsigned long personality);
> +long sys_ppc64_personality(unsigned long personality);
> long sys_rtas(struct rtas_args __user *uargs);
> -long ppc_fadvise64_64(int fd, int advice, u32 offset_high, u32 offset_low,
> - u32 len_high, u32 len_low);
> +long sys_ppc_fadvise64_64(int fd, int advice, u32 offset_high, u32 offset_low,
> + u32 len_high, u32 len_low);
>
> #ifdef CONFIG_COMPAT
> unsigned long compat_sys_mmap2(unsigned long addr, size_t len,
> @@ -40,8 +40,8 @@ int compat_sys_truncate64(const char __user *path, u32 reg4,
> int compat_sys_ftruncate64(unsigned int fd, u32 reg4, unsigned long len1,
> unsigned long len2);
>
> -long ppc32_fadvise64(int fd, u32 unused, u32 offset1, u32 offset2,
> - size_t len, int advice);
> +long compat_sys_ppc32_fadvise64(int fd, u32 unused, u32 offset1, u32 offset2,
> + size_t len, int advice);
>
> long compat_sys_sync_file_range2(int fd, unsigned int flags,
> unsigned int offset1, unsigned int offset2,
> diff --git a/arch/powerpc/kernel/sys_ppc32.c b/arch/powerpc/kernel/sys_ppc32.c
> index bc6491ed6454..dd9039671227 100644
> --- a/arch/powerpc/kernel/sys_ppc32.c
> +++ b/arch/powerpc/kernel/sys_ppc32.c
> @@ -59,52 +59,55 @@
> #define merge_64(high, low) ((u64)high << 32) | low
> #endif
>
> -compat_ssize_t compat_sys_pread64(unsigned int fd, char __user *ubuf, compat_size_t count,
> - u32 reg6, u32 pos1, u32 pos2)
> +COMPAT_SYSCALL_DEFINE6(ppc_pread64,
> + unsigned int, fd,
> + char __user *, ubuf, compat_size_t, count,
> + u32, reg6, u32, pos1, u32, pos2)
> {
> return ksys_pread64(fd, ubuf, count, merge_64(pos1, pos2));
> }
>
> -compat_ssize_t compat_sys_pwrite64(unsigned int fd, const char __user *ubuf, compat_size_t count,
> - u32 reg6, u32 pos1, u32 pos2)
> +COMPAT_SYSCALL_DEFINE6(ppc_pwrite64,
> + unsigned int, fd,
> + const char __user *, ubuf, compat_size_t, count,
> + u32, reg6, u32, pos1, u32, pos2)
> {
> return ksys_pwrite64(fd, ubuf, count, merge_64(pos1, pos2));
> }
>
> -compat_ssize_t compat_sys_readahead(int fd, u32 r4, u32 offset1, u32 offset2, u32 count)
> +COMPAT_SYSCALL_DEFINE5(ppc_readahead,
> + int, fd, u32, r4,
> + u32, offset1, u32, offset2, u32, count)
> {
> return ksys_readahead(fd, merge_64(offset1, offset2), count);
> }
>
> -int compat_sys_truncate64(const char __user * path, u32 reg4,
> - unsigned long len1, unsigned long len2)
> +COMPAT_SYSCALL_DEFINE4(ppc_truncate64,
> + const char __user *, path, u32, reg4,
> + unsigned long, len1, unsigned long, len2)
> {
> return ksys_truncate(path, merge_64(len1, len2));
> }
>
> -long compat_sys_fallocate(int fd, int mode, u32 offset1, u32 offset2,
> - u32 len1, u32 len2)
> -{
> - return ksys_fallocate(fd, mode, ((loff_t)offset1 << 32) | offset2,
> - merge_64(len1, len2));
> -}
> -
> -int compat_sys_ftruncate64(unsigned int fd, u32 reg4, unsigned long len1,
> - unsigned long len2)
> +COMPAT_SYSCALL_DEFINE4(ppc_ftruncate64,
> + unsigned int, fd, u32, reg4,
> + unsigned long, len1, unsigned long, len2)
> {
> return ksys_ftruncate(fd, merge_64(len1, len2));
> }
>
> -long ppc32_fadvise64(int fd, u32 unused, u32 offset1, u32 offset2,
> - size_t len, int advice)
> +COMPAT_SYSCALL_DEFINE6(ppc32_fadvise64,
> + int, fd, u32, unused, u32, offset1, u32, offset2,
> + size_t, len, int, advice)
> {
> return ksys_fadvise64_64(fd, merge_64(offset1, offset2), len,
> advice);
> }
>
> -long compat_sys_sync_file_range2(int fd, unsigned int flags,
> - unsigned offset1, unsigned offset2,
> - unsigned nbytes1, unsigned nbytes2)
> +COMPAT_SYSCALL_DEFINE6(ppc_sync_file_range2,
> + int, fd, unsigned int, flags,
> + unsigned int, offset1, unsigned int, offset2,
> + unsigned int, nbytes1, unsigned int, nbytes2)
> {
> loff_t offset = merge_64(offset1, offset2);
> loff_t nbytes = merge_64(nbytes1, nbytes2);
> diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c
> index 32fadf3c2cd3..2d4c62e5bac7 100644
> --- a/arch/powerpc/kernel/syscalls.c
> +++ b/arch/powerpc/kernel/syscalls.c
> @@ -87,14 +87,23 @@ static inline long do_ppc64_personality(unsigned long personality)
> ret = (ret & ~PER_MASK) | PER_LINUX;
> return ret;
> }
> -long ppc64_personality(unsigned long personality)
> +
> +SYSCALL_DEFINE1(ppc64_personality, unsigned long, personality)
> {
> return do_ppc64_personality(personality);
> }
> -#endif
>
> -long ppc_fadvise64_64(int fd, int advice, u32 offset_high, u32 offset_low,
> - u32 len_high, u32 len_low)
> +#ifdef CONFIG_COMPAT
> +COMPAT_SYSCALL_DEFINE1(ppc64_personality, unsigned long, personality)
> +{
> + return do_ppc64_personality(personality);
> +}
> +#endif /* CONFIG_COMPAT */
> +#endif /* CONFIG_PPC64 */
> +
> +SYSCALL_DEFINE6(ppc_fadvise64_64,
> + int, fd, int, advice, u32, offset_high, u32, offset_low,
> + u32, len_high, u32, len_low)
> {
> return ksys_fadvise64_64(fd, (u64)offset_high << 32 | offset_low,
> (u64)len_high << 32 | len_low, advice);
> diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
> index 4cbbb810ae10..b4c970c9c6b1 100644
> --- a/arch/powerpc/kernel/syscalls/syscall.tbl
> +++ b/arch/powerpc/kernel/syscalls/syscall.tbl
> @@ -178,9 +178,9 @@
> 133 common fchdir sys_fchdir
> 134 common bdflush sys_ni_syscall
> 135 common sysfs sys_sysfs
> -136 32 personality sys_personality ppc64_personality
> -136 64 personality ppc64_personality
> -136 spu personality ppc64_personality
> +136 32 personality sys_personality compat_sys_ppc64_personality
> +136 64 personality sys_ppc64_personality
> +136 spu personality sys_ppc64_personality
> 137 common afs_syscall sys_ni_syscall
> 138 common setfsuid sys_setfsuid
> 139 common setfsgid sys_setfsgid
> @@ -228,8 +228,8 @@
> 176 64 rt_sigtimedwait sys_rt_sigtimedwait
> 177 nospu rt_sigqueueinfo sys_rt_sigqueueinfo compat_sys_rt_sigqueueinfo
> 178 nospu rt_sigsuspend sys_rt_sigsuspend compat_sys_rt_sigsuspend
> -179 common pread64 sys_pread64 compat_sys_pread64
> -180 common pwrite64 sys_pwrite64 compat_sys_pwrite64
> +179 common pread64 sys_pread64 compat_sys_ppc_pread64
> +180 common pwrite64 sys_pwrite64 compat_sys_ppc_pwrite64
> 181 common chown sys_chown
> 182 common getcwd sys_getcwd
> 183 common capget sys_capget
> @@ -242,10 +242,10 @@
> 188 common putpmsg sys_ni_syscall
> 189 nospu vfork sys_vfork
> 190 common ugetrlimit sys_getrlimit compat_sys_getrlimit
> -191 common readahead sys_readahead compat_sys_readahead
> +191 common readahead sys_readahead compat_sys_ppc_readahead
> 192 32 mmap2 sys_mmap2 compat_sys_mmap2
> -193 32 truncate64 sys_truncate64 compat_sys_truncate64
> -194 32 ftruncate64 sys_ftruncate64 compat_sys_ftruncate64
> +193 32 truncate64 sys_truncate64 compat_sys_ppc_truncate64
> +194 32 ftruncate64 sys_ftruncate64 compat_sys_ppc_ftruncate64
> 195 32 stat64 sys_stat64
> 196 32 lstat64 sys_lstat64
> 197 32 fstat64 sys_fstat64
> @@ -288,7 +288,7 @@
> 230 common io_submit sys_io_submit compat_sys_io_submit
> 231 common io_cancel sys_io_cancel
> 232 nospu set_tid_address sys_set_tid_address
> -233 common fadvise64 sys_fadvise64 ppc32_fadvise64
> +233 common fadvise64 sys_fadvise64 compat_sys_ppc32_fadvise64
> 234 nospu exit_group sys_exit_group
> 235 nospu lookup_dcookie sys_lookup_dcookie compat_sys_lookup_dcookie
> 236 common epoll_create sys_epoll_create
> @@ -323,7 +323,7 @@
> 251 spu utimes sys_utimes
> 252 common statfs64 sys_statfs64 compat_sys_statfs64
> 253 common fstatfs64 sys_fstatfs64 compat_sys_fstatfs64
> -254 32 fadvise64_64 ppc_fadvise64_64
> +254 32 fadvise64_64 sys_ppc_fadvise64_64
> 254 spu fadvise64_64 sys_ni_syscall
> 255 common rtas sys_rtas
> 256 32 sys_debug_setcontext sys_debug_setcontext sys_ni_syscall
> @@ -390,7 +390,7 @@
> 305 common signalfd sys_signalfd compat_sys_signalfd
> 306 common timerfd_create sys_timerfd_create
> 307 common eventfd sys_eventfd
> -308 common sync_file_range2 sys_sync_file_range2 compat_sys_sync_file_range2
> +308 common sync_file_range2 sys_sync_file_range2 compat_sys_ppc_sync_file_range2
> 309 nospu fallocate sys_fallocate compat_sys_fallocate
> 310 nospu subpage_prot sys_subpage_prot
> 311 32 timerfd_settime sys_timerfd_settime32
> diff --git a/tools/perf/arch/powerpc/entry/syscalls/syscall.tbl b/tools/perf/arch/powerpc/entry/syscalls/syscall.tbl
> index 4cbbb810ae10..b4c970c9c6b1 100644
> --- a/tools/perf/arch/powerpc/entry/syscalls/syscall.tbl
> +++ b/tools/perf/arch/powerpc/entry/syscalls/syscall.tbl
> @@ -178,9 +178,9 @@
> 133 common fchdir sys_fchdir
> 134 common bdflush sys_ni_syscall
> 135 common sysfs sys_sysfs
> -136 32 personality sys_personality ppc64_personality
> -136 64 personality ppc64_personality
> -136 spu personality ppc64_personality
> +136 32 personality sys_personality compat_sys_ppc64_personality
> +136 64 personality sys_ppc64_personality
> +136 spu personality sys_ppc64_personality
> 137 common afs_syscall sys_ni_syscall
> 138 common setfsuid sys_setfsuid
> 139 common setfsgid sys_setfsgid
> @@ -228,8 +228,8 @@
> 176 64 rt_sigtimedwait sys_rt_sigtimedwait
> 177 nospu rt_sigqueueinfo sys_rt_sigqueueinfo compat_sys_rt_sigqueueinfo
> 178 nospu rt_sigsuspend sys_rt_sigsuspend compat_sys_rt_sigsuspend
> -179 common pread64 sys_pread64 compat_sys_pread64
> -180 common pwrite64 sys_pwrite64 compat_sys_pwrite64
> +179 common pread64 sys_pread64 compat_sys_ppc_pread64
> +180 common pwrite64 sys_pwrite64 compat_sys_ppc_pwrite64
> 181 common chown sys_chown
> 182 common getcwd sys_getcwd
> 183 common capget sys_capget
> @@ -242,10 +242,10 @@
> 188 common putpmsg sys_ni_syscall
> 189 nospu vfork sys_vfork
> 190 common ugetrlimit sys_getrlimit compat_sys_getrlimit
> -191 common readahead sys_readahead compat_sys_readahead
> +191 common readahead sys_readahead compat_sys_ppc_readahead
> 192 32 mmap2 sys_mmap2 compat_sys_mmap2
> -193 32 truncate64 sys_truncate64 compat_sys_truncate64
> -194 32 ftruncate64 sys_ftruncate64 compat_sys_ftruncate64
> +193 32 truncate64 sys_truncate64 compat_sys_ppc_truncate64
> +194 32 ftruncate64 sys_ftruncate64 compat_sys_ppc_ftruncate64
> 195 32 stat64 sys_stat64
> 196 32 lstat64 sys_lstat64
> 197 32 fstat64 sys_fstat64
> @@ -288,7 +288,7 @@
> 230 common io_submit sys_io_submit compat_sys_io_submit
> 231 common io_cancel sys_io_cancel
> 232 nospu set_tid_address sys_set_tid_address
> -233 common fadvise64 sys_fadvise64 ppc32_fadvise64
> +233 common fadvise64 sys_fadvise64 compat_sys_ppc32_fadvise64
> 234 nospu exit_group sys_exit_group
> 235 nospu lookup_dcookie sys_lookup_dcookie compat_sys_lookup_dcookie
> 236 common epoll_create sys_epoll_create
> @@ -323,7 +323,7 @@
> 251 spu utimes sys_utimes
> 252 common statfs64 sys_statfs64 compat_sys_statfs64
> 253 common fstatfs64 sys_fstatfs64 compat_sys_fstatfs64
> -254 32 fadvise64_64 ppc_fadvise64_64
> +254 32 fadvise64_64 sys_ppc_fadvise64_64
> 254 spu fadvise64_64 sys_ni_syscall
> 255 common rtas sys_rtas
> 256 32 sys_debug_setcontext sys_debug_setcontext sys_ni_syscall
> @@ -390,7 +390,7 @@
> 305 common signalfd sys_signalfd compat_sys_signalfd
> 306 common timerfd_create sys_timerfd_create
> 307 common eventfd sys_eventfd
> -308 common sync_file_range2 sys_sync_file_range2 compat_sys_sync_file_range2
> +308 common sync_file_range2 sys_sync_file_range2 compat_sys_ppc_sync_file_range2
> 309 nospu fallocate sys_fallocate compat_sys_fallocate
> 310 nospu subpage_prot sys_subpage_prot
> 311 32 timerfd_settime sys_timerfd_settime32
> --
> 2.34.1
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 08/20] powerpc: Include all arch-specific syscall prototypes
2022-08-24 2:05 [PATCH v4 00/20] powerpc: Syscall wrapper and register clearing Rohan McLure
` (6 preceding siblings ...)
2022-08-24 2:05 ` [PATCH v4 07/20] powerpc: Adopt SYSCALL_DEFINE for arch-specific " Rohan McLure
@ 2022-08-24 2:05 ` Rohan McLure
2022-09-12 10:33 ` Nicholas Piggin
2022-08-24 2:05 ` [PATCH v4 09/20] powerpc: Enable compile-time check for syscall handlers Rohan McLure
` (12 subsequent siblings)
20 siblings, 1 reply; 53+ messages in thread
From: Rohan McLure @ 2022-08-24 2:05 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Rohan McLure
Forward declare all syscall handler prototypes where a generic prototype
is not provided in either linux/syscalls.h or linux/compat.h in
asm/syscalls.h. This is required for compile-time type-checking for
syscall handlers, which is implemented later in this series.
32-bit compatibility syscall handlers are expressed in terms of types in
ppc32.h. Expose this header globally.
Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
V1 -> V2: Explicitly include prototypes.
V2 -> V3: Remove extraneous #include <asm/compat.h> and ppc_fallocate
prototype. Rename header.
---
arch/powerpc/include/asm/syscalls.h | 90 +++++++++++++-----
.../ppc32.h => include/asm/syscalls_32.h} | 0
arch/powerpc/kernel/signal_32.c | 2 +-
arch/powerpc/perf/callchain_32.c | 2 +-
4 files changed, 70 insertions(+), 24 deletions(-)
diff --git a/arch/powerpc/include/asm/syscalls.h b/arch/powerpc/include/asm/syscalls.h
index 3e3aff0835a6..91417dee534e 100644
--- a/arch/powerpc/include/asm/syscalls.h
+++ b/arch/powerpc/include/asm/syscalls.h
@@ -8,45 +8,91 @@
#include <linux/types.h>
#include <linux/compat.h>
+#ifdef CONFIG_PPC64
+#include <asm/syscalls_32.h>
+#endif
+#include <asm/unistd.h>
+#include <asm/ucontext.h>
+
struct rtas_args;
+#ifndef CONFIG_ARCH_HAS_SYSCALL_WRAPPER
+
+/*
+ * PowerPC architecture-specific syscalls
+ */
+
+long sys_rtas(struct rtas_args __user *uargs);
+long sys_ni_syscall(void);
+
+#ifdef CONFIG_PPC64
+long sys_ppc64_personality(unsigned long personality);
+#ifdef CONFIG_COMPAT
+long compat_sys_ppc64_personality(unsigned long personality);
+#endif /* CONFIG_COMPAT */
+#endif /* CONFIG_PPC64 */
+
+/* Parameters are reordered for powerpc to avoid padding */
+long sys_ppc_fadvise64_64(int fd, int advice,
+ u32 offset_high, u32 offset_low,
+ u32 len_high, u32 len_low);
+long sys_swapcontext(struct ucontext __user *old_ctx,
+ struct ucontext __user *new_ctx, long ctx_size);
long sys_mmap(unsigned long addr, size_t len,
unsigned long prot, unsigned long flags,
unsigned long fd, off_t offset);
long sys_mmap2(unsigned long addr, size_t len,
unsigned long prot, unsigned long flags,
unsigned long fd, unsigned long pgoff);
-long sys_ppc64_personality(unsigned long personality);
-long sys_rtas(struct rtas_args __user *uargs);
-long sys_ppc_fadvise64_64(int fd, int advice, u32 offset_high, u32 offset_low,
- u32 len_high, u32 len_low);
+long sys_switch_endian(void);
-#ifdef CONFIG_COMPAT
-unsigned long compat_sys_mmap2(unsigned long addr, size_t len,
- unsigned long prot, unsigned long flags,
- unsigned long fd, unsigned long pgoff);
-
-compat_ssize_t compat_sys_pread64(unsigned int fd, char __user *ubuf, compat_size_t count,
- u32 reg6, u32 pos1, u32 pos2);
+#ifdef CONFIG_PPC32
+long sys_sigreturn(void);
+long sys_debug_setcontext(struct ucontext __user *ctx, int ndbg,
+ struct sig_dbg_op __user *dbg);
+#endif
-compat_ssize_t compat_sys_pwrite64(unsigned int fd, const char __user *ubuf, compat_size_t count,
- u32 reg6, u32 pos1, u32 pos2);
+long sys_rt_sigreturn(void);
-compat_ssize_t compat_sys_readahead(int fd, u32 r4, u32 offset1, u32 offset2, u32 count);
+long sys_subpage_prot(unsigned long addr,
+ unsigned long len, u32 __user *map);
-int compat_sys_truncate64(const char __user *path, u32 reg4,
- unsigned long len1, unsigned long len2);
+#ifdef CONFIG_COMPAT
+long compat_sys_swapcontext(struct ucontext32 __user *old_ctx,
+ struct ucontext32 __user *new_ctx,
+ int ctx_size);
+long compat_sys_old_getrlimit(unsigned int resource,
+ struct compat_rlimit __user *rlim);
+long compat_sys_sigreturn(void);
+long compat_sys_rt_sigreturn(void);
-int compat_sys_ftruncate64(unsigned int fd, u32 reg4, unsigned long len1,
- unsigned long len2);
+/* Architecture-specific implementations in sys_ppc32.c */
+long compat_sys_mmap2(unsigned long addr, size_t len,
+ unsigned long prot, unsigned long flags,
+ unsigned long fd, unsigned long pgoff);
+long compat_sys_ppc_pread64(unsigned int fd,
+ char __user *ubuf, compat_size_t count,
+ u32 reg6, u32 pos1, u32 pos2);
+long compat_sys_ppc_pwrite64(unsigned int fd,
+ const char __user *ubuf, compat_size_t count,
+ u32 reg6, u32 pos1, u32 pos2);
+long compat_sys_ppc_readahead(int fd, u32 r4,
+ u32 offset1, u32 offset2, u32 count);
+long compat_sys_ppc_truncate64(const char __user *path, u32 reg4,
+ unsigned long len1, unsigned long len2);
+long compat_sys_ppc_ftruncate64(unsigned int fd, u32 reg4,
+ unsigned long len1, unsigned long len2);
long compat_sys_ppc32_fadvise64(int fd, u32 unused, u32 offset1, u32 offset2,
size_t len, int advice);
+long compat_sys_ppc_sync_file_range2(int fd, unsigned int flags,
+ unsigned int offset1,
+ unsigned int offset2,
+ unsigned int nbytes1,
+ unsigned int nbytes2);
+#endif /* CONFIG_COMPAT */
-long compat_sys_sync_file_range2(int fd, unsigned int flags,
- unsigned int offset1, unsigned int offset2,
- unsigned int nbytes1, unsigned int nbytes2);
-#endif
+#endif /* CONFIG_ARCH_HAS_SYSCALL_WRAPPER */
#endif /* __KERNEL__ */
#endif /* __ASM_POWERPC_SYSCALLS_H */
diff --git a/arch/powerpc/kernel/ppc32.h b/arch/powerpc/include/asm/syscalls_32.h
similarity index 100%
rename from arch/powerpc/kernel/ppc32.h
rename to arch/powerpc/include/asm/syscalls_32.h
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 157a7403e3eb..c114c7f25645 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -43,7 +43,7 @@
#include <asm/tm.h>
#include <asm/asm-prototypes.h>
#ifdef CONFIG_PPC64
-#include "ppc32.h"
+#include <asm/syscalls_32.h>
#include <asm/unistd.h>
#else
#include <asm/ucontext.h>
diff --git a/arch/powerpc/perf/callchain_32.c b/arch/powerpc/perf/callchain_32.c
index b83c47b7947f..ea8cfe3806dc 100644
--- a/arch/powerpc/perf/callchain_32.c
+++ b/arch/powerpc/perf/callchain_32.c
@@ -19,7 +19,7 @@
#include "callchain.h"
#ifdef CONFIG_PPC64
-#include "../kernel/ppc32.h"
+#include <asm/syscalls_32.h>
#else /* CONFIG_PPC64 */
#define __SIGNAL_FRAMESIZE32 __SIGNAL_FRAMESIZE
--
2.34.1
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v4 08/20] powerpc: Include all arch-specific syscall prototypes
2022-08-24 2:05 ` [PATCH v4 08/20] powerpc: Include all arch-specific syscall prototypes Rohan McLure
@ 2022-09-12 10:33 ` Nicholas Piggin
2022-09-13 7:09 ` Rohan McLure
0 siblings, 1 reply; 53+ messages in thread
From: Nicholas Piggin @ 2022-09-12 10:33 UTC (permalink / raw)
To: Rohan McLure, linuxppc-dev
On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
> Forward declare all syscall handler prototypes where a generic prototype
> is not provided in either linux/syscalls.h or linux/compat.h in
> asm/syscalls.h. This is required for compile-time type-checking for
> syscall handlers, which is implemented later in this series.
>
> 32-bit compatibility syscall handlers are expressed in terms of types in
> ppc32.h. Expose this header globally.
>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> V1 -> V2: Explicitly include prototypes.
> V2 -> V3: Remove extraneous #include <asm/compat.h> and ppc_fallocate
> prototype. Rename header.
> ---
> arch/powerpc/include/asm/syscalls.h | 90 +++++++++++++-----
> .../ppc32.h => include/asm/syscalls_32.h} | 0
> arch/powerpc/kernel/signal_32.c | 2 +-
> arch/powerpc/perf/callchain_32.c | 2 +-
> 4 files changed, 70 insertions(+), 24 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/syscalls.h b/arch/powerpc/include/asm/syscalls.h
> index 3e3aff0835a6..91417dee534e 100644
> --- a/arch/powerpc/include/asm/syscalls.h
> +++ b/arch/powerpc/include/asm/syscalls.h
> @@ -8,45 +8,91 @@
> #include <linux/types.h>
> #include <linux/compat.h>
>
> +#ifdef CONFIG_PPC64
> +#include <asm/syscalls_32.h>
> +#endif
> +#include <asm/unistd.h>
> +#include <asm/ucontext.h>
> +
> struct rtas_args;
>
> +#ifndef CONFIG_ARCH_HAS_SYSCALL_WRAPPER
Do you need this ifdef?
> +
> +/*
> + * PowerPC architecture-specific syscalls
> + */
> +
> +long sys_rtas(struct rtas_args __user *uargs);
> +long sys_ni_syscall(void);
> +
> +#ifdef CONFIG_PPC64
> +long sys_ppc64_personality(unsigned long personality);
> +#ifdef CONFIG_COMPAT
> +long compat_sys_ppc64_personality(unsigned long personality);
> +#endif /* CONFIG_COMPAT */
> +#endif /* CONFIG_PPC64 */
> +
> +/* Parameters are reordered for powerpc to avoid padding */
> +long sys_ppc_fadvise64_64(int fd, int advice,
> + u32 offset_high, u32 offset_low,
> + u32 len_high, u32 len_low);
Should this be under PPC32 since you're adding the ifdefs?
Because you added a new comment here... This is register padding
to do with something, even/odd pair calling convention? I can't
remember the details would you be able to expand the comment a bit
because I'm sure I'll forget it again too.
Thanks,
Nick
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 08/20] powerpc: Include all arch-specific syscall prototypes
2022-09-12 10:33 ` Nicholas Piggin
@ 2022-09-13 7:09 ` Rohan McLure
0 siblings, 0 replies; 53+ messages in thread
From: Rohan McLure @ 2022-09-13 7:09 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linuxppc-dev
> On 12 Sep 2022, at 8:33 pm, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
>> Forward declare all syscall handler prototypes where a generic prototype
>> is not provided in either linux/syscalls.h or linux/compat.h in
>> asm/syscalls.h. This is required for compile-time type-checking for
>> syscall handlers, which is implemented later in this series.
>>
>> 32-bit compatibility syscall handlers are expressed in terms of types in
>> ppc32.h. Expose this header globally.
>>
>> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
>> ---
>> V1 -> V2: Explicitly include prototypes.
>> V2 -> V3: Remove extraneous #include <asm/compat.h> and ppc_fallocate
>> prototype. Rename header.
>> ---
>> arch/powerpc/include/asm/syscalls.h | 90 +++++++++++++-----
>> .../ppc32.h => include/asm/syscalls_32.h} | 0
>> arch/powerpc/kernel/signal_32.c | 2 +-
>> arch/powerpc/perf/callchain_32.c | 2 +-
>> 4 files changed, 70 insertions(+), 24 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/syscalls.h b/arch/powerpc/include/asm/syscalls.h
>> index 3e3aff0835a6..91417dee534e 100644
>> --- a/arch/powerpc/include/asm/syscalls.h
>> +++ b/arch/powerpc/include/asm/syscalls.h
>> @@ -8,45 +8,91 @@
>> #include <linux/types.h>
>> #include <linux/compat.h>
>>
>> +#ifdef CONFIG_PPC64
>> +#include <asm/syscalls_32.h>
>> +#endif
>> +#include <asm/unistd.h>
>> +#include <asm/ucontext.h>
>> +
>> struct rtas_args;
>>
>> +#ifndef CONFIG_ARCH_HAS_SYSCALL_WRAPPER
>
> Do you need this ifdef?
Good spot. I’ll introduce that with syscall wrappers.
>> +
>> +/*
>> + * PowerPC architecture-specific syscalls
>> + */
>> +
>> +long sys_rtas(struct rtas_args __user *uargs);
>> +long sys_ni_syscall(void);
>> +
>> +#ifdef CONFIG_PPC64
>> +long sys_ppc64_personality(unsigned long personality);
>> +#ifdef CONFIG_COMPAT
>> +long compat_sys_ppc64_personality(unsigned long personality);
>> +#endif /* CONFIG_COMPAT */
>> +#endif /* CONFIG_PPC64 */
>> +
>> +/* Parameters are reordered for powerpc to avoid padding */
>> +long sys_ppc_fadvise64_64(int fd, int advice,
>> + u32 offset_high, u32 offset_low,
>> + u32 len_high, u32 len_low);
>
> Should this be under PPC32 since you're adding the ifdefs?
>
> Because you added a new comment here... This is register padding
> to do with something, even/odd pair calling convention? I can't
> remember the details would you be able to expand the comment a bit
> because I'm sure I'll forget it again too.
Cool. I’ll neaten this up and change that comment.
>
> Thanks,
> Nick
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 09/20] powerpc: Enable compile-time check for syscall handlers
2022-08-24 2:05 [PATCH v4 00/20] powerpc: Syscall wrapper and register clearing Rohan McLure
` (7 preceding siblings ...)
2022-08-24 2:05 ` [PATCH v4 08/20] powerpc: Include all arch-specific syscall prototypes Rohan McLure
@ 2022-08-24 2:05 ` Rohan McLure
2022-09-12 10:42 ` Nicholas Piggin
2022-08-24 2:05 ` [PATCH v4 10/20] powerpc: Use common syscall handler type Rohan McLure
` (11 subsequent siblings)
20 siblings, 1 reply; 53+ messages in thread
From: Rohan McLure @ 2022-08-24 2:05 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Rohan McLure, Arnd Bergmann
The table of syscall handlers and registered compatibility syscall
handlers has in past been produced using assembly, with function
references resolved at link time. This moves link-time errors to
compile-time, by rewriting systbl.S in C, and including the
linux/syscalls.h, linux/compat.h and asm/syscalls.h headers for
prototypes.
Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
V1 -> V2: New patch.
---
arch/powerpc/kernel/{systbl.S => systbl.c} | 27 ++++++++++----------
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/arch/powerpc/kernel/systbl.S b/arch/powerpc/kernel/systbl.c
similarity index 59%
rename from arch/powerpc/kernel/systbl.S
rename to arch/powerpc/kernel/systbl.c
index cb3358886203..99ffdfef6b9c 100644
--- a/arch/powerpc/kernel/systbl.S
+++ b/arch/powerpc/kernel/systbl.c
@@ -10,31 +10,32 @@
* PPC64 updates by Dave Engebretsen (engebret@us.ibm.com)
*/
-#include <asm/ppc_asm.h>
+#include <linux/syscalls.h>
+#include <linux/compat.h>
+#include <asm/unistd.h>
+#include <asm/syscalls.h>
-.section .rodata,"a"
+#define __SYSCALL_WITH_COMPAT(nr, entry, compat) __SYSCALL(nr, entry)
-#ifdef CONFIG_PPC64
- .p2align 3
-#define __SYSCALL(nr, entry) .8byte entry
+#ifdef CONFIG_ARCH_HAS_SYSCALL_WRAPPER
+#define __SYSCALL(nr, entry) [nr] = __powerpc_##entry,
+#define __powerpc_sys_ni_syscall sys_ni_syscall
#else
-#define __SYSCALL(nr, entry) .long entry
+#define __SYSCALL(nr, entry) [nr] = entry,
#endif
-#define __SYSCALL_WITH_COMPAT(nr, native, compat) __SYSCALL(nr, native)
-.globl sys_call_table
-sys_call_table:
+void *sys_call_table[] = {
#ifdef CONFIG_PPC64
#include <asm/syscall_table_64.h>
#else
#include <asm/syscall_table_32.h>
#endif
+};
#ifdef CONFIG_COMPAT
#undef __SYSCALL_WITH_COMPAT
#define __SYSCALL_WITH_COMPAT(nr, native, compat) __SYSCALL(nr, compat)
-.globl compat_sys_call_table
-compat_sys_call_table:
-#define compat_sys_sigsuspend sys_sigsuspend
+void *compat_sys_call_table[] = {
#include <asm/syscall_table_32.h>
-#endif
+};
+#endif /* CONFIG_COMPAT */
--
2.34.1
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v4 09/20] powerpc: Enable compile-time check for syscall handlers
2022-08-24 2:05 ` [PATCH v4 09/20] powerpc: Enable compile-time check for syscall handlers Rohan McLure
@ 2022-09-12 10:42 ` Nicholas Piggin
2022-09-13 2:29 ` Michael Ellerman
0 siblings, 1 reply; 53+ messages in thread
From: Nicholas Piggin @ 2022-09-12 10:42 UTC (permalink / raw)
To: Rohan McLure, linuxppc-dev; +Cc: Arnd Bergmann
On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
> The table of syscall handlers and registered compatibility syscall
> handlers has in past been produced using assembly, with function
> references resolved at link time. This moves link-time errors to
> compile-time, by rewriting systbl.S in C, and including the
> linux/syscalls.h, linux/compat.h and asm/syscalls.h headers for
> prototypes.
Well this is pretty cool.
Unrelated, but since commit ab66dcc76d6a, is
arch/powerpc/kernel/systbl_chk.sh unused in the tree?
>
> Reported-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> V1 -> V2: New patch.
> ---
> arch/powerpc/kernel/{systbl.S => systbl.c} | 27 ++++++++++----------
> 1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/arch/powerpc/kernel/systbl.S b/arch/powerpc/kernel/systbl.c
> similarity index 59%
> rename from arch/powerpc/kernel/systbl.S
> rename to arch/powerpc/kernel/systbl.c
> index cb3358886203..99ffdfef6b9c 100644
> --- a/arch/powerpc/kernel/systbl.S
> +++ b/arch/powerpc/kernel/systbl.c
> @@ -10,31 +10,32 @@
> * PPC64 updates by Dave Engebretsen (engebret@us.ibm.com)
> */
>
> -#include <asm/ppc_asm.h>
> +#include <linux/syscalls.h>
> +#include <linux/compat.h>
> +#include <asm/unistd.h>
> +#include <asm/syscalls.h>
>
> -.section .rodata,"a"
> +#define __SYSCALL_WITH_COMPAT(nr, entry, compat) __SYSCALL(nr, entry)
>
> -#ifdef CONFIG_PPC64
> - .p2align 3
> -#define __SYSCALL(nr, entry) .8byte entry
> +#ifdef CONFIG_ARCH_HAS_SYSCALL_WRAPPER
> +#define __SYSCALL(nr, entry) [nr] = __powerpc_##entry,
> +#define __powerpc_sys_ni_syscall sys_ni_syscall
> #else
> -#define __SYSCALL(nr, entry) .long entry
> +#define __SYSCALL(nr, entry) [nr] = entry,
> #endif
>
> -#define __SYSCALL_WITH_COMPAT(nr, native, compat) __SYSCALL(nr, native)
> -.globl sys_call_table
> -sys_call_table:
> +void *sys_call_table[] = {
Humour me, the asm version had this in rodata, does this change that or
does it somehow get put back in there? Should this be const?
Otherwise,
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> #ifdef CONFIG_PPC64
> #include <asm/syscall_table_64.h>
> #else
> #include <asm/syscall_table_32.h>
> #endif
> +};
>
> #ifdef CONFIG_COMPAT
> #undef __SYSCALL_WITH_COMPAT
> #define __SYSCALL_WITH_COMPAT(nr, native, compat) __SYSCALL(nr, compat)
> -.globl compat_sys_call_table
> -compat_sys_call_table:
> -#define compat_sys_sigsuspend sys_sigsuspend
> +void *compat_sys_call_table[] = {
> #include <asm/syscall_table_32.h>
> -#endif
> +};
> +#endif /* CONFIG_COMPAT */
> --
> 2.34.1
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 09/20] powerpc: Enable compile-time check for syscall handlers
2022-09-12 10:42 ` Nicholas Piggin
@ 2022-09-13 2:29 ` Michael Ellerman
0 siblings, 0 replies; 53+ messages in thread
From: Michael Ellerman @ 2022-09-13 2:29 UTC (permalink / raw)
To: Nicholas Piggin, Rohan McLure, linuxppc-dev; +Cc: Arnd Bergmann
"Nicholas Piggin" <npiggin@gmail.com> writes:
> On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
...
>> diff --git a/arch/powerpc/kernel/systbl.S b/arch/powerpc/kernel/systbl.c
>> similarity index 59%
>> rename from arch/powerpc/kernel/systbl.S
>> rename to arch/powerpc/kernel/systbl.c
>> index cb3358886203..99ffdfef6b9c 100644
>> --- a/arch/powerpc/kernel/systbl.S
>> +++ b/arch/powerpc/kernel/systbl.c
>> @@ -10,31 +10,32 @@
>> * PPC64 updates by Dave Engebretsen (engebret@us.ibm.com)
>> */
>>
>> -#include <asm/ppc_asm.h>
>> +#include <linux/syscalls.h>
>> +#include <linux/compat.h>
>> +#include <asm/unistd.h>
>> +#include <asm/syscalls.h>
>>
>> -.section .rodata,"a"
>> +#define __SYSCALL_WITH_COMPAT(nr, entry, compat) __SYSCALL(nr, entry)
>>
>> -#ifdef CONFIG_PPC64
>> - .p2align 3
>> -#define __SYSCALL(nr, entry) .8byte entry
>> +#ifdef CONFIG_ARCH_HAS_SYSCALL_WRAPPER
>> +#define __SYSCALL(nr, entry) [nr] = __powerpc_##entry,
>> +#define __powerpc_sys_ni_syscall sys_ni_syscall
>> #else
>> -#define __SYSCALL(nr, entry) .long entry
>> +#define __SYSCALL(nr, entry) [nr] = entry,
>> #endif
>>
>> -#define __SYSCALL_WITH_COMPAT(nr, native, compat) __SYSCALL(nr, native)
>> -.globl sys_call_table
>> -sys_call_table:
>> +void *sys_call_table[] = {
>
> Humour me, the asm version had this in rodata, does this change that or
> does it somehow get put back in there? Should this be const?
Even with const I still see it not landing in rodata for some reason.
$ grep -e start_rodata -e end_rodata -e sys_call_table .build/System.map
c000000000f90000 D __start_rodata
c0000000012b0000 R __end_rodata
c0000000027d51b0 D sys_call_table
vs before the series:
c000000000f80000 D __start_rodata
c000000000f805f0 D sys_call_table
c0000000012a0000 R __end_rodata
cheers
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 10/20] powerpc: Use common syscall handler type
2022-08-24 2:05 [PATCH v4 00/20] powerpc: Syscall wrapper and register clearing Rohan McLure
` (8 preceding siblings ...)
2022-08-24 2:05 ` [PATCH v4 09/20] powerpc: Enable compile-time check for syscall handlers Rohan McLure
@ 2022-08-24 2:05 ` Rohan McLure
2022-09-12 10:56 ` Nicholas Piggin
2022-08-24 2:05 ` [PATCH v4 11/20] powerpc: Add ZEROIZE_GPRS macros for register clears Rohan McLure
` (10 subsequent siblings)
20 siblings, 1 reply; 53+ messages in thread
From: Rohan McLure @ 2022-08-24 2:05 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Rohan McLure
Cause syscall handlers to be typed as follows when called indirectly
throughout the kernel.
typedef long (*syscall_fn)(unsigned long, unsigned long, unsigned long,
unsigned long, unsigned long, unsigned long);
Since both 32 and 64-bit abis allow for at least the first six
machine-word length parameters to a function to be passed by registers,
even handlers which admit fewer than six parameters may be viewed as
having the above type.
Fixup comparisons in VDSO to avoid pointer-integer comparison. Introduce
explicit cast on systems with SPUs.
Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
V1 -> V2: New patch.
V2 -> V3: Remove unnecessary cast from const syscall_fn to syscall_fn
---
arch/powerpc/include/asm/syscall.h | 7 +++++--
arch/powerpc/include/asm/syscalls.h | 1 +
arch/powerpc/kernel/systbl.c | 6 +++---
arch/powerpc/kernel/vdso.c | 4 ++--
arch/powerpc/platforms/cell/spu_callbacks.c | 6 +++---
5 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/include/asm/syscall.h b/arch/powerpc/include/asm/syscall.h
index 25fc8ad9a27a..d2a8dfd5de33 100644
--- a/arch/powerpc/include/asm/syscall.h
+++ b/arch/powerpc/include/asm/syscall.h
@@ -14,9 +14,12 @@
#include <linux/sched.h>
#include <linux/thread_info.h>
+typedef long (*syscall_fn)(unsigned long, unsigned long, unsigned long,
+ unsigned long, unsigned long, unsigned long);
+
/* ftrace syscalls requires exporting the sys_call_table */
-extern const unsigned long sys_call_table[];
-extern const unsigned long compat_sys_call_table[];
+extern const syscall_fn sys_call_table[];
+extern const syscall_fn compat_sys_call_table[];
static inline int syscall_get_nr(struct task_struct *task, struct pt_regs *regs)
{
diff --git a/arch/powerpc/include/asm/syscalls.h b/arch/powerpc/include/asm/syscalls.h
index 91417dee534e..e979b7593d2b 100644
--- a/arch/powerpc/include/asm/syscalls.h
+++ b/arch/powerpc/include/asm/syscalls.h
@@ -8,6 +8,7 @@
#include <linux/types.h>
#include <linux/compat.h>
+#include <asm/syscall.h>
#ifdef CONFIG_PPC64
#include <asm/syscalls_32.h>
#endif
diff --git a/arch/powerpc/kernel/systbl.c b/arch/powerpc/kernel/systbl.c
index 99ffdfef6b9c..b88a9c2a1f50 100644
--- a/arch/powerpc/kernel/systbl.c
+++ b/arch/powerpc/kernel/systbl.c
@@ -21,10 +21,10 @@
#define __SYSCALL(nr, entry) [nr] = __powerpc_##entry,
#define __powerpc_sys_ni_syscall sys_ni_syscall
#else
-#define __SYSCALL(nr, entry) [nr] = entry,
+#define __SYSCALL(nr, entry) [nr] = (void *) entry,
#endif
-void *sys_call_table[] = {
+const syscall_fn sys_call_table[] = {
#ifdef CONFIG_PPC64
#include <asm/syscall_table_64.h>
#else
@@ -35,7 +35,7 @@ void *sys_call_table[] = {
#ifdef CONFIG_COMPAT
#undef __SYSCALL_WITH_COMPAT
#define __SYSCALL_WITH_COMPAT(nr, native, compat) __SYSCALL(nr, compat)
-void *compat_sys_call_table[] = {
+const syscall_fn compat_sys_call_table[] = {
#include <asm/syscall_table_32.h>
};
#endif /* CONFIG_COMPAT */
diff --git a/arch/powerpc/kernel/vdso.c b/arch/powerpc/kernel/vdso.c
index 0da287544054..080c9e7de0c0 100644
--- a/arch/powerpc/kernel/vdso.c
+++ b/arch/powerpc/kernel/vdso.c
@@ -313,10 +313,10 @@ static void __init vdso_setup_syscall_map(void)
unsigned int i;
for (i = 0; i < NR_syscalls; i++) {
- if (sys_call_table[i] != (unsigned long)&sys_ni_syscall)
+ if (sys_call_table[i] != (void *)&sys_ni_syscall)
vdso_data->syscall_map[i >> 5] |= 0x80000000UL >> (i & 0x1f);
if (IS_ENABLED(CONFIG_COMPAT) &&
- compat_sys_call_table[i] != (unsigned long)&sys_ni_syscall)
+ compat_sys_call_table[i] != (void *)&sys_ni_syscall)
vdso_data->compat_syscall_map[i >> 5] |= 0x80000000UL >> (i & 0x1f);
}
}
diff --git a/arch/powerpc/platforms/cell/spu_callbacks.c b/arch/powerpc/platforms/cell/spu_callbacks.c
index fe0d8797a00a..e780c14c5733 100644
--- a/arch/powerpc/platforms/cell/spu_callbacks.c
+++ b/arch/powerpc/platforms/cell/spu_callbacks.c
@@ -34,15 +34,15 @@
* mbind, mq_open, ipc, ...
*/
-static void *spu_syscall_table[] = {
+static const syscall_fn spu_syscall_table[] = {
#define __SYSCALL_WITH_COMPAT(nr, entry, compat) __SYSCALL(nr, entry)
-#define __SYSCALL(nr, entry) [nr] = entry,
+#define __SYSCALL(nr, entry) [nr] = (void *) entry,
#include <asm/syscall_table_spu.h>
};
long spu_sys_callback(struct spu_syscall_block *s)
{
- long (*syscall)(u64 a1, u64 a2, u64 a3, u64 a4, u64 a5, u64 a6);
+ syscall_fn syscall;
if (s->nr_ret >= ARRAY_SIZE(spu_syscall_table)) {
pr_debug("%s: invalid syscall #%lld", __func__, s->nr_ret);
--
2.34.1
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v4 10/20] powerpc: Use common syscall handler type
2022-08-24 2:05 ` [PATCH v4 10/20] powerpc: Use common syscall handler type Rohan McLure
@ 2022-09-12 10:56 ` Nicholas Piggin
2022-09-15 5:45 ` Rohan McLure
0 siblings, 1 reply; 53+ messages in thread
From: Nicholas Piggin @ 2022-09-12 10:56 UTC (permalink / raw)
To: Rohan McLure, linuxppc-dev
On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
> Cause syscall handlers to be typed as follows when called indirectly
> throughout the kernel.
>
> typedef long (*syscall_fn)(unsigned long, unsigned long, unsigned long,
> unsigned long, unsigned long, unsigned long);
The point is... better type checking?
>
> Since both 32 and 64-bit abis allow for at least the first six
> machine-word length parameters to a function to be passed by registers,
> even handlers which admit fewer than six parameters may be viewed as
> having the above type.
>
> Fixup comparisons in VDSO to avoid pointer-integer comparison. Introduce
> explicit cast on systems with SPUs.
>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> V1 -> V2: New patch.
> V2 -> V3: Remove unnecessary cast from const syscall_fn to syscall_fn
> ---
> arch/powerpc/include/asm/syscall.h | 7 +++++--
> arch/powerpc/include/asm/syscalls.h | 1 +
> arch/powerpc/kernel/systbl.c | 6 +++---
> arch/powerpc/kernel/vdso.c | 4 ++--
> arch/powerpc/platforms/cell/spu_callbacks.c | 6 +++---
> 5 files changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/syscall.h b/arch/powerpc/include/asm/syscall.h
> index 25fc8ad9a27a..d2a8dfd5de33 100644
> --- a/arch/powerpc/include/asm/syscall.h
> +++ b/arch/powerpc/include/asm/syscall.h
> @@ -14,9 +14,12 @@
> #include <linux/sched.h>
> #include <linux/thread_info.h>
>
> +typedef long (*syscall_fn)(unsigned long, unsigned long, unsigned long,
> + unsigned long, unsigned long, unsigned long);
> +
> /* ftrace syscalls requires exporting the sys_call_table */
> -extern const unsigned long sys_call_table[];
> -extern const unsigned long compat_sys_call_table[];
> +extern const syscall_fn sys_call_table[];
> +extern const syscall_fn compat_sys_call_table[];
Ah you constify it in this patch. I think the previous patch should have
kept the const, and it should keep the unsigned long type rather than
use void *. Either that or do this patch first.
> static inline int syscall_get_nr(struct task_struct *task, struct pt_regs *regs)
> {
> diff --git a/arch/powerpc/include/asm/syscalls.h b/arch/powerpc/include/asm/syscalls.h
> index 91417dee534e..e979b7593d2b 100644
> --- a/arch/powerpc/include/asm/syscalls.h
> +++ b/arch/powerpc/include/asm/syscalls.h
> @@ -8,6 +8,7 @@
> #include <linux/types.h>
> #include <linux/compat.h>
>
> +#include <asm/syscall.h>
> #ifdef CONFIG_PPC64
> #include <asm/syscalls_32.h>
> #endif
Is this necessary or should be in another patch?
> diff --git a/arch/powerpc/kernel/systbl.c b/arch/powerpc/kernel/systbl.c
> index 99ffdfef6b9c..b88a9c2a1f50 100644
> --- a/arch/powerpc/kernel/systbl.c
> +++ b/arch/powerpc/kernel/systbl.c
> @@ -21,10 +21,10 @@
> #define __SYSCALL(nr, entry) [nr] = __powerpc_##entry,
> #define __powerpc_sys_ni_syscall sys_ni_syscall
> #else
> -#define __SYSCALL(nr, entry) [nr] = entry,
> +#define __SYSCALL(nr, entry) [nr] = (void *) entry,
> #endif
Also perhaps this should have been in the prior pach and this pach
should change the cast from void to syscall_fn ?
>
> -void *sys_call_table[] = {
> +const syscall_fn sys_call_table[] = {
> #ifdef CONFIG_PPC64
> #include <asm/syscall_table_64.h>
> #else
> @@ -35,7 +35,7 @@ void *sys_call_table[] = {
> #ifdef CONFIG_COMPAT
> #undef __SYSCALL_WITH_COMPAT
> #define __SYSCALL_WITH_COMPAT(nr, native, compat) __SYSCALL(nr, compat)
> -void *compat_sys_call_table[] = {
> +const syscall_fn compat_sys_call_table[] = {
> #include <asm/syscall_table_32.h>
> };
> #endif /* CONFIG_COMPAT */
> diff --git a/arch/powerpc/kernel/vdso.c b/arch/powerpc/kernel/vdso.c
> index 0da287544054..080c9e7de0c0 100644
> --- a/arch/powerpc/kernel/vdso.c
> +++ b/arch/powerpc/kernel/vdso.c
> @@ -313,10 +313,10 @@ static void __init vdso_setup_syscall_map(void)
> unsigned int i;
>
> for (i = 0; i < NR_syscalls; i++) {
> - if (sys_call_table[i] != (unsigned long)&sys_ni_syscall)
> + if (sys_call_table[i] != (void *)&sys_ni_syscall)
> vdso_data->syscall_map[i >> 5] |= 0x80000000UL >> (i & 0x1f);
> if (IS_ENABLED(CONFIG_COMPAT) &&
> - compat_sys_call_table[i] != (unsigned long)&sys_ni_syscall)
> + compat_sys_call_table[i] != (void *)&sys_ni_syscall)
Also these.
Thanks,
Nick
> vdso_data->compat_syscall_map[i >> 5] |= 0x80000000UL >> (i & 0x1f);
> }
> }
> diff --git a/arch/powerpc/platforms/cell/spu_callbacks.c b/arch/powerpc/platforms/cell/spu_callbacks.c
> index fe0d8797a00a..e780c14c5733 100644
> --- a/arch/powerpc/platforms/cell/spu_callbacks.c
> +++ b/arch/powerpc/platforms/cell/spu_callbacks.c
> @@ -34,15 +34,15 @@
> * mbind, mq_open, ipc, ...
> */
>
> -static void *spu_syscall_table[] = {
> +static const syscall_fn spu_syscall_table[] = {
> #define __SYSCALL_WITH_COMPAT(nr, entry, compat) __SYSCALL(nr, entry)
> -#define __SYSCALL(nr, entry) [nr] = entry,
> +#define __SYSCALL(nr, entry) [nr] = (void *) entry,
> #include <asm/syscall_table_spu.h>
> };
>
> long spu_sys_callback(struct spu_syscall_block *s)
> {
> - long (*syscall)(u64 a1, u64 a2, u64 a3, u64 a4, u64 a5, u64 a6);
> + syscall_fn syscall;
>
> if (s->nr_ret >= ARRAY_SIZE(spu_syscall_table)) {
> pr_debug("%s: invalid syscall #%lld", __func__, s->nr_ret);
> --
> 2.34.1
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 10/20] powerpc: Use common syscall handler type
2022-09-12 10:56 ` Nicholas Piggin
@ 2022-09-15 5:45 ` Rohan McLure
2022-09-16 1:02 ` Nicholas Piggin
0 siblings, 1 reply; 53+ messages in thread
From: Rohan McLure @ 2022-09-15 5:45 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linuxppc-dev
> On 12 Sep 2022, at 8:56 pm, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
>> Cause syscall handlers to be typed as follows when called indirectly
>> throughout the kernel.
>>
>> typedef long (*syscall_fn)(unsigned long, unsigned long, unsigned long,
>> unsigned long, unsigned long, unsigned long);
>
> The point is... better type checking?
>
>>
>> Since both 32 and 64-bit abis allow for at least the first six
>> machine-word length parameters to a function to be passed by registers,
>> even handlers which admit fewer than six parameters may be viewed as
>> having the above type.
>>
>> Fixup comparisons in VDSO to avoid pointer-integer comparison. Introduce
>> explicit cast on systems with SPUs.
>>
>> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
>> ---
>> V1 -> V2: New patch.
>> V2 -> V3: Remove unnecessary cast from const syscall_fn to syscall_fn
>> ---
>> arch/powerpc/include/asm/syscall.h | 7 +++++--
>> arch/powerpc/include/asm/syscalls.h | 1 +
>> arch/powerpc/kernel/systbl.c | 6 +++---
>> arch/powerpc/kernel/vdso.c | 4 ++--
>> arch/powerpc/platforms/cell/spu_callbacks.c | 6 +++---
>> 5 files changed, 14 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/syscall.h b/arch/powerpc/include/asm/syscall.h
>> index 25fc8ad9a27a..d2a8dfd5de33 100644
>> --- a/arch/powerpc/include/asm/syscall.h
>> +++ b/arch/powerpc/include/asm/syscall.h
>> @@ -14,9 +14,12 @@
>> #include <linux/sched.h>
>> #include <linux/thread_info.h>
>>
>> +typedef long (*syscall_fn)(unsigned long, unsigned long, unsigned long,
>> + unsigned long, unsigned long, unsigned long);
>> +
>> /* ftrace syscalls requires exporting the sys_call_table */
>> -extern const unsigned long sys_call_table[];
>> -extern const unsigned long compat_sys_call_table[];
>> +extern const syscall_fn sys_call_table[];
>> +extern const syscall_fn compat_sys_call_table[];
>
> Ah you constify it in this patch. I think the previous patch should have
> kept the const, and it should keep the unsigned long type rather than
> use void *. Either that or do this patch first.
>
>> static inline int syscall_get_nr(struct task_struct *task, struct pt_regs *regs)
>> {
>> diff --git a/arch/powerpc/include/asm/syscalls.h b/arch/powerpc/include/asm/syscalls.h
>> index 91417dee534e..e979b7593d2b 100644
>> --- a/arch/powerpc/include/asm/syscalls.h
>> +++ b/arch/powerpc/include/asm/syscalls.h
>> @@ -8,6 +8,7 @@
>> #include <linux/types.h>
>> #include <linux/compat.h>
>>
>> +#include <asm/syscall.h>
>> #ifdef CONFIG_PPC64
>> #include <asm/syscalls_32.h>
>> #endif
>
> Is this necessary or should be in another patch?
Good spot. This belongs in the patch that produces systbl.c.
>
>> diff --git a/arch/powerpc/kernel/systbl.c b/arch/powerpc/kernel/systbl.c
>> index 99ffdfef6b9c..b88a9c2a1f50 100644
>> --- a/arch/powerpc/kernel/systbl.c
>> +++ b/arch/powerpc/kernel/systbl.c
>> @@ -21,10 +21,10 @@
>> #define __SYSCALL(nr, entry) [nr] = __powerpc_##entry,
>> #define __powerpc_sys_ni_syscall sys_ni_syscall
>> #else
>> -#define __SYSCALL(nr, entry) [nr] = entry,
>> +#define __SYSCALL(nr, entry) [nr] = (void *) entry,
>> #endif
>
> Also perhaps this should have been in the prior pach and this pach
> should change the cast from void to syscall_fn ?
This cast to (void *) kicks in when casting functions with six or fewer
parameters to six-parameter type accepting and returning u64. Sadly I can’t
find a way to avoid -Wcast-function-type even with (__force syscall_fn) short
of an ugly casti to void* here. Any suggestions?
>
>>
>> -void *sys_call_table[] = {
>> +const syscall_fn sys_call_table[] = {
>> #ifdef CONFIG_PPC64
>> #include <asm/syscall_table_64.h>
>> #else
>> @@ -35,7 +35,7 @@ void *sys_call_table[] = {
>> #ifdef CONFIG_COMPAT
>> #undef __SYSCALL_WITH_COMPAT
>> #define __SYSCALL_WITH_COMPAT(nr, native, compat) __SYSCALL(nr, compat)
>> -void *compat_sys_call_table[] = {
>> +const syscall_fn compat_sys_call_table[] = {
>> #include <asm/syscall_table_32.h>
>> };
>> #endif /* CONFIG_COMPAT */
>> diff --git a/arch/powerpc/kernel/vdso.c b/arch/powerpc/kernel/vdso.c
>> index 0da287544054..080c9e7de0c0 100644
>> --- a/arch/powerpc/kernel/vdso.c
>> +++ b/arch/powerpc/kernel/vdso.c
>> @@ -313,10 +313,10 @@ static void __init vdso_setup_syscall_map(void)
>> unsigned int i;
>>
>> for (i = 0; i < NR_syscalls; i++) {
>> - if (sys_call_table[i] != (unsigned long)&sys_ni_syscall)
>> + if (sys_call_table[i] != (void *)&sys_ni_syscall)
>> vdso_data->syscall_map[i >> 5] |= 0x80000000UL >> (i & 0x1f);
>> if (IS_ENABLED(CONFIG_COMPAT) &&
>> - compat_sys_call_table[i] != (unsigned long)&sys_ni_syscall)
>> + compat_sys_call_table[i] != (void *)&sys_ni_syscall)
>
> Also these.
>
> Thanks,
> Nick
>
>> vdso_data->compat_syscall_map[i >> 5] |= 0x80000000UL >> (i & 0x1f);
>> }
>> }
>> diff --git a/arch/powerpc/platforms/cell/spu_callbacks.c b/arch/powerpc/platforms/cell/spu_callbacks.c
>> index fe0d8797a00a..e780c14c5733 100644
>> --- a/arch/powerpc/platforms/cell/spu_callbacks.c
>> +++ b/arch/powerpc/platforms/cell/spu_callbacks.c
>> @@ -34,15 +34,15 @@
>> * mbind, mq_open, ipc, ...
>> */
>>
>> -static void *spu_syscall_table[] = {
>> +static const syscall_fn spu_syscall_table[] = {
>> #define __SYSCALL_WITH_COMPAT(nr, entry, compat) __SYSCALL(nr, entry)
>> -#define __SYSCALL(nr, entry) [nr] = entry,
>> +#define __SYSCALL(nr, entry) [nr] = (void *) entry,
>> #include <asm/syscall_table_spu.h>
>> };
>>
>> long spu_sys_callback(struct spu_syscall_block *s)
>> {
>> - long (*syscall)(u64 a1, u64 a2, u64 a3, u64 a4, u64 a5, u64 a6);
>> + syscall_fn syscall;
>>
>> if (s->nr_ret >= ARRAY_SIZE(spu_syscall_table)) {
>> pr_debug("%s: invalid syscall #%lld", __func__, s->nr_ret);
>> --
>> 2.34.1
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 10/20] powerpc: Use common syscall handler type
2022-09-15 5:45 ` Rohan McLure
@ 2022-09-16 1:02 ` Nicholas Piggin
0 siblings, 0 replies; 53+ messages in thread
From: Nicholas Piggin @ 2022-09-16 1:02 UTC (permalink / raw)
To: Rohan McLure; +Cc: linuxppc-dev
On Thu Sep 15, 2022 at 3:45 PM AEST, Rohan McLure wrote:
>
>
> > On 12 Sep 2022, at 8:56 pm, Nicholas Piggin <npiggin@gmail.com> wrote:
> >
> > On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
> >> Cause syscall handlers to be typed as follows when called indirectly
> >> throughout the kernel.
> >>
> >> typedef long (*syscall_fn)(unsigned long, unsigned long, unsigned long,
> >> unsigned long, unsigned long, unsigned long);
> >
> > The point is... better type checking?
> >
> >>
> >> Since both 32 and 64-bit abis allow for at least the first six
> >> machine-word length parameters to a function to be passed by registers,
> >> even handlers which admit fewer than six parameters may be viewed as
> >> having the above type.
> >>
> >> Fixup comparisons in VDSO to avoid pointer-integer comparison. Introduce
> >> explicit cast on systems with SPUs.
> >>
> >> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> >> ---
> >> V1 -> V2: New patch.
> >> V2 -> V3: Remove unnecessary cast from const syscall_fn to syscall_fn
> >> ---
> >> arch/powerpc/include/asm/syscall.h | 7 +++++--
> >> arch/powerpc/include/asm/syscalls.h | 1 +
> >> arch/powerpc/kernel/systbl.c | 6 +++---
> >> arch/powerpc/kernel/vdso.c | 4 ++--
> >> arch/powerpc/platforms/cell/spu_callbacks.c | 6 +++---
> >> 5 files changed, 14 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/arch/powerpc/include/asm/syscall.h b/arch/powerpc/include/asm/syscall.h
> >> index 25fc8ad9a27a..d2a8dfd5de33 100644
> >> --- a/arch/powerpc/include/asm/syscall.h
> >> +++ b/arch/powerpc/include/asm/syscall.h
> >> @@ -14,9 +14,12 @@
> >> #include <linux/sched.h>
> >> #include <linux/thread_info.h>
> >>
> >> +typedef long (*syscall_fn)(unsigned long, unsigned long, unsigned long,
> >> + unsigned long, unsigned long, unsigned long);
> >> +
> >> /* ftrace syscalls requires exporting the sys_call_table */
> >> -extern const unsigned long sys_call_table[];
> >> -extern const unsigned long compat_sys_call_table[];
> >> +extern const syscall_fn sys_call_table[];
> >> +extern const syscall_fn compat_sys_call_table[];
> >
> > Ah you constify it in this patch. I think the previous patch should have
> > kept the const, and it should keep the unsigned long type rather than
> > use void *. Either that or do this patch first.
> >
> >> static inline int syscall_get_nr(struct task_struct *task, struct pt_regs *regs)
> >> {
> >> diff --git a/arch/powerpc/include/asm/syscalls.h b/arch/powerpc/include/asm/syscalls.h
> >> index 91417dee534e..e979b7593d2b 100644
> >> --- a/arch/powerpc/include/asm/syscalls.h
> >> +++ b/arch/powerpc/include/asm/syscalls.h
> >> @@ -8,6 +8,7 @@
> >> #include <linux/types.h>
> >> #include <linux/compat.h>
> >>
> >> +#include <asm/syscall.h>
> >> #ifdef CONFIG_PPC64
> >> #include <asm/syscalls_32.h>
> >> #endif
> >
> > Is this necessary or should be in another patch?
>
> Good spot. This belongs in the patch that produces systbl.c.
>
> >
> >> diff --git a/arch/powerpc/kernel/systbl.c b/arch/powerpc/kernel/systbl.c
> >> index 99ffdfef6b9c..b88a9c2a1f50 100644
> >> --- a/arch/powerpc/kernel/systbl.c
> >> +++ b/arch/powerpc/kernel/systbl.c
> >> @@ -21,10 +21,10 @@
> >> #define __SYSCALL(nr, entry) [nr] = __powerpc_##entry,
> >> #define __powerpc_sys_ni_syscall sys_ni_syscall
> >> #else
> >> -#define __SYSCALL(nr, entry) [nr] = entry,
> >> +#define __SYSCALL(nr, entry) [nr] = (void *) entry,
> >> #endif
> >
> > Also perhaps this should have been in the prior pach and this pach
> > should change the cast from void to syscall_fn ?
>
> This cast to (void *) kicks in when casting functions with six or fewer
Right, I was just wondering if it needs to be in the previous patch
because that's where you changed the type from unsigned long to void *.
Maybe there's some reason it's not required, I didn't entirely follow
all the macro expansion.
> parameters to six-parameter type accepting and returning u64. Sadly I can’t
> find a way to avoid -Wcast-function-type even with (__force syscall_fn) short
> of an ugly casti to void* here. Any suggestions?
Ah okay. I think __force is a sparse specific attribute. Not sure if
gcc/clang can do it. There is a diag thing which maybe can turn off
warnings selectively, but if (void *) is turning off the warning
selectively then there would be no benefit to using it :) That's fine to
keep using void *.
Thanks,
Nick
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 11/20] powerpc: Add ZEROIZE_GPRS macros for register clears
2022-08-24 2:05 [PATCH v4 00/20] powerpc: Syscall wrapper and register clearing Rohan McLure
` (9 preceding siblings ...)
2022-08-24 2:05 ` [PATCH v4 10/20] powerpc: Use common syscall handler type Rohan McLure
@ 2022-08-24 2:05 ` Rohan McLure
2022-09-12 11:09 ` Nicholas Piggin
2022-08-24 2:05 ` [PATCH v4 12/20] Revert "powerpc/syscall: Save r3 in regs->orig_r3" Rohan McLure
` (9 subsequent siblings)
20 siblings, 1 reply; 53+ messages in thread
From: Rohan McLure @ 2022-08-24 2:05 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Rohan McLure
Macros for restoring and saving registers to and from the stack exist.
Provide macros with the same interface for clearing a range of gprs by
setting each register's value in that range to zero.
The resulting macros are called ZEROIZE_GPRS and ZEROIZE_NVGPRS, keeping
with the naming of the accompanying restore and save macros, and usage
of zeroize to describe this operation elsewhere in the kernel.
Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
V1 -> V2: Change 'ZERO' usage in naming to 'NULLIFY', a more obvious verb
V2 -> V3: Change 'NULLIFY' usage in naming to 'ZEROIZE', which has
precedent in kernel and explicitly specifies that we are zeroing.
V3 -> V4: Update commit message to use zeroize.
---
arch/powerpc/include/asm/ppc_asm.h | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/arch/powerpc/include/asm/ppc_asm.h b/arch/powerpc/include/asm/ppc_asm.h
index 83c02f5a7f2a..b95689ada59c 100644
--- a/arch/powerpc/include/asm/ppc_asm.h
+++ b/arch/powerpc/include/asm/ppc_asm.h
@@ -33,6 +33,20 @@
.endr
.endm
+/*
+ * This expands to a sequence of register clears for regs start to end
+ * inclusive, of the form:
+ *
+ * li rN, 0
+ */
+.macro ZEROIZE_REGS start, end
+ .Lreg=\start
+ .rept (\end - \start + 1)
+ li .Lreg, 0
+ .Lreg=.Lreg+1
+ .endr
+.endm
+
/*
* Macros for storing registers into and loading registers from
* exception frames.
@@ -49,6 +63,14 @@
#define REST_NVGPRS(base) REST_GPRS(13, 31, base)
#endif
+#define ZEROIZE_GPRS(start, end) ZEROIZE_REGS start, end
+#ifdef __powerpc64__
+#define ZEROIZE_NVGPRS() ZEROIZE_GPRS(14, 31)
+#else
+#define ZEROIZE_NVGPRS() ZEROIZE_GPRS(13, 31)
+#endif
+#define ZEROIZE_GPR(n) ZEROIZE_GPRS(n, n)
+
#define SAVE_GPR(n, base) SAVE_GPRS(n, n, base)
#define REST_GPR(n, base) REST_GPRS(n, n, base)
--
2.34.1
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v4 11/20] powerpc: Add ZEROIZE_GPRS macros for register clears
2022-08-24 2:05 ` [PATCH v4 11/20] powerpc: Add ZEROIZE_GPRS macros for register clears Rohan McLure
@ 2022-09-12 11:09 ` Nicholas Piggin
2022-09-15 5:47 ` Rohan McLure
0 siblings, 1 reply; 53+ messages in thread
From: Nicholas Piggin @ 2022-09-12 11:09 UTC (permalink / raw)
To: Rohan McLure, linuxppc-dev
On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
> Macros for restoring and saving registers to and from the stack exist.
> Provide macros with the same interface for clearing a range of gprs by
> setting each register's value in that range to zero.
Can I bikeshed this a bit more and ask if you would change the order?
Saving and restoring macros are incidental, and are neither the change
nor the reson for it. I think the changelog reads better if you state
the change (or the problem) up front.
Provid register zeroing macros ... follow the same convention as
existing register stack save/restore macros ... will be used in later
change to zero registers.
Or not, if you disagree.
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> The resulting macros are called ZEROIZE_GPRS and ZEROIZE_NVGPRS, keeping
> with the naming of the accompanying restore and save macros, and usage
> of zeroize to describe this operation elsewhere in the kernel.
>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> V1 -> V2: Change 'ZERO' usage in naming to 'NULLIFY', a more obvious verb
> V2 -> V3: Change 'NULLIFY' usage in naming to 'ZEROIZE', which has
> precedent in kernel and explicitly specifies that we are zeroing.
> V3 -> V4: Update commit message to use zeroize.
> ---
> arch/powerpc/include/asm/ppc_asm.h | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/ppc_asm.h b/arch/powerpc/include/asm/ppc_asm.h
> index 83c02f5a7f2a..b95689ada59c 100644
> --- a/arch/powerpc/include/asm/ppc_asm.h
> +++ b/arch/powerpc/include/asm/ppc_asm.h
> @@ -33,6 +33,20 @@
> .endr
> .endm
>
> +/*
> + * This expands to a sequence of register clears for regs start to end
> + * inclusive, of the form:
> + *
> + * li rN, 0
> + */
> +.macro ZEROIZE_REGS start, end
> + .Lreg=\start
> + .rept (\end - \start + 1)
> + li .Lreg, 0
> + .Lreg=.Lreg+1
> + .endr
> +.endm
> +
> /*
> * Macros for storing registers into and loading registers from
> * exception frames.
> @@ -49,6 +63,14 @@
> #define REST_NVGPRS(base) REST_GPRS(13, 31, base)
> #endif
>
> +#define ZEROIZE_GPRS(start, end) ZEROIZE_REGS start, end
> +#ifdef __powerpc64__
> +#define ZEROIZE_NVGPRS() ZEROIZE_GPRS(14, 31)
> +#else
> +#define ZEROIZE_NVGPRS() ZEROIZE_GPRS(13, 31)
> +#endif
> +#define ZEROIZE_GPR(n) ZEROIZE_GPRS(n, n)
> +
> #define SAVE_GPR(n, base) SAVE_GPRS(n, n, base)
> #define REST_GPR(n, base) REST_GPRS(n, n, base)
>
> --
> 2.34.1
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 11/20] powerpc: Add ZEROIZE_GPRS macros for register clears
2022-09-12 11:09 ` Nicholas Piggin
@ 2022-09-15 5:47 ` Rohan McLure
0 siblings, 0 replies; 53+ messages in thread
From: Rohan McLure @ 2022-09-15 5:47 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linuxppc-dev
> On 12 Sep 2022, at 9:09 pm, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
>> Macros for restoring and saving registers to and from the stack exist.
>> Provide macros with the same interface for clearing a range of gprs by
>> setting each register's value in that range to zero.
>
> Can I bikeshed this a bit more and ask if you would change the order?
>
> Saving and restoring macros are incidental, and are neither the change
> nor the reson for it. I think the changelog reads better if you state
> the change (or the problem) up front.
>
> Provid register zeroing macros ... follow the same convention as
> existing register stack save/restore macros ... will be used in later
> change to zero registers.
Thanks for the suggestion, that should read better.
>
> Or not, if you disagree.
>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
>> The resulting macros are called ZEROIZE_GPRS and ZEROIZE_NVGPRS, keeping
>> with the naming of the accompanying restore and save macros, and usage
>> of zeroize to describe this operation elsewhere in the kernel.
>>
>> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
>> ---
>> V1 -> V2: Change 'ZERO' usage in naming to 'NULLIFY', a more obvious verb
>> V2 -> V3: Change 'NULLIFY' usage in naming to 'ZEROIZE', which has
>> precedent in kernel and explicitly specifies that we are zeroing.
>> V3 -> V4: Update commit message to use zeroize.
>> ---
>> arch/powerpc/include/asm/ppc_asm.h | 22 ++++++++++++++++++++++
>> 1 file changed, 22 insertions(+)
>>
>> diff --git a/arch/powerpc/include/asm/ppc_asm.h b/arch/powerpc/include/asm/ppc_asm.h
>> index 83c02f5a7f2a..b95689ada59c 100644
>> --- a/arch/powerpc/include/asm/ppc_asm.h
>> +++ b/arch/powerpc/include/asm/ppc_asm.h
>> @@ -33,6 +33,20 @@
>> .endr
>> .endm
>>
>> +/*
>> + * This expands to a sequence of register clears for regs start to end
>> + * inclusive, of the form:
>> + *
>> + * li rN, 0
>> + */
>> +.macro ZEROIZE_REGS start, end
>> + .Lreg=\start
>> + .rept (\end - \start + 1)
>> + li .Lreg, 0
>> + .Lreg=.Lreg+1
>> + .endr
>> +.endm
>> +
>> /*
>> * Macros for storing registers into and loading registers from
>> * exception frames.
>> @@ -49,6 +63,14 @@
>> #define REST_NVGPRS(base) REST_GPRS(13, 31, base)
>> #endif
>>
>> +#define ZEROIZE_GPRS(start, end) ZEROIZE_REGS start, end
>> +#ifdef __powerpc64__
>> +#define ZEROIZE_NVGPRS() ZEROIZE_GPRS(14, 31)
>> +#else
>> +#define ZEROIZE_NVGPRS() ZEROIZE_GPRS(13, 31)
>> +#endif
>> +#define ZEROIZE_GPR(n) ZEROIZE_GPRS(n, n)
>> +
>> #define SAVE_GPR(n, base) SAVE_GPRS(n, n, base)
>> #define REST_GPR(n, base) REST_GPRS(n, n, base)
>>
>> --
>> 2.34.1
>
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 12/20] Revert "powerpc/syscall: Save r3 in regs->orig_r3"
2022-08-24 2:05 [PATCH v4 00/20] powerpc: Syscall wrapper and register clearing Rohan McLure
` (10 preceding siblings ...)
2022-08-24 2:05 ` [PATCH v4 11/20] powerpc: Add ZEROIZE_GPRS macros for register clears Rohan McLure
@ 2022-08-24 2:05 ` Rohan McLure
2022-09-12 11:14 ` Nicholas Piggin
2022-08-24 2:05 ` [PATCH v4 13/20] powerpc: Provide syscall wrapper Rohan McLure
` (8 subsequent siblings)
20 siblings, 1 reply; 53+ messages in thread
From: Rohan McLure @ 2022-08-24 2:05 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Rohan McLure
This reverts commit 8875f47b7681aa4e4484a9b612577b044725f839.
Save caller's original r3 state to the kernel stackframe before entering
system_call_exception. This allows for user registers to be cleared by
the time system_call_exception is entered, reducing the influence of
user registers on speculation within the kernel.
Prior to this commit, orig_r3 was saved at the beginning of
system_call_exception. Instead, save orig_r3 while the user value is
still live in r3.
Also replicate this early save in 32-bit. A similar save was removed in
commit 6f76a01173cc ("powerpc/syscall: implement system call entry/exit logic in C for PPC32")
when 32-bit adopted system_call_exception. Revert its removal of orig_r3
saves.
Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
V2 -> V3: New commit.
---
arch/powerpc/kernel/entry_32.S | 1 +
arch/powerpc/kernel/interrupt_64.S | 2 ++
arch/powerpc/kernel/syscall.c | 1 -
3 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
index 1d599df6f169..44dfce9a60c5 100644
--- a/arch/powerpc/kernel/entry_32.S
+++ b/arch/powerpc/kernel/entry_32.S
@@ -101,6 +101,7 @@ __kuep_unlock:
.globl transfer_to_syscall
transfer_to_syscall:
+ stw r3, ORIG_GPR3(r1)
stw r11, GPR1(r1)
stw r11, 0(r1)
mflr r12
diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S
index ce25b28cf418..71d2d9497283 100644
--- a/arch/powerpc/kernel/interrupt_64.S
+++ b/arch/powerpc/kernel/interrupt_64.S
@@ -91,6 +91,7 @@ _ASM_NOKPROBE_SYMBOL(system_call_vectored_\name)
li r11,\trapnr
std r11,_TRAP(r1)
std r12,_CCR(r1)
+ std r3,ORIG_GPR3(r1)
addi r10,r1,STACK_FRAME_OVERHEAD
ld r11,exception_marker@toc(r2)
std r11,-16(r10) /* "regshere" marker */
@@ -275,6 +276,7 @@ END_BTB_FLUSH_SECTION
std r10,_LINK(r1)
std r11,_TRAP(r1)
std r12,_CCR(r1)
+ std r3,ORIG_GPR3(r1)
addi r10,r1,STACK_FRAME_OVERHEAD
ld r11,exception_marker@toc(r2)
std r11,-16(r10) /* "regshere" marker */
diff --git a/arch/powerpc/kernel/syscall.c b/arch/powerpc/kernel/syscall.c
index 81ace9e8b72b..64102a64fd84 100644
--- a/arch/powerpc/kernel/syscall.c
+++ b/arch/powerpc/kernel/syscall.c
@@ -25,7 +25,6 @@ notrace long system_call_exception(long r3, long r4, long r5,
kuap_lock();
add_random_kstack_offset();
- regs->orig_gpr3 = r3;
if (IS_ENABLED(CONFIG_PPC_IRQ_SOFT_MASK_DEBUG))
BUG_ON(irq_soft_mask_return() != IRQS_ALL_DISABLED);
--
2.34.1
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v4 12/20] Revert "powerpc/syscall: Save r3 in regs->orig_r3"
2022-08-24 2:05 ` [PATCH v4 12/20] Revert "powerpc/syscall: Save r3 in regs->orig_r3" Rohan McLure
@ 2022-09-12 11:14 ` Nicholas Piggin
0 siblings, 0 replies; 53+ messages in thread
From: Nicholas Piggin @ 2022-09-12 11:14 UTC (permalink / raw)
To: Rohan McLure, linuxppc-dev
On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
> This reverts commit 8875f47b7681aa4e4484a9b612577b044725f839.
Can you use short hash and commit title format? Also it's no longer
just reverting that patch, so maybe just come up with a new title
for this patch and reference the two patches here?
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
Oh, I meant to say for the last patch and this one. Can you move
them to after patch 13? That way all your build and wrapper mucking
are in the first patches, and then all the zeroizing comes next.
Thanks,
Nick
>
> Save caller's original r3 state to the kernel stackframe before entering
> system_call_exception. This allows for user registers to be cleared by
> the time system_call_exception is entered, reducing the influence of
> user registers on speculation within the kernel.
>
> Prior to this commit, orig_r3 was saved at the beginning of
> system_call_exception. Instead, save orig_r3 while the user value is
> still live in r3.
>
> Also replicate this early save in 32-bit. A similar save was removed in
> commit 6f76a01173cc ("powerpc/syscall: implement system call entry/exit logic in C for PPC32")
> when 32-bit adopted system_call_exception. Revert its removal of orig_r3
> saves.
>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> V2 -> V3: New commit.
> ---
> arch/powerpc/kernel/entry_32.S | 1 +
> arch/powerpc/kernel/interrupt_64.S | 2 ++
> arch/powerpc/kernel/syscall.c | 1 -
> 3 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
> index 1d599df6f169..44dfce9a60c5 100644
> --- a/arch/powerpc/kernel/entry_32.S
> +++ b/arch/powerpc/kernel/entry_32.S
> @@ -101,6 +101,7 @@ __kuep_unlock:
>
> .globl transfer_to_syscall
> transfer_to_syscall:
> + stw r3, ORIG_GPR3(r1)
> stw r11, GPR1(r1)
> stw r11, 0(r1)
> mflr r12
> diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S
> index ce25b28cf418..71d2d9497283 100644
> --- a/arch/powerpc/kernel/interrupt_64.S
> +++ b/arch/powerpc/kernel/interrupt_64.S
> @@ -91,6 +91,7 @@ _ASM_NOKPROBE_SYMBOL(system_call_vectored_\name)
> li r11,\trapnr
> std r11,_TRAP(r1)
> std r12,_CCR(r1)
> + std r3,ORIG_GPR3(r1)
> addi r10,r1,STACK_FRAME_OVERHEAD
> ld r11,exception_marker@toc(r2)
> std r11,-16(r10) /* "regshere" marker */
> @@ -275,6 +276,7 @@ END_BTB_FLUSH_SECTION
> std r10,_LINK(r1)
> std r11,_TRAP(r1)
> std r12,_CCR(r1)
> + std r3,ORIG_GPR3(r1)
> addi r10,r1,STACK_FRAME_OVERHEAD
> ld r11,exception_marker@toc(r2)
> std r11,-16(r10) /* "regshere" marker */
> diff --git a/arch/powerpc/kernel/syscall.c b/arch/powerpc/kernel/syscall.c
> index 81ace9e8b72b..64102a64fd84 100644
> --- a/arch/powerpc/kernel/syscall.c
> +++ b/arch/powerpc/kernel/syscall.c
> @@ -25,7 +25,6 @@ notrace long system_call_exception(long r3, long r4, long r5,
> kuap_lock();
>
> add_random_kstack_offset();
> - regs->orig_gpr3 = r3;
>
> if (IS_ENABLED(CONFIG_PPC_IRQ_SOFT_MASK_DEBUG))
> BUG_ON(irq_soft_mask_return() != IRQS_ALL_DISABLED);
> --
> 2.34.1
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 13/20] powerpc: Provide syscall wrapper
2022-08-24 2:05 [PATCH v4 00/20] powerpc: Syscall wrapper and register clearing Rohan McLure
` (11 preceding siblings ...)
2022-08-24 2:05 ` [PATCH v4 12/20] Revert "powerpc/syscall: Save r3 in regs->orig_r3" Rohan McLure
@ 2022-08-24 2:05 ` Rohan McLure
2022-09-12 11:26 ` Nicholas Piggin
2022-08-24 2:05 ` [PATCH v4 14/20] powerpc/64s: Clear/restore caller gprs in syscall interrupt/return Rohan McLure
` (7 subsequent siblings)
20 siblings, 1 reply; 53+ messages in thread
From: Rohan McLure @ 2022-08-24 2:05 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Rohan McLure, Andrew Donnellan
Implement syscall wrapper as per s390, x86, arm64. When enabled
cause handlers to accept parameters from a stack frame rather than
from user scratch register state. This allows for user registers to be
safely cleared in order to reduce caller influence on speculation
within syscall routine. The wrapper is a macro that emits syscall
handler symbols that call into the target handler, obtaining its
parameters from a struct pt_regs on the stack.
As registers are already saved to the stack prior to calling
system_call_exception, it appears that this function is executed more
efficiently with the new stack-pointer convention than with parameters
passed by registers, avoiding the allocation of a stack frame for this
method. On a 32-bit system, we see >20% performance increases on the
null_syscall microbenchmark, and on a Power 8 the performance gains
amortise the cost of clearing and restoring registers which is
implemented at the end of this series, seeing final result of ~5.6%
performance improvement on null_syscall.
Syscalls are wrapped in this fashion on all platforms except for the
Cell processor as this commit does not provide SPU support. This can be
quickly fixed in a successive patch, but requires spu_sys_callback to
allocate a pt_regs structure to satisfy the wrapped calling convention.
Co-developed-by: Andrew Donnellan <ajd@linux.ibm.com>
Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com>
Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
V1 -> V2: Generate prototypes for symbols produced by the wrapper.
V2 -> V3: Rebased to remove conflict with 1547db7d1f44
("powerpc: Move system_call_exception() to syscall.c"). Also remove copy
from gpr3 save slot on stackframe to orig_r3's slot. Fix whitespace with
preprocessor defines in system_call_exception.
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/interrupt.h | 3 +-
arch/powerpc/include/asm/syscall.h | 4 +
arch/powerpc/include/asm/syscall_wrapper.h | 84 ++++++++++++++++++++
arch/powerpc/include/asm/syscalls.h | 25 +++++-
arch/powerpc/kernel/entry_32.S | 6 +-
arch/powerpc/kernel/interrupt_64.S | 16 ++--
arch/powerpc/kernel/syscall.c | 31 +++-----
arch/powerpc/kernel/systbl.c | 2 +
arch/powerpc/kernel/vdso.c | 2 +
10 files changed, 142 insertions(+), 32 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 4c466acdc70d..ef6c83e79c9b 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -137,6 +137,7 @@ config PPC
select ARCH_HAS_STRICT_KERNEL_RWX if (PPC_BOOK3S || PPC_8xx || 40x) && !HIBERNATION
select ARCH_HAS_STRICT_KERNEL_RWX if FSL_BOOKE && !HIBERNATION && !RANDOMIZE_BASE
select ARCH_HAS_STRICT_MODULE_RWX if ARCH_HAS_STRICT_KERNEL_RWX
+ select ARCH_HAS_SYSCALL_WRAPPER if !SPU_BASE
select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
select ARCH_HAS_UACCESS_FLUSHCACHE
select ARCH_HAS_UBSAN_SANITIZE_ALL
diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
index 8069dbc4b8d1..3f9cad81585c 100644
--- a/arch/powerpc/include/asm/interrupt.h
+++ b/arch/powerpc/include/asm/interrupt.h
@@ -665,8 +665,7 @@ static inline void interrupt_cond_local_irq_enable(struct pt_regs *regs)
local_irq_enable();
}
-long system_call_exception(long r3, long r4, long r5, long r6, long r7, long r8,
- unsigned long r0, struct pt_regs *regs);
+long system_call_exception(unsigned long r0, struct pt_regs *regs);
notrace unsigned long syscall_exit_prepare(unsigned long r3, struct pt_regs *regs, long scv);
notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs);
notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs);
diff --git a/arch/powerpc/include/asm/syscall.h b/arch/powerpc/include/asm/syscall.h
index d2a8dfd5de33..3dd36c5e334a 100644
--- a/arch/powerpc/include/asm/syscall.h
+++ b/arch/powerpc/include/asm/syscall.h
@@ -14,8 +14,12 @@
#include <linux/sched.h>
#include <linux/thread_info.h>
+#ifdef CONFIG_ARCH_HAS_SYSCALL_WRAPPER
+typedef long (*syscall_fn)(const struct pt_regs *);
+#else
typedef long (*syscall_fn)(unsigned long, unsigned long, unsigned long,
unsigned long, unsigned long, unsigned long);
+#endif
/* ftrace syscalls requires exporting the sys_call_table */
extern const syscall_fn sys_call_table[];
diff --git a/arch/powerpc/include/asm/syscall_wrapper.h b/arch/powerpc/include/asm/syscall_wrapper.h
new file mode 100644
index 000000000000..91bcfa40f740
--- /dev/null
+++ b/arch/powerpc/include/asm/syscall_wrapper.h
@@ -0,0 +1,84 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * syscall_wrapper.h - powerpc specific wrappers to syscall definitions
+ *
+ * Based on arch/{x86,arm64}/include/asm/syscall_wrapper.h
+ */
+
+#ifndef __ASM_SYSCALL_WRAPPER_H
+#define __ASM_SYSCALL_WRAPPER_H
+
+struct pt_regs;
+
+#define SC_POWERPC_REGS_TO_ARGS(x, ...) \
+ __MAP(x,__SC_ARGS \
+ ,,regs->gpr[3],,regs->gpr[4],,regs->gpr[5] \
+ ,,regs->gpr[6],,regs->gpr[7],,regs->gpr[8])
+
+#ifdef CONFIG_COMPAT
+
+#define COMPAT_SYSCALL_DEFINEx(x, name, ...) \
+ long __powerpc_compat_sys##name(const struct pt_regs *regs); \
+ ALLOW_ERROR_INJECTION(__powerpc_compat_sys##name, ERRNO); \
+ static long __se_compat_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)); \
+ static inline long __do_compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)); \
+ long __powerpc_compat_sys##name(const struct pt_regs *regs) \
+ { \
+ return __se_compat_sys##name(SC_POWERPC_REGS_TO_ARGS(x,__VA_ARGS__)); \
+ } \
+ static long __se_compat_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)) \
+ { \
+ return __do_compat_sys##name(__MAP(x,__SC_DELOUSE,__VA_ARGS__)); \
+ } \
+ static inline long __do_compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))
+
+#define COMPAT_SYSCALL_DEFINE0(sname) \
+ long __powerpc_compat_sys_##sname(const struct pt_regs *__unused); \
+ ALLOW_ERROR_INJECTION(__powerpc_compat_sys_##sname, ERRNO); \
+ long __powerpc_compat_sys_##sname(const struct pt_regs *__unused)
+
+#define COND_SYSCALL_COMPAT(name) \
+ long __powerpc_compat_sys_##name(const struct pt_regs *regs); \
+ long __weak __powerpc_compat_sys_##name(const struct pt_regs *regs) \
+ { \
+ return sys_ni_syscall(); \
+ }
+#define COMPAT_SYS_NI(name) \
+ SYSCALL_ALIAS(__powerpc_compat_sys_##name, sys_ni_posix_timers);
+
+#endif /* CONFIG_COMPAT */
+
+#define __SYSCALL_DEFINEx(x, name, ...) \
+ long __powerpc_sys##name(const struct pt_regs *regs); \
+ ALLOW_ERROR_INJECTION(__powerpc_sys##name, ERRNO); \
+ static long __se_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)); \
+ static inline long __do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)); \
+ long __powerpc_sys##name(const struct pt_regs *regs) \
+ { \
+ return __se_sys##name(SC_POWERPC_REGS_TO_ARGS(x,__VA_ARGS__)); \
+ } \
+ static long __se_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)) \
+ { \
+ long ret = __do_sys##name(__MAP(x,__SC_CAST,__VA_ARGS__)); \
+ __MAP(x,__SC_TEST,__VA_ARGS__); \
+ __PROTECT(x, ret,__MAP(x,__SC_ARGS,__VA_ARGS__)); \
+ return ret; \
+ } \
+ static inline long __do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))
+
+#define SYSCALL_DEFINE0(sname) \
+ SYSCALL_METADATA(_##sname, 0); \
+ long __powerpc_sys_##sname(const struct pt_regs *__unused); \
+ ALLOW_ERROR_INJECTION(__powerpc_sys_##sname, ERRNO); \
+ long __powerpc_sys_##sname(const struct pt_regs *__unused)
+
+#define COND_SYSCALL(name) \
+ long __powerpc_sys_##name(const struct pt_regs *regs); \
+ long __weak __powerpc_sys_##name(const struct pt_regs *regs) \
+ { \
+ return sys_ni_syscall(); \
+ }
+
+#define SYS_NI(name) SYSCALL_ALIAS(__powerpc_sys_##name, sys_ni_posix_timers);
+
+#endif /* __ASM_SYSCALL_WRAPPER_H */
diff --git a/arch/powerpc/include/asm/syscalls.h b/arch/powerpc/include/asm/syscalls.h
index e979b7593d2b..14af648ee82c 100644
--- a/arch/powerpc/include/asm/syscalls.h
+++ b/arch/powerpc/include/asm/syscalls.h
@@ -15,6 +15,12 @@
#include <asm/unistd.h>
#include <asm/ucontext.h>
+#ifndef CONFIG_ARCH_HAS_SYSCALL_WRAPPER
+long sys_ni_syscall(void);
+#else
+long sys_ni_syscall(const struct pt_regs *regs);
+#endif
+
struct rtas_args;
#ifndef CONFIG_ARCH_HAS_SYSCALL_WRAPPER
@@ -24,7 +30,6 @@ struct rtas_args;
*/
long sys_rtas(struct rtas_args __user *uargs);
-long sys_ni_syscall(void);
#ifdef CONFIG_PPC64
long sys_ppc64_personality(unsigned long personality);
@@ -93,6 +98,24 @@ long compat_sys_ppc_sync_file_range2(int fd, unsigned int flags,
unsigned int nbytes2);
#endif /* CONFIG_COMPAT */
+#else
+
+#define __SYSCALL_WITH_COMPAT(nr, native, compat) __SYSCALL(nr, native)
+#define __SYSCALL(nr, entry) \
+ long __powerpc_##entry(const struct pt_regs *regs);
+
+#ifdef CONFIG_PPC64
+#include <asm/syscall_table_64.h>
+#else
+#include <asm/syscall_table_32.h>
+#endif /* CONFIG_PPC64 */
+
+#ifdef CONFIG_COMPAT
+#undef __SYSCALL_WITH_COMPAT
+#define __SYSCALL_WITH_COMPAT(nr, native, compat) __SYSCALL(nr, compat)
+#include <asm/syscall_table_32.h>
+#endif /* CONFIG_COMPAT */
+
#endif /* CONFIG_ARCH_HAS_SYSCALL_WRAPPER */
#endif /* __KERNEL__ */
diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
index 44dfce9a60c5..8d6e02ef5dc0 100644
--- a/arch/powerpc/kernel/entry_32.S
+++ b/arch/powerpc/kernel/entry_32.S
@@ -122,9 +122,9 @@ transfer_to_syscall:
SAVE_NVGPRS(r1)
kuep_lock
- /* Calling convention has r9 = orig r0, r10 = regs */
- addi r10,r1,STACK_FRAME_OVERHEAD
- mr r9,r0
+ /* Calling convention has r3 = orig r0, r4 = regs */
+ addi r4,r1,STACK_FRAME_OVERHEAD
+ mr r3,r0
bl system_call_exception
ret_from_syscall:
diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S
index 71d2d9497283..0178aeba3820 100644
--- a/arch/powerpc/kernel/interrupt_64.S
+++ b/arch/powerpc/kernel/interrupt_64.S
@@ -92,9 +92,9 @@ _ASM_NOKPROBE_SYMBOL(system_call_vectored_\name)
std r11,_TRAP(r1)
std r12,_CCR(r1)
std r3,ORIG_GPR3(r1)
- addi r10,r1,STACK_FRAME_OVERHEAD
+ addi r4,r1,STACK_FRAME_OVERHEAD
ld r11,exception_marker@toc(r2)
- std r11,-16(r10) /* "regshere" marker */
+ std r11,-16(r4) /* "regshere" marker */
BEGIN_FTR_SECTION
HMT_MEDIUM
@@ -109,8 +109,8 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
* but this is the best we can do.
*/
- /* Calling convention has r9 = orig r0, r10 = regs */
- mr r9,r0
+ /* Calling convention has r3 = orig r0, r4 = regs */
+ mr r3,r0
bl system_call_exception
.Lsyscall_vectored_\name\()_exit:
@@ -277,9 +277,9 @@ END_BTB_FLUSH_SECTION
std r11,_TRAP(r1)
std r12,_CCR(r1)
std r3,ORIG_GPR3(r1)
- addi r10,r1,STACK_FRAME_OVERHEAD
+ addi r4,r1,STACK_FRAME_OVERHEAD
ld r11,exception_marker@toc(r2)
- std r11,-16(r10) /* "regshere" marker */
+ std r11,-16(r4) /* "regshere" marker */
#ifdef CONFIG_PPC_BOOK3S
li r11,1
@@ -300,8 +300,8 @@ END_BTB_FLUSH_SECTION
wrteei 1
#endif
- /* Calling convention has r9 = orig r0, r10 = regs */
- mr r9,r0
+ /* Calling convention has r3 = orig r0, r4 = regs */
+ mr r3,r0
bl system_call_exception
.Lsyscall_exit:
diff --git a/arch/powerpc/kernel/syscall.c b/arch/powerpc/kernel/syscall.c
index 64102a64fd84..bda5221b420e 100644
--- a/arch/powerpc/kernel/syscall.c
+++ b/arch/powerpc/kernel/syscall.c
@@ -12,12 +12,8 @@
#include <asm/unistd.h>
-typedef long (*syscall_fn)(long, long, long, long, long, long);
-
/* Has to run notrace because it is entered not completely "reconciled" */
-notrace long system_call_exception(long r3, long r4, long r5,
- long r6, long r7, long r8,
- unsigned long r0, struct pt_regs *regs)
+notrace long system_call_exception(unsigned long r0, struct pt_regs *regs)
{
long ret;
syscall_fn f;
@@ -138,12 +134,6 @@ notrace long system_call_exception(long r3, long r4, long r5,
r0 = do_syscall_trace_enter(regs);
if (unlikely(r0 >= NR_syscalls))
return regs->gpr[3];
- r3 = regs->gpr[3];
- r4 = regs->gpr[4];
- r5 = regs->gpr[5];
- r6 = regs->gpr[6];
- r7 = regs->gpr[7];
- r8 = regs->gpr[8];
} else if (unlikely(r0 >= NR_syscalls)) {
if (unlikely(trap_is_unsupported_scv(regs))) {
@@ -160,18 +150,23 @@ notrace long system_call_exception(long r3, long r4, long r5,
if (unlikely(is_compat_task())) {
f = (void *)compat_sys_call_table[r0];
- r3 &= 0x00000000ffffffffULL;
- r4 &= 0x00000000ffffffffULL;
- r5 &= 0x00000000ffffffffULL;
- r6 &= 0x00000000ffffffffULL;
- r7 &= 0x00000000ffffffffULL;
- r8 &= 0x00000000ffffffffULL;
+ regs->gpr[3] &= 0x00000000ffffffffULL;
+ regs->gpr[4] &= 0x00000000ffffffffULL;
+ regs->gpr[5] &= 0x00000000ffffffffULL;
+ regs->gpr[6] &= 0x00000000ffffffffULL;
+ regs->gpr[7] &= 0x00000000ffffffffULL;
+ regs->gpr[8] &= 0x00000000ffffffffULL;
} else {
f = (void *)sys_call_table[r0];
}
- ret = f(r3, r4, r5, r6, r7, r8);
+#ifdef CONFIG_ARCH_HAS_SYSCALL_WRAPPER
+ ret = f(regs);
+#else
+ ret = f(regs->gpr[3], regs->gpr[4], regs->gpr[5],
+ regs->gpr[6], regs->gpr[7], regs->gpr[8]);
+#endif
/*
* Ultimately, this value will get limited by KSTACK_OFFSET_MAX(),
diff --git a/arch/powerpc/kernel/systbl.c b/arch/powerpc/kernel/systbl.c
index b88a9c2a1f50..cb05e302ea58 100644
--- a/arch/powerpc/kernel/systbl.c
+++ b/arch/powerpc/kernel/systbl.c
@@ -15,8 +15,10 @@
#include <asm/unistd.h>
#include <asm/syscalls.h>
+#undef __SYSCALL_WITH_COMPAT
#define __SYSCALL_WITH_COMPAT(nr, entry, compat) __SYSCALL(nr, entry)
+#undef __SYSCALL
#ifdef CONFIG_ARCH_HAS_SYSCALL_WRAPPER
#define __SYSCALL(nr, entry) [nr] = __powerpc_##entry,
#define __powerpc_sys_ni_syscall sys_ni_syscall
diff --git a/arch/powerpc/kernel/vdso.c b/arch/powerpc/kernel/vdso.c
index 080c9e7de0c0..88d83162eaf1 100644
--- a/arch/powerpc/kernel/vdso.c
+++ b/arch/powerpc/kernel/vdso.c
@@ -39,6 +39,8 @@
extern char vdso32_start, vdso32_end;
extern char vdso64_start, vdso64_end;
+long sys_ni_syscall(void);
+
/*
* The vdso data page (aka. systemcfg for old ppc64 fans) is here.
* Once the early boot kernel code no longer needs to muck around
--
2.34.1
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v4 13/20] powerpc: Provide syscall wrapper
2022-08-24 2:05 ` [PATCH v4 13/20] powerpc: Provide syscall wrapper Rohan McLure
@ 2022-09-12 11:26 ` Nicholas Piggin
0 siblings, 0 replies; 53+ messages in thread
From: Nicholas Piggin @ 2022-09-12 11:26 UTC (permalink / raw)
To: Rohan McLure, linuxppc-dev; +Cc: Andrew Donnellan
On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
> Implement syscall wrapper as per s390, x86, arm64. When enabled
> cause handlers to accept parameters from a stack frame rather than
> from user scratch register state. This allows for user registers to be
> safely cleared in order to reduce caller influence on speculation
> within syscall routine. The wrapper is a macro that emits syscall
> handler symbols that call into the target handler, obtaining its
> parameters from a struct pt_regs on the stack.
>
> As registers are already saved to the stack prior to calling
> system_call_exception, it appears that this function is executed more
> efficiently with the new stack-pointer convention than with parameters
> passed by registers, avoiding the allocation of a stack frame for this
> method. On a 32-bit system, we see >20% performance increases on the
> null_syscall microbenchmark, and on a Power 8 the performance gains
> amortise the cost of clearing and restoring registers which is
> implemented at the end of this series, seeing final result of ~5.6%
> performance improvement on null_syscall.
>
> Syscalls are wrapped in this fashion on all platforms except for the
> Cell processor as this commit does not provide SPU support. This can be
> quickly fixed in a successive patch, but requires spu_sys_callback to
> allocate a pt_regs structure to satisfy the wrapped calling convention.
I didn't look closely at the syscall wrapper macro gunk but it
generally looks pretty fine, not much change to actual code which
is good...
>
> Co-developed-by: Andrew Donnellan <ajd@linux.ibm.com>
> Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> V1 -> V2: Generate prototypes for symbols produced by the wrapper.
> V2 -> V3: Rebased to remove conflict with 1547db7d1f44
> ("powerpc: Move system_call_exception() to syscall.c"). Also remove copy
> from gpr3 save slot on stackframe to orig_r3's slot. Fix whitespace with
> preprocessor defines in system_call_exception.
> ---
> arch/powerpc/Kconfig | 1 +
> arch/powerpc/include/asm/interrupt.h | 3 +-
> arch/powerpc/include/asm/syscall.h | 4 +
> arch/powerpc/include/asm/syscall_wrapper.h | 84 ++++++++++++++++++++
> arch/powerpc/include/asm/syscalls.h | 25 +++++-
> arch/powerpc/kernel/entry_32.S | 6 +-
> arch/powerpc/kernel/interrupt_64.S | 16 ++--
> arch/powerpc/kernel/syscall.c | 31 +++-----
> arch/powerpc/kernel/systbl.c | 2 +
> arch/powerpc/kernel/vdso.c | 2 +
> 10 files changed, 142 insertions(+), 32 deletions(-)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 4c466acdc70d..ef6c83e79c9b 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -137,6 +137,7 @@ config PPC
> select ARCH_HAS_STRICT_KERNEL_RWX if (PPC_BOOK3S || PPC_8xx || 40x) && !HIBERNATION
> select ARCH_HAS_STRICT_KERNEL_RWX if FSL_BOOKE && !HIBERNATION && !RANDOMIZE_BASE
> select ARCH_HAS_STRICT_MODULE_RWX if ARCH_HAS_STRICT_KERNEL_RWX
> + select ARCH_HAS_SYSCALL_WRAPPER if !SPU_BASE
> select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
> select ARCH_HAS_UACCESS_FLUSHCACHE
> select ARCH_HAS_UBSAN_SANITIZE_ALL
> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
> index 8069dbc4b8d1..3f9cad81585c 100644
> --- a/arch/powerpc/include/asm/interrupt.h
> +++ b/arch/powerpc/include/asm/interrupt.h
> @@ -665,8 +665,7 @@ static inline void interrupt_cond_local_irq_enable(struct pt_regs *regs)
> local_irq_enable();
> }
>
> -long system_call_exception(long r3, long r4, long r5, long r6, long r7, long r8,
> - unsigned long r0, struct pt_regs *regs);
> +long system_call_exception(unsigned long r0, struct pt_regs *regs);
Can you change the calling convention to be regs then r0 so regs matches
the argument position in the syscall?
Then we perhaps should look at storing the registers to pt_regs using
the same r3 as base register to save the values in the asm too. That
way it's friendlier to possible static load hit store predictors. Not
for your series but just a possible later idea.
Thanks,
Nick
> notrace unsigned long syscall_exit_prepare(unsigned long r3, struct pt_regs *regs, long scv);
> notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs);
> notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs);
> diff --git a/arch/powerpc/include/asm/syscall.h b/arch/powerpc/include/asm/syscall.h
> index d2a8dfd5de33..3dd36c5e334a 100644
> --- a/arch/powerpc/include/asm/syscall.h
> +++ b/arch/powerpc/include/asm/syscall.h
> @@ -14,8 +14,12 @@
> #include <linux/sched.h>
> #include <linux/thread_info.h>
>
> +#ifdef CONFIG_ARCH_HAS_SYSCALL_WRAPPER
> +typedef long (*syscall_fn)(const struct pt_regs *);
> +#else
> typedef long (*syscall_fn)(unsigned long, unsigned long, unsigned long,
> unsigned long, unsigned long, unsigned long);
> +#endif
>
> /* ftrace syscalls requires exporting the sys_call_table */
> extern const syscall_fn sys_call_table[];
> diff --git a/arch/powerpc/include/asm/syscall_wrapper.h b/arch/powerpc/include/asm/syscall_wrapper.h
> new file mode 100644
> index 000000000000..91bcfa40f740
> --- /dev/null
> +++ b/arch/powerpc/include/asm/syscall_wrapper.h
> @@ -0,0 +1,84 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * syscall_wrapper.h - powerpc specific wrappers to syscall definitions
> + *
> + * Based on arch/{x86,arm64}/include/asm/syscall_wrapper.h
> + */
> +
> +#ifndef __ASM_SYSCALL_WRAPPER_H
> +#define __ASM_SYSCALL_WRAPPER_H
> +
> +struct pt_regs;
> +
> +#define SC_POWERPC_REGS_TO_ARGS(x, ...) \
> + __MAP(x,__SC_ARGS \
> + ,,regs->gpr[3],,regs->gpr[4],,regs->gpr[5] \
> + ,,regs->gpr[6],,regs->gpr[7],,regs->gpr[8])
> +
> +#ifdef CONFIG_COMPAT
> +
> +#define COMPAT_SYSCALL_DEFINEx(x, name, ...) \
> + long __powerpc_compat_sys##name(const struct pt_regs *regs); \
> + ALLOW_ERROR_INJECTION(__powerpc_compat_sys##name, ERRNO); \
> + static long __se_compat_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)); \
> + static inline long __do_compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)); \
> + long __powerpc_compat_sys##name(const struct pt_regs *regs) \
> + { \
> + return __se_compat_sys##name(SC_POWERPC_REGS_TO_ARGS(x,__VA_ARGS__)); \
> + } \
> + static long __se_compat_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)) \
> + { \
> + return __do_compat_sys##name(__MAP(x,__SC_DELOUSE,__VA_ARGS__)); \
> + } \
> + static inline long __do_compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))
> +
> +#define COMPAT_SYSCALL_DEFINE0(sname) \
> + long __powerpc_compat_sys_##sname(const struct pt_regs *__unused); \
> + ALLOW_ERROR_INJECTION(__powerpc_compat_sys_##sname, ERRNO); \
> + long __powerpc_compat_sys_##sname(const struct pt_regs *__unused)
> +
> +#define COND_SYSCALL_COMPAT(name) \
> + long __powerpc_compat_sys_##name(const struct pt_regs *regs); \
> + long __weak __powerpc_compat_sys_##name(const struct pt_regs *regs) \
> + { \
> + return sys_ni_syscall(); \
> + }
> +#define COMPAT_SYS_NI(name) \
> + SYSCALL_ALIAS(__powerpc_compat_sys_##name, sys_ni_posix_timers);
> +
> +#endif /* CONFIG_COMPAT */
> +
> +#define __SYSCALL_DEFINEx(x, name, ...) \
> + long __powerpc_sys##name(const struct pt_regs *regs); \
> + ALLOW_ERROR_INJECTION(__powerpc_sys##name, ERRNO); \
> + static long __se_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)); \
> + static inline long __do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)); \
> + long __powerpc_sys##name(const struct pt_regs *regs) \
> + { \
> + return __se_sys##name(SC_POWERPC_REGS_TO_ARGS(x,__VA_ARGS__)); \
> + } \
> + static long __se_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)) \
> + { \
> + long ret = __do_sys##name(__MAP(x,__SC_CAST,__VA_ARGS__)); \
> + __MAP(x,__SC_TEST,__VA_ARGS__); \
> + __PROTECT(x, ret,__MAP(x,__SC_ARGS,__VA_ARGS__)); \
> + return ret; \
> + } \
> + static inline long __do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))
> +
> +#define SYSCALL_DEFINE0(sname) \
> + SYSCALL_METADATA(_##sname, 0); \
> + long __powerpc_sys_##sname(const struct pt_regs *__unused); \
> + ALLOW_ERROR_INJECTION(__powerpc_sys_##sname, ERRNO); \
> + long __powerpc_sys_##sname(const struct pt_regs *__unused)
> +
> +#define COND_SYSCALL(name) \
> + long __powerpc_sys_##name(const struct pt_regs *regs); \
> + long __weak __powerpc_sys_##name(const struct pt_regs *regs) \
> + { \
> + return sys_ni_syscall(); \
> + }
> +
> +#define SYS_NI(name) SYSCALL_ALIAS(__powerpc_sys_##name, sys_ni_posix_timers);
> +
> +#endif /* __ASM_SYSCALL_WRAPPER_H */
> diff --git a/arch/powerpc/include/asm/syscalls.h b/arch/powerpc/include/asm/syscalls.h
> index e979b7593d2b..14af648ee82c 100644
> --- a/arch/powerpc/include/asm/syscalls.h
> +++ b/arch/powerpc/include/asm/syscalls.h
> @@ -15,6 +15,12 @@
> #include <asm/unistd.h>
> #include <asm/ucontext.h>
>
> +#ifndef CONFIG_ARCH_HAS_SYSCALL_WRAPPER
> +long sys_ni_syscall(void);
> +#else
> +long sys_ni_syscall(const struct pt_regs *regs);
> +#endif
> +
> struct rtas_args;
>
> #ifndef CONFIG_ARCH_HAS_SYSCALL_WRAPPER
> @@ -24,7 +30,6 @@ struct rtas_args;
> */
>
> long sys_rtas(struct rtas_args __user *uargs);
> -long sys_ni_syscall(void);
>
> #ifdef CONFIG_PPC64
> long sys_ppc64_personality(unsigned long personality);
> @@ -93,6 +98,24 @@ long compat_sys_ppc_sync_file_range2(int fd, unsigned int flags,
> unsigned int nbytes2);
> #endif /* CONFIG_COMPAT */
>
> +#else
> +
> +#define __SYSCALL_WITH_COMPAT(nr, native, compat) __SYSCALL(nr, native)
> +#define __SYSCALL(nr, entry) \
> + long __powerpc_##entry(const struct pt_regs *regs);
> +
> +#ifdef CONFIG_PPC64
> +#include <asm/syscall_table_64.h>
> +#else
> +#include <asm/syscall_table_32.h>
> +#endif /* CONFIG_PPC64 */
> +
> +#ifdef CONFIG_COMPAT
> +#undef __SYSCALL_WITH_COMPAT
> +#define __SYSCALL_WITH_COMPAT(nr, native, compat) __SYSCALL(nr, compat)
> +#include <asm/syscall_table_32.h>
> +#endif /* CONFIG_COMPAT */
> +
> #endif /* CONFIG_ARCH_HAS_SYSCALL_WRAPPER */
>
> #endif /* __KERNEL__ */
> diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
> index 44dfce9a60c5..8d6e02ef5dc0 100644
> --- a/arch/powerpc/kernel/entry_32.S
> +++ b/arch/powerpc/kernel/entry_32.S
> @@ -122,9 +122,9 @@ transfer_to_syscall:
> SAVE_NVGPRS(r1)
> kuep_lock
>
> - /* Calling convention has r9 = orig r0, r10 = regs */
> - addi r10,r1,STACK_FRAME_OVERHEAD
> - mr r9,r0
> + /* Calling convention has r3 = orig r0, r4 = regs */
> + addi r4,r1,STACK_FRAME_OVERHEAD
> + mr r3,r0
> bl system_call_exception
>
> ret_from_syscall:
> diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S
> index 71d2d9497283..0178aeba3820 100644
> --- a/arch/powerpc/kernel/interrupt_64.S
> +++ b/arch/powerpc/kernel/interrupt_64.S
> @@ -92,9 +92,9 @@ _ASM_NOKPROBE_SYMBOL(system_call_vectored_\name)
> std r11,_TRAP(r1)
> std r12,_CCR(r1)
> std r3,ORIG_GPR3(r1)
> - addi r10,r1,STACK_FRAME_OVERHEAD
> + addi r4,r1,STACK_FRAME_OVERHEAD
> ld r11,exception_marker@toc(r2)
> - std r11,-16(r10) /* "regshere" marker */
> + std r11,-16(r4) /* "regshere" marker */
>
> BEGIN_FTR_SECTION
> HMT_MEDIUM
> @@ -109,8 +109,8 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> * but this is the best we can do.
> */
>
> - /* Calling convention has r9 = orig r0, r10 = regs */
> - mr r9,r0
> + /* Calling convention has r3 = orig r0, r4 = regs */
> + mr r3,r0
> bl system_call_exception
>
> .Lsyscall_vectored_\name\()_exit:
> @@ -277,9 +277,9 @@ END_BTB_FLUSH_SECTION
> std r11,_TRAP(r1)
> std r12,_CCR(r1)
> std r3,ORIG_GPR3(r1)
> - addi r10,r1,STACK_FRAME_OVERHEAD
> + addi r4,r1,STACK_FRAME_OVERHEAD
> ld r11,exception_marker@toc(r2)
> - std r11,-16(r10) /* "regshere" marker */
> + std r11,-16(r4) /* "regshere" marker */
>
> #ifdef CONFIG_PPC_BOOK3S
> li r11,1
> @@ -300,8 +300,8 @@ END_BTB_FLUSH_SECTION
> wrteei 1
> #endif
>
> - /* Calling convention has r9 = orig r0, r10 = regs */
> - mr r9,r0
> + /* Calling convention has r3 = orig r0, r4 = regs */
> + mr r3,r0
> bl system_call_exception
>
> .Lsyscall_exit:
> diff --git a/arch/powerpc/kernel/syscall.c b/arch/powerpc/kernel/syscall.c
> index 64102a64fd84..bda5221b420e 100644
> --- a/arch/powerpc/kernel/syscall.c
> +++ b/arch/powerpc/kernel/syscall.c
> @@ -12,12 +12,8 @@
> #include <asm/unistd.h>
>
>
> -typedef long (*syscall_fn)(long, long, long, long, long, long);
> -
> /* Has to run notrace because it is entered not completely "reconciled" */
> -notrace long system_call_exception(long r3, long r4, long r5,
> - long r6, long r7, long r8,
> - unsigned long r0, struct pt_regs *regs)
> +notrace long system_call_exception(unsigned long r0, struct pt_regs *regs)
> {
> long ret;
> syscall_fn f;
> @@ -138,12 +134,6 @@ notrace long system_call_exception(long r3, long r4, long r5,
> r0 = do_syscall_trace_enter(regs);
> if (unlikely(r0 >= NR_syscalls))
> return regs->gpr[3];
> - r3 = regs->gpr[3];
> - r4 = regs->gpr[4];
> - r5 = regs->gpr[5];
> - r6 = regs->gpr[6];
> - r7 = regs->gpr[7];
> - r8 = regs->gpr[8];
>
> } else if (unlikely(r0 >= NR_syscalls)) {
> if (unlikely(trap_is_unsupported_scv(regs))) {
> @@ -160,18 +150,23 @@ notrace long system_call_exception(long r3, long r4, long r5,
> if (unlikely(is_compat_task())) {
> f = (void *)compat_sys_call_table[r0];
>
> - r3 &= 0x00000000ffffffffULL;
> - r4 &= 0x00000000ffffffffULL;
> - r5 &= 0x00000000ffffffffULL;
> - r6 &= 0x00000000ffffffffULL;
> - r7 &= 0x00000000ffffffffULL;
> - r8 &= 0x00000000ffffffffULL;
> + regs->gpr[3] &= 0x00000000ffffffffULL;
> + regs->gpr[4] &= 0x00000000ffffffffULL;
> + regs->gpr[5] &= 0x00000000ffffffffULL;
> + regs->gpr[6] &= 0x00000000ffffffffULL;
> + regs->gpr[7] &= 0x00000000ffffffffULL;
> + regs->gpr[8] &= 0x00000000ffffffffULL;
>
> } else {
> f = (void *)sys_call_table[r0];
> }
>
> - ret = f(r3, r4, r5, r6, r7, r8);
> +#ifdef CONFIG_ARCH_HAS_SYSCALL_WRAPPER
> + ret = f(regs);
> +#else
> + ret = f(regs->gpr[3], regs->gpr[4], regs->gpr[5],
> + regs->gpr[6], regs->gpr[7], regs->gpr[8]);
> +#endif
>
> /*
> * Ultimately, this value will get limited by KSTACK_OFFSET_MAX(),
> diff --git a/arch/powerpc/kernel/systbl.c b/arch/powerpc/kernel/systbl.c
> index b88a9c2a1f50..cb05e302ea58 100644
> --- a/arch/powerpc/kernel/systbl.c
> +++ b/arch/powerpc/kernel/systbl.c
> @@ -15,8 +15,10 @@
> #include <asm/unistd.h>
> #include <asm/syscalls.h>
>
> +#undef __SYSCALL_WITH_COMPAT
> #define __SYSCALL_WITH_COMPAT(nr, entry, compat) __SYSCALL(nr, entry)
>
> +#undef __SYSCALL
> #ifdef CONFIG_ARCH_HAS_SYSCALL_WRAPPER
> #define __SYSCALL(nr, entry) [nr] = __powerpc_##entry,
> #define __powerpc_sys_ni_syscall sys_ni_syscall
> diff --git a/arch/powerpc/kernel/vdso.c b/arch/powerpc/kernel/vdso.c
> index 080c9e7de0c0..88d83162eaf1 100644
> --- a/arch/powerpc/kernel/vdso.c
> +++ b/arch/powerpc/kernel/vdso.c
> @@ -39,6 +39,8 @@
> extern char vdso32_start, vdso32_end;
> extern char vdso64_start, vdso64_end;
>
> +long sys_ni_syscall(void);
> +
> /*
> * The vdso data page (aka. systemcfg for old ppc64 fans) is here.
> * Once the early boot kernel code no longer needs to muck around
> --
> 2.34.1
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 14/20] powerpc/64s: Clear/restore caller gprs in syscall interrupt/return
2022-08-24 2:05 [PATCH v4 00/20] powerpc: Syscall wrapper and register clearing Rohan McLure
` (12 preceding siblings ...)
2022-08-24 2:05 ` [PATCH v4 13/20] powerpc: Provide syscall wrapper Rohan McLure
@ 2022-08-24 2:05 ` Rohan McLure
2022-09-12 11:47 ` Nicholas Piggin
2022-08-24 2:05 ` [PATCH v4 15/20] powerpc/64s: Use {ZEROIZE,SAVE,REST}_GPRS macros in sc, scv 0 handlers Rohan McLure
` (6 subsequent siblings)
20 siblings, 1 reply; 53+ messages in thread
From: Rohan McLure @ 2022-08-24 2:05 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Rohan McLure
Clear user state in gprs (assign to zero) to reduce the influence of user
registers on speculation within kernel syscall handlers. Clears occur
at the very beginning of the sc and scv 0 interrupt handlers, with
restores occurring following the execution of the syscall handler.
Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
V1 -> V2: Update summary
V2 -> V3: Remove erroneous summary paragraph on syscall_exit_prepare
V3 -> V4: Use ZEROIZE instead of NULLIFY
---
arch/powerpc/kernel/interrupt_64.S | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S
index 0178aeba3820..a8065209dd8c 100644
--- a/arch/powerpc/kernel/interrupt_64.S
+++ b/arch/powerpc/kernel/interrupt_64.S
@@ -70,7 +70,7 @@ _ASM_NOKPROBE_SYMBOL(system_call_vectored_\name)
ld r2,PACATOC(r13)
mfcr r12
li r11,0
- /* Can we avoid saving r3-r8 in common case? */
+ /* Save syscall parameters in r3-r8 */
std r3,GPR3(r1)
std r4,GPR4(r1)
std r5,GPR5(r1)
@@ -109,6 +109,13 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
* but this is the best we can do.
*/
+ /*
+ * Zero user registers to prevent influencing speculative execution
+ * state of kernel code.
+ */
+ ZEROIZE_GPRS(5, 12)
+ ZEROIZE_NVGPRS()
+
/* Calling convention has r3 = orig r0, r4 = regs */
mr r3,r0
bl system_call_exception
@@ -139,6 +146,7 @@ BEGIN_FTR_SECTION
HMT_MEDIUM_LOW
END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
+ REST_NVGPRS(r1)
cmpdi r3,0
bne .Lsyscall_vectored_\name\()_restore_regs
@@ -181,7 +189,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
ld r4,_LINK(r1)
ld r5,_XER(r1)
- REST_NVGPRS(r1)
ld r0,GPR0(r1)
mtcr r2
mtctr r3
@@ -249,7 +256,7 @@ END_BTB_FLUSH_SECTION
ld r2,PACATOC(r13)
mfcr r12
li r11,0
- /* Can we avoid saving r3-r8 in common case? */
+ /* Save syscall parameters in r3-r8 */
std r3,GPR3(r1)
std r4,GPR4(r1)
std r5,GPR5(r1)
@@ -300,6 +307,13 @@ END_BTB_FLUSH_SECTION
wrteei 1
#endif
+ /*
+ * Zero user registers to prevent influencing speculative execution
+ * state of kernel code.
+ */
+ ZEROIZE_GPRS(5, 12)
+ ZEROIZE_NVGPRS()
+
/* Calling convention has r3 = orig r0, r4 = regs */
mr r3,r0
bl system_call_exception
@@ -342,6 +356,7 @@ BEGIN_FTR_SECTION
stdcx. r0,0,r1 /* to clear the reservation */
END_FTR_SECTION_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
+ REST_NVGPRS(r1)
cmpdi r3,0
bne .Lsyscall_restore_regs
/* Zero volatile regs that may contain sensitive kernel data */
@@ -377,7 +392,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
.Lsyscall_restore_regs:
ld r3,_CTR(r1)
ld r4,_XER(r1)
- REST_NVGPRS(r1)
mtctr r3
mtspr SPRN_XER,r4
ld r0,GPR0(r1)
--
2.34.1
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v4 14/20] powerpc/64s: Clear/restore caller gprs in syscall interrupt/return
2022-08-24 2:05 ` [PATCH v4 14/20] powerpc/64s: Clear/restore caller gprs in syscall interrupt/return Rohan McLure
@ 2022-09-12 11:47 ` Nicholas Piggin
0 siblings, 0 replies; 53+ messages in thread
From: Nicholas Piggin @ 2022-09-12 11:47 UTC (permalink / raw)
To: Rohan McLure, linuxppc-dev
On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
> Clear user state in gprs (assign to zero) to reduce the influence of user
> registers on speculation within kernel syscall handlers. Clears occur
> at the very beginning of the sc and scv 0 interrupt handlers, with
> restores occurring following the execution of the syscall handler.
>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> V1 -> V2: Update summary
> V2 -> V3: Remove erroneous summary paragraph on syscall_exit_prepare
> V3 -> V4: Use ZEROIZE instead of NULLIFY
> ---
> arch/powerpc/kernel/interrupt_64.S | 22 ++++++++++++++++++----
> 1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S
> index 0178aeba3820..a8065209dd8c 100644
> --- a/arch/powerpc/kernel/interrupt_64.S
> +++ b/arch/powerpc/kernel/interrupt_64.S
> @@ -70,7 +70,7 @@ _ASM_NOKPROBE_SYMBOL(system_call_vectored_\name)
> ld r2,PACATOC(r13)
> mfcr r12
> li r11,0
> - /* Can we avoid saving r3-r8 in common case? */
> + /* Save syscall parameters in r3-r8 */
> std r3,GPR3(r1)
> std r4,GPR4(r1)
> std r5,GPR5(r1)
> @@ -109,6 +109,13 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> * but this is the best we can do.
> */
>
> + /*
> + * Zero user registers to prevent influencing speculative execution
> + * state of kernel code.
> + */
> + ZEROIZE_GPRS(5, 12)
> + ZEROIZE_NVGPRS()
> +
> /* Calling convention has r3 = orig r0, r4 = regs */
> mr r3,r0
Ther's not much reason for this to go right against
system_call_exception. You could set it up above where you set r4,
and zero r0 as well with the rest of the volatiles? I guess it'll
be overwritten by mflr r0 right away... although that could be an
implementation detail not everything requires it and mcount call
gets noped out. So probably doesn't hurt to zero it, while we're
being paranoid.
Thanks,
Nick
> bl system_call_exception
> @@ -139,6 +146,7 @@ BEGIN_FTR_SECTION
> HMT_MEDIUM_LOW
> END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>
> + REST_NVGPRS(r1)
> cmpdi r3,0
> bne .Lsyscall_vectored_\name\()_restore_regs
>
> @@ -181,7 +189,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> ld r4,_LINK(r1)
> ld r5,_XER(r1)
>
> - REST_NVGPRS(r1)
> ld r0,GPR0(r1)
> mtcr r2
> mtctr r3
> @@ -249,7 +256,7 @@ END_BTB_FLUSH_SECTION
> ld r2,PACATOC(r13)
> mfcr r12
> li r11,0
> - /* Can we avoid saving r3-r8 in common case? */
> + /* Save syscall parameters in r3-r8 */
> std r3,GPR3(r1)
> std r4,GPR4(r1)
> std r5,GPR5(r1)
> @@ -300,6 +307,13 @@ END_BTB_FLUSH_SECTION
> wrteei 1
> #endif
>
> + /*
> + * Zero user registers to prevent influencing speculative execution
> + * state of kernel code.
> + */
> + ZEROIZE_GPRS(5, 12)
> + ZEROIZE_NVGPRS()
> +
> /* Calling convention has r3 = orig r0, r4 = regs */
> mr r3,r0
> bl system_call_exception
> @@ -342,6 +356,7 @@ BEGIN_FTR_SECTION
> stdcx. r0,0,r1 /* to clear the reservation */
> END_FTR_SECTION_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
>
> + REST_NVGPRS(r1)
> cmpdi r3,0
> bne .Lsyscall_restore_regs
> /* Zero volatile regs that may contain sensitive kernel data */
> @@ -377,7 +392,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> .Lsyscall_restore_regs:
> ld r3,_CTR(r1)
> ld r4,_XER(r1)
> - REST_NVGPRS(r1)
> mtctr r3
> mtspr SPRN_XER,r4
> ld r0,GPR0(r1)
> --
> 2.34.1
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 15/20] powerpc/64s: Use {ZEROIZE,SAVE,REST}_GPRS macros in sc, scv 0 handlers
2022-08-24 2:05 [PATCH v4 00/20] powerpc: Syscall wrapper and register clearing Rohan McLure
` (13 preceding siblings ...)
2022-08-24 2:05 ` [PATCH v4 14/20] powerpc/64s: Clear/restore caller gprs in syscall interrupt/return Rohan McLure
@ 2022-08-24 2:05 ` Rohan McLure
2022-09-12 11:49 ` Nicholas Piggin
2022-08-24 2:05 ` [PATCH v4 16/20] powerpc/32: Clarify interrupt restores with REST_GPR macro in entry_32.S Rohan McLure
` (5 subsequent siblings)
20 siblings, 1 reply; 53+ messages in thread
From: Rohan McLure @ 2022-08-24 2:05 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Rohan McLure
Use the convenience macros for saving/clearing/restoring gprs in keeping
with syscall calling conventions. The plural variants of these macros
can store a range of registers for concision.
This works well when the user gpr value we are hoping to save is still
live. In the syscall interrupt handlers, user register state is
sometimes juggled between registers. Hold-off from issuing the SAVE_GPR
macro for applicable neighbouring lines to highlight the delicate
register save logic.
Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
V1 -> V2: Update summary
V2 -> V3: Update summary regarding exclusions for the SAVE_GPR marco.
Acknowledge new name for ZEROIZE_GPR{,S} macros.
---
arch/powerpc/kernel/interrupt_64.S | 43 ++++++----------------------
1 file changed, 9 insertions(+), 34 deletions(-)
diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S
index a8065209dd8c..ad302ad93433 100644
--- a/arch/powerpc/kernel/interrupt_64.S
+++ b/arch/powerpc/kernel/interrupt_64.S
@@ -71,12 +71,7 @@ _ASM_NOKPROBE_SYMBOL(system_call_vectored_\name)
mfcr r12
li r11,0
/* Save syscall parameters in r3-r8 */
- std r3,GPR3(r1)
- std r4,GPR4(r1)
- std r5,GPR5(r1)
- std r6,GPR6(r1)
- std r7,GPR7(r1)
- std r8,GPR8(r1)
+ SAVE_GPRS(3, 8, r1)
/* Zero r9-r12, this should only be required when restoring all GPRs */
std r11,GPR9(r1)
std r11,GPR10(r1)
@@ -157,17 +152,10 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
/* Could zero these as per ABI, but we may consider a stricter ABI
* which preserves these if libc implementations can benefit, so
* restore them for now until further measurement is done. */
- ld r0,GPR0(r1)
- ld r4,GPR4(r1)
- ld r5,GPR5(r1)
- ld r6,GPR6(r1)
- ld r7,GPR7(r1)
- ld r8,GPR8(r1)
+ REST_GPR(0, r1)
+ REST_GPRS(4, 8, r1)
/* Zero volatile regs that may contain sensitive kernel data */
- li r9,0
- li r10,0
- li r11,0
- li r12,0
+ ZEROIZE_GPRS(9, 12)
mtspr SPRN_XER,r0
/*
@@ -189,7 +177,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
ld r4,_LINK(r1)
ld r5,_XER(r1)
- ld r0,GPR0(r1)
+ REST_GPR(0, r1)
mtcr r2
mtctr r3
mtlr r4
@@ -257,12 +245,7 @@ END_BTB_FLUSH_SECTION
mfcr r12
li r11,0
/* Save syscall parameters in r3-r8 */
- std r3,GPR3(r1)
- std r4,GPR4(r1)
- std r5,GPR5(r1)
- std r6,GPR6(r1)
- std r7,GPR7(r1)
- std r8,GPR8(r1)
+ SAVE_GPRS(3, 8, r1)
/* Zero r9-r12, this should only be required when restoring all GPRs */
std r11,GPR9(r1)
std r11,GPR10(r1)
@@ -360,16 +343,8 @@ END_FTR_SECTION_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
cmpdi r3,0
bne .Lsyscall_restore_regs
/* Zero volatile regs that may contain sensitive kernel data */
- li r0,0
- li r4,0
- li r5,0
- li r6,0
- li r7,0
- li r8,0
- li r9,0
- li r10,0
- li r11,0
- li r12,0
+ ZEROIZE_GPR(0)
+ ZEROIZE_GPRS(4, 12)
mtctr r0
mtspr SPRN_XER,r0
.Lsyscall_restore_regs_cont:
@@ -394,7 +369,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
ld r4,_XER(r1)
mtctr r3
mtspr SPRN_XER,r4
- ld r0,GPR0(r1)
+ REST_GPR(0, r1)
REST_GPRS(4, 12, r1)
b .Lsyscall_restore_regs_cont
.Lsyscall_rst_end:
--
2.34.1
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v4 15/20] powerpc/64s: Use {ZEROIZE,SAVE,REST}_GPRS macros in sc, scv 0 handlers
2022-08-24 2:05 ` [PATCH v4 15/20] powerpc/64s: Use {ZEROIZE,SAVE,REST}_GPRS macros in sc, scv 0 handlers Rohan McLure
@ 2022-09-12 11:49 ` Nicholas Piggin
0 siblings, 0 replies; 53+ messages in thread
From: Nicholas Piggin @ 2022-09-12 11:49 UTC (permalink / raw)
To: Rohan McLure, linuxppc-dev
On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
> Use the convenience macros for saving/clearing/restoring gprs in keeping
> with syscall calling conventions. The plural variants of these macros
> can store a range of registers for concision.
>
> This works well when the user gpr value we are hoping to save is still
> live. In the syscall interrupt handlers, user register state is
> sometimes juggled between registers. Hold-off from issuing the SAVE_GPR
> macro for applicable neighbouring lines to highlight the delicate
> register save logic.
>
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> V1 -> V2: Update summary
> V2 -> V3: Update summary regarding exclusions for the SAVE_GPR marco.
> Acknowledge new name for ZEROIZE_GPR{,S} macros.
> ---
> arch/powerpc/kernel/interrupt_64.S | 43 ++++++----------------------
> 1 file changed, 9 insertions(+), 34 deletions(-)
>
> diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S
> index a8065209dd8c..ad302ad93433 100644
> --- a/arch/powerpc/kernel/interrupt_64.S
> +++ b/arch/powerpc/kernel/interrupt_64.S
> @@ -71,12 +71,7 @@ _ASM_NOKPROBE_SYMBOL(system_call_vectored_\name)
> mfcr r12
> li r11,0
> /* Save syscall parameters in r3-r8 */
> - std r3,GPR3(r1)
> - std r4,GPR4(r1)
> - std r5,GPR5(r1)
> - std r6,GPR6(r1)
> - std r7,GPR7(r1)
> - std r8,GPR8(r1)
> + SAVE_GPRS(3, 8, r1)
> /* Zero r9-r12, this should only be required when restoring all GPRs */
> std r11,GPR9(r1)
> std r11,GPR10(r1)
> @@ -157,17 +152,10 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> /* Could zero these as per ABI, but we may consider a stricter ABI
> * which preserves these if libc implementations can benefit, so
> * restore them for now until further measurement is done. */
> - ld r0,GPR0(r1)
> - ld r4,GPR4(r1)
> - ld r5,GPR5(r1)
> - ld r6,GPR6(r1)
> - ld r7,GPR7(r1)
> - ld r8,GPR8(r1)
> + REST_GPR(0, r1)
> + REST_GPRS(4, 8, r1)
> /* Zero volatile regs that may contain sensitive kernel data */
> - li r9,0
> - li r10,0
> - li r11,0
> - li r12,0
> + ZEROIZE_GPRS(9, 12)
> mtspr SPRN_XER,r0
>
> /*
> @@ -189,7 +177,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> ld r4,_LINK(r1)
> ld r5,_XER(r1)
>
> - ld r0,GPR0(r1)
> + REST_GPR(0, r1)
> mtcr r2
> mtctr r3
> mtlr r4
> @@ -257,12 +245,7 @@ END_BTB_FLUSH_SECTION
> mfcr r12
> li r11,0
> /* Save syscall parameters in r3-r8 */
> - std r3,GPR3(r1)
> - std r4,GPR4(r1)
> - std r5,GPR5(r1)
> - std r6,GPR6(r1)
> - std r7,GPR7(r1)
> - std r8,GPR8(r1)
> + SAVE_GPRS(3, 8, r1)
> /* Zero r9-r12, this should only be required when restoring all GPRs */
> std r11,GPR9(r1)
> std r11,GPR10(r1)
> @@ -360,16 +343,8 @@ END_FTR_SECTION_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
> cmpdi r3,0
> bne .Lsyscall_restore_regs
> /* Zero volatile regs that may contain sensitive kernel data */
> - li r0,0
> - li r4,0
> - li r5,0
> - li r6,0
> - li r7,0
> - li r8,0
> - li r9,0
> - li r10,0
> - li r11,0
> - li r12,0
> + ZEROIZE_GPR(0)
> + ZEROIZE_GPRS(4, 12)
> mtctr r0
> mtspr SPRN_XER,r0
> .Lsyscall_restore_regs_cont:
> @@ -394,7 +369,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> ld r4,_XER(r1)
> mtctr r3
> mtspr SPRN_XER,r4
> - ld r0,GPR0(r1)
> + REST_GPR(0, r1)
> REST_GPRS(4, 12, r1)
> b .Lsyscall_restore_regs_cont
> .Lsyscall_rst_end:
> --
> 2.34.1
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 16/20] powerpc/32: Clarify interrupt restores with REST_GPR macro in entry_32.S
2022-08-24 2:05 [PATCH v4 00/20] powerpc: Syscall wrapper and register clearing Rohan McLure
` (14 preceding siblings ...)
2022-08-24 2:05 ` [PATCH v4 15/20] powerpc/64s: Use {ZEROIZE,SAVE,REST}_GPRS macros in sc, scv 0 handlers Rohan McLure
@ 2022-08-24 2:05 ` Rohan McLure
2022-08-24 2:05 ` [PATCH v4 17/20] powerpc/64e: Clarify register saves and clears with {SAVE,ZEROIZE}_GPRS Rohan McLure
` (4 subsequent siblings)
20 siblings, 0 replies; 53+ messages in thread
From: Rohan McLure @ 2022-08-24 2:05 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Rohan McLure
Restoring the register state of the interrupted thread involves issuing
a large number of predictable loads to the kernel stack frame. Issue the
REST_GPR{,S} macros to clearly signal when this is happening, and bunch
together restores at the end of the interrupt handler where the saved
value is not consumed earlier in the handler code.
Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
Reported-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
V2 -> V3: New patch.
V3 -> V4: Minimise restores in the unrecoverable window between
restoring SRR0/1 and return from interrupt.
---
arch/powerpc/kernel/entry_32.S | 33 +++++++++++++-------------------
1 file changed, 13 insertions(+), 20 deletions(-)
diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
index 8d6e02ef5dc0..963d646915fe 100644
--- a/arch/powerpc/kernel/entry_32.S
+++ b/arch/powerpc/kernel/entry_32.S
@@ -68,7 +68,7 @@ prepare_transfer_to_handler:
lwz r9,_MSR(r11) /* if sleeping, clear MSR.EE */
rlwinm r9,r9,0,~MSR_EE
lwz r12,_LINK(r11) /* and return to address in LR */
- lwz r2, GPR2(r11)
+ REST_GPR(2, r11)
b fast_exception_return
_ASM_NOKPROBE_SYMBOL(prepare_transfer_to_handler)
#endif /* CONFIG_PPC_BOOK3S_32 || CONFIG_E500 */
@@ -144,7 +144,7 @@ ret_from_syscall:
lwz r7,_NIP(r1)
lwz r8,_MSR(r1)
cmpwi r3,0
- lwz r3,GPR3(r1)
+ REST_GPR(3, r1)
syscall_exit_finish:
mtspr SPRN_SRR0,r7
mtspr SPRN_SRR1,r8
@@ -152,8 +152,8 @@ syscall_exit_finish:
bne 3f
mtcr r5
-1: lwz r2,GPR2(r1)
- lwz r1,GPR1(r1)
+1: REST_GPR(2, r1)
+ REST_GPR(1, r1)
rfi
#ifdef CONFIG_40x
b . /* Prevent prefetch past rfi */
@@ -165,10 +165,8 @@ syscall_exit_finish:
REST_NVGPRS(r1)
mtctr r4
mtxer r5
- lwz r0,GPR0(r1)
- lwz r3,GPR3(r1)
- REST_GPRS(4, 11, r1)
- lwz r12,GPR12(r1)
+ REST_GPR(0, r1)
+ REST_GPRS(3, 12, r1)
b 1b
#ifdef CONFIG_44x
@@ -260,9 +258,8 @@ fast_exception_return:
beq 3f /* if not, we've got problems */
#endif
-2: REST_GPRS(3, 6, r11)
- lwz r10,_CCR(r11)
- REST_GPRS(1, 2, r11)
+2: lwz r10,_CCR(r11)
+ REST_GPRS(1, 6, r11)
mtcr r10
lwz r10,_LINK(r11)
mtlr r10
@@ -277,7 +274,7 @@ fast_exception_return:
mtspr SPRN_SRR0,r12
REST_GPR(9, r11)
REST_GPR(12, r11)
- lwz r11,GPR11(r11)
+ REST_GPR(11, r11)
rfi
#ifdef CONFIG_40x
b . /* Prevent prefetch past rfi */
@@ -454,9 +451,8 @@ _ASM_NOKPROBE_SYMBOL(interrupt_return)
lwz r3,_MSR(r1); \
andi. r3,r3,MSR_PR; \
bne interrupt_return; \
- lwz r0,GPR0(r1); \
- lwz r2,GPR2(r1); \
- REST_GPRS(3, 8, r1); \
+ REST_GPR(0, r1); \
+ REST_GPRS(2, 8, r1); \
lwz r10,_XER(r1); \
lwz r11,_CTR(r1); \
mtspr SPRN_XER,r10; \
@@ -475,11 +471,8 @@ _ASM_NOKPROBE_SYMBOL(interrupt_return)
lwz r12,_MSR(r1); \
mtspr exc_lvl_srr0,r11; \
mtspr exc_lvl_srr1,r12; \
- lwz r9,GPR9(r1); \
- lwz r12,GPR12(r1); \
- lwz r10,GPR10(r1); \
- lwz r11,GPR11(r1); \
- lwz r1,GPR1(r1); \
+ REST_GPRS(9, 12, r1); \
+ REST_GPR(1, r1); \
exc_lvl_rfi; \
b .; /* prevent prefetch past exc_lvl_rfi */
--
2.34.1
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v4 17/20] powerpc/64e: Clarify register saves and clears with {SAVE,ZEROIZE}_GPRS
2022-08-24 2:05 [PATCH v4 00/20] powerpc: Syscall wrapper and register clearing Rohan McLure
` (15 preceding siblings ...)
2022-08-24 2:05 ` [PATCH v4 16/20] powerpc/32: Clarify interrupt restores with REST_GPR macro in entry_32.S Rohan McLure
@ 2022-08-24 2:05 ` Rohan McLure
2022-09-12 12:17 ` Nicholas Piggin
2022-08-24 2:05 ` [PATCH v4 18/20] powerpc/64s: Fix comment on interrupt handler prologue Rohan McLure
` (3 subsequent siblings)
20 siblings, 1 reply; 53+ messages in thread
From: Rohan McLure @ 2022-08-24 2:05 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Rohan McLure
The common interrupt handler prologue macro and the bad_stack
trampolines include consecutive sequences of register saves, and some
register clears. Neaten such instances by expanding use of the SAVE_GPRS
macro and employing the ZEROIZE_GPR macro when appropriate.
Also simplify an invocation of SAVE_GPRS targetting all non-volatile
registers to SAVE_NVGPRS.
Signed-off-by: Rohan Mclure <rmclure@linux.ibm.com>
---
V3 -> V4: New commit.
---
arch/powerpc/kernel/exceptions-64e.S | 27 +++++++++++---------------
1 file changed, 11 insertions(+), 16 deletions(-)
diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
index 67dc4e3179a0..48c640ca425d 100644
--- a/arch/powerpc/kernel/exceptions-64e.S
+++ b/arch/powerpc/kernel/exceptions-64e.S
@@ -216,17 +216,14 @@ END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
mtlr r10
mtcr r11
- ld r10,GPR10(r1)
- ld r11,GPR11(r1)
- ld r12,GPR12(r1)
+ REST_GPRS(10, 12, r1)
mtspr \scratch,r0
std r10,\paca_ex+EX_R10(r13);
std r11,\paca_ex+EX_R11(r13);
ld r10,_NIP(r1)
ld r11,_MSR(r1)
- ld r0,GPR0(r1)
- ld r1,GPR1(r1)
+ REST_GPRS(0, 1, r1)
mtspr \srr0,r10
mtspr \srr1,r11
ld r10,\paca_ex+EX_R10(r13)
@@ -372,16 +369,15 @@ ret_from_mc_except:
/* Core exception code for all exceptions except TLB misses. */
#define EXCEPTION_COMMON_LVL(n, scratch, excf) \
exc_##n##_common: \
- std r0,GPR0(r1); /* save r0 in stackframe */ \
- std r2,GPR2(r1); /* save r2 in stackframe */ \
- SAVE_GPRS(3, 9, r1); /* save r3 - r9 in stackframe */ \
+ SAVE_GPR(0, r1); /* save r0 in stackframe */ \
+ SAVE_GPRS(2, 9, r1); /* save r2 - r9 in stackframe */ \
std r10,_NIP(r1); /* save SRR0 to stackframe */ \
std r11,_MSR(r1); /* save SRR1 to stackframe */ \
beq 2f; /* if from kernel mode */ \
2: ld r3,excf+EX_R10(r13); /* get back r10 */ \
ld r4,excf+EX_R11(r13); /* get back r11 */ \
mfspr r5,scratch; /* get back r13 */ \
- std r12,GPR12(r1); /* save r12 in stackframe */ \
+ SAVE_GPR(12, r1); /* save r12 in stackframe */ \
ld r2,PACATOC(r13); /* get kernel TOC into r2 */ \
mflr r6; /* save LR in stackframe */ \
mfctr r7; /* save CTR in stackframe */ \
@@ -390,7 +386,7 @@ exc_##n##_common: \
lwz r10,excf+EX_CR(r13); /* load orig CR back from PACA */ \
lbz r11,PACAIRQSOFTMASK(r13); /* get current IRQ softe */ \
ld r12,exception_marker@toc(r2); \
- li r0,0; \
+ ZEROIZE_GPR(0); \
std r3,GPR10(r1); /* save r10 to stackframe */ \
std r4,GPR11(r1); /* save r11 to stackframe */ \
std r5,GPR13(r1); /* save it to stackframe */ \
@@ -1056,15 +1052,14 @@ bad_stack_book3e:
mfspr r11,SPRN_ESR
std r10,_DEAR(r1)
std r11,_ESR(r1)
- std r0,GPR0(r1); /* save r0 in stackframe */ \
- std r2,GPR2(r1); /* save r2 in stackframe */ \
- SAVE_GPRS(3, 9, r1); /* save r3 - r9 in stackframe */ \
+ SAVE_GPR(0, r1); /* save r0 in stackframe */ \
+ SAVE_GPRS(2, 9, r1); /* save r2 - r9 in stackframe */ \
ld r3,PACA_EXGEN+EX_R10(r13);/* get back r10 */ \
ld r4,PACA_EXGEN+EX_R11(r13);/* get back r11 */ \
mfspr r5,SPRN_SPRG_GEN_SCRATCH;/* get back r13 XXX can be wrong */ \
std r3,GPR10(r1); /* save r10 to stackframe */ \
std r4,GPR11(r1); /* save r11 to stackframe */ \
- std r12,GPR12(r1); /* save r12 in stackframe */ \
+ SAVE_GPR(12, r1); /* save r12 in stackframe */ \
std r5,GPR13(r1); /* save it to stackframe */ \
mflr r10
mfctr r11
@@ -1072,12 +1067,12 @@ bad_stack_book3e:
std r10,_LINK(r1)
std r11,_CTR(r1)
std r12,_XER(r1)
- SAVE_GPRS(14, 31, r1)
+ SAVE_NVGPRS(r1)
lhz r12,PACA_TRAP_SAVE(r13)
std r12,_TRAP(r1)
addi r11,r1,INT_FRAME_SIZE
std r11,0(r1)
- li r12,0
+ ZEROIZE_GPR(12)
std r12,0(r11)
ld r2,PACATOC(r13)
1: addi r3,r1,STACK_FRAME_OVERHEAD
--
2.34.1
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v4 17/20] powerpc/64e: Clarify register saves and clears with {SAVE,ZEROIZE}_GPRS
2022-08-24 2:05 ` [PATCH v4 17/20] powerpc/64e: Clarify register saves and clears with {SAVE,ZEROIZE}_GPRS Rohan McLure
@ 2022-09-12 12:17 ` Nicholas Piggin
0 siblings, 0 replies; 53+ messages in thread
From: Nicholas Piggin @ 2022-09-12 12:17 UTC (permalink / raw)
To: Rohan McLure, linuxppc-dev
On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
> The common interrupt handler prologue macro and the bad_stack
> trampolines include consecutive sequences of register saves, and some
> register clears. Neaten such instances by expanding use of the SAVE_GPRS
> macro and employing the ZEROIZE_GPR macro when appropriate.
>
> Also simplify an invocation of SAVE_GPRS targetting all non-volatile
> registers to SAVE_NVGPRS.
>
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> Signed-off-by: Rohan Mclure <rmclure@linux.ibm.com>
> ---
> V3 -> V4: New commit.
> ---
> arch/powerpc/kernel/exceptions-64e.S | 27 +++++++++++---------------
> 1 file changed, 11 insertions(+), 16 deletions(-)
>
> diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
> index 67dc4e3179a0..48c640ca425d 100644
> --- a/arch/powerpc/kernel/exceptions-64e.S
> +++ b/arch/powerpc/kernel/exceptions-64e.S
> @@ -216,17 +216,14 @@ END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
> mtlr r10
> mtcr r11
>
> - ld r10,GPR10(r1)
> - ld r11,GPR11(r1)
> - ld r12,GPR12(r1)
> + REST_GPRS(10, 12, r1)
> mtspr \scratch,r0
>
> std r10,\paca_ex+EX_R10(r13);
> std r11,\paca_ex+EX_R11(r13);
> ld r10,_NIP(r1)
> ld r11,_MSR(r1)
> - ld r0,GPR0(r1)
> - ld r1,GPR1(r1)
> + REST_GPRS(0, 1, r1)
> mtspr \srr0,r10
> mtspr \srr1,r11
> ld r10,\paca_ex+EX_R10(r13)
> @@ -372,16 +369,15 @@ ret_from_mc_except:
> /* Core exception code for all exceptions except TLB misses. */
> #define EXCEPTION_COMMON_LVL(n, scratch, excf) \
> exc_##n##_common: \
> - std r0,GPR0(r1); /* save r0 in stackframe */ \
> - std r2,GPR2(r1); /* save r2 in stackframe */ \
> - SAVE_GPRS(3, 9, r1); /* save r3 - r9 in stackframe */ \
> + SAVE_GPR(0, r1); /* save r0 in stackframe */ \
> + SAVE_GPRS(2, 9, r1); /* save r2 - r9 in stackframe */ \
> std r10,_NIP(r1); /* save SRR0 to stackframe */ \
> std r11,_MSR(r1); /* save SRR1 to stackframe */ \
> beq 2f; /* if from kernel mode */ \
> 2: ld r3,excf+EX_R10(r13); /* get back r10 */ \
> ld r4,excf+EX_R11(r13); /* get back r11 */ \
> mfspr r5,scratch; /* get back r13 */ \
> - std r12,GPR12(r1); /* save r12 in stackframe */ \
> + SAVE_GPR(12, r1); /* save r12 in stackframe */ \
> ld r2,PACATOC(r13); /* get kernel TOC into r2 */ \
> mflr r6; /* save LR in stackframe */ \
> mfctr r7; /* save CTR in stackframe */ \
> @@ -390,7 +386,7 @@ exc_##n##_common: \
> lwz r10,excf+EX_CR(r13); /* load orig CR back from PACA */ \
> lbz r11,PACAIRQSOFTMASK(r13); /* get current IRQ softe */ \
> ld r12,exception_marker@toc(r2); \
> - li r0,0; \
> + ZEROIZE_GPR(0); \
> std r3,GPR10(r1); /* save r10 to stackframe */ \
> std r4,GPR11(r1); /* save r11 to stackframe */ \
> std r5,GPR13(r1); /* save it to stackframe */ \
> @@ -1056,15 +1052,14 @@ bad_stack_book3e:
> mfspr r11,SPRN_ESR
> std r10,_DEAR(r1)
> std r11,_ESR(r1)
> - std r0,GPR0(r1); /* save r0 in stackframe */ \
> - std r2,GPR2(r1); /* save r2 in stackframe */ \
> - SAVE_GPRS(3, 9, r1); /* save r3 - r9 in stackframe */ \
> + SAVE_GPR(0, r1); /* save r0 in stackframe */ \
> + SAVE_GPRS(2, 9, r1); /* save r2 - r9 in stackframe */ \
> ld r3,PACA_EXGEN+EX_R10(r13);/* get back r10 */ \
> ld r4,PACA_EXGEN+EX_R11(r13);/* get back r11 */ \
> mfspr r5,SPRN_SPRG_GEN_SCRATCH;/* get back r13 XXX can be wrong */ \
> std r3,GPR10(r1); /* save r10 to stackframe */ \
> std r4,GPR11(r1); /* save r11 to stackframe */ \
> - std r12,GPR12(r1); /* save r12 in stackframe */ \
> + SAVE_GPR(12, r1); /* save r12 in stackframe */ \
> std r5,GPR13(r1); /* save it to stackframe */ \
> mflr r10
> mfctr r11
> @@ -1072,12 +1067,12 @@ bad_stack_book3e:
> std r10,_LINK(r1)
> std r11,_CTR(r1)
> std r12,_XER(r1)
> - SAVE_GPRS(14, 31, r1)
> + SAVE_NVGPRS(r1)
> lhz r12,PACA_TRAP_SAVE(r13)
> std r12,_TRAP(r1)
> addi r11,r1,INT_FRAME_SIZE
> std r11,0(r1)
> - li r12,0
> + ZEROIZE_GPR(12)
> std r12,0(r11)
> ld r2,PACATOC(r13)
> 1: addi r3,r1,STACK_FRAME_OVERHEAD
> --
> 2.34.1
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 18/20] powerpc/64s: Fix comment on interrupt handler prologue
2022-08-24 2:05 [PATCH v4 00/20] powerpc: Syscall wrapper and register clearing Rohan McLure
` (16 preceding siblings ...)
2022-08-24 2:05 ` [PATCH v4 17/20] powerpc/64e: Clarify register saves and clears with {SAVE,ZEROIZE}_GPRS Rohan McLure
@ 2022-08-24 2:05 ` Rohan McLure
2022-09-12 11:51 ` Nicholas Piggin
2022-08-24 2:05 ` [PATCH v4 19/20] powerpc/64s: Clear gprs on interrupt routine entry in Book3S Rohan McLure
` (2 subsequent siblings)
20 siblings, 1 reply; 53+ messages in thread
From: Rohan McLure @ 2022-08-24 2:05 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Rohan McLure
Interrupt handlers on 64s systems will often need to save register state
from the interrupted process to make space for loading special purpose
registers or for internal state.
Fix a comment documenting a common code path macro in the beginning of
interrupt handlers where r10 is saved to the PACA to afford space for
the value of the CFAR. Comment is currently written as if r10-r12 are
saved to PACA, but in fact only r10 is saved, with r11-r12 saved much
later. The distance in code between these saves has grown over the many
revisions of this macro. Fix this by signalling with a comment where
r11-r12 are saved to the PACA.
Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
V1 -> V2: Given its own commit
V2 -> V3: Annotate r11-r12 save locations with comment.
---
arch/powerpc/kernel/exceptions-64s.S | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index 3d0dc133a9ae..a3b51441b039 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -281,7 +281,7 @@ BEGIN_FTR_SECTION
mfspr r9,SPRN_PPR
END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
HMT_MEDIUM
- std r10,IAREA+EX_R10(r13) /* save r10 - r12 */
+ std r10,IAREA+EX_R10(r13) /* save r10 */
.if ICFAR
BEGIN_FTR_SECTION
mfspr r10,SPRN_CFAR
@@ -321,7 +321,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_CFAR)
mfctr r10
std r10,IAREA+EX_CTR(r13)
mfcr r9
- std r11,IAREA+EX_R11(r13)
+ std r11,IAREA+EX_R11(r13) /* save r11 - r12 */
std r12,IAREA+EX_R12(r13)
/*
--
2.34.1
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v4 18/20] powerpc/64s: Fix comment on interrupt handler prologue
2022-08-24 2:05 ` [PATCH v4 18/20] powerpc/64s: Fix comment on interrupt handler prologue Rohan McLure
@ 2022-09-12 11:51 ` Nicholas Piggin
0 siblings, 0 replies; 53+ messages in thread
From: Nicholas Piggin @ 2022-09-12 11:51 UTC (permalink / raw)
To: Rohan McLure, linuxppc-dev
On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
> Interrupt handlers on 64s systems will often need to save register state
> from the interrupted process to make space for loading special purpose
> registers or for internal state.
>
> Fix a comment documenting a common code path macro in the beginning of
> interrupt handlers where r10 is saved to the PACA to afford space for
> the value of the CFAR. Comment is currently written as if r10-r12 are
> saved to PACA, but in fact only r10 is saved, with r11-r12 saved much
> later. The distance in code between these saves has grown over the many
> revisions of this macro. Fix this by signalling with a comment where
> r11-r12 are saved to the PACA.
Wondering if this kind of comment is useful at all really... But it is
better than broken comment.
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> V1 -> V2: Given its own commit
> V2 -> V3: Annotate r11-r12 save locations with comment.
> ---
> arch/powerpc/kernel/exceptions-64s.S | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index 3d0dc133a9ae..a3b51441b039 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -281,7 +281,7 @@ BEGIN_FTR_SECTION
> mfspr r9,SPRN_PPR
> END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> HMT_MEDIUM
> - std r10,IAREA+EX_R10(r13) /* save r10 - r12 */
> + std r10,IAREA+EX_R10(r13) /* save r10 */
> .if ICFAR
> BEGIN_FTR_SECTION
> mfspr r10,SPRN_CFAR
> @@ -321,7 +321,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_CFAR)
> mfctr r10
> std r10,IAREA+EX_CTR(r13)
> mfcr r9
> - std r11,IAREA+EX_R11(r13)
> + std r11,IAREA+EX_R11(r13) /* save r11 - r12 */
> std r12,IAREA+EX_R12(r13)
>
> /*
> --
> 2.34.1
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 19/20] powerpc/64s: Clear gprs on interrupt routine entry in Book3S
2022-08-24 2:05 [PATCH v4 00/20] powerpc: Syscall wrapper and register clearing Rohan McLure
` (17 preceding siblings ...)
2022-08-24 2:05 ` [PATCH v4 18/20] powerpc/64s: Fix comment on interrupt handler prologue Rohan McLure
@ 2022-08-24 2:05 ` Rohan McLure
2022-09-12 12:15 ` Nicholas Piggin
2022-08-24 2:05 ` [PATCH v4 20/20] powerpc/64e: Clear gprs on interrupt routine entry Rohan McLure
2022-09-12 0:55 ` [PATCH v4 00/20] powerpc: Syscall wrapper and register clearing Rohan McLure
20 siblings, 1 reply; 53+ messages in thread
From: Rohan McLure @ 2022-08-24 2:05 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Rohan McLure
Zero GPRS r0, r2-r11, r14-r31, on entry into the kernel for all
other interrupt sources to limit influence of user-space values
in potential speculation gadgets. The remaining gprs are overwritten by
entry macros to interrupt handlers, irrespective of whether or not a
given handler consumes these register values.
Prior to this commit, r14-r31 are restored on a per-interrupt basis at
exit, but now they are always restored. Remove explicit REST_NVGPRS
invocations as non-volatiles must now always be restored. 32-bit systems
do not clear user registers on interrupt, and continue to depend on the
return value of interrupt_exit_user_prepare to determine whether or not
to restore non-volatiles.
The mmap_bench benchmark in selftests should rapidly invoke pagefaults.
See ~0.8% performance regression with this mitigation, but this
indicates the worst-case performance due to heavier-weight interrupt
handlers.
Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
V1 -> V2: Add benchmark data
V2 -> V3: Use ZEROIZE_GPR{,S} macro renames, clarify
interrupt_exit_user_prepare changes in summary.
---
arch/powerpc/kernel/exceptions-64s.S | 21 ++++++++-------------
arch/powerpc/kernel/interrupt_64.S | 9 ++-------
2 files changed, 10 insertions(+), 20 deletions(-)
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index a3b51441b039..038e42fb2182 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -502,6 +502,7 @@ DEFINE_FIXED_SYMBOL(\name\()_common_real, text)
std r10,0(r1) /* make stack chain pointer */
std r0,GPR0(r1) /* save r0 in stackframe */
std r10,GPR1(r1) /* save r1 in stackframe */
+ ZEROIZE_GPR(0)
/* Mark our [H]SRRs valid for return */
li r10,1
@@ -538,14 +539,18 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
ld r10,IAREA+EX_R10(r13)
std r9,GPR9(r1)
std r10,GPR10(r1)
+ ZEROIZE_GPRS(9, 10)
ld r9,IAREA+EX_R11(r13) /* move r11 - r13 to stackframe */
ld r10,IAREA+EX_R12(r13)
ld r11,IAREA+EX_R13(r13)
std r9,GPR11(r1)
std r10,GPR12(r1)
std r11,GPR13(r1)
+ /* keep r12 ([H]SRR1/MSR), r13 (PACA) for interrupt routine */
+ ZEROIZE_GPR(11)
SAVE_NVGPRS(r1)
+ ZEROIZE_NVGPRS()
.if IDAR
.if IISIDE
@@ -577,8 +582,8 @@ BEGIN_FTR_SECTION
END_FTR_SECTION_IFSET(CPU_FTR_CFAR)
ld r10,IAREA+EX_CTR(r13)
std r10,_CTR(r1)
- std r2,GPR2(r1) /* save r2 in stackframe */
- SAVE_GPRS(3, 8, r1) /* save r3 - r8 in stackframe */
+ SAVE_GPRS(2, 8, r1) /* save r2 - r8 in stackframe */
+ ZEROIZE_GPRS(2, 8)
mflr r9 /* Get LR, later save to stack */
ld r2,PACATOC(r13) /* get kernel TOC into r2 */
std r9,_LINK(r1)
@@ -696,6 +701,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_CFAR)
mtlr r9
ld r9,_CCR(r1)
mtcr r9
+ REST_NVGPRS(r1)
REST_GPRS(2, 13, r1)
REST_GPR(0, r1)
/* restore original r1. */
@@ -1368,11 +1374,6 @@ ALT_MMU_FTR_SECTION_END_IFCLR(MMU_FTR_TYPE_RADIX)
b interrupt_return_srr
1: bl do_break
- /*
- * do_break() may have changed the NV GPRS while handling a breakpoint.
- * If so, we need to restore them with their updated values.
- */
- REST_NVGPRS(r1)
b interrupt_return_srr
@@ -1598,7 +1599,6 @@ EXC_COMMON_BEGIN(alignment_common)
GEN_COMMON alignment
addi r3,r1,STACK_FRAME_OVERHEAD
bl alignment_exception
- REST_NVGPRS(r1) /* instruction emulation may change GPRs */
b interrupt_return_srr
@@ -1708,7 +1708,6 @@ EXC_COMMON_BEGIN(program_check_common)
.Ldo_program_check:
addi r3,r1,STACK_FRAME_OVERHEAD
bl program_check_exception
- REST_NVGPRS(r1) /* instruction emulation may change GPRs */
b interrupt_return_srr
@@ -2139,7 +2138,6 @@ EXC_COMMON_BEGIN(emulation_assist_common)
GEN_COMMON emulation_assist
addi r3,r1,STACK_FRAME_OVERHEAD
bl emulation_assist_interrupt
- REST_NVGPRS(r1) /* instruction emulation may change GPRs */
b interrupt_return_hsrr
@@ -2457,7 +2455,6 @@ EXC_COMMON_BEGIN(facility_unavailable_common)
GEN_COMMON facility_unavailable
addi r3,r1,STACK_FRAME_OVERHEAD
bl facility_unavailable_exception
- REST_NVGPRS(r1) /* instruction emulation may change GPRs */
b interrupt_return_srr
@@ -2485,7 +2482,6 @@ EXC_COMMON_BEGIN(h_facility_unavailable_common)
GEN_COMMON h_facility_unavailable
addi r3,r1,STACK_FRAME_OVERHEAD
bl facility_unavailable_exception
- REST_NVGPRS(r1) /* XXX Shouldn't be necessary in practice */
b interrupt_return_hsrr
@@ -2711,7 +2707,6 @@ EXC_COMMON_BEGIN(altivec_assist_common)
addi r3,r1,STACK_FRAME_OVERHEAD
#ifdef CONFIG_ALTIVEC
bl altivec_assist_exception
- REST_NVGPRS(r1) /* instruction emulation may change GPRs */
#else
bl unknown_exception
#endif
diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S
index ad302ad93433..f9ee93e3a0d3 100644
--- a/arch/powerpc/kernel/interrupt_64.S
+++ b/arch/powerpc/kernel/interrupt_64.S
@@ -432,9 +432,6 @@ interrupt_return_\srr\()_user: /* make backtraces match the _kernel variant */
_ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_user)
addi r3,r1,STACK_FRAME_OVERHEAD
bl interrupt_exit_user_prepare
- cmpdi r3,0
- bne- .Lrestore_nvgprs_\srr
-.Lrestore_nvgprs_\srr\()_cont:
std r1,PACA_EXIT_SAVE_R1(r13) /* save r1 for restart */
#ifdef CONFIG_PPC_BOOK3S
.Linterrupt_return_\srr\()_user_rst_start:
@@ -448,6 +445,7 @@ _ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_user)
stb r11,PACAIRQHAPPENED(r13) # clear out possible HARD_DIS
.Lfast_user_interrupt_return_\srr\():
+ REST_NVGPRS(r1)
#ifdef CONFIG_PPC_BOOK3S
.ifc \srr,srr
lbz r4,PACASRR_VALID(r13)
@@ -517,10 +515,6 @@ ALT_FTR_SECTION_END_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
b . /* prevent speculative execution */
.Linterrupt_return_\srr\()_user_rst_end:
-.Lrestore_nvgprs_\srr\():
- REST_NVGPRS(r1)
- b .Lrestore_nvgprs_\srr\()_cont
-
#ifdef CONFIG_PPC_BOOK3S
interrupt_return_\srr\()_user_restart:
_ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_user_restart)
@@ -561,6 +555,7 @@ _ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_kernel)
1:
.Lfast_kernel_interrupt_return_\srr\():
+ REST_NVGPRS(r1)
cmpdi cr1,r3,0
#ifdef CONFIG_PPC_BOOK3S
.ifc \srr,srr
--
2.34.1
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v4 19/20] powerpc/64s: Clear gprs on interrupt routine entry in Book3S
2022-08-24 2:05 ` [PATCH v4 19/20] powerpc/64s: Clear gprs on interrupt routine entry in Book3S Rohan McLure
@ 2022-09-12 12:15 ` Nicholas Piggin
2022-09-15 6:55 ` Rohan McLure
0 siblings, 1 reply; 53+ messages in thread
From: Nicholas Piggin @ 2022-09-12 12:15 UTC (permalink / raw)
To: Rohan McLure, linuxppc-dev
On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
> Zero GPRS r0, r2-r11, r14-r31, on entry into the kernel for all
> other interrupt sources to limit influence of user-space values
> in potential speculation gadgets. The remaining gprs are overwritten by
> entry macros to interrupt handlers, irrespective of whether or not a
> given handler consumes these register values.
>
> Prior to this commit, r14-r31 are restored on a per-interrupt basis at
> exit, but now they are always restored. Remove explicit REST_NVGPRS
> invocations as non-volatiles must now always be restored. 32-bit systems
> do not clear user registers on interrupt, and continue to depend on the
> return value of interrupt_exit_user_prepare to determine whether or not
> to restore non-volatiles.
>
> The mmap_bench benchmark in selftests should rapidly invoke pagefaults.
> See ~0.8% performance regression with this mitigation, but this
> indicates the worst-case performance due to heavier-weight interrupt
> handlers.
Ow, my heart :(
Are we not keeping a CONFIG option to rid ourselves of this vile
performance robbing thing? Are we getting rid of the whole
_TIF_RESTOREALL thing too, or does PPC32 want to keep it?
>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> V1 -> V2: Add benchmark data
> V2 -> V3: Use ZEROIZE_GPR{,S} macro renames, clarify
> interrupt_exit_user_prepare changes in summary.
> ---
> arch/powerpc/kernel/exceptions-64s.S | 21 ++++++++-------------
> arch/powerpc/kernel/interrupt_64.S | 9 ++-------
> 2 files changed, 10 insertions(+), 20 deletions(-)
>
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index a3b51441b039..038e42fb2182 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -502,6 +502,7 @@ DEFINE_FIXED_SYMBOL(\name\()_common_real, text)
> std r10,0(r1) /* make stack chain pointer */
> std r0,GPR0(r1) /* save r0 in stackframe */
> std r10,GPR1(r1) /* save r1 in stackframe */
> + ZEROIZE_GPR(0)
>
> /* Mark our [H]SRRs valid for return */
> li r10,1
> @@ -538,14 +539,18 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> ld r10,IAREA+EX_R10(r13)
> std r9,GPR9(r1)
> std r10,GPR10(r1)
> + ZEROIZE_GPRS(9, 10)
You use 9/10 right afterwards, this'd have to move down to where
you zero r11 at least.
> ld r9,IAREA+EX_R11(r13) /* move r11 - r13 to stackframe */
> ld r10,IAREA+EX_R12(r13)
> ld r11,IAREA+EX_R13(r13)
> std r9,GPR11(r1)
> std r10,GPR12(r1)
> std r11,GPR13(r1)
> + /* keep r12 ([H]SRR1/MSR), r13 (PACA) for interrupt routine */
> + ZEROIZE_GPR(11)
Kernel always has to keep r13 so no need to comment that. Keeping r11,
is that for those annoying fp_unavailable etc handlers?
There's probably not much a user can do with this, given they're set
from the MSR. Use can influence some bits of its MSR though. So long
as we're being paranoid, you could add an IOPTION to retain r11 only for
the handlers that need it, or have them load it from MSR and zero it
here.
Thanks,
Nick
>
> SAVE_NVGPRS(r1)
> + ZEROIZE_NVGPRS()
>
> .if IDAR
> .if IISIDE
> @@ -577,8 +582,8 @@ BEGIN_FTR_SECTION
> END_FTR_SECTION_IFSET(CPU_FTR_CFAR)
> ld r10,IAREA+EX_CTR(r13)
> std r10,_CTR(r1)
> - std r2,GPR2(r1) /* save r2 in stackframe */
> - SAVE_GPRS(3, 8, r1) /* save r3 - r8 in stackframe */
> + SAVE_GPRS(2, 8, r1) /* save r2 - r8 in stackframe */
> + ZEROIZE_GPRS(2, 8)
> mflr r9 /* Get LR, later save to stack */
> ld r2,PACATOC(r13) /* get kernel TOC into r2 */
> std r9,_LINK(r1)
> @@ -696,6 +701,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_CFAR)
> mtlr r9
> ld r9,_CCR(r1)
> mtcr r9
> + REST_NVGPRS(r1)
> REST_GPRS(2, 13, r1)
> REST_GPR(0, r1)
> /* restore original r1. */
> @@ -1368,11 +1374,6 @@ ALT_MMU_FTR_SECTION_END_IFCLR(MMU_FTR_TYPE_RADIX)
> b interrupt_return_srr
>
> 1: bl do_break
> - /*
> - * do_break() may have changed the NV GPRS while handling a breakpoint.
> - * If so, we need to restore them with their updated values.
> - */
> - REST_NVGPRS(r1)
> b interrupt_return_srr
>
>
> @@ -1598,7 +1599,6 @@ EXC_COMMON_BEGIN(alignment_common)
> GEN_COMMON alignment
> addi r3,r1,STACK_FRAME_OVERHEAD
> bl alignment_exception
> - REST_NVGPRS(r1) /* instruction emulation may change GPRs */
> b interrupt_return_srr
>
>
> @@ -1708,7 +1708,6 @@ EXC_COMMON_BEGIN(program_check_common)
> .Ldo_program_check:
> addi r3,r1,STACK_FRAME_OVERHEAD
> bl program_check_exception
> - REST_NVGPRS(r1) /* instruction emulation may change GPRs */
> b interrupt_return_srr
>
>
> @@ -2139,7 +2138,6 @@ EXC_COMMON_BEGIN(emulation_assist_common)
> GEN_COMMON emulation_assist
> addi r3,r1,STACK_FRAME_OVERHEAD
> bl emulation_assist_interrupt
> - REST_NVGPRS(r1) /* instruction emulation may change GPRs */
> b interrupt_return_hsrr
>
>
> @@ -2457,7 +2455,6 @@ EXC_COMMON_BEGIN(facility_unavailable_common)
> GEN_COMMON facility_unavailable
> addi r3,r1,STACK_FRAME_OVERHEAD
> bl facility_unavailable_exception
> - REST_NVGPRS(r1) /* instruction emulation may change GPRs */
> b interrupt_return_srr
>
>
> @@ -2485,7 +2482,6 @@ EXC_COMMON_BEGIN(h_facility_unavailable_common)
> GEN_COMMON h_facility_unavailable
> addi r3,r1,STACK_FRAME_OVERHEAD
> bl facility_unavailable_exception
> - REST_NVGPRS(r1) /* XXX Shouldn't be necessary in practice */
> b interrupt_return_hsrr
>
>
> @@ -2711,7 +2707,6 @@ EXC_COMMON_BEGIN(altivec_assist_common)
> addi r3,r1,STACK_FRAME_OVERHEAD
> #ifdef CONFIG_ALTIVEC
> bl altivec_assist_exception
> - REST_NVGPRS(r1) /* instruction emulation may change GPRs */
> #else
> bl unknown_exception
> #endif
> diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S
> index ad302ad93433..f9ee93e3a0d3 100644
> --- a/arch/powerpc/kernel/interrupt_64.S
> +++ b/arch/powerpc/kernel/interrupt_64.S
> @@ -432,9 +432,6 @@ interrupt_return_\srr\()_user: /* make backtraces match the _kernel variant */
> _ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_user)
> addi r3,r1,STACK_FRAME_OVERHEAD
> bl interrupt_exit_user_prepare
> - cmpdi r3,0
> - bne- .Lrestore_nvgprs_\srr
> -.Lrestore_nvgprs_\srr\()_cont:
> std r1,PACA_EXIT_SAVE_R1(r13) /* save r1 for restart */
> #ifdef CONFIG_PPC_BOOK3S
> .Linterrupt_return_\srr\()_user_rst_start:
> @@ -448,6 +445,7 @@ _ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_user)
> stb r11,PACAIRQHAPPENED(r13) # clear out possible HARD_DIS
>
> .Lfast_user_interrupt_return_\srr\():
> + REST_NVGPRS(r1)
> #ifdef CONFIG_PPC_BOOK3S
> .ifc \srr,srr
> lbz r4,PACASRR_VALID(r13)
> @@ -517,10 +515,6 @@ ALT_FTR_SECTION_END_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
> b . /* prevent speculative execution */
> .Linterrupt_return_\srr\()_user_rst_end:
>
> -.Lrestore_nvgprs_\srr\():
> - REST_NVGPRS(r1)
> - b .Lrestore_nvgprs_\srr\()_cont
> -
> #ifdef CONFIG_PPC_BOOK3S
> interrupt_return_\srr\()_user_restart:
> _ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_user_restart)
> @@ -561,6 +555,7 @@ _ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_kernel)
> 1:
>
> .Lfast_kernel_interrupt_return_\srr\():
> + REST_NVGPRS(r1)
> cmpdi cr1,r3,0
> #ifdef CONFIG_PPC_BOOK3S
> .ifc \srr,srr
> --
> 2.34.1
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 19/20] powerpc/64s: Clear gprs on interrupt routine entry in Book3S
2022-09-12 12:15 ` Nicholas Piggin
@ 2022-09-15 6:55 ` Rohan McLure
2022-09-16 0:43 ` Nicholas Piggin
0 siblings, 1 reply; 53+ messages in thread
From: Rohan McLure @ 2022-09-15 6:55 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linuxppc-dev
> On 12 Sep 2022, at 10:15 pm, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
>> Zero GPRS r0, r2-r11, r14-r31, on entry into the kernel for all
>> other interrupt sources to limit influence of user-space values
>> in potential speculation gadgets. The remaining gprs are overwritten by
>> entry macros to interrupt handlers, irrespective of whether or not a
>> given handler consumes these register values.
>>
>> Prior to this commit, r14-r31 are restored on a per-interrupt basis at
>> exit, but now they are always restored. Remove explicit REST_NVGPRS
>> invocations as non-volatiles must now always be restored. 32-bit systems
>> do not clear user registers on interrupt, and continue to depend on the
>> return value of interrupt_exit_user_prepare to determine whether or not
>> to restore non-volatiles.
>>
>> The mmap_bench benchmark in selftests should rapidly invoke pagefaults.
>> See ~0.8% performance regression with this mitigation, but this
>> indicates the worst-case performance due to heavier-weight interrupt
>> handlers.
>
> Ow, my heart :(
>
> Are we not keeping a CONFIG option to rid ourselves of this vile
> performance robbing thing? Are we getting rid of the whole
> _TIF_RESTOREALL thing too, or does PPC32 want to keep it?
I see no reason not to include a CONFIG option for this
mitigation here other than simplicity. Any suggestions for a name?
I’m thinking PPC64_SANITIZE_INTERRUPTS. Defaults on Book3E_64, optional
on Book3S_64.
>>
>> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
>> ---
>> V1 -> V2: Add benchmark data
>> V2 -> V3: Use ZEROIZE_GPR{,S} macro renames, clarify
>> interrupt_exit_user_prepare changes in summary.
>> ---
>> arch/powerpc/kernel/exceptions-64s.S | 21 ++++++++-------------
>> arch/powerpc/kernel/interrupt_64.S | 9 ++-------
>> 2 files changed, 10 insertions(+), 20 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
>> index a3b51441b039..038e42fb2182 100644
>> --- a/arch/powerpc/kernel/exceptions-64s.S
>> +++ b/arch/powerpc/kernel/exceptions-64s.S
>> @@ -502,6 +502,7 @@ DEFINE_FIXED_SYMBOL(\name\()_common_real, text)
>> std r10,0(r1) /* make stack chain pointer */
>> std r0,GPR0(r1) /* save r0 in stackframe */
>> std r10,GPR1(r1) /* save r1 in stackframe */
>> + ZEROIZE_GPR(0)
>>
>> /* Mark our [H]SRRs valid for return */
>> li r10,1
>> @@ -538,14 +539,18 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>> ld r10,IAREA+EX_R10(r13)
>> std r9,GPR9(r1)
>> std r10,GPR10(r1)
>> + ZEROIZE_GPRS(9, 10)
>
> You use 9/10 right afterwards, this'd have to move down to where
> you zero r11 at least.
>
>> ld r9,IAREA+EX_R11(r13) /* move r11 - r13 to stackframe */
>> ld r10,IAREA+EX_R12(r13)
>> ld r11,IAREA+EX_R13(r13)
>> std r9,GPR11(r1)
>> std r10,GPR12(r1)
>> std r11,GPR13(r1)
>> + /* keep r12 ([H]SRR1/MSR), r13 (PACA) for interrupt routine */
>> + ZEROIZE_GPR(11)
>
> Kernel always has to keep r13 so no need to comment that. Keeping r11,
> is that for those annoying fp_unavailable etc handlers?
>
> There's probably not much a user can do with this, given they're set
> from the MSR. Use can influence some bits of its MSR though. So long
> as we're being paranoid, you could add an IOPTION to retain r11 only for
> the handlers that need it, or have them load it from MSR and zero it
> here.
Good suggestion. Presume you’re referring to r12 here. I might go the
IOPTION route.
>
> Thanks,
> Nick
>
>>
>> SAVE_NVGPRS(r1)
>> + ZEROIZE_NVGPRS()
>>
>> .if IDAR
>> .if IISIDE
>> @@ -577,8 +582,8 @@ BEGIN_FTR_SECTION
>> END_FTR_SECTION_IFSET(CPU_FTR_CFAR)
>> ld r10,IAREA+EX_CTR(r13)
>> std r10,_CTR(r1)
>> - std r2,GPR2(r1) /* save r2 in stackframe */
>> - SAVE_GPRS(3, 8, r1) /* save r3 - r8 in stackframe */
>> + SAVE_GPRS(2, 8, r1) /* save r2 - r8 in stackframe */
>> + ZEROIZE_GPRS(2, 8)
>> mflr r9 /* Get LR, later save to stack */
>> ld r2,PACATOC(r13) /* get kernel TOC into r2 */
>> std r9,_LINK(r1)
>> @@ -696,6 +701,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_CFAR)
>> mtlr r9
>> ld r9,_CCR(r1)
>> mtcr r9
>> + REST_NVGPRS(r1)
>> REST_GPRS(2, 13, r1)
>> REST_GPR(0, r1)
>> /* restore original r1. */
>> @@ -1368,11 +1374,6 @@ ALT_MMU_FTR_SECTION_END_IFCLR(MMU_FTR_TYPE_RADIX)
>> b interrupt_return_srr
>>
>> 1: bl do_break
>> - /*
>> - * do_break() may have changed the NV GPRS while handling a breakpoint.
>> - * If so, we need to restore them with their updated values.
>> - */
>> - REST_NVGPRS(r1)
>> b interrupt_return_srr
>>
>>
>> @@ -1598,7 +1599,6 @@ EXC_COMMON_BEGIN(alignment_common)
>> GEN_COMMON alignment
>> addi r3,r1,STACK_FRAME_OVERHEAD
>> bl alignment_exception
>> - REST_NVGPRS(r1) /* instruction emulation may change GPRs */
>> b interrupt_return_srr
>>
>>
>> @@ -1708,7 +1708,6 @@ EXC_COMMON_BEGIN(program_check_common)
>> .Ldo_program_check:
>> addi r3,r1,STACK_FRAME_OVERHEAD
>> bl program_check_exception
>> - REST_NVGPRS(r1) /* instruction emulation may change GPRs */
>> b interrupt_return_srr
>>
>>
>> @@ -2139,7 +2138,6 @@ EXC_COMMON_BEGIN(emulation_assist_common)
>> GEN_COMMON emulation_assist
>> addi r3,r1,STACK_FRAME_OVERHEAD
>> bl emulation_assist_interrupt
>> - REST_NVGPRS(r1) /* instruction emulation may change GPRs */
>> b interrupt_return_hsrr
>>
>>
>> @@ -2457,7 +2455,6 @@ EXC_COMMON_BEGIN(facility_unavailable_common)
>> GEN_COMMON facility_unavailable
>> addi r3,r1,STACK_FRAME_OVERHEAD
>> bl facility_unavailable_exception
>> - REST_NVGPRS(r1) /* instruction emulation may change GPRs */
>> b interrupt_return_srr
>>
>>
>> @@ -2485,7 +2482,6 @@ EXC_COMMON_BEGIN(h_facility_unavailable_common)
>> GEN_COMMON h_facility_unavailable
>> addi r3,r1,STACK_FRAME_OVERHEAD
>> bl facility_unavailable_exception
>> - REST_NVGPRS(r1) /* XXX Shouldn't be necessary in practice */
>> b interrupt_return_hsrr
>>
>>
>> @@ -2711,7 +2707,6 @@ EXC_COMMON_BEGIN(altivec_assist_common)
>> addi r3,r1,STACK_FRAME_OVERHEAD
>> #ifdef CONFIG_ALTIVEC
>> bl altivec_assist_exception
>> - REST_NVGPRS(r1) /* instruction emulation may change GPRs */
>> #else
>> bl unknown_exception
>> #endif
>> diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S
>> index ad302ad93433..f9ee93e3a0d3 100644
>> --- a/arch/powerpc/kernel/interrupt_64.S
>> +++ b/arch/powerpc/kernel/interrupt_64.S
>> @@ -432,9 +432,6 @@ interrupt_return_\srr\()_user: /* make backtraces match the _kernel variant */
>> _ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_user)
>> addi r3,r1,STACK_FRAME_OVERHEAD
>> bl interrupt_exit_user_prepare
>> - cmpdi r3,0
>> - bne- .Lrestore_nvgprs_\srr
>> -.Lrestore_nvgprs_\srr\()_cont:
>> std r1,PACA_EXIT_SAVE_R1(r13) /* save r1 for restart */
>> #ifdef CONFIG_PPC_BOOK3S
>> .Linterrupt_return_\srr\()_user_rst_start:
>> @@ -448,6 +445,7 @@ _ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_user)
>> stb r11,PACAIRQHAPPENED(r13) # clear out possible HARD_DIS
>>
>> .Lfast_user_interrupt_return_\srr\():
>> + REST_NVGPRS(r1)
>> #ifdef CONFIG_PPC_BOOK3S
>> .ifc \srr,srr
>> lbz r4,PACASRR_VALID(r13)
>> @@ -517,10 +515,6 @@ ALT_FTR_SECTION_END_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
>> b . /* prevent speculative execution */
>> .Linterrupt_return_\srr\()_user_rst_end:
>>
>> -.Lrestore_nvgprs_\srr\():
>> - REST_NVGPRS(r1)
>> - b .Lrestore_nvgprs_\srr\()_cont
>> -
>> #ifdef CONFIG_PPC_BOOK3S
>> interrupt_return_\srr\()_user_restart:
>> _ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_user_restart)
>> @@ -561,6 +555,7 @@ _ASM_NOKPROBE_SYMBOL(interrupt_return_\srr\()_kernel)
>> 1:
>>
>> .Lfast_kernel_interrupt_return_\srr\():
>> + REST_NVGPRS(r1)
>> cmpdi cr1,r3,0
>> #ifdef CONFIG_PPC_BOOK3S
>> .ifc \srr,srr
>> --
>> 2.34.1
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 19/20] powerpc/64s: Clear gprs on interrupt routine entry in Book3S
2022-09-15 6:55 ` Rohan McLure
@ 2022-09-16 0:43 ` Nicholas Piggin
0 siblings, 0 replies; 53+ messages in thread
From: Nicholas Piggin @ 2022-09-16 0:43 UTC (permalink / raw)
To: Rohan McLure; +Cc: linuxppc-dev
On Thu Sep 15, 2022 at 4:55 PM AEST, Rohan McLure wrote:
>
>
> > On 12 Sep 2022, at 10:15 pm, Nicholas Piggin <npiggin@gmail.com> wrote:
> >
> > On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
> >> Zero GPRS r0, r2-r11, r14-r31, on entry into the kernel for all
> >> other interrupt sources to limit influence of user-space values
> >> in potential speculation gadgets. The remaining gprs are overwritten by
> >> entry macros to interrupt handlers, irrespective of whether or not a
> >> given handler consumes these register values.
> >>
> >> Prior to this commit, r14-r31 are restored on a per-interrupt basis at
> >> exit, but now they are always restored. Remove explicit REST_NVGPRS
> >> invocations as non-volatiles must now always be restored. 32-bit systems
> >> do not clear user registers on interrupt, and continue to depend on the
> >> return value of interrupt_exit_user_prepare to determine whether or not
> >> to restore non-volatiles.
> >>
> >> The mmap_bench benchmark in selftests should rapidly invoke pagefaults.
> >> See ~0.8% performance regression with this mitigation, but this
> >> indicates the worst-case performance due to heavier-weight interrupt
> >> handlers.
> >
> > Ow, my heart :(
> >
> > Are we not keeping a CONFIG option to rid ourselves of this vile
> > performance robbing thing? Are we getting rid of the whole
> > _TIF_RESTOREALL thing too, or does PPC32 want to keep it?
>
> I see no reason not to include a CONFIG option for this
> mitigation here other than simplicity. Any suggestions for a name?
> I’m thinking PPC64_SANITIZE_INTERRUPTS. Defaults on Book3E_64, optional
> on Book3S_64.
INTERRUPT_SANITIZE_REGISTERS perhaps?
>
> >>
> >> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> >> ---
> >> V1 -> V2: Add benchmark data
> >> V2 -> V3: Use ZEROIZE_GPR{,S} macro renames, clarify
> >> interrupt_exit_user_prepare changes in summary.
> >> ---
> >> arch/powerpc/kernel/exceptions-64s.S | 21 ++++++++-------------
> >> arch/powerpc/kernel/interrupt_64.S | 9 ++-------
> >> 2 files changed, 10 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> >> index a3b51441b039..038e42fb2182 100644
> >> --- a/arch/powerpc/kernel/exceptions-64s.S
> >> +++ b/arch/powerpc/kernel/exceptions-64s.S
> >> @@ -502,6 +502,7 @@ DEFINE_FIXED_SYMBOL(\name\()_common_real, text)
> >> std r10,0(r1) /* make stack chain pointer */
> >> std r0,GPR0(r1) /* save r0 in stackframe */
> >> std r10,GPR1(r1) /* save r1 in stackframe */
> >> + ZEROIZE_GPR(0)
> >>
> >> /* Mark our [H]SRRs valid for return */
> >> li r10,1
> >> @@ -538,14 +539,18 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> >> ld r10,IAREA+EX_R10(r13)
> >> std r9,GPR9(r1)
> >> std r10,GPR10(r1)
> >> + ZEROIZE_GPRS(9, 10)
> >
> > You use 9/10 right afterwards, this'd have to move down to where
> > you zero r11 at least.
> >
> >> ld r9,IAREA+EX_R11(r13) /* move r11 - r13 to stackframe */
> >> ld r10,IAREA+EX_R12(r13)
> >> ld r11,IAREA+EX_R13(r13)
> >> std r9,GPR11(r1)
> >> std r10,GPR12(r1)
> >> std r11,GPR13(r1)
> >> + /* keep r12 ([H]SRR1/MSR), r13 (PACA) for interrupt routine */
> >> + ZEROIZE_GPR(11)
> >
> > Kernel always has to keep r13 so no need to comment that. Keeping r11,
> > is that for those annoying fp_unavailable etc handlers?
> >
> > There's probably not much a user can do with this, given they're set
> > from the MSR. Use can influence some bits of its MSR though. So long
> > as we're being paranoid, you could add an IOPTION to retain r11 only for
> > the handlers that need it, or have them load it from MSR and zero it
> > here.
>
> Good suggestion. Presume you’re referring to r12 here. I might go the
> IOPTION route.
Yeah r12, I think you need it because some of those assembly handlers
expect it there to check SRR1 bits. Having an IOPTION is probably good
and it documents that quirk of those handlers. That's bitten me before.
Thanks,
Nick
^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 20/20] powerpc/64e: Clear gprs on interrupt routine entry
2022-08-24 2:05 [PATCH v4 00/20] powerpc: Syscall wrapper and register clearing Rohan McLure
` (18 preceding siblings ...)
2022-08-24 2:05 ` [PATCH v4 19/20] powerpc/64s: Clear gprs on interrupt routine entry in Book3S Rohan McLure
@ 2022-08-24 2:05 ` Rohan McLure
2022-09-12 0:55 ` [PATCH v4 00/20] powerpc: Syscall wrapper and register clearing Rohan McLure
20 siblings, 0 replies; 53+ messages in thread
From: Rohan McLure @ 2022-08-24 2:05 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Rohan McLure
Zero GPRS r14-r31 on entry into the kernel for interrupt sources to
limit influence of user-space values in potential speculation gadgets.
Prior to this commit, all other GPRS are reassigned during the common
prologue to interrupt handlers and so need not be zeroised explicitly.
This may be done safely, without loss of register state prior to the
interrupt, as the common prologue saves the initial values of
non-volatiles, which are unconditionally restored in interrupt_64.S.
Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
---
V3 -> V4: New patch.
---
arch/powerpc/kernel/exceptions-64e.S | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
index 48c640ca425d..296b3bf6b2a6 100644
--- a/arch/powerpc/kernel/exceptions-64e.S
+++ b/arch/powerpc/kernel/exceptions-64e.S
@@ -401,7 +401,8 @@ exc_##n##_common: \
std r12,STACK_FRAME_OVERHEAD-16(r1); /* mark the frame */ \
std r3,_TRAP(r1); /* set trap number */ \
std r0,RESULT(r1); /* clear regs->result */ \
- SAVE_NVGPRS(r1);
+ SAVE_NVGPRS(r1); \
+ ZEROIZE_NVGPRS(); /* minimise speculation influence */
#define EXCEPTION_COMMON(n) \
EXCEPTION_COMMON_LVL(n, SPRN_SPRG_GEN_SCRATCH, PACA_EXGEN)
@@ -1068,6 +1069,7 @@ bad_stack_book3e:
std r11,_CTR(r1)
std r12,_XER(r1)
SAVE_NVGPRS(r1)
+ ZEROIZE_NVGPRS()
lhz r12,PACA_TRAP_SAVE(r13)
std r12,_TRAP(r1)
addi r11,r1,INT_FRAME_SIZE
--
2.34.1
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v4 00/20] powerpc: Syscall wrapper and register clearing
2022-08-24 2:05 [PATCH v4 00/20] powerpc: Syscall wrapper and register clearing Rohan McLure
` (19 preceding siblings ...)
2022-08-24 2:05 ` [PATCH v4 20/20] powerpc/64e: Clear gprs on interrupt routine entry Rohan McLure
@ 2022-09-12 0:55 ` Rohan McLure
20 siblings, 0 replies; 53+ messages in thread
From: Rohan McLure @ 2022-09-12 0:55 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin, Andrew Donnellan
Any comments for this revision? Hopefully these revisions address 32-bit and embedded systems appropriately.
Thanks,
Rohan
> On 24 Aug 2022, at 12:05 pm, Rohan McLure <rmclure@linux.ibm.com> wrote:
>
> V3 available here:
>
> Link: https://lore.kernel.org/all/4C3A8815-67FF-41EB-A703-981920CA1201@linux.ibm.com/T/
>
> Implement a syscall wrapper, causing arguments to handlers to be passed
> via a struct pt_regs on the stack. The syscall wrapper is implemented
> for all platforms other than the Cell processor, from which SPUs expect
> the ability to directly call syscall handler symbols with the regular
> in-register calling convention.
>
> Adopting syscall wrappers requires redefinition of architecture-specific
> syscalls and compatibility syscalls to use the SYSCALL_DEFINE and
> COMPAT_SYSCALL_DEFINE macros, as well as removal of direct-references to
> the emitted syscall-handler symbols from within the kernel. This work
> lead to the following modernisations of powerpc's syscall handlers:
>
> - Replace syscall 82 semantics with sys_old_select and remove
> ppc_select handler, which features direct call to both sys_old_select
> and sys_select.
> - Use a generic fallocate compatibility syscall
>
> Replace asm implementation of syscall table with C implementation for
> more compile-time checks.
>
> Many compatibility syscalls are candidates to be removed in favour of
> generically defined handlers, but exhibit different parameter orderings
> and numberings due to 32-bit ABI support for 64-bit parameters. The
> parameter reorderings are however consistent with arm. A future patch
> series will serve to modernise syscalls by providing generic
> implementations featuring these reorderings.
>
> The design of this syscall is very similar to the s390, x86 and arm64
> implementations. See also Commit 4378a7d4be30 (arm64: implement syscall wrappers).
> The motivation for this change is that it allows for the clearing of
> register state when entering the kernel via through interrupt handlers
> on 64-bit servers. This serves to reduce the influence of values in
> registers carried over from the interrupted process, e.g. syscall
> parameters from user space, or user state at the site of a pagefault.
> All values in registers are saved and zeroized at the entry to an
> interrupt handler and restored afterward. While this may sound like a
> heavy-weight mitigation, many gprs are already saved and restored on
> handling of an interrupt, and the mmap_bench benchmark on Power 9 guest,
> repeatedly invoking the pagefault handler suggests at most ~0.8%
> regression in performance. Realistic workloads are not constantly
> producing interrupts, and so this does not indicate realistic slowdown.
>
> Using wrapped syscalls yields to a performance improvement of ~5.6% on
> the null_syscall benchmark on pseries guests, by removing the need for
> system_call_exception to allocate its own stack frame. This amortises
> the additional costs of saving and restoring non-volatile registers
> (register clearing is cheap on super scalar platforms), and so the
> final mitigation actually yields a net performance improvement of ~0.6%
> on the null_syscall benchmark.
>
> Patch Changelog:
>
> - Fix instances where NULLIFY_GPRS were still present
> - Minimise unrecoverable windows in entry_32.S between SRR0/1 restores
> and RFI
> - Remove all references to syscall symbols prior to introducing syscall
> wrapper.
> - Remove unnecessary duplication of syscall handlers with sys_... and
> powerpc_sys_... symbols.
> - Clear non-volatile registers on Book3E systems, as some of these
> systems feature hardware speculation, and we already unconditionally
> restore NVGPRS.
>
> Rohan McLure (20):
> powerpc: Remove asmlinkage from syscall handler definitions
> powerpc: Use generic fallocate compatibility syscall
> powerpc/32: Remove powerpc select specialisation
> powerpc: Provide do_ppc64_personality helper
> powerpc: Remove direct call to personality syscall handler
> powerpc: Remove direct call to mmap2 syscall handlers
> powerpc: Adopt SYSCALL_DEFINE for arch-specific syscall handlers
> powerpc: Include all arch-specific syscall prototypes
> powerpc: Enable compile-time check for syscall handlers
> powerpc: Use common syscall handler type
> powerpc: Add ZEROIZE_GPRS macros for register clears
> Revert "powerpc/syscall: Save r3 in regs->orig_r3"
> powerpc: Provide syscall wrapper
> powerpc/64s: Clear/restore caller gprs in syscall interrupt/return
> powerpc/64s: Use {ZEROIZE,SAVE,REST}_GPRS macros in sc, scv 0 handlers
> powerpc/32: Clarify interrupt restores with REST_GPR macro in
> entry_32.S
> powerpc/64e: Clarify register saves and clears with
> {SAVE,ZEROIZE}_GPRS
> powerpc/64s: Fix comment on interrupt handler prologue
> powerpc/64s: Clear gprs on interrupt routine entry in Book3S
> powerpc/64e: Clear gprs on interrupt routine entry
>
> arch/powerpc/Kconfig | 1 +
> arch/powerpc/include/asm/compat.h | 5 +
> arch/powerpc/include/asm/interrupt.h | 3 +-
> arch/powerpc/include/asm/ppc_asm.h | 22 +++
> arch/powerpc/include/asm/syscall.h | 11 +-
> arch/powerpc/include/asm/syscall_wrapper.h | 84 +++++++++++
> arch/powerpc/include/asm/syscalls.h | 128 +++++++++++++----
> .../ppc32.h => include/asm/syscalls_32.h} | 0
> arch/powerpc/include/asm/unistd.h | 1 +
> arch/powerpc/kernel/entry_32.S | 40 +++---
> arch/powerpc/kernel/exceptions-64e.S | 31 ++--
> arch/powerpc/kernel/exceptions-64s.S | 25 ++--
> arch/powerpc/kernel/interrupt_64.S | 92 +++++-------
> arch/powerpc/kernel/signal_32.c | 2 +-
> arch/powerpc/kernel/sys_ppc32.c | 54 ++++---
> arch/powerpc/kernel/syscall.c | 32 ++---
> arch/powerpc/kernel/syscalls.c | 51 ++++---
> arch/powerpc/kernel/syscalls/syscall.tbl | 24 ++--
> arch/powerpc/kernel/{systbl.S => systbl.c} | 29 ++--
> arch/powerpc/kernel/vdso.c | 6 +-
> arch/powerpc/perf/callchain_32.c | 2 +-
> arch/powerpc/platforms/cell/spu_callbacks.c | 6 +-
> .../arch/powerpc/entry/syscalls/syscall.tbl | 24 ++--
> 23 files changed, 415 insertions(+), 258 deletions(-)
> create mode 100644 arch/powerpc/include/asm/syscall_wrapper.h
> rename arch/powerpc/{kernel/ppc32.h => include/asm/syscalls_32.h} (100%)
> rename arch/powerpc/kernel/{systbl.S => systbl.c} (55%)
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 53+ messages in thread