DMA 4GB boundary protection

There are many adapters which can not handle DMAing acrosss any 4 GB boundary. For instance the latest Emulex adapters.

This normally is not an issue as firmware gives us dma-windows under 4gigs. However, some of the new System-P boxes have dma-windows above 4gigs, and this present a problem.

I propose fixing it in the IOMMU allocation instead of making each driver protect against it as it is more efficient, and won't require changing every driver which has not considered this issue.

Quoted 7 lines · Jake Moilanen

There are many adapters which can not handle DMAing acrosss any 4 GB boundary. For instance the latest Emulex adapters.

This normally is not an issue as firmware gives us dma-windows under 4gigs. However, some of the new System-P boxes have dma-windows above 4gigs, and this present a problem.

Quoted 3 lines

I propose fixing it in the IOMMU allocation instead of making each driver protect against it as it is more efficient, and won't require changing every driver which has not considered this issue.

Sorry, but you need to fix the drivers. They might be used on a system without an IOMMU, or with direct mapping. It's particularily likely in the case of DAC-capable devices, and that's the only case for which you can cross a 4GB boundary in the first place.

Quoted 8 lines · Olof Johansson

Quoted 7 lines · Jake Moilanen

There are many adapters which can not handle DMAing acrosss any 4 GB boundary. For instance the latest Emulex adapters.

This normally is not an issue as firmware gives us dma-windows under 4gigs. However, some of the new System-P boxes have dma-windows above 4gigs, and this present a problem.

Quoted 8 lines

Quoted 3 lines

I propose fixing it in the IOMMU allocation instead of making each driver protect against it as it is more efficient, and won't require changing every driver which has not considered this issue.

Sorry, but you need to fix the drivers. They might be used on a system without an IOMMU, or with direct mapping. It's particularily likely in the case of DAC-capable devices, and that's the only case for which you can cross a 4GB boundary in the first place.

Hrm... ok, that would be ideal, _but_

- We currently don't have platforms that DMA > 32 bits and don't have an iommu

- Drivers are broken -today- and I doubt they can all be audited and fixed (and fixes bacported to distros) quickly

- That problem is very common and can be very hard to debug when it happens.

So while I agree, the logical fix would be in the drivers, I think that -also- making sure the iommu code will not hand off sections crossing 4G boundaries does make some sense. It will at least reduce the likehood of strange and hard to debug problems on those setups that do have an iommu and >4G of RAM, which is pretty much all of them at the moment on ppc64.

Can you double check the correctness of the patch ? I'll have a better look myself next week hopefully.

- Drivers are broken -today- and I doubt they can all be audited and fixed (and fixes bacported to distros) quickly

Quoted 2 lines · Benjamin Herrenschmidt

- Drivers are broken -today- and I doubt they can all be audited and fixed (and fixes bacported to distros) quickly

Which drivers? All requests coming from the block layer make sure you never span the 4GB boundary, and IIRC that same is true for networking. Similarly dma_alloc_coherent and pci_alloc_consinstant only allocate from the lower 4G unless told otherwise.

A driver writer
would have to be extra-ordinarily stuid to get this wrong. (And yes, some driver writers are..)

This normally is not an issue as firmware gives us dma-windows under 4gigs. However, some of the new System-P boxes have dma-windows above 4gigs, and this present a problem.

I propose fixing it in the IOMMU allocation instead of making each driver protect against it as it is more efficient, and won't require changing every driver which has not considered this issue.

The drawback of this patch is that it adds code to every single allocation. Instead, you should just mark the last entry before the 4GB boundary as allocated when you setup the bitmaps for the table. That way, no allocation will ever be able to cross over.

Even nicer would be to only do it when a boot option is specified, so we actually have a chance to expose and find the driver bugs instead of papering them over.

Quoted 5 lines

The drawback of this patch is that it adds code to every single allocation.
Instead, you should just mark the last entry before the 4GB boundary as allocated when you setup the bitmaps for the table. That way, no allocation will ever be able to cross over.

Jake said that this bug happens when crossing _any_ 4GB boundary, so that means reserving a few more blocks.

Quoted 3 lines

Even nicer would be to only do it when a boot option is specified, so we actually have a chance to expose and find the driver bugs instead of papering them over.

