Message-ID: <20170129091912.GA26237@gmail.com>
Date:   Sun, 29 Jan 2017 10:19:12 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Andy Lutomirski <luto@...capital.net>,
        Borislav Petkov <bp@...en8.de>,
        "H . Peter Anvin" <hpa@...or.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Yinghai Lu <yinghai@...nel.org>
Subject: Re: [PATCH 37/50] x86/boot/e820: Use 'enum e820_type' in 'struct
 e820_entry'


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

> On Sat, Jan 28, 2017 at 2:11 PM, Ingo Molnar <mingo@...nel.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

Powered by blists - more mailing lists