xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Stefano Stabellini <sstabellini@kernel.org>
To: jgross@suse.com, boris.ostrovsky@oracle.com
Cc: "Stefano Stabellini" <sstabellini@kernel.org>,
	"Andrew Cooper" <Andrew.Cooper3@citrix.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	julien.grall@gmail.com, "Julien Grall" <julien.grall@arm.com>,
	"Jan Beulich" <JBeulich@suse.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	"Dave P Martin" <dave.martin@arm.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] xen/evtchn and forced threaded irq
Date: Mon, 27 Apr 2020 16:20:11 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.21.2004271552430.29217@sstabellini-ThinkPad-T480s> (raw)
In-Reply-To: <CAF3u54Ct7nBjoLw9Vzb=aZVu=N5Ccp5_k6GxLo_ZSA=YCsco6A@mail.gmail.com>

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


  parent reply	other threads:[~2020-04-27 23:20 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <5e256d9a-572c-e01e-7706-407f99245b00@arm.com>
2019-02-20  0:02 ` Boris Ostrovsky
     [not found] ` <20190220000209.GA4091@localhost.localdomain>
2019-02-20 14:15   ` Julien Grall
     [not found]   ` <a872d480-9f1b-6cd7-e507-ac4fcdf705af@arm.com>
2019-02-20 17:07     ` Boris Ostrovsky
     [not found]     ` <21214d47-9c68-e6bf-007a-4047cc2b02f9@oracle.com>
2019-02-20 18:05       ` Julien Grall
     [not found]       ` <8f7445d7-fa50-f3e9-44f5-cc2aebd020f4@arm.com>
2019-02-20 20:04         ` Boris Ostrovsky
     [not found]         ` <15bc52cb-82d8-4d2c-e5a8-c6ae8de90276@oracle.com>
2019-02-20 20:46           ` Julien Grall
     [not found]           ` <5df8888a-4a29-fccd-bac2-a9c170244029@arm.com>
2019-02-20 21:46             ` Boris Ostrovsky
     [not found]             ` <1574a7fe-a5ac-4bc2-d3f0-967d8d01e5f1@oracle.com>
2019-02-20 22:03               ` Julien Grall
     [not found]               ` <1100e6b1-6fa8-06f0-8ecc-b0699a2ce5f4@arm.com>
2019-02-21  8:07                 ` Roger Pau Monné
     [not found]                 ` <20190221080752.zy2qlzb4vi7tbu3p@Air-de-Roger>
2019-02-21  8:38                   ` Julien Grall
2019-02-21  8:52                     ` Juergen Gross
2019-02-21  9:14                     ` Roger Pau Monné
     [not found]                     ` <20190221091431.vqi53op37mvhi25z@Air-de-Roger>
2019-02-21 20:46                       ` Julien Grall
2020-04-27 23:20                     ` Stefano Stabellini [this message]
2019-02-22 11:44                 ` Jan Beulich
2019-02-22 12:38             ` Oleksandr Andrushchenko
     [not found]             ` <f7e78bf6-6e65-6d6c-c1ad-dca7f1e66b17@gmail.com>
2019-02-22 13:33               ` Julien Grall
     [not found]               ` <fe44917f-bbe9-3e0e-fdda-2eb4db9f25c2@arm.com>
2019-02-25 13:24                 ` Oleksandr Andrushchenko
     [not found]                 ` <13a9616c-2d9a-f90b-3358-f2dcadbbb64d@gmail.com>
2019-02-25 13:55                   ` Julien Grall
     [not found]                   ` <dbfd87e9-48fc-f641-9e24-ddb6c4f61135@arm.com>
2019-02-25 14:08                     ` Oleksandr Andrushchenko
     [not found]                     ` <1aeda04d-3420-fa50-ad33-a0b3e981f5e4@gmail.com>
2019-02-25 15:26                       ` Julien Grall
2019-02-26  9:14                     ` Roger Pau Monné
     [not found]                     ` <20190226091420.klgldhotiecezw6h@Air-de-Roger>
2019-02-26  9:30                       ` Andrew Cooper
     [not found]                       ` <038b837c-63c0-afb7-ca7b-75f61af7518e@citrix.com>
2019-02-26  9:44                         ` Roger Pau Monné
2019-02-26  9:45                         ` Paul Durrant
     [not found]                         ` <20190226094459.33y2ygrjei3sf3gk@Air-de-Roger>
2019-02-26 10:03                           ` Julien Grall
     [not found]                           ` <21c331d5-0cfa-6f7e-3db4-40b7ece45bc8@arm.com>
2019-02-26 10:17                             ` Roger Pau Monné
     [not found]                             ` <20190226101721.kh5vbrqdlnrtvhwh@Air-de-Roger>
2019-02-26 10:26                               ` Julien Grall
     [not found]                               ` <cdd5781c-3a60-b9f7-205f-fadeee88206e@arm.com>
2019-02-26 11:02                                 ` Roger Pau Monné
     [not found]                                 ` <20190226110231.46luhevhlmefdldo@Air-de-Roger>
2019-02-27 11:09                                   ` Julien Grall
2019-02-21  8:17 ` Juergen Gross

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.DEB.2.21.2004271552430.29217@sstabellini-ThinkPad-T480s \
    --to=sstabellini@kernel.org \
    --cc=Andrew.Cooper3@citrix.com \
    --cc=JBeulich@suse.com \
    --cc=boris.ostrovsky@oracle.com \
    --cc=dave.martin@arm.com \
    --cc=jgross@suse.com \
    --cc=julien.grall@arm.com \
    --cc=julien.grall@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=roger.pau@citrix.com \
    --cc=xen-devel@lists.xenproject.org \
    --subject='Re: [Xen-devel] xen/evtchn and forced threaded irq' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).