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(-)
--
2.30.2
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
Cc: [email protected] # v5.11
Signed-off-by: Joerg Roedel <[email protected]>
---
drivers/iommu/amd/init.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 3280e6f5b720..61dae1800b7f 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -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:
--
2.30.2
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
Cc: [email protected] # v5.11
Signed-off-by: Joerg Roedel <[email protected]>
---
drivers/iommu/amd/init.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 9126efcbaf2c..3280e6f5b720 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -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();
--
2.30.2
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")
Cc: [email protected] # v5.11
Signed-off-by: Joerg Roedel <[email protected]>
---
drivers/iommu/amd/init.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 61dae1800b7f..321f5906e6ed 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -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;
}
--
2.30.2
On Wed, Mar 17, 2021 at 05:10:34PM +0800, Joerg Roedel wrote:
> 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
Series are Acked-by: Huang Rui <[email protected]>
>
> drivers/iommu/amd/init.c | 36 ++++++++++++++++++++----------------
> 1 file changed, 20 insertions(+), 16 deletions(-)
>
> --
> 2.30.2
>
On Wed, 2021-03-17 at 10:10 +0100, Joerg Roedel wrote:
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 3280e6f5b720..61dae1800b7f 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -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:
> --
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?
On Wed, Mar 17, 2021 at 01:37:16PM +0000, David Woodhouse wrote:
> 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
On Wed, Mar 17, 2021 at 11:47:11AM +0000, David Woodhouse wrote:
> 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
On 17 March 2021 13:32:35 GMT, Joerg Roedel <[email protected]> wrote:
>On Wed, Mar 17, 2021 at 11:47:11AM +0000, David Woodhouse wrote:
>> 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.
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 :)
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
On Wed, Mar 17, 2021 at 06:48:50PM +0800, Huang Rui wrote:
> Series are Acked-by: Huang Rui <[email protected]>
Thanks, series is applied for v5.12