Skip to contentSkip to conversation

iommu/amd: Fix booting with amd_iommu=off

PATCH In review Joerg Roedel 3 participants 10 messages Mar 17, 2021 – Mar 18, 2021

Subsystems (3)
lore
Share & tools
lore

Joerg Roedel

author maintainer
#0 lore raw

cover iommu/amd: Fix booting with amd_iommu=off

From: Joerg Roedel <[email protected]>

Hi,

it turned out that booting a kernel with amd_iommu=off on a machine that has an AMD IOMMU causes an early kernel crash. There are two reasons for this, and fixing one of them is already sufficient, but both reasons deserve fixing, which is done in this patch-set.

Regards,

Joerg

Joerg Roedel (3):
iommu/amd: Move Stoney Ridge check to detect_ivrs() iommu/amd: Don't call early_amd_iommu_init() when AMD IOMMU is
disabled
iommu/amd: Keep track of amd_iommu_irq_remap state

 drivers/iommu/amd/init.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)
Signature · 6 lines
2.30.2

_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
author maintainer
#1 lore raw

PATCH 1/3 iommu/amd: Move Stoney Ridge check to detect_ivrs()

From: Joerg Roedel <[email protected]>

The AMD IOMMU will not be enabled on AMD Stoney Ridge systems. Bail out even earlier and refuse to even detect the IOMMU there.

References: https://bugzilla.kernel.org/show_bug.cgi?id=212133
References: https://bugzilla.suse.com/show_bug.cgi?id=1183132

---
 drivers/iommu/amd/init.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)
drivers/iommu/amd/init.c +12 -11
index 9126efcbaf2c..3280e6f5b720 100644
@@ -2714,7 +2714,6 @@ static int __init early_amd_iommu_init(void)
 	struct acpi_table_header *ivrs_base;
 	int i, remap_cache_sz, ret;
 	acpi_status status;
-	u32 pci_id;
 
 	if (!amd_iommu_detected)
 		return -ENODEV;
@@ -2804,16 +2803,6 @@ static int __init early_amd_iommu_init(void)
 	if (ret)
 		goto out;
 
-	/* Disable IOMMU if there's Stoney Ridge graphics */
-	for (i = 0; i < 32; i++) {
-		pci_id = read_pci_config(0, i, 0, 0);
-		if ((pci_id & 0xffff) == 0x1002 && (pci_id >> 16) == 0x98e4) {
-			pr_info("Disable IOMMU on Stoney Ridge\n");
-			amd_iommu_disabled = true;
-			break;
-		}
-	}
-
 	/* Disable any previously enabled IOMMUs */
 	if (!is_kdump_kernel() || amd_iommu_disabled)
 		disable_iommus();
@@ -2880,6 +2869,7 @@ static bool detect_ivrs(void)
 {
 	struct acpi_table_header *ivrs_base;
 	acpi_status status;
+	int i;
 
 	status = acpi_get_table("IVRS", 0, &ivrs_base);
 	if (status == AE_NOT_FOUND)
@@ -2892,6 +2882,17 @@ static bool detect_ivrs(void)
 
 	acpi_put_table(ivrs_base);
 
+	/* Don't use IOMMU if there is Stoney Ridge graphics */
+	for (i = 0; i < 32; i++) {
+		u32 pci_id;
+
+		pci_id = read_pci_config(0, i, 0, 0);
+		if ((pci_id & 0xffff) == 0x1002 && (pci_id >> 16) == 0x98e4) {
+			pr_info("Disable IOMMU on Stoney Ridge\n");
+			return false;
+		}
+	}
+
 	/* Make sure ACS will be enabled during PCI probe */
 	pci_request_acs();
 
Signature · 6 lines
2.30.2

_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
author maintainer
#2 lore raw

PATCH 2/3 iommu/amd: Don't call early_amd_iommu_init() when AMD IOMMU is disabled

In reply to Joerg Roedel

From: Joerg Roedel <[email protected]>

Don't even try to initialize the AMD IOMMU hardware when amd_iommu=off has been passed on the kernel command line.

References: https://bugzilla.kernel.org/show_bug.cgi?id=212133
References: https://bugzilla.suse.com/show_bug.cgi?id=1183132

---
 drivers/iommu/amd/init.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/iommu/amd/init.c +4 -4
index 3280e6f5b720..61dae1800b7f 100644
@@ -2919,12 +2919,12 @@ static int __init state_next(void)
 		}
 		break;
 	case IOMMU_IVRS_DETECTED:
-		ret = early_amd_iommu_init();
-		init_state = ret ? IOMMU_INIT_ERROR : IOMMU_ACPI_FINISHED;
-		if (init_state == IOMMU_ACPI_FINISHED && amd_iommu_disabled) {
-			pr_info("AMD IOMMU disabled\n");
+		if (amd_iommu_disabled) {
 			init_state = IOMMU_CMDLINE_DISABLED;
 			ret = -EINVAL;
+		} else {
+			ret = early_amd_iommu_init();
+			init_state = ret ? IOMMU_INIT_ERROR : IOMMU_ACPI_FINISHED;
 		}
 		break;
 	case IOMMU_ACPI_FINISHED:
Signature · 6 lines
2.30.2

_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
#3 lore raw

Re: [PATCH 2/3] iommu/amd: Don't call early_amd_iommu_init() when AMD IOMMU is disabled

Quoted 12 lines · Joerg Roedel
drivers/iommu/amd/init.c +0 -3
index 3280e6f5b720..61dae1800b7f 100644
@@ -2919,12 +2919,12 @@ static int __init state_next(void)
                }
                break;
        case IOMMU_IVRS_DETECTED:
