arm64: PCI_IOBASE fixes

Mark Brown reported build failures in the page table dump code, due to lack of an explicit include for PCI_IOBASE.

While looking into that I spotted that the VA space allocated for PCI I/O overlaps with the fixmap allocation, and that in a couple of places we erroneously assume that the I/O space is 16MiB while the allocted VA space (and associated IO_SPACE_LIMIT) cover 32MiB.

The first patch fixes the VA space allocation, preventing overlap and removing erroneous assumptions about the I/O space size. I'm still not that sure of my understanding of the fixmap code and the entries below __end_of_permanent_fixed_addresses (which aren't accounted for in mem_init or the page table dump code), but at least they now live at the very bottom of the kernel mapping and can't potentially overlap with anything.

The second patch adds the missing explicit includes for the remaining definitions used in the dump code, and has been updated from the previous posting [2] to take the effects of patch 1 into account.

I've built and boot tested a v3.19-rc4 defconfig with these patches applied.

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/315228.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/315326.html

Mark Rutland (2):
arm64: Fix overlapping VA allocations
arm64: mm: dump: add missing includes

 arch/arm64/include/asm/io.h     | 5 +++--
 arch/arm64/include/asm/memory.h | 4 +++-
 arch/arm64/mm/dump.c            | 8 ++++++--
 arch/arm64/mm/init.c            | 3 ++-
 4 files changed, 14 insertions(+), 6 deletions(-)

1.9.1

32MiB of space immediately below MODULES_VADDR is allocated as PCI I/O space, yet the final 8KiB of this space is also allocated for the fixmap, allowing for potential clashes between the two.

Additionally, the size of the PCI I/O space is assumed to be 16MiB by mem_init and the page table dumping code, resulting the I/O space being erroneously reported as 16MiB long.

This patch changes the definition of the PCI I/O space allocation to live in asm/memory.h, along with the other VA space allocations. As the fixmap allocation depends on the number of fixmap entries, this is moved below the PCI I/O space allocation. Both the fixmap and PCI I/O space are guarded with a following 2MB of padding. Sites assuming the I/O space was 16MiB are moved over use new PCI_IO_{START,END} definitions.

As a useful side effect, the use of the new PCI_IO_{START,END} definitions prevents a build issue in the dumping code due to a (now redundant) missing include of io.h for PCI_IOBASE, reported by Mark Brown.

---
 arch/arm64/include/asm/io.h     | 5 +++--
 arch/arm64/include/asm/memory.h | 4 +++-
 arch/arm64/mm/dump.c            | 4 ++--
 arch/arm64/mm/init.c            | 3 ++-
 4 files changed, 10 insertions(+), 6 deletions(-)

index 949c406..c180c9d 100644
@@ -26,6 +26,7 @@
 
 #include <asm/byteorder.h>
 #include <asm/barrier.h>
+#include <asm/memory.h>
 #include <asm/pgtable.h>
 #include <asm/early_ioremap.h>
 #include <asm/alternative.h>

@@ -145,8 +146,8 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
  *  I/O port access primitives.
  */
 #define arch_has_dev_port()	(1)
