[PATCH 0/5] x86: Clean up and reorganize the E820 table handling code, #updates

[PATCH 0/5] x86: Clean up and reorganize the E820 table handling code, #updates

[Ingo Molnar]

This is a quick update to the original series, mostly to address Linus's
feedback.

The full tree can be found at:

   git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git WIP.x86/boot

Thanks,

     Ingo

========>

Borislav Petkov (1):
  x86/boot: Fix pr_debug() API braindamage

Ingo Molnar (4):
  x86/boot/e820: Fix and clean up e820_type switch() statements
  x86/boot/e820: Separate the E820 ABI structures from the in-kernel structures
  x86/boot/e820: Simplify e820__update_table()
  xen, x86/headers: Add <linux/device.h> dependency to <asm/xen/page.h>

 arch/x86/boot/compressed/eboot.c       |  16 ++++----
 arch/x86/boot/compressed/kaslr.c       |   2 +-
 arch/x86/boot/memory.c                 |   4 +-
 arch/x86/include/asm/e820/types.h      |  48 +++++++++++++++++++++++-
 arch/x86/include/asm/xen/page.h        |   1 +
 arch/x86/include/uapi/asm/bootparam.h  |  18 ++++++++-
 arch/x86/include/uapi/asm/e820/types.h |  55 ---------------------------
 arch/x86/kernel/e820.c                 | 108 ++++++++++++++++++++++++++---------------------------
 8 files changed, 128 insertions(+), 124 deletions(-)
 delete mode 100644 arch/x86/include/uapi/asm/e820/types.h

-- 
2.7.4

[PATCH 1/5] x86/boot/e820: Fix and clean up e820_type switch() statements

[Ingo Molnar]

A test-build of e820.o with -Wswitch-enum shows the following warnings:

  arch/x86/kernel/e820.c: In function ‘e820_type_to_string’:
  arch/x86/kernel/e820.c:965:2: warning: enumeration value ‘E820_TYPE_RESERVED’ not handled in switch [-Wswitch-enum]
    switch (entry->type) {
    ^

  arch/x86/kernel/e820.c: In function ‘e820_type_to_iomem_type’:
  arch/x86/kernel/e820.c:979:2: warning: enumeration value ‘E820_TYPE_RESERVED’ not handled in switch [-Wswitch-enum]
    switch (entry->type) {
    ^

  arch/x86/kernel/e820.c: In function ‘e820_type_to_iores_desc’:
  arch/x86/kernel/e820.c:993:2: warning: enumeration value ‘E820_TYPE_RESERVED’ not handled in switch [-Wswitch-enum]
    switch (entry->type) {
    ^

  arch/x86/kernel/e820.c: In function ‘do_mark_busy’:
  arch/x86/kernel/e820.c:1015:2: warning: enumeration value ‘E820_TYPE_RAM’ not handled in switch [-Wswitch-enum]
    switch (type) {
	    ^

Here's the four warnings:

  - The one in e820_type_to_string() is a borderline bug, we should differentiate
    known-reserved E820 types from unknown types. Fix it by printing a separate
    message for unknown E820 types.

  - The ones in e820_type_to_iomem_type(), e820_type_to_iores_desc() and
    do_mark_busy() are worth documenting, at least to the extent of
    enumerating them explicitly.

Cc: Alex Thorlton <athorlton@sgi.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Huang, Ying <ying.huang@intel.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul Jackson <pj@sgi.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rafael J. Wysocki <rjw@sisk.pl>
Cc: Tejun Heo <tj@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Wei Yang <richard.weiyang@gmail.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/e820.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index d2c6468a8d38..20834a81854e 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -970,7 +970,8 @@ static const char *__init e820_type_to_string(struct e820_entry *entry)
 	case E820_TYPE_UNUSABLE:	return "Unusable memory";
 	case E820_TYPE_PRAM:		return "Persistent Memory (legacy)";
 	case E820_TYPE_PMEM:		return "Persistent Memory";
-	default:			return "Reserved";
+	case E820_TYPE_RESERVED:	return "Reserved";
+	default:			return "Unknown E820 type";
 	}
 }
 
@@ -984,6 +985,7 @@ static unsigned long __init e820_type_to_iomem_type(struct e820_entry *entry)
 	case E820_TYPE_UNUSABLE:	/* Fall-through: */
 	case E820_TYPE_PRAM:		/* Fall-through: */
 	case E820_TYPE_PMEM:		/* Fall-through: */
+	case E820_TYPE_RESERVED:	/* Fall-through: */
 	default:			return IORESOURCE_MEM;
 	}
 }
