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
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 <[email protected]>
CC: Ingo Molnar <[email protected]>
CC: "H. Peter Anvin" <[email protected]>
CC: [email protected]
CC: Dan Williams <[email protected]>
CC: Matthew Wilcox <[email protected]>
CC: Christoph Hellwig <[email protected]>
CC: Andy Lutomirski <[email protected]>
Signed-off-by: Boaz Harrosh <[email protected]>
---
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
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 <[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 | 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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Mon, Feb 23, 2015 at 4:43 AM, Boaz Harrosh <[email protected]> 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
On Mon, Feb 23, 2015 at 4:46 AM, Boaz Harrosh <[email protected]> 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
On Mon, Feb 23, 2015 at 4:48 AM, Boaz Harrosh <[email protected]> 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
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);
On 02/23/2015 05:46 PM, Andy Lutomirski wrote:
> On Mon, Feb 23, 2015 at 4:43 AM, Boaz Harrosh <[email protected]> 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
On 02/23/2015 05:49 PM, Andy Lutomirski wrote:
> On Mon, Feb 23, 2015 at 4:48 AM, Boaz Harrosh <[email protected]> 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
On 02/24/2015 06:22 AM, Dan Williams wrote:
<>
>> By Popular demand An Extra WARNING message is printed if
>> an "UNKNOWN" is found. It will look like this:
>> e820: WARNING [mem 0x100000000-0x1ffffffff] is unknown type 12
>
> I don't think we need to warn that an unknown range was published, just
> warn if it is consumed.
>
I did not have it at first, Ingo asked for it. I don't mind having
it and I don't mind not. I'd say it is Ingo's call.
> Something like these incremental changes. I don't see the need for
> patch 2 or either version of patch 3.
>
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index 1afa5518baa6..2e755a92d84f 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -134,11 +134,6 @@ static void __init __e820_add_region(struct e820map *e820x, u64 start, u64 size,
> return;
> }
>
> - if (unlikely(_is_unknown_type(type)))
> - pr_warn("e820: WARNING [mem %#010llx-%#010llx] is unknown type %d\n",
> - (unsigned long long) start,
> - (unsigned long long) (start + size - 1), type);
> -
Again Ingo's call
> e820x->map[x].addr = start;
> e820x->map[x].size = size;
> e820x->map[x].type = type;
> @@ -938,7 +933,7 @@ static inline const char *e820_type_to_string(int e820_type)
> case E820_NVS: return "ACPI Non-volatile Storage";
> case E820_UNUSABLE: return "Unusable memory";
> case E820_RESERVED: return "reserved";
> - default: return "reserved-unkown";
> + default: return iomem_unknown_resource_name;
> }
> }
>
> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> index 2c5250222278..d857e79b4bf2 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -194,6 +194,9 @@ extern struct resource * __request_region(struct resource *,
> resource_size_t n,
> const char *name, int flags);
>
> +/* For uniquely tagging unknown memory so we can warn when it is consumed */
> +extern const char iomem_unknown_resource_name[];
> +
> /* Compatibility cruft */
> #define release_region(start,n) __release_region(&ioport_resource, (start), (n))
> #define check_mem_region(start,n) __check_region(&iomem_resource, (start), (n))
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 0bcebffc4e77..38b36c212a48 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -1040,6 +1040,8 @@ resource_size_t resource_alignment(struct resource *res)
>
> static DECLARE_WAIT_QUEUE_HEAD(muxed_resource_wait);
>
> +const char iomem_unknown_resource_name[] = { "reserved-unknown" };
> +
> /**
> * __request_region - create a new busy resource region
> * @parent: parent resource descriptor
> @@ -1092,6 +1094,15 @@ struct resource * __request_region(struct resource *parent,
> break;
> }
> write_unlock(&resource_lock);
> +
> + if (res && res->parent
> + && res->parent->name == iomem_unknown_resource_name) {
No, this is a complete HACK, since when do we hard code specific (GLOBAL)
ARCHs strings in common code. Please look at linux/ioport.h see the richness
of options for all kind of buses and systems. The flag system works perfectly
and I just continue this here.
And really DAN, you prefer a global string that's dead garbage in 99% of arches
to a simple bit flag definition that costs nothing? I don't think so.
> + add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
NACK!!
> + pr_warn("request unknown region [mem %#010llx-%#010llx] %s\n",
> + res->start, res->end,
> + res->name);
> + }
> +
> return res;
> }
> EXPORT_SYMBOL(__request_region);
>
>
I do not understand you guys. Really. Dan you are a Linux Kernel developer
why do you want to go ask some committee an approval for each new type of
device that you want to develop. Why not have a system where the BIOS just
puts up a BAR and an ID, and the rest is up to the drivers you write in C
in the Kernel? What is the motivation of the complication? I would really
like to understand?
Thanks
Boaz
* Boaz Harrosh <[email protected]> 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
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]>
---
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
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 <[email protected]>
<>
Thanks
Boaz
On 02/24/2015 10:34 AM, Ingo Molnar wrote:
>
> * Boaz Harrosh <[email protected]> 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
* Boaz Harrosh <[email protected]> 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
* Boaz Harrosh <[email protected]> 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
On 02/24/2015 11:06 AM, Ingo Molnar wrote:
>
> * Boaz Harrosh <[email protected]> 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
On 02/24/2015 11:07 AM, Ingo Molnar wrote:
>
> * Boaz Harrosh <[email protected]> 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
>
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(-)
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
On Tue, Feb 24, 2015 at 7:00 AM, Boaz Harrosh <[email protected]> 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 <[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(-)
>
> 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.
On Mon, Feb 23, 2015 at 11:20 PM, Boaz Harrosh <[email protected]> wrote:
> On 02/23/2015 05:46 PM, Andy Lutomirski wrote:
>> On Mon, Feb 23, 2015 at 4:43 AM, Boaz Harrosh <[email protected]> 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
On 02/24/2015 07:04 PM, Dan Williams wrote:
> On Tue, Feb 24, 2015 at 7:00 AM, Boaz Harrosh <[email protected]> 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 <[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(-)
>>
>> 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
* Boaz Harrosh <[email protected]> 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
On 02/25/2015 12:22 PM, Ingo Molnar wrote:
>
> * Boaz Harrosh <[email protected]> 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
On Mon, Feb 23, 2015 at 11:59 PM, Boaz Harrosh <[email protected]> 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.