> On 3 Jun 2022, at 7:04 pm, Arnd Bergmann <a...@arndb.de> wrote: > > On Wed, Jun 1, 2022 at 7:48 AM Rohan McLure <rmcl...@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