-               ret = early_amd_iommu_init();
-               init_state = ret ? IOMMU_INIT_ERROR : IOMMU_ACPI_FINISHED;
-               if (init_state == IOMMU_ACPI_FINISHED && amd_iommu_disabled) {

drivers/iommu/amd/init.c static int __init state_next(void) · line 2930

Quoted 10 lines
drivers/iommu/amd/init.c +4 -1
-                       pr_info("AMD IOMMU disabled\n");
+               if (amd_iommu_disabled) {
                        init_state = IOMMU_CMDLINE_DISABLED;
                        ret = -EINVAL;
+               } else {
+                       ret = early_amd_iommu_init();
+                       init_state = ret ? IOMMU_INIT_ERROR : IOMMU_ACPI_FINISHED;
                }
                break;
        case IOMMU_ACPI_FINISHED:

If you've already moved the Stoney Ridge check out of the way, there's no real reason why you can't just set init_state=IOMMU_CMDLINE_DISABLED directly from parse_amd_iommu_options(), is there? Then you don't need the condition here at all?

_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

author maintainer
#4 lore raw

Re: [PATCH 2/3] iommu/amd: Don't call early_amd_iommu_init() when AMD IOMMU is disabled

Quoted 4 lines · David Woodhouse

If you've already moved the Stoney Ridge check out of the way, there's no real reason why you can't just set init_state=IOMMU_CMDLINE_DISABLED directly from parse_amd_iommu_options(), is there? Then you don't need the condition here at all?

True, there is even more room for optimization. The amd_iommu_disabled variable can go away entirely, including its checks in early_amd_iommu_init(). I will do a patch-set on-top of this for v5.13 which does more cleanups.

Thanks,

Joerg
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

#5 lore raw

Re: [PATCH 2/3] iommu/amd: Don't call early_amd_iommu_init() when AMD IOMMU is disabled

Quoted 5 lines · Joerg Roedel
Quoted 1 line · David Woodhouse

If you've already moved the Stoney Ridge check out of the way,

there's

Quoted 1 line

no real reason why you can't just set

init_state=IOMMU_CMDLINE_DISABLED

Quoted 8 lines
Quoted 1 line

directly from parse_amd_iommu_options(), is there? Then you don't

need

Quoted 1 line

the condition here at all?

True, there is even more room for optimization. The amd_iommu_disabled variable can go away entirely, including its checks in early_amd_iommu_init(). I will do a patch-set on-top of this for v5.13 which does more cleanups.

If we can get to the point where we don't even need to check amd_iommu_irq_remap in the ...select() function because the IRQ domain is never even registered in the case where the flag ends up false, all the better :)

Signature · 5 lines
Sent from my Android device with K-9 Mail. Please excuse my brevity.
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
author maintainer
#6 lore raw

Re: [PATCH 2/3] iommu/amd: Don't call early_amd_iommu_init() when AMD IOMMU is disabled

Quoted 4 lines · David Woodhouse

If we can get to the point where we don't even need to check amd_iommu_irq_remap in the ...select() function because the IRQ domain is never even registered in the case where the flag ends up false, all the better :)

This should already be achieved with this patch :)

But the check is still needed if something goes wrong during IOMMU initialization. In this case the IOMMUs are teared down and the memory is freed. But the IRQ domains stay registered for now, mostly because the upper-level APIs to register them lack a deregister function.

I havn't looked into the details yet whether it is suffient to call irq_domain_remove() on a domain created with arch_create_remap_msi_irq_domain() for example. This needs more research on my side :)

Regards,

Joerg
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

author maintainer
#7 lore raw

PATCH 3/3 iommu/amd: Keep track of amd_iommu_irq_remap state

In reply to Joerg Roedel

From: Joerg Roedel <[email protected]>

The amd_iommu_irq_remap variable is set to true in amd_iommu_prepare(). But if initialization fails it is not set to false. Fix that and correctly keep track of whether irq remapping is enabled or not.

References: https://bugzilla.kernel.org/show_bug.cgi?id=212133
References: https://bugzilla.suse.com/show_bug.cgi?id=1183132

Fixes b34f10c2dc59 ("iommu/amd: Stop irq_remapping_select() matching when remapping is disabled")
---
 drivers/iommu/amd/init.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/iommu/amd/init.c +4 -1
index 61dae1800b7f..321f5906e6ed 100644
@@ -3002,8 +3002,11 @@ int __init amd_iommu_prepare(void)
 	amd_iommu_irq_remap = true;
 
 	ret = iommu_go_to_state(IOMMU_ACPI_FINISHED);
-	if (ret)
+	if (ret) {
+		amd_iommu_irq_remap = false;
 		return ret;
+	}
+
 	return amd_iommu_irq_remap ? 0 : -ENODEV;
 }
 
Signature · 6 lines
2.30.2

_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
#8 lore raw
In reply to Joerg Roedel
Quoted 11 lines · Joerg Roedel

From: Joerg Roedel <[email protected]>

Hi,

it turned out that booting a kernel with amd_iommu=off on a machine that has an AMD IOMMU causes an early kernel crash. There are two reasons for this, and fixing one of them is already sufficient, but both reasons deserve fixing, which is done in this patch-set.

Regards,

Quoted 7 lines

Joerg

Joerg Roedel (3):
iommu/amd: Move Stoney Ridge check to detect_ivrs() iommu/amd: Don't call early_amd_iommu_init() when AMD IOMMU is
disabled
iommu/amd: Keep track of amd_iommu_irq_remap state

Series are Acked-by: Huang Rui <[email protected]>

Quoted 7 lines
 drivers/iommu/amd/init.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)
Signature · 1 line
2.30.2

_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu