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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> 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. If you have any better idea ...

> For these kinds of purposes, it ought to be entirely fine to do a
> lockless pagewalk of the p2m, because we have to maintain atomicity of
> updates vs the hardware pagewalk anyway.  We do not care about any side
> effects if the target isn't a RAM page.
> 
> That ought to remove any IRQ problems from the equation.

First - how do you suggest to signal to the page walk logic that there
should be no lock acquired? And then I don't think there's a direct
relationship here with what we need to guarantee correct hardware page
walk behavior. Unless you mean to suggest that here we want to rely on
either locking or interrupts being off (the latter guaranteeing that
flush IPIs wouldn't complete while we're still doing software walking
here).

Jan