LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 0/3 v2] e820: Fix handling of NvDIMM chips
@ 2015-02-23 12:29 Boaz Harrosh
  2015-02-23 12:33 ` [PATCH 1/3] e820: Don't let unknown DIMM type come out BUSY Boaz Harrosh
                   ` (4 more replies)
  0 siblings, 5 replies; 30+ messages in thread
From: Boaz Harrosh @ 2015-02-23 12:29 UTC (permalink / raw)
  To: Ingo Molnar, Ross Zwisler, x86, linux-kernel, Roger C. Pao,
	Dan Williams, Thomas Gleixner, Linus Torvalds, linux-nvdimm,
	H. Peter Anvin, Matthew Wilcox, Andy Lutomirski,
	Christoph Hellwig

Hi

[v2]
* Added warning at bring up about unknown type
* Added an extra patch to warn-print in request_resource
* changed name from NvDIMM-12 => unknown-12
  I wish we would reconsider this. So we need to suffer until some unknown
  future when ACPI decides to reuse type-12. When this happens we can fix
  it then, NO?
* Now based on 4.0-rc1

If someone still has objections to [PATCH 3A/3] which is the most simple
thing to do, then We will be forced to do [PATCH 3B/3] ugliness. Ingo
your call.

[v1]
There is a deficiency in current e820.c handling where unknown new memory-chip
types come up as a BUSY resource when some other driver (like pmem) tries to
call request_mem_region_exclusive() on that resource. Even though, actually
there is nothing using it.
>From inspecting the code and the history of e820.c it looks like a BUG.

In any way this is a problem for the new type-12 NvDIMM memory chips that
are circulating around. (It is estimated that there are already 100ds of
thousands NvDIMM chips in active use)

The patches below first fixes the above problem for any future type
memory, so external drivers can access these mem chips.

I then also add the NvDIMM type-12 memory constant so it comes up
nice in dprints and at /proc/iomem

Just as before all these chips are very much usable with the pmem
driver. This lets us remove the hack for type-12 NvDIMMs that ignores
the return code from request_mem_region_exclusive() in pmem.c.

For all the pmem people. I maintain a tree with these patches
and latest pmem code here:
	git://git.open-osd.org/pmem.git
	[web-view:http://git.open-osd.org/gitweb.cgi?p=pmem.git;a=summary]

List of patches:
 [PATCH 1/3] e820: Don't let unknown DIMM type come out BUSY
	The main fix

 [PATCH 2/3] resource: Add new flag IORESOURCE_WARN (64bit)
	Warn in request_resource

 [PATCH 3A/3] e820: Add the unknown-12 Memory type (DDR3-NvDIMM)
	Please accept this simple patch

 [PATCH 3B/3] e820: dynamic unknown-xxx names (for DDR3-NvDIMM)
	Else we need this crap

Thanks
Boaz


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 1/3] e820: Don't let unknown DIMM type come out BUSY
  2015-02-23 12:29 [PATCH 0/3 v2] e820: Fix handling of NvDIMM chips Boaz Harrosh
@ 2015-02-23 12:33 ` Boaz Harrosh
  2015-02-24  4:22   ` Dan Williams
  2015-02-23 12:43 ` [PATCH 2/3] resource: Add new flag IORESOURCE_WARN (64bit) Boaz Harrosh
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 30+ messages in thread
From: Boaz Harrosh @ 2015-02-23 12:33 UTC (permalink / raw)
  To: Ingo Molnar, Ross Zwisler, x86, linux-kernel, Roger C. Pao,
	Dan Williams, Thomas Gleixner, Linus Torvalds, linux-nvdimm,
	H. Peter Anvin, Matthew Wilcox, Andy Lutomirski,
	Christoph Hellwig


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

An example of such "UNKNOWN" type is the not Standard type-12
DDR3-NvDIMM which is used by multiple vendors for a while
now. (Estimated 100ds of thousands sold world wide)

CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: x86@kernel.org
CC: Dan Williams <dan.j.williams@intel.com>
CC: Matthew Wilcox <willy@linux.intel.com>
CC: Christoph Hellwig <hch@infradead.org>
CC: Andy Lutomirski <luto@amacapital.net>
Signed-off-by: Boaz Harrosh <boaz@plexistor.com>
---
 arch/x86/kernel/e820.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 46201de..1a8a1c3 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -104,6 +104,21 @@ int __init e820_all_mapped(u64 start, u64 end, unsigned type)
 	return 0;
 }
 
+static bool _is_unknown_type(int e820_type)
+{
+	switch (e820_type) {
+	case E820_RESERVED_KERN:
+	case E820_RAM:
+	case E820_ACPI:
+	case E820_NVS:
+	case E820_UNUSABLE:
+	case E820_RESERVED:
+		return false;
+	default:
+		return true;
+	}
+}
+
 /*
  * Add a memory region to the kernel e820 map.
  */
@@ -119,6 +134,11 @@ 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;
@@ -907,10 +927,16 @@ static inline const char *e820_type_to_string(int e820_type)
 	case E820_ACPI:	return "ACPI Tables";
 	case E820_NVS:	return "ACPI Non-volatile Storage";
 	case E820_UNUSABLE:	return "Unusable memory";
-	default:	return "reserved";
+	case E820_RESERVED:	return "reserved";
+	default:	return "reserved-unkown";
 	}
 }
 
+static bool _is_reserved_type(int e820_type)
+{
+	return (e820_type == E820_RESERVED) || _is_unknown_type(e820_type);
+}
+
 /*
  * Mark e820 reserved areas as busy for the resource manager.
  */
@@ -940,7 +966,8 @@ void __init e820_reserve_resources(void)
 		 * pci device BAR resource and insert them later in
 		 * pcibios_resource_survey()
 		 */
-		if (e820.map[i].type != E820_RESERVED || res->start < (1ULL<<20)) {
+		if (!_is_reserved_type(e820.map[i].type) ||
+		    res->start < (1ULL<<20)) {
 			res->flags |= IORESOURCE_BUSY;
 			insert_resource(&iomem_resource, res);
 		}
-- 
1.9.3



^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 2/3] resource: Add new flag IORESOURCE_WARN (64bit)
  2015-02-23 12:29 [PATCH 0/3 v2] e820: Fix handling of NvDIMM chips Boaz Harrosh
  2015-02-23 12:33 ` [PATCH 1/3] e820: Don't let unknown DIMM type come out BUSY Boaz Harrosh
