[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] Problem with commit bf22ff45bed664aefb5c4e43029057a199b7070c
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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |