[PATCH] x86: tidy e820 output

[PATCH] x86: tidy e820 output

[Bjorn Helgaas]

This tidies e820 output by adding an "e820" prefix and printing ranges in
the same style we use for struct resource with %pR, e.g.:

    - BIOS-e820: 0000000000000000 - 000000000009f400 (usable)
    + BIOS-e820: [mem 0x00000000-0x0009f3ff] (usable)

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
---

 arch/x86/kernel/e820.c |   68 ++++++++++++++++++++----------------------------
 1 files changed, 29 insertions(+), 39 deletions(-)


diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 0d6fc71..ef472b7 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -108,7 +108,9 @@ static void __init __e820_add_region(struct e820map *e820x, u64 start, u64 size,
 	int x = e820x->nr_map;
 
 	if (x >= ARRAY_SIZE(e820x->map)) {
-		printk(KERN_ERR "Ooops! Too many entries in the memory map!\n");
+		printk(KERN_ERR "e820: too many entries; ignoring [mem %#010llx-%#010llx]\n",
+		       (unsigned long long) start,
+		       (unsigned long long) (start + size - 1));
 		return;
 	}
 
@@ -123,29 +125,22 @@ void __init e820_add_region(u64 start, u64 size, int type)
 	__e820_add_region(&e820, start, size, type);
 }
 
-static void __init e820_print_type(u32 type)
+static char * __init e820_type_name(u32 type)
 {
 	switch (type) {
 	case E820_RAM:
 	case E820_RESERVED_KERN:
-		printk(KERN_CONT "(usable)");
-		break;
+		return "(usable)";
 	case E820_RESERVED:
-		printk(KERN_CONT "(reserved)");
-		break;
+		return "(reserved)";
 	case E820_ACPI:
-		printk(KERN_CONT "(ACPI data)");
-		break;
+		return "(ACPI data)";
 	case E820_NVS:
-		printk(KERN_CONT "(ACPI NVS)");
-		break;
+		return "(ACPI NVS)";
 	case E820_UNUSABLE:
-		printk(KERN_CONT "(unusable)");
-		break;
-	default:
-		printk(KERN_CONT "type %u", type);
-		break;
+		return "(unusable)";
 	}
+	return "(unknown)";
 }
 
 void __init e820_print_map(char *who)
@@ -153,12 +148,11 @@ void __init e820_print_map(char *who)
 	int i;
 
 	for (i = 0; i < e820.nr_map; i++) {
-		printk(KERN_INFO " %s: %016Lx - %016Lx ", who,
+		printk(KERN_INFO " %s: [mem %#010Lx-%#010Lx] %s\n", who,
 		       (unsigned long long) e820.map[i].addr,
 		       (unsigned long long)
-		       (e820.map[i].addr + e820.map[i].size));
-		e820_print_type(e820.map[i].type);
-		printk(KERN_CONT "\n");
+		       (e820.map[i].addr + e820.map[i].size - 1),
+		       e820_type_name(e820.map[i].type));
 	}
 }
 
