[RFC v2] arm:extend the reserved mrmory for initrd to be page aligned

[RFC v2] arm:extend the reserved mrmory for initrd to be page aligned

[Wang, Yalin]

this patch extend the start and end address of initrd to be page aligned,
so that we can free all memory including the un-page aligned head or tail
page of initrd, if the start or end address of initrd are not page
aligned, the page can't be freed by free_initrd_mem() function.

Signed-off-by: Yalin Wang <yalin.wang@sonymobile.com>
---
 arch/arm/mm/init.c   | 19 +++++++++++++++++--
 arch/arm64/mm/init.c | 37 +++++++++++++++++++++++++++++++++----
 2 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 659c75d..8490b70 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -277,6 +277,8 @@ phys_addr_t __init arm_memblock_steal(phys_addr_t size, phys_addr_t align)
 void __init arm_memblock_init(const struct machine_desc *mdesc)
 {
 	/* Register the kernel text, kernel data and initrd with memblock. */
+	phys_addr_t phys_initrd_start_orig __maybe_unused;
+	phys_addr_t phys_initrd_size_orig __maybe_unused;
 #ifdef CONFIG_XIP_KERNEL
 	memblock_reserve(__pa(_sdata), _end - _sdata);
 #else
@@ -289,6 +291,13 @@ void __init arm_memblock_init(const struct machine_desc *mdesc)
 		phys_initrd_size = initrd_end - initrd_start;
 	}
 	initrd_start = initrd_end = 0;
