Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1569042 > unrolled thread

[PATCH 37/50] x86/boot/e820: Use 'enum e820_type' in 'struct e820_entry'

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 37/50] x86/boot/e820: Use 'enum e820_type' in 'struct e820_entry' Ingo Molnar <mingo@kernel.org> - 2017-01-28 23:30 +0100
    Re: [PATCH 37/50] x86/boot/e820: Use 'enum e820_type' in 'struct e820_entry' Linus Torvalds <torvalds@linux-foundation.org> - 2017-01-29 00:10 +0100
      Re: [PATCH 37/50] x86/boot/e820: Use 'enum e820_type' in 'struct  e820_entry' Ingo Molnar <mingo@kernel.org> - 2017-01-29 10:30 +0100
        [PATCH] x86/boot/e820: Separate the E820 ABI structures from the  in-kernel structures Ingo Molnar <mingo@kernel.org> - 2017-01-29 16:30 +0100
        Re: [PATCH 37/50] x86/boot/e820: Use 'enum e820_type' in 'struct e820_entry' Linus Torvalds <torvalds@linux-foundation.org> - 2017-01-29 20:20 +0100

#1569042 — [PATCH 37/50] x86/boot/e820: Use 'enum e820_type' in 'struct e820_entry'

Use a stricter type for struct e820_entry. Add a build-time check to make
sure the compiler won't ever pack the enum into a field smaller than
'int'.

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/include/asm/e820/types.h      | 31 -------------------------------
 arch/x86/include/uapi/asm/e820/types.h | 37 ++++++++++++++++++++++++++++++++++---
 arch/x86/kernel/e820.c                 |  3 +++
 3 files changed, 37 insertions(+), 34 deletions(-)

diff --git a/arch/x86/include/asm/e820/types.h b/arch/x86/include/asm/e820/types.h
index 1c3426825535..eb313b62d548 100644
--- a/arch/x86/include/asm/e820/types.h
+++ b/arch/x86/include/asm/e820/types.h
@@ -1,37 +1,6 @@
 #ifndef _ASM_E820_TYPES_H
 #define _ASM_E820_TYPES_H
 
-enum e820_type {
-	E820_RAM		= 1,
-	E820_RESERVED		= 2,
-	E820_ACPI		= 3,
-	E820_NVS		= 4,
-	E820_UNUSABLE		= 5,
-	E820_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_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_RESERVED_KERN	= 128,
-};
-
 #include <uapi/asm/e820/types.h>
 
 /*
diff --git a/arch/x86/include/uapi/asm/e820/types.h b/arch/x86/include/uapi/asm/e820/types.h
index 54b812e80bac..65c7a1beab22 100644
--- a/arch/x86/include/uapi/asm/e820/types.h
+++ b/arch/x86/include/uapi/asm/e820/types.h
@@ -6,14 +6,45 @@
 
 #ifndef __ASSEMBLY__
 
+enum e820_type {
+	E820_RAM		= 1,
+	E820_RESERVED		= 2,
+	E820_ACPI		= 3,
+	E820_NVS		= 4,
+	E820_UNUSABLE		= 5,
+	E820_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_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_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;
-	__u32 type;
+	__u64			addr;
+	__u64			size;
+	enum e820_type		type;
 } __attribute__((packed));
 
 #endif /* __ASSEMBLY__ */
diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 511402d88795..010a289ddd9f 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -1164,6 +1164,9 @@ 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);
+
 	who = x86_init.resources.memory_setup();
 
 	memcpy(e820_table_firmware, e820_table, sizeof(struct e820_table));
-- 
2.7.4

[toc] | [next] | [standalone]

#1569064

On Sat, Jan 28, 2017 at 2:11 PM, Ingo Molnar <mingo@kernel.org> wrote:
> Use a stricter type for struct e820_entry. Add a build-time check to make
> sure the compiler won't ever pack the enum into a field smaller than
> 'int'.

I'm not sure this is a good idea. In fact, I'm pretty sure it's a horrible idea.

The compiler that *users* use might decide that the "enum" fits in a
8-bit unsigned char, and decide to use that. The kernel build won't
notice and the BUG_ON() won't help, because we use a different
compiler.

(Or even if it's the same compiler you can have build flags - the size
of an enum very much depends on various compiler options, particularly
"--short-enums" for gcc).

Basically, we should not use "enum"s in types exported to user space.
The size just isn't sufficiently well defined, and it's a maintenance
nightmare.

Use explicitly sized members only, please. No "enum".

            Linus

[toc] | [prev] | [next] | [standalone]