@@ -435,13 +429,9 @@ static u64 __init __e820_update_range(struct e820map *e820x, u64 start,
 		size = ULLONG_MAX - start;
 
 	end = start + size;
-	printk(KERN_DEBUG "e820 update range: %016Lx - %016Lx ",
-		       (unsigned long long) start,
-		       (unsigned long long) end);
-	e820_print_type(old_type);
-	printk(KERN_CONT " ==> ");
-	e820_print_type(new_type);
-	printk(KERN_CONT "\n");
+	printk(KERN_DEBUG "e820: update [mem %#010Lx-%#010Lx] %s ==> %s\n",
+	       (unsigned long long) start, (unsigned long long) end - 1,
+	       e820_type_name(old_type), e820_type_name(new_type));
 
 	for (i = 0; i < e820x->nr_map; i++) {
 		struct e820entry *ei = &e820x->map[i];
@@ -511,17 +501,16 @@ u64 __init e820_remove_range(u64 start, u64 size, unsigned old_type,
 	int i;
 	u64 end;
 	u64 real_removed_size = 0;
+	char *type = "";
 
 	if (size > (ULLONG_MAX - start))
 		size = ULLONG_MAX - start;
 
 	end = start + size;
-	printk(KERN_DEBUG "e820 remove range: %016Lx - %016Lx ",
-		       (unsigned long long) start,
-		       (unsigned long long) end);
 	if (checktype)
-		e820_print_type(old_type);
-	printk(KERN_CONT "\n");
+		type = e820_type_name(old_type);
+	printk(KERN_DEBUG "e820: remove [mem %#010Lx-%#010Lx] %s\n",
+	       (unsigned long long) start, (unsigned long long) end - 1, type);
 
 	for (i = 0; i < e820.nr_map; i++) {
 		struct e820entry *ei = &e820.map[i];
@@ -574,7 +563,7 @@ void __init update_e820(void)
 	if (sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &nr_map))
 		return;
 	e820.nr_map = nr_map;
-	printk(KERN_INFO "modified physical RAM map:\n");
+	printk(KERN_INFO "e820: modified physical RAM map:\n");
 	e820_print_map("modified");
 }
 static void __init update_e820_saved(void)
@@ -655,8 +644,8 @@ __init void e820_setup_gap(void)
 	pci_mem_start = gapstart;
 
 	printk(KERN_INFO
-	       "Allocating PCI resources starting at %lx (gap: %lx:%lx)\n",
-	       pci_mem_start, gapstart, gapsize);
+	       "e820: [mem %#010lx-%#010lx] available for PCI devices\n",
+	       gapstart, gapstart + gapsize - 1);
 }
 
 /**
@@ -680,7 +669,7 @@ void __init parse_e820_ext(struct setup_data *sdata, unsigned long pa_data)
 	sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &e820.nr_map);
 	if (map_len > PAGE_SIZE)
 		early_iounmap(sdata, map_len);
-	printk(KERN_INFO "extended physical RAM map:\n");
+	printk(KERN_INFO "e820: extended physical RAM map:\n");
 	e820_print_map("extended");
 }
 
@@ -882,7 +871,7 @@ static unsigned long __init e820_end_pfn(unsigned long limit_pfn, unsigned type)
 	if (last_pfn > max_arch_pfn)
 		last_pfn = max_arch_pfn;
 
-	printk(KERN_INFO "last_pfn = %#lx max_arch_pfn = %#lx\n",
+	printk(KERN_INFO "e820: last_pfn = %#lx max_arch_pfn = %#lx\n",
 			 last_pfn, max_arch_pfn);
 	return last_pfn;
 }
@@ -1048,7 +1037,7 @@ void __init finish_e820_parsing(void)
 			early_panic("Invalid user supplied memory map");
 		e820.nr_map = nr;
 
-		printk(KERN_INFO "user-defined physical RAM map:\n");
+		printk(KERN_INFO "e820: user-defined physical RAM map:\n");
 		e820_print_map("user");
 	}
 }
@@ -1156,8 +1145,9 @@ void __init e820_reserve_resources_late(void)
 			end = MAX_RESOURCE_SIZE;
 		if (start >= end)
 			continue;
-		printk(KERN_DEBUG "reserve RAM buffer: %016llx - %016llx ",
-			       start, end);
+		printk(KERN_DEBUG
+		       "e820: reserve RAM buffer [mem %#010llx-%#010llx]\n",
+		       start, end);
 		reserve_region_with_split(&iomem_resource, start, end,
 					  "RAM buffer");
 	}

Re: [PATCH] x86: tidy e820 output

[H. Peter Anvin]

On 09/22/2010 10:27 AM, Bjorn Helgaas wrote:
> 
> This tidies e820 output by adding an "e820" prefix and printing ranges in
> the same style we use for struct resource with %pR, e.g.:
> 
>     - BIOS-e820: 0000000000000000 - 000000000009f400 (usable)
>     + BIOS-e820: [mem 0x00000000-0x0009f3ff] (usable)
> 

I'm sorry, I have to admit to not understanding the difference.  I do
not want to change the number of hex digits from fixed 16 digits, as
that will make the output harder to read when printed in a block (as is
normal for the early e820 dump).  The [mem] prefix seems redundant with
(usable), or am I misreading this?

	-hpa

Re: [PATCH] x86: tidy e820 output

[Bjorn Helgaas]

On Wednesday, September 22, 2010 11:54:49 am H. Peter Anvin wrote:
> On 09/22/2010 10:27 AM, Bjorn Helgaas wrote:
> > 
> > This tidies e820 output by adding an "e820" prefix and printing ranges in
> > the same style we use for struct resource with %pR, e.g.:
> > 
> >     - BIOS-e820: 0000000000000000 - 000000000009f400 (usable)
> >     + BIOS-e820: [mem 0x00000000-0x0009f3ff] (usable)
> 
> I'm sorry, I have to admit to not understanding the difference.  I do
> not want to change the number of hex digits from fixed 16 digits, as
> that will make the output harder to read when printed in a block (as is
> normal for the early e820 dump).  The [mem] prefix seems redundant with
> (usable), or am I misreading this?

These E820 ranges should be easily comparable with similar ranges we
print elsewhere.  Currently we have things like this:

    BIOS-e820: 0000000000000000 - 000000000009f400 (usable)
    BIOS-e820: 000000000009f400 - 00000000000a0000 (reserved)
    BIOS-e820: 00000000fffbc000 - 0000000100000000 (reserved)
    pci_root PNP0A03:00: host bridge window [mem 0x000a0000-0x000bffff]
    pci_root PNP0A03:00: host bridge window [mem 0xe0000000-0xfebfffff]
    pci 0000:00:02.0: reg 10: [mem 0xf0000000-0xf1ffffff pref]
    reserve RAM buffer: 000000000009f400 - 000000000009ffff 
    pnp 00:07: [mem 0xfed00000-0xfed003ff]

It would be easier to integrate the E820 information with the ACPI
and PCI window and BAR information if they looked similar.

We currently have a mix of some with "0x" prefix, some without;
some with eight hex digits, some with sixteen; some with spaces
around the internal "-", some without; some with type (io/mem/etc),
some without; some with uppercase hex (MTRR), most with lowercase;
some including the end address, some not; and even some in PFNs
and most in addresses.  It just makes it harder than it needs
to be to debug issues in this area.

Bjorn

Re: [PATCH] x86: tidy e820 output

[H. Peter Anvin]

On 09/22/2010 11:20 AM, Bjorn Helgaas wrote:
> 
> These E820 ranges should be easily comparable with similar ranges we
> print elsewhere.  Currently we have things like this:
> 
>     BIOS-e820: 0000000000000000 - 000000000009f400 (usable)
>     BIOS-e820: 000000000009f400 - 00000000000a0000 (reserved)
>     BIOS-e820: 00000000fffbc000 - 0000000100000000 (reserved)
>     pci_root PNP0A03:00: host bridge window [mem 0x000a0000-0x000bffff]
>     pci_root PNP0A03:00: host bridge window [mem 0xe0000000-0xfebfffff]
>     pci 0000:00:02.0: reg 10: [mem 0xf0000000-0xf1ffffff pref]
>     reserve RAM buffer: 000000000009f400 - 000000000009ffff 
>     pnp 00:07: [mem 0xfed00000-0xfed003ff]
> 
> It would be easier to integrate the E820 information with the ACPI
> and PCI window and BAR information if they looked similar.
> 
> We currently have a mix of some with "0x" prefix, some without;
> some with eight hex digits, some with sixteen; some with spaces
> around the internal "-", some without; some with type (io/mem/etc),
> some without; some with uppercase hex (MTRR), most with lowercase;
> some including the end address, some not; and even some in PFNs
> and most in addresses.  It just makes it harder than it needs
> to be to debug issues in this area.
> 

Yes, but I think you can also see why I really don't like the thought of
the numbers in the BIOS-e820: block not even lining up anymore.

	-hpa

Re: [PATCH] x86: tidy e820 output

[Bjorn Helgaas]

On Wednesday, September 22, 2010 12:27:59 pm H. Peter Anvin wrote:
> Yes, but I think you can also see why I really don't like the thought of
> the numbers in the BIOS-e820: block not even lining up anymore.

It's true, they don't (well, everything below 4G still lines up, but
not above that).  Do you like this any better?

commit bc43bea8f02ae27dd04f6a8bccc818888362ea0d
Author: Bjorn Helgaas <bjorn.helgaas@hp.com>
Date:   Tue Sep 21 12:32:34 2010 -0600

    x86: tidy e820 output
    
    This tidies e820 output by adding an "e820" prefix and printing ranges
    similarly to the way we print struct resource with %pR, e.g.:
    
        - BIOS-e820: 0000000000000000 - 000000000009f400 (usable)
        + BIOS-e820: 0x0000000000000000-0x000000000009f3ff (usable)
    
    Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 0d6fc71..aabae8d 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -108,7 +108,9 @@ static void __init __e820_add_region(struct e820map *e820x, u64 start, u64 size,
 	int x = e820x->nr_map;
 
 	if (x >= ARRAY_SIZE(e820x->map)) {
-		printk(KERN_ERR "Ooops! Too many entries in the memory map!\n");
+		printk(KERN_ERR "e820: too many entries; ignoring [mem %#010llx-%#010llx]\n",
+		       (unsigned long long) start,
+		       (unsigned long long) (start + size - 1));
 		return;
 	}
 
@@ -123,29 +125,22 @@ void __init e820_add_region(u64 start, u64 size, int type)
 	__e820_add_region(&e820, start, size, type);
 }
 
-static void __init e820_print_type(u32 type)
+static char * __init e820_type_name(u32 type)
 {
 	switch (type) {
 	case E820_RAM:
 	case E820_RESERVED_KERN:
-		printk(KERN_CONT "(usable)");
-		break;
+		return "(usable)";
 	case E820_RESERVED:
-		printk(KERN_CONT "(reserved)");
-		break;
+		return "(reserved)";
 	case E820_ACPI:
-		printk(KERN_CONT "(ACPI data)");
-		break;
+		return "(ACPI data)";
 	case E820_NVS:
-		printk(KERN_CONT "(ACPI NVS)");
-		break;
+		return "(ACPI NVS)";
 	case E820_UNUSABLE:
-		printk(KERN_CONT "(unusable)");
-		break;
-	default:
-		printk(KERN_CONT "type %u", type);
-		break;
+		return "(unusable)";
 	}
+	return "(unknown)";
 }
 
 void __init e820_print_map(char *who)
@@ -153,12 +148,11 @@ void __init e820_print_map(char *who)
 	int i;
 
 	for (i = 0; i < e820.nr_map; i++) {
-		printk(KERN_INFO " %s: %016Lx - %016Lx ", who,
+		printk(KERN_INFO " %s: %#018Lx-%#018Lx %s\n", who,
 		       (unsigned long long) e820.map[i].addr,
 		       (unsigned long long)
-		       (e820.map[i].addr + e820.map[i].size));
-		e820_print_type(e820.map[i].type);
-		printk(KERN_CONT "\n");
+		       (e820.map[i].addr + e820.map[i].size - 1),
+		       e820_type_name(e820.map[i].type));
 	}
 }
 
@@ -435,13 +429,9 @@ static u64 __init __e820_update_range(struct e820map *e820x, u64 start,
 		size = ULLONG_MAX - start;
 
 	end = start + size;
-	printk(KERN_DEBUG "e820 update range: %016Lx - %016Lx ",
-		       (unsigned long long) start,
-		       (unsigned long long) end);
-	e820_print_type(old_type);
-	printk(KERN_CONT " ==> ");
-	e820_print_type(new_type);
-	printk(KERN_CONT "\n");
+	printk(KERN_DEBUG "e820: update [mem %#010Lx-%#010Lx] %s ==> %s\n",
+	       (unsigned long long) start, (unsigned long long) end - 1,
+	       e820_type_name(old_type), e820_type_name(new_type));
 
 	for (i = 0; i < e820x->nr_map; i++) {
 		struct e820entry *ei = &e820x->map[i];
@@ -511,17 +501,16 @@ u64 __init e820_remove_range(u64 start, u64 size, unsigned old_type,
 	int i;
 	u64 end;
 	u64 real_removed_size = 0;
+	char *type = "";
 
 	if (size > (ULLONG_MAX - start))
 		size = ULLONG_MAX - start;
 
 	end = start + size;
-	printk(KERN_DEBUG "e820 remove range: %016Lx - %016Lx ",
-		       (unsigned long long) start,
-		       (unsigned long long) end);
 	if (checktype)
-		e820_print_type(old_type);
-	printk(KERN_CONT "\n");
+		type = e820_type_name(old_type);
+	printk(KERN_DEBUG "e820: remove [mem %#010Lx-%#010Lx] %s\n",
+	       (unsigned long long) start, (unsigned long long) end - 1, type);
 
 	for (i = 0; i < e820.nr_map; i++) {
 		struct e820entry *ei = &e820.map[i];
@@ -574,7 +563,7 @@ void __init update_e820(void)
 	if (sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &nr_map))
 		return;
 	e820.nr_map = nr_map;
-	printk(KERN_INFO "modified physical RAM map:\n");
+	printk(KERN_INFO "e820: modified physical RAM map:\n");
 	e820_print_map("modified");
 }
 static void __init update_e820_saved(void)
@@ -655,8 +644,8 @@ __init void e820_setup_gap(void)
 	pci_mem_start = gapstart;
 
 	printk(KERN_INFO
-	       "Allocating PCI resources starting at %lx (gap: %lx:%lx)\n",
-	       pci_mem_start, gapstart, gapsize);
+	       "e820: [mem %#010lx-%#010lx] available for PCI devices\n",
+	       gapstart, gapstart + gapsize - 1);
 }
 
 /**
@@ -680,7 +669,7 @@ void __init parse_e820_ext(struct setup_data *sdata, unsigned long pa_data)
 	sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &e820.nr_map);
 	if (map_len > PAGE_SIZE)
 		early_iounmap(sdata, map_len);
-	printk(KERN_INFO "extended physical RAM map:\n");
+	printk(KERN_INFO "e820: extended physical RAM map:\n");
 	e820_print_map("extended");
 }
 
@@ -882,7 +871,7 @@ static unsigned long __init e820_end_pfn(unsigned long limit_pfn, unsigned type)
 	if (last_pfn > max_arch_pfn)
 		last_pfn = max_arch_pfn;
 
-	printk(KERN_INFO "last_pfn = %#lx max_arch_pfn = %#lx\n",
+	printk(KERN_INFO "e820: last_pfn = %#lx max_arch_pfn = %#lx\n",
 			 last_pfn, max_arch_pfn);
 	return last_pfn;
 }
@@ -1048,7 +1037,7 @@ void __init finish_e820_parsing(void)
 			early_panic("Invalid user supplied memory map");
 		e820.nr_map = nr;
 
-		printk(KERN_INFO "user-defined physical RAM map:\n");
+		printk(KERN_INFO "e820: user-defined physical RAM map:\n");
 		e820_print_map("user");
 	}
 }
@@ -1156,8 +1145,9 @@ void __init e820_reserve_resources_late(void)
 			end = MAX_RESOURCE_SIZE;
 		if (start >= end)
 			continue;
-		printk(KERN_DEBUG "reserve RAM buffer: %016llx - %016llx ",
-			       start, end);
+		printk(KERN_DEBUG
+		       "e820: reserve RAM buffer [mem %#010llx-%#010llx]\n",
+		       start, end);
 		reserve_region_with_split(&iomem_resource, start, end,
 					  "RAM buffer");
 	}

Re: [PATCH] x86: tidy e820 output

[H. Peter Anvin]

On 09/22/2010 11:53 AM, Bjorn Helgaas wrote:
> 
> It's true, they don't (well, everything below 4G still lines up, but
> not above that).  Do you like this any better?
> 

That's fine with me.  I don't mind the [mem ] bracket either if you
think it's useful.

	-hpa

Re: [PATCH] x86: tidy e820 output

[Bjorn Helgaas]

On Wednesday, September 22, 2010 12:58:36 pm H. Peter Anvin wrote:
> On 09/22/2010 11:53 AM, Bjorn Helgaas wrote:
> > 
> > It's true, they don't (well, everything below 4G still lines up, but
> > not above that).  Do you like this any better?
> > 
> 
> That's fine with me.  I don't mind the [mem ] bracket either if you
> think it's useful.

I took out "[mem" because it made the "(reserved)" lines wider than
80 columns.  But maybe I should just remove the parens around the
E820 type instead, like this:

commit d2338b08303439b23d4909eab3744b3f29f09874
Author: Bjorn Helgaas <bjorn.helgaas@hp.com>
Date:   Tue Sep 21 12:32:34 2010 -0600

    x86: tidy e820 output
    
    This tidies e820 output by adding an "e820" prefix and printing ranges
    similarly to the way we print struct resource with %pR, e.g.:
    
        - BIOS-e820: 0000000000000000 - 000000000009f400 (usable)
        + BIOS-e820: [mem 0x0000000000000000-0x000000000009f3ff] usable
    
    Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 0d6fc71..2bd464f 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -108,7 +108,9 @@ static void __init __e820_add_region(struct e820map *e820x, u64 start, u64 size,
 	int x = e820x->nr_map;
 
 	if (x >= ARRAY_SIZE(e820x->map)) {
-		printk(KERN_ERR "Ooops! Too many entries in the memory map!\n");
+		printk(KERN_ERR "e820: too many entries; ignoring [mem %#010llx-%#010llx]\n",
+		       (unsigned long long) start,
+		       (unsigned long long) (start + size - 1));
 		return;
 	}
 
@@ -123,29 +125,22 @@ void __init e820_add_region(u64 start, u64 size, int type)
 	__e820_add_region(&e820, start, size, type);
 }
 
-static void __init e820_print_type(u32 type)
+static char * __init e820_type_name(u32 type)
 {
 	switch (type) {
 	case E820_RAM:
 	case E820_RESERVED_KERN:
-		printk(KERN_CONT "(usable)");
-		break;
+		return "usable";
 	case E820_RESERVED:
-		printk(KERN_CONT "(reserved)");
-		break;
+		return "reserved";
 	case E820_ACPI:
-		printk(KERN_CONT "(ACPI data)");
-		break;
+		return "ACPI data";
 	case E820_NVS:
-		printk(KERN_CONT "(ACPI NVS)");
-		break;
+		return "ACPI NVS";
 	case E820_UNUSABLE:
-		printk(KERN_CONT "(unusable)");
-		break;
-	default:
-		printk(KERN_CONT "type %u", type);
-		break;
+		return "unusable";
 	}
+	return "(unknown)";
 }
 
 void __init e820_print_map(char *who)
@@ -153,12 +148,11 @@ void __init e820_print_map(char *who)
 	int i;
 
 	for (i = 0; i < e820.nr_map; i++) {
-		printk(KERN_INFO " %s: %016Lx - %016Lx ", who,
+		printk(KERN_INFO " %s: [mem %#018Lx-%#018Lx] %s\n", who,
 		       (unsigned long long) e820.map[i].addr,
 		       (unsigned long long)
-		       (e820.map[i].addr + e820.map[i].size));
-		e820_print_type(e820.map[i].type);
-		printk(KERN_CONT "\n");
+		       (e820.map[i].addr + e820.map[i].size - 1),
+		       e820_type_name(e820.map[i].type));
 	}
 }
 
@@ -435,13 +429,9 @@ static u64 __init __e820_update_range(struct e820map *e820x, u64 start,
 		size = ULLONG_MAX - start;
 
 	end = start + size;
-	printk(KERN_DEBUG "e820 update range: %016Lx - %016Lx ",
-		       (unsigned long long) start,
-		       (unsigned long long) end);
-	e820_print_type(old_type);
-	printk(KERN_CONT " ==> ");
-	e820_print_type(new_type);
-	printk(KERN_CONT "\n");
+	printk(KERN_DEBUG "e820: update [mem %#010Lx-%#010Lx] %s ==> %s\n",
+	       (unsigned long long) start, (unsigned long long) end - 1,
+	       e820_type_name(old_type), e820_type_name(new_type));
 
 	for (i = 0; i < e820x->nr_map; i++) {
 		struct e820entry *ei = &e820x->map[i];
@@ -511,17 +501,16 @@ u64 __init e820_remove_range(u64 start, u64 size, unsigned old_type,
 	int i;
 	u64 end;
 	u64 real_removed_size = 0;
+	char *type = "";
 
 	if (size > (ULLONG_MAX - start))
 		size = ULLONG_MAX - start;
 
 	end = start + size;
-	printk(KERN_DEBUG "e820 remove range: %016Lx - %016Lx ",
-		       (unsigned long long) start,
-		       (unsigned long long) end);
 	if (checktype)
-		e820_print_type(old_type);
-	printk(KERN_CONT "\n");
+		type = e820_type_name(old_type);
+	printk(KERN_DEBUG "e820: remove [mem %#010Lx-%#010Lx] %s\n",
+	       (unsigned long long) start, (unsigned long long) end - 1, type);
 
 	for (i = 0; i < e820.nr_map; i++) {
 		struct e820entry *ei = &e820.map[i];
@@ -574,7 +563,7 @@ void __init update_e820(void)
 	if (sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &nr_map))
 		return;
 	e820.nr_map = nr_map;
-	printk(KERN_INFO "modified physical RAM map:\n");
+	printk(KERN_INFO "e820: modified physical RAM map:\n");
 	e820_print_map("modified");
 }
 static void __init update_e820_saved(void)
@@ -655,8 +644,8 @@ __init void e820_setup_gap(void)
 	pci_mem_start = gapstart;
 
 	printk(KERN_INFO
-	       "Allocating PCI resources starting at %lx (gap: %lx:%lx)\n",
-	       pci_mem_start, gapstart, gapsize);
+	       "e820: [mem %#010lx-%#010lx] available for PCI devices\n",
+	       gapstart, gapstart + gapsize - 1);
 }
 
 /**
@@ -680,7 +669,7 @@ void __init parse_e820_ext(struct setup_data *sdata, unsigned long pa_data)
 	sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &e820.nr_map);
 	if (map_len > PAGE_SIZE)
 		early_iounmap(sdata, map_len);
-	printk(KERN_INFO "extended physical RAM map:\n");
+	printk(KERN_INFO "e820: extended physical RAM map:\n");
 	e820_print_map("extended");
 }
 
@@ -882,7 +871,7 @@ static unsigned long __init e820_end_pfn(unsigned long limit_pfn, unsigned type)
 	if (last_pfn > max_arch_pfn)
 		last_pfn = max_arch_pfn;
 
-	printk(KERN_INFO "last_pfn = %#lx max_arch_pfn = %#lx\n",
+	printk(KERN_INFO "e820: last_pfn = %#lx max_arch_pfn = %#lx\n",
 			 last_pfn, max_arch_pfn);
 	return last_pfn;
 }
@@ -1048,7 +1037,7 @@ void __init finish_e820_parsing(void)
 			early_panic("Invalid user supplied memory map");
 		e820.nr_map = nr;
 
-		printk(KERN_INFO "user-defined physical RAM map:\n");
+		printk(KERN_INFO "e820: user-defined physical RAM map:\n");
 		e820_print_map("user");
 	}
 }
@@ -1156,8 +1145,9 @@ void __init e820_reserve_resources_late(void)
 			end = MAX_RESOURCE_SIZE;
 		if (start >= end)
 			continue;
-		printk(KERN_DEBUG "reserve RAM buffer: %016llx - %016llx ",
-			       start, end);
+		printk(KERN_DEBUG
+		       "e820: reserve RAM buffer [mem %#010llx-%#010llx]\n",
+		       start, end);
 		reserve_region_with_split(&iomem_resource, start, end,
 					  "RAM buffer");
 	}

Re: [PATCH] x86: tidy e820 output

[Yinghai Lu]

On 09/22/2010 12:11 PM, Bjorn Helgaas wrote:
> On Wednesday, September 22, 2010 12:58:36 pm H. Peter Anvin wrote:
>> On 09/22/2010 11:53 AM, Bjorn Helgaas wrote:
>>>
>>> It's true, they don't (well, everything below 4G still lines up, but
>>> not above that).  Do you like this any better?
>>>
>>
>> That's fine with me.  I don't mind the [mem ] bracket either if you
>> think it's useful.
> 
> I took out "[mem" because it made the "(reserved)" lines wider than
> 80 columns.  But maybe I should just remove the parens around the
> E820 type instead, like this:
> 
> commit d2338b08303439b23d4909eab3744b3f29f09874
> Author: Bjorn Helgaas <bjorn.helgaas@hp.com>
> Date:   Tue Sep 21 12:32:34 2010 -0600
> 
>     x86: tidy e820 output
>     
>     This tidies e820 output by adding an "e820" prefix and printing ranges
>     similarly to the way we print struct resource with %pR, e.g.:
>     
>         - BIOS-e820: 0000000000000000 - 000000000009f400 (usable)
>         + BIOS-e820: [mem 0x0000000000000000-0x000000000009f3ff] usable
>     
>     Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> 
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index 0d6fc71..2bd464f 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -108,7 +108,9 @@ static void __init __e820_add_region(struct e820map *e820x, u64 start, u64 size,
>  	int x = e820x->nr_map;
>  
>  	if (x >= ARRAY_SIZE(e820x->map)) {
> -		printk(KERN_ERR "Ooops! Too many entries in the memory map!\n");
> +		printk(KERN_ERR "e820: too many entries; ignoring [mem %#010llx-%#010llx]\n",
> +		       (unsigned long long) start,
> +		       (unsigned long long) (start + size - 1));
>  		return;
>  	}
>  
> @@ -123,29 +125,22 @@ void __init e820_add_region(u64 start, u64 size, int type)
>  	__e820_add_region(&e820, start, size, type);
>  }
>  
> -static void __init e820_print_type(u32 type)
> +static char * __init e820_type_name(u32 type)
>  {
>  	switch (type) {
>  	case E820_RAM:
>  	case E820_RESERVED_KERN:
> -		printk(KERN_CONT "(usable)");
> -		break;
> +		return "usable";
>  	case E820_RESERVED:
> -		printk(KERN_CONT "(reserved)");
> -		break;
> +		return "reserved";
>  	case E820_ACPI:
> -		printk(KERN_CONT "(ACPI data)");
> -		break;
> +		return "ACPI data";
>  	case E820_NVS:
> -		printk(KERN_CONT "(ACPI NVS)");
> -		break;
> +		return "ACPI NVS";
>  	case E820_UNUSABLE:
> -		printk(KERN_CONT "(unusable)");
> -		break;
> -	default:
> -		printk(KERN_CONT "type %u", type);
> -		break;
> +		return "unusable";
>  	}
> +	return "(unknown)";
>  }

type value?

Yinghai

Re: [PATCH] x86: tidy e820 output

[Bjorn Helgaas]

On Wednesday, September 22, 2010 03:07:00 pm Yinghai Lu wrote:
> On 09/22/2010 12:11 PM, Bjorn Helgaas wrote:

> > -static void __init e820_print_type(u32 type)
> > +static char * __init e820_type_name(u32 type)
> >  {
> >  	switch (type) {
> >  	case E820_RAM:
> >  	case E820_RESERVED_KERN:
> > -		printk(KERN_CONT "(usable)");
> > -		break;
> > +		return "usable";
> >  	case E820_RESERVED:
> > -		printk(KERN_CONT "(reserved)");
> > -		break;
> > +		return "reserved";
> >  	case E820_ACPI:
> > -		printk(KERN_CONT "(ACPI data)");
> > -		break;
> > +		return "ACPI data";
> >  	case E820_NVS:
> > -		printk(KERN_CONT "(ACPI NVS)");
> > -		break;
> > +		return "ACPI NVS";
> >  	case E820_UNUSABLE:
> > -		printk(KERN_CONT "(unusable)");
> > -		break;
> > -	default:
> > -		printk(KERN_CONT "type %u", type);
> > -		break;
> > +		return "unusable";
> >  	}
> > +	return "(unknown)";
> >  }
> 
> type value?

I decided the code simplification was worth skipping the type.
I'd certainly rather have the type value, too, but I don't know
how much hassle to go through to debug a firmware problem.  How
important do you think it is?

Bjorn

Re: [PATCH] x86: tidy e820 output

[Yinghai Lu]

On 09/22/2010 02:22 PM, Bjorn Helgaas wrote:
> On Wednesday, September 22, 2010 03:07:00 pm Yinghai Lu wrote:
>> On 09/22/2010 12:11 PM, Bjorn Helgaas wrote:
> 
>>> -static void __init e820_print_type(u32 type)
>>> +static char * __init e820_type_name(u32 type)
>>>  {
>>>  	switch (type) {
>>>  	case E820_RAM:
>>>  	case E820_RESERVED_KERN:
>>> -		printk(KERN_CONT "(usable)");
>>> -		break;
>>> +		return "usable";
>>>  	case E820_RESERVED:
>>> -		printk(KERN_CONT "(reserved)");
>>> -		break;
>>> +		return "reserved";
>>>  	case E820_ACPI:
>>> -		printk(KERN_CONT "(ACPI data)");
>>> -		break;
>>> +		return "ACPI data";
>>>  	case E820_NVS:
>>> -		printk(KERN_CONT "(ACPI NVS)");
>>> -		break;
>>> +		return "ACPI NVS";
>>>  	case E820_UNUSABLE:
>>> -		printk(KERN_CONT "(unusable)");
>>> -		break;
>>> -	default:
>>> -		printk(KERN_CONT "type %u", type);
>>> -		break;
>>> +		return "unusable";
>>>  	}
>>> +	return "(unknown)";
>>>  }
>>
>> type value?
> 
> I decided the code simplification was worth skipping the type.
> I'd certainly rather have the type value, too, but I don't know
> how much hassle to go through to debug a firmware problem.  How
> important do you think it is?

I have some systems with "type 9".

Yinghai

Re: [PATCH] x86: tidy e820 output

[H. Peter Anvin]

On 09/22/2010 02:22 PM, Bjorn Helgaas wrote:
> 
> I decided the code simplification was worth skipping the type.
> I'd certainly rather have the type value, too, but I don't know
> how much hassle to go through to debug a firmware problem.  How
> important do you think it is?
> 

Very.  New types get introduced periodically.

	-hpa

Re: [PATCH] x86: tidy e820 output

[Bjorn Helgaas]

On Wednesday, September 22, 2010 03:34:03 pm H. Peter Anvin wrote:
> On 09/22/2010 02:22 PM, Bjorn Helgaas wrote:
> > 
> > I decided the code simplification was worth skipping the type.
> > I'd certainly rather have the type value, too, but I don't know
> > how much hassle to go through to debug a firmware problem.  How
> > important do you think it is?
> > 
> 
> Very.  New types get introduced periodically.

OK, I managed to overcome my aversion to KERN_CONT to try again:


commit cc1480c3476d86cec8f57817a039f01d215ac89f
Author: Bjorn Helgaas <bjorn.helgaas@hp.com>
Date:   Tue Sep 21 12:32:34 2010 -0600

    x86: tidy e820 output
    
    This tidies e820 output by adding an "e820" prefix and printing ranges
    similarly to the way we print struct resource with %pR, e.g.:
    
        - BIOS-e820: 0000000000000000 - 000000000009f400 (usable)
        + BIOS-e820: [mem 0x0000000000000000-0x000000000009f3ff] usable
    
    Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 0d6fc71..ab364b4 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -108,7 +108,9 @@ static void __init __e820_add_region(struct e820map *e820x, u64 start, u64 size,
 	int x = e820x->nr_map;
 
 	if (x >= ARRAY_SIZE(e820x->map)) {
-		printk(KERN_ERR "Ooops! Too many entries in the memory map!\n");
+		printk(KERN_ERR "e820: too many entries; ignoring [mem %#010llx-%#010llx]\n",
+		       (unsigned long long) start,
+		       (unsigned long long) (start + size - 1));
 		return;
 	}
 
@@ -128,19 +130,19 @@ static void __init e820_print_type(u32 type)
 	switch (type) {
 	case E820_RAM:
 	case E820_RESERVED_KERN:
-		printk(KERN_CONT "(usable)");
+		printk(KERN_CONT "usable");
 		break;
 	case E820_RESERVED:
-		printk(KERN_CONT "(reserved)");
+		printk(KERN_CONT "reserved");
 		break;
 	case E820_ACPI:
-		printk(KERN_CONT "(ACPI data)");
+		printk(KERN_CONT "ACPI data");
 		break;
 	case E820_NVS:
-		printk(KERN_CONT "(ACPI NVS)");
+		printk(KERN_CONT "ACPI NVS");
 		break;
 	case E820_UNUSABLE:
-		printk(KERN_CONT "(unusable)");
+		printk(KERN_CONT "unusable");
 		break;
 	default:
 		printk(KERN_CONT "type %u", type);
@@ -153,10 +155,10 @@ void __init e820_print_map(char *who)
 	int i;
 
 	for (i = 0; i < e820.nr_map; i++) {
-		printk(KERN_INFO " %s: %016Lx - %016Lx ", who,
+		printk(KERN_INFO " %s: [mem %#018Lx-%#018Lx] ", who,
 		       (unsigned long long) e820.map[i].addr,
 		       (unsigned long long)
-		       (e820.map[i].addr + e820.map[i].size));
+		       (e820.map[i].addr + e820.map[i].size - 1));
 		e820_print_type(e820.map[i].type);
 		printk(KERN_CONT "\n");
 	}
@@ -435,9 +437,8 @@ static u64 __init __e820_update_range(struct e820map *e820x, u64 start,
 		size = ULLONG_MAX - start;
 
 	end = start + size;
-	printk(KERN_DEBUG "e820 update range: %016Lx - %016Lx ",
-		       (unsigned long long) start,
-		       (unsigned long long) end);
+	printk(KERN_DEBUG "e820: update [mem %#010Lx-%#010Lx] ",
+	       (unsigned long long) start, (unsigned long long) (end - 1));
 	e820_print_type(old_type);
 	printk(KERN_CONT " ==> ");
 	e820_print_type(new_type);
@@ -516,12 +517,10 @@ u64 __init e820_remove_range(u64 start, u64 size, unsigned old_type,
 		size = ULLONG_MAX - start;
 
 	end = start + size;
-	printk(KERN_DEBUG "e820 remove range: %016Lx - %016Lx ",
-		       (unsigned long long) start,
-		       (unsigned long long) end);
+	printk(KERN_DEBUG "e820: remove [mem %#010Lx-%#010Lx] ",
+	       (unsigned long long) start, (unsigned long long) (end - 1));
 	if (checktype)
 		e820_print_type(old_type);
-	printk(KERN_CONT "\n");
 
 	for (i = 0; i < e820.nr_map; i++) {
 		struct e820entry *ei = &e820.map[i];
@@ -574,7 +573,7 @@ void __init update_e820(void)
 	if (sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &nr_map))
 		return;
 	e820.nr_map = nr_map;
-	printk(KERN_INFO "modified physical RAM map:\n");
+	printk(KERN_INFO "e820: modified physical RAM map:\n");
 	e820_print_map("modified");
 }
 static void __init update_e820_saved(void)
@@ -655,8 +654,8 @@ __init void e820_setup_gap(void)
 	pci_mem_start = gapstart;
 
 	printk(KERN_INFO
-	       "Allocating PCI resources starting at %lx (gap: %lx:%lx)\n",
-	       pci_mem_start, gapstart, gapsize);
+	       "e820: [mem %#010lx-%#010lx] available for PCI devices\n",
+	       gapstart, gapstart + gapsize - 1);
 }
 
 /**
@@ -680,7 +679,7 @@ void __init parse_e820_ext(struct setup_data *sdata, unsigned long pa_data)
 	sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &e820.nr_map);
 	if (map_len > PAGE_SIZE)
 		early_iounmap(sdata, map_len);
-	printk(KERN_INFO "extended physical RAM map:\n");
+	printk(KERN_INFO "e820: extended physical RAM map:\n");
 	e820_print_map("extended");
 }
 
@@ -882,7 +881,7 @@ static unsigned long __init e820_end_pfn(unsigned long limit_pfn, unsigned type)
 	if (last_pfn > max_arch_pfn)
 		last_pfn = max_arch_pfn;
 
-	printk(KERN_INFO "last_pfn = %#lx max_arch_pfn = %#lx\n",
+	printk(KERN_INFO "e820: last_pfn = %#lx max_arch_pfn = %#lx\n",
 			 last_pfn, max_arch_pfn);
 	return last_pfn;
 }
@@ -1048,7 +1047,7 @@ void __init finish_e820_parsing(void)
 			early_panic("Invalid user supplied memory map");
 		e820.nr_map = nr;
 
-		printk(KERN_INFO "user-defined physical RAM map:\n");
+		printk(KERN_INFO "e820: user-defined physical RAM map:\n");
 		e820_print_map("user");
 	}
 }
@@ -1156,8 +1155,9 @@ void __init e820_reserve_resources_late(void)
 			end = MAX_RESOURCE_SIZE;
 		if (start >= end)
 			continue;
-		printk(KERN_DEBUG "reserve RAM buffer: %016llx - %016llx ",
-			       start, end);
+		printk(KERN_DEBUG
+		       "e820: reserve RAM buffer [mem %#010llx-%#010llx]\n",
+		       start, end);
 		reserve_region_with_split(&iomem_resource, start, end,
 					  "RAM buffer");
 	}

Re: [PATCH] x86: tidy e820 output

[H. Peter Anvin]

On 09/22/2010 03:05 PM, Bjorn Helgaas wrote:
> 
> OK, I managed to overcome my aversion to KERN_CONT to try again:
> 

If you don't want to use KERN_CONT there is always sprintf.

	-hpa

Re: [PATCH] x86: tidy e820 output

[David Rientjes]

On Wed, 22 Sep 2010, H. Peter Anvin wrote:

> > It's true, they don't (well, everything below 4G still lines up, but
> > not above that).  Do you like this any better?
> > 
> 
> That's fine with me.  I don't mind the [mem ] bracket either if you
> think it's useful.
> 

This patch is going to break our userspace parsing scripts if you change 
the output format.  Admittedly, we're probably one of the few users who 
actually parses this output, but we do have reasons to do it for our 
firmware.  If there was some improvement being introduced here, we'd 
happily handle multiple regexs (we constantly add new patterns when new 
kernels are released), but I'm not seeing how this is better.

Re: [PATCH] x86: tidy e820 output

[H. Peter Anvin]

On 09/22/2010 03:46 PM, David Rientjes wrote:
> On Wed, 22 Sep 2010, H. Peter Anvin wrote:
> 
>>> It's true, they don't (well, everything below 4G still lines up, but
>>> not above that).  Do you like this any better?
>>>
>>
>> That's fine with me.  I don't mind the [mem ] bracket either if you
>> think it's useful.
>>
> 
> This patch is going to break our userspace parsing scripts if you change 
> the output format.  Admittedly, we're probably one of the few users who 
> actually parses this output, but we do have reasons to do it for our 
> firmware.  If there was some improvement being introduced here, we'd 
> happily handle multiple regexs (we constantly add new patterns when new 
> kernels are released), but I'm not seeing how this is better.

Kernel messages are not an ABI or API.

The user space API for this stuff is /sys/firmware/memmap.

	-hpa

Re: [PATCH] x86: tidy e820 output

[David Rientjes]

On Wed, 22 Sep 2010, H. Peter Anvin wrote:

> > This patch is going to break our userspace parsing scripts if you change 
> > the output format.  Admittedly, we're probably one of the few users who 
> > actually parses this output, but we do have reasons to do it for our 
> > firmware.  If there was some improvement being introduced here, we'd 
> > happily handle multiple regexs (we constantly add new patterns when new 
> > kernels are released), but I'm not seeing how this is better.
> 
> Kernel messages are not an ABI or API.
> 
> The user space API for this stuff is /sys/firmware/memmap.
> 

I'm referring to using [start, start + addr - 1] in the output, like 
/sys/firmware/memmap does, as opposed to [start, start + addr].  Is it not 
valuable to include the actual e820 map in some way, especially when you 
have your own BIOS?  We've always used this output since it isn't 
available later.

Another use of this information that people may already be using it for is 
ensuring the memmap= on the command line is parsed correctly.

So, again, I'm looking for the benefit here in this patch and it's not 
immediately apparent to me.

Re: [PATCH] x86: tidy e820 output

[H. Peter Anvin]

On 09/22/2010 04:12 PM, David Rientjes wrote:
> On Wed, 22 Sep 2010, H. Peter Anvin wrote:
> 
>>> This patch is going to break our userspace parsing scripts if you change 
>>> the output format.  Admittedly, we're probably one of the few users who 
>>> actually parses this output, but we do have reasons to do it for our 
>>> firmware.  If there was some improvement being introduced here, we'd 
>>> happily handle multiple regexs (we constantly add new patterns when new 
>>> kernels are released), but I'm not seeing how this is better.
>>
>> Kernel messages are not an ABI or API.
>>
>> The user space API for this stuff is /sys/firmware/memmap.
>>
> 
> I'm referring to using [start, start + addr - 1] in the output, like 
> /sys/firmware/memmap does, as opposed to [start, start + addr].  Is it not 
> valuable to include the actual e820 map in some way, especially when you 
> have your own BIOS?  We've always used this output since it isn't 
> available later.

"The actual e820 map" contains (start, length, type) -- the end bracket
is not part of it at all.  Either way, /sys/firmware/memmap does provide
the memory map as provided by the firmware through whatever means.

> Another use of this information that people may already be using it for is 
> ensuring the memmap= on the command line is parsed correctly.
> 
> So, again, I'm looking for the benefit here in this patch and it's not 
> immediately apparent to me.

Consistency with other resources displayed seems like a major win to me.

	-hpa
	

Re: [PATCH] x86: tidy e820 output

[David Rientjes]

On Wed, 22 Sep 2010, H. Peter Anvin wrote:

> > I'm referring to using [start, start + addr - 1] in the output, like 
> > /sys/firmware/memmap does, as opposed to [start, start + addr].  Is it not 
> > valuable to include the actual e820 map in some way, especially when you 
> > have your own BIOS?  We've always used this output since it isn't 
> > available later.
> 
> "The actual e820 map" contains (start, length, type) -- the end bracket
> is not part of it at all.  Either way, /sys/firmware/memmap does provide
> the memory map as provided by the firmware through whatever means.
> 

Ok, I agree that fixing the off-by-1 problem that's been there for ages is 
better and worth modifying our scripts when they break.  Thanks!