Re: [PATCH v4 10/20] powerpc: Use common syscall handler type

["Nicholas Piggin" <npiggin@gmail.com>]

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