@@ -998,11 +1000,12 @@ static unsigned long __init e820_type_to_iores_desc(struct e820_entry *entry)
 	case E820_TYPE_RESERVED_KERN:	/* Fall-through: */
 	case E820_TYPE_RAM:		/* Fall-through: */
 	case E820_TYPE_UNUSABLE:	/* Fall-through: */
+	case E820_TYPE_RESERVED:	/* Fall-through: */
 	default:			return IORES_DESC_NONE;
 	}
 }
 
-static bool __init do_mark_busy(u32 type, struct resource *res)
+static bool __init do_mark_busy(enum e820_type type, struct resource *res)
 {
 	/* this is the legacy bios/dos rom-shadow + mmio region */
 	if (res->start < (1ULL<<20))
@@ -1017,6 +1020,11 @@ static bool __init do_mark_busy(u32 type, struct resource *res)
 	case E820_TYPE_PRAM:
 	case E820_TYPE_PMEM:
 		return false;
+	case E820_TYPE_RESERVED_KERN:
+	case E820_TYPE_RAM:
+	case E820_TYPE_ACPI:
+	case E820_TYPE_NVS:
+	case E820_TYPE_UNUSABLE:
 	default:
 		return true;
 	}
-- 
2.7.4

[PATCH 2/5] x86/boot/e820: Separate the E820 ABI structures from the in-kernel structures

[Ingo Molnar]

Linus pointed out that relying on the compiler to pack structures with
enums is fragile not just for the kernel, but for external tooling as
well which might rely on our UAPI headers.

So separate the two from each other: introduce 'struct boot_e820_entry',
which is the boot protocol entry format.

This actually simplifies the code, as e820__update_table() is now never
called directly with boot protocol table entries - we can rely on
append_e820_table() and do a e820__update_table() call afterwards.

( This will allow further simplifications of __e820__update_table(),
  but that will be done in a separate patch. )

This change also has the side effect of not modifying the bootparams structure
anymore - which might be useful for debugging. In theory we could even constify
the boot_params structure - at least from the E820 code's point of view.

