| Message ID | 1424751767.9050.4.camel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/24/2015 06:22 AM, Dan Williams wrote: <> >> By Popular demand An Extra WARNING message is printed if >> an "UNKNOWN" is found. It will look like this: >> e820: WARNING [mem 0x100000000-0x1ffffffff] is unknown type 12 > > I don't think we need to warn that an unknown range was published, just > warn if it is consumed. > I did not have it at first, Ingo asked for it. I don't mind having it and I don't mind not. I'd say it is Ingo's call. > Something like these incremental changes. I don't see the need for > patch 2 or either version of patch 3. > > diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c > index 1afa5518baa6..2e755a92d84f 100644 > --- a/arch/x86/kernel/e820.c > +++ b/arch/x86/kernel/e820.c > @@ -134,11 +134,6 @@ static void __init __e820_add_region(struct e820map *e820x, u64 start, u64 size, > return; > } > > - if (unlikely(_is_unknown_type(type))) > - pr_warn("e820: WARNING [mem %#010llx-%#010llx] is unknown type %d\n", > - (unsigned long long) start, > - (unsigned long long) (start + size - 1), type); > - Again Ingo's call > e820x->map[x].addr = start; > e820x->map[x].size = size; > e820x->map[x].type = type; > @@ -938,7 +933,7 @@ static inline const char *e820_type_to_string(int e820_type) > case E820_NVS: return "ACPI Non-volatile Storage"; > case E820_UNUSABLE: return "Unusable memory"; > case E820_RESERVED: return "reserved"; > - default: return "reserved-unkown"; > + default: return iomem_unknown_resource_name; > } > } > > diff --git a/include/linux/ioport.h b/include/linux/ioport.h > index 2c5250222278..d857e79b4bf2 100644 > --- a/include/linux/ioport.h > +++ b/include/linux/ioport.h > @@ -194,6 +194,9 @@ extern struct resource * __request_region(struct resource *, > resource_size_t n, > const char *name, int flags); > > +/* For uniquely tagging unknown memory so we can warn when it is consumed */ > +extern const char iomem_unknown_resource_name[]; > + > /* Compatibility cruft */ > #define release_region(start,n) __release_region(&ioport_resource, (start), (n)) > #define check_mem_region(start,n) __check_region(&iomem_resource, (start), (n)) > diff --git a/kernel/resource.c b/kernel/resource.c > index 0bcebffc4e77..38b36c212a48 100644 > --- a/kernel/resource.c > +++ b/kernel/resource.c > @@ -1040,6 +1040,8 @@ resource_size_t resource_alignment(struct resource *res) > > static DECLARE_WAIT_QUEUE_HEAD(muxed_resource_wait); > > +const char iomem_unknown_resource_name[] = { "reserved-unknown" }; > + > /** > * __request_region - create a new busy resource region > * @parent: parent resource descriptor > @@ -1092,6 +1094,15 @@ struct resource * __request_region(struct resource *parent, > break; > } > write_unlock(&resource_lock); > + > + if (res && res->parent > + && res->parent->name == iomem_unknown_resource_name) { No, this is a complete HACK, since when do we hard code specific (GLOBAL) ARCHs strings in common code. Please look at linux/ioport.h see the richness of options for all kind of buses and systems. The flag system works perfectly and I just continue this here. And really DAN, you prefer a global string that's dead garbage in 99% of arches to a simple bit flag definition that costs nothing? I don't think so. > + add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK); NACK!! > + pr_warn("request unknown region [mem %#010llx-%#010llx] %s\n", > + res->start, res->end, > + res->name); > + } > + > return res; > } > EXPORT_SYMBOL(__request_region); > > I do not understand you guys. Really. Dan you are a Linux Kernel developer why do you want to go ask some committee an approval for each new type of device that you want to develop. Why not have a system where the BIOS just puts up a BAR and an ID, and the rest is up to the drivers you write in C in the Kernel? What is the motivation of the complication? I would really like to understand? Thanks Boaz
* Boaz Harrosh <boaz@plexistor.com> wrote: > On 02/24/2015 06:22 AM, Dan Williams wrote: > <> > >> By Popular demand An Extra WARNING message is printed if > >> an "UNKNOWN" is found. It will look like this: > >> e820: WARNING [mem 0x100000000-0x1ffffffff] is unknown type 12 > > > > I don't think we need to warn that an unknown range was > > published, just warn if it is consumed. > > I did not have it at first, Ingo asked for it. I don't > mind having it and I don't mind not. I'd say it is Ingo's > call. So at least initially I really would like system operators to be informed when the BIOS does something unexpected to the kernel, especially when it comes to memory mappings. Later on, if it turns out to be benign, we can remove the warning. Thanks, Ingo
On 02/24/2015 10:34 AM, Ingo Molnar wrote: > > * Boaz Harrosh <boaz@plexistor.com> wrote: > >> On 02/24/2015 06:22 AM, Dan Williams wrote: >> <> >>>> By Popular demand An Extra WARNING message is printed if >>>> an "UNKNOWN" is found. It will look like this: >>>> e820: WARNING [mem 0x100000000-0x1ffffffff] is unknown type 12 >>> >>> I don't think we need to warn that an unknown range was >>> published, just warn if it is consumed. >> >> I did not have it at first, Ingo asked for it. I don't >> mind having it and I don't mind not. I'd say it is Ingo's >> call. > > So at least initially I really would like system operators > to be informed when the BIOS does something unexpected to > the kernel, especially when it comes to memory mappings. > OK, yes makes sense. > Later on, if it turns out to be benign, we can remove the > warning. > OK So here it is, with this one patch we are back to business. If we want the warning on lock than we have the 2nd patch, Any version you like. And please, please lets also put in the 3A/3 patch so we can positively scan for specifically these chips in /proc/iomem instead of blindly try any "reserved-unknown" types. > Thanks, > > Ingo Thanks Boaz
On Mon, Feb 23, 2015 at 11:59 PM, Boaz Harrosh <boaz@plexistor.com> wrote: > No, this is a complete HACK, since when do we hard code specific (GLOBAL) > ARCHs strings in common code. Please look at linux/ioport.h see the richness > of options for all kind of buses and systems. The flag system works perfectly > and I just continue this here. > > And really DAN, you prefer a global string that's dead garbage in 99% of arches > to a simple bit flag definition that costs nothing? I don't think so. Glad we're moving ahead with the IORESOURCE_MEM_WARN solution rather than this or the 64-bit-limited IORESOURCE_WARN approach. > >> + add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK); > > NACK!! > I disagree. Ultimately what goes into kernel/resource.c is not up to me, but firmware/driver combinations that subvert standards should be flagged by the kernel. Stepping back from the original motivation, in the general case, an unknown memory type is indiscernible from a BIOS bug. TAINT_FIRMWARE_WORKAROUND is simply a notification that firmware needs to be updated, and I believe a driver attaching to unknown memory is such an event. It does not block a user from using that memory however he or she sees fit.
diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c index 1afa5518baa6..2e755a92d84f 100644 --- a/arch/x86/kernel/e820.c +++ b/arch/x86/kernel/e820.c @@ -134,11 +134,6 @@ static void __init __e820_add_region(struct e820map *e820x, u64 start, u64 size, return; } - if (unlikely(_is_unknown_type(type))) - pr_warn("e820: WARNING [mem %#010llx-%#010llx] is unknown type %d\n", - (unsigned long long) start, - (unsigned long long) (start + size - 1), type); - e820x->map[x].addr = start; e820x->map[x].size = size; e820x->map[x].type = type; @@ -938,7 +933,7 @@ static inline const char *e820_type_to_string(int e820_type) case E820_NVS: return "ACPI Non-volatile Storage"; case E820_UNUSABLE: return "Unusable memory"; case E820_RESERVED: return "reserved"; - default: return "reserved-unkown"; + default: return iomem_unknown_resource_name; } } diff --git a/include/linux/ioport.h b/include/linux/ioport.h index 2c5250222278..d857e79b4bf2 100644 --- a/include/linux/ioport.h +++ b/include/linux/ioport.h @@ -194,6 +194,9 @@ extern struct resource * __request_region(struct resource *, resource_size_t n, const char *name, int flags); +/* For uniquely tagging unknown memory so we can warn when it is consumed */ +extern const char iomem_unknown_resource_name[]; + /* Compatibility cruft */ #define release_region(start,n) __release_region(&ioport_resource, (start), (n)) #define check_mem_region(start,n) __check_region(&iomem_resource, (start), (n)) diff --git a/kernel/resource.c b/kernel/resource.c index 0bcebffc4e77..38b36c212a48 100644 --- a/kernel/resource.c +++ b/kernel/resource.c @@ -1040,6 +1040,8 @@ resource_size_t resource_alignment(struct resource *res) static DECLARE_WAIT_QUEUE_HEAD(muxed_resource_wait); +const char iomem_unknown_resource_name[] = { "reserved-unknown" }; + /** * __request_region - create a new busy resource region * @parent: parent resource descriptor @@ -1092,6 +1094,15 @@ struct resource * __request_region(struct resource *parent, break; } write_unlock(&resource_lock); + + if (res && res->parent + && res->parent->name == iomem_unknown_resource_name) { + add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK); + pr_warn("request unknown region [mem %#010llx-%#010llx] %s\n", + res->start, res->end, + res->name); + } + return res; } EXPORT_SYMBOL(__request_region);