[PATCH 2/6] powerpc: Provide syscall wrapper

[Rohan McLure]

> On 3 Jun 2022, at 7:04 pm, Arnd Bergmann <arnd at arndb.de> wrote:
> 
> On Wed, Jun 1, 2022 at 7:48 AM Rohan McLure <rmclure at linux.ibm.com> wrote:
>> 
>> Syscall wrapper implemented as per s390, x86, arm64, providing the
>> option for gprs to be cleared on entry to the kernel, reducing caller
>> influence influence on speculation within syscall routine. The wrapper
>> is a macro that emits syscall handler implementations with parameters
>> passed by stack pointer.
>> 
>> For platforms supporting this syscall wrapper, emit symbols with usual
>> in-register parameters (`sys...`) to support calls to syscall handlers
>> from within the kernel.
> 
> Nice work!
> 
>> Syscalls are wrapped on all platforms except Cell processor. SPUs require
>> access syscall prototypes which are omitted with ARCH_HAS_SYSCALL_WRAPPER
>> enabled.
> 
> Right, I think it's ok to leave out the SPU side. In the long run, I
> would like to
> go back to requiring the prototypes for everything on all architectures, to
> enforce type checking, but that's a separate piece of work.
> 
>> +/*
>> + * For PowerPC specific syscall implementations, wrapper takes exact name and
>> + * return type for a given function.
>> + */
>> +
>> +#ifdef CONFIG_ARCH_HAS_SYSCALL_WRAPPER
>> +#define PPC_SYSCALL_DEFINE(x, type, name, ...)                                 \
>> +       asmlinkage type __powerpc_##name(const struct pt_regs *regs);           \
>> +       ALLOW_ERROR_INJECTION(__powerpc_##name, ERRNO);                         \
>> +       type sys_##name(__MAP(x,__SC_DECL,__VA_ARGS__));                        \
>> +       static type __se_##name(__MAP(x,__SC_LONG,__VA_ARGS__));                \
>> +       static inline type __do_##name(__MAP(x,__SC_DECL,__VA_ARGS__));         \
> 
> What is the benefit of having a separate set of macros for this? I think that
> adds more complexity than it saves in the end.
> 
>> @@ -68,52 +69,63 @@ unsigned long compat_sys_mmap2(unsigned long addr, size_t len,
>> #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)
>> +PPC_SYSCALL_DEFINE(6, compat_ssize_t, compat_sys_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));
>> }
> 
> We now have generalized versions of most of these system calls, as of 5.19-rc1
> with the addition of the riscv compat support. I think it would be
> best to try removing
> the powerpc private versions wherever possible and use the common version,
> modifying it further where necessary.
> 
> If this doesn't work for some of the syscalls, can you use the
> COMPAT_SYSCALL_DEFINE for those in place of PPC_SYSCALL_DEFINE?
> 
>    Arnd

Hi Arnd,

Thanks for your comments. 

> What is the benefit of having a separate set of macros for this? I think that
> adds more complexity than it saves in the end.

I was unsure whether the exact return types needed to be respected for syscall
handlers or not. I realise that under the existing behaviour,
system_call_exception performs an indirect call, the return type of which is
interpreted as a long, so the return type should be irrelevant. On inspection
PPC_SYSCALL_DEFINE is readily replacable with COMPAT_SYSCALL_DEFINE as you
have suggested.

Before resubmitting this series, I will try for a patch series which modernises
syscall handlers in arch/powerpc, and inspect where powerpc private versions
are strictly necessary, using __ARCH_WANT_... wherever possible.

Rohan