Re: [Xen-devel] xen/evtchn and forced threaded irq
From: Stefano Stabellini
Date: Mon Apr 27 2020 - 19:20:15 EST
On Thu, 21 Feb 2019, Julien Grall wrote:
> Hi Roger,
>
> On Thu, 21 Feb 2019, 08:08 Roger Pau MonnÃ, <roger.pau@xxxxxxxxxx> 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.From ce26c371a8ff7b49c98a3b8c7b57199154cbca59 Mon Sep 17 00:00:00 2001
From: Stefano Stabellini <sstabellini@xxxxxxxxxx>
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@xxxxxxx/
Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
---
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
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