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
> -----Original Message-----
> From: Catalin Marinas [mailto:[email protected]]
> Sent: Thursday, December 04, 2014 8:03 PM
> To: Russell King - ARM Linux
> Cc: Wang, Yalin; '[email protected]'; Will Deacon; 'linux-
> [email protected]'; '[email protected]'; 'linux-
> [email protected]'; 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?
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
On Fri, Dec 05, 2014 at 02:35:29AM +0000, Wang, Yalin wrote:
> > -----Original Message-----
> > From: Catalin Marinas [mailto:[email protected]]
> > Sent: Thursday, December 04, 2014 8:03 PM
> > To: Russell King - ARM Linux
> > Cc: Wang, Yalin; '[email protected]'; Will Deacon; 'linux-
> > [email protected]'; '[email protected]'; 'linux-
> > [email protected]'; 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
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 <[email protected]>
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 <[email protected]>
Reported-by: Peter Maydell <[email protected]>
Cc: Russell King - ARM Linux <[email protected]>
Cc: <[email protected]> # 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)
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 <[email protected]>
> 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.
On 5 December 2014 at 17:27, Russell King - ARM Linux
<[email protected]> 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
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 <[email protected]>
> > 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
On 5 December 2014 at 18:44, Catalin Marinas <[email protected]> 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