e820: Fix handling of NvDIMM chips

From inspecting the code and the history of e820.c it looks like a BUG.

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.

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?]

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.

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

Quoted 12 lines · Boaz Harrosh

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

Quoted 8 lines

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

Quoted 17 lines · Boaz Harrosh

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

Quoted 4 lines

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

Quoted 3 lines

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.

arch/x86/kernel/e820.c +0 -0

index 1afa5518baa6..2e755a92d84f 100644

arch/x86/kernel/e820.c +1 -1

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

include/linux/ioport.h +3 -0

index 2c5250222278..d857e79b4bf2 100644
@@ -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))

kernel/resource.c +2 -0

index 0bcebffc4e77..38b36c212a48 100644
@@ -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

On 02/24/2015 06:22 AM, Dan Williams wrote:
<>

Quoted 4 lines

Quoted 3 lines

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

Quoted 2 lines

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.

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 <[email protected]>

Quoted 9 lines · Boaz Harrosh

On 02/24/2015 06:22 AM, Dan Williams wrote:
<>

Quoted 6 lines

Quoted 3 lines

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.

Quoted 3 lines

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

Quoted 6 lines · Boaz Harrosh

include/linux/ioport.h +0 -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);
 }
 

include/linux/ioport.h static inline bool resource_overlaps(struct resource *r1, struct resource *r2) · line 259

Quoted 2 lines

include/linux/ioport.h +0 -1

-
 #endif /* __ASSEMBLY__ */

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

kernel/resource.c struct resource * __request_region(struct resource *parent, · line 1082

Quoted 12 lines · Boaz Harrosh

kernel/resource.c +5 -1

@@ -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

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 <[email protected]>

CC Ingo Molnar <[email protected]>

CC "H. Peter Anvin" <[email protected]>

CC [email protected]

CC Dan Williams <[email protected]>

CC Andrew Morton <[email protected]>

CC Bjorn Helgaas <[email protected]>

CC Vivek Goyal <[email protected]>

Signed-off-by Boaz Harrosh <[email protected]>

---
 arch/x86/kernel/e820.c | 3 +++
 include/linux/ioport.h | 1 +
 kernel/resource.c      | 9 ++++++++-
 3 files changed, 12 insertions(+), 1 deletion(-)

arch/x86/kernel/e820.c +3 -0

index 1a8a1c3..105bb37 100644
@@ -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

include/linux/ioport.h +1 -0

index 2c525022..f78972b 100644
@@ -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)

kernel/resource.c +0 -0

index 19f2357..4bab16f 100644

Quoted 22 lines · Andy Lutomirski

Quoted 12 lines · Boaz Harrosh

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

Quoted 8 lines

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.

Quoted 3 lines

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

Quoted 2 lines

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?

Quoted 66 lines · Boaz Harrosh

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 <[email protected]>

CC Ingo Molnar <[email protected]>

CC "H. Peter Anvin" <[email protected]>

CC [email protected]

CC Dan Williams <[email protected]>

CC Andrew Morton <[email protected]>

CC Bjorn Helgaas <[email protected]>

CC Vivek Goyal <[email protected]>

Signed-off-by Boaz Harrosh <[email protected]>

---
 arch/x86/kernel/e820.c | 3 +++
 include/linux/ioport.h | 1 +
 kernel/resource.c      | 9 ++++++++-
 3 files changed, 12 insertions(+), 1 deletion(-)

arch/x86/kernel/e820.c +3 -0

index 1a8a1c3..105bb37 100644
@@ -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

include/linux/ioport.h +1 -0

index 2c525022..f78972b 100644
@@ -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)

kernel/resource.c +0 -0

index 19f2357..4bab16f 100644

kernel/resource.c struct resource * __request_region(struct resource *parent, · line 1079

Quoted 6 lines

kernel/resource.c +2 -1

@@ -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.

kernel/resource.c struct resource * __request_region(struct resource *parent, · line 1084

Quoted 5 lines

kernel/resource.c +5 -0

+                                   (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.

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

Quoted 4 lines · Boaz Harrosh

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

Quoted 8 lines

[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

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.

Quoted 2 lines

+ add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);

NACK!!