+	phys_initrd_start_orig = phys_initrd_start;
+	phys_initrd_size_orig = phys_initrd_size;
+	/* make sure the start and end address are page aligned */
+	phys_initrd_size = round_up(phys_initrd_start + phys_initrd_size, PAGE_SIZE);
+	phys_initrd_start = round_down(phys_initrd_start, PAGE_SIZE);
+	phys_initrd_size -= phys_initrd_start;
+
 	if (phys_initrd_size &&
 	    !memblock_is_region_memory(phys_initrd_start, phys_initrd_size)) {
 		pr_err("INITRD: 0x%08llx+0x%08lx is not a memory region - disabling initrd\n",
@@ -305,9 +314,10 @@ void __init arm_memblock_init(const struct machine_desc *mdesc)
 		memblock_reserve(phys_initrd_start, phys_initrd_size);
 
 		/* Now convert initrd to virtual addresses */
-		initrd_start = __phys_to_virt(phys_initrd_start);
-		initrd_end = initrd_start + phys_initrd_size;
+		initrd_start = __phys_to_virt(phys_initrd_start_orig);
+		initrd_end = initrd_start + phys_initrd_size_orig;
 	}
+
 #endif
 
 	arm_mm_memblock_reserve();
@@ -636,6 +646,11 @@ static int keep_initrd;
 void free_initrd_mem(unsigned long start, unsigned long end)
 {
 	if (!keep_initrd) {
+		if (start == initrd_start)
+			start = round_down(start, PAGE_SIZE);
+		if (end == initrd_end)
+			end = round_up(end, PAGE_SIZE);
+
 		poison_init_mem((void *)start, PAGE_ALIGN(end) - start);
 		free_reserved_area((void *)start, (void *)end, -1, "initrd");
 	}
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index 5472c24..9dfd9a6 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -138,15 +138,38 @@ static void arm64_memory_present(void)
 void __init arm64_memblock_init(void)
 {
 	phys_addr_t dma_phys_limit = 0;
-
+	phys_addr_t phys_initrd_start;
+	phys_addr_t phys_initrd_size;
 	/*
 	 * Register the kernel text, kernel data, initrd, and initial
 	 * pagetables with memblock.
 	 */
 	memblock_reserve(__pa(_text), _end - _text);
 #ifdef CONFIG_BLK_DEV_INITRD
-	if (initrd_start)
-		memblock_reserve(__virt_to_phys(initrd_start), initrd_end - initrd_start);
+	if (initrd_start) {
+		phys_initrd_start = __virt_to_phys(initrd_start);
+		phys_initrd_size = initrd_end - initrd_start;
+		/* make sure the start and end address are page aligned */
+		phys_initrd_size = round_up(phys_initrd_start + phys_initrd_size, PAGE_SIZE);
+		phys_initrd_start = round_down(phys_initrd_start, PAGE_SIZE);
+		phys_initrd_size -= phys_initrd_start;
+		if (phys_initrd_size &&
+				!memblock_is_region_memory(phys_initrd_start, phys_initrd_size)) {
+			pr_err("INITRD: %pa+%pa is not a memory region - disabling initrd\n",
+					&phys_initrd_start, &phys_initrd_size);
+			phys_initrd_start = phys_initrd_size = 0;
+		}
+		if (phys_initrd_size &&
+				memblock_is_region_reserved(phys_initrd_start, phys_initrd_size)) {
+			pr_err("INITRD: %pa+%pa overlaps in-use memory region - disabling initrd\n",
+					&phys_initrd_start, &phys_initrd_size);
+			phys_initrd_start = phys_initrd_size = 0;
+		}
+		if (phys_initrd_size)
+			memblock_reserve(phys_initrd_start, phys_initrd_size);
+		else
+			initrd_start = initrd_end = 0;
+	}
 #endif
 
 	if (!efi_enabled(EFI_MEMMAP))
@@ -334,8 +357,14 @@ static int keep_initrd;
 
 void free_initrd_mem(unsigned long start, unsigned long end)
 {
-	if (!keep_initrd)
+	if (!keep_initrd) {
+		if (start == initrd_start)
+			start = round_down(start, PAGE_SIZE);
+		if (end == initrd_end)
+			end = round_up(end, PAGE_SIZE);
+
 		free_reserved_area((void *)start, (void *)end, 0, "initrd");
+	}
 }
 
 static int __init keepinitrd_setup(char *__unused)
-- 
2.1.0

Re: [RFC v2] arm:extend the reserved mrmory for initrd to be page aligned

[Russell King - ARM Linux]

On Mon, Sep 15, 2014 at 07:07:20PM +0800, Wang, Yalin wrote:
> this patch extend the start and end address of initrd to be page aligned,
> so that we can free all memory including the un-page aligned head or tail
> page of initrd, if the start or end address of initrd are not page
> aligned, the page can't be freed by free_initrd_mem() function.

Better, but I think it's more complicated than it needs to be:

> Signed-off-by: Yalin Wang <yalin.wang@sonymobile.com>
> ---
>  arch/arm/mm/init.c   | 19 +++++++++++++++++--
>  arch/arm64/mm/init.c | 37 +++++++++++++++++++++++++++++++++----
>  2 files changed, 50 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> index 659c75d..8490b70 100644
> --- a/arch/arm/mm/init.c
> +++ b/arch/arm/mm/init.c
> @@ -277,6 +277,8 @@ phys_addr_t __init arm_memblock_steal(phys_addr_t size, phys_addr_t align)
>  void __init arm_memblock_init(const struct machine_desc *mdesc)
>  {
>  	/* Register the kernel text, kernel data and initrd with memblock. */
> +	phys_addr_t phys_initrd_start_orig __maybe_unused;
> +	phys_addr_t phys_initrd_size_orig __maybe_unused;
>  #ifdef CONFIG_XIP_KERNEL
>  	memblock_reserve(__pa(_sdata), _end - _sdata);
>  #else
> @@ -289,6 +291,13 @@ void __init arm_memblock_init(const struct machine_desc *mdesc)
>  		phys_initrd_size = initrd_end - initrd_start;
>  	}
>  	initrd_start = initrd_end = 0;
> +	phys_initrd_start_orig = phys_initrd_start;
> +	phys_initrd_size_orig = phys_initrd_size;
> +	/* make sure the start and end address are page aligned */
> +	phys_initrd_size = round_up(phys_initrd_start + phys_initrd_size, PAGE_SIZE);
> +	phys_initrd_start = round_down(phys_initrd_start, PAGE_SIZE);
> +	phys_initrd_size -= phys_initrd_start;
> +
>  	if (phys_initrd_size &&
>  	    !memblock_is_region_memory(phys_initrd_start, phys_initrd_size)) {
>  		pr_err("INITRD: 0x%08llx+0x%08lx is not a memory region - disabling initrd\n",
> @@ -305,9 +314,10 @@ void __init arm_memblock_init(const struct machine_desc *mdesc)
>  		memblock_reserve(phys_initrd_start, phys_initrd_size);
>  
>  		/* Now convert initrd to virtual addresses */
> -		initrd_start = __phys_to_virt(phys_initrd_start);
> -		initrd_end = initrd_start + phys_initrd_size;
> +		initrd_start = __phys_to_virt(phys_initrd_start_orig);
> +		initrd_end = initrd_start + phys_initrd_size_orig;
>  	}
> +

I think all the above is entirely unnecessary.  The memblock APIs
(especially memblock_reserve()) will mark the overlapped pages as reserved
- they round down the starting address, and round up the end address
(calculated from start + size).

Hence, this:

> @@ -636,6 +646,11 @@ static int keep_initrd;
>  void free_initrd_mem(unsigned long start, unsigned long end)
>  {
>  	if (!keep_initrd) {
> +		if (start == initrd_start)
> +			start = round_down(start, PAGE_SIZE);
> +		if (end == initrd_end)
> +			end = round_up(end, PAGE_SIZE);
> +
>  		poison_init_mem((void *)start, PAGE_ALIGN(end) - start);
>  		free_reserved_area((void *)start, (void *)end, -1, "initrd");
>  	}

is the only bit of code you likely need to achieve your goal.

Thinking about this, I think that you are quite right to align these.
The memory around the initrd is defined to be system memory, and we
already free the pages around it, so it *is* wrong not to free the
partial initrd pages.

Good catch.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

RE: [RFC v2] arm:extend the reserved mrmory for initrd to be page aligned

[Wang, Yalin]

Great!
yeah, you are right,
just keep the change in free_initrd_mem( ) is ok.
we don't need keep reserved memory to be aligned ,

Thanks!

________________________________________
From: Russell King - ARM Linux [linux@arm.linux.org.uk]
Sent: Monday, September 15, 2014 7:33 PM
To: Wang, Yalin
Cc: 'Will Deacon'; 'linux-kernel@vger.kernel.org'; 'linux-arm-kernel@lists.infradead.org'; 'linux-mm@kvack.org'; 'linux-arm-msm@vger.kernel.org'
Subject: Re: [RFC v2] arm:extend the reserved mrmory for initrd to be page      aligned

On Mon, Sep 15, 2014 at 07:07:20PM +0800, Wang, Yalin wrote:
> this patch extend the start and end address of initrd to be page aligned,
> so that we can free all memory including the un-page aligned head or tail
> page of initrd, if the start or end address of initrd are not page
> aligned, the page can't be freed by free_initrd_mem() function.

Better, but I think it's more complicated than it needs to be:

> Signed-off-by: Yalin Wang <yalin.wang@sonymobile.com>
> ---
>  arch/arm/mm/init.c   | 19 +++++++++++++++++--
>  arch/arm64/mm/init.c | 37 +++++++++++++++++++++++++++++++++----
>  2 files changed, 50 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> index 659c75d..8490b70 100644
> --- a/arch/arm/mm/init.c
> +++ b/arch/arm/mm/init.c
> @@ -277,6 +277,8 @@ phys_addr_t __init arm_memblock_steal(phys_addr_t size, phys_addr_t align)
>  void __init arm_memblock_init(const struct machine_desc *mdesc)
>  {
>       /* Register the kernel text, kernel data and initrd with memblock. */
> +     phys_addr_t phys_initrd_start_orig __maybe_unused;
> +     phys_addr_t phys_initrd_size_orig __maybe_unused;
>  #ifdef CONFIG_XIP_KERNEL
>       memblock_reserve(__pa(_sdata), _end - _sdata);
>  #else
> @@ -289,6 +291,13 @@ void __init arm_memblock_init(const struct machine_desc *mdesc)
>               phys_initrd_size = initrd_end - initrd_start;
>       }
>       initrd_start = initrd_end = 0;
> +     phys_initrd_start_orig = phys_initrd_start;
> +     phys_initrd_size_orig = phys_initrd_size;
> +     /* make sure the start and end address are page aligned */
> +     phys_initrd_size = round_up(phys_initrd_start + phys_initrd_size, PAGE_SIZE);
> +     phys_initrd_start = round_down(phys_initrd_start, PAGE_SIZE);
> +     phys_initrd_size -= phys_initrd_start;
> +
>       if (phys_initrd_size &&
>           !memblock_is_region_memory(phys_initrd_start, phys_initrd_size)) {
>               pr_err("INITRD: 0x%08llx+0x%08lx is not a memory region - disabling initrd\n",
> @@ -305,9 +314,10 @@ void __init arm_memblock_init(const struct machine_desc *mdesc)
>               memblock_reserve(phys_initrd_start, phys_initrd_size);
>
>               /* Now convert initrd to virtual addresses */
> -             initrd_start = __phys_to_virt(phys_initrd_start);
> -             initrd_end = initrd_start + phys_initrd_size;
> +             initrd_start = __phys_to_virt(phys_initrd_start_orig);
> +             initrd_end = initrd_start + phys_initrd_size_orig;
>       }
> +

I think all the above is entirely unnecessary.  The memblock APIs
(especially memblock_reserve()) will mark the overlapped pages as reserved
- they round down the starting address, and round up the end address
(calculated from start + size).

Hence, this:

> @@ -636,6 +646,11 @@ static int keep_initrd;
>  void free_initrd_mem(unsigned long start, unsigned long end)
>  {
>       if (!keep_initrd) {
> +             if (start == initrd_start)
> +                     start = round_down(start, PAGE_SIZE);
> +             if (end == initrd_end)
> +                     end = round_up(end, PAGE_SIZE);
> +
>               poison_init_mem((void *)start, PAGE_ALIGN(end) - start);
>               free_reserved_area((void *)start, (void *)end, -1, "initrd");
>       }

is the only bit of code you likely need to achieve your goal.

Thinking about this, I think that you are quite right to align these.
The memory around the initrd is defined to be system memory, and we
already free the pages around it, so it *is* wrong not to free the
partial initrd pages.

Good catch.

--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Re: [RFC v2] arm:extend the reserved mrmory for initrd to be page aligned

[Catalin Marinas]

On Mon, Sep 15, 2014 at 12:33:25PM +0100, Russell King - ARM Linux wrote:
> On Mon, Sep 15, 2014 at 07:07:20PM +0800, Wang, Yalin wrote:
> > @@ -636,6 +646,11 @@ static int keep_initrd;
> >  void free_initrd_mem(unsigned long start, unsigned long end)
> >  {
> >  	if (!keep_initrd) {
> > +		if (start == initrd_start)
> > +			start = round_down(start, PAGE_SIZE);
> > +		if (end == initrd_end)
> > +			end = round_up(end, PAGE_SIZE);
> > +
> >  		poison_init_mem((void *)start, PAGE_ALIGN(end) - start);
> >  		free_reserved_area((void *)start, (void *)end, -1, "initrd");
> >  	}
> 
> is the only bit of code you likely need to achieve your goal.
> 
> Thinking about this, I think that you are quite right to align these.
> The memory around the initrd is defined to be system memory, and we
> already free the pages around it, so it *is* wrong not to free the
> partial initrd pages.

Actually, I think we have a problem, at least on arm64 (raised by Peter
Maydell). There is no guarantee that the page around start/end of initrd
is free, it may contain the dtb for example. This is even more obvious
when we have a 64KB page kernel (the boot loader doesn't know the page
size that the kernel is going to use).

The bug was there before as we had poison_init_mem() already (not it
disappeared since free_reserved_area does the poisoning).

So as a quick fix I think we need the rounding the other way (and in the
general case we probably lose a page at the end of initrd):

diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index 494297c698ca..39fd080683e7 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -335,9 +335,9 @@ void free_initrd_mem(unsigned long start, unsigned long end)
 {
 	if (!keep_initrd) {
 		if (start == initrd_start)
-			start = round_down(start, PAGE_SIZE);
+			start = round_up(start, PAGE_SIZE);
 		if (end == initrd_end)
-			end = round_up(end, PAGE_SIZE);
+			end = round_down(end, PAGE_SIZE);
 
 		free_reserved_area((void *)start, (void *)end, 0, "initrd");
 	}

A better fix would be to check what else is around the start/end of
initrd.

-- 
Catalin

RE: [RFC v2] arm:extend the reserved mrmory for initrd to be page aligned

[Wang, Yalin]

> -----Original Message-----
> From: Catalin Marinas [mailto:catalin.marinas@arm.com]
> Sent: Thursday, December 04, 2014 8:03 PM
> To: Russell King - ARM Linux
> Cc: Wang, Yalin; 'linux-mm@kvack.org'; Will Deacon; 'linux-
> kernel@vger.kernel.org'; 'linux-arm-kernel@lists.infradead.org'; 'linux-
> arm-msm@vger.kernel.org'; Peter Maydell
> Subject: Re: [RFC v2] arm:extend the reserved mrmory for initrd to be page
> aligned
> 
> On Mon, Sep 15, 2014 at 12:33:25PM +0100, Russell King - ARM Linux wrote:
> > On Mon, Sep 15, 2014 at 07:07:20PM +0800, Wang, Yalin wrote:
> > > @@ -636,6 +646,11 @@ static int keep_initrd;  void
> > > free_initrd_mem(unsigned long start, unsigned long end)  {
> > >  	if (!keep_initrd) {
> > > +		if (start == initrd_start)
> > > +			start = round_down(start, PAGE_SIZE);
> > > +		if (end == initrd_end)
> > > +			end = round_up(end, PAGE_SIZE);
> > > +
> > >  		poison_init_mem((void *)start, PAGE_ALIGN(end) - start);
> > >  		free_reserved_area((void *)start, (void *)end, -1, "initrd");
> > >  	}
> >
> > is the only bit of code you likely need to achieve your goal.
> >
> > Thinking about this, I think that you are quite right to align these.
> > The memory around the initrd is defined to be system memory, and we
> > already free the pages around it, so it *is* wrong not to free the
> > partial initrd pages.
> 
> Actually, I think we have a problem, at least on arm64 (raised by Peter
> Maydell). There is no guarantee that the page around start/end of initrd is
> free, it may contain the dtb for example. This is even more obvious when we
> have a 64KB page kernel (the boot loader doesn't know the page size that
> the kernel is going to use).
> 
> The bug was there before as we had poison_init_mem() already (not it
> disappeared since free_reserved_area does the poisoning).
> 
> So as a quick fix I think we need the rounding the other way (and in the
> general case we probably lose a page at the end of initrd):
> 
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c index
> 494297c698ca..39fd080683e7 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -335,9 +335,9 @@ void free_initrd_mem(unsigned long start, unsigned long
> end)  {
>  	if (!keep_initrd) {
>  		if (start == initrd_start)
> -			start = round_down(start, PAGE_SIZE);
> +			start = round_up(start, PAGE_SIZE);
>  		if (end == initrd_end)
> -			end = round_up(end, PAGE_SIZE);
> +			end = round_down(end, PAGE_SIZE);
> 
>  		free_reserved_area((void *)start, (void *)end, 0, "initrd");
>  	}
> 
> A better fix would be to check what else is around the start/end of initrd.
I think a better way is add some head info in Image header,
So that bootloader  can know the kernel CONFIG_PAGE_SIZE ,
For example we can add PAGE_SIZE in zImage header .
How about this way?

Re: [RFC v2] arm:extend the reserved mrmory for initrd to be page aligned

[Will Deacon]

On Thu, Dec 04, 2014 at 12:03:05PM +0000, Catalin Marinas wrote:
> On Mon, Sep 15, 2014 at 12:33:25PM +0100, Russell King - ARM Linux wrote:
> > On Mon, Sep 15, 2014 at 07:07:20PM +0800, Wang, Yalin wrote:
> > > @@ -636,6 +646,11 @@ static int keep_initrd;
> > >  void free_initrd_mem(unsigned long start, unsigned long end)
> > >  {
> > >  	if (!keep_initrd) {
> > > +		if (start == initrd_start)
> > > +			start = round_down(start, PAGE_SIZE);
> > > +		if (end == initrd_end)
> > > +			end = round_up(end, PAGE_SIZE);
> > > +
> > >  		poison_init_mem((void *)start, PAGE_ALIGN(end) - start);
> > >  		free_reserved_area((void *)start, (void *)end, -1, "initrd");
> > >  	}
> > 
> > is the only bit of code you likely need to achieve your goal.
> > 
> > Thinking about this, I think that you are quite right to align these.
> > The memory around the initrd is defined to be system memory, and we
> > already free the pages around it, so it *is* wrong not to free the
> > partial initrd pages.
> 
> Actually, I think we have a problem, at least on arm64 (raised by Peter
> Maydell). There is no guarantee that the page around start/end of initrd
> is free, it may contain the dtb for example. This is even more obvious
> when we have a 64KB page kernel (the boot loader doesn't know the page
> size that the kernel is going to use).
> 
> The bug was there before as we had poison_init_mem() already (not it
> disappeared since free_reserved_area does the poisoning).
> 
> So as a quick fix I think we need the rounding the other way (and in the
> general case we probably lose a page at the end of initrd):
> 
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index 494297c698ca..39fd080683e7 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -335,9 +335,9 @@ void free_initrd_mem(unsigned long start, unsigned long end)
>  {
>  	if (!keep_initrd) {
>  		if (start == initrd_start)
> -			start = round_down(start, PAGE_SIZE);
> +			start = round_up(start, PAGE_SIZE);
>  		if (end == initrd_end)
> -			end = round_up(end, PAGE_SIZE);
> +			end = round_down(end, PAGE_SIZE);
>  
>  		free_reserved_area((void *)start, (void *)end, 0, "initrd");
>  	}
> 
> A better fix would be to check what else is around the start/end of
> initrd.

Care to submit this as a proper patch? We should at least fix Peter's issue
before doing things like extending headers, which won't work for older
kernels anyway.

Will

Re: [RFC v2] arm:extend the reserved mrmory for initrd to be page aligned

[Catalin Marinas]

On Fri, Dec 05, 2014 at 02:35:29AM +0000, Wang, Yalin wrote:
> > -----Original Message-----
> > From: Catalin Marinas [mailto:catalin.marinas@arm.com]
> > Sent: Thursday, December 04, 2014 8:03 PM
> > To: Russell King - ARM Linux
> > Cc: Wang, Yalin; 'linux-mm@kvack.org'; Will Deacon; 'linux-
> > kernel@vger.kernel.org'; 'linux-arm-kernel@lists.infradead.org'; 'linux-
> > arm-msm@vger.kernel.org'; Peter Maydell
> > Subject: Re: [RFC v2] arm:extend the reserved mrmory for initrd to be page
> > aligned
> > 
> > On Mon, Sep 15, 2014 at 12:33:25PM +0100, Russell King - ARM Linux wrote:
> > > On Mon, Sep 15, 2014 at 07:07:20PM +0800, Wang, Yalin wrote:
> > > > @@ -636,6 +646,11 @@ static int keep_initrd;  void
> > > > free_initrd_mem(unsigned long start, unsigned long end)  {
> > > >  	if (!keep_initrd) {
> > > > +		if (start == initrd_start)
> > > > +			start = round_down(start, PAGE_SIZE);
> > > > +		if (end == initrd_end)
> > > > +			end = round_up(end, PAGE_SIZE);
> > > > +
> > > >  		poison_init_mem((void *)start, PAGE_ALIGN(end) - start);
> > > >  		free_reserved_area((void *)start, (void *)end, -1, "initrd");
> > > >  	}
> > >
> > > is the only bit of code you likely need to achieve your goal.
> > >
> > > Thinking about this, I think that you are quite right to align these.
> > > The memory around the initrd is defined to be system memory, and we
> > > already free the pages around it, so it *is* wrong not to free the
> > > partial initrd pages.
> > 
> > Actually, I think we have a problem, at least on arm64 (raised by Peter
> > Maydell). There is no guarantee that the page around start/end of initrd is
> > free, it may contain the dtb for example. This is even more obvious when we
> > have a 64KB page kernel (the boot loader doesn't know the page size that
> > the kernel is going to use).
> > 
> > The bug was there before as we had poison_init_mem() already (not it
> > disappeared since free_reserved_area does the poisoning).
> > 
> > So as a quick fix I think we need the rounding the other way (and in the
> > general case we probably lose a page at the end of initrd):
> > 
> > diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c index
> > 494297c698ca..39fd080683e7 100644
> > --- a/arch/arm64/mm/init.c
> > +++ b/arch/arm64/mm/init.c
> > @@ -335,9 +335,9 @@ void free_initrd_mem(unsigned long start, unsigned long
> > end)  {
> >  	if (!keep_initrd) {
> >  		if (start == initrd_start)
> > -			start = round_down(start, PAGE_SIZE);
> > +			start = round_up(start, PAGE_SIZE);
> >  		if (end == initrd_end)
> > -			end = round_up(end, PAGE_SIZE);
> > +			end = round_down(end, PAGE_SIZE);
> > 
> >  		free_reserved_area((void *)start, (void *)end, 0, "initrd");
> >  	}
> > 
> > A better fix would be to check what else is around the start/end of initrd.
> I think a better way is add some head info in Image header,
> So that bootloader  can know the kernel CONFIG_PAGE_SIZE ,
> For example we can add PAGE_SIZE in zImage header .
> How about this way?

The problem is that we don't know how many boot loaders are affected. We
could simply mandate in booting.txt that the dtb and initrd are not
closer than 64KB but we have the same issue, existing boot loaders.

-- 
Catalin

Re: [RFC v2] arm:extend the reserved mrmory for initrd to be page aligned

[Catalin Marinas]

On Fri, Dec 05, 2014 at 12:05:06PM +0000, Will Deacon wrote:
> On Thu, Dec 04, 2014 at 12:03:05PM +0000, Catalin Marinas wrote:
> > On Mon, Sep 15, 2014 at 12:33:25PM +0100, Russell King - ARM Linux wrote:
> > > On Mon, Sep 15, 2014 at 07:07:20PM +0800, Wang, Yalin wrote:
> > > > @@ -636,6 +646,11 @@ static int keep_initrd;
> > > >  void free_initrd_mem(unsigned long start, unsigned long end)
> > > >  {
> > > >  	if (!keep_initrd) {
> > > > +		if (start == initrd_start)
> > > > +			start = round_down(start, PAGE_SIZE);
> > > > +		if (end == initrd_end)
> > > > +			end = round_up(end, PAGE_SIZE);
> > > > +
> > > >  		poison_init_mem((void *)start, PAGE_ALIGN(end) - start);
> > > >  		free_reserved_area((void *)start, (void *)end, -1, "initrd");
> > > >  	}
> > > 
> > > is the only bit of code you likely need to achieve your goal.
> > > 
> > > Thinking about this, I think that you are quite right to align these.
> > > The memory around the initrd is defined to be system memory, and we
> > > already free the pages around it, so it *is* wrong not to free the
> > > partial initrd pages.
> > 
> > Actually, I think we have a problem, at least on arm64 (raised by Peter
> > Maydell). There is no guarantee that the page around start/end of initrd
> > is free, it may contain the dtb for example. This is even more obvious
> > when we have a 64KB page kernel (the boot loader doesn't know the page
> > size that the kernel is going to use).
> > 
> > The bug was there before as we had poison_init_mem() already (not it
> > disappeared since free_reserved_area does the poisoning).
> > 
> > So as a quick fix I think we need the rounding the other way (and in the
> > general case we probably lose a page at the end of initrd):
> > 
> > diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> > index 494297c698ca..39fd080683e7 100644
> > --- a/arch/arm64/mm/init.c
> > +++ b/arch/arm64/mm/init.c
> > @@ -335,9 +335,9 @@ void free_initrd_mem(unsigned long start, unsigned long end)
> >  {
> >  	if (!keep_initrd) {
> >  		if (start == initrd_start)
> > -			start = round_down(start, PAGE_SIZE);
> > +			start = round_up(start, PAGE_SIZE);
> >  		if (end == initrd_end)
> > -			end = round_up(end, PAGE_SIZE);
> > +			end = round_down(end, PAGE_SIZE);
> >  
> >  		free_reserved_area((void *)start, (void *)end, 0, "initrd");
> >  	}
> > 
> > A better fix would be to check what else is around the start/end of
> > initrd.
> 
> Care to submit this as a proper patch? We should at least fix Peter's issue
> before doing things like extending headers, which won't work for older
> kernels anyway.

Quick fix is the revert of the whole patch, together with removing
PAGE_ALIGN(end) in poison_init_mem() on arm32. If Russell is ok with
this patch, we can take it via the arm64 tree, otherwise I'll send you a
partial revert only for the arm64 part.

-------------8<-----------------------

>From 8e317c6be00abe280de4dcdd598d2e92009174b6 Mon Sep 17 00:00:00 2001
From: Catalin Marinas <catalin.marinas@arm.com>
Date: Fri, 5 Dec 2014 16:41:52 +0000
Subject: [PATCH] Revert "ARM: 8167/1: extend the reserved memory for initrd to
 be page aligned"

This reverts commit 421520ba98290a73b35b7644e877a48f18e06004. There is
no guarantee that the boot-loader places other images like dtb in a
different page than initrd start/end. When this happens, such pages must
not be freed. The free_reserved_area() already takes care of rounding up
"start" and rounding down "end" to avoid freeing partially used pages.

In addition to the revert, this patch also removes the arm32
PAGE_ALIGN(end) when calculating the size of the memory to be poisoned.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Reported-by: Peter Maydell <Peter.Maydell@arm.com>
Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
Cc: <stable@vger.kernel.org> # 3.17+
---
 arch/arm/mm/init.c   | 7 +------
 arch/arm64/mm/init.c | 8 +-------
 2 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 92bba32d9230..108d6949c727 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -636,12 +636,7 @@ static int keep_initrd;
 void free_initrd_mem(unsigned long start, unsigned long end)
 {
 	if (!keep_initrd) {
-		if (start == initrd_start)
-			start = round_down(start, PAGE_SIZE);
-		if (end == initrd_end)
-			end = round_up(end, PAGE_SIZE);
-
-		poison_init_mem((void *)start, PAGE_ALIGN(end) - start);
+		poison_init_mem((void *)start, end - start);
 		free_reserved_area((void *)start, (void *)end, -1, "initrd");
 	}
 }
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index 494297c698ca..fff81f02251c 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -333,14 +333,8 @@ static int keep_initrd;
 
 void free_initrd_mem(unsigned long start, unsigned long end)
 {
-	if (!keep_initrd) {
-		if (start == initrd_start)
-			start = round_down(start, PAGE_SIZE);
-		if (end == initrd_end)
-			end = round_up(end, PAGE_SIZE);
-
+	if (!keep_initrd)
 		free_reserved_area((void *)start, (void *)end, 0, "initrd");
-	}
 }
 
 static int __init keepinitrd_setup(char *__unused)

Re: [RFC v2] arm:extend the reserved mrmory for initrd to be page aligned

[Russell King - ARM Linux]

On Fri, Dec 05, 2014 at 05:07:45PM +0000, Catalin Marinas wrote:
> On Fri, Dec 05, 2014 at 12:05:06PM +0000, Will Deacon wrote:
> > Care to submit this as a proper patch? We should at least fix Peter's issue
> > before doing things like extending headers, which won't work for older
> > kernels anyway.
> 
> Quick fix is the revert of the whole patch, together with removing
> PAGE_ALIGN(end) in poison_init_mem() on arm32. If Russell is ok with
> this patch, we can take it via the arm64 tree, otherwise I'll send you a
> partial revert only for the arm64 part.

Not really.  Let's look at the history.

For years, we've been poisoning memory, page aligning the end pointer.
This has never been an issue.

However, patch 8167/1 changed things so we freed the overlapping pages.
Since we've always poisoned up to the end of the final page, freeing it
should not be a problem, especially as (I said above) we've been poisoning
it for years.

The issue is more about what happens at the start.

In any case:

> >From 8e317c6be00abe280de4dcdd598d2e92009174b6 Mon Sep 17 00:00:00 2001
> From: Catalin Marinas <catalin.marinas@arm.com>
> Date: Fri, 5 Dec 2014 16:41:52 +0000
> Subject: [PATCH] Revert "ARM: 8167/1: extend the reserved memory for initrd to
>  be page aligned"
> 
> This reverts commit 421520ba98290a73b35b7644e877a48f18e06004. There is
> no guarantee that the boot-loader places other images like dtb in a
> different page than initrd start/end. When this happens, such pages must
> not be freed. The free_reserved_area() already takes care of rounding up
> "start" and rounding down "end" to avoid freeing partially used pages.
> 
> In addition to the revert, this patch also removes the arm32
> PAGE_ALIGN(end) when calculating the size of the memory to be poisoned.

which makes the summary line rather misleading, and I really don't think
we need to do this on ARM for the simple reason that we've been doing it
for soo long that it can't be an issue.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Re: [RFC v2] arm:extend the reserved mrmory for initrd to be page aligned

[Peter Maydell]

On 5 December 2014 at 17:27, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Dec 05, 2014 at 05:07:45PM +0000, Catalin Marinas wrote:
>> On Fri, Dec 05, 2014 at 12:05:06PM +0000, Will Deacon wrote:
>> > Care to submit this as a proper patch? We should at least fix Peter's issue
>> > before doing things like extending headers, which won't work for older
>> > kernels anyway.
>>
>> Quick fix is the revert of the whole patch, together with removing
>> PAGE_ALIGN(end) in poison_init_mem() on arm32. If Russell is ok with
>> this patch, we can take it via the arm64 tree, otherwise I'll send you a
>> partial revert only for the arm64 part.
>
> Not really.  Let's look at the history.
>
> For years, we've been poisoning memory, page aligning the end pointer.
> This has never been an issue.

Depends what you mean by "never been an issue". I had to change
QEMU (commit 98ed805c, January 2013) for 32-bit ARM back when the
kernel started trashing the tail end of the page after the initrd
with the poisoning, to 4K-align the dtb so it didn't share a page
with the initrd-tail. That nobody else complained suggests that most
bootloaders don't in practice overlap the two, though (ie that
QEMU is an outlier in how it chooses to arrange things in memory).

I should probably have reported the breakage at the time, but
I took the pragmatic (lazy?) approach of just changing our
bootloader code.

thanks
-- PMM

Re: [RFC v2] arm:extend the reserved mrmory for initrd to be page aligned

[Catalin Marinas]

On Fri, Dec 05, 2014 at 05:27:02PM +0000, Russell King - ARM Linux wrote:
> On Fri, Dec 05, 2014 at 05:07:45PM +0000, Catalin Marinas wrote:
> > From 8e317c6be00abe280de4dcdd598d2e92009174b6 Mon Sep 17 00:00:00 2001
> > From: Catalin Marinas <catalin.marinas@arm.com>
> > Date: Fri, 5 Dec 2014 16:41:52 +0000
> > Subject: [PATCH] Revert "ARM: 8167/1: extend the reserved memory for initrd to
> >  be page aligned"
> > 
> > This reverts commit 421520ba98290a73b35b7644e877a48f18e06004. There is
> > no guarantee that the boot-loader places other images like dtb in a
> > different page than initrd start/end. When this happens, such pages must
> > not be freed. The free_reserved_area() already takes care of rounding up
> > "start" and rounding down "end" to avoid freeing partially used pages.
> > 
> > In addition to the revert, this patch also removes the arm32
> > PAGE_ALIGN(end) when calculating the size of the memory to be poisoned.
> 
> which makes the summary line rather misleading, and I really don't think
> we need to do this on ARM for the simple reason that we've been doing it
> for soo long that it can't be an issue.

I started this as a revert and then realised that it doesn't solve
anything for arm32 without changing the poisoning.

Anyway, if you are happy with how it is, I'll drop the arm32 part. As I
said yesterday, the issue is worse for arm64 with 64K pages.

-- 
Catalin

Re: [RFC v2] arm:extend the reserved mrmory for initrd to be page aligned

[Peter Maydell]

On 5 December 2014 at 18:44, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Fri, Dec 05, 2014 at 05:27:02PM +0000, Russell King - ARM Linux wrote:
>> which makes the summary line rather misleading, and I really don't think
>> we need to do this on ARM for the simple reason that we've been doing it
>> for soo long that it can't be an issue.
>
> I started this as a revert and then realised that it doesn't solve
> anything for arm32 without changing the poisoning.
>
> Anyway, if you are happy with how it is, I'll drop the arm32 part. As I
> said yesterday, the issue is worse for arm64 with 64K pages.

If you do want to retain the arm32 "mustn't be in the 4K page of
the initrd tail" behaviour then it would probably be a good idea
to document this in the Booting spec.

thanks
-- PMM