[PATCH 0/3] Support persistent memory as reserved type in e820/EFI

[PATCH 0/3] Support persistent memory as reserved type in e820/EFI

[Toshi Kani]

ACPI 6.0 defines persistent memory ranges in multiple firmware
interfaces, E820_PMEM type in e820, EFI_PERSISTENT_MEMORY type
in EFI, and ACPI NFIT table.  This EFI spec change, however,
hit a bug in the grub bootloader, which handles EFI_PERSISTENT_MEMORY
as regular memory and potentially corrupts stored user data [1].

This issue leads FW vendors to consider using generic reserved
type in e820 and EFI to cover persistent memory, so that no new
type is used in e820 and EFI.  The kernel can initialize persistent
memory from ACPI NFIT table alone.  This basic approach may
continue in future that new types will only be defined to ACPI
tables.

This however causes a problem in the iomem table.  On x86, for
instance, e820_reserve_resources() initializes top-level entries
(iomem_resource.child) from the e820 table in early boot-time.
The reserved type does not provide any specific type information.

This patch-set introduces iomem_set_desc() to allow drivers, such
as ACPI drivers, to set I/O descriptor to a corresponding top-level
iomem entry when they enumerate ACPI tables later in the boot sequence
or run-time.

[1] https://lists.gnu.org/archive/html/grub-devel/2015-11/msg00209.html

Patch 1 fixes __request_region() to handle inheritance of attribute
properly, which I noticed while testing this patch-set.

Patch 2 adds iomem_set_desc(), which allows drivers (ex. ACPI) to set
I/O descriptor to a corresponding iomem entry. 

Patch 3 changes the ACPI nfit driver to call iomem_set_desc() for
persistent memory ranges described in ACPI NFIT table.

---
This patch-set applies on top of the io resource patch-set below, and
is based on the tip tree.
https://lkml.org/lkml/2016/1/26/886

---
Toshi Kani (3):
 1/3 resource: Make __request_region to inherit from immediate parent
 2/3 resource: Add iomem_set_desc() to set I/O descriptor
 3/3 ACPI: change NFIT driver to set pmem type to iomem entry

---
 drivers/acpi/nfit.c    |  6 ++++++
 include/linux/ioport.h |  2 ++
 kernel/resource.c      | 48 +++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 53 insertions(+), 3 deletions(-)

[PATCH 1/3] resource: Make __request_region to inherit from immediate parent

[Toshi Kani]

__request_region() sets 'flags' from a given 'parent' to inherit
its attribute.  When a target resource has conflict, this function
inserts a resource entry under the conflicted entry by updating
'parent'.  In this case, the new resource entry needs to inherit
attribute from the updated parent.

For instance, request_mem_region() calls __request_region() with
'parent' set to &iomem_resource, which is the root entry of the
whole iomem range.  When this request results in inserting a new
entry "new" under "DEV-A", "new" needs to inherit from the immediate
parent "DEV-A" as it holds specific attribute for the range.

root (&iomem_resource)
 :
 + "DEV-A"
    + "new"

Change __request_region() to set 'flags' and 'desc' of a new entry
from the immediate parent.

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Dan Williams <dan.j.williams@intel.com>
---
 kernel/resource.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index 4983430..9df79ff 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1085,15 +1085,16 @@ struct resource * __request_region(struct resource *parent,
 	res->name = name;
 	res->start = start;
 	res->end = start + n - 1;
-	res->flags = resource_type(parent) | resource_ext_type(parent);
-	res->flags |= IORESOURCE_BUSY | flags;
-	res->desc = IORES_DESC_NONE;
 
 	write_lock(&resource_lock);
 
 	for (;;) {
 		struct resource *conflict;
 
+		res->flags = resource_type(parent) | resource_ext_type(parent);
+		res->flags |= IORESOURCE_BUSY | flags;
+		res->desc = parent->desc;
+
 		conflict = __request_resource(parent, res);
 		if (!conflict)
 			break;

[PATCH 2/3] resource: Add iomem_set_desc() to set I/O descriptor

[Toshi Kani]

ACPI 6.0 defines persistent memory ranges in multiple firmware
interfaces, E820_PMEM type in e820, EFI_PERSISTENT_MEMORY type
in EFI, and ACPI NFIT table.  This EFI spec change, however,
hit a bug in the grub bootloader, which handles EFI_PERSISTENT_MEMORY
as regular memory and potentially corrupts stored user data [1].

This issue leads FW vendors to consider using generic reserved
type in e820 and EFI to cover persistent memory, so that no new
type is used in e820 and EFI.  The kernel can initialize persistent
memory from ACPI NFIT table alone.  This basic approach may
continue in future that new types will only be defined to ACPI
tables.

This however causes a problem in the iomem table.  On x86, for
instance, e820_reserve_resources() initializes top-level entries
(iomem_resource.child) from the e820 table in early boot-time.
The reserved type does not provide any specific type information.

Hence, this patch adds iomem_set_desc(), which allows drivers
to set IO descriptor to a corresponding top-level iomem entry
that is not marked as BUSY.  Drivers, such as ACPI drivers, can
call this interface to set a specific type when they enumerate
ACPI tables later in the boot sequence or run-time.

[1] https://lists.gnu.org/archive/html/grub-devel/2015-11/msg00209.html
Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Dan Williams <dan.j.williams@intel.com>
---
 include/linux/ioport.h |    2 ++
 kernel/resource.c      |   41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index afb4559..54f7435 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -260,6 +260,8 @@ extern void __devm_release_region(struct device *dev, struct resource *parent,
 				  resource_size_t start, resource_size_t n);
 extern int iomem_map_sanity_check(resource_size_t addr, unsigned long size);
 extern int iomem_is_exclusive(u64 addr);
+extern int iomem_set_desc(resource_size_t start, size_t size,
+	unsigned long desc);
 
 extern int
 walk_system_ram_range(unsigned long start_pfn, unsigned long nr_pages,
diff --git a/kernel/resource.c b/kernel/resource.c
index 9df79ff..7d68297 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1534,6 +1534,47 @@ int iomem_is_exclusive(u64 addr)
 	return err;
 }
 
+/**
+ * iomem_set_desc - set I/O descriptor to the corresponding iomem entry
+ * @start: start address
+ * @size: size of the range
+ * @desc: I/O descriptor
+ *
+ * Set IO descriptor to the corresponding top-level iomem entry.
+ * Return 0 for success, -EBUSY when the entry is marked as BUSY,
+ * -EINVAL when no corresponding entry is found.
+ */
+int iomem_set_desc(resource_size_t start, size_t size, unsigned long desc)
+{
+	resource_size_t end = start + size - 1;
+	struct resource *p;
+	int match = 0, ret = -EINVAL;
+
+	write_lock(&resource_lock);
+
+	for (p = iomem_resource.child; p ; p = p->sibling) {
+		if (p->start < start)
+			continue;
+		if ((p->start == start) && (p->end == end))
+			match = 1;
+		break;
+	}
+
+	if (match) {
+		if (p->flags & IORESOURCE_BUSY) {
+			ret = -EBUSY;
+		} else {
+			p->desc = desc;
+			ret = 0;
+		}
+	}
+
+	write_unlock(&resource_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(iomem_set_desc);
+
 struct resource_entry *resource_list_create_entry(struct resource *res,
 						  size_t extra_size)
 {

[PATCH 3/3] ACPI: Change NFIT driver to set PMEM type to iomem entry

[Toshi Kani]

Change acpi_nfit_register_region() to call iomem_set_desc() with
IORES_DESC_PERSISTENT_MEMORY for NFIT_SPA_PM ranges found in ACPI
NFIT table.

When FW sets E820_PMEM in e820 and EFI_PERSISTENT_MEMORY in EFI,
this code simply sets PMEM type again to "Persistent Memory" entries
in the iomem table.  When FW sets reserved type for persistent
memory ranges, it sets PMEM type to "reserved" entries covering
PMEM ranges.

This allows the EINJ driver, which calls region_intersects() with
IORES_DESC_PERSISTENT_MEMORY to check persistent memory ranges,
to work continuously even if FW sets reserved type to persistent
memory in e820 and EFI.

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 drivers/acpi/nfit.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index ad6d8c6..add04f0 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -1781,6 +1781,12 @@ static int acpi_nfit_register_region(struct acpi_nfit_desc *acpi_desc,
 
 	nvdimm_bus = acpi_desc->nvdimm_bus;
 	if (nfit_spa_type(spa) == NFIT_SPA_PM) {
+		rc = iomem_set_desc(spa->address, spa->length,
+					IORES_DESC_PERSISTENT_MEMORY);
+		if (rc)
+			dev_dbg(acpi_desc->dev,
+				"error setting iomem desc: %d\n", rc);
+
 		rc = acpi_nfit_find_poison(acpi_desc, ndr_desc);
 		if (rc) {
 			dev_err(acpi_desc->dev,

Re: [PATCH 3/3] ACPI: Change NFIT driver to set PMEM type to iomem entry

[Dan Williams]

On Tue, Feb 2, 2016 at 10:55 AM, Toshi Kani <toshi.kani@hpe.com> wrote:
> Change acpi_nfit_register_region() to call iomem_set_desc() with
> IORES_DESC_PERSISTENT_MEMORY for NFIT_SPA_PM ranges found in ACPI
> NFIT table.
>
> When FW sets E820_PMEM in e820 and EFI_PERSISTENT_MEMORY in EFI,
> this code simply sets PMEM type again to "Persistent Memory" entries
> in the iomem table.  When FW sets reserved type for persistent
> memory ranges, it sets PMEM type to "reserved" entries covering
> PMEM ranges.
>
> This allows the EINJ driver, which calls region_intersects() with
> IORES_DESC_PERSISTENT_MEMORY to check persistent memory ranges,
> to work continuously even if FW sets reserved type to persistent
> memory in e820 and EFI.
>
> Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
>  drivers/acpi/nfit.c |    6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> index ad6d8c6..add04f0 100644
> --- a/drivers/acpi/nfit.c
> +++ b/drivers/acpi/nfit.c
> @@ -1781,6 +1781,12 @@ static int acpi_nfit_register_region(struct acpi_nfit_desc *acpi_desc,
>
>         nvdimm_bus = acpi_desc->nvdimm_bus;
>         if (nfit_spa_type(spa) == NFIT_SPA_PM) {
> +               rc = iomem_set_desc(spa->address, spa->length,
> +                                       IORES_DESC_PERSISTENT_MEMORY);
> +               if (rc)
> +                       dev_dbg(acpi_desc->dev,
> +                               "error setting iomem desc: %d\n", rc);
> +

Hmm, if we set the type on driver load, should we clear the type on
driver unload?

Actually it might be more straightforward to specify a type at
request_region() time.  That way it gets released at release_region().
We're already setting a resource name at request_region time, adding a
type annotation at the time seems appropriate.

Re: [PATCH 3/3] ACPI: Change NFIT driver to set PMEM type to iomem entry

[Toshi Kani]

On Fri, 2016-02-12 at 11:41 -0800, Dan Williams wrote:
> On Tue, Feb 2, 2016 at 10:55 AM, Toshi Kani <toshi.kani@hpe.com> wrote:
> > Change acpi_nfit_register_region() to call iomem_set_desc() with
> > IORES_DESC_PERSISTENT_MEMORY for NFIT_SPA_PM ranges found in ACPI
> > NFIT table.
> > 
> > When FW sets E820_PMEM in e820 and EFI_PERSISTENT_MEMORY in EFI,
> > this code simply sets PMEM type again to "Persistent Memory" entries
> > in the iomem table.  When FW sets reserved type for persistent
> > memory ranges, it sets PMEM type to "reserved" entries covering
> > PMEM ranges.
> > 
> > This allows the EINJ driver, which calls region_intersects() with
> > IORES_DESC_PERSISTENT_MEMORY to check persistent memory ranges,
> > to work continuously even if FW sets reserved type to persistent
> > memory in e820 and EFI.
> > 
> > Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Cc: Borislav Petkov <bp@suse.de>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > ---
> >  drivers/acpi/nfit.c |    6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> > index ad6d8c6..add04f0 100644
> > --- a/drivers/acpi/nfit.c
> > +++ b/drivers/acpi/nfit.c
> > @@ -1781,6 +1781,12 @@ static int acpi_nfit_register_region(struct
> > acpi_nfit_desc *acpi_desc,
> > 
> >         nvdimm_bus = acpi_desc->nvdimm_bus;
> >         if (nfit_spa_type(spa) == NFIT_SPA_PM) {
> > +               rc = iomem_set_desc(spa->address, spa->length,
> > +                                       IORES_DESC_PERSISTENT_MEMORY);
> > +               if (rc)
> > +                       dev_dbg(acpi_desc->dev,
> > +                               "error setting iomem desc: %d\n", rc);
> > +
> 
> Hmm, if we set the type on driver load, should we clear the type on
> driver unload?

I think this type update should stay for the life-cycle of this iomem entry
itself since this range is PMEM even after the driver is unloaded.  This is
an extension of the boot-time iomem table initialization from e820/EFI,
which allows ACPI to set a correct type.  This is independent from driver's
resource allocations.

> Actually it might be more straightforward to specify a type at
> request_region() time.  That way it gets released at release_region().
> We're already setting a resource name at request_region time, adding a
> type annotation at the time seems appropriate.

I first considered simply setting "namespaceX.X" as PMEM.  However,
region_intersects() and its friends only check the top-level entries, not
their children, of the iomem table.  And I think a child should have the
same type as the parent as I fixed it in patch 1/3.

Thanks,
-Toshi

Re: [PATCH 3/3] ACPI: Change NFIT driver to set PMEM type to iomem entry

[Dan Williams]

On Fri, Feb 12, 2016 at 2:30 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
> On Fri, 2016-02-12 at 11:41 -0800, Dan Williams wrote:
>> On Tue, Feb 2, 2016 at 10:55 AM, Toshi Kani <toshi.kani@hpe.com> wrote:
>> > Change acpi_nfit_register_region() to call iomem_set_desc() with
>> > IORES_DESC_PERSISTENT_MEMORY for NFIT_SPA_PM ranges found in ACPI
>> > NFIT table.
>> >
>> > When FW sets E820_PMEM in e820 and EFI_PERSISTENT_MEMORY in EFI,
>> > this code simply sets PMEM type again to "Persistent Memory" entries
>> > in the iomem table.  When FW sets reserved type for persistent
>> > memory ranges, it sets PMEM type to "reserved" entries covering
>> > PMEM ranges.
>> >
>> > This allows the EINJ driver, which calls region_intersects() with
>> > IORES_DESC_PERSISTENT_MEMORY to check persistent memory ranges,
>> > to work continuously even if FW sets reserved type to persistent
>> > memory in e820 and EFI.
>> >
>> > Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
>> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
>> > Cc: Dan Williams <dan.j.williams@intel.com>
>> > Cc: Ingo Molnar <mingo@kernel.org>
>> > Cc: Borislav Petkov <bp@suse.de>
>> > Cc: Andrew Morton <akpm@linux-foundation.org>
>> > ---
>> >  drivers/acpi/nfit.c |    6 ++++++
>> >  1 file changed, 6 insertions(+)
>> >
>> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
>> > index ad6d8c6..add04f0 100644
>> > --- a/drivers/acpi/nfit.c
>> > +++ b/drivers/acpi/nfit.c
>> > @@ -1781,6 +1781,12 @@ static int acpi_nfit_register_region(struct
>> > acpi_nfit_desc *acpi_desc,
>> >
>> >         nvdimm_bus = acpi_desc->nvdimm_bus;
>> >         if (nfit_spa_type(spa) == NFIT_SPA_PM) {
>> > +               rc = iomem_set_desc(spa->address, spa->length,
>> > +                                       IORES_DESC_PERSISTENT_MEMORY);
>> > +               if (rc)
>> > +                       dev_dbg(acpi_desc->dev,
>> > +                               "error setting iomem desc: %d\n", rc);
>> > +
>>
>> Hmm, if we set the type on driver load, should we clear the type on
>> driver unload?
>
> I think this type update should stay for the life-cycle of this iomem entry
> itself since this range is PMEM even after the driver is unloaded.  This is
> an extension of the boot-time iomem table initialization from e820/EFI,
> which allows ACPI to set a correct type.  This is independent from driver's
> resource allocations.
>
>> Actually it might be more straightforward to specify a type at
>> request_region() time.  That way it gets released at release_region().
>> We're already setting a resource name at request_region time, adding a
>> type annotation at the time seems appropriate.
>
> I first considered simply setting "namespaceX.X" as PMEM.  However,
> region_intersects() and its friends only check the top-level entries, not
> their children, of the iomem table.  And I think a child should have the
> same type as the parent as I fixed it in patch 1/3.

Did we investigate updating region_intersects() to check children?
When a child sub-divides a region with different types it may be the
wrong answer to check the parent.  Is there a problem with moving
checking to the child?

Re: [PATCH 3/3] ACPI: Change NFIT driver to set PMEM type to iomem entry

[Toshi Kani]

On Fri, 2016-02-12 at 15:32 -0800, Dan Williams wrote:
> On Fri, Feb 12, 2016 at 2:30 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
> > On Fri, 2016-02-12 at 11:41 -0800, Dan Williams wrote:
> > > On Tue, Feb 2, 2016 at 10:55 AM, Toshi Kani <toshi.kani@hpe.com> wro
 :
> > > Hmm, if we set the type on driver load, should we clear the type on
> > > driver unload?
> > 
> > I think this type update should stay for the life-cycle of this iomem
> > entry itself since this range is PMEM even after the driver is
> > unloaded.  This is an extension of the boot-time iomem table
> > initialization from e820/EFI, which allows ACPI to set a correct
> > type.  This is independent from driver's resource allocations.
> > 
> > > Actually it might be more straightforward to specify a type at
> > > request_region() time.  That way it gets released at
> > > release_region().  We're already setting a resource name at
> > > request_region time, adding a type annotation at the time seems
> > > appropriate.
> > 
> > I first considered simply setting "namespaceX.X" as PMEM.  However,
> > region_intersects() and its friends only check the top-level entries,
> > not their children, of the iomem table.  And I think a child should
> > have the same type as the parent as I fixed it in patch 1/3.
> 
> Did we investigate updating region_intersects() to check children?
> When a child sub-divides a region with different types it may be the
> wrong answer to check the parent.  Is there a problem with moving
> checking to the child?

Here are three options I can think of.

1) Set pmem type to "reserved" (This patch-set)
 - Add a new iomem_set_desc(), which sets a given type to a top-level
entry.  Change the ACPI NFIT driver to call it to set pmem type to
"reserved" entry.
 - region_intersects() finds a pmem entry by checking top-level entries (no
change).

2) Change region_intersects() to check children's type
 - Add a new request_region_ext(), which is an extension to
request_region() to allow specifying a type of resource.  It puts a new
child entry under "reserved".  Change the pmem driver to call this func.
 - Change region_intersects() to check children's type for finding this
child pmem entry.

3) Pmem driver to call insert_resource()
 - Change the pmem driver to call insert_resource(), which puts a new pmem
entry as the parent of "reserved".
 - region_intersects() finds a pmem entry by checking top-level entries (no
change).
 - Add a new release_resource_self(), which releases a given entry and
keeps its children if any.  Change the pmem driver to call it for release.


This patch-set implements 1).  The pmem type is set to "reserved" for its
life-cycle.  This option is simplest.

For 2), the changes to region_intersects() may be too complex for
maintenance.  Here are a few examples when region_intersects() is called
with addr [1-10] where iomem has entry P and its children.

Case A: P is fully covered by children C1 & C2.  region_intersects()
ignores P's type, but checks C1 and C2's.
  
  P [1-10] + C1 [1-5]
           + C2 [6-10]

Case B: C2 is fully covered by C3, but P is not.  region_intersects()
ignores C2's type, but checks P, C1, C3's.

  P [1-10] + C1 [1-2]
           + C2 [6-10] + C3 [6-10]

I think region_intersects() will need to construct a flat table from the
tree while making recursive calls to walk thru all children.

3) is similar to 2), but avoids the changes to region_intersects() since
insert_resource() inserts a new entry as the parent to "reserved".
 However, a new interface is necessary to put "reserved" back to top-level
when releasing the added entry.

My recommendation is go with either 1) or 3).  What do you think?

Thanks,
-Toshi

Re: [PATCH 3/3] ACPI: Change NFIT driver to set PMEM type to iomem entry

[Toshi Kani]

On Tue, 2016-02-16 at 19:00 -0700, Toshi Kani wrote:
> On Fri, 2016-02-12 at 15:32 -0800, Dan Williams wrote:
> > On Fri, Feb 12, 2016 at 2:30 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
> > > On Fri, 2016-02-12 at 11:41 -0800, Dan Williams wrote:
> > > > On Tue, Feb 2, 2016 at 10:55 AM, Toshi Kani <toshi.kani@hpe.com>
> > > > wro
>  :
> > > > Hmm, if we set the type on driver load, should we clear the type on
> > > > driver unload?
> > > 
> > > I think this type update should stay for the life-cycle of this iomem
> > > entry itself since this range is PMEM even after the driver is
> > > unloaded.  This is an extension of the boot-time iomem table
> > > initialization from e820/EFI, which allows ACPI to set a correct
> > > type.  This is independent from driver's resource allocations.
> > > 
> > > > Actually it might be more straightforward to specify a type at
> > > > request_region() time.  That way it gets released at
> > > > release_region().  We're already setting a resource name at
> > > > request_region time, adding a type annotation at the time seems
> > > > appropriate.
> > > 
> > > I first considered simply setting "namespaceX.X" as PMEM.  However,
> > > region_intersects() and its friends only check the top-level entries,
> > > not their children, of the iomem table.  And I think a child should
> > > have the same type as the parent as I fixed it in patch 1/3.
> > 
> > Did we investigate updating region_intersects() to check children?
> > When a child sub-divides a region with different types it may be the
> > wrong answer to check the parent.  Is there a problem with moving
> > checking to the child?
> 
> Here are three options I can think of.
> 
> 1) Set pmem type to "reserved" (This patch-set)
>  - Add a new iomem_set_desc(), which sets a given type to a top-level
> entry.  Change the ACPI NFIT driver to call it to set pmem type to
> "reserved" entry.
>  - region_intersects() finds a pmem entry by checking top-level entries
> (no change).
> 
> 2) Change region_intersects() to check children's type
>  - Add a new request_region_ext(), which is an extension to
> request_region() to allow specifying a type of resource.  It puts a new
> child entry under "reserved".  Change the pmem driver to call this func.
>  - Change region_intersects() to check children's type for finding this
> child pmem entry.
> 
> 3) Pmem driver to call insert_resource()
>  - Change the pmem driver to call insert_resource(), which puts a new
> pmem entry as the parent of "reserved".
>  - region_intersects() finds a pmem entry by checking top-level entries
> (no change).
>  - Add a new release_resource_self(), which releases a given entry and
> keeps its children if any.  Change the pmem driver to call it for
> release.

Thinking further, 3) needs to be modified as follows.  insert_resource()
should only be allowed for producers of resource (ex. nfit), not consumers
(ex. pmem).  It also needs to export insert_resource().

3) NFIT driver to call insert_resource()
 - Change the ACPI nfit driver to call insert_resource() when a target
range is not marked as PMEM (i.e. "reserved") or not present in iomem.
 This puts a new PMEM entry as the parent of "reserved".
 - region_intersects() finds a pmem entry by checking top-level entries (no
change).
 - Add a new release_resource_self(), which releases a given entry and
keeps its children if any.  Change the nfit driver to call it for release.

> This patch-set implements 1).  The pmem type is set to "reserved" for its
> life-cycle.  This option is simplest.
> 
> For 2), the changes to region_intersects() may be too complex for
> maintenance.  

I should have said "region_intersects() may be overly complicated for this
purpose and maintenance".

> Here are a few examples when region_intersects() is called
> with addr [1-10] where iomem has entry P and its children.
> 
> Case A: P is fully covered by children C1 & C2.  region_intersects()
> ignores P's type, but checks C1 and C2's.
>   
>   P [1-10] + C1 [1-5]
>            + C2 [6-10]
> 
> Case B: C2 is fully covered by C3, but P is not.  region_intersects()
> ignores C2's type, but checks P, C1, C3's.
> 
>   P [1-10] + C1 [1-2]
>            + C2 [6-10] + C3 [6-10]
> 
> I think region_intersects() will need to construct a flat table from the
> tree while making recursive calls to walk thru all children.
> 
> 3) is similar to 2), but avoids the changes to region_intersects() since
> insert_resource() inserts a new entry as the parent to "reserved".

3) is actually similar to 1) as both options change the producer side.

>  However, a new interface is necessary to put "reserved" back to top-
> level when releasing the added entry.
> 
> My recommendation is go with either 1) or 3).  What do you think?

I think we should modify the producer side, so 1) or 3) are still my
recommendation.

Thanks,
-Toshi
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] ACPI: Change NFIT driver to set PMEM type to iomem entry

[Dan Williams]

