[PATCH 0/6] AMD IOMMU updates

[PATCH 0/6] AMD IOMMU updates

[Joerg Roedel]

Hi,

this series of patches contain some updates to the AMD IOMMU code currently in
tip/master. The updates address some objections discussed on the initial
patches and also contain optimization and small code cleanup.
The code with these updates is tested on real hardware under load and showed no
problems.

diffstat:

 arch/x86/Kconfig                  |   10 +++++++++-
 arch/x86/kernel/amd_iommu.c       |   15 ++++++++++-----
 arch/x86/kernel/amd_iommu_init.c  |   16 ++++++----------
 include/asm-x86/amd_iommu_types.h |    2 ++
 4 files changed, 27 insertions(+), 16 deletions(-)

[PATCH 1/6] AMD IOMMU: more verbose Kconfig description text

[Joerg Roedel]

This patch replaces the short description text for AMD IOMMU in Kconfig with a
more verbose one.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/Kconfig |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index dc6425e..c31047a 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -568,7 +568,15 @@ config AMD_IOMMU
 	select SWIOTLB
 	depends on X86_64 && PCI && ACPI
 	help
-	  Select this to get support for AMD IOMMU hardware in your system.
+	  With this option you can enable support for AMD IOMMU hardware in
+	  your system. An IOMMU is a hardware component which provides
+	  remapping of DMA memory accesses from devices. With an AMD IOMMU you
+	  can isolate the the DMA memory of different devices and protect the
+	  system from misbehaving device drivers or hardware.
+
+	  You can find out if your system has an AMD IOMMU if you look into
+	  your BIOS for an option to enable it or if you have an IVRS ACPI
+	  table.
 
 # need this always selected by IOMMU for the VIA workaround
 config SWIOTLB
-- 
1.5.3.7

[PATCH 2/6] AMD IOMMU: remove unnecessary set_bit_string

[Joerg Roedel]

The set_bit_string call in the address allocator is not necessary because its
already called in iommu_area_alloc().

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kernel/amd_iommu.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
index a1b3856..329b2c3 100644
--- a/arch/x86/kernel/amd_iommu.c
+++ b/arch/x86/kernel/amd_iommu.c
@@ -301,7 +301,6 @@ static unsigned long dma_ops_alloc_addresses(struct device *dev,
 				0, boundary_size, 0);
 
 	if (likely(address != -1)) {
-		set_bit_string(dom->bitmap, address, pages);
 		dom->next_bit = address + pages;
 		address <<= PAGE_SHIFT;
 	} else
-- 
1.5.3.7

[PATCH 3/6] AMD IOMMU: flush domain TLB when there is more than one page to flush

[Joerg Roedel]

