Re: [patch V3 16/22] genirq/msi: Provide new domain id based interfaces for freeing interrupts

[David Woodhouse <dwmw2@infradead.org>]

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

On Fri, 2022-11-25 at 00:24 +0100, Thomas Gleixner wrote:
> Provide two sorts of interfaces to handle the different use cases:
> 
>   - msi_domain_free_irqs_range():
> 
>         Handles a caller defined precise range
> 
>   - msi_domain_free_irqs_all():
> 
>         Frees all interrupts associated to a domain
> 
> The latter is useful for device teardown and to handle the legacy MSI support
> which does not have any range information available.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---

...

> +static void msi_domain_free_locked(struct device *dev, struct msi_ctrl *ctrl)
>  {
> +       struct msi_domain_info *info;
> +       struct msi_domain_ops *ops;
> +       struct irq_domain *domain;
> +
> +       if (!msi_ctrl_valid(dev, ctrl))
> +               return;
> +
> +       domain = msi_get_device_domain(dev, ctrl->domid);
> +       if (!domain)
> +               return;
> +
> +       info = domain->host_data;
> +       ops = info->ops;
> +
> +       if (ops->domain_free_irqs)
> +               ops->domain_free_irqs(domain, dev);

Do you want a call to msi_free_dev_descs(dev) here? In the case where
the core code calls ops->domain_alloc_irqs() it *has* allocated the
descriptors first... but it's expecting the irqdomain to free them?

However, it's not quite as simple as adding msi_free_dev_descs()...

> +       else
> +               __msi_domain_free_irqs(dev, domain, ctrl);
> +

The igb driver seems to set up a single MSI-X in its setup, then tear
that down, then try again with more. Thus exercising the teardown path.

In 6.2-rc3 it fails under Xen (emulation in qemu) thus:

[    1.491207] igb: Intel(R) Gigabit Ethernet Network Driver
[    1.494003] igb: Copyright (c) 2007-2014 Intel Corporation.
[    1.664907] ACPI: \_SB_.LNKA: Enabled at IRQ 10
[    1.670837] ------------[ cut here ]------------
[    1.672644] WARNING: CPU: 1 PID: 1 at drivers/xen/events/events_base.c:793 xen_free_irq+0x156/0x170
[    1.676202] Modules linked in:
[    1.677638] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 6.2.0-rc3+ #1059
[    1.680134] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.1-0-g3208b098f51a-prebuilt.qemu.org 04/01/2014
[    1.684484] RIP: 0010:xen_free_irq+0x156/0x170
[    1.686240] Code: 5c 41 5d 41 5e 41 5f e9 08 03 95 ff e8 a3 5b 95 ff 48 85 c0 74 14 48 8b 58 30 e9 df fe ff ff 31 f6 89 ef e8 6c 59 95 ff eb 94 <0f> 0b 5b 5d 41 5c 41 5d 41 5e 41 5f c3 cc cc cc cc 0f 0b eb 86 0f
[    1.692888] RSP: 0000:ffffc90000013ac8 EFLAGS: 00010246
[    1.694705] RAX: 0000000000000000 RBX: 0000000000000026 RCX: 0000000000000000
[    1.697113] RDX: 0000000000000028 RSI: ffff888001400490 RDI: 0000000000000026
[    1.699498] RBP: 0000000000000026 R08: ffff8880014005e8 R09: ffffffff89c00240
[    1.701917] R10: 0000000000000000 R11: 0000000000000000 R12: 00000000fffffffe
[    1.704520] R13: ffffffff89de6880 R14: 0000000000000000 R15: 0000000000000005
[    1.707202] FS:  0000000000000000(0000) GS:ffff88803ed00000(0000) knlGS:0000000000000000
[    1.709974] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    1.711867] CR2: 0000000000000000 CR3: 000000003c812001 CR4: 0000000000170ee0
[    1.714260] Call Trace:
[    1.715145]  <TASK>
[    1.715897]  xen_destroy_irq+0x64/0x120
[    1.717181]  ? msi_find_desc+0x41/0xb0
[    1.718552]  xen_teardown_msi_irqs+0x3d/0x70
[    1.720064]  msi_domain_free_locked.part.0+0x58/0x1c0
[    1.721791]  msi_domain_free_irqs_all_locked+0x6a/0x90
[    1.723551]  __pci_enable_msix_range+0x353/0x590
[    1.725159]  igb_set_interrupt_capability+0x90/0x1c0
[    1.726879]  igb_init_interrupt_scheme+0x2d/0x230
[    1.728494]  ? rcu_read_lock_sched_held+0x3f/0x80
[    1.730361]  igb_sw_init+0x1b3/0x260
[    1.731797]  igb_probe+0x3b6/0xf00
[    1.733146]  ? _raw_spin_unlock_irqrestore+0x40/0x60
[    1.734834]  local_pci_probe+0x41/0x80
[    1.736164]  pci_call_probe+0x54/0x160
[    1.737441]  pci_device_probe+0x7c/0x100
[    1.738828]  ? driver_sysfs_add+0x71/0xd0
[    1.740229]  really_probe+0xde/0x380
[    1.741434]  ? pm_runtime_barrier+0x50/0x90
[    1.742873]  __driver_probe_device+0x78/0x170
[    1.744314]  driver_probe_device+0x1f/0x90
[    1.745689]  __driver_attach+0xd2/0x1c0
[    1.747035]  ? __pfx___driver_attach+0x10/0x10
[    1.748518]  bus_for_each_dev+0x79/0xc0
[    1.749859]  bus_add_driver+0x1b1/0x200
[    1.751182]  driver_register+0x89/0xe0
[    1.752472]  ? __pfx_igb_init_module+0x10/0x10
[    1.754054]  do_one_initcall+0x5b/0x320
[    1.755573]  ? rcu_read_lock_sched_held+0x3f/0x80
[    1.757375]  kernel_init_freeable+0x1a6/0x1ec
[    1.759005]  ? __pfx_kernel_init+0x10/0x10
[    1.760375]  kernel_init+0x16/0x130
[    1.761554]  ret_from_fork+0x2c/0x50
[    1.762797]  </TASK>
[    1.763590] irq event stamp: 1798623
[    1.764869] hardirqs last  enabled at (1798633): [<ffffffff8814aa8e>] __up_console_sem+0x5e/0x70
[    1.767762] hardirqs last disabled at (1798642): [<ffffffff8814aa73>] __up_console_sem+0x43/0x70
[    1.770715] softirqs last  enabled at (1798570): [<ffffffff88d91f76>] __do_softirq+0x356/0x4da
[    1.773576] softirqs last disabled at (1798565): [<ffffffff880bb83d>] __irq_exit_rcu+0xdd/0x150
[    1.776492] ---[ end trace 0000000000000000 ]---
[    1.839462] igb 0000:00:04.0: added PHC on eth0
[    1.843531] igb 0000:00:04.0: Intel(R) Gigabit Ethernet Network Connection
[    1.843541] igb 0000:00:04.0: eth0: (PCIe:5.0Gb/s:Width x4) 00:1e:67:cb:7a:93
[    1.843620] igb 0000:00:04.0: eth0: PBA No: 006000-000
[    1.849237] igb 0000:00:04.0: Using legacy interrupts. 1 rx queue(s), 1 tx queue(s)


