lkml.org 
[lkml]   [2017]   [Jan]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patches in this message
/
Date
From
SubjectRe: [PATCH 37/50] x86/boot/e820: Use 'enum e820_type' in 'struct e820_entry'

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

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

\
 
 \ /
  Last update: 2017-01-29 10:30    [W:0.418 / U:29.264 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site