-#define IO_SPACE_LIMIT		(SZ_32M - 1)
-#define PCI_IOBASE		((void __iomem *)(MODULES_VADDR - SZ_32M))
+#define IO_SPACE_LIMIT		(PCI_IO_END - PCI_IO_START - 1)
+#define PCI_IOBASE		((void __iomem *)PCI_IO_START)
 
 /*
  * String version of I/O memory access operations.

index 6486b2b..a183577 100644
@@ -45,7 +45,9 @@
 #define PAGE_OFFSET		(UL(0xffffffffffffffff) << (VA_BITS - 1))
 #define MODULES_END		(PAGE_OFFSET)
 #define MODULES_VADDR		(MODULES_END - SZ_64M)
-#define FIXADDR_TOP		(MODULES_VADDR - SZ_2M - PAGE_SIZE)
+#define PCI_IO_END		(MODULES_VADDR - SZ_2M)
+#define PCI_IO_START		(PCI_IO_END - SZ_32M)
+#define FIXADDR_TOP		(PCI_IO_START - SZ_2M)
 #define TASK_SIZE_64		(UL(1) << VA_BITS)
 
 #ifdef CONFIG_COMPAT

index cf33f33..203a6cf 100644
@@ -52,8 +52,8 @@ static struct addr_marker address_markers[] = {
 	{ 0,			"vmemmap start" },
 	{ 0,			"vmemmap end" },
 #endif
-	{ (unsigned long) PCI_IOBASE,		"PCI I/O start" },
-	{ (unsigned long) PCI_IOBASE + SZ_16M,	"PCI I/O end" },
+	{ PCI_IO_START,		"PCI I/O start" },
+	{ PCI_IO_END,		"PCI I/O end" },
 	{ FIXADDR_START,	"Fixmap start" },
 	{ FIXADDR_TOP,		"Fixmap end" },
 	{ MODULES_VADDR,	"Modules start" },

index bac492c..9f2406d 100644
@@ -35,6 +35,7 @@
 #include <linux/efi.h>
 
 #include <asm/fixmap.h>
+#include <asm/memory.h>
 #include <asm/sections.h>
 #include <asm/setup.h>
 #include <asm/sizes.h>

@@ -291,7 +292,7 @@ void __init mem_init(void)
 		  MLM((unsigned long)virt_to_page(PAGE_OFFSET),
 		      (unsigned long)virt_to_page(high_memory)),
 #endif
-		  MLM((unsigned long)PCI_IOBASE, (unsigned long)PCI_IOBASE + SZ_16M),
+		  MLM(PCI_IO_START, PCI_IO_END),
 		  MLK(FIXADDR_START, FIXADDR_TOP),
 		  MLM(MODULES_VADDR, MODULES_END),
 		  MLM(PAGE_OFFSET, (unsigned long)high_memory),

1.9.1

I couldn't spot instances in core code (maybe I missed them), just arch/arm and arch/hexagon:

[mark at leverpostej:~/src/linux]% git grep IO_SPACE_LIMIT -- arch | grep '&'
arch/arm/include/asm/io.h:#define __io(a) __typesafe_io(PCI_IO_VIRT_BASE + ((a) & IO_SPACE_LIMIT))
arch/arm/include/asm/io.h:#define __io(a) __typesafe_io((a) & IO_SPACE_LIMIT)

arch/hexagon/include/asm/io.h:  return readb(_IO_BASE + (port & IO_SPACE_LIMIT));
arch/hexagon/include/asm/io.h:  return readw(_IO_BASE + (port & IO_SPACE_LIMIT));
arch/hexagon/include/asm/io.h:  return readl(_IO_BASE + (port & IO_SPACE_LIMIT));
arch/hexagon/include/asm/io.h:  writeb(data, _IO_BASE + (port & IO_SPACE_LIMIT));
arch/hexagon/include/asm/io.h:  writew(data, _IO_BASE + (port & IO_SPACE_LIMIT));
arch/hexagon/include/asm/io.h:  writel(data, _IO_BASE + (port & IO_SPACE_LIMIT));

Are PCI I/O addresses expected to wrap in this manner? To me it seems that masking addresses in this way just hides a bug if addresses are provided that don't fit inside the I/O space -- we'll generate an address inside the I/O space, but it could still be wrong.

If we do require IO_SPACE_LIMIT to function as a mask, then I think that we should add an explicit check to asm/io.h for that:

/*
* IO_SPACE_LIMIT needs to function as a mask.
*/
BUILD_BUG_ON_NOT_POWER_OF_2(IO_SPACE_LIMIT + 1);

That should highlight potential problems if someone updates the I/O space definitions in future. If core expects that IO_SPACE_LIMIT is usable as a mask then that should probably live in asm-generic/io.h.

I'd very much prefer that we have the the PCI_IO_END defintion and define PCI_IO_START in terms of that. That makes it easier to ensure that the boot-time VA space layout dump and the page table dumper agree with the actual VA space layout.

Are you asking for the I/O space to be shrunk to 16MB?

I can drop the 2MB padding if that's what you're suggesting?

Done.

Thanks,
Mark.

drivers/pci/probe.c: mask64 = PCI_BASE_ADDRESS_IO_MASK & (u32)IO_SPACE_LIMIT; include/asm-generic/io.h: return PCI_IOBASE + (port & IO_SPACE_LIMIT);

Will

The arm64 dump code is currently relying on some definitions which are pulled in via transitive dependencies. While current build issues have been fixed by a separate patch, it seems we have implicit dependencies on the following definitions:

* MODULES_VADDR         (asm/memory.h)
* MODULES_END           (asm/memory.h)
* PAGE_OFFSET           (asm/memory.h)
* PTE_*                 (asm/pgtable-hwdef.h)
* ENOMEM                (linux/errno.h)
* device_initcall       (linux/init.h)

This patch ensures we explicitly include the relevant headers for the above items, hopefully preventing future issues as headers are refactored.

---
 arch/arm64/mm/dump.c | 4 ++++
 1 file changed, 4 insertions(+)

This was previously posted as a standalone patch [1]. As patch 1 fixes the original issue, this patch has been modified to fix the remaining potential issues that we don't yet seem to be hitting. I've also taken Russell's advice to use linux/errno.h.

Steve, I've dropped your ack now that the fix for the original issue is in patch 1. Hopefully that's just a formality :)

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/315326.html

index 203a6cf..48a4a2f 100644
@@ -14,13 +14,17 @@
  * of the License.
  */
 #include <linux/debugfs.h>
+#include <linux/errno.h>
 #include <linux/fs.h>
+#include <linux/init.h>
 #include <linux/mm.h>
 #include <linux/sched.h>
 #include <linux/seq_file.h>
 
 #include <asm/fixmap.h>
+#include <asm/memory.h>
 #include <asm/pgtable.h>
+#include <asm/pgtable-hwdef.h>
 
 #define LOWEST_ADDR	(UL(0xffffffffffffffff) << VA_BITS)
 

1.9.1