On Mon, Dec 06, 2021 at 05:54:23PM -0800, 'Ira Weiny' wrote:
[snip]
> >
> > Though, if you look at the xen_pv_evtchn_do_upcall() part where you
> > added this extra invocation you might figure out that adding
> > pkrs_restore_irq() to irqentry_exit_cond_resched() and explicitely to
> > the 'else' path in irqentry_exit() makes it magically consistent for
> > both use cases.
> >
>
> Thank you, yes good catch. However, I think I need at least 1 more call in the
> !regs_irqs_disabled() && state.exit_rcu case right?
>
> 11:29:48 > git di
> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
> index 717091910ebc..667676ebc129 100644
> --- a/arch/x86/entry/common.c
> +++ b/arch/x86/entry/common.c
> @@ -363,8 +363,6 @@ __visible noinstr void xen_pv_evtchn_do_upcall(struct pt_regs *regs)
>
> inhcall = get_and_clear_inhcall();
> if (inhcall && !WARN_ON_ONCE(state.exit_rcu)) {
> - /* Normally called by irqentry_exit, restore pkrs here */
> - pkrs_restore_irq(regs);
> irqentry_exit_cond_resched();
> instrumentation_end();
> restore_inhcall(inhcall);
> diff --git a/kernel/entry/common.c b/kernel/entry/common.c
> index 1c0a70a17e93..60ae3d4c9cc0 100644
> --- a/kernel/entry/common.c
> +++ b/kernel/entry/common.c
> @@ -385,6 +385,7 @@ noinstr irqentry_state_t irqentry_enter(struct pt_regs *regs)
>
> void irqentry_exit_cond_resched(void)
Opps... Of course regs will need to be passed in here now...
Ira
> {
> + pkrs_restore_irq(regs);
> if (!preempt_count()) {
> /* Sanity check RCU and thread stack */
> rcu_irq_exit_check_preempt();
> @@ -408,8 +409,6 @@ noinstr void irqentry_exit(struct pt_regs *regs, irqentry_state_t state)
> return;
> }
>
> - pkrs_restore_irq(regs);
> -
> if (!regs_irqs_disabled(regs)) {
> /*
> * If RCU was not watching on entry this needs to be done
> @@ -421,6 +420,7 @@ noinstr void irqentry_exit(struct pt_regs *regs, irqentry_state_t state)
> /* Tell the tracer that IRET will enable interrupts */
> trace_hardirqs_on_prepare();
> lockdep_hardirqs_on_prepare(CALLER_ADDR0);
> + pkrs_restore_irq(regs);
> instrumentation_end();
> rcu_irq_exit();
> lockdep_hardirqs_on(CALLER_ADDR0);
> @@ -439,6 +439,10 @@ noinstr void irqentry_exit(struct pt_regs *regs, irqentry_state_t state)
> trace_hardirqs_on();
> instrumentation_end();
> } else {
> + instrumentation_begin();
> + pkrs_restore_irq(regs);
> + instrumentation_end();
> +
> /*
> * IRQ flags state is correct already. Just tell RCU if it
> * was not watching on entry.
> @@ -470,8 +474,8 @@ irqentry_state_t noinstr irqentry_nmi_enter(struct pt_regs *regs)
>
> void noinstr irqentry_nmi_exit(struct pt_regs *regs, irqentry_state_t irq_state)
> {
> - pkrs_restore_irq(regs);
> instrumentation_begin();
> + pkrs_restore_irq(regs);
> ftrace_nmi_exit();
> if (irq_state.lockdep) {
> trace_hardirqs_on_prepare();
>
>
> Thank you again for the review,
> Ira
>
>
> [0] https://lore.kernel.org/lkml/20210804043231.2655537-5-ira.weiny@xxxxxxxxx/
> [1] https://lore.kernel.org/lkml/20201217131924.GW3040@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
> [2] https://lore.kernel.org/lkml/20201216013202.GY1563847@xxxxxxxxxxxxxxxxxxxxxxxxx/
> [3] https://lore.kernel.org/lkml/87y2hwqwng.fsf@xxxxxxxxxxxxxxxxxxxxxxx/
>
> > Thanks,
> >
> > tglx
