Re: [Xen-devel] xen/evtchn and forced threaded irq

[Stefano Stabellini <sstabellini@kernel.org>]

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

On Thu, 21 Feb 2019, Julien Grall wrote:
> Hi Roger,
> 
> On Thu, 21 Feb 2019, 08:08 Roger Pau Monné, <roger.pau@citrix.com> wrote:
>       FWIW, you can also mask the interrupt while waiting for the thread to
>       execute the interrupt handler. Ie:
> 
> 
> Thank you for providing steps, however where would the masking be done? By the irqchip or a custom solution?
> 
> 
>       1. Interrupt injected
>       2. Execute guest event channel callback
>       3. Scan for pending interrupts
>       4. Mask interrupt
>       5. Clear pending field
>       6. Queue threaded handler
>       7. Go to 3 until all interrupts are drained
>       [...]
>       8. Execute interrupt handler in thread
>       9. Unmask interrupt
> 
>       That should prevent you from stacking interrupts?

Sorry for coming late to the thread, and thanks Julien for pointing it
out to me. I am afraid I was the one to break the flow back in 2011 with
the following commit:

  7e186bdd0098 xen: do not clear and mask evtchns in __xen_evtchn_do_upcall

Oops :-)


Xen event channels have their own workflow; the one Roger wrote above.
They used to be handled using handle_fasteoi_irq until 7e186bdd0098,
then I switched (almost) all of them to handle_edge_irq.

Looking closely at irq handling again, it doesn't look like we can do
what we need with handle_edge_irq today: we can't mask the event channel
before clearing it. But we can do that if we go back to using
handle_fasteoi_irq.

In fact, I managed to verify that LinuxRT works fine as dom0 with the
attached dynamic.patch that switches back xen_dynamic_chip IRQs to
handle_fasteoi_irq.


From the rest of this thread, it looks like the issue might appear with
PIRQs as well. Thus, I wrote a second patch pirqs.patch to switch back
to handle_fasteoi_irq PIRQs as well. However, Xen on ARM does not use
PIRQs so I couldn't test it at all. I would appreciate if Boris/Juegen
tested it. Let me know what you want me to do with the second patch.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=dynamic.patch, Size: 2643 bytes --]

From ce26c371a8ff7b49c98a3b8c7b57199154cbca59 Mon Sep 17 00:00:00 2001
From: Stefano Stabellini <sstabellini@kernel.org>
Date: Mon, 27 Apr 2020 16:15:26 -0700
Subject: [PATCH] xen: use handle_fasteoi_irq to handle xen events

When handling Xen events, we need to make sure the following sequence is
followed:

- mask event
- handle event and clear event (the order does not matter)
- unmask event

It is not possible to implement this flow with handle_edge_irq, so
switch back to handle_fasteoi_irq. Please note that Xen event irqs are
ONESHOT. Also note that handle_fasteoi_irq was in-use before the
following commit, that is partially reverted by this patch:

7e186bdd0098 xen: do not clear and mask evtchns in __xen_evtchn_do_upcall

PIRQ handling is left unchanged.

This patch fixes a domU hang observed when using LinuxRT as dom0 kernel.

Link: https://lore.kernel.org/lkml/5e256d9a-572c-e01e-7706-407f99245b00@arm.com/
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
 drivers/xen/events/events_base.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 499eff7d3f65..5f9b8104dbcf 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -845,7 +845,7 @@ int bind_evtchn_to_irq(unsigned int evtchn)
 			goto out;
 
 		irq_set_chip_and_handler_name(irq, &xen_dynamic_chip,
-					      handle_edge_irq, "event");
+					      handle_fasteoi_irq, "event");
 
 		ret = xen_irq_info_evtchn_setup(irq, evtchn);
 		if (ret < 0) {
@@ -978,7 +978,7 @@ int bind_virq_to_irq(unsigned int virq, unsigned int cpu, bool percpu)
 						      handle_percpu_irq, "virq");
 		else
 			irq_set_chip_and_handler_name(irq, &xen_dynamic_chip,
-						      handle_edge_irq, "virq");
+						      handle_fasteoi_irq, "virq");
 
 		bind_virq.virq = virq;
 		bind_virq.vcpu = xen_vcpu_nr(cpu);
@@ -1377,12 +1377,6 @@ static void ack_dynirq(struct irq_data *data)
 		clear_evtchn(evtchn);
 }
 
-static void mask_ack_dynirq(struct irq_data *data)
-{
-	disable_dynirq(data);
-	ack_dynirq(data);
-}
-
 static int retrigger_dynirq(struct irq_data *data)
 {
 	unsigned int evtchn = evtchn_from_irq(data->irq);
@@ -1585,8 +1579,7 @@ static struct irq_chip xen_dynamic_chip __read_mostly = {
 	.irq_mask		= disable_dynirq,
 	.irq_unmask		= enable_dynirq,
 
-	.irq_ack		= ack_dynirq,
-	.irq_mask_ack		= mask_ack_dynirq,
+	.irq_eoi		= ack_dynirq,
 
 	.irq_set_affinity	= set_affinity_irq,
 	.irq_retrigger		= retrigger_dynirq,
-- 
2.17.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: Type: text/x-diff; name=pirqs.patch, Size: 2492 bytes --]

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 5f9b8104dbcf..57a29c94fefc 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -498,12 +498,6 @@ static void eoi_pirq(struct irq_data *data)
 	}
 }
 
-static void mask_ack_pirq(struct irq_data *data)
-{
-	disable_dynirq(data);
-	eoi_pirq(data);
-}
-
 static unsigned int __startup_pirq(unsigned int irq)
 {
 	struct evtchn_bind_pirq bind_pirq;
@@ -684,13 +678,9 @@ int xen_bind_pirq_gsi_to_irq(unsigned gsi,
 	}
 
 	pirq_query_unmask(irq);
-	/* We try to use the handler with the appropriate semantic for the
-	 * type of interrupt: if the interrupt is an edge triggered
-	 * interrupt we use handle_edge_irq.
-	 *
-	 * On the other hand if the interrupt is level triggered we use
-	 * handle_fasteoi_irq like the native code does for this kind of
-	 * interrupts.
+	/* We use handle_fasteoi_irq for PIRQs because we want to keep
+	 * the evtchn masked while handling and clearing the event.
+	 * Unmasking the evtchn should only happen after clearing it.
 	 *
 	 * Depending on the Xen version, pirq_needs_eoi might return true
 	 * not only for level triggered interrupts but for edge triggered
@@ -699,12 +689,8 @@ int xen_bind_pirq_gsi_to_irq(unsigned gsi,
 	 * hasn't received an eoi yet. Therefore using the fasteoi handler
 	 * is the right choice either way.
 	 */
-	if (shareable)
-		irq_set_chip_and_handler_name(irq, &xen_pirq_chip,
-				handle_fasteoi_irq, name);
-	else
-		irq_set_chip_and_handler_name(irq, &xen_pirq_chip,
-				handle_edge_irq, name);
+	irq_set_chip_and_handler_name(irq, &xen_pirq_chip,
+			handle_fasteoi_irq, name);
 
 out:
 	mutex_unlock(&irq_mapping_update_lock);
@@ -739,7 +725,7 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc,
 		goto out;
 
 	for (i = 0; i < nvec; i++) {
-		irq_set_chip_and_handler_name(irq + i, &xen_pirq_chip, handle_edge_irq, name);
+		irq_set_chip_and_handler_name(irq + i, &xen_pirq_chip, handle_fasteoi_irq, name);
 
 		ret = xen_irq_info_pirq_setup(irq + i, 0, pirq + i, 0, domid,
 					      i == 0 ? 0 : PIRQ_MSI_GROUP);
@@ -1596,9 +1582,7 @@ static struct irq_chip xen_pirq_chip __read_mostly = {
 	.irq_mask		= disable_dynirq,
 	.irq_unmask		= enable_dynirq,
 
-	.irq_ack		= eoi_pirq,
 	.irq_eoi		= eoi_pirq,
-	.irq_mask_ack		= mask_ack_pirq,
 
 	.irq_set_affinity	= set_affinity_irq,
 
-- 
2.17.1