@ 2015-02-23 12:43 ` Boaz Harrosh
  2015-02-23 15:46   ` Andy Lutomirski
  2015-02-23 12:46 ` [PATCH 3A/3 good] e820: Add the unknown-12 Memory type (DDR3-NvDIMM) Boaz Harrosh
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 30+ messages in thread
From: Boaz Harrosh @ 2015-02-23 12:43 UTC (permalink / raw)
  To: Ingo Molnar, Ross Zwisler, x86, linux-kernel, Roger C. Pao,
	Dan Williams, Thomas Gleixner, Linus Torvalds, linux-nvdimm,
	H. Peter Anvin, Matthew Wilcox, Andy Lutomirski,
	Christoph Hellwig, Andrew Morton, Vivek Goyal


Resource providers set this flag if they want
that request_region_XXX will print a warning in dmesg
if this particular resource is locked by a driver.

Thous acting as a Protocol Police about experimental
devices that did not pass a comity approval.

The warn print looks like this:
  [Feb22 19:59] resource: request unknown region [mem 0x100000000-0x1ffffffff] unkown-12
Where the unkown-12 is taken from the res->name

The Only user of  this flag is x86/kernel/e820.c that
wants to WARN about UNKNOWN memory types.

NOTE: This patch looks very simple, a bit flag
  communicates between a resource provider ie e820.c
  that a warning should be printed, and resource.c
  prints such a message, when the resource is locked
  for use.

  BUT the basic flaw here is that we have run out of flags
  for a 32-bit long. My route here is to create wrappers
  that at 32-bit do nothing.

  Since only current user is e820.c for DDR3-NvDIMMs, this
  should be fine, because DDR3-NvDIMMs are not very useful
  with 32-bit ARCHES. In fact the smallest chip I know is
  4G, and NvDIMM is not currently supported as highmem (nor
  are there any plans to support it)

  OR: Maybe there is an extra bit that can be used at:
      /* PnP memory I/O specific bits */
        @@ -92,6 +90,7 @@ struct resource {
	 #define IORESOURCE_MEM_32BIT		(3<<3)
	 #define IORESOURCE_MEM_SHADOWABLE	(1<<5)	/* dup: IORESOURCE_SHADOWABLE */
	 #define IORESOURCE_MEM_EXPANSIONROM	(1<<6)
	+#define IORESOURCE_MEM_WARN		(1<<7)	/* WARN if used by drivers */

	 /* PnP I/O specific bits (IORESOURCE_BITS) */
	 #define IORESOURCE_IO_16BIT_ADDR	(1<<0)

    And get rid of the wrappers, Please advise ?

CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: x86@kernel.org
CC: Dan Williams <dan.j.williams@intel.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Bjorn Helgaas <bhelgaas@google.com>
CC: Vivek Goyal <vgoyal@redhat.com>

Signed-off-by: Boaz Harrosh <boaz@plexistor.com>
---
 arch/x86/kernel/e820.c |  3 +++
 include/linux/ioport.h | 23 +++++++++++++++++++++++
 kernel/resource.c      |  7 ++++++-
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 1a8a1c3..18a9850 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -961,6 +961,9 @@ void __init e820_reserve_resources(void)
 
 		res->flags = IORESOURCE_MEM;
 
+		if (_is_unknown_type(e820.map[i].type))
+			resource_set_warn_on_use(res, true);
+
 		/*
 		 * don't register the region that could be conflicted with
 		 * pci device BAR resource and insert them later in
diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 2c525022..9a51738 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -54,6 +54,8 @@ struct resource {
 #define IORESOURCE_UNSET	0x20000000	/* No address assigned yet */
 #define IORESOURCE_AUTO		0x40000000
 #define IORESOURCE_BUSY		0x80000000	/* Driver has marked this resource busy */
+/* Flags only on 64bit */
+#define IORESOURCE_WARN		0x100000000	/* Use with wrapper Only */
 
 /* PnP IRQ specific bits (IORESOURCE_BITS) */
 #define IORESOURCE_IRQ_HIGHEDGE		(1<<0)
@@ -255,6 +257,27 @@ static inline bool resource_overlaps(struct resource *r1, struct resource *r2)
        return (r1->start <= r2->end && r1->end >= r2->start);
 }
 
+static inline void resource_set_warn_on_use(struct resource *r, bool warn)
+{
+	if (sizeof(r->flags) > sizeof(u32)) {
+		if (warn)
+			r->flags |= IORESOURCE_WARN;
+		else
+			r->flags &= ~IORESOURCE_WARN;
+	}
+	/* I'm not doing any prints here for the else case because I do not want
+	 * to add the extra chain of includes this needs. This is a pretty low
+	 *  level Header
+	 */
+}
+
+static inline bool resource_warn_on_use(struct resource *r)
+{
+	if (sizeof(r->flags) > sizeof(u32))
+		return (r->flags & IORESOURCE_WARN) != 0;
+	else
+		return false;
+}
 
 #endif /* __ASSEMBLY__ */
 #endif	/* _LINUX_IOPORT_H */
diff --git a/kernel/resource.c b/kernel/resource.c
index 19f2357..eca6920 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1075,8 +1075,13 @@ struct resource * __request_region(struct resource *parent,
 			break;
 		if (conflict != parent) {
 			parent = conflict;
-			if (!(conflict->flags & IORESOURCE_BUSY))
+			if (!(conflict->flags & IORESOURCE_BUSY)) {
+				if (resource_warn_on_use(conflict))
+					pr_warn("request unknown region [mem %#010llx-%#010llx] %s\n",
+						conflict->start, conflict->end,
+						conflict->name);
 				continue;
+			}
 		}
 		if (conflict->flags & flags & IORESOURCE_MUXED) {
 			add_wait_queue(&muxed_resource_wait, &wait);
-- 
1.9.3



^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 3A/3 good] e820: Add the unknown-12 Memory type (DDR3-NvDIMM)
  2015-02-23 12:29 [PATCH 0/3 v2] e820: Fix handling of NvDIMM chips Boaz Harrosh
  2015-02-23 12:33 ` [PATCH 1/3] e820: Don't let unknown DIMM type come out BUSY Boaz Harrosh
  2015-02-23 12:43 ` [PATCH 2/3] resource: Add new flag IORESOURCE_WARN (64bit) Boaz Harrosh
@ 2015-02-23 12:46 ` Boaz Harrosh
  2015-02-23 15:48   ` Andy Lutomirski
  2015-02-23 12:48 ` [PATCH 3B/3 fat] e820: dynamic unknown-xxx names (for DDR3-NvDIMM) Boaz Harrosh
  2015-02-25 10:22 ` [PATCH 0/3 v2] e820: Fix handling of NvDIMM chips Ingo Molnar
  4 siblings, 1 reply; 30+ messages in thread
From: Boaz Harrosh @ 2015-02-23 12:46 UTC (permalink / raw)
  To: Ingo Molnar, Ross Zwisler, x86, linux-kernel, Roger C. Pao,
	Dan Williams, Thomas Gleixner, Linus Torvalds, linux-nvdimm,
	H. Peter Anvin, Matthew Wilcox, Andy Lutomirski,
	Christoph Hellwig


There are multiple vendors of DDR3 NvDIMMs out in the market today.
At various stages of development/production. It is estimated that
there are already more the 100ds of thousands chips sold to
testers and sites.

All the BIOS vendors I know of, tagged these chips at e820 table
as type-12 memory.

Now the ACPI comity, as far as I know, did not yet define a
standard type for NvDIMM. Also, as far as I know any NvDIMM
standard will only be defined for DDR4. So DDR3 NvDIMM is
probably stuck with this none STD type.

I Wish and call the ACPI comity to Define that NvDIMM is type-12.
Also for DDR4

In this patch I name type-12 "unknown-12". This is because of
ACPI politics that refuse to reserve type-12 as DDR3-NvDIMM
and members keep saying:
	"What if ACPI assigns type-12 for something else in future"

[And I say: Then just don't. Please?]

Signed-off-by: Boaz Harrosh <boaz@plexistor.com>
---
 arch/x86/kernel/e820.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 18a9850..023dd29 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -25,6 +25,11 @@
 #include <asm/proto.h>
 #include <asm/setup.h>
 
+/* These are nonstandard Memory types that we do not want in the exported
+ * header.
+ */
+#define E820_UNKNOWN_12 12	/* This is the unofficial DDR3-NVDIMM */
+
 /*
  * The e820 map is the map that gets modified e.g. with command line parameters
  * and that is also registered with modifications in the kernel resource tree
@@ -169,6 +174,9 @@ static void __init e820_print_type(u32 type)
 	case E820_UNUSABLE:
 		printk(KERN_CONT "unusable");
 		break;
+	case E820_UNKNOWN_12:
+		printk(KERN_CONT "unknown-12");
+		break;
 	default:
 		printk(KERN_CONT "type %u", type);
 		break;
@@ -928,6 +936,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";
+	case E820_UNKNOWN_12:	return "unkown-12";
 	default:	return "reserved-unkown";
 	}
 }
-- 
1.9.3



^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 3B/3 fat] e820: dynamic unknown-xxx names (for DDR3-NvDIMM)
  2015-02-23 12:29 [PATCH 0/3 v2] e820: Fix handling of NvDIMM chips Boaz Harrosh
                   ` (2 preceding siblings ...)
  2015-02-23 12:46 ` [PATCH 3A/3 good] e820: Add the unknown-12 Memory type (DDR3-NvDIMM) Boaz Harrosh
@ 2015-02-23 12:48 ` Boaz Harrosh
  2015-02-23 15:49   ` Andy Lutomirski
  2015-02-25 10:22 ` [PATCH 0/3 v2] e820: Fix handling of NvDIMM chips Ingo Molnar
  4 siblings, 1 reply; 30+ messages in thread
From: Boaz Harrosh @ 2015-02-23 12:48 UTC (permalink / raw)
  To: Ingo Molnar, Ross Zwisler, x86, linux-kernel, Roger C. Pao,
	Dan Williams, Thomas Gleixner, Linus Torvalds, linux-nvdimm,
	H. Peter Anvin, Matthew Wilcox, Andy Lutomirski,
	Christoph Hellwig


There are multiple vendors of DDR3 NvDIMMs out in the market today.
At various stages of development/production. It is estimated that
there are already more the 100ds of thousands chips sold to
testers and sites.

All the BIOS vendors I know of, tagged these chips at e820 table
as type-12 memory.

Now the ACPI comity, as far as I know, did not yet define a
standard type for NvDIMM. Also, as far as I know any NvDIMM
standard will only be defined for DDR4. So DDR3 NvDIMM is
probably stuck with this none STD type.

I Wish and call the ACPI comity to Define that NvDIMM is type-12.
Also for DDR4

In this patch I dynamically sprintf names into a static buffer
(max two unknown names) of the form "unknown-XXX" where XXX
is the type number. This is so we can return static string to
caller.

If there are too many types or type is bigger than 999 we
name the unknown region as "unknown-xxx"

I hope this patch is not accepted and that the simple patch
that just names above as "unknown-12" is. KISS right?

Signed-off-by: Boaz Harrosh <boaz@plexistor.com>
---
 arch/x86/include/uapi/asm/e820.h |  1 -
 arch/x86/kernel/e820.c           | 29 ++++++++++++++++++++++++++++-
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/uapi/asm/e820.h b/arch/x86/include/uapi/asm/e820.h
index d993e33..1f11303 100644
--- a/arch/x86/include/uapi/asm/e820.h
+++ b/arch/x86/include/uapi/asm/e820.h
@@ -33,7 +33,6 @@
 #define E820_NVS	4
 #define E820_UNUSABLE	5
 
-
 /*
  * reserved RAM used by kernel itself
  * if CONFIG_INTEL_TXT is enabled, memory of this type will be
diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 18a9850..3e06bab 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -919,6 +919,33 @@ void __init finish_e820_parsing(void)
 	}
 }
 
+static const char *_unknown_name(uint e820_type)
+{
+	enum {
+		MAX_UNKNOWN_TYPES = 1,
+		UNKNOWN_NAME_SIZE =  16, /* unknown-xxx\0 */
+	};
+	static struct __unknown_name__ {
+		int type;
+		char name[UNKNOWN_NAME_SIZE];
+	} names[MAX_UNKNOWN_TYPES];
+	static int num_names;
+
+	int i;
+
+	for (i = 0; i < num_names; ++i)
+		if (e820_type == names[i].type)
+			return names[i].name;
+
+	if ((num_names == MAX_UNKNOWN_TYPES) || (e820_type > 999))
+		return "unknown-xxx";
+
+	snprintf(names[++num_names].name, UNKNOWN_NAME_SIZE,
+		 "unknown-%03d", e820_type);
+	names[num_names].type = e820_type;
+	return names[num_names].name;
+}
+
 static inline const char *e820_type_to_string(int e820_type)
 {
 	switch (e820_type) {
@@ -928,7 +955,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 _unknown_name(e820_type);
 	}
 }
 
-- 
1.9.3



^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/3] resource: Add new flag IORESOURCE_WARN (64bit)
  2015-02-23 12:43 ` [PATCH 2/3] resource: Add new flag IORESOURCE_WARN (64bit) Boaz Harrosh
@ 2015-02-23 15:46   ` Andy Lutomirski
  2015-02-24  7:20     ` Boaz Harrosh
  2015-02-24  8:39     ` [PATCH 2/3 v3] resource: Add new flag IORESOURCE_MEM_WARN Boaz Harrosh
  0 siblings, 2 replies; 30+ messages in thread
