From: Ira Weiny <[email protected]>
In preparation for adding PKS information to struct irqentry_state_t
change all call sites and usages to pass the struct by reference
instead of by value.
Signed-off-by: Ira Weiny <[email protected]>
---
arch/x86/entry/common.c | 16 +++++++---------
arch/x86/include/asm/idtentry.h | 29 +++++++++++++++++------------
arch/x86/kernel/kvm.c | 4 ++--
arch/x86/kernel/nmi.c | 7 ++++---
arch/x86/kernel/traps.c | 21 +++++++++++++--------
arch/x86/mm/fault.c | 4 ++--
include/linux/entry-common.h | 7 ++++---
kernel/entry/common.c | 20 ++++++++------------
8 files changed, 57 insertions(+), 51 deletions(-)
diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index 870efeec8bda..305da13770b6 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -209,9 +209,9 @@ SYSCALL_DEFINE0(ni_syscall)
return -ENOSYS;
}
-noinstr bool idtentry_enter_nmi(struct pt_regs *regs)
+noinstr void idtentry_enter_nmi(struct pt_regs *regs, irqentry_state_t *irq_state)
{
- bool irq_state = lockdep_hardirqs_enabled();
+ irq_state->exit_rcu = lockdep_hardirqs_enabled();
__nmi_enter();
lockdep_hardirqs_off(CALLER_ADDR0);
@@ -222,15 +222,13 @@ noinstr bool idtentry_enter_nmi(struct pt_regs *regs)
trace_hardirqs_off_finish();
ftrace_nmi_enter();
instrumentation_end();
-
- return irq_state;
}
-noinstr void idtentry_exit_nmi(struct pt_regs *regs, bool restore)
+noinstr void idtentry_exit_nmi(struct pt_regs *regs, irqentry_state_t *irq_state)
{
instrumentation_begin();
ftrace_nmi_exit();
- if (restore) {
+ if (irq_state->exit_rcu) {
trace_hardirqs_on_prepare();
lockdep_hardirqs_on_prepare(CALLER_ADDR0);
}
@@ -238,7 +236,7 @@ noinstr void idtentry_exit_nmi(struct pt_regs *regs, bool restore)
rcu_nmi_exit();
lockdep_hardirq_exit();
- if (restore)
+ if (irq_state->exit_rcu)
lockdep_hardirqs_on(CALLER_ADDR0);
__nmi_exit();
}
@@ -295,7 +293,7 @@ __visible noinstr void xen_pv_evtchn_do_upcall(struct pt_regs *regs)
bool inhcall;
irqentry_state_t state;
- state = irqentry_enter(regs);
+ irqentry_enter(regs, &state);
old_regs = set_irq_regs(regs);
instrumentation_begin();
@@ -311,7 +309,7 @@ __visible noinstr void xen_pv_evtchn_do_upcall(struct pt_regs *regs)
instrumentation_end();
restore_inhcall(inhcall);
} else {
- irqentry_exit(regs, state);
+ irqentry_exit(regs, &state);
}
}
#endif /* CONFIG_XEN_PV */
diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h
index a0638640f1ed..622889ba21d0 100644
--- a/arch/x86/include/asm/idtentry.h
+++ b/arch/x86/include/asm/idtentry.h
@@ -11,8 +11,8 @@
#include <asm/irq_stack.h>
-bool idtentry_enter_nmi(struct pt_regs *regs);
-void idtentry_exit_nmi(struct pt_regs *regs, bool irq_state);
+void idtentry_enter_nmi(struct pt_regs *regs, irqentry_state_t *irq_state);
+void idtentry_exit_nmi(struct pt_regs *regs, irqentry_state_t *irq_state);
/**
* DECLARE_IDTENTRY - Declare functions for simple IDT entry points
@@ -52,12 +52,13 @@ static __always_inline void __##func(struct pt_regs *regs); \
\
__visible noinstr void func(struct pt_regs *regs) \
{ \
- irqentry_state_t state = irqentry_enter(regs); \
+ irqentry_state_t state; \
\
+ irqentry_enter(regs, &state); \
instrumentation_begin(); \
__##func (regs); \
instrumentation_end(); \
- irqentry_exit(regs, state); \
+ irqentry_exit(regs, &state); \
} \
\
static __always_inline void __##func(struct pt_regs *regs)
@@ -99,12 +100,13 @@ static __always_inline void __##func(struct pt_regs *regs, \
__visible noinstr void func(struct pt_regs *regs, \
unsigned long error_code) \
{ \
- irqentry_state_t state = irqentry_enter(regs); \
+ irqentry_state_t state; \
\
+ irqentry_enter(regs, &state); \
instrumentation_begin(); \
__##func (regs, error_code); \
instrumentation_end(); \
- irqentry_exit(regs, state); \
+ irqentry_exit(regs, &state); \
} \
\
static __always_inline void __##func(struct pt_regs *regs, \
@@ -195,15 +197,16 @@ static __always_inline void __##func(struct pt_regs *regs, u8 vector); \
__visible noinstr void func(struct pt_regs *regs, \
unsigned long error_code) \
{ \
- irqentry_state_t state = irqentry_enter(regs); \
+ irqentry_state_t state; \
\
+ irqentry_enter(regs, &state); \
instrumentation_begin(); \
irq_enter_rcu(); \
kvm_set_cpu_l1tf_flush_l1d(); \
__##func (regs, (u8)error_code); \
irq_exit_rcu(); \
instrumentation_end(); \
- irqentry_exit(regs, state); \
+ irqentry_exit(regs, &state); \
} \
\
static __always_inline void __##func(struct pt_regs *regs, u8 vector)
@@ -237,15 +240,16 @@ static void __##func(struct pt_regs *regs); \
\
__visible noinstr void func(struct pt_regs *regs) \
{ \
- irqentry_state_t state = irqentry_enter(regs); \
+ irqentry_state_t state; \
\
+ irqentry_enter(regs, &state); \
instrumentation_begin(); \
irq_enter_rcu(); \
kvm_set_cpu_l1tf_flush_l1d(); \
run_sysvec_on_irqstack_cond(__##func, regs); \
irq_exit_rcu(); \
instrumentation_end(); \
- irqentry_exit(regs, state); \
+ irqentry_exit(regs, &state); \
} \
\
static noinline void __##func(struct pt_regs *regs)
@@ -266,15 +270,16 @@ static __always_inline void __##func(struct pt_regs *regs); \
\
__visible noinstr void func(struct pt_regs *regs) \
{ \
- irqentry_state_t state = irqentry_enter(regs); \
+ irqentry_state_t state; \
\
+ irqentry_enter(regs, &state); \
instrumentation_begin(); \
__irq_enter_raw(); \
kvm_set_cpu_l1tf_flush_l1d(); \
__##func (regs); \
__irq_exit_raw(); \
instrumentation_end(); \
- irqentry_exit(regs, state); \
+ irqentry_exit(regs, &state); \
} \
\
static __always_inline void __##func(struct pt_regs *regs)
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 9663ba31347c..c6be0a54236f 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -241,7 +241,7 @@ noinstr bool __kvm_handle_async_pf(struct pt_regs *regs, u32 token)
if (!flags)
return false;
- state = irqentry_enter(regs);
+ irqentry_enter(regs, &state);
instrumentation_begin();
/*
@@ -262,7 +262,7 @@ noinstr bool __kvm_handle_async_pf(struct pt_regs *regs, u32 token)
}
instrumentation_end();
- irqentry_exit(regs, state);
+ irqentry_exit(regs, &state);
return true;
}
diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
index 4fc9954a9560..68c07cad0150 100644
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -33,6 +33,7 @@
#include <asm/reboot.h>
#include <asm/cache.h>
#include <asm/nospec-branch.h>
+#include <asm/idtentry.h>
#define CREATE_TRACE_POINTS
#include <trace/events/nmi.h>
@@ -475,7 +476,7 @@ static DEFINE_PER_CPU(unsigned long, nmi_dr7);
DEFINE_IDTENTRY_RAW(exc_nmi)
{
- bool irq_state;
+ irqentry_state_t irq_state = { };
if (IS_ENABLED(CONFIG_SMP) && arch_cpu_is_offline(smp_processor_id()))
return;
@@ -490,14 +491,14 @@ DEFINE_IDTENTRY_RAW(exc_nmi)
this_cpu_write(nmi_dr7, local_db_save());
- irq_state = idtentry_enter_nmi(regs);
+ idtentry_enter_nmi(regs, &irq_state);
inc_irq_stat(__nmi_count);
if (!ignore_nmis)
default_do_nmi(regs);
- idtentry_exit_nmi(regs, irq_state);
+ idtentry_exit_nmi(regs, &irq_state);
local_db_restore(this_cpu_read(nmi_dr7));
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 81a2fb711091..daf7bc02fc99 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -254,11 +254,11 @@ DEFINE_IDTENTRY_RAW(exc_invalid_op)
if (!user_mode(regs) && handle_bug(regs))
return;
- state = irqentry_enter(regs);
+ irqentry_enter(regs, &state);
instrumentation_begin();
handle_invalid_op(regs);
instrumentation_end();
- irqentry_exit(regs, state);
+ irqentry_exit(regs, &state);
}
DEFINE_IDTENTRY(exc_coproc_segment_overrun)
@@ -342,6 +342,7 @@ __visible void __noreturn handle_stack_overflow(const char *message,
*/
DEFINE_IDTENTRY_DF(exc_double_fault)
{
+ irqentry_state_t irq_state;
static const char str[] = "double fault";
struct task_struct *tsk = current;
@@ -404,7 +405,7 @@ DEFINE_IDTENTRY_DF(exc_double_fault)
}
#endif
- idtentry_enter_nmi(regs);
+ idtentry_enter_nmi(regs, &irq_state);
instrumentation_begin();
notify_die(DIE_TRAP, str, regs, error_code, X86_TRAP_DF, SIGSEGV);
@@ -650,12 +651,15 @@ DEFINE_IDTENTRY_RAW(exc_int3)
instrumentation_end();
irqentry_exit_to_user_mode(regs);
} else {
- bool irq_state = idtentry_enter_nmi(regs);
+ irqentry_state_t irq_state;
+
+ idtentry_enter_nmi(regs, &irq_state);
+
instrumentation_begin();
if (!do_int3(regs))
die("int3", regs, 0);
instrumentation_end();
- idtentry_exit_nmi(regs, irq_state);
+ idtentry_exit_nmi(regs, &irq_state);
}
}
@@ -861,7 +865,9 @@ static __always_inline void exc_debug_kernel(struct pt_regs *regs,
* includes the entry stack is excluded for everything.
*/
unsigned long dr7 = local_db_save();
- bool irq_state = idtentry_enter_nmi(regs);
+ irqentry_state_t irq_state;
+
+ idtentry_enter_nmi(regs, &irq_state);
instrumentation_begin();
/*
@@ -880,8 +886,7 @@ static __always_inline void exc_debug_kernel(struct pt_regs *regs,
handle_debug(regs, dr6, false);
instrumentation_end();
- idtentry_exit_nmi(regs, irq_state);
-
+ idtentry_exit_nmi(regs, &irq_state);
local_db_restore(dr7);
}
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 6e3e8a124903..e55bc4bff389 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -1476,11 +1476,11 @@ DEFINE_IDTENTRY_RAW_ERRORCODE(exc_page_fault)
* code reenabled RCU to avoid subsequent wreckage which helps
* debugability.
*/
- state = irqentry_enter(regs);
+ irqentry_enter(regs, &state);
instrumentation_begin();
handle_page_fault(regs, error_code, address);
instrumentation_end();
- irqentry_exit(regs, state);
+ irqentry_exit(regs, &state);
}
diff --git a/include/linux/entry-common.h b/include/linux/entry-common.h
index 159c7476b11b..de4f24c554ee 100644
--- a/include/linux/entry-common.h
+++ b/include/linux/entry-common.h
@@ -349,6 +349,7 @@ typedef struct irqentry_state {
/**
* irqentry_enter - Handle state tracking on ordinary interrupt entries
* @regs: Pointer to pt_regs of interrupted context
+ * @state: Pointer to an object to store the irq state
*
* Invokes:
* - lockdep irqflag state tracking as low level ASM entry disabled
@@ -377,7 +378,7 @@ typedef struct irqentry_state {
*
* Returns: An opaque object that must be passed to idtentry_exit()
*/
-irqentry_state_t noinstr irqentry_enter(struct pt_regs *regs);
+void noinstr irqentry_enter(struct pt_regs *regs, irqentry_state_t *state);
/**
* irqentry_exit_cond_resched - Conditionally reschedule on return from interrupt
@@ -389,7 +390,7 @@ void irqentry_exit_cond_resched(void);
/**
* irqentry_exit - Handle return from exception that used irqentry_enter()
* @regs: Pointer to pt_regs (exception entry regs)
- * @state: Return value from matching call to irqentry_enter()
+ * @state: Reference to the value saved in irqentry_enter()
*
* Depending on the return target (kernel/user) this runs the necessary
* preemption and work checks if possible and reguired and returns to
@@ -400,6 +401,6 @@ void irqentry_exit_cond_resched(void);
*
* Counterpart to irqentry_enter().
*/
-void noinstr irqentry_exit(struct pt_regs *regs, irqentry_state_t state);
+void noinstr irqentry_exit(struct pt_regs *regs, irqentry_state_t *state);
#endif
diff --git a/kernel/entry/common.c b/kernel/entry/common.c
index 6fdb6105e6d6..21601993ad1b 100644
--- a/kernel/entry/common.c
+++ b/kernel/entry/common.c
@@ -282,15 +282,13 @@ noinstr void irqentry_exit_to_user_mode(struct pt_regs *regs)
exit_to_user_mode();
}
-noinstr irqentry_state_t irqentry_enter(struct pt_regs *regs)
+noinstr void irqentry_enter(struct pt_regs *regs, irqentry_state_t *state)
{
- irqentry_state_t ret = {
- .exit_rcu = false,
- };
+ state->exit_rcu = false;
if (user_mode(regs)) {
irqentry_enter_from_user_mode(regs);
- return ret;
+ return;
}
/*
@@ -328,8 +326,8 @@ noinstr irqentry_state_t irqentry_enter(struct pt_regs *regs)
trace_hardirqs_off_finish();
instrumentation_end();
- ret.exit_rcu = true;
- return ret;
+ state->exit_rcu = true;
+ return;
}
/*
@@ -343,8 +341,6 @@ noinstr irqentry_state_t irqentry_enter(struct pt_regs *regs)
/* Use the combo lockdep/tracing function */
trace_hardirqs_off();
instrumentation_end();
-
- return ret;
}
void irqentry_exit_cond_resched(void)
@@ -359,7 +355,7 @@ void irqentry_exit_cond_resched(void)
}
}
-noinstr void irqentry_exit(struct pt_regs *regs, irqentry_state_t state)
+noinstr void irqentry_exit(struct pt_regs *regs, irqentry_state_t *state)
{
lockdep_assert_irqs_disabled();
@@ -372,7 +368,7 @@ noinstr void irqentry_exit(struct pt_regs *regs, irqentry_state_t state)
* carefully and needs the same ordering of lockdep/tracing
* and RCU as the return to user mode path.
*/
- if (state.exit_rcu) {
+ if (state->exit_rcu) {
instrumentation_begin();
/* Tell the tracer that IRET will enable interrupts */
trace_hardirqs_on_prepare();
@@ -394,7 +390,7 @@ noinstr void irqentry_exit(struct pt_regs *regs, irqentry_state_t state)
* IRQ flags state is correct already. Just tell RCU if it
* was not watching on entry.
*/
- if (state.exit_rcu)
+ if (state->exit_rcu)
rcu_irq_exit();
}
}
--
2.28.0.rc0.12.gb6a658bd00c9
On Fri, Oct 09, 2020 at 12:42:55PM -0700, [email protected] wrote:
> -noinstr bool idtentry_enter_nmi(struct pt_regs *regs)
> +noinstr void idtentry_enter_nmi(struct pt_regs *regs, irqentry_state_t *irq_state)
> {
> - bool irq_state = lockdep_hardirqs_enabled();
> + irq_state->exit_rcu = lockdep_hardirqs_enabled();
>
> __nmi_enter();
> lockdep_hardirqs_off(CALLER_ADDR0);
> @@ -222,15 +222,13 @@ noinstr bool idtentry_enter_nmi(struct pt_regs *regs)
> trace_hardirqs_off_finish();
> ftrace_nmi_enter();
> instrumentation_end();
> -
> - return irq_state;
> }
>
> -noinstr void idtentry_exit_nmi(struct pt_regs *regs, bool restore)
> +noinstr void idtentry_exit_nmi(struct pt_regs *regs, irqentry_state_t *irq_state)
> {
> instrumentation_begin();
> ftrace_nmi_exit();
> - if (restore) {
> + if (irq_state->exit_rcu) {
> trace_hardirqs_on_prepare();
> lockdep_hardirqs_on_prepare(CALLER_ADDR0);
> }
> @@ -238,7 +236,7 @@ noinstr void idtentry_exit_nmi(struct pt_regs *regs, bool restore)
>
> rcu_nmi_exit();
> lockdep_hardirq_exit();
> - if (restore)
> + if (irq_state->exit_rcu)
> lockdep_hardirqs_on(CALLER_ADDR0);
> __nmi_exit();
> }
That's not nice.. The NMI path is different from the IRQ path and has a
different variable. Yes, this works, but *groan*.
Maybe union them if you want to avoid bloating the structure, but the
above makes it really hard to read.
On Fri, Oct 16 2020 at 13:45, Peter Zijlstra wrote:
> On Fri, Oct 09, 2020 at 12:42:55PM -0700, [email protected] wrote:
>> @@ -238,7 +236,7 @@ noinstr void idtentry_exit_nmi(struct pt_regs *regs, bool restore)
>>
>> rcu_nmi_exit();
>> lockdep_hardirq_exit();
>> - if (restore)
>> + if (irq_state->exit_rcu)
>> lockdep_hardirqs_on(CALLER_ADDR0);
>> __nmi_exit();
>> }
>
> That's not nice.. The NMI path is different from the IRQ path and has a
> different variable. Yes, this works, but *groan*.
>
> Maybe union them if you want to avoid bloating the structure, but the
> above makes it really hard to read.
Right, and also that nmi entry thing should not be in x86. Something
like the untested below as first cleanup.
Thanks,
tglx
----
Subject: x86/entry: Move nmi entry/exit into common code
From: Thomas Gleixner <[email protected]>
Date: Fri, 11 Sep 2020 10:09:56 +0200
Add blurb here.
Signed-off-by: Thomas Gleixner <[email protected]>
---
arch/x86/entry/common.c | 34 ----------------------------------
arch/x86/include/asm/idtentry.h | 3 ---
arch/x86/kernel/cpu/mce/core.c | 6 +++---
arch/x86/kernel/nmi.c | 6 +++---
arch/x86/kernel/traps.c | 13 +++++++------
include/linux/entry-common.h | 20 ++++++++++++++++++++
kernel/entry/common.c | 36 ++++++++++++++++++++++++++++++++++++
7 files changed, 69 insertions(+), 49 deletions(-)
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -209,40 +209,6 @@ SYSCALL_DEFINE0(ni_syscall)
return -ENOSYS;
}
-noinstr bool idtentry_enter_nmi(struct pt_regs *regs)
-{
- bool irq_state = lockdep_hardirqs_enabled();
-
- __nmi_enter();
- lockdep_hardirqs_off(CALLER_ADDR0);
- lockdep_hardirq_enter();
- rcu_nmi_enter();
-
- instrumentation_begin();
- trace_hardirqs_off_finish();
- ftrace_nmi_enter();
- instrumentation_end();
-
- return irq_state;
-}
-
-noinstr void idtentry_exit_nmi(struct pt_regs *regs, bool restore)
-{
- instrumentation_begin();
- ftrace_nmi_exit();
- if (restore) {
- trace_hardirqs_on_prepare();
- lockdep_hardirqs_on_prepare(CALLER_ADDR0);
- }
- instrumentation_end();
-
- rcu_nmi_exit();
- lockdep_hardirq_exit();
- if (restore)
- lockdep_hardirqs_on(CALLER_ADDR0);
- __nmi_exit();
-}
-
#ifdef CONFIG_XEN_PV
#ifndef CONFIG_PREEMPTION
/*
--- a/arch/x86/include/asm/idtentry.h
+++ b/arch/x86/include/asm/idtentry.h
@@ -11,9 +11,6 @@
#include <asm/irq_stack.h>
-bool idtentry_enter_nmi(struct pt_regs *regs);
-void idtentry_exit_nmi(struct pt_regs *regs, bool irq_state);
-
/**
* DECLARE_IDTENTRY - Declare functions for simple IDT entry points
* No error code pushed by hardware
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1983,7 +1983,7 @@ void (*machine_check_vector)(struct pt_r
static __always_inline void exc_machine_check_kernel(struct pt_regs *regs)
{
- bool irq_state;
+ irqentry_state_t irq_state;
WARN_ON_ONCE(user_mode(regs));
@@ -1995,7 +1995,7 @@ static __always_inline void exc_machine_
mce_check_crashing_cpu())
return;
- irq_state = idtentry_enter_nmi(regs);
+ irq_state = irqentry_nmi_enter(regs);
/*
* The call targets are marked noinstr, but objtool can't figure
* that out because it's an indirect call. Annotate it.
@@ -2006,7 +2006,7 @@ static __always_inline void exc_machine_
if (regs->flags & X86_EFLAGS_IF)
trace_hardirqs_on_prepare();
instrumentation_end();
- idtentry_exit_nmi(regs, irq_state);
+ irqentry_nmi_exit(regs, irq_state);
}
static __always_inline void exc_machine_check_user(struct pt_regs *regs)
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -475,7 +475,7 @@ static DEFINE_PER_CPU(unsigned long, nmi
DEFINE_IDTENTRY_RAW(exc_nmi)
{
- bool irq_state;
+ irqentry_state_t irq_state;
/*
* Re-enable NMIs right here when running as an SEV-ES guest. This might
@@ -502,14 +502,14 @@ DEFINE_IDTENTRY_RAW(exc_nmi)
this_cpu_write(nmi_dr7, local_db_save());
- irq_state = idtentry_enter_nmi(regs);
+ irq_state = irqentry_nmi_enter(regs);
inc_irq_stat(__nmi_count);
if (!ignore_nmis)
default_do_nmi(regs);
- idtentry_exit_nmi(regs, irq_state);
+ irqentry_nmi_exit(regs, irq_state);
local_db_restore(this_cpu_read(nmi_dr7));
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -405,7 +405,7 @@ DEFINE_IDTENTRY_DF(exc_double_fault)
}
#endif
- idtentry_enter_nmi(regs);
+ irqentry_nmi_enter(regs);
instrumentation_begin();
notify_die(DIE_TRAP, str, regs, error_code, X86_TRAP_DF, SIGSEGV);
@@ -651,12 +651,13 @@ DEFINE_IDTENTRY_RAW(exc_int3)
instrumentation_end();
irqentry_exit_to_user_mode(regs);
} else {
- bool irq_state = idtentry_enter_nmi(regs);
+ irqentry_state_t irq_state = irqentry_nmi_enter(regs);
+
instrumentation_begin();
if (!do_int3(regs))
die("int3", regs, 0);
instrumentation_end();
- idtentry_exit_nmi(regs, irq_state);
+ irqentry_nmi_exit(regs, irq_state);
}
}
@@ -864,7 +865,7 @@ static __always_inline void exc_debug_ke
* includes the entry stack is excluded for everything.
*/
unsigned long dr7 = local_db_save();
- bool irq_state = idtentry_enter_nmi(regs);
+ irqentry_state_t irq_state = irqentry_nmi_enter(regs);
instrumentation_begin();
/*
@@ -907,7 +908,7 @@ static __always_inline void exc_debug_ke
regs->flags &= ~X86_EFLAGS_TF;
out:
instrumentation_end();
- idtentry_exit_nmi(regs, irq_state);
+ irqentry_nmi_exit(regs, irq_state);
local_db_restore(dr7);
}
@@ -925,7 +926,7 @@ static __always_inline void exc_debug_us
/*
* NB: We can't easily clear DR7 here because
- * idtentry_exit_to_usermode() can invoke ptrace, schedule, access
+ * irqentry_exit_to_usermode() can invoke ptrace, schedule, access
* user memory, etc. This means that a recursive #DB is possible. If
* this happens, that #DB will hit exc_debug_kernel() and clear DR7.
* Since we're not on the IST stack right now, everything will be
--- a/include/linux/entry-common.h
+++ b/include/linux/entry-common.h
@@ -343,6 +343,7 @@ void irqentry_exit_to_user_mode(struct p
#ifndef irqentry_state
typedef struct irqentry_state {
bool exit_rcu;
+ bool lockdep;
} irqentry_state_t;
#endif
@@ -402,4 +403,23 @@ void irqentry_exit_cond_resched(void);
*/
void noinstr irqentry_exit(struct pt_regs *regs, irqentry_state_t state);
+/**
+ * irqentry_nmi_enter - Handle NMI entry
+ * @regs: Pointer to currents pt_regs
+ *
+ * Similar to irqentry_enter() but taking care of the NMI constraints.
+ */
+irqentry_state_t noinstr irqentry_nmi_enter(struct pt_regs *regs);
+
+/**
+ * irqentry_nmi_exit - Handle return from NMI handling
+ * @regs: Pointer to pt_regs (NMI entry regs)
+ * @state: Return value from matching call to irqentry_nmi_enter()
+ *
+ * Last action before returning to the low level assmenbly code.
+ *
+ * Counterpart to irqentry_nmi_enter().
+ */
+void noinstr irqentry_nmi_exit(struct pt_regs *regs, irqentry_state_t irq_state);
+
#endif
--- a/kernel/entry/common.c
+++ b/kernel/entry/common.c
@@ -398,3 +398,39 @@ noinstr void irqentry_exit(struct pt_reg
rcu_irq_exit();
}
}
+
+irqentry_state_t noinstr irqentry_nmi_enter(struct pt_regs *regs)
+{
+ irqentry_state_t irq_state;
+
+ irq_state.lockdep = lockdep_hardirqs_enabled();
+
+ __nmi_enter();
+ lockdep_hardirqs_off(CALLER_ADDR0);
+ lockdep_hardirq_enter();
+ rcu_nmi_enter();
+
+ instrumentation_begin();
+ trace_hardirqs_off_finish();
+ ftrace_nmi_enter();
+ instrumentation_end();
+
+ return irq_state;
+}
+
+void noinstr irqentry_nmi_exit(struct pt_regs *regs, irqentry_state_t irq_state)
+{
+ instrumentation_begin();
+ ftrace_nmi_exit();
+ if (irq_state.lockdep) {
+ trace_hardirqs_on_prepare();
+ lockdep_hardirqs_on_prepare(CALLER_ADDR0);
+ }
+ instrumentation_end();
+
+ rcu_nmi_exit();
+ lockdep_hardirq_exit();
+ if (irq_state.lockdep)
+ lockdep_hardirqs_on(CALLER_ADDR0);
+ __nmi_exit();
+}
On Sun, Oct 18 2020 at 22:37, Ira Weiny wrote:
> On Fri, Oct 16, 2020 at 02:55:21PM +0200, Thomas Gleixner wrote:
>> Subject: x86/entry: Move nmi entry/exit into common code
>> From: Thomas Gleixner <[email protected]>
>> Date: Fri, 11 Sep 2020 10:09:56 +0200
>>
>> Add blurb here.
>
> How about:
>
> To prepare for saving PKRS values across NMI's we lift the
> idtentry_[enter|exit]_nmi() to the common code. Rename them to
> irqentry_nmi_[enter|exit]() to reflect the new generic nature and store the
> state in the same irqentry_state_t structure as the other irqentry_*()
> functions. Finally, differentiate the state being stored between the NMI and
> IRQ path by adding 'lockdep' to irqentry_state_t.
No. This has absolutely nothing to do with PKRS. It's a cleanup valuable
by itself and that's how it should have been done right away.
So the proper changelog is:
Lockdep state handling on NMI enter and exit is nothing specific to
X86. It's not any different on other architectures. Also the extra
state type is not necessary, irqentry_state_t can carry the necessary
information as well.
Move it to common code and extend irqentry_state_t to carry lockdep
state.
>> --- a/include/linux/entry-common.h
>> +++ b/include/linux/entry-common.h
>> @@ -343,6 +343,7 @@ void irqentry_exit_to_user_mode(struct p
>> #ifndef irqentry_state
>> typedef struct irqentry_state {
>> bool exit_rcu;
>> + bool lockdep;
>> } irqentry_state_t;
>
> Building on what Peter said do you agree this should be made into a union?
>
> It may not be strictly necessary in this patch but I think it would reflect the
> mutual exclusivity better and could be changed easy enough in the follow on
> patch which adds the pkrs state.
Why the heck should it be changed in a patch which adds something
completely different?
Either it's mutually exclusive or not and if so it want's to be done in
this patch and not in a change which extends the struct for other
reasons.
Thanks,
tglx
On Fri, Oct 16, 2020 at 02:55:21PM +0200, Thomas Gleixner wrote:
> On Fri, Oct 16 2020 at 13:45, Peter Zijlstra wrote:
> > On Fri, Oct 09, 2020 at 12:42:55PM -0700, [email protected] wrote:
> >> @@ -238,7 +236,7 @@ noinstr void idtentry_exit_nmi(struct pt_regs *regs, bool restore)
> >>
> >> rcu_nmi_exit();
> >> lockdep_hardirq_exit();
> >> - if (restore)
> >> + if (irq_state->exit_rcu)
> >> lockdep_hardirqs_on(CALLER_ADDR0);
> >> __nmi_exit();
> >> }
> >
> > That's not nice.. The NMI path is different from the IRQ path and has a
> > different variable. Yes, this works, but *groan*.
> >
> > Maybe union them if you want to avoid bloating the structure, but the
> > above makes it really hard to read.
>
> Right, and also that nmi entry thing should not be in x86. Something
> like the untested below as first cleanup.
Ok, I see what Peter was talking about. I've added this patch to the series.
>
> Thanks,
>
> tglx
> ----
> Subject: x86/entry: Move nmi entry/exit into common code
> From: Thomas Gleixner <[email protected]>
> Date: Fri, 11 Sep 2020 10:09:56 +0200
>
> Add blurb here.
How about:
To prepare for saving PKRS values across NMI's we lift the
idtentry_[enter|exit]_nmi() to the common code. Rename them to
irqentry_nmi_[enter|exit]() to reflect the new generic nature and store the
state in the same irqentry_state_t structure as the other irqentry_*()
functions. Finally, differentiate the state being stored between the NMI and
IRQ path by adding 'lockdep' to irqentry_state_t.
?
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> ---
> arch/x86/entry/common.c | 34 ----------------------------------
> arch/x86/include/asm/idtentry.h | 3 ---
> arch/x86/kernel/cpu/mce/core.c | 6 +++---
> arch/x86/kernel/nmi.c | 6 +++---
> arch/x86/kernel/traps.c | 13 +++++++------
> include/linux/entry-common.h | 20 ++++++++++++++++++++
> kernel/entry/common.c | 36 ++++++++++++++++++++++++++++++++++++
> 7 files changed, 69 insertions(+), 49 deletions(-)
>
[snip]
> --- a/include/linux/entry-common.h
> +++ b/include/linux/entry-common.h
> @@ -343,6 +343,7 @@ void irqentry_exit_to_user_mode(struct p
> #ifndef irqentry_state
> typedef struct irqentry_state {
> bool exit_rcu;
> + bool lockdep;
> } irqentry_state_t;
Building on what Peter said do you agree this should be made into a union?
It may not be strictly necessary in this patch but I think it would reflect the
mutual exclusivity better and could be changed easy enough in the follow on
patch which adds the pkrs state.
Ira
On Mon, Oct 19, 2020 at 11:32:50AM +0200, Thomas Gleixner wrote:
> On Sun, Oct 18 2020 at 22:37, Ira Weiny wrote:
> > On Fri, Oct 16, 2020 at 02:55:21PM +0200, Thomas Gleixner wrote:
> >> Subject: x86/entry: Move nmi entry/exit into common code
> >> From: Thomas Gleixner <[email protected]>
> >> Date: Fri, 11 Sep 2020 10:09:56 +0200
> >>
> >> Add blurb here.
> >
> > How about:
> >
> > To prepare for saving PKRS values across NMI's we lift the
> > idtentry_[enter|exit]_nmi() to the common code. Rename them to
> > irqentry_nmi_[enter|exit]() to reflect the new generic nature and store the
> > state in the same irqentry_state_t structure as the other irqentry_*()
> > functions. Finally, differentiate the state being stored between the NMI and
> > IRQ path by adding 'lockdep' to irqentry_state_t.
>
> No. This has absolutely nothing to do with PKRS. It's a cleanup valuable
> by itself and that's how it should have been done right away.
>
> So the proper changelog is:
>
> Lockdep state handling on NMI enter and exit is nothing specific to
> X86. It's not any different on other architectures. Also the extra
> state type is not necessary, irqentry_state_t can carry the necessary
> information as well.
>
> Move it to common code and extend irqentry_state_t to carry lockdep
> state.
Ok sounds good, thanks.
>
> >> --- a/include/linux/entry-common.h
> >> +++ b/include/linux/entry-common.h
> >> @@ -343,6 +343,7 @@ void irqentry_exit_to_user_mode(struct p
> >> #ifndef irqentry_state
> >> typedef struct irqentry_state {
> >> bool exit_rcu;
> >> + bool lockdep;
> >> } irqentry_state_t;
> >
> > Building on what Peter said do you agree this should be made into a union?
> >
> > It may not be strictly necessary in this patch but I think it would reflect the
> > mutual exclusivity better and could be changed easy enough in the follow on
> > patch which adds the pkrs state.
>
> Why the heck should it be changed in a patch which adds something
> completely different?
Because the PKRS stuff is used in both NMI and IRQ state.
>
> Either it's mutually exclusive or not and if so it want's to be done in
> this patch and not in a change which extends the struct for other
> reasons.
Sorry, let me clarify. After this patch we have.
typedef union irqentry_state {
bool exit_rcu;
bool lockdep;
} irqentry_state_t;
Which reflects the mutual exclusion of the 2 variables.
But then when the pkrs stuff is added the union changes back to a structure and
looks like this.
typedef struct irqentry_state {
#ifdef CONFIG_ARCH_HAS_SUPERVISOR_PKEYS
u32 pkrs;
u32 thread_pkrs;
#endif
union {
bool exit_rcu;
bool lockdep;
};
} irqentry_state_t;
Because the pkrs information is in addition to exit_rcu OR lockdep.
So this is what I meant by 'could be changed easy enough in the follow on
patch'.
Is that clear?
Ira
On Mon, Oct 19, 2020 at 11:12:44PM +0200, Thomas Gleixner wrote:
> On Mon, Oct 19 2020 at 13:26, Ira Weiny wrote:
> > On Mon, Oct 19, 2020 at 11:32:50AM +0200, Thomas Gleixner wrote:
> > Sorry, let me clarify. After this patch we have.
> >
> > typedef union irqentry_state {
> > bool exit_rcu;
> > bool lockdep;
> > } irqentry_state_t;
> >
> > Which reflects the mutual exclusion of the 2 variables.
>
> Huch? From the patch I gave you:
>
> #ifndef irqentry_state
> typedef struct irqentry_state {
> bool exit_rcu;
> + bool lockdep;
> } irqentry_state_t;
> #endif
>
> How is that a union?
I was proposing to make it a union.
>
> > But then when the pkrs stuff is added the union changes back to a structure and
> > looks like this.
>
> So you want:
>
> 1) Move stuff to struct irqentry_state (my patch)
>
> 2) Change it to a union and pass it as pointer at the same time
No, I would have made it a union in your patch.
Pass by reference would remain largely the same.
>
> 3) Change it back to struct to add PKRS
Yes. :-/
>
> > Is that clear?
>
> What's clear is that the above is nonsense. We can just do
>
> #ifndef irqentry_state
> typedef struct irqentry_state {
> union {
> bool exit_rcu;
> bool lockdep;
> };
> } irqentry_state_t;
> #endif
>
> right in the patch which I gave you. Because that actually makes sense.
Ok I'm very sorry. I was thinking that having a struct containing nothing but
an anonymous union would be unacceptable as a stand alone item in your patch.
In my experience other maintainers would have rejected such a change and
would have asked; 'why not just make it a union'?
I'm very happy skipping the gymnastics on individual patches in favor of making
the whole series work out in the end.
Thank you for your help again. :-)
Ira
On Mon, Oct 19 2020 at 13:26, Ira Weiny wrote:
> On Mon, Oct 19, 2020 at 11:32:50AM +0200, Thomas Gleixner wrote:
> Sorry, let me clarify. After this patch we have.
>
> typedef union irqentry_state {
> bool exit_rcu;
> bool lockdep;
> } irqentry_state_t;
>
> Which reflects the mutual exclusion of the 2 variables.
Huch? From the patch I gave you:
#ifndef irqentry_state
typedef struct irqentry_state {
bool exit_rcu;
+ bool lockdep;
} irqentry_state_t;
#endif
How is that a union?
> But then when the pkrs stuff is added the union changes back to a structure and
> looks like this.
So you want:
1) Move stuff to struct irqentry_state (my patch)
2) Change it to a union and pass it as pointer at the same time
3) Change it back to struct to add PKRS
> Is that clear?
What's clear is that the above is nonsense. We can just do
#ifndef irqentry_state
typedef struct irqentry_state {
union {
bool exit_rcu;
bool lockdep;
};
} irqentry_state_t;
#endif
right in the patch which I gave you. Because that actually makes sense.
Thanks,
tglx