[2/3] resource: Add new flag IORESOURCE_WARN (64bit)
diff mbox

Message ID 54EB206E.4010009@plexistor.com
State New, archived
Headers show

Commit Message

Boaz Harrosh Feb. 23, 2015, 12:43 p.m. UTC
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(-)

Comments

Andy Lutomirski Feb. 23, 2015, 3:46 p.m. UTC | #1
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
Boaz Harrosh Feb. 24, 2015, 7:20 a.m. UTC | #2
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
Andy Lutomirski Feb. 24, 2015, 7:58 p.m. UTC | #3
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

Patch
diff mbox

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