LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 0/3] handle init errors more gracefully in amd_iommu
@ 2019-05-17  0:52 Kevin Mitchell
  2019-05-17  0:52 ` [PATCH 1/3] iommu/amd: make iommu_disable safer Kevin Mitchell
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Kevin Mitchell @ 2019-05-17  0:52 UTC (permalink / raw)
  To: Joerg Roedel; +Cc: iommu, linux-kernel, Kevin Mitchell

This series makes error handling more robust in the amd_iommu init
code. It was initially motivated by problematic firmware that does not
set up the physical address of the iommu. This led to a NULL dereference
panic when iommu_disable was called during cleanup.

While the first patch is sufficient to avoid the panic, the subsequent
two move the cleanup closer to the actual error and avoid calling the
cleanup code twice when amd_iommu=off is specified on the command line.

I have tested this series on a variety of AMD CPUs with firmware
exhibiting the issue. I have additionally tested on platforms where the
firmware has been fixed. I tried both with and without amd_iommu=off. I
have also tested on older CPUs where no IOMMU is detected and even one
where the GART driver ends up running.

Thanks,

Kevin

Kevin Mitchell (3):
  iommu/amd: make iommu_disable safer
  iommu/amd: move gart fallback to amd_iommu_init
  iommu/amd: only free resources once on init error

 drivers/iommu/amd_iommu_init.c | 45 ++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 21 deletions(-)

-- 
2.20.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/3] iommu/amd: make iommu_disable safer
  2019-05-17  0:52 [PATCH 0/3] handle init errors more gracefully in amd_iommu Kevin Mitchell
@ 2019-05-17  0:52 ` Kevin Mitchell
  2019-05-27 13:50   ` Joerg Roedel
  2019-05-17  0:52 ` [PATCH 2/3] iommu/amd: move gart fallback to amd_iommu_init Kevin Mitchell
  2019-05-17  0:52 ` [PATCH 3/3] iommu/amd: only free resources once on init error Kevin Mitchell
  2 siblings, 1 reply; 8+ messages in thread
From: Kevin Mitchell @ 2019-05-17  0:52 UTC (permalink / raw)
  To: Joerg Roedel; +Cc: iommu, linux-kernel, Kevin Mitchell

Make it safe to call iommu_disable during early init error conditions
before mmio_base is set, but after the struct amd_iommu has been added
to the amd_iommu_list. For example, this happens if firmware fails to
fill in mmio_phys in the ACPI table leading to a NULL pointer
dereference in iommu_feature_disable.

Signed-off-by: Kevin Mitchell <kevmitch@arista.com>
---
 drivers/iommu/amd_iommu_init.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index f773792d77fd..3798d7303c99 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -424,6 +424,9 @@ static void iommu_enable(struct amd_iommu *iommu)
 
 static void iommu_disable(struct amd_iommu *iommu)
 {
+	if (!iommu->mmio_base)
+		return;
+
 	/* Disable command buffer */
 	iommu_feature_disable(iommu, CONTROL_CMDBUF_EN);
 
-- 
2.20.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/3] iommu/amd: move gart fallback to amd_iommu_init
  2019-05-17  0:52 [PATCH 0/3] handle init errors more gracefully in amd_iommu Kevin Mitchell
  2019-05-17  0:52 ` [PATCH 1/3] iommu/amd: make iommu_disable safer Kevin Mitchell
@ 2019-05-17  0:52 ` Kevin Mitchell
  2019-05-17  0:52 ` [PATCH 3/3] iommu/amd: only free resources once on init error Kevin Mitchell
  2 siblings, 0 replies; 8+ messages in thread
From: Kevin Mitchell @ 2019-05-17  0:52 UTC (permalink / raw)
  To: Joerg Roedel; +Cc: iommu, linux-kernel, Kevin Mitchell

The fallback to the GART driver in the case amd_iommu doesn't work was
executed in a function called free_iommu_resources, which didn't really
make sense. This was even being called twice if amd_iommu=off was
specified on the command line.

The only complication is that it needs to be verified that amd_iommu has
fully relinquished control by calling free_iommu_resources and emptying
the amd_iommu_list.

Signed-off-by: Kevin Mitchell <kevmitch@arista.com>
---
 drivers/iommu/amd_iommu_init.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 3798d7303c99..5f3df5ae6ba8 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -2345,15 +2345,6 @@ static void __init free_iommu_resources(void)
 	amd_iommu_dev_table = NULL;
 
 	free_iommu_all();
