This patch-set extends the EINJ driver to allow injecting a memory
error to NVDIMM. It first extends iomem resource interface to support
checking a NVDIMM region.
Patch 1/3 changes region_intersects() to accept non-RAM regions, and
adds region_intersects_ram().
Patch 2/3 adds region_intersects_pmem() to check a NVDIMM region.
Patch 3/3 changes the EINJ driver to allow injecting a memory error
to NVDIMM.
---
v3:
- Check the param2 value before target memory type. (Tony Luck)
- Add a blank line before if-statement. Remove an unnecessary brakets.
(Borislav Petkov)
v2:
- Change the EINJ driver to call region_intersects_ram() for checking
RAM with a specified size. (Dan Williams)
- Add export to region_intersects_ram().
---
Toshi Kani (3):
1/3 resource: Add @flags to region_intersects()
2/3 resource: Add region_intersects_pmem()
3/3 ACPI/APEI/EINJ: Allow memory error injection to NVDIMM
---
drivers/acpi/apei/einj.c | 12 ++++++++----
include/linux/mm.h | 5 ++++-
kernel/memremap.c | 5 ++---
kernel/resource.c | 35 ++++++++++++++++++++++++++++-------
4 files changed, 42 insertions(+), 15 deletions(-)
region_intersects() checks if a specified region partially overlaps
or fully eclipses a resource identified by @name. It currently sets
resource flags statically, which prevents the caller from specifying
a non-RAM region, such as persistent memory. Add @flags so that
any region can be specified to the function.
A helper function, region_intersects_ram(), is added so that the
callers that check a RAM region do not have to specify its iomem
resource name and flags. This interface is exported for modules,
such as the EINJ driver.
Signed-off-by: Toshi Kani <[email protected]>
Reviewed-by: Dan Williams <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Vishal Verma <[email protected]>
---
include/linux/mm.h | 4 +++-
kernel/memremap.c | 5 ++---
kernel/resource.c | 23 ++++++++++++++++-------
3 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 00bad77..c776af3 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -362,7 +362,9 @@ enum {
REGION_MIXED,
};
-int region_intersects(resource_size_t offset, size_t size, const char *type);
+int region_intersects(resource_size_t offset, size_t size, const char *type,
+ unsigned long flags);
+int region_intersects_ram(resource_size_t offset, size_t size);
/* Support for virtually mapped pages */
struct page *vmalloc_to_page(const void *addr);
diff --git a/kernel/memremap.c b/kernel/memremap.c
index 7658d32..98f52f1 100644
--- a/kernel/memremap.c
+++ b/kernel/memremap.c
@@ -57,7 +57,7 @@ static void *try_ram_remap(resource_size_t offset, size_t size)
*/
void *memremap(resource_size_t offset, size_t size, unsigned long flags)
{
- int is_ram = region_intersects(offset, size, "System RAM");
+ int is_ram = region_intersects_ram(offset, size);
void *addr = NULL;
if (is_ram == REGION_MIXED) {
@@ -162,8 +162,7 @@ static void devm_memremap_pages_release(struct device *dev, void *res)
void *devm_memremap_pages(struct device *dev, struct resource *res)
{
- int is_ram = region_intersects(res->start, resource_size(res),
- "System RAM");
+ int is_ram = region_intersects_ram(res->start, resource_size(res));
struct page_map *page_map;
int error, nid;
diff --git a/kernel/resource.c b/kernel/resource.c
index f150dbb..15c133e 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -497,6 +497,7 @@ EXPORT_SYMBOL_GPL(page_is_ram);
* @start: region start address
* @size: size of region
* @name: name of resource (in iomem_resource)
+ * @flags: flags of resource (in iomem_resource)
*
* Check if the specified region partially overlaps or fully eclipses a
* resource identified by @name. Return REGION_DISJOINT if the region
@@ -504,15 +505,11 @@ EXPORT_SYMBOL_GPL(page_is_ram);
* @type and another resource, and return REGION_INTERSECTS if the
* region overlaps @type and no other defined resource. Note, that
* REGION_INTERSECTS is also returned in the case when the specified
- * region overlaps RAM and undefined memory holes.
- *
- * region_intersect() is used by memory remapping functions to ensure
- * the user is not remapping RAM and is a vast speed up over walking
- * through the resource table page by page.
+ * region overlaps with undefined memory holes.
*/
-int region_intersects(resource_size_t start, size_t size, const char *name)
+int region_intersects(resource_size_t start, size_t size, const char *name,
+ unsigned long flags)
{
- unsigned long flags = IORESOURCE_MEM | IORESOURCE_BUSY;
resource_size_t end = start + size - 1;
int type = 0; int other = 0;
struct resource *p;
@@ -539,6 +536,18 @@ int region_intersects(resource_size_t start, size_t size, const char *name)
return REGION_DISJOINT;
}
+/*
+ * region_intersect_ram() is used by memory remapping functions to ensure
+ * the user is not remapping RAM and is a vast speed up over walking
+ * through the resource table page by page.
+ */
+int region_intersects_ram(resource_size_t start, size_t size)
+{
+ return region_intersects(start, size, "System RAM",
+ IORESOURCE_MEM | IORESOURCE_BUSY);
+}
+EXPORT_SYMBOL_GPL(region_intersects_ram);
+
void __weak arch_remove_reservations(struct resource *avail)
{
}
Add region_intersects_pmem(), which checks if a specified address
range is persistent memory registered as "Persistent Memory" in
the iomem_resource list.
Note, it does not support legacy persistent memory registered as
"Persistent Memory (legacy)". It can be supported by extending
this function or a separate function, if necessary.
This interface is exported so that it can be called from modules,
such as the EINJ driver.
Signed-off-by: Toshi Kani <[email protected]>
Reviewed-by: Dan Williams <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Vishal Verma <[email protected]>
---
include/linux/mm.h | 1 +
kernel/resource.c | 12 ++++++++++++
2 files changed, 13 insertions(+)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index c776af3..3825879 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -365,6 +365,7 @@ enum {
int region_intersects(resource_size_t offset, size_t size, const char *type,
unsigned long flags);
int region_intersects_ram(resource_size_t offset, size_t size);
+int region_intersects_pmem(resource_size_t offset, size_t size);
/* Support for virtually mapped pages */
struct page *vmalloc_to_page(const void *addr);
diff --git a/kernel/resource.c b/kernel/resource.c
index 15c133e..5230e63 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -548,6 +548,18 @@ int region_intersects_ram(resource_size_t start, size_t size)
}
EXPORT_SYMBOL_GPL(region_intersects_ram);
+/*
+ * region_intersects_pmem() checks if a specified address range is
+ * persistent memory, registered as "Persistent Memory", in the
+ * iomem_resource list.
+ */
+int region_intersects_pmem(resource_size_t start, size_t size)
+{
+ return region_intersects(start, size, "Persistent Memory",
+ IORESOURCE_MEM);
+}
+EXPORT_SYMBOL_GPL(region_intersects_pmem);
+
void __weak arch_remove_reservations(struct resource *avail)
{
}
In the case of memory error injection, einj_error_inject() checks
if a target address is regular RAM. Update this check to add a call
to region_intersects_pmem() to verify if a target address range is
NVDIMM. This allows injecting a memory error to both RAM and NVDIMM
for testing.
In addition, the current RAM check, page_is_ram(), is replaced with
region_intersects_ram() so that it can verify a target address range
with the requested size.
Signed-off-by: Toshi Kani <[email protected]>
Reviewed-by: Dan Williams <[email protected]>
Acked-by: Tony Luck <[email protected]>
Cc: Rafael J. Wysocki <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Vishal Verma <[email protected]>
---
drivers/acpi/apei/einj.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
index 0431883..52792d2 100644
--- a/drivers/acpi/apei/einj.c
+++ b/drivers/acpi/apei/einj.c
@@ -519,7 +519,7 @@ static int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2,
u64 param3, u64 param4)
{
int rc;
- unsigned long pfn;
+ u64 base_addr, size;
/* If user manually set "flags", make sure it is legal */
if (flags && (flags &
@@ -545,10 +545,15 @@ static int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2,
/*
* Disallow crazy address masks that give BIOS leeway to pick
* injection address almost anywhere. Insist on page or
- * better granularity and that target address is normal RAM.
+ * better granularity and that target address is normal RAM or
+ * NVDIMM.
*/
- pfn = PFN_DOWN(param1 & param2);
- if (!page_is_ram(pfn) || ((param2 & PAGE_MASK) != PAGE_MASK))
+ base_addr = param1 & param2;
+ size = ~param2 + 1;
+
+ if (((param2 & PAGE_MASK) != PAGE_MASK) ||
+ ((region_intersects_ram(base_addr, size) != REGION_INTERSECTS) &&
+ (region_intersects_pmem(base_addr, size) != REGION_INTERSECTS)))
return -EINVAL;
inject:
On Tue, Nov 24, 2015 at 03:33:36PM -0700, Toshi Kani wrote:
> region_intersects() checks if a specified region partially overlaps
> or fully eclipses a resource identified by @name. It currently sets
> resource flags statically, which prevents the caller from specifying
> a non-RAM region, such as persistent memory. Add @flags so that
> any region can be specified to the function.
>
> A helper function, region_intersects_ram(), is added so that the
> callers that check a RAM region do not have to specify its iomem
> resource name and flags. This interface is exported for modules,
> such as the EINJ driver.
>
> Signed-off-by: Toshi Kani <[email protected]>
> Reviewed-by: Dan Williams <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Vishal Verma <[email protected]>
> ---
> include/linux/mm.h | 4 +++-
> kernel/memremap.c | 5 ++---
> kernel/resource.c | 23 ++++++++++++++++-------
> 3 files changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 00bad77..c776af3 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -362,7 +362,9 @@ enum {
> REGION_MIXED,
> };
>
> -int region_intersects(resource_size_t offset, size_t size, const char *type);
> +int region_intersects(resource_size_t offset, size_t size, const char *type,
> + unsigned long flags);
> +int region_intersects_ram(resource_size_t offset, size_t size);
>
> /* Support for virtually mapped pages */
> struct page *vmalloc_to_page(const void *addr);
> diff --git a/kernel/memremap.c b/kernel/memremap.c
> index 7658d32..98f52f1 100644
> --- a/kernel/memremap.c
> +++ b/kernel/memremap.c
> @@ -57,7 +57,7 @@ static void *try_ram_remap(resource_size_t offset, size_t size)
> */
> void *memremap(resource_size_t offset, size_t size, unsigned long flags)
> {
> - int is_ram = region_intersects(offset, size, "System RAM");
Ok, question: why do those resource things types gets identified with
a string?! We have here "System RAM" and next patch adds "Persistent
Memory".
And "persistent memory" or "System RaM" won't work and this is just
silly.
Couldn't struct resource have gained some typedef flags instead which we
can much easily test? Using the strings looks really yucky.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
On Tue, Dec 1, 2015 at 5:50 AM, Borislav Petkov <[email protected]> wrote:
> On Tue, Nov 24, 2015 at 03:33:36PM -0700, Toshi Kani wrote:
>> region_intersects() checks if a specified region partially overlaps
>> or fully eclipses a resource identified by @name. It currently sets
>> resource flags statically, which prevents the caller from specifying
>> a non-RAM region, such as persistent memory. Add @flags so that
>> any region can be specified to the function.
>>
>> A helper function, region_intersects_ram(), is added so that the
>> callers that check a RAM region do not have to specify its iomem
>> resource name and flags. This interface is exported for modules,
>> such as the EINJ driver.
>>
>> Signed-off-by: Toshi Kani <[email protected]>
>> Reviewed-by: Dan Williams <[email protected]>
>> Cc: Andrew Morton <[email protected]>
>> Cc: Vishal Verma <[email protected]>
>> ---
>> include/linux/mm.h | 4 +++-
>> kernel/memremap.c | 5 ++---
>> kernel/resource.c | 23 ++++++++++++++++-------
>> 3 files changed, 21 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index 00bad77..c776af3 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -362,7 +362,9 @@ enum {
>> REGION_MIXED,
>> };
>>
>> -int region_intersects(resource_size_t offset, size_t size, const char *type);
>> +int region_intersects(resource_size_t offset, size_t size, const char *type,
>> + unsigned long flags);
>> +int region_intersects_ram(resource_size_t offset, size_t size);
>>
>> /* Support for virtually mapped pages */
>> struct page *vmalloc_to_page(const void *addr);
>> diff --git a/kernel/memremap.c b/kernel/memremap.c
>> index 7658d32..98f52f1 100644
>> --- a/kernel/memremap.c
>> +++ b/kernel/memremap.c
>> @@ -57,7 +57,7 @@ static void *try_ram_remap(resource_size_t offset, size_t size)
>> */
>> void *memremap(resource_size_t offset, size_t size, unsigned long flags)
>> {
>> - int is_ram = region_intersects(offset, size, "System RAM");
>
> Ok, question: why do those resource things types gets identified with
> a string?! We have here "System RAM" and next patch adds "Persistent
> Memory".
>
> And "persistent memory" or "System RaM" won't work and this is just
> silly.
>
> Couldn't struct resource have gained some typedef flags instead which we
> can much easily test? Using the strings looks really yucky.
>
At least in the case of region_intersects() I was just following
existing strcmp() convention from walk_system_ram_range.
We could define 'const char *system_ram = "System RAM"' somewhere and
then do pointer comparisons to cut down on the thrash of adding new
flags to 'struct resource'?
Dan Williams <[email protected]> writes:
>>> @@ -57,7 +57,7 @@ static void *try_ram_remap(resource_size_t offset, size_t size)
>>> */
>>> void *memremap(resource_size_t offset, size_t size, unsigned long flags)
>>> {
>>> - int is_ram = region_intersects(offset, size, "System RAM");
>>
>> Ok, question: why do those resource things types gets identified with
>> a string?! We have here "System RAM" and next patch adds "Persistent
>> Memory".
>>
>> And "persistent memory" or "System RaM" won't work and this is just
>> silly.
>>
>> Couldn't struct resource have gained some typedef flags instead which we
>> can much easily test? Using the strings looks really yucky.
>>
>
> At least in the case of region_intersects() I was just following
> existing strcmp() convention from walk_system_ram_range.
...which is done in the page fault path. I agree with the suggestion to
get strcmp out of that path.
-Jeff
On Tue, Dec 01, 2015 at 08:54:23AM -0800, Dan Williams wrote:
> On Tue, Dec 1, 2015 at 5:50 AM, Borislav Petkov <[email protected]> wrote:
> > On Tue, Nov 24, 2015 at 03:33:36PM -0700, Toshi Kani wrote:
> >> region_intersects() checks if a specified region partially overlaps
> >> or fully eclipses a resource identified by @name. It currently sets
> >> resource flags statically, which prevents the caller from specifying
> >> a non-RAM region, such as persistent memory. Add @flags so that
> >> any region can be specified to the function.
> >>
> >> A helper function, region_intersects_ram(), is added so that the
> >> callers that check a RAM region do not have to specify its iomem
> >> resource name and flags. This interface is exported for modules,
> >> such as the EINJ driver.
> >>
> >> Signed-off-by: Toshi Kani <[email protected]>
> >> Reviewed-by: Dan Williams <[email protected]>
> >> Cc: Andrew Morton <[email protected]>
> >> Cc: Vishal Verma <[email protected]>
> >> ---
> >> include/linux/mm.h | 4 +++-
> >> kernel/memremap.c | 5 ++---
> >> kernel/resource.c | 23 ++++++++++++++++-------
> >> 3 files changed, 21 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/include/linux/mm.h b/include/linux/mm.h
> >> index 00bad77..c776af3 100644
> >> --- a/include/linux/mm.h
> >> +++ b/include/linux/mm.h
> >> @@ -362,7 +362,9 @@ enum {
> >> REGION_MIXED,
> >> };
> >>
> >> -int region_intersects(resource_size_t offset, size_t size, const char *type);
> >> +int region_intersects(resource_size_t offset, size_t size, const char *type,
> >> + unsigned long flags);
> >> +int region_intersects_ram(resource_size_t offset, size_t size);
> >>
> >> /* Support for virtually mapped pages */
> >> struct page *vmalloc_to_page(const void *addr);
> >> diff --git a/kernel/memremap.c b/kernel/memremap.c
> >> index 7658d32..98f52f1 100644
> >> --- a/kernel/memremap.c
> >> +++ b/kernel/memremap.c
> >> @@ -57,7 +57,7 @@ static void *try_ram_remap(resource_size_t offset, size_t size)
> >> */
> >> void *memremap(resource_size_t offset, size_t size, unsigned long flags)
> >> {
> >> - int is_ram = region_intersects(offset, size, "System RAM");
> >
> > Ok, question: why do those resource things types gets identified with
> > a string?! We have here "System RAM" and next patch adds "Persistent
> > Memory".
> >
> > And "persistent memory" or "System RaM" won't work and this is just
> > silly.
> >
> > Couldn't struct resource have gained some typedef flags instead which we
> > can much easily test? Using the strings looks really yucky.
> >
>
> At least in the case of region_intersects() I was just following
> existing strcmp() convention from walk_system_ram_range.
Oh sure, I didn't mean you. I was simply questioning that whole
identify-resource-by-its-name approach. And that came with:
67cf13ceed89 ("x86: optimize resource lookups for ioremap")
I just think it is silly and that we should be identifying resource
things in a more robust way.
Btw, the ->name thing in struct resource has been there since a *long*
time, added by:
commit 40f6b7cc623f95d2a08b9adae7a6793055af4768
Author: linus1 <[email protected]>
Date: Wed Jun 30 11:00:00 1999 -0600
Import 2.3.11pre1
I'm not sure what it was used for, perhaps for human-readable output in
/proc/iomem.
Let me CC Linus, he would know, most likely. akpm is already on CC.
> We could define 'const char *system_ram = "System RAM"' somewhere and
> then do pointer comparisons to cut down on the thrash of adding new
> flags to 'struct resource'?
See above. I think flags or type_flags or so should be cleaner/better...
I could be missing some aspect though, according to which, the name is
the proper way to ident those but I can't think of one...
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
On Tue, Dec 1, 2015 at 9:13 AM, Borislav Petkov <[email protected]> wrote:
>
> Oh sure, I didn't mean you. I was simply questioning that whole
> identify-resource-by-its-name approach. And that came with:
>
> 67cf13ceed89 ("x86: optimize resource lookups for ioremap")
>
> I just think it is silly and that we should be identifying resource
> things in a more robust way.
I could easily imagine just adding a IORESOURCE_RAM flag (or SYSMEM or
whatever). That sounds sane. I agree that comparing the string is
ugly.
> Btw, the ->name thing in struct resource has been there since a *long*
> time
It's pretty much always been there. It is indeed meant for things
like /proc/iomem etc, and as a debug aid when printing conflicts,
yadda yadda. Just showing the numbers is usually useless for figuring
out exactly *what* something conflicts with.
Linus
On Thu, Dec 03, 2015 at 11:54:19AM -0700, Toshi Kani wrote:
> Adding a new type for regular memory will require inspecting the codes
> using IORESOURCE_MEM currently, and modify them to use the new type if
> their target ranges are regular memory. There are many references to this
> type across multiple architectures and drivers, which make this inspection
> and testing challenging.
What's wrong with adding a new type_flags to struct resource and not
touching IORESOURCE_* at all?
They'll be called something like RES_TYPE_RAM, _PMEM, _SYSMEM...
Or would that confuse more...?
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
On Tue, 2015-12-01 at 09:19 -0800, Linus Torvalds wrote:
> On Tue, Dec 1, 2015 at 9:13 AM, Borislav Petkov <[email protected]> wrote:
> >
> > Oh sure, I didn't mean you. I was simply questioning that whole
> > identify-resource-by-its-name approach. And that came with:
> >
> > 67cf13ceed89 ("x86: optimize resource lookups for ioremap")
> >
> > I just think it is silly and that we should be identifying resource
> > things in a more robust way.
>
> I could easily imagine just adding a IORESOURCE_RAM flag (or SYSMEM or
> whatever). That sounds sane. I agree that comparing the string is
> ugly.
>
> > Btw, the ->name thing in struct resource has been there since a *long*
> > time
>
> It's pretty much always been there. It is indeed meant for things
> like /proc/iomem etc, and as a debug aid when printing conflicts,
> yadda yadda. Just showing the numbers is usually useless for figuring
> out exactly *what* something conflicts with.
I agree that regular memory should have its own type, which separates
itself from MMIO. By looking at how IORESOURCE types are used, this change
has the following challenges, and I am sure I missed some more.
1. Large number of IORESOURCE_MEM usage
Adding a new type for regular memory will require inspecting the codes
using IORESOURCE_MEM currently, and modify them to use the new type if
their target ranges are regular memory. There are many references to this
type across multiple architectures and drivers, which make this inspection
and testing challenging.
http://lxr.free-electrons.com/ident?i=IORESOURCE_MEM
2. Lack of free flags bit in resource
The flags bits are defined in include/linux/ioport.h. The flags are
defined as unsigned long, which is 32-bit in 32-bit config. The most of
the bits have been assigned already. Bus-specific bits for IORESOURCE_MEM
have been assigned mostly as well (line 82).
3. Interaction with pnp subsystem
The same IORESOURCE types and bus-specific flags are used by the pnp
subsystem. pnp_mem objects represent IORESOURCE_MEM type listed by
pnp_dev. Adding a new IORESOURCE type likely requires adding a new object
type and its interfaces to pnp.
4. I/O resource names represent allocation types
While IORESOURCE types represent hardware types and capabilities, the
string names represent resource allocation types and usages. For instance,
regular memory is allocated for the OS as "System RAM", kdump as "Crash
kernel", FW as "ACPI Tables", and so on. Hence, a new type representing
"System RAM" needs to be usage based, which is different from the current
IORESOURCE types.
I think this work will require a separate patch series at least. For this
patch series, supporting error injections to NVDIMM, I propose that we make
the change suggested by Dan:
"We could define 'const char *system_ram = "System RAM"' somewhere andthen
do pointer comparisons to cut down on the thrash of adding newflags to
'struct resource'?"
Let me know if you have any suggestions/concerns.
Thanks,
-Toshi
On Thu, Dec 3, 2015 at 10:40 AM, Borislav Petkov <[email protected]> wrote:
> On Thu, Dec 03, 2015 at 11:54:19AM -0700, Toshi Kani wrote:
>> Adding a new type for regular memory will require inspecting the codes
>> using IORESOURCE_MEM currently, and modify them to use the new type if
>> their target ranges are regular memory. There are many references to this
>> type across multiple architectures and drivers, which make this inspection
>> and testing challenging.
>
> What's wrong with adding a new type_flags to struct resource and not
> touching IORESOURCE_* at all?
Bah. Both of these ideas are bogus.
Just add a new flag. The bits are already modifiers that you can
*combine* to show what kind of resource it is, and we already have
things like IORESOURCE_PREFETCH etc, that are in *addition* to the
normal IORESOURCE_MEM bit.
Just add another modifier: IORESOURCE_RAM.
So it would still show up as IORESOURCE_MEM, but it would have
additional information specifying that it's actually RAM.
If somebody does something like
if (res->flags == IORESOURCE_MEM)
then they are already completely broken and won't work *anyway*. It's
a bitmask, bit a set of values.
Linus
On Thu, 2015-12-03 at 11:01 -0800, Linus Torvalds wrote:
> On Thu, Dec 3, 2015 at 10:40 AM, Borislav Petkov <[email protected]> wrote:
> > On Thu, Dec 03, 2015 at 11:54:19AM -0700, Toshi Kani wrote:
> > > Adding a new type for regular memory will require inspecting the
> > > codes using IORESOURCE_MEM currently, and modify them to use the new
> > > type if their target ranges are regular memory. There are many
> > > references to this type across multiple architectures and drivers,
> > > which make this inspection and testing challenging.
> >
> > What's wrong with adding a new type_flags to struct resource and not
> > touching IORESOURCE_* at all?
>
> Bah. Both of these ideas are bogus.
>
> Just add a new flag. The bits are already modifiers that you can
> *combine* to show what kind of resource it is, and we already have
> things like IORESOURCE_PREFETCH etc, that are in *addition* to the
> normal IORESOURCE_MEM bit.
>
> Just add another modifier: IORESOURCE_RAM.
>
> So it would still show up as IORESOURCE_MEM, but it would have
> additional information specifying that it's actually RAM.
>
> If somebody does something like
>
> if (res->flags == IORESOURCE_MEM)
>
> then they are already completely broken and won't work *anyway*. It's
> a bitmask, bit a set of values.
Yes, if we can assign new modifiers, that will be quite simple. :-) I
assume we can allocate new bits from the remaining free bits as follows.
+#define IORESOURCE_SYSTEM_RAM 0x01000000 /* System RAM */
+#define IORESOURCE_PMEM 0x02000000 /* Persistent memory */
#define IORESOURCE_EXCLUSIVE 0x08000000 /* Userland may not map
this resource */
Note, SYSTEM_RAM represents the OS memory, i.e. "System RAM", not any RAM
ranges.
With the new modifiers, region_intersect() can check these ranges. One
caveat is that the modifiers are not very extensible for new types as they
are bit maps. region_intersect() will no longer be capable of checking any
regions with any given name. I think this is OK since this function was
introduced recently, and is only used for checking "System RAM" and
"Persistent Memory" (with this patch series).
Thanks,
-Toshi
On Thu, Dec 3, 2015 at 12:35 PM, Toshi Kani <[email protected]> wrote:
> On Thu, 2015-12-03 at 11:01 -0800, Linus Torvalds wrote:
>> On Thu, Dec 3, 2015 at 10:40 AM, Borislav Petkov <[email protected]> wrote:
>> > On Thu, Dec 03, 2015 at 11:54:19AM -0700, Toshi Kani wrote:
>> > > Adding a new type for regular memory will require inspecting the
>> > > codes using IORESOURCE_MEM currently, and modify them to use the new
>> > > type if their target ranges are regular memory. There are many
>> > > references to this type across multiple architectures and drivers,
>> > > which make this inspection and testing challenging.
>> >
>> > What's wrong with adding a new type_flags to struct resource and not
>> > touching IORESOURCE_* at all?
>>
>> Bah. Both of these ideas are bogus.
>>
>> Just add a new flag. The bits are already modifiers that you can
>> *combine* to show what kind of resource it is, and we already have
>> things like IORESOURCE_PREFETCH etc, that are in *addition* to the
>> normal IORESOURCE_MEM bit.
>>
>> Just add another modifier: IORESOURCE_RAM.
>>
>> So it would still show up as IORESOURCE_MEM, but it would have
>> additional information specifying that it's actually RAM.
>>
>> If somebody does something like
>>
>> if (res->flags == IORESOURCE_MEM)
>>
>> then they are already completely broken and won't work *anyway*. It's
>> a bitmask, bit a set of values.
>
> Yes, if we can assign new modifiers, that will be quite simple. :-) I
> assume we can allocate new bits from the remaining free bits as follows.
>
> +#define IORESOURCE_SYSTEM_RAM 0x01000000 /* System RAM */
> +#define IORESOURCE_PMEM 0x02000000 /* Persistent memory */
> #define IORESOURCE_EXCLUSIVE 0x08000000 /* Userland may not map
> this resource */
>
> Note, SYSTEM_RAM represents the OS memory, i.e. "System RAM", not any RAM
> ranges.
>
> With the new modifiers, region_intersect() can check these ranges. One
> caveat is that the modifiers are not very extensible for new types as they
> are bit maps. region_intersect() will no longer be capable of checking any
> regions with any given name. I think this is OK since this function was
> introduced recently, and is only used for checking "System RAM" and
> "Persistent Memory" (with this patch series).
IORESOURCE_PMEM is not descriptive enough for the two different types
of pmem in the kernel. How about we go with just
IORESOURCE_SYSTEM_RAM for now since "is_ram()" checks are common. Let
the rest continue to be checked by strcmp().
For example the nvdimm-e820 driver cares about "Persistent Memory
(legacy)", while other forms of pmem may just be "reserved" and only
the driver knows that it is pmem. An IORESOURCE_PMEM would not be
reliable nor descriptive enough.
On Wed, 2015-12-09 at 08:25 -0800, Dan Williams wrote:
> On Thu, Dec 3, 2015 at 12:35 PM, Toshi Kani <[email protected]> wrote:
> > On Thu, 2015-12-03 at 11:01 -0800, Linus Torvalds wrote:
> > > On Thu, Dec 3, 2015 at 10:40 AM, Borislav Petkov <[email protected]>
> > > wrote:
> > > > On Thu, Dec 03, 2015 at 11:54:19AM -0700, Toshi Kani wrote:
> > > > > Adding a new type for regular memory will require inspecting the
> > > > > codes using IORESOURCE_MEM currently, and modify them to use the
> > > > > new type if their target ranges are regular memory. There are
> > > > > many references to this type across multiple architectures and
> > > > > drivers, which make this inspection and testing challenging.
> > > >
> > > > What's wrong with adding a new type_flags to struct resource and
> > > > not touching IORESOURCE_* at all?
> > >
> > > Bah. Both of these ideas are bogus.
> > >
> > > Just add a new flag. The bits are already modifiers that you can
> > > *combine* to show what kind of resource it is, and we already have
> > > things like IORESOURCE_PREFETCH etc, that are in *addition* to the
> > > normal IORESOURCE_MEM bit.
> > >
> > > Just add another modifier: IORESOURCE_RAM.
> > >
> > > So it would still show up as IORESOURCE_MEM, but it would have
> > > additional information specifying that it's actually RAM.
> > >
> > > If somebody does something like
> > >
> > > if (res->flags == IORESOURCE_MEM)
> > >
> > > then they are already completely broken and won't work *anyway*. It's
> > > a bitmask, bit a set of values.
> >
> > Yes, if we can assign new modifiers, that will be quite simple. :-) I
> > assume we can allocate new bits from the remaining free bits as
> > follows.
> >
> > +#define IORESOURCE_SYSTEM_RAM 0x01000000 /* System RAM */
> > +#define IORESOURCE_PMEM 0x02000000 /* Persistent memory */
> > #define IORESOURCE_EXCLUSIVE 0x08000000 /* Userland may not map
> > this resource */
> >
> > Note, SYSTEM_RAM represents the OS memory, i.e. "System RAM", not any
> > RAM ranges.
> >
> > With the new modifiers, region_intersect() can check these ranges. One
> > caveat is that the modifiers are not very extensible for new types as
> > they are bit maps. region_intersect() will no longer be capable of
> > checking any regions with any given name. I think this is OK since
> > this function was introduced recently, and is only used for checking
> > "System RAM" and "Persistent Memory" (with this patch series).
>
> IORESOURCE_PMEM is not descriptive enough for the two different types
> of pmem in the kernel. How about we go with just
> IORESOURCE_SYSTEM_RAM for now since "is_ram()" checks are common. Let
> the rest continue to be checked by strcmp().
>
> For example the nvdimm-e820 driver cares about "Persistent Memory
> (legacy)", while other forms of pmem may just be "reserved" and only
> the driver knows that it is pmem. An IORESOURCE_PMEM would not be
> reliable nor descriptive enough.
Agreed. I will introduce a new type for System RAM, and leave the strcmp
check for other types.
Thanks,
-Toshi