From: "Nicholas Piggin" <npiggin@gmail•com>
To: "Rohan McLure" <rmclure@linux•ibm.com>
Cc: linuxppc-dev@lists•ozlabs.org
Subject: Re: [PATCH v4 10/20] powerpc: Use common syscall handler type
Date: Fri, 16 Sep 2022 11:02:56 +1000 [thread overview]
Message-ID: <CMXF5QO08SOS.38XBOA3F2L41P@bobo> (raw)
In-Reply-To: <6E8ADD7A-6122-4615-8BFC-C1104F13B283@linux.ibm.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
next prev parent reply other threads:[~2022-09-16 1:03 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
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-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
2022-09-12 8:38 ` Nicholas Piggin
2022-09-12 9:57 ` Arnd Bergmann
2022-09-12 11:00 ` Christophe Leroy
2022-09-12 11:07 ` Arnd Bergmann
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
2022-08-24 2:05 ` [PATCH v4 04/20] powerpc: Provide do_ppc64_personality helper 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
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
2022-09-12 9:47 ` Nicholas Piggin
2022-09-15 5:06 ` Rohan McLure
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
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
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
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
2022-09-16 1:02 ` Nicholas Piggin [this message]
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
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
2022-08-24 2:05 ` [PATCH v4 13/20] powerpc: Provide syscall wrapper 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
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
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
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
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
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
2022-09-16 0:43 ` 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CMXF5QO08SOS.38XBOA3F2L41P@bobo \
--to=npiggin@gmail$(echo .)com \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=rmclure@linux$(echo .)ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox