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

From: Ingo Molnar
Date: Sun Jan 29 2017 - 10:23:55 EST

* Ingo Molnar <mingo@xxxxxxxxxx> 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

- 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,