[PATCH 0/2] improve debugability of early boot of the kernel

[PATCH 0/2] improve debugability of early boot of the kernel

[Nicolas Pitre]

Especially with the recent rework of static mappings, some people are
stuck with non booting kernels and very little to help them debug it
by themselves.  Turns out that this can be remedied with a patch I posted
a couple months ago, however this version is much improved.

This depends on commit 94e5a85b3be0 "ARM: earlier initialization of vectors
page" that can be found in Russell's tree.

[PATCH 1/2] DEBUG_LL: limit early mapping to the minimum
[PATCH 2/2] remove the debugging restrictions in devicemaps_init()


Nicolas

[PATCH 1/2] DEBUG_LL: limit early mapping to the minimum

[Nicolas Pitre]

There is just no point mapping up to 512MB for a serial port.
Using a single 1MB entry is way sufficient for all users.

Signed-off-by: Nicolas Pitre <nico@linaro.org>
---
 arch/arm/kernel/head.S |    9 +--------
 1 files changed, 1 insertions(+), 8 deletions(-)

diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S
index 3bf0c7f8b0..835898e7d7 100644
--- a/arch/arm/kernel/head.S
+++ b/arch/arm/kernel/head.S
@@ -277,10 +277,6 @@ __create_page_tables:
 	mov	r3, r3, lsl #PMD_ORDER
 
 	add	r0, r4, r3
-	rsb	r3, r3, #0x4000			@ PTRS_PER_PGD*sizeof(long)
-	cmp	r3, #0x0800			@ limit to 512MB
-	movhi	r3, #0x0800
-	add	r6, r0, r3
 	mov	r3, r7, lsr #SECTION_SHIFT
 	ldr	r7, [r10, #PROCINFO_IO_MMUFLAGS] @ io_mmuflags
 	orr	r3, r7, r3, lsl #SECTION_SHIFT
@@ -289,13 +285,10 @@ __create_page_tables:
 #else
 	orr	r3, r3, #PMD_SECT_XN
 #endif
-1:	str	r3, [r0], #4
+	str	r3, [r0], #4
 #ifdef CONFIG_ARM_LPAE
 	str	r7, [r0], #4
 #endif
-	add	r3, r3, #1 << SECTION_SHIFT
-	cmp	r0, r6
-	blo	1b
 
 #else /* CONFIG_DEBUG_ICEDCC || CONFIG_DEBUG_SEMIHOSTING */
 	/* we don't need any serial debugging mappings */
-- 
1.7.9.rc2

[PATCH 2/2] remove the debugging restrictions in devicemaps_init()

[Nicolas Pitre]

More code is getting involved through devicemaps_init() these days,
including calls to printk() or BUG().  Not being able to access any
device to print out debugging information and only being presented with
a silent kernel crash in that case has become a major inconvenience
lately.

By having the active page table separate from the one being initialized,
it is possible to preserve the initial debug mapping that was set in
head.S until the final page table is ready.  Because there exists some
code that installs a partial mapping in order to probe the hardware
before installing additional mappings, it is necessary to set up the
vector mapping early which enables the fault handler to copy some of
the newly created mappings into our page table copy as needed.

This patch implements such a temporary page table copy only for
non LPAE configurations at the moment.

Signed-off-by: Nicolas Pitre <nico@linaro.org>
---
 arch/arm/mm/mmu.c |   95 +++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 71 insertions(+), 24 deletions(-)

diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index f77f1dbbdf..80e3a5410f 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -987,16 +987,47 @@ void __init arm_mm_memblock_reserve(void)
 #endif
 }
 
+#if defined(CONFIG_DEBUG_LL) && !defined(CONFIG_ARM_LPAE)
+
+/*
+ * The debug mappings will be cleared from the initial page table by
+ * devicemaps_init(), and eventually recreated via mdesc->map_io().
+ * To allow for debug devices to always remain accessible, we switch to
+ * a temporary copy of the current page table while the final one is being
+ * manipulated, and switch back once the final mappings are in place.
+ */
+static pgd_t * __init install_temp_mm(void)
+{
+	pgd_t *temp_pgd = early_alloc(PTRS_PER_PGD * sizeof(pgd_t));
+	pgd_t *init_pgd = pgd_offset_k(0);
+
+	/* copy vector and kernel space mappings */
+	pgd_val(temp_pgd[0]) = pgd_val(init_pgd[0]);
+	memcpy(temp_pgd + USER_PTRS_PER_PGD, init_pgd + USER_PTRS_PER_PGD,
+	       (PTRS_PER_PGD - USER_PTRS_PER_PGD) * sizeof(pgd_t));
+	clean_dcache_area(temp_pgd, PTRS_PER_PGD * sizeof(pgd_t));
+	cpu_switch_mm(temp_pgd, &init_mm);
+	return temp_pgd;
+}
+
+static void __init remove_temp_mm(pgd_t *temp_pgd)
+{
+	cpu_switch_mm(init_mm.pgd, &init_mm);
+	memblock_free(__pa(temp_pgd), PTRS_PER_PGD * sizeof(pgd_t));
+}
+
+#else
+#define install_temp_mm()	(NULL)
+#define remove_temp_mm(mm)	do { (void)(mm); } while (0)
+#endif
+
 /*
- * Set up the device mappings.  Since we clear out the page tables for all
- * mappings above VMALLOC_START, we will remove any debug device mappings.
- * This means you have to be careful how you debug this function, or any
- * called function.  This means you can't use any function or debugging
- * method which may touch any device, otherwise the kernel _will_ crash.
+ * Set up the device mappings.
  */
 static void __init devicemaps_init(struct machine_desc *mdesc)
 {
 	struct map_desc map;
+	pgd_t *temp_pgd;
 	unsigned long addr;
 	void *vectors;
 
@@ -1004,10 +1035,41 @@ static void __init devicemaps_init(struct machine_desc *mdesc)
 	 * Allocate the vector page early.
 	 */
 	vectors = early_alloc(PAGE_SIZE);
-
 	early_trap_init(vectors);
 
-	for (addr = VMALLOC_START; addr; addr += PMD_SIZE)
+	/*
+	 * Create a mapping for the machine vectors at the high-vectors
+	 * location (0xffff0000).  If we aren't using high-vectors, also
+	 * create a mapping at the low-vectors virtual address.
+	 */
+	pmd_clear(pmd_off_k(0xffff0000));
+	map.pfn = __phys_to_pfn(virt_to_phys(vectors));
+	map.virtual = 0xffff0000;
+	map.length = PAGE_SIZE;
+	map.type = MT_HIGH_VECTORS;
+	create_mapping(&map);
+
+	if (!vectors_high()) {
+		map.virtual = 0;
+		map.type = MT_LOW_VECTORS;
+		create_mapping(&map);
+	}
+
+	/*
+	 * After this point, any missing entry in our temp mm will be
+	 * populated via do_translation_fault().  This may happen if
+	 * some platform code needs to install a partial mapping to
+	 * probe the hardware in order to install more mappings.
+	 */
+	temp_pgd = install_temp_mm();
+
+	/*
+	 * Clear out the page tables for all mappings above VMALLOC_START
+	 * while preserving the high vector mapping.
+	 */
+	for (addr = VMALLOC_START; 
+	     addr < (0xffff0000UL & PMD_MASK);
+	     addr += PMD_SIZE)
 		pmd_clear(pmd_off_k(addr));
 
 	/*
@@ -1041,28 +1103,13 @@ static void __init devicemaps_init(struct machine_desc *mdesc)
 #endif
 
 	/*
-	 * Create a mapping for the machine vectors at the high-vectors
-	 * location (0xffff0000).  If we aren't using high-vectors, also
-	 * create a mapping@the low-vectors virtual address.
-	 */
-	map.pfn = __phys_to_pfn(virt_to_phys(vectors));
-	map.virtual = 0xffff0000;
-	map.length = PAGE_SIZE;
-	map.type = MT_HIGH_VECTORS;
-	create_mapping(&map);
-
-	if (!vectors_high()) {
-		map.virtual = 0;
-		map.type = MT_LOW_VECTORS;
-		create_mapping(&map);
-	}
-
-	/*
 	 * Ask the machine support to map in the statically mapped devices.
 	 */
 	if (mdesc->map_io)
 		mdesc->map_io();
 
+	remove_temp_mm(temp_pgd);
+
 	/*
 	 * Finally flush the caches and tlb to ensure that we're in a
 	 * consistent state wrt the writebuffer.  This also ensures that
-- 
1.7.9.rc2

[PATCH 2/2] remove the debugging restrictions in devicemaps_init()

[Russell King - ARM Linux]

On Fri, Mar 16, 2012 at 01:44:28AM -0400, Nicolas Pitre wrote:
> More code is getting involved through devicemaps_init() these days,
> including calls to printk() or BUG().  Not being able to access any
> device to print out debugging information and only being presented with
> a silent kernel crash in that case has become a major inconvenience
> lately.
> 
> By having the active page table separate from the one being initialized,
> it is possible to preserve the initial debug mapping that was set in
> head.S until the final page table is ready.  Because there exists some
> code that installs a partial mapping in order to probe the hardware
> before installing additional mappings, it is necessary to set up the
> vector mapping early which enables the fault handler to copy some of
> the newly created mappings into our page table copy as needed.
> 
> This patch implements such a temporary page table copy only for
> non LPAE configurations at the moment.

I've been thinking about this on and off since you posted it, and I'm now
convinced this will cause regressions.  You've probably forgotten about
those CPUs which need to read data in order to clear unwanted cache lines
from their cache.

This patch would cause all such CPUs to freeze when switching to or from
the temporary mapping, because neither page table will contain the necessary
mappings to allow those accesses to succeed.

So I don't think this approach can work.

[PATCH 2/2] remove the debugging restrictions in devicemaps_init()

[Nicolas Pitre]

On Sun, 6 May 2012, Russell King - ARM Linux wrote:

> On Fri, Mar 16, 2012 at 01:44:28AM -0400, Nicolas Pitre wrote:
> > More code is getting involved through devicemaps_init() these days,
> > including calls to printk() or BUG().  Not being able to access any
> > device to print out debugging information and only being presented with
> > a silent kernel crash in that case has become a major inconvenience
> > lately.
> > 
> > By having the active page table separate from the one being initialized,
> > it is possible to preserve the initial debug mapping that was set in
> > head.S until the final page table is ready.  Because there exists some
> > code that installs a partial mapping in order to probe the hardware
> > before installing additional mappings, it is necessary to set up the
> > vector mapping early which enables the fault handler to copy some of
> > the newly created mappings into our page table copy as needed.
> > 
> > This patch implements such a temporary page table copy only for
> > non LPAE configurations at the moment.
> 
> I've been thinking about this on and off since you posted it, and I'm now
> convinced this will cause regressions.  You've probably forgotten about
> those CPUs which need to read data in order to clear unwanted cache lines
> from their cache.

Hmmm... I didn't forget them, but I might not have thought those cases 
all the way through.

Those CPUs I know about with a need to read memory to clear cache lines 
are either StrongARM based or XScale based.  But in those cases it is 
only when cleaning up the whole cache that the process of filling the 
cache with dummy lines is involved.

> This patch would cause all such CPUs to freeze when switching to or from
> the temporary mapping, because neither page table will contain the necessary
> mappings to allow those accesses to succeed.

However, the switching itself doesn't involve a full cache flush.  Only 
clean_dcache_area() is involved when installing the temporary mapping, 
and that translates into cpu_dcache_clean_area() which on all those CPUs 
I previously listed is implemented with a loop of individual cache line 
clean operations.  Switching back to the real mapping doesn't involve 
any cache ops since a full cache flush is performed anyway once the new 
mapping is in place.  TLBs may not be incoherent at that point since 
both mappings are not conflicting with each other.

Therefore I don't think there should be any issues even on those CPUs, 
unless I've overlooked something.

Maybe you could test this on your Assabet simply by adding a dummy 
printk() in create_mapping() once this patch is applied?


Nicolas

[PATCH 2/2] remove the debugging restrictions in devicemaps_init()

[Russell King - ARM Linux]

On Tue, May 08, 2012 at 12:19:30AM -0400, Nicolas Pitre wrote:
> However, the switching itself doesn't involve a full cache flush.

That's where you're wrong.  Look at the switch_mm() helpers in proc-*.S.
StrongARM for example calls v4wb_flush_kern_cache_all() here, which needs
the cache flushing mappings to be in place in the _current_ page table.

[PATCH 2/2] remove the debugging restrictions in devicemaps_init()

[Nicolas Pitre]

On Tue, 8 May 2012, Russell King - ARM Linux wrote:

> On Tue, May 08, 2012 at 12:19:30AM -0400, Nicolas Pitre wrote:
> > However, the switching itself doesn't involve a full cache flush.
> 
> That's where you're wrong.  Look at the switch_mm() helpers in proc-*.S.
> StrongARM for example calls v4wb_flush_kern_cache_all() here, which needs
> the cache flushing mappings to be in place in the _current_ page table.

Right.  I had to be wrong somewhere.

What about abstracting the cache area mapping code in a function of its 
own, and calling it inside install_temp_mm() as well?


Nicolas

[PATCH 2/2] remove the debugging restrictions in devicemaps_init()

[Russell King - ARM Linux]

On Tue, May 08, 2012 at 12:17:40PM -0400, Nicolas Pitre wrote:
> On Tue, 8 May 2012, Russell King - ARM Linux wrote:
> 
> > On Tue, May 08, 2012 at 12:19:30AM -0400, Nicolas Pitre wrote:
> > > However, the switching itself doesn't involve a full cache flush.
> > 
> > That's where you're wrong.  Look at the switch_mm() helpers in proc-*.S.
> > StrongARM for example calls v4wb_flush_kern_cache_all() here, which needs
> > the cache flushing mappings to be in place in the _current_ page table.
> 
> Right.  I had to be wrong somewhere.
> 
> What about abstracting the cache area mapping code in a function of its 
> own, and calling it inside install_temp_mm() as well?

One of the issues is that we check that the mappings don't conflict -
in other words, a section mapping doesn't conflict with a page table
mapping.  We need a clean page table to start with before we start
populating it with the IO entries, so that we know there's no mappings
in IO space to conflict with the newly setup entries.

What we could do is allocate a new page table, use that page table to
setup the new mappings, memcpy them to the swapper page table as a
final step before we kick the caches and TLB.

The down side to that approach is we have people abusing the map_io
method for other purposes (people - including you - just don't listen
when I tell them that they shouldn't do this kind of stuff) such as
setting up mappings which they then immediately access.  (In case
you've forgotten, Assabet, the probing of the SCR register at map_io
time to discover the platforms configuration.  That set the precident
and now other platforms also do this kind of thing.)

So, I don't think there's any easy solution to this - certainly not
one which is going to be fragile or restricted to a small set of
configurations.

Overall, I'm not sure it's worth trying to solve this.

[PATCH 2/2] remove the debugging restrictions in devicemaps_init()

[Nicolas Pitre]

On Sat, 12 May 2012, Russell King - ARM Linux wrote:

> On Tue, May 08, 2012 at 12:17:40PM -0400, Nicolas Pitre wrote:
> > On Tue, 8 May 2012, Russell King - ARM Linux wrote:
> > 
> > > On Tue, May 08, 2012 at 12:19:30AM -0400, Nicolas Pitre wrote:
> > > > However, the switching itself doesn't involve a full cache flush.
> > > 
> > > That's where you're wrong.  Look at the switch_mm() helpers in proc-*.S.
> > > StrongARM for example calls v4wb_flush_kern_cache_all() here, which needs
> > > the cache flushing mappings to be in place in the _current_ page table.
> > 
> > Right.  I had to be wrong somewhere.
> > 
> > What about abstracting the cache area mapping code in a function of its 
> > own, and calling it inside install_temp_mm() as well?
> 
> One of the issues is that we check that the mappings don't conflict -
> in other words, a section mapping doesn't conflict with a page table
> mapping.  We need a clean page table to start with before we start
> populating it with the IO entries, so that we know there's no mappings
> in IO space to conflict with the newly setup entries.
> 
> What we could do is allocate a new page table, use that page table to
> setup the new mappings, memcpy them to the swapper page table as a
> final step before we kick the caches and TLB.

That's exactly what my first incarnation of this patch did.

> The down side to that approach is we have people abusing the map_io
> method for other purposes (people - including you - just don't listen
> when I tell them that they shouldn't do this kind of stuff) such as
> setting up mappings which they then immediately access.  (In case
> you've forgotten, Assabet, the probing of the SCR register at map_io
> time to discover the platforms configuration.  That set the precident
> and now other platforms also do this kind of thing.)

I publicly complained about people doing that back then when I 
discovered that my first patch did break some platforms that did that -- 
s3c24xx being a prominent example.  But I certainly didn't remember that 
I might be the one who is responsible for having done it first.

Of course there is a catch22 problem, as some platforms, such as 
s3c24xx, have to probe for the hardware they're on before installing 
their final mapping.

> So, I don't think there's any easy solution to this - certainly not
> one which is going to be fragile or restricted to a small set of
> configurations.

Well, it would be StrongARM and XScale (excluding XSc3) 
based configurations that wouldn't work.  While not insignificant, this 
is not the majority of the platforms people are working on today.  And
all those reported issues of kernel not booting that would have 
benefited from this patch were neither on StrongARM nor XScale.

I therefore suggest amending my patch as follows:

diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index b302a5620b..1e22e3bf71 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -988,7 +988,7 @@ void __init arm_mm_memblock_reserve(void)
 #endif
 }
 
-#if defined(CONFIG_DEBUG_LL) && !defined(CONFIG_ARM_LPAE)
+#if defined(CONFIG_DEBUG_LL) && !defined(CONFIG_ARM_LPAE) && !defined(FLUSH_BASE)
 
 /*
  * The debug mappings will be cleared from the initial page table by

This will benefit those platforms that can accommodate it, while the 
others will simply remain as before.  What do you think?


Nicolas