From: Andy Lutomirski @ 2015-02-23 15:46 UTC (permalink / raw)
  To: Boaz Harrosh
  Cc: Ingo Molnar, Ross Zwisler, X86 ML, linux-kernel, Roger C. Pao,
	Dan Williams, Thomas Gleixner, Linus Torvalds, linux-nvdimm,
	H. Peter Anvin, Matthew Wilcox, Christoph Hellwig, Andrew Morton,
	Vivek Goyal

On Mon, Feb 23, 2015 at 4:43 AM, Boaz Harrosh <boaz@plexistor.com> wrote:
>
> Resource providers set this flag if they want
> that request_region_XXX will print a warning in dmesg
> if this particular resource is locked by a driver.
>
> Thous acting as a Protocol Police about experimental
> devices that did not pass a comity approval.
>
> The warn print looks like this:
>   [Feb22 19:59] resource: request unknown region [mem 0x100000000-0x1ffffffff] unkown-12
> Where the unkown-12 is taken from the res->name
>
> The Only user of  this flag is x86/kernel/e820.c that
> wants to WARN about UNKNOWN memory types.
>
> NOTE: This patch looks very simple, a bit flag
>   communicates between a resource provider ie e820.c
>   that a warning should be printed, and resource.c
>   prints such a message, when the resource is locked
>   for use.

I'm not really convinced this is necessary.  If you somehow manage to
reserve a physical address corresponding to an nvdimm, you probably
know what you're doing.  After all, no in-tree driver will do this by
default.

--Andy

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 3A/3 good] e820: Add the unknown-12 Memory type (DDR3-NvDIMM)
  2015-02-23 12:46 ` [PATCH 3A/3 good] e820: Add the unknown-12 Memory type (DDR3-NvDIMM) Boaz Harrosh
@ 2015-02-23 15:48   ` Andy Lutomirski
  0 siblings, 0 replies; 30+ messages in thread
From: Andy Lutomirski @ 2015-02-23 15:48 UTC (permalink / raw)
  To: Boaz Harrosh
  Cc: Ingo Molnar, Ross Zwisler, X86 ML, linux-kernel, Roger C. Pao,
	Dan Williams, Thomas Gleixner, Linus Torvalds, linux-nvdimm,
	H. Peter Anvin, Matthew Wilcox, Christoph Hellwig

On Mon, Feb 23, 2015 at 4:46 AM, Boaz Harrosh <boaz@plexistor.com> wrote:
>
> There are multiple vendors of DDR3 NvDIMMs out in the market today.
> At various stages of development/production. It is estimated that
> there are already more the 100ds of thousands chips sold to
> testers and sites.
>
> All the BIOS vendors I know of, tagged these chips at e820 table
> as type-12 memory.
>

I have no problem with this patch.

> Now the ACPI comity, as far as I know, did not yet define a
> standard type for NvDIMM. Also, as far as I know any NvDIMM
> standard will only be defined for DDR4. So DDR3 NvDIMM is
> probably stuck with this none STD type.

s/comity/committee?

>
> I Wish and call the ACPI comity to Define that NvDIMM is type-12.
> Also for DDR4
>
> In this patch I name type-12 "unknown-12". This is because of
> ACPI politics that refuse to reserve type-12 as DDR3-NvDIMM
> and members keep saying:
>         "What if ACPI assigns type-12 for something else in future"
>
> [And I say: Then just don't. Please?]

Good luck :)

--Andy

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 3B/3 fat] e820: dynamic unknown-xxx names (for DDR3-NvDIMM)
  2015-02-23 12:48 ` [PATCH 3B/3 fat] e820: dynamic unknown-xxx names (for DDR3-NvDIMM) Boaz Harrosh
@ 2015-02-23 15:49   ` Andy Lutomirski
  2015-02-24  7:38     ` Boaz Harrosh
  0 siblings, 1 reply; 30+ messages in thread
From: Andy Lutomirski @ 2015-02-23 15:49 UTC (permalink / raw)
  To: Boaz Harrosh
  Cc: Ingo Molnar, Ross Zwisler, X86 ML, linux-kernel, Roger C. Pao,
	Dan Williams, Thomas Gleixner, Linus Torvalds, linux-nvdimm,
	H. Peter Anvin, Matthew Wilcox, Christoph Hellwig

On Mon, Feb 23, 2015 at 4:48 AM, Boaz Harrosh <boaz@plexistor.com> wrote:
>
> There are multiple vendors of DDR3 NvDIMMs out in the market today.
> At various stages of development/production. It is estimated that
> there are already more the 100ds of thousands chips sold to
> testers and sites.
>
> All the BIOS vendors I know of, tagged these chips at e820 table
> as type-12 memory.
>
> Now the ACPI comity, as far as I know, did not yet define a
> standard type for NvDIMM. Also, as far as I know any NvDIMM
> standard will only be defined for DDR4. So DDR3 NvDIMM is
> probably stuck with this none STD type.
>
> I Wish and call the ACPI comity to Define that NvDIMM is type-12.
> Also for DDR4
>
> In this patch I dynamically sprintf names into a static buffer
> (max two unknown names) of the form "unknown-XXX" where XXX
> is the type number. This is so we can return static string to
> caller.

I prefer the other variant.

For Pete's sake, people, defining new e820 types is ludicrous.  It's
already sort of happened for nvdimms (and I really hope that type 12
is on its way out), and if it every happens again, we can deal with it
them.

--Andy

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/3] e820: Don't let unknown DIMM type come out BUSY
  2015-02-23 12:33 ` [PATCH 1/3] e820: Don't let unknown DIMM type come out BUSY Boaz Harrosh
@ 2015-02-24  4:22   ` Dan Williams
  2015-02-24  7:59     ` Boaz Harrosh
  0 siblings, 1 reply; 30+ messages in thread
From: Dan Williams @ 2015-02-24  4:22 UTC (permalink / raw)
  To: Boaz Harrosh
  Cc: Ingo Molnar, Ross Zwisler, x86, linux-kernel, Roger C. Pao,
	Thomas Gleixner, Linus Torvalds, linux-nvdimm, H. Peter Anvin,
	Matthew Wilcox, Andy Lutomirski, Christoph Hellwig

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.

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



^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/3] resource: Add new flag IORESOURCE_WARN (64bit)
  2015-02-23 15:46   ` Andy Lutomirski
@ 2015-02-24  7:20     ` Boaz Harrosh
  2015-02-24 19:58       ` Andy Lutomirski
  2015-02-24  8:39     ` [PATCH 2/3 v3] resource: Add new flag IORESOURCE_MEM_WARN Boaz Harrosh
  1 sibling, 1 reply; 30+ messages in thread
From: Boaz Harrosh @ 2015-02-24  7:20 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Ingo Molnar, Ross Zwisler, X86 ML, linux-kernel, Roger C. Pao,
	Dan Williams, Thomas Gleixner, Linus Torvalds, linux-nvdimm,
	H. Peter Anvin, Matthew Wilcox, Christoph Hellwig, Andrew Morton,
	Vivek Goyal

On 02/23/2015 05:46 PM, Andy Lutomirski wrote:
> On Mon, Feb 23, 2015 at 4:43 AM, Boaz Harrosh <boaz@plexistor.com> wrote:
>>
>> Resource providers set this flag if they want
>> that request_region_XXX will print a warning in dmesg
>> if this particular resource is locked by a driver.
>>
>> Thous acting as a Protocol Police about experimental
>> devices that did not pass a comity approval.
>>
>> The warn print looks like this:
>>   [Feb22 19:59] resource: request unknown region [mem 0x100000000-0x1ffffffff] unkown-12
>> Where the unkown-12 is taken from the res->name
>>
>> The Only user of  this flag is x86/kernel/e820.c that
>> wants to WARN about UNKNOWN memory types.
>>
>> NOTE: This patch looks very simple, a bit flag
>>   communicates between a resource provider ie e820.c
>>   that a warning should be printed, and resource.c
>>   prints such a message, when the resource is locked
>>   for use.
> 
> I'm not really convinced this is necessary.  If you somehow manage to
> reserve a physical address corresponding to an nvdimm, you probably
> know what you're doing.  

I think so too but, Ingo asked for it and I understand how it can be
useful

> After all, no in-tree driver will do this by
> default.

What? why? I intend to send pmem upstream soon. For god's sake
These devices are out there, lots of them, used everyday, since
when do we keep people systems out-of-tree?
And why because some comity did not anticipate that the DDR bus
will be used for "something else". With PCI or any other bus
I develop a new card, give it an ID, scan for it and use it.
DDR no, I need to re-write my BIOS, god. I wish it was Linux
that was scanning the DDR bus, who gives a *ck about BIOS.
I thought it was you who pushed for Linux's scan of the DDR bus?

DDR bus will be used for lots more then flat NvDIMM, we will see
soon, and already see, lots of Devices off of the DDR bus which as
nothing to do with memory. The BIOS and e820 better be put aside,
we need a simple scan and ID for these devices and let upper drivers
take care of them. What do you want to happen, that each new device
needs to go through this ordeal all over again?

> 
> --Andy
> 

Free life
Boaz


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 3B/3 fat] e820: dynamic unknown-xxx names (for DDR3-NvDIMM)
  2015-02-23 15:49   ` Andy Lutomirski
@ 2015-02-24  7:38     ` Boaz Harrosh
  0 siblings, 0 replies; 30+ messages in thread
From: Boaz Harrosh @ 2015-02-24  7:38 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Ingo Molnar, Ross Zwisler, X86 ML, linux-kernel, Roger C. Pao,
	Dan Williams, Thomas Gleixner, Linus Torvalds, linux-nvdimm,
	H. Peter Anvin, Matthew Wilcox, Christoph Hellwig

