Re: panic kexec broken on ARM64?

[Marc Zyngier <marc.zyngier@arm.com>]

On 05/07/18 11:13, takahiro.akashi@linaro.org wrote:
> On Wed, Jul 04, 2018 at 10:02:24AM +0100, Marc Zyngier wrote:
>> On 04/07/18 09:41, takahiro.akashi@linaro.org wrote:
>>> On Tue, Jul 03, 2018 at 09:58:44AM +0100, Marc Zyngier wrote:
>>>> On 03/07/18 08:01, takahiro.akashi@linaro.org wrote:
>>>>> Marc, James,
>>>>>
>>>>> I'd like to re-ignite the discussion.
>>>>>
>>>>> On Sun, Jun 10, 2018 at 01:24:17PM +0100, Marc Zyngier wrote:
>>>>>> On Wed, 06 Jun 2018 12:37:02 +0100,
>>>>>> James Morse wrote:
>>>>>>>
>>>>>>> Hi Stefan,
>>>>>>>
>>>>>>> On 06/06/18 08:02, Stefan Wahren wrote:
>>>>>>>> Am 05.06.2018 um 19:46 schrieb James Morse:
>>>>>>>>> On 05/06/18 09:01, Petr Tesarik wrote:
>>>>>>>>>> I attached a hardware debugger and found
>>>>>>>>>> out that all CPU cores were stopped except one which was stuck in the
>>>>>>>>>> idle thread. It seems that irq_set_irqchip_state() may sleep, which is
>>>>>>>>>> definitely not safe after a kernel panic.
>>>>>>>
>>>>>>>>> I don't know much about irqchip stuff, but __irq_get_desc_lock() takes a
>>>>>>>>> raw_spin_lock(), and calls gic_irq_get_irqchip_state() which is just poking
>>>>>>>>> around in mmio registers, this should all be safe unless you re-entered the same
>>>>>>>>> code.
>>>>>>>
>>>>>>>>>> If I'm right, then this is broken in general, but I have only ever seen
>>>>>>>>>> it on RPi 3 Model B+ (even RPi3 Model B works fine), so the issue may
>>>>>>>>>> be more subtle.
>>>>>>>
>>>>>>>>> Is there a hardware difference around the interrupt controller on these?
>>>>>>>
>>>>>>>> No, but the RPi 3 B has a different USB network chip on board (smsc95xx, Fast
>>>>>>>> ethernet) instead of lan78xx (Gigabit ethernet).
>>>>>>>
>>>>>>> Bingo: its the lan78xx driver that is sleeping from the irqchip
>>>>>>> callbacks; The smsc95xx driver doesn't have a struct irq_chip, which
>>>>>>> is why the RPi-3-B doesn't do this.
>>>>>>>
>>>>>>> It may be valid for kdump to only teardown the 'root irqdomain' (if
>>>>>>> that even means anything). I assume these secondary irqchip's would
>>>>>>> have a summary-interrupt that goes to another irqchip. But I can't
>>>>>>> see a way to tell them apart..,
>>>>>>
>>>>>> There is none. A cascaded irqchip is just like a root irqchip, just
>>>>>> that its output line is connected to another irqchip. But we have no
>>>>>> easy way to identify the parent. Also, this particular driver looks
>>>>>> quite creative (it reinvents the wheel for chained interrupts -- see
>>>>>> intr_complete and lan78xx_status), meaning that even if we could have
>>>>>> a magic way of identify a chained irqchip, we'd miss that one. Broken.
>>>>>>
>>>>>>> I think we need to wait until after the merge window for Marc's
>>>>>>> wisdom on this!
>>>>>>
>>>>>> Overall, I can't think of an easy fix. We have a few options, but none
>>>>>> of them involve a centralised change:
>>>>>>
>>>>>> 1) We provide a reset infrastructure for irqchips, with an opt-in
>>>>>>    mechanism. This involves changing the way we teardown irqs at
>>>>>>    crash-time, and we'd then need some notion of reset ordering (think
>>>>>>    of the layered ITS and GICv3, for example).
>>>>>
>>>>> Does this mean that all the irqchips have to be implemented with reset?
>>>>
>>>> No. Only those that want to be reset at kexec time.
>>>
>>> I don't get the point yet. Who should have reset interface?
>>> What is the criteria?
>>
>> The criteria is "this irqchip requires a reset to be safely used in the
>> secondary kernel". This is a judgement call from the person writing the
>> driver.
> 
> This doesn't tell me anything more than "do it if you need it."
> So let me ask you in other words.
> Does gic driver need to provide a reset function?
> Whether yes or no, why do you think so?

Because I know the architecture and I can assess that it needs it. Case
in point: The RDs have memory tables. kexec without disabling LPIs, and
you end-up with memory corruption.

Sorry, but there is no magic bullet. You have to understand what you're
doing.

> 
>>
>>>
>>>>>>
>>>>>> 2) We provide a way to identify interrupts that are ultimately backed
>>>>>>    by a root controller, which implies walking down the hierarchy for
>>>>>
>>>>> To be clear, from bottom to top (or root), right?
>>>>
>>>> I'm not sure I understand your question. The idea is to walk the
>>>> irq_data chain, until we hit a root irqchip. If we do hit one, we
>>>> deactivate/eoi/disable this interrupt. If we don't, we do nothing.
>>>
>>> I thought that we would traverse the (chained irq) hierarchy from
>>> bottom to top and call deactivate or others in that order.
>>> Am I wrong here?
>>
>> You *cannot* traverse a hierarchy through a chained irq. At that stage,
>> irq_data->parent_data is NULL.
> 
> What do you mean by "at that stage?"

At the point you reach a secondary interrupt controller that exposese a
chained IRQ.

> In your patch, "genirq: Add predicate for irq served by root irqchip,"
> irq_irqchip_is_root() dereferences its parent_data.

And? Hierarchies and chained controllers are two orthogonal concepts.
You can have a hierarchy on top of a chained controller, and that
doesn't make it a root controller.

	M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec