lkml.org 
[lkml]   [2021]   [Feb]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patches in this message
/
SubjectRe: xen/evtchn: Interrupt for port 34, but apparently not enabled; per-user 00000000a86a4c1b on 5.10
From
Date
On 14.12.20 22:25, Julien Grall wrote:
> Hi Juergen,
>
> When testing Linux 5.10 dom0, I could reliably hit the following warning
> with using event 2L ABI:
>
> [  589.591737] Interrupt for port 34, but apparently not enabled;
> per-user 00000000a86a4c1b
> [  589.593259] WARNING: CPU: 0 PID: 1111 at
> /home/ANT.AMAZON.COM/jgrall/works/oss/linux/drivers/xen/evtchn.c:170
> evtchn_interrupt+0xeb/0x100
> [  589.595514] Modules linked in:
> [  589.596145] CPU: 0 PID: 1111 Comm: qemu-system-i38 Tainted: G
> W         5.10.0+ #180
> [  589.597708] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
> rel-1.12.0-59-gc9ba5276e321-prebuilt.qemu.org 04/01/2014
> [  589.599782] RIP: e030:evtchn_interrupt+0xeb/0x100
> [  589.600698] Code: 48 8d bb d8 01 00 00 ba 01 00 00 00 be 1d 00 00 00
> e8 d9 10 ca ff eb b2 8b 75 20 48 89 da 48 c7 c7 a8 31 3d 82 e8 65 29 a0
> ff <0f> 0b e9 42 ff ff ff 0f 1f 40 00 66 2e 0f 1f 84 00 00 00 00 00 0f
> [  589.604087] RSP: e02b:ffffc90040003e70 EFLAGS: 00010086
> [  589.605102] RAX: 0000000000000000 RBX: ffff888102091800 RCX:
> 0000000000000027
> [  589.606445] RDX: 0000000000000000 RSI: ffff88817fe19150 RDI:
> ffff88817fe19158
> [  589.607790] RBP: ffff88810f5ab980 R08: 0000000000000001 R09:
> 0000000000328980
> [  589.609134] R10: 0000000000000000 R11: ffffc90040003c70 R12:
> ffff888107fd3c00
> [  589.610484] R13: ffffc90040003ed4 R14: 0000000000000000 R15:
> ffff88810f5ffd80
> [  589.611828] FS:  00007f960c4b8ac0(0000) GS:ffff88817fe00000(0000)
> knlGS:0000000000000000
> [  589.613348] CS:  10000e030 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  589.614525] CR2: 00007f17ee72e000 CR3: 000000010f5b6000 CR4:
> 0000000000050660
> [  589.615874] Call Trace:
> [  589.616402]  <IRQ>
> [  589.616855]  __handle_irq_event_percpu+0x4e/0x2c0
> [  589.617784]  handle_irq_event_percpu+0x30/0x80
> [  589.618660]  handle_irq_event+0x3a/0x60
> [  589.619428]  handle_edge_irq+0x9b/0x1f0
> [  589.620209]  generic_handle_irq+0x4f/0x60
> [  589.621008]  evtchn_2l_handle_events+0x160/0x280
> [  589.621913]  __xen_evtchn_do_upcall+0x66/0xb0
> [  589.622767]  __xen_pv_evtchn_do_upcall+0x11/0x20
> [  589.623665]  asm_call_irq_on_stack+0x12/0x20
> [  589.624511]  </IRQ>
> [  589.624978]  xen_pv_evtchn_do_upcall+0x77/0xf0
> [  589.625848]  exc_xen_hypervisor_callback+0x8/0x10
>
> This can be reproduced when creating/destroying guest in a loop.
> Although, I have struggled to reproduce it on a vanilla Xen.
>
> After several hours of debugging, I think I have found the root cause.
>
> While we only expect the unmask to happen when the event channel is
> EOIed, there is an unmask happening as part of handle_edge_irq() because
> the interrupt was seen as pending by another vCPU (IRQS_PENDING is set).
>
> It turns out that the event channel is set for multiple vCPU is in
> cpu_evtchn_mask. This is happening because the affinity is not cleared
> when freeing an event channel.
>
> The implementation of evtchn_2l_handle_events() will look for all the
> active interrupts for the current vCPU and later on clear the pending
> bit (via the ack() callback). IOW, I believe, this is not an atomic
> operation.
>
> Even if Xen will notify the event to a single vCPU, evtchn_pending_sel
> may still be set on the other vCPU (thanks to a different event
> channel). Therefore, there is a chance that two vCPUs will try to handle
> the same interrupt.
>
> The IRQ handler handle_edge_irq() is able to deal with that and will
> mask/unmask the interrupt. This will mess us with the lateeoi logic
> (although, I managed to reproduce it once without XSA-332).
>
> My initial idea to fix the problem was to switch the affinity from CPU X
> to CPU0 when the event channel is freed.
>
> However, I am not sure this is enough because I haven't found anything
> yet preventing a race between evtchn_2l_handle_events9) and
> evtchn_2l_bind_vcpu().
>
> So maybe we want to introduce a refcounting (if there is nothing
> provided by the IRQ framework) and only unmask when the counter drop to 0.
>
> Any opinions?

With the two attached patches testing on my side survived more than 2
hours of constant guest reboots and destroy/create loops. Without the
patches the WARN()s came up after less than one minute.

Can you please give it a try?


Juergen

From 908940c92fb916146a7ce24bc41a18125967c54a Mon Sep 17 00:00:00 2001
From: Juergen Gross <jgross@suse.com>
Date: Wed, 3 Feb 2021 16:24:42 +0100
Subject: [PATCH 1/2] xen/events: reset affinity of 2-level event initially

When creating a new event channel with 2-level events the affinity
needs to be reset initially in order to avoid using an old affinity
from earlier usage of the event channel port.

The same applies to the affinity when onlining a vcpu: all old
affinity settings for this vcpu must be reset. As percpu events get
initialized before the percpu event channel hook is called,
resetting of the affinities happens after offlining a vcpu (this is
working, as initial percpu memory is zeroed out).

Cc: stable@vger.kernel.org
Reported-by: Julien Grall <julien@xen.org>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
drivers/xen/events/events_2l.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)

diff --git a/drivers/xen/events/events_2l.c b/drivers/xen/events/events_2l.c
index da87f3a1e351..23217940144a 100644
--- a/drivers/xen/events/events_2l.c
+++ b/drivers/xen/events/events_2l.c
@@ -47,6 +47,16 @@ static unsigned evtchn_2l_max_channels(void)
return EVTCHN_2L_NR_CHANNELS;
}

+static int evtchn_2l_setup(evtchn_port_t evtchn)
+{
+ unsigned int cpu;
+
+ for_each_online_cpu(cpu)
+ clear_bit(evtchn, BM(per_cpu(cpu_evtchn_mask, cpu)));
+
+ return 0;
+}
+
static void evtchn_2l_bind_to_cpu(evtchn_port_t evtchn, unsigned int cpu,
unsigned int old_cpu)
{
@@ -355,9 +365,18 @@ static void evtchn_2l_resume(void)
EVTCHN_2L_NR_CHANNELS/BITS_PER_EVTCHN_WORD);
}

+static int evtchn_2l_percpu_deinit(unsigned int cpu)
+{
+ memset(per_cpu(cpu_evtchn_mask, cpu), 0, sizeof(xen_ulong_t) *
+ EVTCHN_2L_NR_CHANNELS/BITS_PER_EVTCHN_WORD);
+
+ return 0;
+}
+
static const struct evtchn_ops evtchn_ops_2l = {
.max_channels = evtchn_2l_max_channels,
.nr_channels = evtchn_2l_max_channels,
+ .setup = evtchn_2l_setup,
.bind_to_cpu = evtchn_2l_bind_to_cpu,
.clear_pending = evtchn_2l_clear_pending,
.set_pending = evtchn_2l_set_pending,
@@ -367,6 +386,7 @@ static const struct evtchn_ops evtchn_ops_2l = {
.unmask = evtchn_2l_unmask,
.handle_events = evtchn_2l_handle_events,
.resume = evtchn_2l_resume,
+ .percpu_deinit = evtchn_2l_percpu_deinit,
};

void __init xen_evtchn_2l_init(void)
--
2.26.2
From 1f59b7827f734f2d86cff149ea8d297944e136d1 Mon Sep 17 00:00:00 2001
From: Juergen Gross <jgross@suse.com>
Date: Tue, 15 Dec 2020 10:37:11 +0100
Subject: [PATCH 2/2] xen/events: don't unmask an event channel when an eoi is
pending

An event channel should be kept masked when an eoi is pending for it.
When being migrated to another cpu it might be unmasked, though.

In order to avoid this keep two different flags for each event channel
to be able to distinguish "normal" masking/unmasking from eoi related
masking/unmasking. The event channel should only be able to generate
an interrupt if both flags are cleared.

Cc: stable@vger.kernel.org
Fixes: 54c9de89895e0a36047 ("xen/events: add a new late EOI evtchn framework")
Reported-by: Julien Grall <julien@xen.org>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
drivers/xen/events/events_base.c | 63 +++++++++++++++++++++++++++-----
1 file changed, 53 insertions(+), 10 deletions(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index e850f79351cb..6a836d131e73 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -97,7 +97,9 @@ struct irq_info {
short refcnt;
u8 spurious_cnt;
u8 is_accounted;
- enum xen_irq_type type; /* type */
+ short type; /* type: IRQT_* */
+ bool masked; /* Is event explicitly masked? */
+ bool eoi_pending; /* Is EOI pending? */
unsigned irq;
evtchn_port_t evtchn; /* event channel */
unsigned short cpu; /* cpu bound */
@@ -302,6 +304,8 @@ static int xen_irq_info_common_setup(struct irq_info *info,
info->irq = irq;
info->evtchn = evtchn;
info->cpu = cpu;
+ info->masked = true;
+ info->eoi_pending = false;

ret = set_evtchn_to_irq(evtchn, irq);
if (ret < 0)
@@ -585,7 +589,10 @@ static void xen_irq_lateeoi_locked(struct irq_info *info, bool spurious)
}

info->eoi_time = 0;
- unmask_evtchn(evtchn);
+ info->eoi_pending = false;
+
+ if (!info->masked)
+ unmask_evtchn(evtchn);
}

static void xen_irq_lateeoi_worker(struct work_struct *work)
@@ -830,7 +837,11 @@ static unsigned int __startup_pirq(unsigned int irq)
goto err;

out:
- unmask_evtchn(evtchn);
+ info->masked = false;
+
+ if (!info->eoi_pending)
+ unmask_evtchn(evtchn);
+
eoi_pirq(irq_get_irq_data(irq));

return 0;
@@ -857,6 +868,7 @@ static void shutdown_pirq(struct irq_data *data)
if (!VALID_EVTCHN(evtchn))
return;

+ info->masked = true;
mask_evtchn(evtchn);
xen_evtchn_close(evtchn);
xen_irq_info_cleanup(info);
@@ -1768,18 +1780,26 @@ static int set_affinity_irq(struct irq_data *data, const struct cpumask *dest,

static void enable_dynirq(struct irq_data *data)
{
- evtchn_port_t evtchn = evtchn_from_irq(data->irq);
+ struct irq_info *info = info_for_irq(data->irq);
+ evtchn_port_t evtchn = info ? info->evtchn : 0;

- if (VALID_EVTCHN(evtchn))
- unmask_evtchn(evtchn);
+ if (VALID_EVTCHN(evtchn)) {
+ info->masked = false;
+
+ if (!info->eoi_pending)
+ unmask_evtchn(evtchn);
+ }
}

static void disable_dynirq(struct irq_data *data)
{
- evtchn_port_t evtchn = evtchn_from_irq(data->irq);
+ struct irq_info *info = info_for_irq(data->irq);
+ evtchn_port_t evtchn = info ? info->evtchn : 0;

- if (VALID_EVTCHN(evtchn))
+ if (VALID_EVTCHN(evtchn)) {
+ info->masked = true;
mask_evtchn(evtchn);
+ }
}

static void ack_dynirq(struct irq_data *data)
@@ -1798,6 +1818,29 @@ static void mask_ack_dynirq(struct irq_data *data)
ack_dynirq(data);
}

+static void lateeoi_ack_dynirq(struct irq_data *data)
+{
+ struct irq_info *info = info_for_irq(data->irq);
+ evtchn_port_t evtchn = info ? info->evtchn : 0;
+
+ if (VALID_EVTCHN(evtchn)) {
+ info->eoi_pending = true;
+ mask_evtchn(evtchn);
+ }
+}
+
+static void lateeoi_mask_ack_dynirq(struct irq_data *data)
+{
+ struct irq_info *info = info_for_irq(data->irq);
+ evtchn_port_t evtchn = info ? info->evtchn : 0;
+
+ if (VALID_EVTCHN(evtchn)) {
+ info->masked = true;
+ info->eoi_pending = true;
+ mask_evtchn(evtchn);
+ }
+}
+
static int retrigger_dynirq(struct irq_data *data)
{
evtchn_port_t evtchn = evtchn_from_irq(data->irq);
@@ -2023,8 +2066,8 @@ static struct irq_chip xen_lateeoi_chip __read_mostly = {
.irq_mask = disable_dynirq,
.irq_unmask = enable_dynirq,

- .irq_ack = mask_ack_dynirq,
- .irq_mask_ack = mask_ack_dynirq,
+ .irq_ack = lateeoi_ack_dynirq,
+ .irq_mask_ack = lateeoi_mask_ack_dynirq,

.irq_set_affinity = set_affinity_irq,
.irq_retrigger = retrigger_dynirq,
--
2.26.2
[unhandled content-type:application/pgp-keys][unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2021-02-04 13:45    [W:0.123 / U:4.400 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site