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

[Ingo Molnar]

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

==================>