Problem with commit bf22ff45bed664aefb5c4e43029057a199b7070c

Problem with commit bf22ff45bed664aefb5c4e43029057a199b7070c

[Juergen Gross]

[-- Attachment #1: Type: text/plain, Size: 831 bytes --]

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(). Unfortunately above commit will let the unmask operation
be a nop as the irq handling doesn't know about the masking done before.

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?


Juergen

[-- Attachment #2: mask.patch --]
[-- Type: text/x-patch, Size: 2129 bytes --]

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 2e567d8433b3..79e82534a3c3 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -346,9 +346,17 @@ static void bind_evtchn_to_cpu(unsigned int chn, unsigned int cpu)
 static void xen_evtchn_mask_all(void)
 {
 	unsigned int evtchn;
+	int irq;
+	struct irq_desc *desc;
 
-	for (evtchn = 0; evtchn < xen_evtchn_nr_channels(); evtchn++)
-		mask_evtchn(evtchn);
+	for (evtchn = 0; evtchn < xen_evtchn_nr_channels(); evtchn++) {
+		irq = get_evtchn_to_irq(evtchn);
+		desc = irq_to_desc(irq);
+		if (desc)
+			mask_irq(desc);
+		else
+			mask_evtchn(evtchn);
+	}
 }
 
 /**
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 00db35b61e9e..4b6ab670639a 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -509,6 +509,7 @@ extern int setup_irq(unsigned int irq, struct irqaction *new);
 extern void remove_irq(unsigned int irq, struct irqaction *act);
 extern int setup_percpu_irq(unsigned int irq, struct irqaction *new);
 extern void remove_percpu_irq(unsigned int irq, struct irqaction *act);
+extern void mask_irq(struct irq_desc *desc);
 
 extern void irq_cpu_online(void);
 extern void irq_cpu_offline(void);
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 2e30d925a40d..7e25a1ad2857 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -408,6 +408,7 @@ void mask_irq(struct irq_desc *desc)
 		irq_state_set_masked(desc);
 	}
 }
+EXPORT_SYMBOL_GPL(mask_irq);
 
 void unmask_irq(struct irq_desc *desc)
 {
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index 9da14d125df4..5e2bc4d799af 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -81,7 +81,6 @@ extern void irq_enable(struct irq_desc *desc);
 extern void irq_disable(struct irq_desc *desc);
 extern void irq_percpu_enable(struct irq_desc *desc, unsigned int cpu);
 extern void irq_percpu_disable(struct irq_desc *desc, unsigned int cpu);
-extern void mask_irq(struct irq_desc *desc);
 extern void unmask_irq(struct irq_desc *desc);
 extern void unmask_threaded_irq(struct irq_desc *desc);
 

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Problem with commit bf22ff45bed664aefb5c4e43029057a199b7070c

[Thomas Gleixner]

On Wed, 12 Jul 2017, Thomas Gleixner wrote:
> On Mon, 10 Jul 2017, Juergen Gross wrote:
> > 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).
> 
> The untested patch below should give you hooks to do what you need to do.
> 
> Add the irq_suspend/resume callbacks and set the IRQCHIP_GENERIC_SUSPEND
> flag on your xen irqchip, so it actually gets invoked.
> 
> I have to make that opt in right now because the callbacks are used in the
> generic irqchip implementation already. We can revisit that when you can
> confirm that this is actually solving the problem.

There might be an even simpler solution.

As this is using the regular suspend_device_irqs() call, you just might get
away with setting IRQCHIP_MASK_ON_SUSPEND for your irq chip. That does not
use the lazy disable approach, it also masks all interrupts which are not
marked as wakeup irqs. I assume none of them is when you do that
save/restore dance.

That said, you still might make the whole mechanism cleaner by using the
irq chip callbacks so you can avoid traversing all the interrupts another
time. But I can't say for sure as I got lost in that xen event channel code.

Thanks,

	tglx

Re: Problem with commit bf22ff45bed664aefb5c4e43029057a199b7070c

[Thomas Gleixner]

On Wed, 12 Jul 2017, Thomas Gleixner wrote:
> On Mon, 10 Jul 2017, Juergen Gross wrote:
> > 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).
> 
> The untested patch below should give you hooks to do what you need to do.
> 
> Add the irq_suspend/resume callbacks and set the IRQCHIP_GENERIC_SUSPEND
> flag on your xen irqchip, so it actually gets invoked.
> 
> I have to make that opt in right now because the callbacks are used in the
> generic irqchip implementation already. We can revisit that when you can
> confirm that this is actually solving the problem.

There might be an even simpler solution.

As this is using the regular suspend_device_irqs() call, you just might get
away with setting IRQCHIP_MASK_ON_SUSPEND for your irq chip. That does not
use the lazy disable approach, it also masks all interrupts which are not
marked as wakeup irqs. I assume none of them is when you do that
save/restore dance.

That said, you still might make the whole mechanism cleaner by using the
irq chip callbacks so you can avoid traversing all the interrupts another
time. But I can't say for sure as I got lost in that xen event channel code.

Thanks,

	tglx


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Problem with commit bf22ff45bed664aefb5c4e43029057a199b7070c

[Thomas Gleixner]

On Mon, 10 Jul 2017, Juergen Gross wrote:
> 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.

Fair enough!

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

The untested patch below should give you hooks to do what you need to do.

Add the irq_suspend/resume callbacks and set the IRQCHIP_GENERIC_SUSPEND
flag on your xen irqchip, so it actually gets invoked.

I have to make that opt in right now because the callbacks are used in the
generic irqchip implementation already. We can revisit that when you can
confirm that this is actually solving the problem.

Thanks,

	tglx

8<----------------------
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -476,6 +476,8 @@ struct irq_chip {
  * IRQCHIP_SKIP_SET_WAKE:	Skip chip.irq_set_wake(), for this irq chip
  * IRQCHIP_ONESHOT_SAFE:	One shot does not require mask/unmask
  * IRQCHIP_EOI_THREADED:	Chip requires eoi() on unmask in threaded mode
+ * IRQCHIP_GENERIC_SUSPEND:	Use the suspend/resume callbacks in
+ *				device_irq_suspend/resume
  */
 enum {
 	IRQCHIP_SET_TYPE_MASKED		= (1 <<  0),
@@ -485,6 +487,7 @@ enum {
 	IRQCHIP_SKIP_SET_WAKE		= (1 <<  4),
 	IRQCHIP_ONESHOT_SAFE		= (1 <<  5),
 	IRQCHIP_EOI_THREADED		= (1 <<  6),
+	IRQCHIP_GENERIC_SUSPEND		= (1 <<  7),
 };
 
 #include <linux/irqdesc.h>
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -70,6 +70,8 @@ void irq_pm_remove_action(struct irq_des
 
 static bool suspend_device_irq(struct irq_desc *desc)
 {
+	struct irq_chip *chip;
+
 	if (!desc->action || irq_desc_is_chained(desc) ||
 	    desc->no_suspend_depth)
 		return false;
@@ -94,8 +96,13 @@ static bool suspend_device_irq(struct ir
 	 * chip level. The chip implementation indicates that with
 	 * IRQCHIP_MASK_ON_SUSPEND.
 	 */
-	if (irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_ON_SUSPEND)
+	chip = irq_desc_get_chip(desc);
+	if (chip->flags & IRQCHIP_MASK_ON_SUSPEND)
 		mask_irq(desc);
+
+	if ((chip->flags & IRQCHIP_GENERIC_SUSPEND) && chip->irq_suspend)
+		chip->irq_suspend(&desc->irq_data);
+
 	return true;
 }
 
@@ -138,6 +145,8 @@ EXPORT_SYMBOL_GPL(suspend_device_irqs);
 
 static void resume_irq(struct irq_desc *desc)
 {
+	struct irq_chip *chip;
+
 	irqd_clear(&desc->irq_data, IRQD_WAKEUP_ARMED);
 
 	if (desc->istate & IRQS_SUSPENDED)
@@ -150,6 +159,10 @@ static void resume_irq(struct irq_desc *
 	/* Pretend that it got disabled ! */
 	desc->depth++;
 resume:
+	chip = irq_desc_get_chip(desc);
+	if ((chip->flags & IRQCHIP_GENERIC_SUSPEND) && chip->irq_resume)
+		chip->irq_resume(&desc->irq_data);
+
 	desc->istate &= ~IRQS_SUSPENDED;
 	__enable_irq(desc);
 }

Re: Problem with commit bf22ff45bed664aefb5c4e43029057a199b7070c

[Thomas Gleixner]

On Mon, 10 Jul 2017, Juergen Gross wrote:
> 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.

Fair enough!

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

The untested patch below should give you hooks to do what you need to do.

Add the irq_suspend/resume callbacks and set the IRQCHIP_GENERIC_SUSPEND
flag on your xen irqchip, so it actually gets invoked.

I have to make that opt in right now because the callbacks are used in the
generic irqchip implementation already. We can revisit that when you can
confirm that this is actually solving the problem.

Thanks,

	tglx

8<----------------------
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -476,6 +476,8 @@ struct irq_chip {
  * IRQCHIP_SKIP_SET_WAKE:	Skip chip.irq_set_wake(), for this irq chip
  * IRQCHIP_ONESHOT_SAFE:	One shot does not require mask/unmask
  * IRQCHIP_EOI_THREADED:	Chip requires eoi() on unmask in threaded mode
+ * IRQCHIP_GENERIC_SUSPEND:	Use the suspend/resume callbacks in
+ *				device_irq_suspend/resume
  */
 enum {
 	IRQCHIP_SET_TYPE_MASKED		= (1 <<  0),
@@ -485,6 +487,7 @@ enum {
 	IRQCHIP_SKIP_SET_WAKE		= (1 <<  4),
 	IRQCHIP_ONESHOT_SAFE		= (1 <<  5),
 	IRQCHIP_EOI_THREADED		= (1 <<  6),
+	IRQCHIP_GENERIC_SUSPEND		= (1 <<  7),
 };
 
 #include <linux/irqdesc.h>
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -70,6 +70,8 @@ void irq_pm_remove_action(struct irq_des
 
 static bool suspend_device_irq(struct irq_desc *desc)
 {
+	struct irq_chip *chip;
+
 	if (!desc->action || irq_desc_is_chained(desc) ||
 	    desc->no_suspend_depth)
 		return false;
@@ -94,8 +96,13 @@ static bool suspend_device_irq(struct ir
 	 * chip level. The chip implementation indicates that with
 	 * IRQCHIP_MASK_ON_SUSPEND.
 	 */
-	if (irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_ON_SUSPEND)
+	chip = irq_desc_get_chip(desc);
+	if (chip->flags & IRQCHIP_MASK_ON_SUSPEND)
 		mask_irq(desc);
+
+	if ((chip->flags & IRQCHIP_GENERIC_SUSPEND) && chip->irq_suspend)
+		chip->irq_suspend(&desc->irq_data);
+
 	return true;
 }
 
@@ -138,6 +145,8 @@ EXPORT_SYMBOL_GPL(suspend_device_irqs);
 
 static void resume_irq(struct irq_desc *desc)
 {
+	struct irq_chip *chip;
+
 	irqd_clear(&desc->irq_data, IRQD_WAKEUP_ARMED);
 
 	if (desc->istate & IRQS_SUSPENDED)
@@ -150,6 +159,10 @@ static void resume_irq(struct irq_desc *
 	/* Pretend that it got disabled ! */
 	desc->depth++;
 resume:
+	chip = irq_desc_get_chip(desc);
+	if ((chip->flags & IRQCHIP_GENERIC_SUSPEND) && chip->irq_resume)
+		chip->irq_resume(&desc->irq_data);
+
 	desc->istate &= ~IRQS_SUSPENDED;
 	__enable_irq(desc);
 }



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Problem with commit bf22ff45bed664aefb5c4e43029057a199b7070c

[Thomas Gleixner]

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?

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

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

Thanks,

	tglx

Re: Problem with commit bf22ff45bed664aefb5c4e43029057a199b7070c

[Thomas Gleixner]

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?

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

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

Thanks,

	tglx



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Problem with commit bf22ff45bed664aefb5c4e43029057a199b7070c

[Marc Zyngier]

On 07/07/17 15:51, 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(). Unfortunately above commit will let the unmask operation
> be a nop as the irq handling doesn't know about the masking done before.
> 
> 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?

The correct API to prevent an interrupt from firing would be
disable_irq(), which is much more straightforward and is readily exported.

I'm unsure of what the expected flow is though, so there might be some
more fixes to be made in your code.

Thanks,

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

Re: Problem with commit bf22ff45bed664aefb5c4e43029057a199b7070c

[Marc Zyngier]

On 07/07/17 15:51, 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(). Unfortunately above commit will let the unmask operation
> be a nop as the irq handling doesn't know about the masking done before.
> 
> 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?

The correct API to prevent an interrupt from firing would be
disable_irq(), which is much more straightforward and is readily exported.

I'm unsure of what the expected flow is though, so there might be some
more fixes to be made in your code.

Thanks,

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Problem with commit bf22ff45bed664aefb5c4e43029057a199b7070c

[Juergen Gross]

On 07/07/17 18:41, 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(). Unfortunately above commit will let the unmask operation
>> be a nop as the irq handling doesn't know about the masking done before.
> 
> Rightfully so. Making assumptions about the inner workings of core code is
> always wrong.

That was my thought, too. :-)

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

Thank you very much


Juergen

Re: Problem with commit bf22ff45bed664aefb5c4e43029057a199b7070c

[Juergen Gross]

On 07/07/17 18:41, 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(). Unfortunately above commit will let the unmask operation
>> be a nop as the irq handling doesn't know about the masking done before.
> 
> Rightfully so. Making assumptions about the inner workings of core code is
> always wrong.

That was my thought, too. :-)

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

Thank you very much


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Problem with commit bf22ff45bed664aefb5c4e43029057a199b7070c

[Thomas Gleixner]

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(). Unfortunately above commit will let the unmask operation
> be a nop as the irq handling doesn't know about the masking done before.

Rightfully so. Making assumptions about the inner workings of core code is
always wrong.

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

Thanks,

	tglx

Re: Problem with commit bf22ff45bed664aefb5c4e43029057a199b7070c

[Thomas Gleixner]

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(). Unfortunately above commit will let the unmask operation
> be a nop as the irq handling doesn't know about the masking done before.

Rightfully so. Making assumptions about the inner workings of core code is
always wrong.

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

Thanks,

	tglx

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Problem with commit bf22ff45bed664aefb5c4e43029057a199b7070c

[Juergen Gross]

[-- Attachment #1: Type: text/plain, Size: 831 bytes --]

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(). Unfortunately above commit will let the unmask operation
be a nop as the irq handling doesn't know about the masking done before.

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?


Juergen

[-- Attachment #2: mask.patch --]
[-- Type: text/x-patch, Size: 2129 bytes --]

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 2e567d8433b3..79e82534a3c3 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -346,9 +346,17 @@ static void bind_evtchn_to_cpu(unsigned int chn, unsigned int cpu)
 static void xen_evtchn_mask_all(void)
 {
 	unsigned int evtchn;
+	int irq;
+	struct irq_desc *desc;
 
-	for (evtchn = 0; evtchn < xen_evtchn_nr_channels(); evtchn++)
-		mask_evtchn(evtchn);
+	for (evtchn = 0; evtchn < xen_evtchn_nr_channels(); evtchn++) {
+		irq = get_evtchn_to_irq(evtchn);
+		desc = irq_to_desc(irq);
+		if (desc)
+			mask_irq(desc);
+		else
+			mask_evtchn(evtchn);
+	}
 }
 
 /**
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 00db35b61e9e..4b6ab670639a 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -509,6 +509,7 @@ extern int setup_irq(unsigned int irq, struct irqaction *new);
 extern void remove_irq(unsigned int irq, struct irqaction *act);
 extern int setup_percpu_irq(unsigned int irq, struct irqaction *new);
 extern void remove_percpu_irq(unsigned int irq, struct irqaction *act);
+extern void mask_irq(struct irq_desc *desc);
 
 extern void irq_cpu_online(void);
 extern void irq_cpu_offline(void);
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 2e30d925a40d..7e25a1ad2857 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -408,6 +408,7 @@ void mask_irq(struct irq_desc *desc)
 		irq_state_set_masked(desc);
 	}
 }
+EXPORT_SYMBOL_GPL(mask_irq);
 
 void unmask_irq(struct irq_desc *desc)
 {
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index 9da14d125df4..5e2bc4d799af 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -81,7 +81,6 @@ extern void irq_enable(struct irq_desc *desc);
 extern void irq_disable(struct irq_desc *desc);
 extern void irq_percpu_enable(struct irq_desc *desc, unsigned int cpu);
 extern void irq_percpu_disable(struct irq_desc *desc, unsigned int cpu);
-extern void mask_irq(struct irq_desc *desc);
 extern void unmask_irq(struct irq_desc *desc);
 extern void unmask_threaded_irq(struct irq_desc *desc);