Almost all drivers (*) that can do DAC already avoid crossing the SAC-to-DAC boundary. I have never heard about a card having the bug on _any_ 4GB crossing, I doubt they are that common. Just fix the drivers :-)

Quoted 3 lines

I propose fixing it in the IOMMU allocation instead of making each driver protect against it as it is more efficient, and won't require changing every driver which has not considered this issue.

The drawback of this patch is that it adds code to every single allocation. Instead, you should just mark the last entry before the 4GB boundary as allocated when you setup the bitmaps for the table. That way, no allocation will ever be able to cross over.

Even nicer would be to only do it when a boot option is specified, so we actually have a chance to expose and find the driver bugs instead of papering them over.

Also, remember to use IOMMU_PAGE_SHIFT instead of PAGE_SHIFT, they might be different.

Quoted 4 lines

Quoted 3 lines

I propose fixing it in the IOMMU allocation instead of making each driver protect against it as it is more efficient, and won't require changing every driver which has not considered this issue.

Quoted 4 lines

The drawback of this patch is that it adds code to every single allocation. Instead, you should just mark the last entry before the 4GB boundary as allocated when you setup the bitmaps for the table. That way, no allocation will ever be able to cross over.

Agreed. While it's not an issue at 4KB, when we go to say 64k, or 16MB TCE sizes, this extra space will be wasted. But, I believe we'll need a little code for this anyways.

Quoted 3 lines

Even nicer would be to only do it when a boot option is specified, so we actually have a chance to expose and find the driver bugs instead of papering them over.

Done.

Quoted 2 lines

Also, remember to use IOMMU_PAGE_SHIFT instead of PAGE_SHIFT, they might be different.

Yup...

Here's a patch addressing Olof's concerns.

arch/powerpc/kernel/iommu.c +6 -0