-
-#ifdef CONFIG_GART_IOMMU
-	/*
-	 * We failed to initialize the AMD IOMMU - try fallback to GART
-	 * if possible.
-	 */
-	gart_iommu_init();
-
-#endif
 }
 
 /* SB IOAPIC is always on this device in AMD systems */
@@ -2774,6 +2765,16 @@ static int __init amd_iommu_init(void)
 		}
 	}
 
+#ifdef CONFIG_GART_IOMMU
+	if (ret && list_empty(&amd_iommu_list)) {
+		/*
+		 * We failed to initialize the AMD IOMMU - try fallback
+		 * to GART if possible.
+		 */
+		gart_iommu_init();
+	}
+#endif
+
 	for_each_iommu(iommu)
 		amd_iommu_debugfs_setup(iommu);
 
-- 
2.20.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 3/3] iommu/amd: only free resources once on init error
  2019-05-17  0:52 [PATCH 0/3] handle init errors more gracefully in amd_iommu Kevin Mitchell
  2019-05-17  0:52 ` [PATCH 1/3] iommu/amd: make iommu_disable safer Kevin Mitchell
  2019-05-17  0:52 ` [PATCH 2/3] iommu/amd: move gart fallback to amd_iommu_init Kevin Mitchell
@ 2019-05-17  0:52 ` Kevin Mitchell
  2 siblings, 0 replies; 8+ messages in thread
From: Kevin Mitchell @ 2019-05-17  0:52 UTC (permalink / raw)
  To: Joerg Roedel; +Cc: iommu, linux-kernel, Kevin Mitchell

When amd_iommu=off was specified on the command line, free_X_resources
functions were called immediately after early_amd_iommu_init. They were
then called again when amd_iommu_init also failed (as expected).

Instead, call them only once: at the end of state_next() whenever
there's an error. These functions should be safe to call any time and
any number of times. However, since state_next is never called again in
an error state, the cleanup will only ever be run once.

This also ensures that cleanup code is run as soon as possible after an
error is detected rather than waiting for amd_iommu_init() to be called.

Signed-off-by: Kevin Mitchell <kevmitch@arista.com>
---
 drivers/iommu/amd_iommu_init.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 5f3df5ae6ba8..24fc060fe596 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -2638,8 +2638,6 @@ static int __init state_next(void)
 		init_state = ret ? IOMMU_INIT_ERROR : IOMMU_ACPI_FINISHED;
 		if (init_state == IOMMU_ACPI_FINISHED && amd_iommu_disabled) {
 			pr_info("AMD IOMMU disabled on kernel command-line\n");
-			free_dma_resources();
-			free_iommu_resources();
 			init_state = IOMMU_CMDLINE_DISABLED;
 			ret = -EINVAL;
 		}
@@ -2680,6 +2678,19 @@ static int __init state_next(void)
 		BUG();
 	}
 
+	if (ret) {
+		free_dma_resources();
+		if (!irq_remapping_enabled) {
+			disable_iommus();
+			free_iommu_resources();
+		} else {
+			struct amd_iommu *iommu;
+
+			uninit_device_table_dma();
+			for_each_iommu(iommu)
+				iommu_flush_all_caches(iommu);
+		}
+	}
 	return ret;
 }
 
@@ -2753,18 +2764,6 @@ static int __init amd_iommu_init(void)
 	int ret;
 
 	ret = iommu_go_to_state(IOMMU_INITIALIZED);
-	if (ret) {
-		free_dma_resources();
-		if (!irq_remapping_enabled) {
-			disable_iommus();
-			free_iommu_resources();
-		} else {
-			uninit_device_table_dma();
-			for_each_iommu(iommu)
-				iommu_flush_all_caches(iommu);
-		}
-	}
-
 #ifdef CONFIG_GART_IOMMU
 	if (ret && list_empty(&amd_iommu_list)) {
 		/*
-- 
2.20.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] iommu/amd: make iommu_disable safer
  2019-05-17  0:52 ` [PATCH 1/3] iommu/amd: make iommu_disable safer Kevin Mitchell
@ 2019-05-27 13:50   ` Joerg Roedel
  0 siblings, 0 replies; 8+ messages in thread
From: Joerg Roedel @ 2019-05-27 13:50 UTC (permalink / raw)
  To: Kevin Mitchell; +Cc: iommu, linux-kernel

Hi Kevin,

On Thu, May 16, 2019 at 05:52:40PM -0700, Kevin Mitchell wrote:
> Signed-off-by: Kevin Mitchell <kevmitch@arista.com>

somehow I didn't receive the cover letter and patch 3 of this set, can
you please re-send so I have it all for review?

Regards,

	Joerg

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/3] handle init errors more gracefully in amd_iommu
  2019-06-12 21:52 Kevin Mitchell
@ 2019-07-01 11:54 ` Joerg Roedel
  0 siblings, 0 replies; 8+ messages in thread