Remove the uapi/asm/e820/types.h file, as it's not used anymore - all
kernel side E820 types are defined in asm/e820/types.h.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Alex Thorlton <athorlton@sgi.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Huang, Ying <ying.huang@intel.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Paul Jackson <pj@sgi.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rafael J. Wysocki <rjw@sisk.pl>
Cc: Tejun Heo <tj@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Wei Yang <richard.weiyang@gmail.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/boot/compressed/eboot.c       | 16 ++++++++--------
 arch/x86/boot/compressed/kaslr.c       |  2 +-
 arch/x86/boot/memory.c                 |  4 ++--
 arch/x86/include/asm/e820/types.h      | 48 ++++++++++++++++++++++++++++++++++++++++++++++-
 arch/x86/include/uapi/asm/bootparam.h  | 18 ++++++++++++++++--
 arch/x86/include/uapi/asm/e820/types.h | 55 ------------------------------------------------------
 arch/x86/kernel/e820.c                 | 20 +++++++++-----------
 7 files changed, 83 insertions(+), 80 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 4cfba2f79dfd..a6099d7c39f6 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -900,7 +900,7 @@ static void add_e820ext(struct boot_params *params,
 	unsigned long size;
 
 	e820ext->type = SETUP_E820_EXT;
-	e820ext->len = nr_entries * sizeof(struct e820_entry);
+	e820ext->len = nr_entries * sizeof(struct boot_e820_entry);
 	e820ext->next = 0;
 
 	data = (struct setup_data *)(unsigned long)params->hdr.setup_data;
@@ -917,9 +917,9 @@ static void add_e820ext(struct boot_params *params,
 static efi_status_t setup_e820(struct boot_params *params,
 			       struct setup_data *e820ext, u32 e820ext_size)
 {
-	struct e820_entry *e820_table = &params->e820_table[0];
+	struct boot_e820_entry *entry = params->e820_table;
 	struct efi_info *efi = &params->efi_info;
-	struct e820_entry *prev = NULL;
+	struct boot_e820_entry *prev = NULL;
 	u32 nr_entries;
 	u32 nr_desc;
 	int i;
@@ -990,13 +990,13 @@ static efi_status_t setup_e820(struct boot_params *params,
 				return EFI_BUFFER_TOO_SMALL;
 
 			/* boot_params map full, switch to e820 extended */
-			e820_table = (struct e820_entry *)e820ext->data;
+			entry = (struct boot_e820_entry *)e820ext->data;
 		}
 
-		e820_table->addr = d->phys_addr;
-		e820_table->size = d->num_pages << PAGE_SHIFT;
-		e820_table->type = e820_type;
-		prev = e820_table++;
+		entry->addr = d->phys_addr;
+		entry->size = d->num_pages << PAGE_SHIFT;
+		entry->type = e820_type;
+		prev = entry++;
 		nr_entries++;
 	}
 
diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index e8155eab5474..6d9a546ec7ae 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -426,7 +426,7 @@ static unsigned long slots_fetch_random(void)
 	return 0;
 }
 
-static void process_e820_entry(struct e820_entry *entry,
+static void process_e820_entry(struct boot_e820_entry *entry,
 			       unsigned long minimum,
 			       unsigned long image_size)
 {
diff --git a/arch/x86/boot/memory.c b/arch/x86/boot/memory.c
index db62445b75aa..d9c28c87e477 100644
--- a/arch/x86/boot/memory.c
+++ b/arch/x86/boot/memory.c
@@ -21,8 +21,8 @@ static int detect_memory_e820(void)
 {
 	int count = 0;
 	struct biosregs ireg, oreg;
-	struct e820_entry *desc = boot_params.e820_table;
-	static struct e820_entry buf; /* static so it is zeroed */
+	struct boot_e820_entry *desc = boot_params.e820_table;
+	static struct boot_e820_entry buf; /* static so it is zeroed */
 
 	initregs(&ireg);
 	ireg.ax  = 0xe820;
diff --git a/arch/x86/include/asm/e820/types.h b/arch/x86/include/asm/e820/types.h
index cf6074f8d563..4adeed03a9a1 100644
--- a/arch/x86/include/asm/e820/types.h
+++ b/arch/x86/include/asm/e820/types.h
@@ -1,7 +1,53 @@
 #ifndef _ASM_E820_TYPES_H
 #define _ASM_E820_TYPES_H
 
-#include <uapi/asm/e820/types.h>
+#include <uapi/asm/bootparam.h>
+
+/*
+ * These are the E820 types known to the kernel:
+ */
+enum e820_type {
+	E820_TYPE_RAM		= 1,
+	E820_TYPE_RESERVED	= 2,
+	E820_TYPE_ACPI		= 3,
+	E820_TYPE_NVS		= 4,
+	E820_TYPE_UNUSABLE	= 5,
+	E820_TYPE_PMEM		= 7,
+
+	/*
+	 * This is a non-standardized way to represent ADR or
+	 * NVDIMM regions that persist over a reboot.
+	 *
+	 * The kernel will ignore their special capabilities
+	 * unless the CONFIG_X86_PMEM_LEGACY=y option is set.
+	 *
+	 * ( Note that older platforms also used 6 for the same
+	 *   type of memory, but newer versions switched to 12 as
+	 *   6 was assigned differently. Some time they will learn... )
+	 */
+	E820_TYPE_PRAM		= 12,
+
+	/*
+	 * Reserved RAM used by the kernel itself if
+	 * CONFIG_INTEL_TXT=y is enabled, memory of this type
+	 * will be included in the S3 integrity calculation
+	 * and so should not include any memory that the BIOS
+	 * might alter over the S3 transition:
+	 */
+	E820_TYPE_RESERVED_KERN	= 128,
+};
+
+/*
+ * A single E820 map entry, describing a memory range of [addr...addr+size-1],
+ * of 'type' memory type:
+ *
+ * (We pack it because there can be thousands of them on large systems.)
+ */
+struct e820_entry {
+	u64			addr;
+	u64			size;
+	enum e820_type		type;
+} __attribute__((packed));
 
 /*
  * The legacy E820 BIOS limits us to 128 (E820_MAX_ENTRIES_ZEROPAGE) nodes
diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index 7f04c45aa429..2a5fd6bb0601 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -34,7 +34,6 @@
 #include <linux/screen_info.h>
 #include <linux/apm_bios.h>
 #include <linux/edd.h>
-#include <uapi/asm/e820/types.h>
 #include <asm/ist.h>
 #include <video/edid.h>
 
@@ -111,6 +110,21 @@ struct efi_info {
 	__u32 efi_memmap_hi;
 };
 
+/*
+ * This is the maximum number of entries in struct boot_params::e820_table
+ * (the zeropage), which is part of the x86 boot protocol ABI:
+ */
+#define E820_MAX_ENTRIES_ZEROPAGE 128
+
+/*
+ * The E820 memory region entry of the boot protocol ABI:
+ */
+struct boot_e820_entry {
+	__u64 addr;
+	__u64 size;
+	__u32 type;
+} __attribute__((packed));
+
 /* The so-called "zeropage" */
 struct boot_params {
 	struct screen_info screen_info;			/* 0x000 */
@@ -152,7 +166,7 @@ struct boot_params {
 	struct setup_header hdr;    /* setup header */	/* 0x1f1 */
 	__u8  _pad7[0x290-0x1f1-sizeof(struct setup_header)];
 	__u32 edd_mbr_sig_buffer[EDD_MBR_SIG_MAX];	/* 0x290 */
-	struct e820_entry e820_table[E820_MAX_ENTRIES_ZEROPAGE]; /* 0x2d0 */
+	struct boot_e820_entry e820_table[E820_MAX_ENTRIES_ZEROPAGE]; /* 0x2d0 */
 	__u8  _pad8[48];				/* 0xcd0 */
 	struct edd_info eddbuf[EDDMAXNR];		/* 0xd00 */
 	__u8  _pad9[276];				/* 0xeec */
diff --git a/arch/x86/include/uapi/asm/e820/types.h b/arch/x86/include/uapi/asm/e820/types.h
deleted file mode 100644
index 3ac962f724f5..000000000000
--- a/arch/x86/include/uapi/asm/e820/types.h
+++ /dev/null
@@ -1,55 +0,0 @@
-#ifndef _UAPI_ASM_E820_TYPES_H
-#define _UAPI_ASM_E820_TYPES_H
-
-/*
- * This is the maximum number of entries in struct boot_params::e820_table (the zeropage),
- * which is part of the x86 boot protocol ABI:
- */
-#define E820_MAX_ENTRIES_ZEROPAGE 128
-
-#ifndef __ASSEMBLY__
-
-enum e820_type {
-	E820_TYPE_RAM		= 1,
-	E820_TYPE_RESERVED	= 2,
-	E820_TYPE_ACPI		= 3,
-	E820_TYPE_NVS		= 4,
-	E820_TYPE_UNUSABLE	= 5,
-	E820_TYPE_PMEM		= 7,
-
-	/*
-	 * This is a non-standardized way to represent ADR or
-	 * NVDIMM regions that persist over a reboot.
-	 *
-	 * The kernel will ignore their special capabilities
-	 * unless the CONFIG_X86_PMEM_LEGACY=y option is set.
-	 *
-	 * ( Note that older platforms also used 6 for the same
-	 *   type of memory, but newer versions switched to 12 as
-	 *   6 was assigned differently. Some time they will learn... )
-	 */
-	E820_TYPE_PRAM		= 12,
-
-	/*
-	 * Reserved RAM used by the kernel itself if
-	 * CONFIG_INTEL_TXT=y is enabled, memory of this type
-	 * will be included in the S3 integrity calculation
-	 * and so should not include any memory that the BIOS
-	 * might alter over the S3 transition:
-	 */
-	E820_TYPE_RESERVED_KERN	= 128,
-};
-
-/*
- * A single E820 map entry, describing a memory range of [addr...addr+size-1],
- * of 'type' memory type:
- */
-struct e820_entry {
-	__u64			addr;
-	__u64			size;
-	enum e820_type		type;
-} __attribute__((packed));
-
-#endif /* __ASSEMBLY__ */
-
-#endif /* _UAPI_ASM_E820_TYPES_H */
diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 20834a81854e..2da2f7238a72 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -366,9 +366,9 @@ int __init e820__update_table(struct e820_table *table)
 	return __e820__update_table(table->entries, ARRAY_SIZE(table->entries), &table->nr_entries);
 }
 
-static int __init __append_e820_table(struct e820_entry *entries, u32 nr_entries)
+static int __init __append_e820_table(struct boot_e820_entry *entries, u32 nr_entries)
 {
-	struct e820_entry *entry = entries;
+	struct boot_e820_entry *entry = entries;
 
 	while (nr_entries) {
 		u64 start = entry->addr;
@@ -397,7 +397,7 @@ static int __init __append_e820_table(struct e820_entry *entries, u32 nr_entries
  * will have given us a memory map that we can use to properly
  * set up memory.  If we aren't, we'll fake a memory map.
  */
-static int __init append_e820_table(struct e820_entry *entries, u32 nr_entries)
+static int __init append_e820_table(struct boot_e820_entry *entries, u32 nr_entries)
 {
 	/* Only one memory region (or negative)? Ignore it */
 	if (nr_entries < 2)
@@ -668,12 +668,12 @@ __init void e820__reallocate_tables(void)
 void __init e820__memory_setup_extended(u64 phys_addr, u32 data_len)
 {
 	int entries;
-	struct e820_entry *extmap;
+	struct boot_e820_entry *extmap;
 	struct setup_data *sdata;
 
 	sdata = early_memremap(phys_addr, data_len);
 	entries = sdata->len / sizeof(*extmap);
-	extmap = (struct e820_entry *)(sdata->data);
+	extmap = (struct boot_e820_entry *)(sdata->data);
 
 	__append_e820_table(extmap, entries);
 	e820__update_table(e820_table);
@@ -1140,7 +1140,6 @@ void __init e820__reserve_resources_late(void)
 char *__init e820__memory_setup_default(void)
 {
 	char *who = "BIOS-e820";
-	u32 new_nr;
 
 	/*
 	 * Try to copy the BIOS-supplied E820-map.
@@ -1148,10 +1147,6 @@ char *__init e820__memory_setup_default(void)
 	 * Otherwise fake a memory map; one section from 0k->640k,
 	 * the next section from 1mb->appropriate_mem_k
 	 */
-	new_nr = boot_params.e820_entries;
-	__e820__update_table(boot_params.e820_table, ARRAY_SIZE(boot_params.e820_table), &new_nr);
-	boot_params.e820_entries = new_nr;
-
 	if (append_e820_table(boot_params.e820_table, boot_params.e820_entries) < 0) {
 		u64 mem_size;
 
@@ -1169,6 +1164,9 @@ char *__init e820__memory_setup_default(void)
 		e820__range_add(HIGH_MEMORY, mem_size << 10, E820_TYPE_RAM);
 	}
 
+	/* We just appended a lot of ranges, sanitize the table: */
+	e820__update_table(e820_table);
+
 	return who;
 }
 
@@ -1182,7 +1180,7 @@ void __init e820__memory_setup(void)
 	char *who;
 
 	/* This is a firmware interface ABI - make sure we don't break it: */
-	BUILD_BUG_ON(sizeof(struct e820_entry) != 20);
+	BUILD_BUG_ON(sizeof(struct boot_e820_entry) != 20);
 
 	who = x86_init.resources.memory_setup();
 
-- 
2.7.4

[PATCH 3/5] x86/boot/e820: Simplify e820__update_table()

[Ingo Molnar]

 - Remove the now unnecessary __e820__update_table() wrappery

 - Move statics out from function scope, to make the logic clearer

 - Rename local variables to be more in line with the rest of 820.c

 - Remove unnecessary local variables: old_nr, *nr_entries

No change in functionality.

Cc: Alex Thorlton <athorlton@sgi.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Huang, Ying <ying.huang@intel.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul Jackson <pj@sgi.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rafael J. Wysocki <rjw@sisk.pl>
Cc: Tejun Heo <tj@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Wei Yang <richard.weiyang@gmail.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/e820.c | 70 +++++++++++++++++++++++++++++++---------------------------------------
 1 file changed, 31 insertions(+), 39 deletions(-)

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 2da2f7238a72..e2fb20ac5135 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -235,6 +235,11 @@ struct change_member {
 	unsigned long long	addr;
 };
 
+static struct change_member	change_point_list[2*E820_MAX_ENTRIES]	__initdata;
+static struct change_member	*change_point[2*E820_MAX_ENTRIES]	__initdata;
+static struct e820_entry	*overlap_list[E820_MAX_ENTRIES]		__initdata;
+static struct e820_entry	new_entries[E820_MAX_ENTRIES]		__initdata;
+
 static int __init cpcompare(const void *a, const void *b)
 {
 	struct change_member * const *app = a, * const *bpp = b;
@@ -252,51 +257,46 @@ static int __init cpcompare(const void *a, const void *b)
 	return (ap->addr != ap->entry->addr) - (bp->addr != bp->entry->addr);
 }
 
-static int __init __e820__update_table(struct e820_entry *entries, u32 max_nr_entries, u32 *nr_entries)
+int __init e820__update_table(struct e820_table *table)
 {
-	static struct change_member change_point_list[2*E820_MAX_ENTRIES] __initdata;
-	static struct change_member *change_point[2*E820_MAX_ENTRIES] __initdata;
-	static struct e820_entry *overlap_list[E820_MAX_ENTRIES] __initdata;
-	static struct e820_entry new_entries[E820_MAX_ENTRIES] __initdata;
+	struct e820_entry *entries = table->entries;
+	u32 max_nr_entries = ARRAY_SIZE(table->entries);
 	enum e820_type current_type, last_type;
 	unsigned long long last_addr;
-	u32 chgidx;
-	u32 overlap_entries;
-	u32 new_nr_entries;
-	u32 old_nr, new_nr, chg_nr;
-	u32 i;
+	u32 new_nr_entries, overlap_entries;
+	u32 i, chg_idx, chg_nr;
 
 	/* If there's only one memory region, don't bother: */
-	if (*nr_entries < 2)
+	if (table->nr_entries < 2)
 		return -1;
 
-	old_nr = *nr_entries;
-	BUG_ON(old_nr > max_nr_entries);
+	table->nr_entries = table->nr_entries;
+	BUG_ON(table->nr_entries > max_nr_entries);
 
 	/* Bail out if we find any unreasonable addresses in the map: */
-	for (i = 0; i < old_nr; i++) {
+	for (i = 0; i < table->nr_entries; i++) {
 		if (entries[i].addr + entries[i].size < entries[i].addr)
 			return -1;
 	}
 
 	/* Create pointers for initial change-point information (for sorting): */
-	for (i = 0; i < 2 * old_nr; i++)
+	for (i = 0; i < 2 * table->nr_entries; i++)
 		change_point[i] = &change_point_list[i];
 
 	/*
 	 * Record all known change-points (starting and ending addresses),
 	 * omitting empty memory regions:
 	 */
-	chgidx = 0;
-	for (i = 0; i < old_nr; i++)	{
+	chg_idx = 0;
+	for (i = 0; i < table->nr_entries; i++)	{
 		if (entries[i].size != 0) {
-			change_point[chgidx]->addr	= entries[i].addr;
-			change_point[chgidx++]->entry	= &entries[i];
-			change_point[chgidx]->addr	= entries[i].addr + entries[i].size;
-			change_point[chgidx++]->entry	= &entries[i];
+			change_point[chg_idx]->addr	= entries[i].addr;
+			change_point[chg_idx++]->entry	= &entries[i];
+			change_point[chg_idx]->addr	= entries[i].addr + entries[i].size;
+			change_point[chg_idx++]->entry	= &entries[i];
 		}
 	}
-	chg_nr = chgidx;
+	chg_nr = chg_idx;
 
 	/* Sort change-point list by memory addresses (low -> high): */
 	sort(change_point, chg_nr, sizeof(*change_point), cpcompare, NULL);
@@ -308,15 +308,15 @@ static int __init __e820__update_table(struct e820_entry *entries, u32 max_nr_en
 	last_addr = 0;		 /* Start with 0 as last starting address */
 
 	/* Loop through change-points, determining effect on the new map: */
-	for (chgidx = 0; chgidx < chg_nr; chgidx++) {
+	for (chg_idx = 0; chg_idx < chg_nr; chg_idx++) {
 		/* Keep track of all overlapping entries */
-		if (change_point[chgidx]->addr == change_point[chgidx]->entry->addr) {
+		if (change_point[chg_idx]->addr == change_point[chg_idx]->entry->addr) {
 			/* Add map entry to overlap list (> 1 entry implies an overlap) */
-			overlap_list[overlap_entries++] = change_point[chgidx]->entry;
+			overlap_list[overlap_entries++] = change_point[chg_idx]->entry;
 		} else {
 			/* Remove entry from list (order independent, so swap with last): */
 			for (i = 0; i < overlap_entries; i++) {
-				if (overlap_list[i] == change_point[chgidx]->entry)
+				if (overlap_list[i] == change_point[chg_idx]->entry)
 					overlap_list[i] = overlap_list[overlap_entries-1];
 			}
 			overlap_entries--;
@@ -335,7 +335,7 @@ static int __init __e820__update_table(struct e820_entry *entries, u32 max_nr_en
 		/* Continue building up new map based on this information: */
 		if (current_type != last_type || current_type == E820_TYPE_PRAM) {
 			if (last_type != 0)	 {
-				new_entries[new_nr_entries].size = change_point[chgidx]->addr - last_addr;
+				new_entries[new_nr_entries].size = change_point[chg_idx]->addr - last_addr;
 				/* Move forward only if the new size was non-zero: */
 				if (new_entries[new_nr_entries].size != 0)
 					/* No more space left for new entries? */
@@ -343,29 +343,21 @@ static int __init __e820__update_table(struct e820_entry *entries, u32 max_nr_en
 						break;
 			}
 			if (current_type != 0)	{
-				new_entries[new_nr_entries].addr = change_point[chgidx]->addr;
+				new_entries[new_nr_entries].addr = change_point[chg_idx]->addr;
 				new_entries[new_nr_entries].type = current_type;
-				last_addr = change_point[chgidx]->addr;
+				last_addr = change_point[chg_idx]->addr;
 			}
 			last_type = current_type;
 		}
 	}
 
-	/* Retain count for the new entries: */
-	new_nr = new_nr_entries;
-
 	/* Copy the new entries into the original location: */
-	memcpy(entries, new_entries, new_nr*sizeof(*entries));
-	*nr_entries = new_nr;
+	memcpy(entries, new_entries, new_nr_entries*sizeof(*entries));
+	table->nr_entries = new_nr_entries;
 
 	return 0;
 }
 
-int __init e820__update_table(struct e820_table *table)
-{
-	return __e820__update_table(table->entries, ARRAY_SIZE(table->entries), &table->nr_entries);
-}
-
 static int __init __append_e820_table(struct boot_e820_entry *entries, u32 nr_entries)
 {
 	struct boot_e820_entry *entry = entries;
-- 
2.7.4

[PATCH 4/5] xen, x86/headers: Add <linux/device.h> dependency to <asm/xen/page.h>

[Ingo Molnar]

The following patch (not upstream yet):

  "x86/boot/e820: Remove spurious asm/e820/api.h inclusions"

Removed the (spurious) <asm/e820.h> include line from <asm/pgtable.h> to
reduce header file dependencies - but a Xen header has (unintentionally)
learned to rely on the indirect inclusion of <linux/device.h>.

This resulted in the following (harmless) build warning:

   arch/x86/include/asm/xen/page.h:302:7: warning: 'struct device' declared inside parameter list

Include <linux/device.h> explicitly.

No change in functionality.

Reported-by: kbuild test robot <fengguang.wu@intel.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: <stefano.stabellini@eu.citrix.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/xen/page.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/include/asm/xen/page.h b/arch/x86/include/asm/xen/page.h
index 33cbd3db97b9..64c5e745ebad 100644
--- a/arch/x86/include/asm/xen/page.h
+++ b/arch/x86/include/asm/xen/page.h
@@ -6,6 +6,7 @@
 #include <linux/spinlock.h>
 #include <linux/pfn.h>
 #include <linux/mm.h>
+#include <linux/device.h>
 
 #include <linux/uaccess.h>
 #include <asm/page.h>
-- 
2.7.4

Re: [PATCH 4/5] xen, x86/headers: Add <linux/device.h> dependency to <asm/xen/page.h>

[Juergen Gross]

On 01/02/17 10:51, Ingo Molnar wrote:
> The following patch (not upstream yet):
> 
>   "x86/boot/e820: Remove spurious asm/e820/api.h inclusions"
> 
> Removed the (spurious) <asm/e820.h> include line from <asm/pgtable.h> to
> reduce header file dependencies - but a Xen header has (unintentionally)
> learned to rely on the indirect inclusion of <linux/device.h>.
> 
> This resulted in the following (harmless) build warning:
> 
>    arch/x86/include/asm/xen/page.h:302:7: warning: 'struct device' declared inside parameter list
> 
> Include <linux/device.h> explicitly.
> 
> No change in functionality.
> 
> Reported-by: kbuild test robot <fengguang.wu@intel.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: <stefano.stabellini@eu.citrix.com>
> Cc: Juergen Gross <jgross@suse.com>

Hmm, how did you send this? In spite of the Cc: list in the patch only
few of those listed here are included in the Cc: header of the mail.

> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Ingo Molnar <mingo@kernel.org>

Acked-by: Juergen Gross <jgross@suse.com>


Juergen

Re: [PATCH 4/5] xen, x86/headers: Add <linux/device.h> dependency to <asm/xen/page.h>

[Ingo Molnar]

* Juergen Gross <jgross@suse.com> wrote:

> On 01/02/17 10:51, Ingo Molnar wrote:
> > The following patch (not upstream yet):
> > 
> >   "x86/boot/e820: Remove spurious asm/e820/api.h inclusions"
> > 
> > Removed the (spurious) <asm/e820.h> include line from <asm/pgtable.h> to
> > reduce header file dependencies - but a Xen header has (unintentionally)
> > learned to rely on the indirect inclusion of <linux/device.h>.
> > 
> > This resulted in the following (harmless) build warning:
> > 
> >    arch/x86/include/asm/xen/page.h:302:7: warning: 'struct device' declared inside parameter list
> > 
> > Include <linux/device.h> explicitly.
> > 
> > No change in functionality.
> > 
> > Reported-by: kbuild test robot <fengguang.wu@intel.com>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Cc: <stefano.stabellini@eu.citrix.com>
> > Cc: Juergen Gross <jgross@suse.com>
> 
> Hmm, how did you send this? In spite of the Cc: list in the patch only
> few of those listed here are included in the Cc: header of the mail.

I restricted it explicitly in the sending script to those I thought might be 
interested in the series, as LKML (and/or gmail) would occasionally reject a mail 
based on too many Cc: entries...

Note that the tip-bot notification would still have Cc:-ed everyone mentioned in 
the Cc: lists, so had I screwed up the patch you'd still have been informed and 
you'd have the opportunity to embarrass me! :-)

> > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Ingo Molnar <mingo@kernel.org>
> 
> Acked-by: Juergen Gross <jgross@suse.com>

Thanks! Added this to the changelog.

	Ingo

[PATCH 5/5] x86/boot: Fix pr_debug() API braindamage

[Ingo Molnar]

From: Borislav Petkov <bp@alien8.de>

What looked like a straightforward conversion from printk(KERN_DEBUG, ...)
to pr_debug() broke the boot log output:

  DMI:    /M57SLI-S4, BIOS FF 01/24/2008
 -e820: update [mem 0x00000000-0x00000fff] usable ==> reserved
 -e820: remove [mem 0x000a0000-0x000fffff] usable
 +usable ==> reserved
 +usable
  e820: last_pfn = 0x230000 max_arch_pfn = 0x400000000

 ...

  x86/PAT: Configuration [0-7]: WB  WC  UC- UC  WB  WC  UC- WT
 -e820: update [mem 0xd0000000-0xffffffff] usable ==> reserved
 +usable ==> reserved

i.e. spurious (and nonsensical) kernel log entries were created...

We need a pr_debug_and_I_mean_it() function which does nothing but
printk(KERN_DEBUG...

Signed-off-by: Borislav Petkov <bp@alien8.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-kernel@vger.kernel.org
[ Wrote changelog. ]
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/e820.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index e2fb20ac5135..6e9b26fa6d05 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -411,7 +411,7 @@ __e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_ty
 		size = ULLONG_MAX - start;
 
 	end = start + size;
-	pr_debug("e820: update [mem %#010Lx-%#010Lx] ", start, end - 1);
+	printk(KERN_DEBUG "e820: update [mem %#010Lx-%#010Lx] ", start, end - 1);
 	e820_print_type(old_type);
 	pr_cont(" ==> ");
 	e820_print_type(new_type);
@@ -487,7 +487,7 @@ u64 __init e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool
 		size = ULLONG_MAX - start;
 
 	end = start + size;
-	pr_debug("e820: remove [mem %#010Lx-%#010Lx] ", start, end - 1);
+	printk(KERN_DEBUG "e820: remove [mem %#010Lx-%#010Lx] ", start, end - 1);
 	if (check_type)
 		e820_print_type(old_type);
 	pr_cont("\n");
@@ -1121,7 +1121,7 @@ void __init e820__reserve_resources_late(void)
 		if (start >= end)
 			continue;
 
-		pr_debug("e820: reserve RAM buffer [mem %#010llx-%#010llx]\n", 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");
 	}
 }
-- 
2.7.4

Re: [PATCH 5/5] x86/boot: Fix pr_debug() API braindamage

[Joe Perches]

On Wed, 2017-02-01 at 10:51 +0100, Ingo Molnar wrote:
> From: Borislav Petkov <bp@alien8.de>

> What looked like a straightforward conversion from printk(KERN_DEBUG, ...)
> to pr_debug() broke the boot log output:
> 
>   DMI:    /M57SLI-S4, BIOS FF 01/24/2008
>  -e820: update [mem 0x00000000-0x00000fff] usable ==> reserved
>  -e820: remove [mem 0x000a0000-0x000fffff] usable
>  +usable ==> reserved
>  +usable
>   e820: last_pfn = 0x230000 max_arch_pfn = 0x400000000

<sigh>

Ingo, why didn't you fix this several days ago
when I pointed out this breakage right after
you posted your original patch?

https://lkml.org/lkml/2017/1/28/246