On 02/23/2015 05:49 PM, Andy Lutomirski wrote:
> On Mon, Feb 23, 2015 at 4:48 AM, Boaz Harrosh <boaz@plexistor.com> wrote:
>>
>> There are multiple vendors of DDR3 NvDIMMs out in the market today.
>> At various stages of development/production. It is estimated that
>> there are already more the 100ds of thousands chips sold to
>> testers and sites.
>>
>> All the BIOS vendors I know of, tagged these chips at e820 table
>> as type-12 memory.
>>
>> Now the ACPI comity, as far as I know, did not yet define a
>> standard type for NvDIMM. Also, as far as I know any NvDIMM
>> standard will only be defined for DDR4. So DDR3 NvDIMM is
>> probably stuck with this none STD type.
>>
>> I Wish and call the ACPI comity to Define that NvDIMM is type-12.
>> Also for DDR4
>>
>> In this patch I dynamically sprintf names into a static buffer
>> (max two unknown names) of the form "unknown-XXX" where XXX
>> is the type number. This is so we can return static string to
>> caller.
> 
> I prefer the other variant.
> 

Me too

> For Pete's sake, people, defining new e820 types is ludicrous.  It's
> already sort of happened for nvdimms (and I really hope that type 12
> is on its way out), and if it every happens again, we can deal with it
> them.
> 

No, you are wrong sir. What we need to do is define an open system.

e820 is just a table that communicates between the firmware and
the Kernel of the results of the DDR bus scan. Now I bet the same
DDR buses are scanned much differently on other ARCHs. It is only
the forsaken x86 that caries the BIOS baggage,  (for economical reasons
BTW, not technical)
Fine I'm not going to fight that fight, but the BIOS should Just ID
the devices say VENDOR/DEVICE like the PCI does. Or string ID like
USB, or a UUID, and the BAR it is using and get out of the way for real
drivers.

I have seen DDR cards with processors on them, and all systems, and
weird combinations of flashes and RAMs, and batteries, you name it.
It should be easy for new devices to be added, without a single committee
(God how many double letters in this word)

> --Andy
> 

Cheers
Boaz


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/3] e820: Don't let unknown DIMM type come out BUSY
  2015-02-24  4:22   ` Dan Williams
@ 2015-02-24  7:59     ` Boaz Harrosh
  2015-02-24  8:34       ` Ingo Molnar
  2015-02-26  2:09       ` Dan Williams
  0 siblings, 2 replies; 30+ messages in thread
From: Boaz Harrosh @ 2015-02-24  7:59 UTC (permalink / raw)
  To: Dan Williams
  Cc: Ingo Molnar, Ross Zwisler, x86, linux-kernel, Roger C. Pao,
	Thomas Gleixner, Linus Torvalds, linux-nvdimm, H. Peter Anvin,
	Matthew Wilcox, Andy Lutomirski, Christoph Hellwig

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/3] e820: Don't let unknown DIMM type come out BUSY
  2015-02-24  7:59     ` Boaz Harrosh
@ 2015-02-24  8:34       ` Ingo Molnar
  2015-02-24  8:51         ` Boaz Harrosh
  2015-02-26  2:09       ` Dan Williams
  1 sibling, 1 reply; 30+ messages in thread
From: Ingo Molnar @ 2015-02-24  8:34 UTC (permalink / raw)
  To: Boaz Harrosh
  Cc: Dan Williams, Ingo Molnar, Ross Zwisler, x86, linux-kernel,
	Roger C. Pao, Thomas Gleixner, Linus Torvalds, linux-nvdimm,
	H. Peter Anvin, Matthew Wilcox, Andy Lutomirski,
	Christoph Hellwig


* 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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 2/3 v3] resource: Add new flag IORESOURCE_MEM_WARN
  2015-02-23 15:46   ` Andy Lutomirski
  2015-02-24  7:20     ` Boaz Harrosh
@ 2015-02-24  8:39     ` Boaz Harrosh
  2015-02-24  8:44       ` Boaz Harrosh
                         ` (2 more replies)
  1 sibling, 3 replies; 30+ messages in thread
From: Boaz Harrosh @ 2015-02-24  8:39 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Andy Lutomirski, Ross Zwisler, X86 ML, linux-kernel,
	Roger C. Pao, Dan Williams, Thomas Gleixner, Linus Torvalds,
	linux-nvdimm, H. Peter Anvin, Matthew Wilcox, Christoph Hellwig,
	Andrew Morton, Vivek Goyal


Resource providers set this flag if they want
that request_region_XXX will print a warning in dmesg
if this particular resource is locked by a driver.

Thous acting as a Protocol Police about experimental
devices that did not pass a comity approval.

The Only user of  this flag is x86/kernel/e820.c that
wants to WARN about UNKNOWN memory types.

Signed-off-by: Boaz Harrosh <boaz@plexistor.com>
---
 arch/x86/kernel/e820.c | 3 +++
 include/linux/ioport.h | 2 +-
 kernel/resource.c      | 7 ++++++-
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 1a8a1c3..105bb37 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -961,6 +961,9 @@ void __init e820_reserve_resources(void)
 
 		res->flags = IORESOURCE_MEM;
 
+		if (_is_unknown_type(e820.map[i].type))
+			res->flags |= IORESOURCE_MEM_WARN;
+
 		/*
 		 * don't register the region that could be conflicted with
 		 * pci device BAR resource and insert them later in
diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 2c525022..183af93 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -90,6 +90,7 @@ struct resource {
 #define IORESOURCE_MEM_32BIT		(3<<3)
 #define IORESOURCE_MEM_SHADOWABLE	(1<<5)	/* dup: IORESOURCE_SHADOWABLE */
 #define IORESOURCE_MEM_EXPANSIONROM	(1<<6)
+#define IORESOURCE_MEM_WARN		(1<<7)	/* WARN if requested by driver */
 
 /* PnP I/O specific bits (IORESOURCE_BITS) */
 #define IORESOURCE_IO_16BIT_ADDR	(1<<0)
@@ -255,6 +256,5 @@ static inline bool resource_overlaps(struct resource *r1, struct resource *r2)
        return (r1->start <= r2->end && r1->end >= r2->start);
 }
 
-
 #endif /* __ASSEMBLY__ */
 #endif	/* _LINUX_IOPORT_H */
diff --git a/kernel/resource.c b/kernel/resource.c
index 19f2357..fca23ff 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1075,8 +1075,13 @@ struct resource * __request_region(struct resource *parent,
 			break;
 		if (conflict != parent) {
 			parent = conflict;
-			if (!(conflict->flags & IORESOURCE_BUSY))
+			if (!(conflict->flags & IORESOURCE_BUSY)) {
+				if (conflict->flags & IORESOURCE_MEM_WARN)
+					pr_warn("request unknown region [mem %#010llx-%#010llx] %s\n",
+						conflict->start, conflict->end,
+						conflict->name);
 				continue;
+			}
 		}
 		if (conflict->flags & flags & IORESOURCE_MUXED) {
 			add_wait_queue(&muxed_resource_wait, &wait);
-- 
1.9.3



^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/3 v3] resource: Add new flag IORESOURCE_MEM_WARN
  2015-02-24  8:39     ` [PATCH 2/3 v3] resource: Add new flag IORESOURCE_MEM_WARN Boaz Harrosh
@ 2015-02-24  8:44       ` Boaz Harrosh
  2015-02-24  9:06       ` Ingo Molnar
  2015-02-24  9:07       ` Ingo Molnar
  2 siblings, 0 replies; 30+ messages in thread
From: Boaz Harrosh @ 2015-02-24  8:44 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Andy Lutomirski, Ross Zwisler, X86 ML, linux-kernel,
	Roger C. Pao, Dan Williams, Thomas Gleixner, Linus Torvalds,
	linux-nvdimm, H. Peter Anvin, Matthew Wilcox, Christoph Hellwig,
	Andrew Morton, Vivek Goyal

On 02/24/2015 10:39 AM, Boaz Harrosh wrote:
> 
> Resource providers set this flag if they want
> that request_region_XXX will print a warning in dmesg
> if this particular resource is locked by a driver.
> 
> Thous acting as a Protocol Police about experimental
> devices that did not pass a comity approval.
> 
> The Only user of  this flag is x86/kernel/e820.c that
> wants to WARN about UNKNOWN memory types.
> 

Hi Ingo

So I slept on it and I think this simple version is safe and
does what we need. It is more simple than the wrappers thing

Who can push such a patch, can you push it through your tree or
is there another maintainer that needs to push this?

Who can we ask about the safeness of these flags?

> Signed-off-by: Boaz Harrosh <boaz@plexistor.com>
<>

Thanks
Boaz


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/3] e820: Don't let unknown DIMM type come out BUSY
  2015-02-24  8:34       ` Ingo Molnar
@ 2015-02-24  8:51         ` Boaz Harrosh
  0 siblings, 0 replies; 30+ messages in thread
From: Boaz Harrosh @ 2015-02-24  8:51 UTC (permalink / raw)
  To: Ingo Molnar, Boaz Harrosh
  Cc: Dan Williams, Ingo Molnar, Ross Zwisler, x86, linux-kernel,
	Roger C. Pao, Thomas Gleixner, Linus Torvalds, linux-nvdimm,
	H. Peter Anvin, Matthew Wilcox, Andy Lutomirski,
	Christoph Hellwig

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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/3 v3] resource: Add new flag IORESOURCE_MEM_WARN
  2015-02-24  8:39     ` [PATCH 2/3 v3] resource: Add new flag IORESOURCE_MEM_WARN Boaz Harrosh
  2015-02-24  8:44       ` Boaz Harrosh
@ 2015-02-24  9:06       ` Ingo Molnar
  2015-02-24  9:08         ` Boaz Harrosh
  2015-02-24  9:07       ` Ingo Molnar
  2 siblings, 1 reply; 30+ messages in thread
From: Ingo Molnar @ 2015-02-24  9:06 UTC (permalink / raw)
  To: Boaz Harrosh
  Cc: Ingo Molnar, Andy Lutomirski, Ross Zwisler, X86 ML, linux-kernel,
	Roger C. Pao, Dan Williams, Thomas Gleixner, Linus Torvalds,
	linux-nvdimm, H. Peter Anvin, Matthew Wilcox, Christoph Hellwig,
	Andrew Morton, Vivek Goyal


