Re: [PATCH RFC V3 6/9] x86/entry: Pass irqentry_state_t by reference

2020-10-20 Thread Ira Weiny 
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 {
> > boolexit_rcu;
> > boollockdep;
> > } 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 {
>   boolexit_rcu;
> +   boollockdep;
>  } 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 {
>   boolexit_rcu;
> boollockdep;
> };
>  } 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

Re: [PATCH RFC V3 6/9] x86/entry: Pass irqentry_state_t by reference

2020-10-19 Thread Thomas Gleixner 
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 {
>   boolexit_rcu;
>   boollockdep;
> } 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 {
boolexit_rcu;
+   boollockdep;
 } 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 {
boolexit_rcu;
boollockdep;
};
 } irqentry_state_t;
 #endif

right in the patch which I gave you. Because that actually makes sense.

Thanks,

tglx

Re: [PATCH RFC V3 6/9] x86/entry: Pass irqentry_state_t by reference

2020-10-19 Thread Ira Weiny 
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 
> >> 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 {
> >>boolexit_rcu;
> >> +  boollockdep;
> >>  } 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 {
boolexit_rcu;
boollockdep;
} 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 {
boolexit_rcu;
boollockdep;
};
} 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

Re: [PATCH RFC V3 6/9] x86/entry: Pass irqentry_state_t by reference

2020-10-19 Thread Thomas Gleixner 
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 
>> 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 {
>>  boolexit_rcu;
>> +boollockdep;
>>  } 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

Re: [PATCH RFC V3 6/9] x86/entry: Pass irqentry_state_t by reference

2020-10-18 Thread Ira Weiny 
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, ira.we...@intel.com 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 
> 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 
> ---
>  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 {
>   boolexit_rcu;
> + boollockdep;
>  } 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

Re: [PATCH RFC V3 6/9] x86/entry: Pass irqentry_state_t by reference

2020-10-16 Thread Thomas Gleixner 
On Fri, Oct 16 2020 at 13:45, Peter Zijlstra wrote:
> On Fri, Oct 09, 2020 at 12:42:55PM -0700, ira.we...@intel.com 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 
Date: Fri, 11 Sep 2020 10:09:56 +0200

Add blurb here.

Signed-off-by: Thomas Gleixner 
---
 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 
 
-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)

Re: [PATCH RFC V3 6/9] x86/entry: Pass irqentry_state_t by reference

2020-10-16 Thread Peter Zijlstra 
On Fri, Oct 09, 2020 at 12:42:55PM -0700, ira.we...@intel.com 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.

[PATCH RFC V3 6/9] x86/entry: Pass irqentry_state_t by reference

2020-10-09 Thread ira . weiny 
From: Ira Weiny 

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 
---
 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, );
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, );
}
 }
 #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 
 
-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, );   \
instrumentation_begin();\
__##func (regs);\
instrumentation_end();  \
-   irqentry_exit(regs, state); \
+   irqentry_exit(regs, );\
 }  \
\
 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; \
\
+