Re: Problem with commit bf22ff45bed664aefb5c4e43029057a199b7070c

[Juergen Gross]

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