| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ce53d3db-518d-49e0-95b0-16df1432f14a@csgroup.eu>
Date: Thu, 12 Sep 2024 11:11:36 +0200
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: Luming Yu <luming.yu@...ngroup.cn>, linuxppc-dev@...ts.ozlabs.org,
linux-kernel@...r.kernel.org, mpe@...erman.id.au, npiggin@...il.com,
jialong.yang@...ngroup.cn, luming.yu@...il.com
Subject: Re: [PATCH 1/2] powerpc/entry: convert to common and generic entry
Le 12/09/2024 à 10:24, Luming Yu a écrit :
> From: Yu Luming <luming.yu@...il.com>
>
> convert powerpc entry code in syscall and fault to use syscall_work
> and irqentry_state as well as common calls from generic entry infrastructure.
Could you add more description about the change ?
When I look at x86, riscv or s390 commits for the same thing, they tell
a lot more:
Commit 27d6b4d14f5c ("x86/entry: Use generic syscall entry function")
Commit f0bddf50586d ("riscv: entry: Convert to generic entry")
Commit 56e62a737028 ("s390: convert to generic entry")
Can you elso provide some benchmark comparisons, at least using the
null_syscall selftest
tools/testing/selftests/powerpc/benchmarks/null_syscall.c
>
> Signed-off-by: Luming Yu <luming.yu@...ngroup.cn>
> ---
> arch/powerpc/Kconfig | 1 +
> arch/powerpc/include/asm/hw_irq.h | 5 +++++
> arch/powerpc/include/asm/processor.h | 6 ++++++
> arch/powerpc/include/asm/syscall.h | 5 +++++
> arch/powerpc/include/asm/thread_info.h | 1 +
> arch/powerpc/kernel/syscall.c | 6 +++++-
> arch/powerpc/mm/fault.c | 5 +++++
> 7 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index e21f72bcb61f..e94e7e4bfd40 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -202,6 +202,7 @@ config PPC
> select GENERIC_IRQ_SHOW_LEVEL
> select GENERIC_PCI_IOMAP if PCI
> select GENERIC_PTDUMP
> + select GENERIC_ENTRY
> select GENERIC_SMP_IDLE_THREAD
> select GENERIC_TIME_VSYSCALL
> select GENERIC_VDSO_TIME_NS
> diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h
> index 317659fdeacf..a3d591784c95 100644
> --- a/arch/powerpc/include/asm/hw_irq.h
> +++ b/arch/powerpc/include/asm/hw_irq.h
> @@ -216,6 +216,11 @@ static inline bool arch_irqs_disabled(void)
> return arch_irqs_disabled_flags(arch_local_save_flags());
> }
>
> +/*common entry*/
> +static __always_inline bool regs_irqs_disabled(struct pt_regs *regs)
> +{
> + return arch_irqs_disabled();
> +}
> static inline void set_pmi_irq_pending(void)
> {
> /*
> diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
> index b2c51d337e60..1292282f8b0e 100644
> --- a/arch/powerpc/include/asm/processor.h
> +++ b/arch/powerpc/include/asm/processor.h
> @@ -383,6 +383,12 @@ int validate_sp(unsigned long sp, struct task_struct *p);
> int validate_sp_size(unsigned long sp, struct task_struct *p,
> unsigned long nbytes);
>
> +/*for common entry*/
> +static __always_inline bool on_thread_stack(void)
> +{
> + return validate_sp(current_stack_pointer, current);
I don't understand. Other architectures have something more simple for
on_thread_stack().
Also, validate_sp() will also return true when on irq_stack or emergency
stack.
> +}
> +
> /*
> * Prefetch macros.
> */
> diff --git a/arch/powerpc/include/asm/syscall.h b/arch/powerpc/include/asm/syscall.h
> index 3dd36c5e334a..0e94806c7bfe 100644
> --- a/arch/powerpc/include/asm/syscall.h
> +++ b/arch/powerpc/include/asm/syscall.h
> @@ -119,4 +119,9 @@ static inline int syscall_get_arch(struct task_struct *task)
> else
> return AUDIT_ARCH_PPC64;
> }
> +
> +static inline bool arch_syscall_is_vdso_sigreturn(struct pt_regs *regs)
> +{
> + return false;
> +}
> #endif /* _ASM_SYSCALL_H */
> diff --git a/arch/powerpc/include/asm/thread_info.h b/arch/powerpc/include/asm/thread_info.h
> index 47e226032f9c..c52ca3aaebb5 100644
> --- a/arch/powerpc/include/asm/thread_info.h
> +++ b/arch/powerpc/include/asm/thread_info.h
> @@ -58,6 +58,7 @@ struct thread_info {
> unsigned int cpu;
> #endif
> unsigned long local_flags; /* private flags for thread */
> + unsigned long syscall_work;
> #ifdef CONFIG_LIVEPATCH_64
> unsigned long *livepatch_sp;
> #endif
> diff --git a/arch/powerpc/kernel/syscall.c b/arch/powerpc/kernel/syscall.c
> index 77fedb190c93..cbf0510ed10e 100644
> --- a/arch/powerpc/kernel/syscall.c
> +++ b/arch/powerpc/kernel/syscall.c
> @@ -3,6 +3,7 @@
> #include <linux/compat.h>
> #include <linux/context_tracking.h>
> #include <linux/randomize_kstack.h>
> +#include <linux/entry-common.h>
>
> #include <asm/interrupt.h>
> #include <asm/kup.h>
> @@ -131,7 +132,7 @@ notrace long system_call_exception(struct pt_regs *regs, unsigned long r0)
> * and the test against NR_syscalls will fail and the return
> * value to be used is in regs->gpr[3].
> */
> - r0 = do_syscall_trace_enter(regs);
> + syscall_enter_from_user_mode(regs, r0);
shouldn't this be:
r0 = syscall_enter_from_user_mode(regs, r0);
> if (unlikely(r0 >= NR_syscalls))
> return regs->gpr[3];
>
> @@ -185,5 +186,8 @@ notrace long system_call_exception(struct pt_regs *regs, unsigned long r0)
> */
> choose_random_kstack_offset(mftb());
>
> + /*common entry*/
> + syscall_exit_to_user_mode(regs);
> +
This seems to do a lot. Isn't there stuff that was previously done by
powerpc and needs to be removed now ?
> return ret;
> }
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 9e49ede2bc1c..64c6eb06ebe8 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -23,6 +23,7 @@
> #include <linux/mman.h>
> #include <linux/mm.h>
> #include <linux/interrupt.h>
> +#include <linux/entry-common.h>
> #include <linux/highmem.h>
> #include <linux/extable.h>
> #include <linux/kprobes.h>
> @@ -569,15 +570,19 @@ NOKPROBE_SYMBOL(___do_page_fault);
> static __always_inline void __do_page_fault(struct pt_regs *regs)
> {
> long err;
> + irqentry_state_t state = irqentry_enter(regs);
It is already called below in do_page_fault(), is it normal to do it twice ?
>
> err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> if (unlikely(err))
> bad_page_fault(regs, err);
> + irqentry_exit(regs, state);
> }
>
> DEFINE_INTERRUPT_HANDLER(do_page_fault)
> {
> + irqentry_state_t state = irqentry_enter(regs);
> __do_page_fault(regs);
> + irqentry_exit(regs, state);
No need to do the same in hash__do_page_fault() ?
> }
>
> #ifdef CONFIG_PPC_BOOK3S_64
Powered by blists - more mailing lists