[PATCH v1 1/6] iommu/dmar: Fix crash on boot when DMAR is disabled

[PATCH v1 1/6] iommu/dmar: Fix crash on boot when DMAR is disabled

[Andy Shevchenko]

By default CONFIG_INTEL_IOMMU_DEFAULT_ON is not set and thus
dmar_disabled variable is set.

Intel IOMMU driver based on above doesn't set intel_iommu_enabled
variable.

The commit b0119e870837 ("iommu: Introduce new 'struct iommu_device'")
mistakenly assumes it never happens and tries to unregister not ever
registered resources, which crashes the kernel at boot time:

	BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
	IP: iommu_device_unregister+0x31/0x60

Make unregister procedure conditional in free_iommu().

Fixes: b0119e870837 ("iommu: Introduce new 'struct iommu_device'")
Cc: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/iommu/dmar.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index d9c0decfc91a..36e3f430d265 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -1108,8 +1108,10 @@ static int alloc_iommu(struct dmar_drhd_unit *drhd)
 
 static void free_iommu(struct intel_iommu *iommu)
 {
-	iommu_device_sysfs_remove(&iommu->iommu);
-	iommu_device_unregister(&iommu->iommu);
+	if (intel_iommu_enabled) {
+		iommu_device_unregister(&iommu->iommu);
+		iommu_device_sysfs_remove(&iommu->iommu);
+	}
 
 	if (iommu->irq) {
 		if (iommu->pr_irq) {
-- 
2.11.0

[PATCH v1 2/6] iommu/dmar: Rectify return code handling in detect_intel_iommu()

[Andy Shevchenko]

There is inconsistency in return codes across the functions called from
detect_intel_iommu().

Make it consistent and propagate return code to the caller.

Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/iommu/dmar.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 36e3f430d265..322765a0a210 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -551,7 +551,7 @@ static int __init dmar_table_detect(void)
 		status = AE_NOT_FOUND;
 	}
 
-	return (ACPI_SUCCESS(status) ? 1 : 0);
+	return (ACPI_SUCCESS(status) ? 0 : -ENOENT);
 }
 
 static int dmar_walk_remapping_entries(struct acpi_dmar_header *start,
@@ -891,17 +891,17 @@ int __init detect_intel_iommu(void)
 
 	down_write(&dmar_global_lock);
 	ret = dmar_table_detect();
-	if (ret)
-		ret = !dmar_walk_dmar_table((struct acpi_table_dmar *)dmar_tbl,
-					    &validate_drhd_cb);
-	if (ret && !no_iommu && !iommu_detected && !dmar_disabled) {
+	if (!ret)
+		ret = dmar_walk_dmar_table((struct acpi_table_dmar *)dmar_tbl,
+					   &validate_drhd_cb);
+	if (!ret && !no_iommu && !iommu_detected && !dmar_disabled) {
 		iommu_detected = 1;
 		/* Make sure ACS will be enabled */
 		pci_request_acs();
 	}
 
 #ifdef CONFIG_X86
-	if (ret)
+	if (!ret)
 		x86_init.iommu.iommu_init = intel_iommu_init;
 #endif
 
@@ -911,10 +911,9 @@ int __init detect_intel_iommu(void)
 	}
 	up_write(&dmar_global_lock);
 
-	return ret ? 1 : -ENODEV;
+	return ret;
 }
 
-
 static void unmap_iommu(struct intel_iommu *iommu)
 {
 	iounmap(iommu->reg);
-- 
2.11.0

Re: [PATCH v1 2/6] iommu/dmar: Rectify return code handling in detect_intel_iommu()

[Andy Shevchenko]

On Wed, 2017-02-15 at 16:42 +0200, Andy Shevchenko wrote:
> There is inconsistency in return codes across the functions called
> from
> detect_intel_iommu().
> 
> Make it consistent and propagate return code to the caller.

 
> @@ -911,10 +911,9 @@ int __init detect_intel_iommu(void)
>  	}
>  	up_write(&dmar_global_lock);
>  
> -	return ret ? 1 : -ENODEV;
> +	return ret;

Accidentally it works since we have no late_init() defined.

I will fix this in v2, but will wait for the comments regarding the rest
of the series.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v1 3/6] iommu/dmar: Return directly from a loop in dmar_dev_scope_status()

[Andy Shevchenko]

