Re: Problem with commit bf22ff45bed664aefb5c4e43029057a199b7070c

From: Juergen Gross
Date: Mon Jul 10 2017 - 03:24:01 EST


On 07/07/17 19:11, Thomas Gleixner wrote:
> On Fri, 7 Jul 2017, Thomas Gleixner wrote:
>
>> On Fri, 7 Jul 2017, Juergen Gross wrote:
>>
>>> Commit bf22ff45bed664aefb5c4e43029057a199b7070c ("genirq: Avoid
>>> unnecessary low level irq function calls") breaks Xen guest
>>> save/restore handling.
>>>
>>> The main problem are the PV devices using Xen event channels as
>>> interrupt sources which are represented as an "irq chip" in the kernel.
>>> When saving the guest the event channels are masked internally. At
>>> restore time event channels are re-established and unmasked via
>>> irq_startup().
>
> And how exactly gets irq_startup() invoked on those event channels?

[ 30.791879] Call Trace:
[ 30.791883] ? irq_get_irq_data+0xe/0x20
[ 30.791886] enable_dynirq+0x23/0x30
[ 30.791888] unmask_irq.part.33+0x26/0x40
[ 30.791890] irq_enable+0x65/0x70
[ 30.791891] irq_startup+0x3c/0x110
[ 30.791893] __enable_irq+0x37/0x60
[ 30.791895] resume_irqs+0xbe/0xe0
[ 30.791897] irq_pm_syscore_resume+0x13/0x20
[ 30.791900] syscore_resume+0x50/0x1b0
[ 30.791902] xen_suspend+0x76/0x140

>
>>> I have a patch repairing the issue, but I'm not sure if this way to do
>>> it would be accepted. I have exported mask_irq() and I'm doing the
>>> masking now through this function. Would the attached patch be
>>> acceptable? Or is there a better way to solve the problem?
>>
>> Without looking at the patch (too lazy to fiddle with attachments right
>> now), this is definitely wrong. I'll have a look later tonight.
>
> Not that I'm surprised, but that patch is exactly what I expected. Export a
> random function, which helps to paper over the real problem and run away.
> These functions are internal for a reason and we worked hard on making
> people understand that fiddling with the internals of interrupts is a
> NONO. If there are special requirements for a good reason, then we create
> proper interfaces and infrastructure, if there is no good reason, then the
> problematic code needs to be fixed. There is no exception for XEN.

I'm absolutely on your side here. That was the reason I didn't send
the patch right away, but asked how to solve my issue in a way which
isn't "quick and dirty". The patch was just the easiest way to explain
what should be the result of the proper solution.

> Can you please explain how that save/restore stuff works and which
> functions are involved?

It is based on suspend/resume framework. The main work to be done
additionally is to disconnect from the pv-backends at save time and
connect to the pv-backends again at restore time.

The main function triggering all that is xen_suspend() (as seen in
above backtrace).


Juergen