From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750854AbdA2J3K (ORCPT ); Sun, 29 Jan 2017 04:29:10 -0500 Received: from mail-wm0-f68.google.com ([74.125.82.68]:34743 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750765AbdA2J3H (ORCPT ); Sun, 29 Jan 2017 04:29:07 -0500 Date: Sun, 29 Jan 2017 10:19:12 +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: Re: [PATCH 37/50] x86/boot/e820: Use 'enum e820_type' in 'struct e820_entry' Message-ID: <20170129091912.GA26237@gmail.com> References: <1485641531-22124-1-git-send-email-mingo@kernel.org> <1485641531-22124-38-git-send-email-mingo@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: 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 * Linus Torvalds wrote: > On Sat, Jan 28, 2017 at 2:11 PM, Ingo Molnar 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