Re: [PATCH 1/2] powerpc/entry: convert to common and generic entry
From: Christophe Leroy
Date: Thu Sep 12 2024 - 05:11:58 EST
Le 12/09/2024 à 10:24, Luming Yu a écrit :
From: Yu Luming <luming.yu@xxxxxxxxx>
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@xxxxxxxxxxxx>
---
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