* Boaz Harrosh <boaz@plexistor.com> wrote:

> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -255,6 +256,5 @@ static inline bool resource_overlaps(struct resource *r1, struct resource *r2)
>         return (r1->start <= r2->end && r1->end >= r2->start);
>  }
>  
> -
>  #endif /* __ASSEMBLY__ */

While the newline is spurious, it should probably not be 
removed in this patch.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/3 v3] resource: Add new flag IORESOURCE_MEM_WARN
  2015-02-24  8:39     ` [PATCH 2/3 v3] resource: Add new flag IORESOURCE_MEM_WARN Boaz Harrosh
  2015-02-24  8:44       ` Boaz Harrosh
  2015-02-24  9:06       ` Ingo Molnar
@ 2015-02-24  9:07       ` Ingo Molnar
  2015-02-24  9:09         ` Boaz Harrosh
  2015-02-24 15:00         ` [PATCH 2/3 v4] " Boaz Harrosh
  2 siblings, 2 replies; 30+ messages in thread
From: Ingo Molnar @ 2015-02-24  9:07 UTC (permalink / raw)
  To: Boaz Harrosh
  Cc: Ingo Molnar, Andy Lutomirski, Ross Zwisler, X86 ML, linux-kernel,
	Roger C. Pao, Dan Williams, Thomas Gleixner, Linus Torvalds,
	linux-nvdimm, H. Peter Anvin, Matthew Wilcox, Christoph Hellwig,
	Andrew Morton, Vivek Goyal


* Boaz Harrosh <boaz@plexistor.com> wrote:

> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -1075,8 +1075,13 @@ struct resource * __request_region(struct resource *parent,
>  			break;
>  		if (conflict != parent) {
>  			parent = conflict;
> -			if (!(conflict->flags & IORESOURCE_BUSY))
> +			if (!(conflict->flags & IORESOURCE_BUSY)) {
> +				if (conflict->flags & IORESOURCE_MEM_WARN)
> +					pr_warn("request unknown region [mem %#010llx-%#010llx] %s\n",
> +						conflict->start, conflict->end,
> +						conflict->name);

Maybe:

   'request region with unknown memory type [mem ...]'?

The driver (we hope!) very well knows about the region so 
it's not totally unknown.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/3 v3] resource: Add new flag IORESOURCE_MEM_WARN
  2015-02-24  9:06       ` Ingo Molnar
@ 2015-02-24  9:08         ` Boaz Harrosh
  0 siblings, 0 replies; 30+ messages in thread
From: Boaz Harrosh @ 2015-02-24  9:08 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Ingo Molnar, Andy Lutomirski, Ross Zwisler, X86 ML, linux-kernel,
	Roger C. Pao, Dan Williams, Thomas Gleixner, Linus Torvalds,
	linux-nvdimm, H. Peter Anvin, Matthew Wilcox, Christoph Hellwig,
	Andrew Morton, Vivek Goyal

On 02/24/2015 11:06 AM, Ingo Molnar wrote:
> 
> * Boaz Harrosh <boaz@plexistor.com> wrote:
> 
>> --- a/include/linux/ioport.h
>> +++ b/include/linux/ioport.h
>> @@ -255,6 +256,5 @@ static inline bool resource_overlaps(struct resource *r1, struct resource *r2)
>>         return (r1->start <= r2->end && r1->end >= r2->start);
>>  }
>>  
>> -
>>  #endif /* __ASSEMBLY__ */
> 
> While the newline is spurious, it should probably not be 
> removed in this patch.
> 

Rrrr you are right. thanks. I did not notice that I did this.

I will resend.

Boaz


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/3 v3] resource: Add new flag IORESOURCE_MEM_WARN
  2015-02-24  9:07       ` Ingo Molnar
@ 2015-02-24  9:09         ` Boaz Harrosh
  2015-02-24 15:00         ` [PATCH 2/3 v4] " Boaz Harrosh
  1 sibling, 0 replies; 30+ messages in thread
From: Boaz Harrosh @ 2015-02-24  9:09 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Ingo Molnar, Andy Lutomirski, Ross Zwisler, X86 ML, linux-kernel,
	Roger C. Pao, Dan Williams, Thomas Gleixner, Linus Torvalds,
	linux-nvdimm, H. Peter Anvin, Matthew Wilcox, Christoph Hellwig,
	Andrew Morton, Vivek Goyal

On 02/24/2015 11:07 AM, Ingo Molnar wrote:
> 
> * Boaz Harrosh <boaz@plexistor.com> wrote:
> 
>> --- a/kernel/resource.c
>> +++ b/kernel/resource.c
>> @@ -1075,8 +1075,13 @@ struct resource * __request_region(struct resource *parent,
>>  			break;
>>  		if (conflict != parent) {
>>  			parent = conflict;
>> -			if (!(conflict->flags & IORESOURCE_BUSY))
>> +			if (!(conflict->flags & IORESOURCE_BUSY)) {
>> +				if (conflict->flags & IORESOURCE_MEM_WARN)
>> +					pr_warn("request unknown region [mem %#010llx-%#010llx] %s\n",
>> +						conflict->start, conflict->end,
>> +						conflict->name);
> 
> Maybe:
> 
>    'request region with unknown memory type [mem ...]'?
> 
> The driver (we hope!) very well knows about the region so 
> it's not totally unknown.
> 

OK makes sense I will fix that too

> Thanks,
> 
> 	Ingo
> 


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 2/3 v4] resource: Add new flag IORESOURCE_MEM_WARN
  2015-02-24  9:07       ` Ingo Molnar
  2015-02-24  9:09         ` Boaz Harrosh
@ 2015-02-24 15:00         ` Boaz Harrosh
  2015-02-24 17:04           ` Dan Williams
  1 sibling, 1 reply; 30+ messages in thread
From: Boaz Harrosh @ 2015-02-24 15:00 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Ingo Molnar, Andy Lutomirski, Ross Zwisler, X86 ML, linux-kernel,
	Roger C. Pao, Dan Williams, Thomas Gleixner, Linus Torvalds,
	linux-nvdimm, H. Peter Anvin, Matthew Wilcox, Christoph Hellwig,
	Andrew Morton, Vivek Goyal


Resource providers set this flag if they want
that request_region will print a warning in dmesg
if this particular memory resource is locked by a driver.

Thous acting as a Protocol Police about experimental
devices that did not pass a committee approval.

The Only user of  this flag is x86/kernel/e820.c that
wants to WARN about UNKNOWN memory types.

NOTE: It would be preferred if I defined a general flag say
      IORESOURCE_WARN, where any kind of resource provider
      can WARN on use, but we have run out of flags in the
      32bit long systems. So I defined a free bit from the
      resource specific flags for mem resources. This is
      why I need to check if this is a memory resource first
      so not to conflict with other resource specific flags.
      (Though actually no one is using this specific bit)

CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: x86@kernel.org
CC: Dan Williams <dan.j.williams@intel.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Bjorn Helgaas <bhelgaas@google.com>
CC: Vivek Goyal <vgoyal@redhat.com>
Signed-off-by: Boaz Harrosh <boaz@plexistor.com>
---
 arch/x86/kernel/e820.c | 3 +++
 include/linux/ioport.h | 1 +
 kernel/resource.c      | 9 ++++++++-
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 1a8a1c3..105bb37 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -961,6 +961,9 @@ void __init e820_reserve_resources(void)
 
 		res->flags = IORESOURCE_MEM;
 
+		if (_is_unknown_type(e820.map[i].type))
+			res->flags |= IORESOURCE_MEM_WARN;
+
 		/*
 		 * don't register the region that could be conflicted with
 		 * pci device BAR resource and insert them later in
diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 2c525022..f78972b 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -90,6 +90,7 @@ struct resource {
 #define IORESOURCE_MEM_32BIT		(3<<3)
 #define IORESOURCE_MEM_SHADOWABLE	(1<<5)	/* dup: IORESOURCE_SHADOWABLE */
 #define IORESOURCE_MEM_EXPANSIONROM	(1<<6)
+#define IORESOURCE_MEM_WARN		(1<<7)	/* WARN if requested by driver */
 
 /* PnP I/O specific bits (IORESOURCE_BITS) */
 #define IORESOURCE_IO_16BIT_ADDR	(1<<0)
diff --git a/kernel/resource.c b/kernel/resource.c
index 19f2357..4bab16f 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1075,8 +1075,15 @@ struct resource * __request_region(struct resource *parent,
 			break;
 		if (conflict != parent) {
 			parent = conflict;
-			if (!(conflict->flags & IORESOURCE_BUSY))
+			if (!(conflict->flags & IORESOURCE_BUSY)) {
+				if (unlikely(
+				    (resource_type(conflict) == IORESOURCE_MEM)
+				    && (conflict->flags & IORESOURCE_MEM_WARN)))
+					pr_warn("request region with unknown memory type [mem %#010llx-%#010llx] %s\n",
+						conflict->start, conflict->end,
+						conflict->name);
 				continue;
+			}
 		}
 		if (conflict->flags & flags & IORESOURCE_MUXED) {
 			add_wait_queue(&muxed_resource_wait, &wait);
-- 
1.9.3


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/3 v4] resource: Add new flag IORESOURCE_MEM_WARN
  2015-02-24 15:00         ` [PATCH 2/3 v4] " Boaz Harrosh
@ 2015-02-24 17:04           ` Dan Williams
  2015-02-25  6:36             ` Boaz Harrosh
  0 siblings, 1 reply; 30+ messages in thread
From: Dan Williams @ 2015-02-24 17:04 UTC (permalink / raw)
  To: Boaz Harrosh
  Cc: Ingo Molnar, Ingo Molnar, Andy Lutomirski, Ross Zwisler, X86 ML,
	linux-kernel, Roger C. Pao, Thomas Gleixner, Linus Torvalds,
	linux-nvdimm, H. Peter Anvin, Matthew Wilcox, Christoph Hellwig,
	Andrew Morton, Vivek Goyal

On Tue, Feb 24, 2015 at 7:00 AM, Boaz Harrosh <boaz@plexistor.com> wrote:
>
> Resource providers set this flag if they want
> that request_region will print a warning in dmesg
> if this particular memory resource is locked by a driver.
>
> Thous acting as a Protocol Police about experimental
> devices that did not pass a committee approval.
>
> The Only user of  this flag is x86/kernel/e820.c that
> wants to WARN about UNKNOWN memory types.
>
> NOTE: It would be preferred if I defined a general flag say
>       IORESOURCE_WARN, where any kind of resource provider
>       can WARN on use, but we have run out of flags in the
>       32bit long systems. So I defined a free bit from the
>       resource specific flags for mem resources. This is
>       why I need to check if this is a memory resource first
>       so not to conflict with other resource specific flags.
>       (Though actually no one is using this specific bit)
>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: "H. Peter Anvin" <hpa@zytor.com>
> CC: x86@kernel.org
> CC: Dan Williams <dan.j.williams@intel.com>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: Bjorn Helgaas <bhelgaas@google.com>
> CC: Vivek Goyal <vgoyal@redhat.com>
> Signed-off-by: Boaz Harrosh <boaz@plexistor.com>
> ---
>  arch/x86/kernel/e820.c | 3 +++
>  include/linux/ioport.h | 1 +
>  kernel/resource.c      | 9 ++++++++-
>  3 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index 1a8a1c3..105bb37 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -961,6 +961,9 @@ void __init e820_reserve_resources(void)
>
>                 res->flags = IORESOURCE_MEM;
>
> +               if (_is_unknown_type(e820.map[i].type))
> +                       res->flags |= IORESOURCE_MEM_WARN;
> +
>                 /*
>                  * don't register the region that could be conflicted with
>                  * pci device BAR resource and insert them later in
> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> index 2c525022..f78972b 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -90,6 +90,7 @@ struct resource {
>  #define IORESOURCE_MEM_32BIT           (3<<3)
>  #define IORESOURCE_MEM_SHADOWABLE      (1<<5)  /* dup: IORESOURCE_SHADOWABLE */
>  #define IORESOURCE_MEM_EXPANSIONROM    (1<<6)
> +#define IORESOURCE_MEM_WARN            (1<<7)  /* WARN if requested by driver */
>
>  /* PnP I/O specific bits (IORESOURCE_BITS) */
>  #define IORESOURCE_IO_16BIT_ADDR       (1<<0)
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 19f2357..4bab16f 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -1075,8 +1075,15 @@ struct resource * __request_region(struct resource *parent,
>                         break;
>                 if (conflict != parent) {
>                         parent = conflict;
> -                       if (!(conflict->flags & IORESOURCE_BUSY))
> +                       if (!(conflict->flags & IORESOURCE_BUSY)) {
> +                               if (unlikely(

No need for unlikely(), this isn't a hot path.

> +                                   (resource_type(conflict) == IORESOURCE_MEM)
> +                                   && (conflict->flags & IORESOURCE_MEM_WARN)))
> +                                       pr_warn("request region with unknown memory type [mem %#010llx-%#010llx] %s\n",
> +                                               conflict->start, conflict->end,
> +                                               conflict->name);

I think this should also dump the res->name to identify who is requesting it.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/3] resource: Add new flag IORESOURCE_WARN (64bit)
  2015-02-24  7:20     ` Boaz Harrosh
@ 2015-02-24 19:58       ` Andy Lutomirski
  0 siblings, 0 replies; 30+ messages in thread
From: Andy Lutomirski @ 2015-02-24 19:58 UTC (permalink / raw)
  To: Boaz Harrosh
  Cc: Ingo Molnar, Ross Zwisler, X86 ML, linux-kernel, Roger C. Pao,
	Dan Williams, Thomas Gleixner, Linus Torvalds, linux-nvdimm,
	H. Peter Anvin, Matthew Wilcox, Christoph Hellwig, Andrew Morton,
	Vivek Goyal

On Mon, Feb 23, 2015 at 11:20 PM, Boaz Harrosh <boaz@plexistor.com> wrote:
> On 02/23/2015 05:46 PM, Andy Lutomirski wrote:
>> On Mon, Feb 23, 2015 at 4:43 AM, Boaz Harrosh <boaz@plexistor.com> wrote:
>>>
>>> Resource providers set this flag if they want
>>> that request_region_XXX will print a warning in dmesg
>>> if this particular resource is locked by a driver.
>>>
>>> Thous acting as a Protocol Police about experimental
>>> devices that did not pass a comity approval.
>>>
>>> The warn print looks like this:
>>>   [Feb22 19:59] resource: request unknown region [mem 0x100000000-0x1ffffffff] unkown-12
>>> Where the unkown-12 is taken from the res->name
>>>
>>> The Only user of  this flag is x86/kernel/e820.c that
>>> wants to WARN about UNKNOWN memory types.
>>>
>>> NOTE: This patch looks very simple, a bit flag
>>>   communicates between a resource provider ie e820.c
>>>   that a warning should be printed, and resource.c
>>>   prints such a message, when the resource is locked
>>>   for use.
>>
>> I'm not really convinced this is necessary.  If you somehow manage to
>> reserve a physical address corresponding to an nvdimm, you probably
>> know what you're doing.
>
> I think so too but, Ingo asked for it and I understand how it can be
> useful
>
>> After all, no in-tree driver will do this by
>> default.
>
> What? why? I intend to send pmem upstream soon. For god's sake
> These devices are out there, lots of them, used everyday, since
> when do we keep people systems out-of-tree?
> And why because some comity did not anticipate that the DDR bus
> will be used for "something else". With PCI or any other bus
> I develop a new card, give it an ID, scan for it and use it.
> DDR no, I need to re-write my BIOS, god. I wish it was Linux
> that was scanning the DDR bus, who gives a *ck about BIOS.
> I thought it was you who pushed for Linux's scan of the DDR bus?

This was a scan of the i2c part of the bus, which is kind of
enumerable.  If Linux can reliably enumerate nvdimms, then great!  But
the mere existence of a type 12 e820 region does not indicate the
presence of a working nvdimm (I know this because I have the
super-secret docs for one of these things).  It's also a problem on
EFI, because there's no such thing as type 12.

>
> DDR bus will be used for lots more then flat NvDIMM, we will see
> soon, and already see, lots of Devices off of the DDR bus which as
> nothing to do with memory. The BIOS and e820 better be put aside,
> we need a simple scan and ID for these devices and let upper drivers
> take care of them. What do you want to happen, that each new device
> needs to go through this ordeal all over again?

The BIOS *has* to be involved due to the way that memory controllers work.

In order to access the data part of the bus, the memory controller
needs to be programmed to map all the memory to the right physical
addresses, set up timings, etc.  Until that happens, there is no RAM.
That means that gnarly code runs out of cache (crazy dance, documented
(!) on SNB and earlier and sort of documented on Haswell, dunno about
IVB) very very early in boot.  Booting Linux before that happens would
be impossible.  Afterwards, the memory initialization code has done
its thing, and it had better have mapped the nvdimms somewhere useful,
and not, say, interleaved with other memory, if Linux is supposed to
use them.

So we need BIOS to tell us where they are and what they are.  For that
to work, we need a data channel to BIOS that's guaranteed to exist,
and e820 is not a useful answer.

AFAIK the ACPI people are working on it, and I'm sure that there's
some politics involved, but it'll happen.  In the mean time, if you
can write a driver that works reliably and doesn't cause damage,
great!

--Andy

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/3 v4] resource: Add new flag IORESOURCE_MEM_WARN
  2015-02-24 17:04           ` Dan Williams
@ 2015-02-25  6:36             ` Boaz Harrosh
  0 siblings, 0 replies; 30+ messages in thread
From: Boaz Harrosh @ 2015-02-25  6:36 UTC (permalink / raw)
  To: Dan Williams
  Cc: Ingo Molnar, Ingo Molnar, Andy Lutomirski, Ross Zwisler, X86 ML,
	linux-kernel, Roger C. Pao, Thomas Gleixner, Linus Torvalds,
	linux-nvdimm, H. Peter Anvin, Matthew Wilcox, Christoph Hellwig,
	Andrew Morton, Vivek Goyal

On 02/24/2015 07:04 PM, Dan Williams wrote:
> On Tue, Feb 24, 2015 at 7:00 AM, Boaz Harrosh <boaz@plexistor.com> wrote:
>>
>> Resource providers set this flag if they want
>> that request_region will print a warning in dmesg
>> if this particular memory resource is locked by a driver.
>>
>> Thous acting as a Protocol Police about experimental
>> devices that did not pass a committee approval.
>>
>> The Only user of  this flag is x86/kernel/e820.c that
>> wants to WARN about UNKNOWN memory types.
>>
>> NOTE: It would be preferred if I defined a general flag say
>>       IORESOURCE_WARN, where any kind of resource provider
>>       can WARN on use, but we have run out of flags in the
>>       32bit long systems. So I defined a free bit from the
>>       resource specific flags for mem resources. This is
>>       why I need to check if this is a memory resource first
>>       so not to conflict with other resource specific flags.
>>       (Though actually no one is using this specific bit)
>>
>> CC: Thomas Gleixner <tglx@linutronix.de>
>> CC: Ingo Molnar <mingo@redhat.com>
>> CC: "H. Peter Anvin" <hpa@zytor.com>
>> CC: x86@kernel.org
>> CC: Dan Williams <dan.j.williams@intel.com>
>> CC: Andrew Morton <akpm@linux-foundation.org>
>> CC: Bjorn Helgaas <bhelgaas@google.com>
>> CC: Vivek Goyal <vgoyal@redhat.com>
>> Signed-off-by: Boaz Harrosh <boaz@plexistor.com>
>> ---
>>  arch/x86/kernel/e820.c | 3 +++
>>  include/linux/ioport.h | 1 +
>>  kernel/resource.c      | 9 ++++++++-
>>  3 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
>> index 1a8a1c3..105bb37 100644
>> --- a/arch/x86/kernel/e820.c
>> +++ b/arch/x86/kernel/e820.c
>> @@ -961,6 +961,9 @@ void __init e820_reserve_resources(void)
>>
>>                 res->flags = IORESOURCE_MEM;
>>
>> +               if (_is_unknown_type(e820.map[i].type))
>> +                       res->flags |= IORESOURCE_MEM_WARN;
>> +
>>                 /*
>>                  * don't register the region that could be conflicted with
>>                  * pci device BAR resource and insert them later in
>> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
>> index 2c525022..f78972b 100644
>> --- a/include/linux/ioport.h
>> +++ b/include/linux/ioport.h
>> @@ -90,6 +90,7 @@ struct resource {
>>  #define IORESOURCE_MEM_32BIT           (3<<3)
>>  #define IORESOURCE_MEM_SHADOWABLE      (1<<5)  /* dup: IORESOURCE_SHADOWABLE */
>>  #define IORESOURCE_MEM_EXPANSIONROM    (1<<6)
>> +#define IORESOURCE_MEM_WARN            (1<<7)  /* WARN if requested by driver */
>>
>>  /* PnP I/O specific bits (IORESOURCE_BITS) */
>>  #define IORESOURCE_IO_16BIT_ADDR       (1<<0)
>> diff --git a/kernel/resource.c b/kernel/resource.c
>> index 19f2357..4bab16f 100644
>> --- a/kernel/resource.c
>> +++ b/kernel/resource.c
>> @@ -1075,8 +1075,15 @@ struct resource * __request_region(struct resource *parent,
>>                         break;
>>                 if (conflict != parent) {
>>                         parent = conflict;
>> -                       if (!(conflict->flags & IORESOURCE_BUSY))
>> +                       if (!(conflict->flags & IORESOURCE_BUSY)) {
>> +                               if (unlikely(
> 
> No need for unlikely(), this isn't a hot path.
> 
>> +                                   (resource_type(conflict) == IORESOURCE_MEM)
>> +                                   && (conflict->flags & IORESOURCE_MEM_WARN)))
>> +                                       pr_warn("request region with unknown memory type [mem %#010llx-%#010llx] %s\n",
>> +                                               conflict->start, conflict->end,
>> +                                               conflict->name);
> 
> I think this should also dump the res->name to identify who is requesting it.
> 

OK Will send a version 4

Thanks
Boaz
 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 0/3 v2] e820: Fix handling of NvDIMM chips
  2015-02-23 12:29 [PATCH 0/3 v2] e820: Fix handling of NvDIMM chips Boaz Harrosh
                   ` (3 preceding siblings ...)
  2015-02-23 12:48 ` [PATCH 3B/3 fat] e820: dynamic unknown-xxx names (for DDR3-NvDIMM) Boaz Harrosh
@ 2015-02-25 10:22 ` Ingo Molnar
  2015-02-25 14:42   ` Boaz Harrosh
  4 siblings, 1 reply; 30+ messages in thread
From: Ingo Molnar @ 2015-02-25 10:22 UTC (permalink / raw)
  To: Boaz Harrosh
  Cc: Ingo Molnar, Ross Zwisler, x86, linux-kernel, Roger C. Pao,
	Dan Williams, Thomas Gleixner, Linus Torvalds, linux-nvdimm,
	H. Peter Anvin, Matthew Wilcox, Andy Lutomirski,
	Christoph Hellwig


* Boaz Harrosh <boaz@plexistor.com> wrote:

> List of patches:
>  [PATCH 1/3] e820: Don't let unknown DIMM type come out BUSY
> 	The main fix
> 
>  [PATCH 2/3] resource: Add new flag IORESOURCE_WARN (64bit)
> 	Warn in request_resource
> 
>  [PATCH 3A/3] e820: Add the unknown-12 Memory type (DDR3-NvDIMM)
> 	Please accept this simple patch
> 
>  [PATCH 3B/3] e820: dynamic unknown-xxx names (for DDR3-NvDIMM)
> 	Else we need this crap

Please also include your nvdimm driver in your next 
submission (even if that is not part of the submission 
yet), so that we can see how the driver makes use of the 
new facility.

There's quite a bit of confusion about what means what, 
people are not of one opinion, and it's easier to see and 
check the fine code instead of making assumptions.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 0/3 v2] e820: Fix handling of NvDIMM chips
  2015-02-25 10:22 ` [PATCH 0/3 v2] e820: Fix handling of NvDIMM chips Ingo Molnar
@ 2015-02-25 14:42   ` Boaz Harrosh
  0 siblings, 0 replies; 30+ messages in thread
From: Boaz Harrosh @ 2015-02-25 14:42 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Ingo Molnar, Ross Zwisler, x86, linux-kernel, Roger C. Pao,
	Dan Williams, Thomas Gleixner, Linus Torvalds, linux-nvdimm,
	H. Peter Anvin, Matthew Wilcox, Andy Lutomirski,
	Christoph Hellwig

On 02/25/2015 12:22 PM, Ingo Molnar wrote:
> 
> * Boaz Harrosh <boaz@plexistor.com> wrote:
> 
>> List of patches:
>>  [PATCH 1/3] e820: Don't let unknown DIMM type come out BUSY
>> 	The main fix
>>
>>  [PATCH 2/3] resource: Add new flag IORESOURCE_WARN (64bit)
>> 	Warn in request_resource
>>
>>  [PATCH 3A/3] e820: Add the unknown-12 Memory type (DDR3-NvDIMM)
>> 	Please accept this simple patch
>>
>>  [PATCH 3B/3] e820: dynamic unknown-xxx names (for DDR3-NvDIMM)
>> 	Else we need this crap
> 
> Please also include your nvdimm driver in your next 
> submission (even if that is not part of the submission 
> yet), so that we can see how the driver makes use of the 
> new facility.
> 
> There's quite a bit of confusion about what means what, 
> people are not of one opinion, and it's easier to see and 
> check the fine code instead of making assumptions.
> 

Hi Ingo

Sure, I will send it as RFC as part of the next send.
Ingo, did we please decide on the [PATCH 3A/3]?

> Thanks,
> 
> 	Ingo
> 

Thanks very much
Boaz


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/3] e820: Don't let unknown DIMM type come out BUSY
  2015-02-24  7:59     ` Boaz Harrosh
  2015-02-24  8:34       ` Ingo Molnar
@ 2015-02-26  2:09       ` Dan Williams
  1 sibling, 0 replies; 30+ messages in thread
From: Dan Williams @ 2015-02-26  2:09 UTC (permalink / raw)
  To: Boaz Harrosh
  Cc: Ingo Molnar, Ross Zwisler, X86 ML, linux-kernel, Roger C. Pao,
	Thomas Gleixner, Linus Torvalds, linux-nvdimm, H. Peter Anvin,
	Matthew Wilcox, Andy Lutomirski, Christoph Hellwig

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.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/3] e820: Don't let unknown DIMM type come out BUSY
  2015-03-05 20:41   ` Dan Williams
@ 2015-03-09 10:54     ` Boaz Harrosh
  0 siblings, 0 replies; 30+ messages in thread
From: Boaz Harrosh @ 2015-03-09 10:54 UTC (permalink / raw)
  To: Dan Williams
  Cc: Ingo Molnar, X86 ML, linux-kernel, Roger C. Pao, Thomas Gleixner,
	linux-nvdimm, H. Peter Anvin, Matthew Wilcox, Andy Lutomirski,
	Christoph Hellwig, Ross Zwisler

On 03/05/2015 10:41 PM, Dan Williams wrote:
> On Thu, Mar 5, 2015 at 2:20 AM, Boaz Harrosh <boaz@plexistor.com> wrote:
>>
>> There is something not very nice (Gentlemen nice) In current
>> e820.c code.
>>
>> At Multiple places for example @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
>>
>> An example of such "UNKNOWN" type is the not Standard type-12
>> DDR3-NvDIMM which is used by multiple vendors for a while
>> now. (Estimated 100ds of thousands sold world wide)
>>
>> CC: Thomas Gleixner <tglx@linutronix.de>
>> CC: Ingo Molnar <mingo@redhat.com>
>> CC: "H. Peter Anvin" <hpa@zytor.com>
>> CC: x86@kernel.org
>> CC: Dan Williams <dan.j.williams@intel.com>
>> CC: Matthew Wilcox <willy@linux.intel.com>
>> CC: Christoph Hellwig <hch@infradead.org>
>> CC: Andy Lutomirski <luto@amacapital.net>
>> Signed-off-by: Boaz Harrosh <boaz@plexistor.com>
>> ---
>>  arch/x86/kernel/e820.c | 31 +++++++++++++++++++++++++++++--
>>  1 file changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
>> index 46201de..c3a11cd 100644
>> --- a/arch/x86/kernel/e820.c
>> +++ b/arch/x86/kernel/e820.c
>> @@ -104,6 +104,21 @@ int __init e820_all_mapped(u64 start, u64 end, unsigned type)
>>         return 0;
>>  }
>>
>> +static bool _is_unknown_type(int e820_type)
> 
> Any reason for the leading "_"?
> 

I have a style guide that says that any static function starts with a _ and
need not a global prefix like e820_ so to not conflict when compiled in-kernel.

But This is not the style in this file. So sorry, do I must send a new version
patch?

>> +{
>> +       switch (e820_type) {
>> +       case E820_RESERVED_KERN:
>> +       case E820_RAM:
>> +       case E820_ACPI:
>> +       case E820_NVS:
>> +       case E820_UNUSABLE:
>> +       case E820_RESERVED:
>> +               return false;
>> +       default:
>> +               return true;
>> +       }
>> +}
>> +
>>  /*
>>   * Add a memory region to the kernel e820 map.
>>   */
>> @@ -119,6 +134,11 @@ static void __init __e820_add_region(struct e820map *e820x, u64 start, u64 size,
>>                 return;
>>         }
>>
>> +       if (unlikely(_is_unknown_type(type)))
> 
> Unnecessary unlikely()?
> 

Its more for a programmer's communication then CPU optimization. Though also for
the CPU it is, "don't waste space at branch predictor", in anyway it can not hurt.

But I like it here for the reading code flow that communicates. "Even if this happens
%100 of the time the code flow should be not to take this error branch"

>> +               pr_warn("e820: WARNING [mem %#010llx-%#010llx] is unknown type %d\n",
>> +                      (unsigned long long) start,
>> +                      (unsigned long long) (start + size - 1), type);
> 
> I still think this warning can go and 

I did not have it at first, but Ingo felt it has merit, I trust his experience and
instincts. I kind of like it now in the lab logs, for machines that actually have
NvDIMMs in them.

> we can just fold patch 2 into this one, but other than that this looks ok to me.
> 

No! we cannot.

1. For one it is two different subsystems and maintainers. You want them separate
   So they might go in through different trees, also conflict in different way if
   code advances.

2. These are two different topics. This patch is about fixing a bug, The resource being
   busy was not intentional.
   The second patch is about a global WARNING for when some mem resource is used, which
   is independent of this here problem and is a new fixture.

Thanks
Boaz


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/3] e820: Don't let unknown DIMM type come out BUSY
  2015-03-05 10:20 ` [PATCH 1/3] e820: Don't let unknown DIMM type come out BUSY Boaz Harrosh
@ 2015-03-05 20:41   ` Dan Williams
  2015-03-09 10:54     ` Boaz Harrosh
  0 siblings, 1 reply; 30+ messages in thread
From: Dan Williams @ 2015-03-05 20:41 UTC (permalink / raw)
  To: Boaz Harrosh
  Cc: Ingo Molnar, X86 ML, linux-kernel, Roger C. Pao, Thomas Gleixner,
	linux-nvdimm, H. Peter Anvin, Matthew Wilcox, Andy Lutomirski,
	Christoph Hellwig, Ross Zwisler

On Thu, Mar 5, 2015 at 2:20 AM, Boaz Harrosh <boaz@plexistor.com> wrote:
>
> There is something not very nice (Gentlemen nice) In current
> e820.c code.
>
> At Multiple places for example @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
>
> An example of such "UNKNOWN" type is the not Standard type-12
> DDR3-NvDIMM which is used by multiple vendors for a while
> now. (Estimated 100ds of thousands sold world wide)
>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: "H. Peter Anvin" <hpa@zytor.com>
> CC: x86@kernel.org
> CC: Dan Williams <dan.j.williams@intel.com>
> CC: Matthew Wilcox <willy@linux.intel.com>
> CC: Christoph Hellwig <hch@infradead.org>
> CC: Andy Lutomirski <luto@amacapital.net>
> Signed-off-by: Boaz Harrosh <boaz@plexistor.com>
> ---
>  arch/x86/kernel/e820.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index 46201de..c3a11cd 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -104,6 +104,21 @@ int __init e820_all_mapped(u64 start, u64 end, unsigned type)
>         return 0;
>  }
>
> +static bool _is_unknown_type(int e820_type)

Any reason for the leading "_"?

> +{
> +       switch (e820_type) {
> +       case E820_RESERVED_KERN:
> +       case E820_RAM:
> +       case E820_ACPI:
> +       case E820_NVS:
> +       case E820_UNUSABLE:
> +       case E820_RESERVED:
> +               return false;
> +       default:
> +               return true;
> +       }
> +}
> +
>  /*
>   * Add a memory region to the kernel e820 map.
>   */
> @@ -119,6 +134,11 @@ static void __init __e820_add_region(struct e820map *e820x, u64 start, u64 size,
>                 return;
>         }
>
> +       if (unlikely(_is_unknown_type(type)))

Unnecessary unlikely()?

> +               pr_warn("e820: WARNING [mem %#010llx-%#010llx] is unknown type %d\n",
> +                      (unsigned long long) start,
> +                      (unsigned long long) (start + size - 1), type);

I still think this warning can go and we can just fold patch 2 into
this one, but other than that this looks ok to me.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 1/3] e820: Don't let unknown DIMM type come out BUSY
  2015-03-05 10:16 [PATCH 0/3 v5] " Boaz Harrosh
