x86/iommu: Bootup stage cleanups

Boris reported that the conversion to stacked irqdomains causes a GFP_KERNEL allocation from atomic context splat.

The callchain is:

kernel_init_freeable
smp_prepare_cpus
default_setup_apic_routing
enable_IR_x2apic
irq_remapping_enable
intel_enable_irq_remapping
intel_setup_irq_remapping
irq_domain_add_hierarchy
__irq_domain_add

The reason for the splat is that smp_prepare_cpus() is pointlessly disabling preemption and enable_IR_x2apic is calling irq_remapping_enable with interrupts disabled.

Removing the completely useless preemption disable from smp_prepare_cpus() is a no brainer, but resolving the interrupt disabled issue in enable_IR_x2apic/irq_remapping_enable forced me to look deeper into the iommu setup code.

Convoluted is probably an euphemism for that maze.

The early detection of iommus happens in mem_init() which looks for the availability of ACPI tables describing iommus.

The actual parsing of the tables happens for AMD in setup_irq_remapping_ops() which calls remap_ops.prepare() and for Intel in irq_remapping_enable().

The AMD parsing allocates data structures, memory pages and the irq domain from the prepare callback, which makes a lot of sense as this has nothing to do with the actual enablement. That AMD code uses GFP_KERNEL allocations inside of a preempt disabled region. That issue is solved by removing the silly preempt_disable() from smp_prepare_cpus().

For Intel the actual parsing and allocation is moved from the enable to the prepare callback. That cures the reported splat and allows to use GFP_KERNEL allocations for the iommu internal stuff as well. While in the early bootup it does not matter much, for physical hotplug GFP_ATOMIC allocations are not desired at all.

That solves the issue at hand, but the iommu setup stays a tangled maze with completely non obvious setup mechanisms depending on the options (iommu on/off - irq remapping on/off).

Thanks,

tglx

---
 arch/x86/include/asm/irq_remapping.h |    2 -
 arch/x86/kernel/apic/apic.c          |    3 -
 arch/x86/kernel/smpboot.c            |    8 ----
 drivers/iommu/intel_irq_remapping.c  |   68 +++++++++++++++++++++++------------
 drivers/iommu/irq_remapping.c        |   19 +++------
 kernel/irq/irqdomain.c               |   21 +---------
 6 files changed, 56 insertions(+), 65 deletions(-)

There is no reason to keep preemption disabled in this function.

We only have two other threads live: kthreadd and idle. Neither of them is going to preempt. But that preempt_disable forces all the code inside to do GFP_ATOMIC allocations which is just insane.

Remove it.

---
 arch/x86/kernel/smpboot.c |    8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