On Wed, Feb 17, 2016 at 10:00 AM, Toshi Kani <toshi.kani@hpe.com> wrote:
> On Tue, 2016-02-16 at 19:00 -0700, Toshi Kani wrote:
>> On Fri, 2016-02-12 at 15:32 -0800, Dan Williams wrote:
>> > On Fri, Feb 12, 2016 at 2:30 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
>> > > On Fri, 2016-02-12 at 11:41 -0800, Dan Williams wrote:
>> > > > On Tue, Feb 2, 2016 at 10:55 AM, Toshi Kani <toshi.kani@hpe.com>
>> > > > wro
>>  :
>> > > > Hmm, if we set the type on driver load, should we clear the type on
>> > > > driver unload?
>> > >
>> > > I think this type update should stay for the life-cycle of this iomem
>> > > entry itself since this range is PMEM even after the driver is
>> > > unloaded.  This is an extension of the boot-time iomem table
>> > > initialization from e820/EFI, which allows ACPI to set a correct
>> > > type.  This is independent from driver's resource allocations.
>> > >
>> > > > Actually it might be more straightforward to specify a type at
>> > > > request_region() time.  That way it gets released at
>> > > > release_region().  We're already setting a resource name at
>> > > > request_region time, adding a type annotation at the time seems
>> > > > appropriate.
>> > >
>> > > I first considered simply setting "namespaceX.X" as PMEM.  However,
>> > > region_intersects() and its friends only check the top-level entries,
>> > > not their children, of the iomem table.  And I think a child should
>> > > have the same type as the parent as I fixed it in patch 1/3.
>> >
>> > Did we investigate updating region_intersects() to check children?
>> > When a child sub-divides a region with different types it may be the
>> > wrong answer to check the parent.  Is there a problem with moving
>> > checking to the child?
>>
>> Here are three options I can think of.
>>
>> 1) Set pmem type to "reserved" (This patch-set)
>>  - Add a new iomem_set_desc(), which sets a given type to a top-level
>> entry.  Change the ACPI NFIT driver to call it to set pmem type to
>> "reserved" entry.
>>  - region_intersects() finds a pmem entry by checking top-level entries
>> (no change).
>>
>> 2) Change region_intersects() to check children's type
>>  - Add a new request_region_ext(), which is an extension to
>> request_region() to allow specifying a type of resource.  It puts a new
>> child entry under "reserved".  Change the pmem driver to call this func.
>>  - Change region_intersects() to check children's type for finding this
>> child pmem entry.
>>
>> 3) Pmem driver to call insert_resource()
>>  - Change the pmem driver to call insert_resource(), which puts a new
>> pmem entry as the parent of "reserved".
>>  - region_intersects() finds a pmem entry by checking top-level entries
>> (no change).
>>  - Add a new release_resource_self(), which releases a given entry and
>> keeps its children if any.  Change the pmem driver to call it for
>> release.
>
> Thinking further, 3) needs to be modified as follows.  insert_resource()
> should only be allowed for producers of resource (ex. nfit), not consumers
> (ex. pmem).  It also needs to export insert_resource().
>
> 3) NFIT driver to call insert_resource()
>  - Change the ACPI nfit driver to call insert_resource() when a target
> range is not marked as PMEM (i.e. "reserved") or not present in iomem.
>  This puts a new PMEM entry as the parent of "reserved".
>  - region_intersects() finds a pmem entry by checking top-level entries (no
> change).
>  - Add a new release_resource_self(), which releases a given entry and
> keeps its children if any.  Change the nfit driver to call it for release.
>
>> This patch-set implements 1).  The pmem type is set to "reserved" for its
>> life-cycle.  This option is simplest.
>>
>> For 2), the changes to region_intersects() may be too complex for
>> maintenance.
>
> I should have said "region_intersects() may be overly complicated for this
> purpose and maintenance".
>
>> Here are a few examples when region_intersects() is called
>> with addr [1-10] where iomem has entry P and its children.
>>
>> Case A: P is fully covered by children C1 & C2.  region_intersects()
>> ignores P's type, but checks C1 and C2's.
>>
>>   P [1-10] + C1 [1-5]
>>            + C2 [6-10]
>>
>> Case B: C2 is fully covered by C3, but P is not.  region_intersects()
>> ignores C2's type, but checks P, C1, C3's.
>>
>>   P [1-10] + C1 [1-2]
>>            + C2 [6-10] + C3 [6-10]
>>
>> I think region_intersects() will need to construct a flat table from the
>> tree while making recursive calls to walk thru all children.
>>
>> 3) is similar to 2), but avoids the changes to region_intersects() since
>> insert_resource() inserts a new entry as the parent to "reserved".
>
> 3) is actually similar to 1) as both options change the producer side.
>
>>  However, a new interface is necessary to put "reserved" back to top-
>> level when releasing the added entry.
>>
>> My recommendation is go with either 1) or 3).  What do you think?
>
> I think we should modify the producer side, so 1) or 3) are still my
> recommendation.
>