@ 2015-03-05 10:20 ` Boaz Harrosh
  2015-03-05 20:41   ` Dan Williams
  0 siblings, 1 reply; 30+ messages in thread
From: Boaz Harrosh @ 2015-03-05 10:20 UTC (permalink / raw)
  To: Ingo Molnar, x86, linux-kernel, Roger C. Pao, Dan Williams,
	Thomas Gleixner, linux-nvdimm, H. Peter Anvin, Matthew Wilcox,
	Andy Lutomirski, Christoph Hellwig
  Cc: Ross Zwisler


There is something not very nice (Gentlemen nice) In current
e820.c code.

At Multiple places for example @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

An example of such "UNKNOWN" type is the not Standard type-12
DDR3-NvDIMM which is used by multiple vendors for a while
now. (Estimated 100ds of thousands sold world wide)

CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: x86@kernel.org
CC: Dan Williams <dan.j.williams@intel.com>
CC: Matthew Wilcox <willy@linux.intel.com>
CC: Christoph Hellwig <hch@infradead.org>
CC: Andy Lutomirski <luto@amacapital.net>
Signed-off-by: Boaz Harrosh <boaz@plexistor.com>
---
 arch/x86/kernel/e820.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 46201de..c3a11cd 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -104,6 +104,21 @@ int __init e820_all_mapped(u64 start, u64 end, unsigned type)
 	return 0;
 }
 