Index: tip/arch/x86/kernel/smpboot.c
===================================================================
@@ -1084,7 +1084,6 @@ void __init native_smp_prepare_cpus(unsi
 {
 	unsigned int i;
 
-	preempt_disable();
 	smp_cpu_index_default();
 
 	/*

@@ -1102,22 +1101,19 @@ void __init native_smp_prepare_cpus(unsi
 	}
 	set_cpu_sibling_map(0);
 
-
 	if (smp_sanity_check(max_cpus) < 0) {
 		pr_info("SMP disabled\n");
 		disable_smp();
-		goto out;
+		return;
 	}
 
 	default_setup_apic_routing();
 
-	preempt_disable();
 	if (read_apic_id() != boot_cpu_physical_apicid) {
 		panic("Boot APIC ID in local APIC unexpected (%d vs %d)",
 		     read_apic_id(), boot_cpu_physical_apicid);
 		/* Or can we switch back to PIC here? */
 	}
-	preempt_enable();
 
 	connect_bsp_APIC();
 

@@ -1151,8 +1147,6 @@ void __init native_smp_prepare_cpus(unsi
 		uv_system_init();
 
 	set_mtrr_aps_delayed_init();
-out:
-	preempt_enable();
 }
 
 void arch_enable_nonboot_cpus_begin(void)

enable_IR_x2apic() calls setup_irq_remapping_ops() which by default installs the intel dmar remapping ops and then calls the amd iommu irq remapping prepare callback to figure out whether we are running on an AMD machine with irq remapping hardware.

Right after that it calls irq_remapping_prepare() which pointlessly checks:

	if (!remap_ops || !remap_ops->prepare)
               return -ENODEV;

and then calls

    remap_ops->prepare()

which is silly in the AMD case as it got called from setup_irq_remapping_ops() already a few microseconds ago.

Simplify this and just collapse everything into
irq_remapping_prepare().

The irq_remapping_prepare() remains still silly as it assigns blindly the intel ops, but that's not scope of this patch.

The scope here is to move the preperatory work, i.e. memory allocations out of the atomic section which is required to enable irq remapping.

---
 arch/x86/include/asm/irq_remapping.h |    2 --
 arch/x86/kernel/apic/apic.c          |    3 ---
 drivers/iommu/irq_remapping.c        |   19 +++++++------------
 3 files changed, 7 insertions(+), 17 deletions(-)

Index: tip/arch/x86/include/asm/irq_remapping.h
===================================================================
@@ -31,7 +31,6 @@ struct irq_alloc_info;
 
 #ifdef CONFIG_IRQ_REMAP
 
-extern void setup_irq_remapping_ops(void);
 extern int irq_remapping_supported(void);
 extern void set_irq_remapping_broken(void);
 extern int irq_remapping_prepare(void);

@@ -58,7 +57,6 @@ static inline struct irq_domain *arch_ge
 
 #else  /* CONFIG_IRQ_REMAP */
 
-static inline void setup_irq_remapping_ops(void) { }
 static inline int irq_remapping_supported(void) { return 0; }
 static inline void set_irq_remapping_broken(void) { }
 static inline int irq_remapping_prepare(void) { return -ENODEV; }

Index: tip/arch/x86/kernel/apic/apic.c
===================================================================
@@ -1597,9 +1597,6 @@ void __init enable_IR_x2apic(void)
 	int ret, x2apic_enabled = 0;
 	int hardware_init_ret;
 
-	/* Make sure irq_remap_ops are initialized */
-	setup_irq_remapping_ops();
-
 	hardware_init_ret = irq_remapping_prepare();
 	if (hardware_init_ret && !x2apic_supported())
 		return;

Index: tip/drivers/iommu/irq_remapping.c
===================================================================
@@ -75,16 +75,6 @@ static __init int setup_irqremap(char *s
 }
 early_param("intremap", setup_irqremap);
 
-void __init setup_irq_remapping_ops(void)
-{
-	remap_ops = &intel_irq_remap_ops;
-
-#ifdef CONFIG_AMD_IOMMU
-	if (amd_iommu_irq_ops.prepare() == 0)
-		remap_ops = &amd_iommu_irq_ops;
-#endif
-}
-
 void set_irq_remapping_broken(void)
 {
 	irq_remap_broken = 1;

@@ -103,9 +93,14 @@ int irq_remapping_supported(void)
 
 int __init irq_remapping_prepare(void)
 {
-	if (!remap_ops || !remap_ops->prepare)
-		return -ENODEV;
+	remap_ops = &intel_irq_remap_ops;
 
+#ifdef CONFIG_AMD_IOMMU
+	if (amd_iommu_irq_ops.prepare() == 0) {
+		remap_ops = &amd_iommu_irq_ops;
+		return 0;
+	}
+#endif
 	return remap_ops->prepare();
 }
 

The whole iommu setup for irq remapping is a convoluted mess. The iommu detect function gets called from mem_init() and the prepare callback gets called from enable_IR_x2apic() for unknown reasons.

Of course AMD and Intel setup differs in nonsensical ways. Intels prepare callback is explicit while AMDs prepare callback is implicit in setup_irq_remapping_ops() just to be called in the prepare call again.

Because all of this gets called from enable_IR_x2apic() and the dmar prepare function merily parses the ACPI tables, but does not allocate memory we end up with memory allocation from irq disabled context later on.

AMDs iommu code at least allocates the required memory from the prepare function. That has issues as well, but thats not scope of this patch.

The goal of this change is to distangle the allocation from the actual enablement. There is no point to allocate memory from irq disabled regions with GFP_ATOMIC just because it does not matter at that point in the boot stage. It matters with physical hotplug later on.

There is another issue with the current setup. Due to the conversion to stacked irqdomains we end up with a call into the irqdomain allocation code from irq disabled context, but that code does GFP_KERNEL allocations rightfully as there is no reason to do preperatory allocations with GFP_ATOMIC.

That change caused the allocator code to complain about GFP_KERNEL allocations invoked in atomic context. Boris provided a temporary hackaround which changed the GFP flags if irq_domain_add() got called from atomic context. Not pretty and we really dont want to get this into a mainline release for obvious reasons.

Move the ACPI table parsing and the resulting memory allocations from the enable to the prepare function. That allows to get rid of the horrible hackaround in irq_domain_add() later.

---
 drivers/iommu/intel_irq_remapping.c |   64 ++++++++++++++++++++++++------------
 1 file changed, 44 insertions(+), 20 deletions(-)

Index: tip/drivers/iommu/intel_irq_remapping.c
===================================================================
@@ -539,22 +539,57 @@ static int __init intel_irq_remapping_su
 	return 1;
 }
 
-static int __init intel_enable_irq_remapping(void)
+static void __init intel_cleanup_irq_remapping(void)
+{
+	struct dmar_drhd_unit *drhd;
+	struct intel_iommu *iommu;
+
+	for_each_iommu(iommu, drhd) {
+		if (ecap_ir_support(iommu->ecap)) {
+			iommu_disable_irq_remapping(iommu);
+			intel_teardown_irq_remapping(iommu);
+		}
+	}
+
+	if (x2apic_supported())
+		pr_warn("Failed to enable irq remapping.  You are vulnerable to irq-injection attacks.\n");
+}
+
+static int __init intel_prepare_irq_remapping(void)
 {
 	struct dmar_drhd_unit *drhd;
 	struct intel_iommu *iommu;
-	bool x2apic_present;
-	int setup = 0;
-	int eim = 0;
 
-	x2apic_present = x2apic_supported();
+	if (dmar_table_init() < 0)
+		return -1;
 
 	if (parse_ioapics_under_ir() != 1) {
-		printk(KERN_INFO "Not enable interrupt remapping\n");
+		printk(KERN_INFO "Not enabling interrupt remapping\n");
 		goto error;
 	}
 
-	if (x2apic_present) {
+	for_each_iommu(iommu, drhd) {
+		if (!ecap_ir_support(iommu->ecap))
+			continue;
+
+		/* Do the allocations early */
+		if (intel_setup_irq_remapping(iommu))
+			goto error;
+	}
+	return 0;
+error:
+	intel_cleanup_irq_remapping();
+	return -1;
+}
+
+static int __init intel_enable_irq_remapping(void)
+{
+	struct dmar_drhd_unit *drhd;
+	struct intel_iommu *iommu;
+	int setup = 0;
+	int eim = 0;
+
+	if (x2apic_supported()) {
 		pr_info("Queued invalidation will be enabled to support x2apic and Intr-remapping.\n");
 
 		eim = !dmar_x2apic_optout();

@@ -622,9 +657,6 @@ static int __init intel_enable_irq_remap
 		if (!ecap_ir_support(iommu->ecap))
 			continue;
 
-		if (intel_setup_irq_remapping(iommu))
-			goto error;
-
 		iommu_set_irq_remapping(iommu, eim);
 		setup = 1;
 	}

@@ -639,15 +671,7 @@ static int __init intel_enable_irq_remap
 	return eim ? IRQ_REMAP_X2APIC_MODE : IRQ_REMAP_XAPIC_MODE;
 
 error:
-	for_each_iommu(iommu, drhd)
-		if (ecap_ir_support(iommu->ecap)) {
-			iommu_disable_irq_remapping(iommu);
-			intel_teardown_irq_remapping(iommu);
-		}
-
-	if (x2apic_present)
-		pr_warn("Failed to enable irq remapping.  You are vulnerable to irq-injection attacks.\n");
-
+	intel_cleanup_irq_remapping();
 	return -1;
 }
 

@@ -947,7 +971,7 @@ static struct irq_domain *intel_get_irq_
 
 struct irq_remap_ops intel_irq_remap_ops = {
 	.supported		= intel_irq_remapping_supported,
-	.prepare		= dmar_table_init,
+	.prepare		= intel_prepare_irq_remapping,
 	.enable			= intel_enable_irq_remapping,
 	.disable		= disable_irq_remapping,
 	.reenable		= reenable_irq_remapping,

Got

[  134.510969] calling  ahci_pci_driver_init+0x0/0x1b @ 1
[  134.511387] ahci 0000:00:1f.2: version 3.0
[  134.530941]   alloc irq_desc for 91 on node 0
[  134.531168]   alloc irq_desc for 92 on node 0
[  134.550728]   alloc irq_desc for 93 on node 0
[  134.550995]   alloc irq_desc for 94 on node 0
[  134.551199]   alloc irq_desc for 95 on node 0
[  134.570871]   alloc irq_desc for 96 on node 0
[  134.571090]   alloc irq_desc for 97 on node 0
[  134.571303]   alloc irq_desc for 98 on node 0
[  134.590974]   alloc irq_desc for 99 on node 0
[  134.591205]   alloc irq_desc for 100 on node 0
[  134.610882]   alloc irq_desc for 101 on node 0
[  134.611136]   alloc irq_desc for 102 on node 0
[  134.611364]   alloc irq_desc for 103 on node 0
[  134.630992]   alloc irq_desc for 104 on node 0
[  134.631232]   alloc irq_desc for 105 on node 0
[  134.650885]   alloc irq_desc for 106 on node 0
[  134.651246] ahci 0000:00:1f.2: SSS flag set, parallel bus scan disabled
[  134.670926] ahci 0000:00:1f.2: AHCI 0001.0200 32 slots 6 ports 3
Gbps 0x3f impl SATA mode
[  134.671349] ahci 0000:00:1f.2: flags: 64bit ncq sntf stag pm led
clo pio slum part ccc ems sxs
[  134.691158] ahci 0000:00:1f.2: with iommu 3 : domain 10
[  134.751560] BUG: unable to handle kernel NULL pointer dereference
at 0000000000000118
[  134.751997] IP: [<ffffffff81eafe50>] modify_irte+0x40/0xd0
[  134.770893] PGD 0
[  134.771011] Oops: 0000 [#1] SMP
[  134.771195] Modules linked in:
[  134.771344] CPU: 0 PID: 2169 Comm: kworker/0:1 Tainted: G        W
[  134.811557] Workqueue: events work_for_cpu_fn
[  134.830823] task: ffff881024725240 ti: ffff8810252f8000 task.ti:
ffff8810252f8000
[  134.831176] RIP: 0010:[<ffffffff81eafe50>]  [<ffffffff81eafe50>]
modify_irte+0x40/0xd0
[  134.851029] RSP: 0000:ffff8810252fba18  EFLAGS: 00010096
[  134.851276] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000be00bd
[  134.871322] RDX: 0000000000000000 RSI: ffffffff81eafe3f RDI: 0000000000000046
[  134.891061] RBP: ffff8810252fba48 R08: 0000000000000001 R09: 0000000000000001
[  134.891393] R10: ffff881024725240 R11: 0000000000000292 R12: 0000000000000000
[  134.911249] R13: 0000000000000096 R14: ffff881022b181d0 R15: ffff880079268260
[  134.930824] FS:  0000000000000000(0000) GS:ffff88103de00000(0000)
knlGS:0000000000000000
[  134.931202] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  134.950966] CR2: 0000000000000118 CR3: 0000000002c1a000 CR4: 00000000000007f0
[  134.970775] Stack:
[  134.970883]  ffff8810252fba88 0000000000000046 ffff881022f6c660
ffff881026d00000
[  134.971253]  000000000000005c ffff880079268200 ffff8810252fba58
ffffffff81eaff26
[  134.991066]  ffff8810252fba78 ffffffff81106761 ffff880079268200
ffff88103d889400
[  135.010908] Call Trace:
[  135.011038]  [<ffffffff81eaff26>] intel_irq_remapping_activate+0x16/0x20
[  135.030800]  [<ffffffff81106761>] irq_domain_activate_irq+0x41/0x50
[  135.031103]  [<ffffffff8110674b>] irq_domain_activate_irq+0x2b/0x50
[  135.050857]  [<ffffffff81103f19>] irq_startup+0x29/0x70
[  135.051091]  [<ffffffff81102857>] __setup_irq+0x327/0x590
[  135.070849]  [<ffffffff81a3d820>] ? ahci_bad_pmp_check_ready+0x70/0x70
[  135.071143]  [<ffffffff81102c42>] request_threaded_irq+0xf2/0x150
[  135.090972]  [<ffffffff81a3d820>] ? ahci_bad_pmp_check_ready+0x70/0x70
[  135.091295]  [<ffffffff81a3ff90>] ? ahci_host_activate+0x180/0x180
[  135.111014]  [<ffffffff8110495f>] devm_request_threaded_irq+0x5f/0xb0
[  135.130804]  [<ffffffff81a3feb3>] ahci_host_activate+0xa3/0x180
[  135.131097]  [<ffffffff81a3d391>] ahci_init_one+0x9d1/0xac0
[  135.150841]  [<ffffffff8157d735>] local_pci_probe+0x45/0xa0
[  135.151127]  [<ffffffff810b8868>] work_for_cpu_fn+0x18/0x30
[  135.170843]  [<ffffffff810bbd24>] process_one_work+0x254/0x470
[  135.171103]  [<ffffffff810bbc89>] ? process_one_work+0x1b9/0x470
[  135.190846]  [<ffffffff810bce1b>] worker_thread+0x31b/0x4e0
[  135.191115]  [<ffffffff810ea3bd>] ? trace_hardirqs_on+0xd/0x10
[  135.210920]  [<ffffffff810bcb00>] ? pool_mayday_timeout+0x170/0x170
[  135.211215]  [<ffffffff810c1ff1>] kthread+0x101/0x110
[  135.230902]  [<ffffffff810ea3bd>] ? trace_hardirqs_on+0xd/0x10
[  135.231157]  [<ffffffff810c1ef0>] ? kthread_stop+0x100/0x100
[  135.250930]  [<ffffffff82015e6c>] ret_from_fork+0x7c/0xb0
[  135.251178]  [<ffffffff810c1ef0>] ? kthread_stop+0x100/0x100
[  135.270969] Code: ec 10 48 85 ff 0f 84 90 00 00 00 48 c7 c7 80 34
e0 82 49 89 f6 e8 21 54 16 00 0f b7 53 08 49 89 c5 0f b7 43 0a 4c 8b
23 8d 1c 02 <49> 8b 84 24 18 01 00 00 48 63 fb 48 c1 e7 04 48 03 38 49
8b 06
[  135.291699] RIP  [<ffffffff81eafe50>] modify_irte+0x40/0xd0
[  135.311051]  RSP <ffff8810252fba18>
[  135.311215] CR2: 0000000000000118
[  135.330856] ---[ end trace fee039719f1667df ]---
[  135.333024] BUG: unable to handle kernel paging request at ffffffffffffff98
[  135.350911] IP: [<ffffffff810c2530>] kthread_data+0x10/0x20
[  135.351230] PGD 2c1b067 PUD 2c1d067 PMD 0
[  135.351443] Oops: 0000 [#2] SMP
[  135.370998] Modules linked in:
[  135.371168] CPU: 0 PID: 2169 Comm: kworker/0:1 Tainted: G      D W
[  135.412423] task: ffff881024725240 ti: ffff8810252f8000 task.ti:
ffff8810252f8000
[  135.412798] RIP: 0010:[<ffffffff810c2530>]  [<ffffffff810c2530>]
kthread_data+0x10/0x20
[  135.431159] RSP: 0000:ffff8810252fb538  EFLAGS: 00010096
[  135.450891] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 000000000000000f
[  135.451218] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff881024725240
[  135.471044] RBP: ffff8810252fb538 R08: ffff8810247252d0 R09: 0000000000000001
[  135.490873] R10: ffff881024725240 R11: 000000000000001a R12: ffff88103dfd2c40
[  135.491237] R13: 0000000000000000 R14: 0000000000000000 R15: ffff881024725240
[  135.511046] FS:  0000000000000000(0000) GS:ffff88103de00000(0000)
knlGS:0000000000000000
[  135.530882] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  135.531160] CR2: 0000000000000028 CR3: 0000000002c1a000 CR4: 00000000000007f0
[  135.550979] Stack:
[  135.551074]  ffff8810252fb558 ffffffff810bd065 ffff8810252fb558
ffff881024725240
[  135.570927]  ffff8810252fb678 ffffffff8200fc0b ffff881025ffec00
0000000000009000
[  135.571302]  ffff881024725240 ffff8810252fbfd8 ffff88103dfd3a40
ffff881024725240
[  135.591114] Call Trace:
[  135.591231]  [<ffffffff810bd065>] wq_worker_sleeping+0x15/0xb0
[  135.610996]  [<ffffffff8200fc0b>] __schedule+0x18b/0xa70
[  135.611237]  [<ffffffff810ea3bd>] ? trace_hardirqs_on+0xd/0x10
[  135.630988]  [<ffffffff810a634a>] ? do_exit+0x88a/0x9f0
[  135.631222]  [<ffffffff810a634a>] ? do_exit+0x88a/0x9f0
[  135.650932]  [<ffffffff82010555>] schedule+0x65/0x70
[  135.651186]  [<ffffffff810a6415>] do_exit+0x955/0x9f0
[  135.670899]  [<ffffffff81054a08>] oops_end+0xb8/0xd0
[  135.671136]  [<ffffffff81ffa88a>] no_context+0x309/0x352
[  135.671373]  [<ffffffff81ffaa98>] __bad_area_nosemaphore+0x1c5/0x1e4
[  135.691185]  [<ffffffff81ffaaca>] bad_area_nosemaphore+0x13/0x15
[  135.710934]  [<ffffffff81093f26>] __do_page_fault+0x266/0x590
[  135.711292]  [<ffffffff810c8d20>] ? task_rq_lock+0x50/0xb0
[  135.730941]  [<ffffffff810c8d20>] ? task_rq_lock+0x50/0xb0
[  135.731200]  [<ffffffff820150f2>] ? _raw_spin_lock+0x62/0x70
[  135.750949]  [<ffffffff810c8d20>] ? task_rq_lock+0x50/0xb0
[  135.751195]  [<ffffffff810ea166>] ? trace_hardirqs_on_caller+0x16/0x260
[  135.770989]  [<ffffffff810e7c6f>] ? trace_hardirqs_off_caller+0x1f/0x160
[  135.771309]  [<ffffffff81094296>] do_page_fault+0x46/0x80
[  135.791081]  [<ffffffff82017c72>] page_fault+0x22/0x30
[  135.791310]  [<ffffffff81eafe3f>] ? modify_irte+0x2f/0xd0
[  135.811037]  [<ffffffff81eafe50>] ? modify_irte+0x40/0xd0
[  135.811315]  [<ffffffff81eafe3f>] ? modify_irte+0x2f/0xd0
[  135.831150]  [<ffffffff81eaff26>] intel_irq_remapping_activate+0x16/0x20
[  135.831461]  [<ffffffff81106761>] irq_domain_activate_irq+0x41/0x50
[  135.851716]  [<ffffffff8110674b>] irq_domain_activate_irq+0x2b/0x50
[  135.852020]  [<ffffffff81103f19>] irq_startup+0x29/0x70
[  135.871401]  [<ffffffff81102857>] __setup_irq+0x327/0x590
[  135.871653]  [<ffffffff81a3d820>] ? ahci_bad_pmp_check_ready+0x70/0x70
[  135.891334]  [<ffffffff81102c42>] request_threaded_irq+0xf2/0x150
[  135.911099]  [<ffffffff81a3d820>] ? ahci_bad_pmp_check_ready+0x70/0x70
[  135.911416]  [<ffffffff81a3ff90>] ? ahci_host_activate+0x180/0x180
[  135.931274]  [<ffffffff8110495f>] devm_request_threaded_irq+0x5f/0xb0
[  135.931568]  [<ffffffff81a3feb3>] ahci_host_activate+0xa3/0x180
[  135.951093]  [<ffffffff81a3d391>] ahci_init_one+0x9d1/0xac0
[  135.951375]  [<ffffffff8157d735>] local_pci_probe+0x45/0xa0
[  135.971111]  [<ffffffff810b8868>] work_for_cpu_fn+0x18/0x30
[  135.971366]  [<ffffffff810bbd24>] process_one_work+0x254/0x470
[  135.991196]  [<ffffffff810bbc89>] ? process_one_work+0x1b9/0x470
[  135.991477]  [<ffffffff810bce1b>] worker_thread+0x31b/0x4e0
[  136.011132]  [<ffffffff810ea3bd>] ? trace_hardirqs_on+0xd/0x10
[  136.011393]  [<ffffffff810bcb00>] ? pool_mayday_timeout+0x170/0x170
[  136.031187]  [<ffffffff810c1ff1>] kthread+0x101/0x110
[  136.031420]  [<ffffffff810ea3bd>] ? trace_hardirqs_on+0xd/0x10
[  136.051244]  [<ffffffff810c1ef0>] ? kthread_stop+0x100/0x100
[  136.051494]  [<ffffffff82015e6c>] ret_from_fork+0x7c/0xb0
[  136.071210]  [<ffffffff810c1ef0>] ? kthread_stop+0x100/0x100
[  136.071495] Code: 00 48 89 e5 5d 48 8b 40 88 48 c1 e8 02 83 e0 01
c3 66 2e 0f 1f 84 00 00 00 00 00 66 66 66 66 90 48 8b 87 b8 08 00 00
55 48 89 e5 <48> 8b 40 98 5d c3 66 2e 0f 1f 84 00 00 00 00 00 66 66 66
66 90
[  136.111619] RIP  [<ffffffff810c2530>] kthread_data+0x10/0x20
[  136.131069]  RSP <ffff8810252fb538>
[  136.131253] CR2: ffffffffffffff98
[  136.131406] ---[ end trace fee039719f1667e0 ]---
[  136.151131] Fixing recursive fault but reboot is needed!

It is in tip/apic

Thanks

Yinghai

Hi Yinghai,
From following log messages, it seems that the AHCI controllers
allocates 16 MSI/MSI-X interrupt, and triggers NULL pointer reference when enabling interrupts for AHCI.
It doesn't trigger panic with this code path (allocate/enable
MSI/MSI-X interrupts with IR enabled) on my test system. So could you please help to get more info with the attached test patch? Thanks!
Gerry

From: Jiang Liu <[email protected]>
Date: Thu, 11 Dec 2014 22:25:40 +0800
Subject: [PATCH]

---
 drivers/iommu/intel_irq_remapping.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

index ff35b0336d2b..8503cd467484 100644
@@ -1207,6 +1207,19 @@ static void intel_irq_remapping_activate(struct irq_domain *domain,
 {
 	struct intel_ir_data *data = irq_data->chip_data;
 
+	if (irq_data->irq >= 91 && irq_data->irq <= 106) {
+		struct irq_2_iommu *irq_iommu;
+		struct intel_iommu *iommu;
+
+		irq_iommu = &data->irq_2_iommu;
+		iommu = irq_iommu->iommu;
+		pr_warn("iommu: chip_data %p, iommu %p, index %d, subindex %d, ir_table %p, table_base %p, queue %p\n",
+			data, iommu, irq_iommu->irte_index, irq_iommu->sub_handle,
+			iommu ? iommu->ir_table : NULL,
+			iommu && iommu->ir_table ? iommu->ir_table->base : NULL,
+			iommu && iommu->qi ? iommu->qi->desc : NULL);
+	}
+
 	modify_irte(&data->irq_2_iommu, &data->irte_entry);
 }
 

1.7.10.4

[  113.486917] calling  ahci_pci_driver_init+0x0/0x1b @ 1
[  113.487299] ahci 0000:00:1f.2: version 3.0
[  113.507317] ahci 0000:00:1f.2: SSS flag set, parallel bus scan disabled
[  113.507713] ahci 0000:00:1f.2: AHCI 0001.0200 32 slots 6 ports 3
Gbps 0x3f impl SATA mode
[  113.527019] ahci 0000:00:1f.2: flags: 64bit ncq sntf stag pm led
clo pio slum part ccc ems sxs
[  113.583977] iommu: chip_data ffff881022b97740, iommu
ffff88103d80ae00, index 32, subindex 0, ir_table ffff88103d802af0,
table_base ffff881026c00000, queue ffff88102770c000
[  113.597261] iommu: chip_data ffff881022b97780, iommu
(null), index 0, subindex 0, ir_table           (null), table_base
      (null), queue           (null)
[  113.617124] BUG: unable to handle kernel NULL pointer dereference
at 0000000000000118
[  113.636983] IP: [<ffffffff81eacca0>] modify_irte+0x40/0xd0
[  113.637253] PGD 0
[  113.656978] Oops: 0000 [#1] SMP
[  113.657133] Modules linked in:
[  113.657322] CPU: 0 PID: 2531 Comm: kworker/0:1 Tainted: G        W
[  113.696953] Workqueue: events work_for_cpu_fn
[  113.697189] task: ffff881025535240 ti: ffff881025528000 task.ti:
ffff881025528000
[  113.716965] RIP: 0010:[<ffffffff81eacca0>]  [<ffffffff81eacca0>]

modify_irte+0x40/0xd0

So irq_2_iommu is empty. That's a multi MSI, and that's the second interrupt which gets enabled.

The patch below should fix it.

Thanks,

tglx

index ff35b0336d2b..46da573a4746 100644
@@ -1131,7 +1131,7 @@ static int intel_irq_remapping_alloc(struct irq_domain *domain,
 {
 	struct intel_iommu *iommu = domain->host_data;
 	struct irq_alloc_info *info = arg;
-	struct intel_ir_data *data;
+	struct intel_ir_data *data, *ird;
 	struct irq_data *irq_data;
 	struct irq_cfg *irq_cfg;
 	int i, ret, index;

@@ -1176,14 +1176,20 @@ static int intel_irq_remapping_alloc(struct irq_domain *domain,
 		}
 
 		if (i > 0) {
-			data = kzalloc(sizeof(*data), GFP_KERNEL);
-			if (!data)
+			ird = kzalloc(sizeof(*data), GFP_KERNEL);
+			if (!ird)
 				goto out_free_data;
+			/* Initialize the common data */
+			ird->irq_2_iommu = data->irq_2_iommu;
+			ird->irq_2_iommu.sub_handle = i;
+		} else {
+			ird = data;
 		}
+
 		irq_data->hwirq = (index << 16) + i;
-		irq_data->chip_data = data;
+		irq_data->chip_data = ird;
 		irq_data->chip = &intel_ir_chip;
-		intel_irq_remapping_prepare_irte(data, irq_cfg, info, index, i);
+		intel_irq_remapping_prepare_irte(ird, irq_cfg, info, index, i);
 		irq_set_status_flags(virq + i, IRQ_MOVE_PCNTXT);
 	}
 	return 0;

Yes, that fixes the problem.

Assume you will fold it into

commit 289472f461d922507f75dd2451770282adb3a99b

Author: Jiang Liu <[email protected]>
Date:   Tue Nov 25 13:53:19 2014 +0800

iommu/vt-d: Enhance Intel IR driver to suppport hierarchy irqdomain

Thanks

Yinghai

Looks like you did not put your fix in new tip/x86/apic with Jiang's patchset.

Yinghai

Hi Yinghai,
Sorry, this patch got lost when I was reorganizing the patch
set. I will send out a formal patch for it soon.
Thanks!
Gerry

Reverts: eda7516e1d428 'irqdomain: Correct early allocation of irq domains with IRQs off'

Now that we distangled the allocations and the enablement of irq remapping and removed the pointless preempt disabled region from native_smp_prepare_cpus() we can remove that hackaround

---
 kernel/irq/irqdomain.c |   21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

Index: tip/kernel/irq/irqdomain.c
===================================================================
@@ -46,31 +46,14 @@ struct irq_domain *__irq_domain_add(stru
 				    void *host_data)
 {
 	struct irq_domain *domain;
-	gfp_t gfp_flags = GFP_KERNEL;
-
-#ifdef CONFIG_X86
-	/*
-	 * BIG FAT COMMENT: Early initialization paths like enable_IR_x2apic(),
-	 * for example, call into here with interrupts disabled but then we do
-	 * allocate memory and can sleep so no-no. A proper fix would be to do
-	 * x2APIC IR setup in the early irq setup path but it is too late for
-	 * fixing it this way now, shortly before the merge window.
-	 *
-	 * So we do this little brown paper bag, which is temporary! Do not even
-	 * think of calling irq domain setup code with IRQs disabled. You will
-	 * get frozen-sharked!
-	 */
-	if (irqs_disabled())
-		gfp_flags = GFP_NOFS;
-#endif
 
 	domain = kzalloc_node(sizeof(*domain) + (sizeof(unsigned int) * size),
-			      gfp_flags, of_node_to_nid(of_node));
+			      GFP_KERNEL, of_node_to_nid(of_node));
 	if (WARN_ON(!domain))
 		return NULL;
 
 	/* Fill structure */
-	INIT_RADIX_TREE(&domain->revmap_tree, gfp_flags);
+	INIT_RADIX_TREE(&domain->revmap_tree, GFP_KERNEL);
 	domain->ops = ops;
 	domain->host_data = host_data;
 	domain->of_node = of_node_get(of_node);

No reason anymore to do GFP_ATOMIC allocations which are not harmful in the normal bootup case, but matter in the physical hotplug scenario.

---
 drivers/iommu/intel_irq_remapping.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: tip/drivers/iommu/intel_irq_remapping.c
===================================================================
@@ -406,11 +406,11 @@ static int intel_setup_irq_remapping(str
 	if (iommu->ir_table)
 		return 0;
 
-	ir_table = kzalloc(sizeof(struct ir_table), GFP_ATOMIC);
+	ir_table = kzalloc(sizeof(struct ir_table), GFP_KERNEL);
 	if (!ir_table)
 		return -ENOMEM;
 
-	pages = alloc_pages_node(iommu->node, GFP_ATOMIC | __GFP_ZERO,
+	pages = alloc_pages_node(iommu->node, GFP_KERNEL | __GFP_ZERO,
 				 INTR_REMAP_PAGE_ORDER);
 	if (!pages) {
 		pr_err("IR%d: failed to allocate pages of order %d\n",

Hi Thomas,

Thanks for this nice cleanup, for the IOMMU parts you can add my

This reminded me again how complicated the iommu-setup on x86 is, I will look further in how this could be simplified.

Joerg

When converting x86 to new hierarchy irqdoamin framework, Thomas noticed that the interrupt remapping initialization flow is a little complex and has troubles in memory allocation. Then there is a joint force to simplify IR initialization flow, please refer to related threads at:
https://lkml.org/lkml/2014/12/5/114
https://lkml.org/lkml/2014/12/10/20
https://www.mail-archive.com/[email protected]/msg788792.html

This patch set is based on v3.19-rc3. And you may access it at:
https://github.com/jiangliu/linux.git ir_init_v2

This patch set combimes above three patches to simplify IR initalization flow and solves memory allocation issues. While at it, this patch set also refines CPU X2APIC initialization code for maintenance. It also fixes two bugs related to X2APIC support.
1) System hangs or panics if BIOS enables CPU X2APIC mode but kernel doesn't support X2APIC.
2) System livelocks if BIOS enables CPU X2APIC but opt-outs IR X2APIC.

This patch set has been tested with on an Intel 4-socket system with following configuration:
---------------------------------------------------------------------------
[CPU X2APIC] [IR X2APIC] [Linux IR] [Linux X2APIC] [Result]

1     D		    /		    D		    D		  OK
2     D		    /		    E		    D		  OK
3     D		    /		    E		    E		  OK
4     P		    /		    E		    E		  OK
5     P		    /		    D		    D		  Panic(expected)
6     P		    /		    E		    D		  Panic(expected)
7     P		    H		    E		    E		  OK

----------------------------------------------------------------------------
CPU X2APIC: whether CPU X2APIC is enabled by hardware and BIOS IR X2APIC: whether interrupt remapping hardware supports X2APIC mode Linux IR: whether interrupt remapping is enabled by Linux kernel Linux X2APIC: whether X2APIC is supported by Linux kernel
D: disabled
E: enabled
/: Not care
P: CPU X2APIC pre-enabled by BIOS
H: Hard-coded to opt-out X2APIC support in interrupt remapping hardware

The patch set changes the behevior of the last three rows:
1) Row 5 and 6 panics with clear messages instead of random hang or panic.
2) Row 7 boots successfully instead of livelocking.

The patch set also passes Fengguang's 0day test suites.

Due to lack of hardware platforms for tests, tests on AMD platform are welcomed!

V1->V2:
1) Rebase onto v3.19-rc3

Jiang Liu (11):
x86/apic: Panic if kernel doesn't support x2apic but BIOS has enabled
x2apic
x86/apic: Kill useless variable x2apic_enabled in function

    enable_IR_x2apic()

x86/apic: Correctly detect X2APIC status in function enable_IR() x86/apic: Refine enable_IR_x2apic() and related functions iommu/vt-d: Prepare for killing function irq_remapping_supported() iommu/vt-d: Allow IR works in XAPIC mode though CPU works in X2APIC
mode
x86/apic: Only disable CPU x2apic mode when necessary iommu/irq_remapping: Kill function irq_remapping_supported() and
related code
iommu/irq_remapping: Refine function irq_remapping_prepare() for
maintenance
iommu/irq_remapping: Change variable disable_irq_remap to be static iommu/irq_remapping: Normailize the way to detect whether IR is
enabled

Joerg Roedel (2):
iommu/vt-d: Allocate IRQ remapping data structures only for all
IOMMUs
iommu/amd: Check for irq-remap support amd_iommu_prepare()

Thomas Gleixner (3):
iommu, x86: Restructure setup of the irq remapping feature iommu/vt-d: Move iommu preparatory allocations to
irq_remap_ops.prepare
iommu/vt-d: Convert allocations to GFP_KERNEL

 arch/x86/include/asm/irq_remapping.h |    4 --
 arch/x86/kernel/apic/apic.c          |  104 ++++++++++++++++------------------
 drivers/iommu/amd_iommu.c            |    1 -
 drivers/iommu/amd_iommu_init.c       |   10 +---
 drivers/iommu/amd_iommu_proto.h      |    1 -
 drivers/iommu/intel_irq_remapping.c  |   96 ++++++++++++++++---------------
 drivers/iommu/irq_remapping.c        |   74 ++++++++----------------
 drivers/iommu/irq_remapping.h        |    5 --
 8 files changed, 128 insertions(+), 167 deletions(-)

1.7.10.4

From: Thomas Gleixner <[email protected]>

enable_IR_x2apic() calls setup_irq_remapping_ops() which by default installs the intel dmar remapping ops and then calls the amd iommu irq remapping prepare callback to figure out whether we are running on an AMD machine with irq remapping hardware.

Right after that it calls irq_remapping_prepare() which pointlessly checks:

	if (!remap_ops || !remap_ops->prepare)
               return -ENODEV;

and then calls

    remap_ops->prepare()

which is silly in the AMD case as it got called from setup_irq_remapping_ops() already a few microseconds ago.

Simplify this and just collapse everything into
irq_remapping_prepare().

The irq_remapping_prepare() remains still silly as it assigns blindly the intel ops, but that's not scope of this patch.

The scope here is to move the preperatory work, i.e. memory allocations out of the atomic section which is required to enable irq remapping.

---
 arch/x86/include/asm/irq_remapping.h |    2 --
 arch/x86/kernel/apic/apic.c          |    3 ---
 drivers/iommu/irq_remapping.c        |   19 +++++++------------
 3 files changed, 7 insertions(+), 17 deletions(-)

index b7747c4c2cf2..f1b619e5a50d 100644
@@ -33,7 +33,6 @@ struct irq_cfg;
 
 #ifdef CONFIG_IRQ_REMAP
 
-extern void setup_irq_remapping_ops(void);
 extern int irq_remapping_supported(void);
 extern void set_irq_remapping_broken(void);
 extern int irq_remapping_prepare(void);

@@ -60,7 +59,6 @@ void irq_remap_modify_chip_defaults(struct irq_chip *chip);
 
 #else  /* CONFIG_IRQ_REMAP */
 
-static inline void setup_irq_remapping_ops(void) { }
 static inline int irq_remapping_supported(void) { return 0; }
 static inline void set_irq_remapping_broken(void) { }
 static inline int irq_remapping_prepare(void) { return -ENODEV; }

index 71b8a6cb7f0e..36249c715da0 100644
@@ -1597,9 +1597,6 @@ void __init enable_IR_x2apic(void)
 	int ret, x2apic_enabled = 0;
 	int hardware_init_ret;
 
-	/* Make sure irq_remap_ops are initialized */
-	setup_irq_remapping_ops();
-
 	hardware_init_ret = irq_remapping_prepare();
 	if (hardware_init_ret && !x2apic_supported())
 		return;

index 89c4846683be..91d5884d3ed9 100644
@@ -194,16 +194,6 @@ static __init int setup_irqremap(char *str)
 }
 early_param("intremap", setup_irqremap);
 
-void __init setup_irq_remapping_ops(void)
-{
-	remap_ops = &intel_irq_remap_ops;
-
-#ifdef CONFIG_AMD_IOMMU
-	if (amd_iommu_irq_ops.prepare() == 0)
-		remap_ops = &amd_iommu_irq_ops;
-#endif
-}
-
 void set_irq_remapping_broken(void)
 {
 	irq_remap_broken = 1;

@@ -222,9 +212,14 @@ int irq_remapping_supported(void)
 
 int __init irq_remapping_prepare(void)
 {
-	if (!remap_ops || !remap_ops->prepare)
-		return -ENODEV;
+	remap_ops = &intel_irq_remap_ops;
 
+#ifdef CONFIG_AMD_IOMMU
+	if (amd_iommu_irq_ops.prepare() == 0) {
+		remap_ops = &amd_iommu_irq_ops;
+		return 0;
+	}
+#endif
 	return remap_ops->prepare();
 }
 

1.7.10.4

From: Thomas Gleixner <[email protected]>

The whole iommu setup for irq remapping is a convoluted mess. The iommu detect function gets called from mem_init() and the prepare callback gets called from enable_IR_x2apic() for unknown reasons.

Of course AMD and Intel setup differs in nonsensical ways. Intels prepare callback is explicit while AMDs prepare callback is implicit in setup_irq_remapping_ops() just to be called in the prepare call again.

Because all of this gets called from enable_IR_x2apic() and the dmar prepare function merily parses the ACPI tables, but does not allocate memory we end up with memory allocation from irq disabled context later on.

AMDs iommu code at least allocates the required memory from the prepare function. That has issues as well, but thats not scope of this patch.

The goal of this change is to distangle the allocation from the actual enablement. There is no point to allocate memory from irq disabled regions with GFP_ATOMIC just because it does not matter at that point in the boot stage. It matters with physical hotplug later on.

There is another issue with the current setup. Due to the conversion to stacked irqdomains we end up with a call into the irqdomain allocation code from irq disabled context, but that code does GFP_KERNEL allocations rightfully as there is no reason to do preperatory allocations with GFP_ATOMIC.

That change caused the allocator code to complain about GFP_KERNEL allocations invoked in atomic context. Boris provided a temporary hackaround which changed the GFP flags if irq_domain_add() got called from atomic context. Not pretty and we really dont want to get this into a mainline release for obvious reasons.

Move the ACPI table parsing and the resulting memory allocations from the enable to the prepare function. That allows to get rid of the horrible hackaround in irq_domain_add() later.

[Jiang] Rebased onto v3.19

---
 drivers/iommu/intel_irq_remapping.c |   64 ++++++++++++++++++++++++-----------
 1 file changed, 44 insertions(+), 20 deletions(-)

index a55b207b9425..2360cb6a8896 100644
@@ -595,22 +595,57 @@ static int __init intel_irq_remapping_supported(void)
 	return 1;
 }
 
-static int __init intel_enable_irq_remapping(void)
+static void __init intel_cleanup_irq_remapping(void)
+{
+	struct dmar_drhd_unit *drhd;
+	struct intel_iommu *iommu;
+
+	for_each_iommu(iommu, drhd) {
+		if (ecap_ir_support(iommu->ecap)) {
+			iommu_disable_irq_remapping(iommu);
+			intel_teardown_irq_remapping(iommu);
+		}
+	}
+
+	if (x2apic_supported())
+		pr_warn("Failed to enable irq remapping.  You are vulnerable to irq-injection attacks.\n");
+}
+
+static int __init intel_prepare_irq_remapping(void)
 {
 	struct dmar_drhd_unit *drhd;
 	struct intel_iommu *iommu;
-	bool x2apic_present;
-	int setup = 0;
-	int eim = 0;
 
-	x2apic_present = x2apic_supported();
+	if (dmar_table_init() < 0)
+		return -1;
 
 	if (parse_ioapics_under_ir() != 1) {
-		printk(KERN_INFO "Not enable interrupt remapping\n");
+		printk(KERN_INFO "Not enabling interrupt remapping\n");
 		goto error;
 	}
 
-	if (x2apic_present) {
+	for_each_iommu(iommu, drhd) {
+		if (!ecap_ir_support(iommu->ecap))
+			continue;
+
+		/* Do the allocations early */
+		if (intel_setup_irq_remapping(iommu))
+			goto error;
+	}
+	return 0;
+error:
+	intel_cleanup_irq_remapping();
+	return -1;
+}
+
+static int __init intel_enable_irq_remapping(void)
+{
+	struct dmar_drhd_unit *drhd;
+	struct intel_iommu *iommu;
+	int setup = 0;
+	int eim = 0;
+
+	if (x2apic_supported()) {
 		pr_info("Queued invalidation will be enabled to support x2apic and Intr-remapping.\n");
 
 		eim = !dmar_x2apic_optout();

@@ -678,9 +713,6 @@ static int __init intel_enable_irq_remapping(void)
 		if (!ecap_ir_support(iommu->ecap))
 			continue;
 
-		if (intel_setup_irq_remapping(iommu))
-			goto error;
-
 		iommu_set_irq_remapping(iommu, eim);
 		setup = 1;
 	}

@@ -702,15 +734,7 @@ static int __init intel_enable_irq_remapping(void)
 	return eim ? IRQ_REMAP_X2APIC_MODE : IRQ_REMAP_XAPIC_MODE;
 
 error:
-	for_each_iommu(iommu, drhd)
-		if (ecap_ir_support(iommu->ecap)) {
-			iommu_disable_irq_remapping(iommu);
-			intel_teardown_irq_remapping(iommu);
-		}
-
-	if (x2apic_present)
-		pr_warn("Failed to enable irq remapping.  You are vulnerable to irq-injection attacks.\n");
-
+	intel_cleanup_irq_remapping();
 	return -1;
 }
 

@@ -1200,7 +1224,7 @@ static int intel_alloc_hpet_msi(unsigned int irq, unsigned int id)
 
 struct irq_remap_ops intel_irq_remap_ops = {
 	.supported		= intel_irq_remapping_supported,
-	.prepare		= dmar_table_init,
+	.prepare		= intel_prepare_irq_remapping,
 	.enable			= intel_enable_irq_remapping,
 	.disable		= disable_irq_remapping,
 	.reenable		= reenable_irq_remapping,

1.7.10.4

From: Thomas Gleixner <[email protected]>

No reason anymore to do GFP_ATOMIC allocations which are not harmful in the normal bootup case, but matter in the physical hotplug scenario.

---
 drivers/iommu/intel_irq_remapping.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 2360cb6a8896..1e7e09327753 100644
@@ -481,11 +481,11 @@ static int intel_setup_irq_remapping(struct intel_iommu *iommu)
 	if (iommu->ir_table)
 		return 0;
 
-	ir_table = kzalloc(sizeof(struct ir_table), GFP_ATOMIC);
+	ir_table = kzalloc(sizeof(struct ir_table), GFP_KERNEL);
 	if (!ir_table)
 		return -ENOMEM;
 
-	pages = alloc_pages_node(iommu->node, GFP_ATOMIC | __GFP_ZERO,
+	pages = alloc_pages_node(iommu->node, GFP_KERNEL | __GFP_ZERO,
 				 INTR_REMAP_PAGE_ORDER);
 
 	if (!pages) {

1.7.10.4

When kernel doesn't support X2APIC but BIOS has enabled X2APIC, system may panic or hang without useful messages. On the other hand, it's hard to dynamically disable X2APIC when CONFIG_X86_X2APIC is disabled. So panic with a clear message in such a case.

System panics as below when X2APIC is disabled and interrupt remapping is enabled:

[    0.316118] LAPIC pending interrupts after 512 EOI
[    0.322126] ..TIMER: vector=0x30 apic1=0 pin1=2 apic2=-1 pin2=-1
[    0.368655] Kernel panic - not syncing: timer doesn't work through Interrupt-remapped IO-APIC
[    0.378300] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.18.0+ #340
[    0.385300] Hardware name: Intel Corporation BRICKLAND/BRICKLAND, BIOS BRIVTIN1.86B.0051.L05.1406240953 06/24/2014
[    0.396997]  ffff88046dc03000 ffff88046c307dd8 ffffffff8179dada 00000000000043f2
[    0.405629]  ffffffff81a92158 ffff88046c307e58 ffffffff8179b757 0000000000000002
[    0.414261]  0000000000000008 ffff88046c307e68 ffff88046c307e08 ffffffff813ad82b
[    0.422890] Call Trace:
[    0.425711]  [<ffffffff8179dada>] dump_stack+0x45/0x57
[    0.431533]  [<ffffffff8179b757>] panic+0xc1/0x1f5
[    0.436978]  [<ffffffff813ad82b>] ? delay_tsc+0x3b/0x70
[    0.442910]  [<ffffffff8166fa2c>] panic_if_irq_remap+0x1c/0x20
[    0.449524]  [<ffffffff81d73645>] setup_IO_APIC+0x405/0x82e
[    0.464979]  [<ffffffff81d6fcc2>] native_smp_prepare_cpus+0x2d9/0x31c
[    0.472274]  [<ffffffff81d5d0ac>] kernel_init_freeable+0xd6/0x223
[    0.479170]  [<ffffffff81792ad0>] ? rest_init+0x80/0x80
[    0.485099]  [<ffffffff81792ade>] kernel_init+0xe/0xf0
[    0.490932]  [<ffffffff817a537c>] ret_from_fork+0x7c/0xb0
[    0.497054]  [<ffffffff81792ad0>] ? rest_init+0x80/0x80
[    0.502983] ---[ end Kernel panic - not syncing: timer doesn't work through Interrupt-remapped IO-APIC

System hangs as below when X2APIC and interrupt remapping are both disabled:

[    1.102782] pci 0000:00:02.0: System wakeup disabled by ACPI
[    1.109351] pci 0000:00:03.0: System wakeup disabled by ACPI
[    1.115915] pci 0000:00:03.2: System wakeup disabled by ACPI
[    1.122479] pci 0000:00:03.3: System wakeup disabled by ACPI
[    1.132274] pci 0000:00:1c.0: Enabling MPC IRBNCE
[    1.137620] pci 0000:00:1c.0: Intel PCH root port ACS workaround enabled
[    1.145239] pci 0000:00:1c.0: System wakeup disabled by ACPI
[    1.151790] pci 0000:00:1c.7: Enabling MPC IRBNCE
[    1.157128] pci 0000:00:1c.7: Intel PCH root port ACS workaround enabled
[    1.164748] pci 0000:00:1c.7: System wakeup disabled by ACPI
[    1.171447] pci 0000:00:1e.0: System wakeup disabled by ACPI
[    1.178612] acpiphp: Slot [8] registered
[    1.183095] pci 0000:00:02.0: PCI bridge to [bus 01]
[    1.188867] acpiphp: Slot [2] registered

---
 arch/x86/kernel/apic/apic.c |    8 ++++++++
 1 file changed, 8 insertions(+)

index 36249c715da0..8b58e23bc5e8 100644
@@ -1597,6 +1597,14 @@ void __init enable_IR_x2apic(void)
 	int ret, x2apic_enabled = 0;
 	int hardware_init_ret;
 
+	if (!IS_ENABLED(CONFIG_X86_X2APIC)) {
+		u64 msr;
+
+		rdmsrl(MSR_IA32_APICBASE, msr);
+		if (msr & X2APIC_ENABLE)
+			panic("BIOS has enabled x2apic but kernel doesn't support x2apic, please disable x2apic in BIOS.\n");
+	}
+
 	hardware_init_ret = irq_remapping_prepare();
 	if (hardware_init_ret && !x2apic_supported())
 		return;

1.7.10.4

Local variable x2apic_enabled has been assigned to but never referred, so kill it.

---
 arch/x86/kernel/apic/apic.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

index 8b58e23bc5e8..a614e242473b 100644
@@ -1594,7 +1594,7 @@ int __init enable_IR(void)
 void __init enable_IR_x2apic(void)
 {
 	unsigned long flags;
-	int ret, x2apic_enabled = 0;
+	int ret;
 	int hardware_init_ret;
 
 	if (!IS_ENABLED(CONFIG_X86_X2APIC)) {

@@ -1652,8 +1652,6 @@ void __init enable_IR_x2apic(void)
 		goto skip_x2apic;
 	}
 
-	x2apic_enabled = 1;
-
 	if (x2apic_supported() && !x2apic_mode) {
 		x2apic_mode = 1;
 		enable_x2apic();

1.7.10.4

X2APIC will be disabled if user specifies "nox2apic" on kernel command line, even when x2apic_preenabled is true. So correctly detect X2APIC status by using x2apic_enabled() instead of x2apic_preenabled.

---
 arch/x86/kernel/apic/apic.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index a614e242473b..8ce2b8236c1b 100644
@@ -1580,7 +1580,7 @@ int __init enable_IR(void)
 		return -1;
 	}
 
-	if (!x2apic_preenabled && skip_ioapic_setup) {
+	if (!x2apic_enabled() && skip_ioapic_setup) {
 		pr_info("Skipped enabling intr-remap because of skipping "
 			"io-apic setup\n");
 		return -1;

1.7.10.4

Refine enable_IR_x2apic() and related functions for better readability.

It also changes the way to handle IR in XAPIC mode when enabling X2APIC. Previously it just skips X2APIC initialization without checking max CPU APIC ID in system, which may cause problem if system has CPU with APIC ID bigger than 255. So treat IR in XAPIC mode as same as IR is disabled when enabling CPU X2APIC.

---
 arch/x86/kernel/apic/apic.c |   85 +++++++++++++++++++++----------------------
 1 file changed, 41 insertions(+), 44 deletions(-)

index 8ce2b8236c1b..09ac1e4ef86b 100644
@@ -1572,7 +1572,7 @@ void enable_x2apic(void)
 }
 #endif /* CONFIG_X86_X2APIC */
 
-int __init enable_IR(void)
+static int __init try_to_enable_IR(void)
 {
 #ifdef CONFIG_IRQ_REMAP
 	if (!irq_remapping_supported()) {

@@ -1585,17 +1585,48 @@ int __init enable_IR(void)
 			"io-apic setup\n");
 		return -1;
 	}
+#endif
 
 	return irq_remapping_enable();
+}
+
+static __init void try_to_enable_x2apic(int ir_stat)
+{
+#ifdef CONFIG_X86_X2APIC
+	if (!x2apic_supported())
+		return;
+
+	if (ir_stat != IRQ_REMAP_X2APIC_MODE) {
+		/* IR is required if there is APIC ID > 255 even when running
+		 * under KVM
+		 */
+		if (max_physical_apicid > 255 ||
+		    !hypervisor_x2apic_available()) {
+			pr_info("IRQ remapping doesn't support X2APIC mode, disable x2apic.\n");
+			if (x2apic_preenabled)
+				disable_x2apic();
+			return;
+		}
+
+		/*
+		 * without IR all CPUs can be addressed by IOAPIC/MSI
+		 * only in physical mode
+		 */
+		x2apic_force_phys();
+	}
+
+	if (!x2apic_mode) {
+		x2apic_mode = 1;
+		enable_x2apic();
+		pr_info("Enabled x2apic\n");
+	}
 #endif
-	return -1;
 }
 
 void __init enable_IR_x2apic(void)
 {
 	unsigned long flags;
-	int ret;
-	int hardware_init_ret;
+	int ret, ir_stat;
 
 	if (!IS_ENABLED(CONFIG_X86_X2APIC)) {
 		u64 msr;

@@ -1605,8 +1636,8 @@ void __init enable_IR_x2apic(void)
 			panic("BIOS has enabled x2apic but kernel doesn't support x2apic, please disable x2apic in BIOS.\n");
 	}
 
-	hardware_init_ret = irq_remapping_prepare();
-	if (hardware_init_ret && !x2apic_supported())
+	ir_stat = irq_remapping_prepare();
+	if (ir_stat < 0 && !x2apic_supported())
 		return;
 
 	ret = save_ioapic_entries();

@@ -1621,45 +1652,11 @@ void __init enable_IR_x2apic(void)
 
 	if (x2apic_preenabled && nox2apic)
 		disable_x2apic();
+	if (ir_stat >= 0)
+		ir_stat = try_to_enable_IR();
+	try_to_enable_x2apic(ir_stat);
 
-	if (hardware_init_ret)
-		ret = -1;
-	else
-		ret = enable_IR();
-
-	if (!x2apic_supported())
-		goto skip_x2apic;
-
-	if (ret < 0) {
-		/* IR is required if there is APIC ID > 255 even when running
-		 * under KVM
-		 */
-		if (max_physical_apicid > 255 ||
-		    !hypervisor_x2apic_available()) {
-			if (x2apic_preenabled)
-				disable_x2apic();
-			goto skip_x2apic;
-		}
-		/*
-		 * without IR all CPUs can be addressed by IOAPIC/MSI
-		 * only in physical mode
-		 */
-		x2apic_force_phys();
-	}
-
-	if (ret == IRQ_REMAP_XAPIC_MODE) {
-		pr_info("x2apic not enabled, IRQ remapping is in xapic mode\n");
-		goto skip_x2apic;
-	}
-
-	if (x2apic_supported() && !x2apic_mode) {
-		x2apic_mode = 1;
-		enable_x2apic();
-		pr_info("Enabled x2apic\n");
-	}
-
-skip_x2apic:
-	if (ret < 0) /* IR enabling failed */
+	if (ir_stat < 0) /* IR enabling failed */
 		restore_ioapic_entries();
 	legacy_pic->restore_mask();
 	local_irq_restore(flags);

1.7.10.4

Please don't do that. This wants to be two patches:

1) reordering the code

2) changing the operation

I've split it already, so no need to resend.

Thanks,

tglx

Prepare for killing function irq_remapping_supported() by moving code from intel_irq_remapping_supported() into intel_prepare_irq_remapping().

Combined with patch from Joerg at https://lkml.org/lkml/2014/12/15/487,
so assume an signed-off from Joerg.

---
 drivers/iommu/intel_irq_remapping.c |   58 +++++++++++++++--------------------
 1 file changed, 25 insertions(+), 33 deletions(-)

index 1e7e09327753..8ccc7aa7e43a 100644
@@ -568,30 +568,6 @@ static int __init dmar_x2apic_optout(void)
 
 static int __init intel_irq_remapping_supported(void)
 {
-	struct dmar_drhd_unit *drhd;
-	struct intel_iommu *iommu;
-
-	if (disable_irq_remap)
-		return 0;
-	if (irq_remap_broken) {
-		printk(KERN_WARNING
-			"This system BIOS has enabled interrupt remapping\n"
-			"on a chipset that contains an erratum making that\n"
-			"feature unstable.  To maintain system stability\n"
-			"interrupt remapping is being disabled.  Please\n"
-			"contact your BIOS vendor for an update\n");
-		add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
-		disable_irq_remap = 1;
-		return 0;
-	}
-
-	if (!dmar_ir_support())
-		return 0;
-
-	for_each_iommu(iommu, drhd)
-		if (!ecap_ir_support(iommu->ecap))
-			return 0;
-
 	return 1;
 }
 

@@ -616,26 +592,42 @@ static int __init intel_prepare_irq_remapping(void)
 	struct dmar_drhd_unit *drhd;
 	struct intel_iommu *iommu;
 
+	/* First check whether IRQ remapping should be enabled */
+	if (disable_irq_remap)
+		return -ENODEV;
+
+	if (irq_remap_broken) {
+		printk(KERN_WARNING
+			"This system BIOS has enabled interrupt remapping\n"
+			"on a chipset that contains an erratum making that\n"
+			"feature unstable.  To maintain system stability\n"
+			"interrupt remapping is being disabled.  Please\n"
+			"contact your BIOS vendor for an update\n");
+		add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
+		disable_irq_remap = 1;
+		return -ENODEV;
+	}
+
 	if (dmar_table_init() < 0)
-		return -1;
+		return -ENODEV;
+
+	if (!dmar_ir_support())
+		return -ENODEV;
 
 	if (parse_ioapics_under_ir() != 1) {
 		printk(KERN_INFO "Not enabling interrupt remapping\n");
 		goto error;
 	}
 
-	for_each_iommu(iommu, drhd) {
-		if (!ecap_ir_support(iommu->ecap))
-			continue;
-
-		/* Do the allocations early */
-		if (intel_setup_irq_remapping(iommu))
+	for_each_iommu(iommu, drhd)
+		if (!ecap_ir_support(iommu->ecap) ||
+		    intel_setup_irq_remapping(iommu))
 			goto error;
-	}
 	return 0;
+
 error:
 	intel_cleanup_irq_remapping();
-	return -1;
+	return -ENODEV;
 }
 
 static int __init intel_enable_irq_remapping(void)

1.7.10.4

From: Joerg Roedel <[email protected]>

IRQ remapping is only supported when all IOMMUs in the system support it. So check if all IOMMUs in the system support IRQ remapping before doing the allocations.

[Jiang]
1) Rebased onto v3.19.
2) Remove redundant check of ecap_ir_support(iommu->ecap) in function intel_enable_irq_remapping().

---
 drivers/iommu/intel_irq_remapping.c |   19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

index 8ccc7aa7e43a..137663bd5da2 100644
@@ -619,10 +619,16 @@ static int __init intel_prepare_irq_remapping(void)
 		goto error;
 	}
 
+	/* First make sure all IOMMUs support IRQ remapping */
 	for_each_iommu(iommu, drhd)
-		if (!ecap_ir_support(iommu->ecap) ||
-		    intel_setup_irq_remapping(iommu))
+		if (!ecap_ir_support(iommu->ecap))
+			goto error;
+
+	/* Do the allocations early */
+	for_each_iommu(iommu, drhd)
+		if (intel_setup_irq_remapping(iommu))
 			goto error;
+
 	return 0;
 
 error:

@@ -673,16 +679,12 @@ static int __init intel_enable_irq_remapping(void)
 	/*
 	 * check for the Interrupt-remapping support
 	 */
-	for_each_iommu(iommu, drhd) {
-		if (!ecap_ir_support(iommu->ecap))
-			continue;
-
+	for_each_iommu(iommu, drhd)
 		if (eim && !ecap_eim_support(iommu->ecap)) {
 			printk(KERN_INFO "DRHD %Lx: EIM not supported by DRHD, "
 			       " ecap %Lx\n", drhd->reg_base_addr, iommu->ecap);
 			goto error;
 		}
-	}
 
 	/*
 	 * Enable queued invalidation for all the DRHD's.

@@ -702,9 +704,6 @@ static int __init intel_enable_irq_remapping(void)
 	 * Setup Interrupt-remapping for all the DRHD's now.
 	 */
 	for_each_iommu(iommu, drhd) {
-		if (!ecap_ir_support(iommu->ecap))
-			continue;
-
 		iommu_set_irq_remapping(iommu, eim);
 		setup = 1;
 	}

1.7.10.4

Currently if CPU supports X2APIC, IR hardware must work in X2APIC mode or disabled. Change the code to support IR working in XAPIC mode when CPU supports X2APIC. Then the CPU APIC driver will decide how to handle such as configuration by:
1) Disabling X2APIC mode
2) Forcing X2APIC physical mode

This change also fixes a live locking when
1) BIOS enables CPU X2APIC
2) DMAR table disables X2APIC mode or IR hardware doesn't support X2APIC with following messages:

