| Message ID | 54EB206E.4010009@plexistor.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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);
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(-)