@@ -480,6 +495,21 @@ struct iommu_table *iommu_init_table(str
 	ppc_md.tce_free(tbl, tbl->it_offset, tbl->it_size);
 #endif
 
+	/*
+	 * DMA cannot cross 4 GB boundary.  Mark first entry of each 4
+	 * GB chunk as reserved.
+	 */
+	if (protect4gb) {
+		start_addr = tbl->it_offset << IOMMU_PAGE_SHIFT;

Quoted 10 lines · Jake Moilanen

arch/powerpc/kernel/iommu.c +6 -0

@@ -480,6 +495,21 @@ struct iommu_table *iommu_init_table(str
 	ppc_md.tce_free(tbl, tbl->it_offset, tbl->it_size);
 #endif
 
+	/*
+	 * DMA cannot cross 4 GB boundary.  Mark first entry of each 4
+	 * GB chunk as reserved.
+	 */
+	if (protect4gb) {
+		start_addr = tbl->it_offset << IOMMU_PAGE_SHIFT;

arch/powerpc/kernel/iommu.c struct iommu_table *iommu_init_table(str · line 513

Quoted 10 lines

arch/powerpc/kernel/iommu.c +9 -0

+		/* go up to next 4GB boundary */
+		start_addr = (start_addr + 0x00000000ffffffffl) >> 32;
+		end_addr = (tbl->it_offset + tbl->it_size) << IOMMU_PAGE_SHIFT;
+		for (index = start_addr; index < end_addr; index += (1l << 32)) {
+			/* Reserve 4GB entry */
+			__set_bit((index >> IOMMU_PAGE_SHIFT) - tbl->it_offset,
 tbl->it_map);
+		}
+	}
+

This is done a bit more complicated than it has to be. The >> 32 is a red flag as well.

I would also like it to be the last page in the range, not the first (since otherwise you'll reserve even if the window is less than 4GB.

Something like (untested):

	entries_per_4g = 0x100000000 >> IOMMU_PAGE_SHIFT;

	/* Mark the last bit before a 4GB boundary as used */
	start_index = tbl->it_offset | (entries_per_4g - 1);
	end_index = tbl->it_offset + tbl->it_size;

	for (index = start_index; index < end_index; index += entries_per_4g)
		__set_bit(index, tbl->it_map);

Is easier to follow.

I would also like it to be the last page in the range, not the first (since otherwise you'll reserve even if the window is less than 4GB.

Something like (untested):

	entries_per_4g = 0x100000000 >> IOMMU_PAGE_SHIFT;

	/* Mark the last bit before a 4GB boundary as used */
	start_index = tbl->it_offset | (entries_per_4g - 1);
	end_index = tbl->it_offset + tbl->it_size;

	for (index = start_index; index < end_index; index += entries_per_4g)
		__set_bit(index, tbl->it_map);

Quoted 10 lines · Jake Moilanen

(quoted diff) +6 -0

@@ -480,6 +495,21 @@ struct iommu_table *iommu_init_table(str
 	ppc_md.tce_free(tbl, tbl->it_offset, tbl->it_size);
 #endif
 
+	/*
+	 * DMA cannot cross 4 GB boundary.  Mark first entry of each 4
+	 * GB chunk as reserved.
+	 */
+	if (protect4gb) {
+		start_addr = tbl->it_offset << IOMMU_PAGE_SHIFT;

Review struct iommu_table *iommu_init_table(str · line 513

Quoted 10 lines

(quoted diff) +9 -0

+		/* go up to next 4GB boundary */
+		start_addr = (start_addr + 0x00000000ffffffffl) >> 32;
+		end_addr = (tbl->it_offset + tbl->it_size) << IOMMU_PAGE_SHIFT;
+		for (index = start_addr; index < end_addr; index += (1l << 32)) {
+			/* Reserve 4GB entry */
+			__set_bit((index >> IOMMU_PAGE_SHIFT) - tbl->it_offset,
 tbl->it_map);
+		}
+	}
+

This is done a bit more complicated than it has to be. The >> 32 is a red flag as well.

I would also like it to be the last page in the range, not the first (since otherwise you'll reserve even if the window is less than 4GB.

Something like (untested):

	entries_per_4g = 0x100000000 >> IOMMU_PAGE_SHIFT;

	/* Mark the last bit before a 4GB boundary as used */
	start_index = tbl->it_offset | (entries_per_4g - 1);
	end_index = tbl->it_offset + tbl->it_size;

	for (index = start_index; index < end_index; index += entries_per_4g)
		__set_bit(index, tbl->it_map);

I just had to realign the start_index in case it didn't start at a 4GB boundary. Other than that, it worked like a champ. I also changed to forloop to exit if it's the last entry.

Signed-off-by Jake Moilanen <[email protected]>

---
 arch/powerpc/kernel/iommu.c |   35 ++++++++++++++++++++++++++++++++++-
 1 files changed, 34 insertions(+), 1 deletion(-)

Index: powerpc/arch/powerpc/kernel/iommu.c
===================================================================

arch/powerpc/kernel/iommu.c +34 -1

@@ -47,6 +47,8 @@ static int novmerge = 0;
 static int novmerge = 1;
 #endif
 
+static int protect4gb = 1;
+
 static inline unsigned long iommu_num_pages(unsigned long vaddr,
 					    unsigned long slen)
 {

@@ -58,6 +60,16 @@ static inline unsigned long iommu_num_pa
 	return npages;
 }
 
+static int __init setup_protect4gb(char *str)
+{
+	if (strcmp(str, "on") == 0)
+		protect4gb = 1;
+	else if (strcmp(str, "off") == 0)
+		protect4gb = 0;
+
+	return 1;
+}
+
 static int __init setup_iommu(char *str)
 {
 	if (!strcmp(str, "novmerge"))

@@ -67,6 +79,7 @@ static int __init setup_iommu(char *str)
 	return 1;
 }
 
+__setup("protect4gb=", setup_protect4gb);
 __setup("iommu=", setup_iommu);
 
 static unsigned long iommu_range_alloc(struct iommu_table *tbl,

@@ -429,6 +442,9 @@ void iommu_unmap_sg(struct iommu_table *
 struct iommu_table *iommu_init_table(struct iommu_table *tbl, int nid)
 {
 	unsigned long sz;
+	unsigned long start_index, end_index;
+	unsigned long entries_per_4g;
+	unsigned long index;
 	static int welcomed = 0;
 	struct page *page;
 

@@ -450,7 +466,7 @@ struct iommu_table *iommu_init_table(str
 
 #ifdef CONFIG_CRASH_DUMP
 	if (ppc_md.tce_get) {
-		unsigned long index, tceval;
+		unsigned long tceval;
 		unsigned long tcecount = 0;
 
 		/*

@@ -480,6 +496,23 @@ struct iommu_table *iommu_init_table(str
 	ppc_md.tce_free(tbl, tbl->it_offset, tbl->it_size);
 #endif
 
+	/*
+	 * DMA cannot cross 4 GB boundary.  Mark last entry of each 4
+	 * GB chunk as reserved.
+	 */
+	if (protect4gb) {
+		entries_per_4g = 0x100000000l >> IOMMU_PAGE_SHIFT;
+
+		/* Mark the last bit before a 4GB boundary as used */
+		start_index = (tbl->it_offset << IOMMU_PAGE_SHIFT) >> 32;
+		start_index |= (entries_per_4g - 1);
+
+		end_index = tbl->it_offset + tbl->it_size;
+
+		for (index = start_index; index < end_index - 1; index +=
 entries_per_4g)
+			__set_bit(index, tbl->it_map);
+	}
+
 	if (!welcomed) {
 		printk(KERN_INFO "IOMMU table initialized, virtual merging %s\n",

novmerge ? "disabled" : "enabled");

_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev

I just had to realign the start_index in case it didn't start at a 4GB boundary. Other than that, it worked like a champ. I also changed to for loop to exit if it's the last entry.

I just had to realign the start_index in case it didn't start at a 4GB boundary. Other than that, it worked like a champ. I also changed to for loop to exit if it's the last entry.

Review

Quoted 10 lines

(quoted diff) +10 -0

+	/*
+	 * DMA cannot cross 4 GB boundary.  Mark last entry of each 4
+	 * GB chunk as reserved.
+	 */
+	if (protect4gb) {
+		entries_per_4g = 0x100000000l >> IOMMU_PAGE_SHIFT;
+
+		/* Mark the last bit before a 4GB boundary as used */
+		start_index = (tbl->it_offset << IOMMU_PAGE_SHIFT) >> 32;
+		start_index |= (entries_per_4g - 1);

This looks broken.

The idea is to make start_index the last page before the first 4GB boundary after it_offset. If that happens to be beyond end_index the for loop below will never run. If it's below that, every last page in the 4GB ranges will be marked in the loop. This will work even if the table starts at i.e. 2GB and goes until 10GB.

With the first line above, your start_index will always be 0xfffff (unless the offset is waay up there in the address space).

The logic I had was:

	start_index = tbl->it_offset | (entries_per_4g - 1);                                                                                   

This is also broken, since it doesn't consider it_offset in the loop below. That was my bad, and I guess was what you tried to fix above.

What you really want is:

	start_index = tbl->it_offset | (entries_per_4g - 1);                                                                                   
	start_index -= tbl->it_offset;

	end_index = tbl->it_size;

Say that it_offset is at 3GB, with 4KB pages that means the value is 0xc0000. entries_per_4g is 0x100000, i.e. the logic becomes: 0xc0000 | 0xfffff = 0xfffff (- 0xc0000 = 0x3ffff), which indeed is the last page before 4GB.

If it_offset is at 9GB, i.e. 0x240000, then we get start_index at 0x2fffff (- 0x240000 = 0xbffff) , i.e. yet again last page before the 12GB wrap.

One more try.

There are many adapters which can not handle DMAing acrosss any 4 GB boundary. For instance the latest Emulex adapters.

This normally is not an issue as firmware gives dma-windows under 4gigs. However, some of the new System-P boxes have dma-windows above 4gigs, and this present a problem.

During initialization of the IOMMU tables, the last entry at each 4GB boundary is marked as used. Thus no mappings can cross the boundary. If a table ends at a 4GB boundary, the entry is not marked as used.

A boot option to remove this 4GB protection is given w/ protect4gb=off. This exposes the potential issue for driver and hardware development purposes.

Signed-off-by Jake Moilanen <[email protected]>

---
 arch/powerpc/kernel/iommu.c |   35 ++++++++++++++++++++++++++++++++++-
 1 files changed, 34 insertions(+), 1 deletion(-)

Index: powerpc/arch/powerpc/kernel/iommu.c
===================================================================

arch/powerpc/kernel/iommu.c +34 -1

@@ -47,6 +47,8 @@ static int novmerge = 0;
 static int novmerge = 1;
 #endif
 
+static int protect4gb = 1;
+
 static inline unsigned long iommu_num_pages(unsigned long vaddr,
 					    unsigned long slen)
 {

@@ -58,6 +60,16 @@ static inline unsigned long iommu_num_pa
 	return npages;
 }
 
+static int __init setup_protect4gb(char *str)
+{
+	if (strcmp(str, "on") == 0)
+		protect4gb = 1;
+	else if (strcmp(str, "off") == 0)
+		protect4gb = 0;
+
+	return 1;
+}
+
 static int __init setup_iommu(char *str)
 {
 	if (!strcmp(str, "novmerge"))

@@ -67,6 +79,7 @@ static int __init setup_iommu(char *str)
 	return 1;
 }
 
+__setup("protect4gb=", setup_protect4gb);
 __setup("iommu=", setup_iommu);
 
 static unsigned long iommu_range_alloc(struct iommu_table *tbl,

@@ -429,6 +442,9 @@ void iommu_unmap_sg(struct iommu_table *
 struct iommu_table *iommu_init_table(struct iommu_table *tbl, int nid)
 {
 	unsigned long sz;
+	unsigned long start_index, end_index;
+	unsigned long entries_per_4g;
+	unsigned long index;
 	static int welcomed = 0;
 	struct page *page;
 

@@ -450,7 +466,7 @@ struct iommu_table *iommu_init_table(str
 
 #ifdef CONFIG_CRASH_DUMP
 	if (ppc_md.tce_get) {
-		unsigned long index, tceval;
+		unsigned long tceval;
 		unsigned long tcecount = 0;
 
 		/*

@@ -480,6 +496,23 @@ struct iommu_table *iommu_init_table(str
 	ppc_md.tce_free(tbl, tbl->it_offset, tbl->it_size);
 #endif
 
+	/*
+	 * DMA cannot cross 4 GB boundary.  Mark last entry of each 4
+	 * GB chunk as reserved.
+	 */
+	if (protect4gb) {
+		entries_per_4g = 0x100000000l >> IOMMU_PAGE_SHIFT;
+
+		/* Mark the last bit before a 4GB boundary as used */
+		start_index = tbl->it_offset | (entries_per_4g - 1);
+		start_index -= tbl->it_offset;
+
+		end_index = tbl->it_size;
+
+		for (index = start_index; index < end_index - 1; index +=
 entries_per_4g)
+			__set_bit(index, tbl->it_map);
+	}
+
 	if (!welcomed) {
 		printk(KERN_INFO "IOMMU table initialized, virtual merging %s\n",

novmerge ? "disabled" : "enabled");

_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Quoted 1 line · Jake Moilanen

One more try.

And patch is still mangled....

Quoted 5 lines · Benjamin Herrenschmidt

Quoted 1 line · Jake Moilanen

One more try.

And patch is still mangled....

Hrm....I should just go back to Sylpheed.

Unmangled version:

There are many adapters which can not handle DMAing acrosss any 4 GB boundary. For instance the latest Emulex adapters.

This normally is not an issue as firmware gives dma-windows under 4gigs. However, some of the new System-P boxes have dma-windows above 4gigs, and this present a problem.

During initialization of the IOMMU tables, the last entry at each 4GB boundary is marked as used. Thus no mappings can cross the boundary. If a table ends at a 4GB boundary, the entry is not marked as used.

A boot option to remove this 4GB protection is given w/ protect4gb=off. This exposes the potential issue for driver and hardware development purposes.

Signed-off-by Jake Moilanen <[email protected]>

Quoted 5 lines · Benjamin Herrenschmidt

Quoted 1 line · Jake Moilanen

One more try.

And patch is still mangled....

Hrm....I should just go back to Sylpheed.

Jake,

It looks like I have applied both the final version of this patch (569975591c5530fdc9c7a3c45122e5e46f075a74) and the original version (618d3adc351a24c4c48437c767befb88ca2d199d) that modified iommu_range_alloc().

Should I revert the original version?