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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.07.2022 13:22, Andrew Cooper wrote:
> On 16/12/2021 13:33, Jan Beulich wrote:
>> On 16.12.2021 12:54, Andrew Cooper wrote:
>>> On 13/12/2021 15:12, 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.
>>>>
>>>> Fixes: adb715db698b ("x86/HVM: also dump stacks from 
>>>> show_execution_state()")
>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>> ---
>>>> The obvious (but imo undesirable) alternative is to suppress the call to
>>>> show_hvm_stack() when interrupts are disabled.
>>> show_execution_state() need to work in any context including the #DF
>>> handler,
>> Why? There's no show_execution_state() on that path.
> 
> Yes there is - it's reachable from any BUG().

"That path" was really referring to you mentioning #DF.

> It's also reachable on the NMI path via fatal_trap().
> 
> Talking of, didn't you say you'd found an unexplained deadlock with NMI
> handling... ?

Entirely unrelated to this, but yes.

>>> and
>>>
>>>     /*
>>>      * Stop interleaving prevention: The necessary P2M lookups
>>>      * involve locking, which has to occur with IRQs enabled.
>>>      */
>>>     console_unlock_recursive_irqrestore(flags);
>>>     
>>>     show_hvm_stack(curr, regs);
>>>
>>> is looking distinctly dodgy...
>> Well, yes, it does.
> 
> Because it is.
> 
> You cannot enable interrupts here, because you have no clue if it safe
> to do so.

We're not enabling interrupts here (if "here" is referring to the
quoted piece of code), we're merely restoring them. When they were
off before, they will continue to be off. (In that light calling
show_hvm_stack() is then still wrong in that case.)

If, otoh, you're talking about what the patch is doing, then
we're in an IRQ handler, so context outside of the IRQ must have
had IRQs enabled.

> What you are doing is creating yet another instance of the broken
> pattern we already have with shutdown trying to move itself to CPU0,
> that occasionally explodes in the middle of a context switch.
> 
> Furthermore show_execution_state() it is already broken for any path
> where interrupts are already disabled, including but not limited to the
> one you've found here.
> 
> adb715db698bc8ec3b88c24eb88b21e9da5b6c07 is broken and needs reverting.

Well, okay - but what's the plan then to achieve the intended
functionality?

The suggested alternative with the patch submission (to skip
show_hvm_stack() when IRQs are off) is probably necessary anyway
due to above observation (if we wouldn't outright revert), but
won't get us very far.

Jan