If I add the missing call to msi_free_msi_descs() then it does work,
but complains differently:

[    1.563055] igb: Intel(R) Gigabit Ethernet Network Driver
[    1.566442] igb: Copyright (c) 2007-2014 Intel Corporation.
[    1.737236] ACPI: \_SB_.LNKA: Enabled at IRQ 10
[    1.742162] ------------[ cut here ]------------
[    1.744393] WARNING: CPU: 0 PID: 1 at kernel/irq/msi.c:196 msi_domain_free_descs+0xe1/0x110
[    1.748248] Modules linked in:
[    1.749289] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.2.0-rc3+ #1057
[    1.751466] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.1-0-g3208b098f51a-prebuilt.qemu.org 04/01/2014
[    1.755187] RIP: 0010:msi_domain_free_descs+0xe1/0x110
[    1.756875] Code: 00 48 89 e6 4c 89 e7 e8 ed f4 ba 00 48 89 c3 48 85 c0 0f 84 71 ff ff ff 48 8b 34 24 4c 89 e7 e8 a5 01 bb 00 8b 03 85 c0 74 be <0f> 0b eb cb 48 8b 87 70 03 00 00 be ff ff ff ff 48 8d 78 78 e8 26
[    1.763060] RSP: 0000:ffffc90000013b78 EFLAGS: 00010202
[    1.764804] RAX: 0000000000000026 RBX: ffff888001ac5f00 RCX: 0000000000000000
[    1.767155] RDX: 0000000000000001 RSI: ffffffffa649808a RDI: 00000000ffffffff
[    1.769462] RBP: ffffc90000013ba8 R08: 0000000000000001 R09: 0000000000000000
[    1.771934] R10: 000000006ac46bb1 R11: 00000000aee0433d R12: ffff888001a238c8
[    1.774695] R13: ffffffffa6de6880 R14: ffff888001a51218 R15: ffff888001a50000
[    1.777081] FS:  0000000000000000(0000) GS:ffff88803ec00000(0000) knlGS:0000000000000000
[    1.779754] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    1.781681] CR2: ffff888010801000 CR3: 000000000e812001 CR4: 0000000000170ef0
[    1.784093] Call Trace:
[    1.784880]  <TASK>
[    1.785640]  msi_domain_free_msi_descs_range+0x34/0x60
[    1.787370]  msi_domain_free_locked.part.0+0x58/0x1c0
[    1.789034]  msi_domain_free_irqs_all_locked+0x6a/0x90
[    1.790815]  pci_free_msi_irqs+0xe/0x30
[    1.792157]  pci_disable_msix+0x48/0x60
[    1.793413]  igb_reset_interrupt_capability+0xa4/0xb0
[    1.795077]  igb_sw_init+0x13f/0x260
[    1.796281]  igb_probe+0x3b6/0xf00
[    1.797421]  ? _raw_spin_unlock_irqrestore+0x40/0x60
[    1.799050]  local_pci_probe+0x41/0x80
[    1.800282]  pci_call_probe+0x54/0x160
[    1.801543]  pci_device_probe+0x7c/0x100
[    1.803393]  ? driver_sysfs_add+0x71/0xd0
[    1.804761]  really_probe+0xde/0x380
[    1.806021]  ? pm_runtime_barrier+0x50/0x90
[    1.807395]  __driver_probe_device+0x78/0x170
[    1.808877]  driver_probe_device+0x1f/0x90
[    1.810257]  __driver_attach+0xd2/0x1c0
[    1.811529]  ? __pfx___driver_attach+0x10/0x10
[    1.813251]  bus_for_each_dev+0x79/0xc0
[    1.814534]  bus_add_driver+0x1b1/0x200
[    1.816058]  driver_register+0x89/0xe0
[    1.817291]  ? __pfx_igb_init_module+0x10/0x10
[    1.818821]  do_one_initcall+0x5b/0x320
[    1.820133]  ? rcu_read_lock_sched_held+0x3f/0x80
[    1.821697]  kernel_init_freeable+0x1a6/0x1ec
[    1.823150]  ? __pfx_kernel_init+0x10/0x10
[    1.824484]  kernel_init+0x16/0x130
[    1.825990]  ret_from_fork+0x2c/0x50
[    1.827757]  </TASK>
[    1.828865] irq event stamp: 1797845
[    1.830573] hardirqs last  enabled at (1797855): [<ffffffffa514aa8e>] __up_console_sem+0x5e/0x70
[    1.834809] hardirqs last disabled at (1797864): [<ffffffffa514aa73>] __up_console_sem+0x43/0x70
[    1.838915] softirqs last  enabled at (1797742): [<ffffffffa5d91f76>] __do_softirq+0x356/0x4da
[    1.842442] softirqs last disabled at (1797737): [<ffffffffa50bb83d>] __irq_exit_rcu+0xdd/0x150
[    1.846094] ---[ end trace 0000000000000000 ]---


If I zero msidesc->irq in the loop in xen_teardown_msi_irqs(), *then*
it both works and stops complaining. But I'm mostly just tampering
empirically now...

I can provide a qemu tree which will let you test this easily with just
`qemu-system-x86_64 -kernel ...` but you have to promise not to look at
the way I've fixed some qemu deadlocks just by commenting out the lock
on MSI delivery/translation :)

You'd also have to provide your own igb device as qemu doesn't emulate
those; I'm testing it in passthrough. Or hack the e1000e driver to do a
setup/teardown/setup... or perhaps just unload and reload its module?

I'll provide a SoB just in case it's actually the right way to fix it…

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>

diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index 790550479831..293e23b7229a 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -390,8 +390,10 @@ static void xen_teardown_msi_irqs(struct pci_dev *dev)
 	int i;
 
 	msi_for_each_desc(msidesc, &dev->dev, MSI_DESC_ASSOCIATED) {
-		for (i = 0; i < msidesc->nvec_used; i++)
+		for (i = 0; i < msidesc->nvec_used; i++) {
 			xen_destroy_irq(msidesc->irq + i);
+			msidesc->irq = 0;
+		}
 	}
 }
 

diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index 955267bbc2be..812e1ec1a633 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -1533,9 +1533,10 @@ static void msi_domain_free_locked(struct device *dev, struct msi_ctrl *ctrl)
 	info = domain->host_data;
 	ops = info->ops;
 
-	if (ops->domain_free_irqs)
+	if (ops->domain_free_irqs) {
 		ops->domain_free_irqs(domain, dev);
-	else
+		msi_free_msi_descs(dev);
+	} else
 		__msi_domain_free_irqs(dev, domain, ctrl);
 
 	if (ops->msi_post_free)



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]