#1569167 — Re: [PATCH 37/50] x86/boot/e820: Use 'enum e820_type' in 'struct e820_entry'

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sat, Jan 28, 2017 at 2:11 PM, Ingo Molnar <mingo@kernel.org> wrote:
> > Use a stricter type for struct e820_entry. Add a build-time check to make
> > sure the compiler won't ever pack the enum into a field smaller than
> > 'int'.
> 
> I'm not sure this is a good idea. In fact, I'm pretty sure it's a horrible idea.
> 
> The compiler that *users* use might decide that the "enum" fits in a
> 8-bit unsigned char, and decide to use that. The kernel build won't
> notice and the BUG_ON() won't help, because we use a different
> compiler.

Hm, good point, have not considered that.

> (Or even if it's the same compiler you can have build flags - the size
> of an enum very much depends on various compiler options, particularly
> "--short-enums" for gcc).
> 
> Basically, we should not use "enum"s in types exported to user space.
> The size just isn't sufficiently well defined, and it's a maintenance
> nightmare.
> 
> Use explicitly sized members only, please. No "enum".

Ok.

Would it be acceptable to use enum for the kernel internal representation (our 
e820_table structures never actually comes directly, we construct it ourselves), 
and maintain the very explicitly sized ABI type for the boot protocol only (i.e. 
uapi/asm/bootparam.h)?

( I actually partially implemented that originally, but chickened out after 
  running into header dependency hell - but I think it could be tackled with a bit 
  more effort. )

The vagueness of the C spec about enum size and the resulting enum auto-packing 
efforts of compilers is annoying and a real problem for ABIs, but the reason I'm 
pushing them in this case is that enums also have some advantages within the 
kernel:

 1) they are pretty good self-documenting types

 2) GCC at least has some useful enum warnings for switch() statements, one of 
    which we use in the kernel today:

       -Wswitch

           Warn whenever a "switch" statement has an index of enumerated type and 
           lacks a "case" for one or more of the named codes of that enumeration.  
           (The presence of a "default" label prevents this warning.)  "case" 
           labels outside the enumeration range also provoke warnings when this 
           option is used (even if there is a "default" label).
           This warning is enabled by -Wall.

    There are also more intrusive warnings which found/prevented quite a few bugs
    in various user-space efforts I've been involved with (such as tools/perf):

       -Wswitch-default

           Warn whenever a "switch" statement does not have a "default" case.

       -Wswitch-enum

           Warn whenever a "switch" statement has an index of enumerated type and 
           lacks a "case" for one or more of the named codes of that enumeration.  
           "case" labels outside the enumeration range also provoke warnings when 
           this option is used.  The only difference between -Wswitch and this 
           option is that this option gives a warning about an omitted enumeration 
           code even if there is a "default" label.

So if say one of the e820 functions didn't handle E820_TYPE_PMEM, we'd today 
generate this warning in the kernel build:

  arch/x86/kernel/e820.c: In function ‘e820_print_type’:
  arch/x86/kernel/e820.c:143:2: warning: enumeration value ‘E820_TYPE_PMEM’ not 
  handled in switch [-Wswitch]
    switch (type) {
    ^
  arch/x86/kernel/e820.c:143:2: warning: enumeration value ‘E820_TYPE_PRAM’ not 
  handled in switch [-Wswitch]

And in fact, we could go further than that - the kernel does not enable 
-Wswitch-enum right now (because there are valid patterns that it warns about, 
such as enums with extensive list of values where listing all the values would 
make thecode worse), but when enabled for e820.c it generates this warning:

  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) {
	    ^

All four warnings are interesting IMHO:

The one in e820_type_to_string() should be fixed this way I think:

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";
 	}
 }

As it's useful to have differentiation in output between a known-reserved E820 
range and a totally unknown type range (firmware special or new feature).

The ones in e820_type_to_iomem_type() and e820_type_to_iores_desc() look OK but 
interesting nevertheless, and should probably be fixed this way:

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
@@ -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,6 +1000,7 @@ 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;
 	}
 }

I think it's worth a mention that E820_TYPE_RESERVED is handled differently from 
E820_TYPE_RESERVED_KERN in these two functions: the reason is that 
E820_TYPE_RESERVED_KERN is really a special RAM area and probably somewhat of a 
misnomer - E820_TYPE_RAM_RESERVED might be better.

The one in do_mark_busy() is IMHO interesting as well:

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
@@ -998,7 +1000,7 @@ static unsigned long __init e820_type_to_iores_desc(struct e820_entry *entry)
 	}
 }
 
-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;
 	}

(this patch keeps/documents the status quo.)

