[PATCH] x86: enable interrupts around dump_execstate()

[PATCH] x86: enable interrupts around dump_execstate()

[Jan Beulich]

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@suse.com>
---
The obvious (but imo undesirable) alternative is to suppress the call to
show_hvm_stack() when interrupts are disabled.

--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -1386,22 +1386,40 @@ void smp_send_state_dump(unsigned int cp
  */
 void spurious_interrupt(struct cpu_user_regs *regs)
 {
+    static DEFINE_PER_CPU(unsigned int, recursed);
+    unsigned int cpu = smp_processor_id();
+
     /*
      * Check if this is a vectored interrupt (most likely, as this is probably
      * a request to dump local CPU state or to continue NMI handling).
      * Vectored interrupts are ACKed; spurious interrupts are not.
      */
-    if (apic_isr_read(SPURIOUS_APIC_VECTOR)) {
+    while ( apic_isr_read(SPURIOUS_APIC_VECTOR) )
+    {
         bool is_spurious;
 
+        if ( per_cpu(recursed, cpu)++ )
+            return;
+
         ack_APIC_irq();
         is_spurious = !nmi_check_continuation();
-        if (this_cpu(state_dump_pending)) {
-            this_cpu(state_dump_pending) = false;
+
+        if ( per_cpu(state_dump_pending, cpu) )
+        {
+            per_cpu(state_dump_pending, cpu) = false;
+
+            local_irq_enable();
+
             dump_execstate(regs);
-            is_spurious = false;
+
+            local_irq_disable();
+
+            /* (Ab)use is_spurious to arrange for loop continuation. */
+            is_spurious = per_cpu(recursed, cpu) > 1;
         }
 
+        per_cpu(recursed, cpu) = 0;
+
         if ( !is_spurious )
             return;
     }

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

[Andrew Cooper]

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@suse.com>
> ---
> 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, 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...

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.

~Andrew

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

[Jan Beulich]

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@suse.com>
>> ---
>> 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

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

[Jan Beulich]

On 16.12.2021 14: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@suse.com>

There's a bug here which we need to deal with one way or another.
May I please ask for a response to the issues pointed out with
what you said in your earlier reply?

Thanks, Jan

>>> ---
>>> 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
> 
> 

Ping²: [PATCH] x86: enable interrupts around dump_execstate()

[Jan Beulich]

On 11.01.2022 11:08, Jan Beulich wrote:
> On 16.12.2021 14: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@suse.com>
> 
> There's a bug here which we need to deal with one way or another.
> May I please ask for a response to the issues pointed out with
> what you said in your earlier reply?

I sincerely hope we won't ship another major version with this
issue unfixed. The only option beyond applying this patch that I'm
aware of is to revert the commit pointed at by Fixes:, which imo
would be a shame (moving us further away from proper PVH support,
including Dom0).

Thanks, Jan

>>>> ---
>>>> 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
>>
>>
> 

Fwd: Ping²: [PATCH] x86: enable interrupts around dump_execstate()

[Jan Beulich]

Henry,

-------- Forwarded Message --------
Subject: Ping²: [PATCH] x86: enable interrupts around dump_execstate()
Date: Tue, 5 Jul 2022 18:19:38 +0200
From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>, Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>, xen-devel@lists.xenproject.org <xen-devel@lists.xenproject.org>

>On 11.01.2022 11:08, Jan Beulich wrote:
>> On 16.12.2021 14: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@suse.com>
>> 
>> There's a bug here which we need to deal with one way or another.
>> May I please ask for a response to the issues pointed out with
>> what you said in your earlier reply?
>
>I sincerely hope we won't ship another major version with this
>issue unfixed. The only option beyond applying this patch that I'm
>aware of is to revert the commit pointed at by Fixes:, which imo
>would be a shame (moving us further away from proper PVH support,
>including Dom0).

perhaps another item for the list of things needing resolution for
the release.

Jan

RE: Ping²: [PATCH] x86: enable interrupts around dump_execstate()

[Henry Wang]

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Henry,
> 
> -------- Forwarded Message --------
> Subject: Ping²: [PATCH] x86: enable interrupts around dump_execstate()
> 
> >On 11.01.2022 11:08, Jan Beulich wrote:
> >> On 16.12.2021 14: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@suse.com>
> >>
> >> There's a bug here which we need to deal with one way or another.
> >> May I please ask for a response to the issues pointed out with
> >> what you said in your earlier reply?
> >
> >I sincerely hope we won't ship another major version with this
> >issue unfixed. The only option beyond applying this patch that I'm
> >aware of is to revert the commit pointed at by Fixes:, which imo
> >would be a shame (moving us further away from proper PVH support,
> >including Dom0).
> 
> perhaps another item for the list of things needing resolution for
> the release.

Many thanks for this information! I can see this thread is quite old and
probably even before I became the release manager so thanks for your
effort to find this :))

Yes of course, I've added this series to my blockers list and I will start to
track it so that we can have proper resolution for the 4.17 release.

Kind regards,
Henry

> 
> Jan

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

[Andrew Cooper]

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@suse.com>
>>> ---
>>> 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().

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... ?

>
>> 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.

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.

No amount of playing games with irqs here is going to improve things.

~Andrew

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

[Jan Beulich]

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@suse.com>
>>>> ---
>>>> 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

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

[Roger Pau Monné]

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().

Thanks, Roger.

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

[Jan Beulich]

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(),
but yes - we could nest inside a lower priority interrupt. I'll make
local_irq_enable() depend on !in_irq().

Jan

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

[Jan Beulich]

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(),
> but yes - we could nest inside a lower priority interrupt. I'll make
> local_irq_enable() depend on !in_irq().

Upon further thought I guess more precautions are necessary: We might
have interrupted code holding the P2M lock already, and we might also
have interrupted code holding another MM lock precluding acquiring of
the P2M lock. All of this probably plays into Andrew's concerns, yet
still I don't view it as a viable route to omit the stack dump for HVM
domains, and in particular for PVH Dom0. Sadly I can't think of any
better approach ...

Jan

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

[Roger Pau Monné]

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.

> > but yes - we could nest inside a lower priority interrupt. I'll make
> > local_irq_enable() depend on !in_irq().
> 
> Upon further thought I guess more precautions are necessary: We might
> have interrupted code holding the P2M lock already, and we might also
> have interrupted code holding another MM lock precluding acquiring of
> the P2M lock. All of this probably plays into Andrew's concerns, yet
> still I don't view it as a viable route to omit the stack dump for HVM
> domains, and in particular for PVH Dom0. Sadly I can't think of any
> better approach ...

Yes, I also had those concerns.  The mm locks are recursive, but
spurious_interrupt() hitting in the middle of code already holding any
mm lock is likely to end up triggering the mm lock order checker.

One (likely very risky option ATM) is to introduce a per pCPU flag
that when set will turn all mm locks into noops, and use it here in
order to avoid any locking issues.  This could introduce two issues at
least: first one is how resilient page walking routines are against
page tables changing under their feet.  The second one is that any
page table walker p2m helper should avoid doing modifications to the
p2m, so no P2M_ALLOC or P2M_UNSHARE flags could be used.

Thanks, Roger.

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

[Jan Beulich]

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?

>>> but yes - we could nest inside a lower priority interrupt. I'll make
>>> local_irq_enable() depend on !in_irq().
>>
>> Upon further thought I guess more precautions are necessary: We might
>> have interrupted code holding the P2M lock already, and we might also
>> have interrupted code holding another MM lock precluding acquiring of
>> the P2M lock. All of this probably plays into Andrew's concerns, yet
>> still I don't view it as a viable route to omit the stack dump for HVM
>> domains, and in particular for PVH Dom0. Sadly I can't think of any
>> better approach ...
> 
> Yes, I also had those concerns.  The mm locks are recursive, but
> spurious_interrupt() hitting in the middle of code already holding any
> mm lock is likely to end up triggering the mm lock order checker.

Guarding against this is possible, while ...

> One (likely very risky option ATM) is to introduce a per pCPU flag
> that when set will turn all mm locks into noops, and use it here in
> order to avoid any locking issues.  This could introduce two issues at
> least: first one is how resilient page walking routines are against
> page tables changing under their feet.  The second one is that any
> page table walker p2m helper should avoid doing modifications to the
> p2m, so no P2M_ALLOC or P2M_UNSHARE flags could be used.

... personally I view this as too risky.

Jan

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

[Roger Pau Monné]

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.

> >>> but yes - we could nest inside a lower priority interrupt. I'll make
> >>> local_irq_enable() depend on !in_irq().
> >>
> >> Upon further thought I guess more precautions are necessary: We might
> >> have interrupted code holding the P2M lock already, and we might also
> >> have interrupted code holding another MM lock precluding acquiring of
> >> the P2M lock. All of this probably plays into Andrew's concerns, yet
> >> still I don't view it as a viable route to omit the stack dump for HVM
> >> domains, and in particular for PVH Dom0. Sadly I can't think of any
> >> better approach ...
> > 
> > Yes, I also had those concerns.  The mm locks are recursive, but
> > spurious_interrupt() hitting in the middle of code already holding any
> > mm lock is likely to end up triggering the mm lock order checker.
> 
> Guarding against this is possible, while ...
> 
> > One (likely very risky option ATM) is to introduce a per pCPU flag
> > that when set will turn all mm locks into noops, and use it here in
> > order to avoid any locking issues.  This could introduce two issues at
> > least: first one is how resilient page walking routines are against
> > page tables changing under their feet.  The second one is that any
> > page table walker p2m helper should avoid doing modifications to the
> > p2m, so no P2M_ALLOC or P2M_UNSHARE flags could be used.
> 
> ... personally I view this as too risky.

Is the dump of the stack only used for the debug key handler, or there
are other places this is also used?

Thanks, Roger.

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

[Jan Beulich]

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.

>>>>> but yes - we could nest inside a lower priority interrupt. I'll make
>>>>> local_irq_enable() depend on !in_irq().
>>>>
>>>> Upon further thought I guess more precautions are necessary: We might
>>>> have interrupted code holding the P2M lock already, and we might also
>>>> have interrupted code holding another MM lock precluding acquiring of
>>>> the P2M lock. All of this probably plays into Andrew's concerns, yet
>>>> still I don't view it as a viable route to omit the stack dump for HVM
>>>> domains, and in particular for PVH Dom0. Sadly I can't think of any
>>>> better approach ...
>>>
>>> Yes, I also had those concerns.  The mm locks are recursive, but
>>> spurious_interrupt() hitting in the middle of code already holding any
>>> mm lock is likely to end up triggering the mm lock order checker.
>>
>> Guarding against this is possible, while ...
>>
>>> One (likely very risky option ATM) is to introduce a per pCPU flag
>>> that when set will turn all mm locks into noops, and use it here in
>>> order to avoid any locking issues.  This could introduce two issues at
>>> least: first one is how resilient page walking routines are against
>>> page tables changing under their feet.  The second one is that any
>>> page table walker p2m helper should avoid doing modifications to the
>>> p2m, so no P2M_ALLOC or P2M_UNSHARE flags could be used.
>>
>> ... personally I view this as too risky.
> 
> Is the dump of the stack only used for the debug key handler, or there
> are other places this is also used?

It's called from show_execution_state(), which also dumps state for e.g.
crashes or WARN_ON()s.

Jan

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

[Roger Pau Monné]

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.