This patch changes the domain TLB flushing behavior of the driver. When there
is more than one page to flush it flushes the whole domain TLB instead of every
single page. So we send only a single command to the IOMMU in every case which
is faster to execute.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kernel/amd_iommu.c       |   14 ++++++++++----
 include/asm-x86/amd_iommu_types.h |    2 ++
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
index 329b2c3..f2766d8 100644
--- a/arch/x86/kernel/amd_iommu.c
+++ b/arch/x86/kernel/amd_iommu.c
@@ -140,16 +140,22 @@ static int iommu_queue_inv_iommu_pages(struct amd_iommu *iommu,
 static int iommu_flush_pages(struct amd_iommu *iommu, u16 domid,
 		u64 address, size_t size)
 {
-	int i;
+	int s = 0;
 	unsigned pages = to_pages(address, size);
 
 	address &= PAGE_MASK;
 
-	for (i = 0; i < pages; ++i) {
-		iommu_queue_inv_iommu_pages(iommu, address, domid, 0, 0);
-		address += PAGE_SIZE;
+	if (pages > 1) {
+		/*
+		 * If we have to flush more than one page, flush all
+		 * TLB entries for this domain
+		 */
+		address = CMD_INV_IOMMU_ALL_PAGES_ADDRESS;
+		s = 1;
 	}
 
+	iommu_queue_inv_iommu_pages(iommu, address, domid, 0, s);
+
 	return 0;
 }
 
diff --git a/include/asm-x86/amd_iommu_types.h b/include/asm-x86/amd_iommu_types.h
index 0f39550..7bfcb47 100644
--- a/include/asm-x86/amd_iommu_types.h
+++ b/include/asm-x86/amd_iommu_types.h
@@ -93,6 +93,8 @@
 #define CMD_INV_IOMMU_PAGES_SIZE_MASK	0x01
 #define CMD_INV_IOMMU_PAGES_PDE_MASK	0x02
 
+#define CMD_INV_IOMMU_ALL_PAGES_ADDRESS	0x7fffffffffffffffULL
+
 /* macros and definitions for device table entries */
 #define DEV_ENTRY_VALID         0x00
 #define DEV_ENTRY_TRANSLATION   0x01
-- 
1.5.3.7

RE: [PATCH 3/6] AMD IOMMU: flush domain TLB when there is more than onepage to flush

[Duran, Leo]

On Thursday, July 03, 2008 12:35 PM, Joerg Roedel wrote:

> This patch changes the domain TLB flushing behavior of the driver.
When
> there
> is more than one page to flush it flushes the whole domain TLB instead
> of every
> single page. So we send only a single command to the IOMMU in every
> case which
> is faster to execute.
> 

Joerg,

Instead of flushing the complete TLB, how about just setting the
page-size encoding bits so as to cover all of the pages (rounded up to a
power of 2... which in some cases may flush a few more pages, but not
the complete TLB). This way other devices don't pay the 'flushing'
penalty every time you unmap a buffer for a given device.

Leo.

[PATCH 4/6] AMD IOMMU: honor iommu=off instead of amd_iommu=off

[Joerg Roedel]

This patch removes the amd_iommu=off kernel parameter and honors the generic
iommu=off parameter for the same purpose.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kernel/amd_iommu_init.c |    9 +--------
 1 files changed, 1 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/amd_iommu_init.c b/arch/x86/kernel/amd_iommu_init.c
index 5d9e45c..c54c823 100644
--- a/arch/x86/kernel/amd_iommu_init.c
+++ b/arch/x86/kernel/amd_iommu_init.c
@@ -101,8 +101,6 @@ struct ivmd_header {
 	u64 range_length;
 } __attribute__((packed));
 
-static int __initdata amd_iommu_disable;
-
 u16 amd_iommu_last_bdf;
 struct list_head amd_iommu_unity_map;
 unsigned amd_iommu_aperture_order = 26;
@@ -686,7 +684,7 @@ int __init amd_iommu_init(void)
 	int i, ret = 0;
 
 
-	if (amd_iommu_disable) {
+	if (no_iommu) {
 		printk(KERN_INFO "AMD IOMMU disabled by kernel command line\n");
 		return 0;
 	}
@@ -831,9 +829,6 @@ void __init amd_iommu_detect(void)
 	if (swiotlb || no_iommu || iommu_detected)
 		return;
 
-	if (amd_iommu_disable)
-		return;
-
 	if (acpi_table_parse("IVRS", early_amd_iommu_detect) == 0) {
 		iommu_detected = 1;
 #ifdef CONFIG_GART_IOMMU
@@ -846,8 +841,6 @@ void __init amd_iommu_detect(void)
 static int __init parse_amd_iommu_options(char *str)
 {
 	for (; *str; ++str) {
-		if (strcmp(str, "off") == 0)
-			amd_iommu_disable = 1;
 		if (strcmp(str, "isolate") == 0)
 			amd_iommu_isolate = 1;
 	}
-- 
1.5.3.7

[PATCH 5/6] AMD IOMMU: don't try to init IOMMU if early detect code did not detect one

[Joerg Roedel]

This patch adds a check if the early detect code has found AMD IOMMU hardware
descriptions and does not try to initialize hardware if the check failed.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kernel/amd_iommu_init.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/amd_iommu_init.c b/arch/x86/kernel/amd_iommu_init.c
index c54c823..c970fac 100644
--- a/arch/x86/kernel/amd_iommu_init.c
+++ b/arch/x86/kernel/amd_iommu_init.c
@@ -101,6 +101,8 @@ struct ivmd_header {
 	u64 range_length;
 } __attribute__((packed));
 
+static int __initdata amd_iommu_detected;
+
 u16 amd_iommu_last_bdf;
 struct list_head amd_iommu_unity_map;
 unsigned amd_iommu_aperture_order = 26;
@@ -689,6 +691,9 @@ int __init amd_iommu_init(void)
 		return 0;
 	}
 
+	if (!amd_iommu_detected)
+		return -ENODEV;
+
 	/*
 	 * First parse ACPI tables to find the largest Bus/Dev/Func
 	 * we need to handle. Upon this information the shared data
@@ -831,6 +836,7 @@ void __init amd_iommu_detect(void)
 
 	if (acpi_table_parse("IVRS", early_amd_iommu_detect) == 0) {
 		iommu_detected = 1;
+		amd_iommu_detected = 1;
 #ifdef CONFIG_GART_IOMMU
 		gart_iommu_aperture_disabled = 1;
 		gart_iommu_aperture = 0;
-- 
1.5.3.7

[PATCH 6/6] AMD IOMMU: remove unnecessary code from the iommu_enable function

[Joerg Roedel]

This code removes a leftover from the iommu_enable function. The ctrl variable
is assigned but never used.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kernel/amd_iommu_init.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/amd_iommu_init.c b/arch/x86/kernel/amd_iommu_init.c
index c970fac..2a13e43 100644
--- a/arch/x86/kernel/amd_iommu_init.c
+++ b/arch/x86/kernel/amd_iommu_init.c
@@ -169,14 +169,11 @@ static void __init iommu_feature_disable(struct amd_iommu *iommu, u8 bit)
 
 void __init iommu_enable(struct amd_iommu *iommu)
 {
-	u32 ctrl;
-
 	printk(KERN_INFO "AMD IOMMU: Enabling IOMMU at ");
 	print_devid(iommu->devid, 0);
 	printk(" cap 0x%hx\n", iommu->cap_ptr);
 
 	iommu_feature_enable(iommu, CONTROL_IOMMU_EN);
-	ctrl = readl(iommu->mmio_base + MMIO_CONTROL_OFFSET);
 }
 
 static u8 * __init iommu_map_mmio_space(u64 address)
-- 
1.5.3.7

Re: [PATCH 0/6] AMD IOMMU updates

[Ingo Molnar]

* Joerg Roedel <joerg.roedel@amd.com> wrote:

> Hi,
> 
> this series of patches contain some updates to the AMD IOMMU code 
> currently in tip/master. The updates address some objections discussed 
> on the initial patches and also contain optimization and small code 
> cleanup. The code with these updates is tested on real hardware under 
> load and showed no problems.

applied to tip/x86/amd-iommu and pushed that topic out, thanks Joerg.

also, i've integrated it into tip/master as well (will push that out
after some testing).

The IOMMU driver code seems to be getting into good shape, so i've also 
added the topic to auto-x86-next, which means it will go into linux-next 
on the next iteration (in 1-2 days). Does anyone see any remaining 
problems?

	Ingo

Re: [PATCH 0/6] AMD IOMMU updates

[FUJITA Tomonori]

On Fri, 4 Jul 2008 11:52:13 +0200
Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Joerg Roedel <joerg.roedel@amd.com> wrote:
> 
> > Hi,
> > 
> > this series of patches contain some updates to the AMD IOMMU code 
> > currently in tip/master. The updates address some objections discussed 
> > on the initial patches and also contain optimization and small code 
> > cleanup. The code with these updates is tested on real hardware under 
> > load and showed no problems.
> 
> applied to tip/x86/amd-iommu and pushed that topic out, thanks Joerg.
> 
> also, i've integrated it into tip/master as well (will push that out
> after some testing).
> 
> The IOMMU driver code seems to be getting into good shape, so i've also 
> added the topic to auto-x86-next, which means it will go into linux-next 
> on the next iteration (in 1-2 days). Does anyone see any remaining 
> problems?

Not a showstopper but I like to see the cleanups of the kernel
parameters, as I said before. The AMD IOMMU has two parameters,
amd_iommu=isolate and amd_iommu_size=32M|64M|128M|256M|512M|1G

The former might be useful for the VT-d. The latter is useful for most
of the IOMMU drivers. So I think that it would be better to make them
common parameters.

And if we can change the existing kernel parameters, I think that it
would be better to convert some of the x86 IOMMU parameters to common
parameters.


BTW, "[PATCH 4/6] AMD IOMMU: honor iommu=off instead of amd_iommu=off"
patch in auto-x86-next needs update the kernel parameter.

=
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Subject: [PATCH] AMD IOMMU: fix kernel parameter

amd_iommu=off was replaced with a common parameter, iommu=off.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 Documentation/kernel-parameters.txt |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index cd98762..1a6bb46 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -274,7 +274,6 @@ and is between 256 and 4096 characters. It is defined in the file
 	amd_iommu=	[HW,X86-84]
 			Pass parameters to the AMD IOMMU driver in the system.
 			Possible values are:
-			off     - disable the driver for AMD IOMMU
 			isolate - enable device isolation (each device, as far
 			          as possible, will get its own protection
 			          domain)
-- 
1.5.5.GIT

Re: [PATCH 0/6] AMD IOMMU updates

[Ingo Molnar]

* FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:

> BTW, "[PATCH 4/6] AMD IOMMU: honor iommu=off instead of amd_iommu=off"
> patch in auto-x86-next needs update the kernel parameter.
> 
> =
> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Subject: [PATCH] AMD IOMMU: fix kernel parameter
> 
> amd_iommu=off was replaced with a common parameter, iommu=off.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

applied to tip/x86/amd-iommu, thanks.

	Ingo