| Message ID | 20141205170745.GA31222@e104818-lin.cambridge.arm.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)