[1/3] e820: Don't let unknown DIMM type come out BUSY
diff mbox

Message ID 1424751767.9050.4.camel@intel.com
State New, archived
Headers show

Commit Message

Dan Williams Feb. 24, 2015, 4:22 a.m. UTC
On Mon, 2015-02-23 at 14:33 +0200, Boaz Harrosh wrote:
> There is something not very nice (Gentlemen nice) In current
> e820.c code.
> 
> At Multiple places for example like (@ memblock_x86_fill()) it will
> add the different memory resources *except the E820_RESERVED type*
> 
> Then at e820_reserve_resources() it will mark all !E820_RESERVED
> as busy.
> 
> This is all fine when we have only the known types one of:
> 	E820_RESERVED_KERN:
> 	E820_RAM:
> 	E820_ACPI:
> 	E820_NVS:
> 	E820_UNUSABLE:
> 	E820_RESERVED:
> 
> But if the system encounters a brand new memory type it will
> not add it to any memory list, But will proceed to mark it
> BUSY. So now any other Driver in the system that does know
> how to deal with this new type, is not able to call
> request_mem_region_exclusive() on this new type because it is
> hard coded BUSY even though nothing really uses it.
> 
> So make any unknown type behave like E820_RESERVED memory,
> it will show up as available to first caller of
> request_mem_region_exclusive().
> 
> I Also change the string representation of an unknown type
> from "reserved" (So to not confuse with memmap "reserved"
> region). And call it "reserved-unknown"
> I wish I could return "reserved-type-X" But this is not possible
> because one must return a constant, code-segment, string.
> 
> (NOTE: These unknown-types where called "reserved" in
>  /proc/iomem and in dmesg but behaved differently. What this
>  patch does is name them differently but let them behave
>  the same)
> 
> 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.

Something like these incremental changes.  I don't see the need for
patch 2 or either version of patch 3.

Comments

Boaz Harrosh Feb. 24, 2015, 7:59 a.m. UTC | #1
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
Ingo Molnar Feb. 24, 2015, 8:34 a.m. UTC | #2
* 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
Boaz Harrosh Feb. 24, 2015, 8:51 a.m. UTC | #3
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
Dan Williams Feb. 26, 2015, 2:09 a.m. UTC | #4
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.

Patch
diff mbox

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);