I think 3 is the most promising option.  It aligns the acpi/nfit
driver closer with the acpi/pci_root driver that is also doing
insert_resource() for each root bridge, and removing those resources
when the bridge is disabled/removed.

I'd still want to maintain the ability of nfit to be built as a
module, so we would need to split the resource registration into a
separate built-in object file from nfit.ko.

Re: [PATCH 3/3] ACPI: Change NFIT driver to set PMEM type to iomem entry

[Toshi Kani]

On Wed, 2016-02-17 at 11:34 -0800, Dan Williams wrote:
> On Wed, Feb 17, 2016 at 10:00 AM, Toshi Kani <toshi.kani@hpe.com> wrote:
> > On Tue, 2016-02-16 at 19:00 -0700, Toshi Kani wrote:
> > > On Fri, 2016-02-12 at 15:32 -0800, Dan Williams wrote:
> > > > On Fri, Feb 12, 2016 at 2:30 PM, Toshi Kani <toshi.kani@hpe.com>
> > > > wrote:
 :
> > > Here are three options I can think of.
> > > 
> > > 1) Set pmem type to "reserved" (This patch-set)
> > >  - Add a new iomem_set_desc(), which sets a given type to a top-level
> > > entry.  Change the ACPI NFIT driver to call it to set pmem type to
> > > "reserved" entry.
> > >  - region_intersects() finds a pmem entry by checking top-level
> > > entries (no change).
> > > 
> > > 2) Change region_intersects() to check children's type
> > >  - Add a new request_region_ext(), which is an extension to
> > > request_region() to allow specifying a type of resource.  It puts a
> > > new child entry under "reserved".  Change the pmem driver to call
> > > this func.
> > >  - Change region_intersects() to check children's type for finding
> > > this child pmem entry.
> > > 
> > > 3) Pmem driver to call insert_resource()
> > >  - Change the pmem driver to call insert_resource(), which puts a new
> > > pmem entry as the parent of "reserved".
> > >  - region_intersects() finds a pmem entry by checking top-level
> > > entries (no change).
> > >  - Add a new release_resource_self(), which releases a given entry
> > > and keeps its children if any.  Change the pmem driver to call it for
> > > release.
> > 
> > Thinking further, 3) needs to be modified as follows.  insert_resource(
> > ) should only be allowed for producers of resource (ex. nfit), not
> > consumers (ex. pmem).  It also needs to export insert_resource().
> > 
> > 3) NFIT driver to call insert_resource()
> >  - Change the ACPI nfit driver to call insert_resource() when a target
> > range is not marked as PMEM (i.e. "reserved") or not present in iomem.
> >  This puts a new PMEM entry as the parent of "reserved".
> >  - region_intersects() finds a pmem entry by checking top-level entries
> > (no change).
> >  - Add a new release_resource_self(), which releases a given entry and
> > keeps its children if any.  Change the nfit driver to call it for
> > release.
> > 
> > > This patch-set implements 1).  The pmem type is set to "reserved" for
> > > its life-cycle.  This option is simplest.
> > > 
> > > For 2), the changes to region_intersects() may be too complex for
> > > maintenance.
> > 
> > I should have said "region_intersects() may be overly complicated for
> > this purpose and maintenance".
> > 
> > > Here are a few examples when region_intersects() is called
> > > with addr [1-10] where iomem has entry P and its children.
> > > 
> > > Case A: P is fully covered by children C1 & C2.  region_intersects()
> > > ignores P's type, but checks C1 and C2's.
> > > 
> > >   P [1-10] + C1 [1-5]
> > >            + C2 [6-10]
> > > 
> > > Case B: C2 is fully covered by C3, but P is not.  region_intersects()
> > > ignores C2's type, but checks P, C1, C3's.
> > > 
> > >   P [1-10] + C1 [1-2]
> > >            + C2 [6-10] + C3 [6-10]
> > > 
> > > I think region_intersects() will need to construct a flat table from
> > > the tree while making recursive calls to walk thru all children.
> > > 
> > > 3) is similar to 2), but avoids the changes to region_intersects()
> > > since insert_resource() inserts a new entry as the parent to
> > > "reserved".
> > 
> > 3) is actually similar to 1) as both options change the producer side.
> > 
> > >  However, a new interface is necessary to put "reserved" back to top-
> > > level when releasing the added entry.
> > > 
> > > My recommendation is go with either 1) or 3).  What do you think?
> > 
> > I think we should modify the producer side, so 1) or 3) are still my
> > recommendation.
> > 
> 
> I think 3 is the most promising option.  It aligns the acpi/nfit
> driver closer with the acpi/pci_root driver that is also doing
> insert_resource() for each root bridge, and removing those resources
> when the bridge is disabled/removed.
> 
> I'd still want to maintain the ability of nfit to be built as a
> module, so we would need to split the resource registration into a
> separate built-in object file from nfit.ko.

Sounds good.  I will implement 3), and add an ACPI built-in wrapper
function to call insert_resource().

Thanks!
-Toshi