From: Joerg Roedel @ 2019-07-01 11:54 UTC (permalink / raw)
  To: Kevin Mitchell; +Cc: iommu, linux-kernel

Hi Kevin,

On Wed, Jun 12, 2019 at 02:52:01PM -0700, Kevin Mitchell wrote:
> I have tested this series on a variety of AMD CPUs with firmware
> exhibiting the issue. I have additionally tested on platforms where the
> firmware has been fixed. I tried both with and without amd_iommu=off. I
> have also tested on older CPUs where no IOMMU is detected and even one
> where the GART driver ends up running.

Thanks for the fixes, applied them all.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 0/3] handle init errors more gracefully in amd_iommu
@ 2019-06-12 21:52 Kevin Mitchell
  2019-07-01 11:54 ` Joerg Roedel
  0 siblings, 1 reply; 8+ messages in thread
From: Kevin Mitchell @ 2019-06-12 21:52 UTC (permalink / raw)
  To: Joerg Roedel; +Cc: iommu, linux-kernel, Kevin Mitchell

This series makes error handling more robust in the amd_iommu init
code. It was initially motivated by problematic firmware that does not
set up the physical address of the iommu. This led to a NULL dereference
panic when iommu_disable was called during cleanup.

While the first patch is sufficient to avoid the panic, the subsequent
two move the cleanup closer to the actual error and avoid calling the
cleanup code twice when amd_iommu=off is specified on the command line.

I have tested this series on a variety of AMD CPUs with firmware
exhibiting the issue. I have additionally tested on platforms where the
firmware has been fixed. I tried both with and without amd_iommu=off. I
have also tested on older CPUs where no IOMMU is detected and even one
where the GART driver ends up running.

Thanks,

Kevin

Kevin Mitchell (3):
  iommu/amd: make iommu_disable safer
  iommu/amd: move gart fallback to amd_iommu_init
  iommu/amd: only free resources once on init error

 drivers/iommu/amd_iommu_init.c | 45 ++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 21 deletions(-)

-- 
2.20.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 0/3] handle init errors more gracefully in amd_iommu
@ 2019-05-28 18:29 Kevin Mitchell
  0 siblings, 0 replies; 8+ messages in thread
From: Kevin Mitchell @ 2019-05-28 18:29 UTC (permalink / raw)
  To: Joerg Roedel; +Cc: iommu, linux-kernel, Kevin Mitchell

This series makes error handling more robust in the amd_iommu init
code. It was initially motivated by problematic firmware that does not
set up the physical address of the iommu. This led to a NULL dereference
panic when iommu_disable was called during cleanup.

While the first patch is sufficient to avoid the panic, the subsequent
two move the cleanup closer to the actual error and avoid calling the
cleanup code twice when amd_iommu=off is specified on the command line.

I have tested this series on a variety of AMD CPUs with firmware
exhibiting the issue. I have additionally tested on platforms where the
firmware has been fixed. I tried both with and without amd_iommu=off. I
have also tested on older CPUs where no IOMMU is detected and even one
where the GART driver ends up running.

Thanks,

Kevin

Kevin Mitchell (3):
  iommu/amd: make iommu_disable safer
  iommu/amd: move gart fallback to amd_iommu_init
  iommu/amd: only free resources once on init error

 drivers/iommu/amd_iommu_init.c | 45 ++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 21 deletions(-)

-- 
2.20.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2019-07-01 11:54 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-17  0:52 [PATCH 0/3] handle init errors more gracefully in amd_iommu Kevin Mitchell
2019-05-17  0:52 ` [PATCH 1/3] iommu/amd: make iommu_disable safer Kevin Mitchell
2019-05-27 13:50   ` Joerg Roedel
2019-05-17  0:52 ` [PATCH 2/3] iommu/amd: move gart fallback to amd_iommu_init Kevin Mitchell
2019-05-17  0:52 ` [PATCH 3/3] iommu/amd: only free resources once on init error Kevin Mitchell
2019-05-28 18:29 [PATCH 0/3] handle init errors more gracefully in amd_iommu Kevin Mitchell
2019-06-12 21:52 Kevin Mitchell
2019-07-01 11:54 ` Joerg Roedel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).