[   37.863463] dmar: INTR-REMAP: Request device [[f0:1f.7] fault index 2
[   37.863463] INTR-REMAP:[fault reason 36] Detected reserved fields in the IRTE entry
[   37.879372] dmar: INTR-REMAP: Request device [[f0:1f.7] fault index 2
[   37.879372] INTR-REMAP:[fault reason 36] Detected reserved fields in the IRTE entry
[   37.895282] dmar: INTR-REMAP: Request device [[f0:1f.7] fault index 2
[   37.895282] INTR-REMAP:[fault reason 36] Detected reserved fields in the IRTE entry
[   37.911192] dmar: INTR-REMAP: Request device [[f0:1f.7] fault index 2

---
 drivers/iommu/intel_irq_remapping.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

index 137663bd5da2..9d67c12c2ffb 100644
@@ -32,8 +32,9 @@ struct hpet_scope {
 };
 
 #define IR_X2APIC_MODE(mode) (mode ? (1 << 11) : 0)
-#define IRTE_DEST(dest) ((x2apic_mode) ? dest : dest << 8)
+#define IRTE_DEST(dest) ((eim_mode) ? dest : dest << 8)
 
+static int __read_mostly eim_mode;
 static struct ioapic_scope ir_ioapic[MAX_IO_APICS];
 static struct hpet_scope ir_hpet[MAX_HPET_TBS];
 

@@ -644,8 +645,6 @@ static int __init intel_enable_irq_remapping(void)
 	int eim = 0;
 
 	if (x2apic_supported()) {
-		pr_info("Queued invalidation will be enabled to support x2apic and Intr-remapping.\n");
-
 		eim = !dmar_x2apic_optout();
 		if (!eim)
 			printk(KERN_WARNING

@@ -683,8 +682,11 @@ static int __init intel_enable_irq_remapping(void)
 		if (eim && !ecap_eim_support(iommu->ecap)) {
 			printk(KERN_INFO "DRHD %Lx: EIM not supported by DRHD, "
 			       " ecap %Lx\n", drhd->reg_base_addr, iommu->ecap);
-			goto error;
+			eim = 0;
 		}
+	eim_mode = eim;
+	if (eim)
+		pr_info("Queued invalidation will be enabled to support x2apic and Intr-remapping.\n");
 
 	/*
 	 * Enable queued invalidation for all the DRHD's.

1.7.10.4

When interrupt remapping hardware is not in X2APIC, CPU X2APIC mode will be disabled if:
1) Maximum CPU APIC ID is bigger than 255
2) hypervisior doesn't support x2apic mode.

But we should only check whether hypervisor supports X2APIC mode when hypervisor(CONFIG_HYPERVISOR_GUEST) is enabled, otherwise X2APIC will always be disabled when CONFIG_HYPERVISOR_GUEST is disabled and IR doesn't work in X2APIC mode.

---
 arch/x86/kernel/apic/apic.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 09ac1e4ef86b..d714e72ed6d5 100644
@@ -1601,7 +1601,8 @@ static __init void try_to_enable_x2apic(int ir_stat)
 		 * under KVM
 		 */
 		if (max_physical_apicid > 255 ||
-		    !hypervisor_x2apic_available()) {
+		    (IS_ENABLED(CONFIG_HYPERVISOR_GUEST) &&
+		     !hypervisor_x2apic_available())) {
 			pr_info("IRQ remapping doesn't support X2APIC mode, disable x2apic.\n");
 			if (x2apic_preenabled)
 				disable_x2apic();

1.7.10.4

From: Joerg Roedel <[email protected]>

This allows to get rid of the irq_remapping_supported() function and all its call-backs into the Intel and AMD IOMMU drivers.

---
 drivers/iommu/amd_iommu_init.c |    3 +++
 1 file changed, 3 insertions(+)

index b0522f15730f..0039f87f48b8 100644
@@ -2123,6 +2123,9 @@ static int __init iommu_go_to_state(enum iommu_init_state state)
 #ifdef CONFIG_IRQ_REMAP
 int __init amd_iommu_prepare(void)
 {
+	if (!amd_iommu_irq_remap)
+		return -1;
+
 	return iommu_go_to_state(IOMMU_ACPI_FINISHED);
 }
 

1.7.10.4

Simplify irq_remapping code by killing irq_remapping_supported() and related interfaces.

Joerg posted a similar patch at https://lkml.org/lkml/2014/12/15/490,
so assume an signed-off from Joerg.

---
 arch/x86/include/asm/irq_remapping.h |    2 --
 arch/x86/kernel/apic/apic.c          |    7 +------
 drivers/iommu/amd_iommu.c            |    1 -
 drivers/iommu/amd_iommu_init.c       |    5 -----
 drivers/iommu/amd_iommu_proto.h      |    1 -
 drivers/iommu/intel_irq_remapping.c  |    6 ------
 drivers/iommu/irq_remapping.c        |   12 ++----------
 drivers/iommu/irq_remapping.h        |    3 ---
 8 files changed, 3 insertions(+), 34 deletions(-)

index f1b619e5a50d..6224d316c405 100644
@@ -33,7 +33,6 @@ struct irq_cfg;
 
 #ifdef CONFIG_IRQ_REMAP
 
-extern int irq_remapping_supported(void);
 extern void set_irq_remapping_broken(void);
 extern int irq_remapping_prepare(void);
 extern int irq_remapping_enable(void);

@@ -59,7 +58,6 @@ void irq_remap_modify_chip_defaults(struct irq_chip *chip);
 
 #else  /* CONFIG_IRQ_REMAP */
 
-static inline int irq_remapping_supported(void) { return 0; }
 static inline void set_irq_remapping_broken(void) { }
 static inline int irq_remapping_prepare(void) { return -ENODEV; }
 static inline int irq_remapping_enable(void) { return -ENODEV; }

index d714e72ed6d5..a526abd51c24 100644
@@ -1574,12 +1574,7 @@ void enable_x2apic(void)
 
 static int __init try_to_enable_IR(void)
 {
-#ifdef CONFIG_IRQ_REMAP
-	if (!irq_remapping_supported()) {
-		pr_debug("intr-remapping not supported\n");
-		return -1;
-	}
-
+#ifdef CONFIG_X86_IO_APIC
 	if (!x2apic_enabled() && skip_ioapic_setup) {
 		pr_info("Skipped enabling intr-remap because of skipping "
 			"io-apic setup\n");

index 98024856df07..59de6364a910 100644
@@ -4284,7 +4284,6 @@ static int alloc_hpet_msi(unsigned int irq, unsigned int id)
 }
 
 struct irq_remap_ops amd_iommu_irq_ops = {
-	.supported		= amd_iommu_supported,
 	.prepare		= amd_iommu_prepare,
 	.enable			= amd_iommu_enable,
 	.disable		= amd_iommu_disable,

index 0039f87f48b8..970979ecbebb 100644
@@ -2129,11 +2129,6 @@ int __init amd_iommu_prepare(void)
 	return iommu_go_to_state(IOMMU_ACPI_FINISHED);
 }
 
-int __init amd_iommu_supported(void)
-{
-	return amd_iommu_irq_remap ? 1 : 0;
-}
-
 int __init amd_iommu_enable(void)
 {
 	int ret;

index 95ed6deae47f..861af9d8338a 100644
@@ -33,7 +33,6 @@ extern void amd_iommu_init_notifier(void);
 extern void amd_iommu_init_api(void);
 
 /* Needed for interrupt remapping */
-extern int amd_iommu_supported(void);
 extern int amd_iommu_prepare(void);
 extern int amd_iommu_enable(void);
 extern void amd_iommu_disable(void);

index 9d67c12c2ffb..fb72bd5f438c 100644
@@ -567,11 +567,6 @@ static int __init dmar_x2apic_optout(void)
 	return dmar->flags & DMAR_X2APIC_OPT_OUT;
 }
 
-static int __init intel_irq_remapping_supported(void)
-{
-	return 1;
-}
-
 static void __init intel_cleanup_irq_remapping(void)
 {
 	struct dmar_drhd_unit *drhd;

@@ -1216,7 +1211,6 @@ static int intel_alloc_hpet_msi(unsigned int irq, unsigned int id)
 }
 
 struct irq_remap_ops intel_irq_remap_ops = {
-	.supported		= intel_irq_remapping_supported,
 	.prepare		= intel_prepare_irq_remapping,
 	.enable			= intel_enable_irq_remapping,
 	.disable		= disable_irq_remapping,

index 91d5884d3ed9..e7449b42504d 100644
@@ -199,19 +199,11 @@ void set_irq_remapping_broken(void)
 	irq_remap_broken = 1;
 }
 
-int irq_remapping_supported(void)
+int __init irq_remapping_prepare(void)
 {
 	if (disable_irq_remap)
-		return 0;
-
-	if (!remap_ops || !remap_ops->supported)
-		return 0;
+		return -ENOSYS;
 
-	return remap_ops->supported();
-}
-
-int __init irq_remapping_prepare(void)
-{
 	remap_ops = &intel_irq_remap_ops;
 
 #ifdef CONFIG_AMD_IOMMU

index fde250f86e60..a8edfea0ab5d 100644
@@ -38,9 +38,6 @@ extern int no_x2apic_optout;
 extern int irq_remapping_enabled;
 
 struct irq_remap_ops {
-	/* Check whether Interrupt Remapping is supported */
-	int (*supported)(void);
-
 	/* Initializes hardware and makes it ready for remapping interrupts */
 	int  (*prepare)(void);
 

1.7.10.4

Assign intel_irq_remap_ops to remap_ops only if
intel_irq_remap_ops.prepare() succeeds.

---
 drivers/iommu/irq_remapping.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

index e7449b42504d..7d85d2ba0e8b 100644
@@ -204,15 +204,15 @@ int __init irq_remapping_prepare(void)
 	if (disable_irq_remap)
 		return -ENOSYS;
 
-	remap_ops = &intel_irq_remap_ops;
-
-#ifdef CONFIG_AMD_IOMMU
-	if (amd_iommu_irq_ops.prepare() == 0) {
+	if (intel_irq_remap_ops.prepare() == 0)
+		remap_ops = &intel_irq_remap_ops;
+	else if (IS_ENABLED(CONFIG_AMD_IOMMU) &&
+		 amd_iommu_irq_ops.prepare() == 0)
 		remap_ops = &amd_iommu_irq_ops;
-		return 0;
-	}
-#endif
-	return remap_ops->prepare();
+	else
+		return -ENOSYS;
+
+	return 0;
 }
 
 int __init irq_remapping_enable(void)

1.7.10.4

Change variable disable_irq_remap to be static and simplify the code.

---
 drivers/iommu/amd_iommu_init.c      |    6 +-----
 drivers/iommu/intel_irq_remapping.c |    5 -----
 drivers/iommu/irq_remapping.c       |    3 +--
 drivers/iommu/irq_remapping.h       |    2 --
 4 files changed, 2 insertions(+), 14 deletions(-)

index 970979ecbebb..e430dc8dffdf 100644
@@ -2014,9 +2014,6 @@ static bool detect_ivrs(void)
 	/* Make sure ACS will be enabled during PCI probe */
 	pci_request_acs();
 
-	if (!disable_irq_remap)
-		amd_iommu_irq_remap = true;
-
 	return true;
 }
 

@@ -2123,8 +2120,7 @@ static int __init iommu_go_to_state(enum iommu_init_state state)
 #ifdef CONFIG_IRQ_REMAP
 int __init amd_iommu_prepare(void)
 {
-	if (!amd_iommu_irq_remap)
-		return -1;
+	amd_iommu_irq_remap = true;
 
 	return iommu_go_to_state(IOMMU_ACPI_FINISHED);
 }

index fb72bd5f438c..14de1ab223c8 100644
@@ -588,10 +588,6 @@ static int __init intel_prepare_irq_remapping(void)
 	struct dmar_drhd_unit *drhd;
 	struct intel_iommu *iommu;
 
-	/* First check whether IRQ remapping should be enabled */
-	if (disable_irq_remap)
-		return -ENODEV;
-
 	if (irq_remap_broken) {
 		printk(KERN_WARNING
 			"This system BIOS has enabled interrupt remapping\n"

@@ -600,7 +596,6 @@ static int __init intel_prepare_irq_remapping(void)
 			"interrupt remapping is being disabled.  Please\n"
 			"contact your BIOS vendor for an update\n");
 		add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
-		disable_irq_remap = 1;
 		return -ENODEV;
 	}