As it raises the question of why E820_TYPE_RESERVED is handled differently from 
E820_TYPE_ACPI and E820_TYPE_NVS. Shouldn't E820_TYPE_RESERVED be marked busy as 
well, or do sometimes PCI devices hide there? Even if the code is correct this 
would merit a line of documentation as well, IMHO.

( There's also the -Wswitch-default warning which is not enabled by the kernel 
  build, and which even if enabled does not trigger for e820.o but it's useful as 
  well. )

I.e. I think enum e820_type is a case where very explicit listing of all the 
values in switch() statements actively improves the code, almost unconditionally: 
it resulted in a fix, three improvements in code quality and a couple of questions 
that might result in improved comments.

None of this is possible in such an active way with CPP #defines.

So if you think isolating the enum from the ABI interface would be an acceptable 
solution to the problem we could still use the enum - eat and have the cake and 
such?

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1569216 — [PATCH] x86/boot/e820: Separate the E820 ABI structures from the in-kernel structures

* Ingo Molnar <mingo@kernel.org> wrote:

> > Use explicitly sized members only, please. No "enum".
> 
> Ok.
> 
> Would it be acceptable to use enum for the kernel internal representation (our 
> e820_table structures never actually comes directly, we construct it ourselves), 
                                                     ^-- from the firmware
> and maintain the very explicitly sized ABI type for the boot protocol only (i.e. 
> uapi/asm/bootparam.h)?

I.e. my proposed solution would be to decouple struct e820_table (and thus struct 
e820_entry) by making them kernel internal and thus decoupling them from the boot 
protocol (struct boot_params) - and not expose the enum to UAPI at all, only the 
raw __u32/__u64 fields of the boot protocol.

We already have some pain in the code from the fact that the e820 table of the 
boot protocol is not actually the same as e820_table: table->nr_entries is at a 
different offset in struct boot_params.h. So struct e820_table is already a de 
facto artificial in-kernel construct and has been for a decade or so.

I.e. something like the patch below. (Lightly boot tested on a couple of systems.)

This actually makes things simpler and cleaner all around:

 - Now boot_params is not modified anymore (previous we'd call 
   e820__update_table() on the boot parameters structure itself!).

 - The ugly low level __e820__update_table() is not used anymore (because all 
   e820_table structures now have a standard layout) and can be eliminated in a 
   future patch.

 - Grepping for boot_e820_entry will now actually show all the very low level E820 
   code that directly interfaces with the BIOS or the bootloader. This is a plus.

 - 'struct e820_entry' is now independent of the boot ABI. I boot tested the 
   following experimental change:

|
| --- a/arch/x86/include/asm/e820/types.h
| +++ b/arch/x86/include/asm/e820/types.h
| @@ -44,10 +44,10 @@ enum e820_type {
|   * (We pack it because there can be thousands of them on large systems.)
|   */
|  struct e820_entry {
| +	enum e820_type		type;
| 	u64			addr;
| 	u64			size;
| -	enum e820_type		type;
| -} __attribute__((packed));
| +};
| 
| /*
|  * The legacy E820 BIOS limits us to 128 (E820_MAX_ENTRIES_ZEROPAGE) nodes
|

   ... which changes both the layout and the size of e820_entry on 64-bit systems, 
   and successfully boot-tested it - which is proof that the types are decoupled 
   in practice as well.

   I don't actually think we want to change the ordering and size right now, as 
   distro kernels can have thousands of these entries and their memory efficiency
   matters, but it's nice to know that the decoupling appears to be complete. 
   Maybe some future firmware interface requires a new field, which would now be 
   possible.

 - And of course this patch should also fix the problem you pointed out, the 
   fragility of enum sizes in ABI data structures.

So ... would this be an acceptable approach? (Assuming it works on all systems, 
etc.)

Thanks,

	Ingo

==================>
From c88a9c7e933dd665a47591ebd049a5046c7871c5 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@kernel.org>
Date: Sun, 29 Jan 2017 12:56:13 +0100
Subject: [PATCH] x86/boot/e820: Separate the E820 ABI structures from the in-kernel structures

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();
 

[toc] | [prev] | [next] | [standalone]

#1569266

On Sun, Jan 29, 2017 at 1:19 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> Would it be acceptable to use enum for the kernel internal representation (our
> e820_table structures never actually comes directly, we construct it ourselves),
> and maintain the very explicitly sized ABI type for the boot protocol only (i.e.
> uapi/asm/bootparam.h)?

I guess that would work, assuming that it doesn't cause unnecessary conversions.

Looking at the patch you sent out (assuming that's the complete deal),
it looks reasonable.

               Linus

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web