There is no need to have a temporary variable.

Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/iommu/dmar.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 322765a0a210..24bb0fcca860 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -557,11 +557,10 @@ static int __init dmar_table_detect(void)
 static int dmar_walk_remapping_entries(struct acpi_dmar_header *start,
 				       size_t len, struct dmar_res_callback *cb)
 {
-	int ret = 0;
 	struct acpi_dmar_header *iter, *next;
 	struct acpi_dmar_header *end = ((void *)start) + len;
 
-	for (iter = start; iter < end && ret == 0; iter = next) {
+	for (iter = start; iter < end; iter = next) {
 		next = (void *)iter + iter->length;
 		if (iter->length == 0) {
 			/* Avoid looping forever on bad ACPI tables */
@@ -570,8 +569,7 @@ static int dmar_walk_remapping_entries(struct acpi_dmar_header *start,
 		} else if (next > end) {
 			/* Avoid passing table end */
 			pr_warn(FW_BUG "Record passes table end\n");
-			ret = -EINVAL;
-			break;
+			return -EINVAL;
 		}
 
 		if (cb->print_entry)
@@ -582,15 +580,19 @@ static int dmar_walk_remapping_entries(struct acpi_dmar_header *start,
 			pr_debug("Unknown DMAR structure type %d\n",
 				 iter->type);
 		} else if (cb->cb[iter->type]) {
+			int ret;
+
 			ret = cb->cb[iter->type](iter, cb->arg[iter->type]);
+			if (ret)
+				return ret;
 		} else if (!cb->ignore_unhandled) {
 			pr_warn("No handler for DMAR structure type %d\n",
 				iter->type);
-			ret = -EINVAL;
+			return -EINVAL;
 		}
 	}
 
-	return ret;
+	return 0;
 }
 
 static inline int dmar_walk_dmar_table(struct acpi_table_dmar *dmar,
-- 
2.11.0

[PATCH v1 4/6] iommu/dmar: Remove redundant assignment of ret

[Andy Shevchenko]

There is no need to assign ret to 0 in some cases. Moreover it might
shadow some errors in the future.

Remove such assignments.

Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/iommu/dmar.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 24bb0fcca860..47b4044a5219 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -391,7 +391,7 @@ static int dmar_parse_one_drhd(struct acpi_dmar_header *header, void *arg)
 {
 	struct acpi_dmar_hardware_unit *drhd;
 	struct dmar_drhd_unit *dmaru;
-	int ret = 0;
+	int ret;
 
 	drhd = (struct acpi_dmar_hardware_unit *)header;
 	dmaru = dmar_find_dmaru(drhd);
@@ -609,8 +609,8 @@ static int __init
 parse_dmar_table(void)
 {
 	struct acpi_table_dmar *dmar;
-	int ret = 0;
 	int drhd_count = 0;
+	int ret;
 	struct dmar_res_callback cb = {
 		.print_entry = true,
 		.ignore_unhandled = true,
-- 
2.11.0

[PATCH v1 5/6] iommu/dmar: Remove redundant ' != 0' when check return code

[Andy Shevchenko]

Usual patter when we check for return code, which might be negative
errno, is either ret or !ret.

Remove extra ' != 0' from condition.

Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/iommu/dmar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 47b4044a5219..685e3a2d7e5f 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -311,7 +311,7 @@ static int dmar_pci_bus_add_dev(struct dmar_pci_notify_info *info)
 				((void *)drhd) + drhd->header.length,
 				dmaru->segment,
 				dmaru->devices, dmaru->devices_cnt);
-		if (ret != 0)
+		if (ret)
 			break;
 	}
 	if (ret >= 0)
-- 
2.11.0

[PATCH v1 6/6] iommu/vt-d: Use lo_hi_readq() / lo_hi_writeq()

[Andy Shevchenko]

There is already helper functions to do 64-bit I/O on 32-bit machines or
buses, thus we don't need to reinvent the wheel.

Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 include/linux/intel-iommu.h | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index c573a52ae440..485a5b48f038 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -30,6 +30,8 @@
 #include <linux/mmu_notifier.h>
 #include <linux/list.h>
 #include <linux/iommu.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
+
 #include <asm/cacheflush.h>
 #include <asm/iommu.h>
 
@@ -72,24 +74,8 @@
 
 #define OFFSET_STRIDE		(9)
 
-#ifdef CONFIG_64BIT
 #define dmar_readq(a) readq(a)
 #define dmar_writeq(a,v) writeq(v,a)
-#else
-static inline u64 dmar_readq(void __iomem *addr)
-{
-	u32 lo, hi;
-	lo = readl(addr);
-	hi = readl(addr + 4);
-	return (((u64) hi) << 32) + lo;
-}
-
-static inline void dmar_writeq(void __iomem *addr, u64 val)
-{
-	writel((u32)val, addr);
-	writel((u32)(val >> 32), addr + 4);
-}
-#endif
 
 #define DMAR_VER_MAJOR(v)		(((v) & 0xf0) >> 4)
 #define DMAR_VER_MINOR(v)		((v) & 0x0f)
-- 
2.11.0

Re: [PATCH v1 1/6] iommu/dmar: Fix crash on boot when DMAR is disabled

[Andy Shevchenko]

On Wed, 2017-02-15 at 16:42 +0200, Andy Shevchenko wrote:
> By default CONFIG_INTEL_IOMMU_DEFAULT_ON is not set and thus
> dmar_disabled variable is set.
> 
> Intel IOMMU driver based on above doesn't set intel_iommu_enabled
> variable.
> 
> The commit b0119e870837 ("iommu: Introduce new 'struct iommu_device'")
> mistakenly assumes it never happens and tries to unregister not ever
> registered resources, which crashes the kernel at boot time:
> 
> 	BUG: unable to handle kernel NULL pointer dereference at
> 0000000000000008
> 	IP: iommu_device_unregister+0x31/0x60
> 
> Make unregister procedure conditional in free_iommu().

Joerg, it looks like bug is still present. Can we get the fix merged?
Or do you have better ideas?

> 
> Fixes: b0119e870837 ("iommu: Introduce new 'struct iommu_device'")
> Cc: Joerg Roedel <jroedel@suse.de>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/iommu/dmar.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
> index d9c0decfc91a..36e3f430d265 100644
> --- a/drivers/iommu/dmar.c
> +++ b/drivers/iommu/dmar.c
> @@ -1108,8 +1108,10 @@ static int alloc_iommu(struct dmar_drhd_unit
> *drhd)
>  
>  static void free_iommu(struct intel_iommu *iommu)
>  {
> -	iommu_device_sysfs_remove(&iommu->iommu);
> -	iommu_device_unregister(&iommu->iommu);
> +	if (intel_iommu_enabled) {
> +		iommu_device_unregister(&iommu->iommu);
> +		iommu_device_sysfs_remove(&iommu->iommu);
> +	}
>  
>  	if (iommu->irq) {
>  		if (iommu->pr_irq) {

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v1 1/6] iommu/dmar: Fix crash on boot when DMAR is disabled

[Joerg Roedel]

Hi Andy,

On Wed, Feb 15, 2017 at 04:42:21PM +0200, Andy Shevchenko wrote:
> By default CONFIG_INTEL_IOMMU_DEFAULT_ON is not set and thus
> dmar_disabled variable is set.
> 
> Intel IOMMU driver based on above doesn't set intel_iommu_enabled
> variable.
> 
> The commit b0119e870837 ("iommu: Introduce new 'struct iommu_device'")
> mistakenly assumes it never happens and tries to unregister not ever
> registered resources, which crashes the kernel at boot time:
> 
> 	BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
> 	IP: iommu_device_unregister+0x31/0x60
> 
> Make unregister procedure conditional in free_iommu().
> 
> Fixes: b0119e870837 ("iommu: Introduce new 'struct iommu_device'")
> Cc: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Thanks for the patch, I applied it and will send it upstream asap.



	Joerg

Re: [PATCH v1 1/6] iommu/dmar: Fix crash on boot when DMAR is disabled

[Andy Shevchenko]

On Wed, 2017-02-22 at 12:26 +0100, Joerg Roedel wrote:
> Hi Andy,
> 
> On Wed, Feb 15, 2017 at 04:42:21PM +0200, Andy Shevchenko wrote:
> > By default CONFIG_INTEL_IOMMU_DEFAULT_ON is not set and thus
> > dmar_disabled variable is set.
> > 
> > Intel IOMMU driver based on above doesn't set intel_iommu_enabled
> > variable.
> > 
> > The commit b0119e870837 ("iommu: Introduce new 'struct
> > iommu_device'")
> > mistakenly assumes it never happens and tries to unregister not ever
> > registered resources, which crashes the kernel at boot time:
> > 
> > 	BUG: unable to handle kernel NULL pointer dereference at
> > 0000000000000008
> > 	IP: iommu_device_unregister+0x31/0x60
> > 
> > Make unregister procedure conditional in free_iommu().
> > 
> > Fixes: b0119e870837 ("iommu: Introduce new 'struct iommu_device'")
> > Cc: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> 
> Thanks for the patch, I applied it and will send it upstream asap.

Thanks! Any comments on the rest? I would like to send v2.

-- 
Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Intel Finland Oy