Re: [patch V6 07/37] x86/entry: Provide helpers for execute on irqstack

[Andy Lutomirski <luto@kernel.org>]

On Mon, May 18, 2020 at 4:11 PM Andy Lutomirski <luto@kernel.org> wrote:
>
> On Fri, May 15, 2020 at 5:10 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> >
> > Device interrupt handlers and system vector handlers are executed on the
> > interrupt stack. The stack switch happens in the low level assembly entry
> > code. This conflicts with the efforts to consolidate the exit code in C to
> > ensure correctness vs. RCU and tracing.
> >
> > As there is no way to move #DB away from IST due to the MOV SS issue, the
> > requirements vs. #DB and NMI for switching to the interrupt stack do not
> > exist anymore. The only requirement is that interrupts are disabled.
> >
> > That allows to move the stack switching to C code which simplifies the
> > entry/exit handling further because it allows to switch stacks after
> > handling the entry and on exit before handling RCU, return to usermode and
> > kernel preemption in the same way as for regular exceptions.
> >
> > The initial attempt of having the stack switching in inline ASM caused too
> > much headache vs. objtool and the unwinder. After analysing the use cases
> > it was agreed on that having the stack switch in ASM for the price of an
> > indirect call is acceptable as the main users are indirect call heavy
> > anyway and the few system vectors which are empty shells (scheduler IPI and
> > KVM posted interrupt vectors) can run from the regular stack.
> >
> > Provide helper functions to check whether the interrupt stack is already
> > active and whether stack switching is required.
> >
> > 64 bit only for now. 32 bit has a variant of that already. Once this is
> > cleaned up the two implementations might be consolidated as a cleanup on
> > top.
> >
>
> Acked-by: Andy Lutomirski <luto@kernel.org>
>
> Have you tested by forcing a stack trace from the IRQ stack and making
> sure it unwinds all the way out?

Actually, I revoke my ack.  Can you make one of two changes:

Option A: Add an assertion to run_on_irqstack to verify that irq_count
was -1 at the beginning?  I suppose this also means you could just
explicitly write 0 instead of adding and subtracting.

Option B: Make run_on_irqstack() just call the function on the current
stack if we're already on the irq stack.

Right now, it's too easy to mess up and not verify the right
precondition before calling run_on_irqstack().

If you choose A, perhaps add a helper to do the if(irq_needs_irqstack)
dance so that users can just do:

run_on_irqstack_if_needed(...);

instead of checking everything themselves.