From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751757AbdA2PWP (ORCPT ); Sun, 29 Jan 2017 10:22:15 -0500 Received: from mail-wm0-f66.google.com ([74.125.82.66]:34584 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751683AbdA2PV0 (ORCPT ); Sun, 29 Jan 2017 10:21:26 -0500 Date: Sun, 29 Jan 2017 13:38:48 +0100 From: Ingo Molnar To: Linus Torvalds Cc: Linux Kernel Mailing List , Andrew Morton , Andy Lutomirski , Borislav Petkov , "H . Peter Anvin" , Peter Zijlstra , Thomas Gleixner , Yinghai Lu Subject: [PATCH] x86/boot/e820: Separate the E820 ABI structures from the in-kernel structures Message-ID: <20170129123847.GA32417@gmail.com> References: <1485641531-22124-1-git-send-email-mingo@kernel.org> <1485641531-22124-38-git-send-email-mingo@kernel.org> <20170129091912.GA26237@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170129091912.GA26237@gmail.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Ingo Molnar 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 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 Cc: Alex Thorlton Cc: Andy Lutomirski Cc: Borislav Petkov Cc: Brian Gerst Cc: Dan Williams Cc: Denys Vlasenko Cc: H. Peter Anvin Cc: Huang, Ying Cc: Josh Poimboeuf Cc: Juergen Gross Cc: Paul Jackson Cc: Peter Zijlstra Cc: Rafael J. Wysocki Cc: Tejun Heo Cc: Thomas Gleixner Cc: Wei Yang Cc: Yinghai Lu Cc: linux-kernel@vger.kernel.org Signed-off-by: Ingo Molnar --- 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 = ¶ms->e820_table[0]; + struct boot_e820_entry *entry = params->e820_table; struct efi_info *efi = ¶ms->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 +#include + +/* + * 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 #include #include -#include #include #include