+static bool _is_unknown_type(int e820_type)
+{
+	switch (e820_type) {
+	case E820_RESERVED_KERN:
+	case E820_RAM:
+	case E820_ACPI:
+	case E820_NVS:
+	case E820_UNUSABLE:
+	case E820_RESERVED:
+		return false;
+	default:
+		return true;
+	}
+}
+
 /*
  * Add a memory region to the kernel e820 map.
  */
@@ -119,6 +134,11 @@ 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;
@@ -907,10 +927,16 @@ static inline const char *e820_type_to_string(int e820_type)
 	case E820_ACPI:	return "ACPI Tables";
 	case E820_NVS:	return "ACPI Non-volatile Storage";
 	case E820_UNUSABLE:	return "Unusable memory";
-	default:	return "reserved";
+	case E820_RESERVED:	return "reserved";
+	default:		return "reserved-unknown";
 	}
 }
 
+static bool _is_reserved_type(int e820_type)
+{
+	return (e820_type == E820_RESERVED) || _is_unknown_type(e820_type);
+}
+
 /*
  * Mark e820 reserved areas as busy for the resource manager.
  */
@@ -940,7 +966,8 @@ void __init e820_reserve_resources(void)
 		 * pci device BAR resource and insert them later in
 		 * pcibios_resource_survey()
 		 */
-		if (e820.map[i].type != E820_RESERVED || res->start < (1ULL<<20)) {
+		if (!_is_reserved_type(e820.map[i].type) ||
+		    res->start < (1ULL<<20)) {
 			res->flags |= IORESOURCE_BUSY;
 			insert_resource(&iomem_resource, res);
 		}
-- 
1.9.3



^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2015-03-09 10:54 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-02-23 12:29 [PATCH 0/3 v2] e820: Fix handling of NvDIMM chips Boaz Harrosh
2015-02-23 12:33 ` [PATCH 1/3] e820: Don't let unknown DIMM type come out BUSY Boaz Harrosh
2015-02-24  4:22   ` Dan Williams
2015-02-24  7:59     ` Boaz Harrosh
2015-02-24  8:34       ` Ingo Molnar
2015-02-24  8:51         ` Boaz Harrosh
2015-02-26  2:09       ` Dan Williams
2015-02-23 12:43 ` [PATCH 2/3] resource: Add new flag IORESOURCE_WARN (64bit) Boaz Harrosh
2015-02-23 15:46   ` Andy Lutomirski
2015-02-24  7:20     ` Boaz Harrosh
2015-02-24 19:58       ` Andy Lutomirski
2015-02-24  8:39     ` [PATCH 2/3 v3] resource: Add new flag IORESOURCE_MEM_WARN Boaz Harrosh
2015-02-24  8:44       ` Boaz Harrosh
2015-02-24  9:06       ` Ingo Molnar
2015-02-24  9:08         ` Boaz Harrosh
2015-02-24  9:07       ` Ingo Molnar
2015-02-24  9:09         ` Boaz Harrosh
2015-02-24 15:00         ` [PATCH 2/3 v4] " Boaz Harrosh
2015-02-24 17:04           ` Dan Williams
2015-02-25  6:36             ` Boaz Harrosh
2015-02-23 12:46 ` [PATCH 3A/3 good] e820: Add the unknown-12 Memory type (DDR3-NvDIMM) Boaz Harrosh
2015-02-23 15:48   ` Andy Lutomirski
2015-02-23 12:48 ` [PATCH 3B/3 fat] e820: dynamic unknown-xxx names (for DDR3-NvDIMM) Boaz Harrosh
2015-02-23 15:49   ` Andy Lutomirski
2015-02-24  7:38     ` Boaz Harrosh
2015-02-25 10:22 ` [PATCH 0/3 v2] e820: Fix handling of NvDIMM chips Ingo Molnar
2015-02-25 14:42   ` Boaz Harrosh
2015-03-05 10:16 [PATCH 0/3 v5] " Boaz Harrosh
2015-03-05 10:20 ` [PATCH 1/3] e820: Don't let unknown DIMM type come out BUSY Boaz Harrosh
2015-03-05 20:41   ` Dan Williams
2015-03-09 10:54     ` Boaz Harrosh

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).