Re: [PATCH] x86: enable interrupts around dump_execstate()

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Sep 15, 2022 at 10:01:11AM +0200, Jan Beulich wrote:
> On 14.09.2022 16:23, Roger Pau Monné wrote:
> > On Wed, Sep 14, 2022 at 12:13:49PM +0200, Jan Beulich wrote:
> >> On 14.09.2022 11:13, Roger Pau Monné wrote:
> >>> On Wed, Sep 14, 2022 at 10:31:34AM +0200, Jan Beulich wrote:
> >>>> On 14.09.2022 10:14, Jan Beulich wrote:
> >>>>> On 13.09.2022 16:50, Roger Pau Monné wrote:
> >>>>>> On Mon, Dec 13, 2021 at 04:12:55PM +0100, Jan Beulich wrote:
> >>>>>>> show_hvm_stack() requires interrupts to be enabled to avoids 
> >>>>>>> triggering
> >>>>>>> the consistency check in check_lock() for the p2m lock. To do so in
> >>>>>>> spurious_interrupt() requires adding reentrancy protection / handling
> >>>>>>> there.
> >>>>>>
> >>>>>> There's also an ASSERT(!in_irq()) in _percpu_write_lock() that will
> >>>>>> trigger when trying to acquire the p2m lock from spurious_interrupt()
> >>>>>> context, as p2m_lock() -> mm_write_lock() -> _mm_write_lock ->
> >>>>>> percpu_write_lock().
> >>>>>
> >>>>> s/will/may/ since spurious_interrupt() doesn't itself use irq_enter(),
> >>>
> >>> do_IRQ() does call irq_enter(), and that's the caller of
> >>> spurious_interrupt() AFAICT.
> >>
> >> Hmm, you're right. I was mislead by smp_call_function_interrupt()
> >> explicitly using irq_{enter,exit}(). I guess that should have been
> >> removed in b57458c1d02b ("x86: All vectored interrupts go through
> >> do_IRQ()"). I guess I need to either open-code the variant of in_irq()
> >> I'd need, or (perhaps better for overall state) explicitly irq_exit()
> >> before the check and irq_enter() after the call. Thoughts?
> > 
> > Well, it's ugly but it's likely the easier way to get this working.
> 
> Just to clarify - the first of the options I did name is (of course) not
> viable: If we open-coded a local_irq_count() == 1 check here, the
> assertion you named would still trigger.

Oh yes, sorry, I was referring to calling irq_{exit,enter}() around
the show_hvm_stack() call.

I'm slightly worried that this could cause errors with reentrancy in
case we get further interrupts, or even and NMI.

Mutating the environment to make it suitable for what the function
expects seems troublesome in case we get any other interrupts or
exceptions that rely on the state being correct.

But again I cannot see a good way to sort this out short of
introducing an unlocked p2m walker (and related accessors) to use
under this conditions.  I haven't looked myself, but I would expect
this to be a non-trivial amount of work.

Thanks, Roger.