[PATCH 0/2] arm64: PCI_IOBASE fixes

[PATCH 0/2] arm64: PCI_IOBASE fixes

[Mark Rutland]

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

[PATCH 1/2] arm64: Fix overlapping VA allocations

[Mark Rutland]

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.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reported-by: Mark Brown <broonie@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Laura Abbott <lauraa@codeaurora.org>
Cc: Liviu Dudau <Liviu.Dudau@arm.com>
Cc: Steve Capper <steve.capper@linaro.org>
Cc: Will Deacon <will.deacon@arm.com>
---
 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(-)

diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
index 949c406..c180c9d 100644
--- a/arch/arm64/include/asm/io.h
+++ b/arch/arm64/include/asm/io.h
@@ -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.
diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index 6486b2b..a183577 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -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
diff --git a/arch/arm64/mm/dump.c b/arch/arm64/mm/dump.c
index cf33f33..203a6cf 100644
--- a/arch/arm64/mm/dump.c
+++ b/arch/arm64/mm/dump.c
@@ -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" },
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index bac492c..9f2406d 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -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

[PATCH 2/2] arm64: mm: dump: add missing includes

[Mark Rutland]

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.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reported-by: Mark Brown <broonie@kernel.org>
Cc: Steve Capper <steve.capper@linaro.org>
Cc: Catalin Marinas <Catalin.Marinas@arm.com>
Cc: Laura Abbott <lauraa@codeaurora.org>
Cc: Will Deacon <Will.Deacon@arm.com>
---
 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

diff --git a/arch/arm64/mm/dump.c b/arch/arm64/mm/dump.c
index 203a6cf..48a4a2f 100644
--- a/arch/arm64/mm/dump.c
+++ b/arch/arm64/mm/dump.c
@@ -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

[PATCH 1/2] arm64: Fix overlapping VA allocations

[Catalin Marinas]

On Mon, Jan 12, 2015 at 07:36:47PM +0000, Mark Rutland wrote:
> --- a/arch/arm64/include/asm/io.h
> +++ b/arch/arm64/include/asm/io.h
> @@ -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)

I've seen at least couple of places where IO_SPACE_LIMIT is used as a
mark. So I would prefer it to be something like (power-of-two - 1)
rather than some random (size - 1).

> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -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)

So you could make PCI_IO_START MODULES_VADDR - SZ_16M and FIXADDR_TOP
just below it (or above as it was before, I don't really care).

>  #ifdef CONFIG_COMPAT
> diff --git a/arch/arm64/mm/dump.c b/arch/arm64/mm/dump.c
> index cf33f33..203a6cf 100644
> --- a/arch/arm64/mm/dump.c
> +++ b/arch/arm64/mm/dump.c
> @@ -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" },
> 
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index bac492c..9f2406d 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -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),

If you change the order of FIX_ADDR_TOP with the PCI_IO_START, so you
should change the printed order as well.

-- 
Catalin

[PATCH 1/2] arm64: Fix overlapping VA allocations

[Mark Rutland]

On Tue, Jan 13, 2015 at 03:34:49PM +0000, Catalin Marinas wrote:
> On Mon, Jan 12, 2015 at 07:36:47PM +0000, Mark Rutland wrote:
> > --- a/arch/arm64/include/asm/io.h
> > +++ b/arch/arm64/include/asm/io.h
> > @@ -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)
> 
> I've seen at least couple of places where IO_SPACE_LIMIT is used as a
> mark. So I would prefer it to be something like (power-of-two - 1)
> rather than some random (size - 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.

> > --- a/arch/arm64/include/asm/memory.h
> > +++ b/arch/arm64/include/asm/memory.h
> > @@ -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)
> 
> So you could make PCI_IO_START MODULES_VADDR - SZ_16M and FIXADDR_TOP
> just below it (or above as it was before, I don't really care).

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?

> >  #ifdef CONFIG_COMPAT
> > diff --git a/arch/arm64/mm/dump.c b/arch/arm64/mm/dump.c
> > index cf33f33..203a6cf 100644
> > --- a/arch/arm64/mm/dump.c
> > +++ b/arch/arm64/mm/dump.c
> > @@ -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" },
> > 
> > diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> > index bac492c..9f2406d 100644
> > --- a/arch/arm64/mm/init.c
> > +++ b/arch/arm64/mm/init.c
> > @@ -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),
> 
> If you change the order of FIX_ADDR_TOP with the PCI_IO_START, so you
> should change the printed order as well.

Done.

Thanks,
Mark.

[PATCH 1/2] arm64: Fix overlapping VA allocations

[Will Deacon]

On Wed, Jan 14, 2015 at 10:42:56AM +0000, Mark Rutland wrote:
> On Tue, Jan 13, 2015 at 03:34:49PM +0000, Catalin Marinas wrote:
> > On Mon, Jan 12, 2015 at 07:36:47PM +0000, Mark Rutland wrote:
> > > --- a/arch/arm64/include/asm/io.h
> > > +++ b/arch/arm64/include/asm/io.h
> > > @@ -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)
> > 
> > I've seen at least couple of places where IO_SPACE_LIMIT is used as a
> > mark. So I would prefer it to be something like (power-of-two - 1)
> > rather than some random (size - 1).
> 
> I couldn't spot instances in core code (maybe I missed them), just
> arch/arm and arch/hexagon:

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