LinuxLists.cc - [PATCH v2 0/4] Support persistent memory as reserved type in e820/EFI

2016-03-02 21:58:19

Subject: [PATCH v2 0/4] Support persistent memory as reserved type in e820/EFI

2016-03-02 21:58:29

Subject: [PATCH v2 4/4] ACPI: Change NFIT driver to insert new resource

ACPI 6.0 defines persistent memory (PMEM) ranges in multiple
firmware interfaces, e820, EFI, and ACPI NFIT table. This EFI
change, however, leads to hit a bug in the grub bootloader, which
treats EFI_PERSISTENT_MEMORY type as regular memory and corrupts
stored user data [1].

Therefore, BIOS may set generic reserved type in e820 and EFI
to cover PMEM ranges. The kernel can initialize PMEM ranges
from ACPI NFIT table alone.

This scheme causes a problem in the iomem table, though. On x86,
for instance, e820_reserve_resources() initializes top-level entries
(iomem_resource.child) from the e820 table at early boot-time.
This creates "reserved" entry for a PMEM range, which does not allow
region_intersects() to check with PMEM type.

Change acpi_nfit_register_region() to call acpi_nfit_insert_resource(),
which calls devm_insert_resource() to insert a PMEM entry from NFIT
when the iomem table does not have a PMEM entry already. That is,
when a PMEM range is marked as reserved type in e820, it inserts
"Persistent Memory" entry, which results as follows.

+ "Persistent Memory"
+ "reserved"

This allows the EINJ driver, which calls region_intersects() to
check PMEM ranges, to work continuously even if BIOS sets reserved
type (or sets nothing) to PMEM ranges in e820 and EFI.

[1]: https://lists.gnu.org/archive/html/grub-devel/2015-11/msg00209.html
Signed-off-by: Toshi Kani <[email protected]>
Cc: Rafael J. Wysocki <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Andrew Morton <[email protected]>
---
drivers/acpi/nfit.c | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index fb53db1..d97b53f 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -1571,6 +1571,30 @@ static int ars_status_process_records(struct nvdimm_bus *nvdimm_bus,
return 0;
}

+static int acpi_nfit_insert_resource(struct acpi_nfit_desc *acpi_desc,
+ struct nd_region_desc *ndr_desc)
+{
+ struct resource *res, *nd_res = ndr_desc->res;
+ size_t size = nd_res->end - nd_res->start + 1;
+
+ /* No operation if the region is already registered as PMEM */
+ if (region_intersects(nd_res->start, size, IORESOURCE_MEM,
+ IORES_DESC_PERSISTENT_MEMORY) == REGION_INTERSECTS)
+ return 0;
+
+ res = devm_kzalloc(acpi_desc->dev, sizeof(*res), GFP_KERNEL);
+ if (!res)
+ return -ENOMEM;
+
+ res->name = "Persistent Memory";
+ res->start = nd_res->start;
+ res->end = nd_res->end;
+ res->flags = IORESOURCE_MEM;
+ res->desc = IORES_DESC_PERSISTENT_MEMORY;
+
+ return devm_insert_resource(acpi_desc->dev, &iomem_resource, res);
+}
+
static int acpi_nfit_find_poison(struct acpi_nfit_desc *acpi_desc,
struct nd_region_desc *ndr_desc)
{
@@ -1781,6 +1805,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 = acpi_nfit_insert_resource(acpi_desc, ndr_desc);
+ if (rc)
+ dev_warn(acpi_desc->dev,
+ "failed to insert pmem resource to iomem: %d\n",
+ rc);
+
rc = acpi_nfit_find_poison(acpi_desc, ndr_desc);
if (rc) {
dev_err(acpi_desc->dev,

2016-03-02 21:58:27

by Kani, Toshimitsu

[permalink] [raw]

Subject: [PATCH v2 2/4] resource: Add remove_resource interface

insert_resource() and insert_resource_conflict() are called
by resource producers to insert a new resource. When there
is any conflict, they move conflicting resources down to the
children of the new resource. There is no destructor of these
interfaces, however.

Add remove_resource(), which removes a resource previously
inserted by insert_resource() or insert_resource_conflict(),
and moves the children up to where they were before.

__release_resource() is changed to have @release_child, so
that this function can be used for remove_resource() as well.

Also add comments to clarify that these functions are intended
for producers of resources to avoid any confusion with
request/release_resource() for consumers.

Signed-off-by: Toshi Kani <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Dan Williams <[email protected]>
---
include/linux/ioport.h | 1 +
kernel/resource.c | 51 +++++++++++++++++++++++++++++++++++++++++++-----
2 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index afb4559..8017b8b 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -174,6 +174,7 @@ extern void reserve_region_with_split(struct resource *root,
extern struct resource *insert_resource_conflict(struct resource *parent, struct resource *new);
extern int insert_resource(struct resource *parent, struct resource *new);
extern void insert_resource_expand_to_fit(struct resource *root, struct resource *new);
+extern int remove_resource(struct resource *old);
extern void arch_remove_reservations(struct resource *avail);
extern int allocate_resource(struct resource *root, struct resource *new,
resource_size_t size, resource_size_t min,
diff --git a/kernel/resource.c b/kernel/resource.c
index 5a56e8f..effb6ee 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -233,9 +233,9 @@ static struct resource * __request_resource(struct resource *root, struct resour
}
}

-static int __release_resource(struct resource *old)
+static int __release_resource(struct resource *old, bool release_child)
{
- struct resource *tmp, **p;
+ struct resource *tmp, **p, *chd;

p = &old->parent->child;
for (;;) {
@@ -243,7 +243,17 @@ static int __release_resource(struct resource *old)
if (!tmp)
break;
if (tmp == old) {
- *p = tmp->sibling;
+ if (release_child || !(tmp->child)) {
+ *p = tmp->sibling;
+ } else {
+ for (chd = tmp->child;; chd = chd->sibling) {
+ chd->parent = tmp->parent;
+ if (!(chd->sibling))
+ break;
+ }
+ *p = tmp->child;
+ chd->sibling = tmp->sibling;
+ }
old->parent = NULL;
return 0;
}
@@ -325,7 +335,7 @@ int release_resource(struct resource *old)
int retval;

write_lock(&resource_lock);
- retval = __release_resource(old);
+ retval = __release_resource(old, true);
write_unlock(&resource_lock);
return retval;
}
@@ -679,7 +689,7 @@ static int reallocate_resource(struct resource *root, struct resource *old,
old->start = new.start;
old->end = new.end;
} else {
- __release_resource(old);
+ __release_resource(old, true);
*old = new;
conflict = __request_resource(root, old);
BUG_ON(conflict);
@@ -825,6 +835,9 @@ static struct resource * __insert_resource(struct resource *parent, struct resou
* entirely fit within the range of the new resource, then the new
* resource is inserted and the conflicting resources become children of
* the new resource.
+ *
+ * This function is intended for producers of resources, such as FW modules
+ * and bus drivers.
*/
struct resource *insert_resource_conflict(struct resource *parent, struct resource *new)
{
@@ -842,6 +855,9 @@ struct resource *insert_resource_conflict(struct resource *parent, struct resour
* @new: new resource to insert
*
* Returns 0 on success, -EBUSY if the resource can't be inserted.
+ *
+ * This function is intended for producers of resources, such as FW modules
+ * and bus drivers.
*/
int insert_resource(struct resource *parent, struct resource *new)
{
@@ -885,6 +901,31 @@ void insert_resource_expand_to_fit(struct resource *root, struct resource *new)
write_unlock(&resource_lock);
}

+/**
+ * remove_resource - Remove a resource in the resource tree
+ * @old: resource to remove
+ *
+ * Returns 0 on success, -EINVAL if the resource is not valid.
+ *
+ * This function removes a resource previously inserted by insert_resource()
+ * or insert_resource_conflict(), and moves the children (if any) up to
+ * where they were before. insert_resource() and insert_resource_conflict()
+ * insert a new resource, and move any conflicting resources down to the
+ * children of the new resource.
+ *
+ * insert_resource(), insert_resource_conflict() and remove_resource() are
+ * intended for producers of resources, such as FW modules and bus drivers.
+ */
+int remove_resource(struct resource *old)
+{
+ int retval;
+
+ write_lock(&resource_lock);
+ retval = __release_resource(old, false);
+ write_unlock(&resource_lock);
+ return retval;
+}
+
static int __adjust_resource(struct resource *res, resource_size_t start,
resource_size_t size)
{

2016-03-02 21:58:24

by Kani, Toshimitsu

[permalink] [raw]

Subject: [PATCH v2 3/4] resource: Add device-managed insert/remove_resource()

insert_resource() and remove_resouce() are called by producers
of resources, such as FW modules and bus drivers. These modules
may be implemented as loadable modules.

Add device-managed implementaions of insert_resource() and
remove_resouce() functions.

Signed-off-by: Toshi Kani <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Dan Williams <[email protected]>
---
include/linux/ioport.h | 5 +++
kernel/resource.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 74 insertions(+)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 8017b8b..3580038 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -259,6 +259,11 @@ extern struct resource * __devm_request_region(struct device *dev,

extern void __devm_release_region(struct device *dev, struct resource *parent,
resource_size_t start, resource_size_t n);
+
+extern int devm_insert_resource(struct device *dev, struct resource *root,
+ struct resource *new);
+extern void devm_remove_resource(struct device *dev, struct resource *old);
+
extern int iomem_map_sanity_check(resource_size_t addr, unsigned long size);
extern int iomem_is_exclusive(u64 addr);

diff --git a/kernel/resource.c b/kernel/resource.c
index effb6ee..b1a3394 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1449,6 +1449,75 @@ void __devm_release_region(struct device *dev, struct resource *parent,
EXPORT_SYMBOL(__devm_release_region);

/*
+ * Remove function for devm_insert_resource() and devm_remove_resource().
+ */
+static void devm_resource_remove(struct device *dev, void *ptr)
+{
+ struct resource **r = ptr;
+
+ remove_resource(*r);
+}
+
+/**
+ * devm_insert_resource() - insert an I/O or memory resource
+ * @dev: device for which to produce the resource
+ * @root: root of the resource tree
+ * @new: descriptor of the new resource
+ *
+ * This is a device-managed version of insert_resource(). There is usually
+ * no need to release resources requested by this function explicitly since
+ * that will be taken care of when the device is unbound from its bus driver.
+ * If for some reason the resource needs to be released explicitly, because
+ * of ordering issues for example, bus drivers must call devm_remove_resource()
+ * rather than the regular remove_resource().
+ *
+ * devm_insert_resource() is intended for producers of resources, such as
+ * FW modules and bus drivers.
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int devm_insert_resource(struct device *dev, struct resource *root,
+ struct resource *new)
+{
+ struct resource **ptr;
+ int ret;
+
+ ptr = devres_alloc(devm_resource_remove, sizeof(*ptr), GFP_KERNEL);
+ if (!ptr)
+ return -ENOMEM;
+
+ *ptr = new;
+
+ ret = insert_resource(root, new);
+ if (ret) {
+ dev_err(dev, "unable to insert resource: %pR (%d)\n", new, ret);
+ devres_free(ptr);
+ return -EBUSY;
+ }
+
+ devres_add(dev, ptr);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(devm_insert_resource);
+
+/**
+ * devm_remove_resource() - remove a previously inserted resource
+ * @dev: device for which to remove the resource
+ * @old: descriptor of the resource
+ *
+ * Remove a resource previously inserted using devm_insert_resource().
+ *
+ * devm_remove_resource() is intended for producers of resources, such as
+ * FW modules and bus drivers.
+ */
+void devm_remove_resource(struct device *dev, struct resource *old)
+{
+ WARN_ON(devres_release(dev, devm_resource_remove, devm_resource_match,
+ old));
+}
+EXPORT_SYMBOL_GPL(devm_remove_resource);
+
+/*
* Called from init/main.c to reserve IO ports.
*/
#define MAXRESERVE 4

2016-03-02 21:59:33

by Kani, Toshimitsu

[permalink] [raw]

Subject: [PATCH v2 1/4] resource: Change __request_region to inherit from immediate parent

__request_region() sets 'flags' of a new resource from @parent
as it inherits the parent's attribute. When a target resource
has a conflict, this function inserts the new resource entry
under the conflicted entry by updating @parent. In this case,
the new resource entry needs to inherit attribute from the updated
parent. This conflict is a typical case since __request_region()
is used to allocate a new resource from a specific resource range.

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 "DEV-A" under "BUS-1", "DEV-A" needs to inherit from the
immediate parent "BUS-1" as it holds specific attribute for the
range.

root (&iomem_resource)
:
+ "BUS-1"
+ "DEV-A"

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

Signed-off-by: Toshi Kani <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Dan Williams <[email protected]>
---
kernel/resource.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index 4d46605..5a56e8f 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;

2016-03-03 22:45:14

by Dan Williams

[permalink] [raw]

Subject: Re: [PATCH v2 3/4] resource: Add device-managed insert/remove_resource()

On Wed, Mar 2, 2016 at 2:50 PM, Toshi Kani <[email protected]> wrote:
> insert_resource() and remove_resouce() are called by producers
> of resources, such as FW modules and bus drivers. These modules
> may be implemented as loadable modules.
>
> Add device-managed implementaions of insert_resource() and
> remove_resouce() functions.
>
> Signed-off-by: Toshi Kani <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Dan Williams <[email protected]>
> ---
> include/linux/ioport.h | 5 +++
> kernel/resource.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 74 insertions(+)
>
> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> index 8017b8b..3580038 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -259,6 +259,11 @@ extern struct resource * __devm_request_region(struct device *dev,
>
> extern void __devm_release_region(struct device *dev, struct resource *parent,
> resource_size_t start, resource_size_t n);
> +
> +extern int devm_insert_resource(struct device *dev, struct resource *root,
> + struct resource *new);
> +extern void devm_remove_resource(struct device *dev, struct resource *old);
> +
> extern int iomem_map_sanity_check(resource_size_t addr, unsigned long size);
> extern int iomem_is_exclusive(u64 addr);
>
> diff --git a/kernel/resource.c b/kernel/resource.c
> index effb6ee..b1a3394 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -1449,6 +1449,75 @@ void __devm_release_region(struct device *dev, struct resource *parent,
> EXPORT_SYMBOL(__devm_release_region);
>
> /*
> + * Remove function for devm_insert_resource() and devm_remove_resource().
> + */
> +static void devm_resource_remove(struct device *dev, void *ptr)
> +{
> + struct resource **r = ptr;
> +
> + remove_resource(*r);
> +}
> +

Maybe call this __devm_remove_resource? I think this makes it clearer
that it is a private helper function. I was initially confused about
the difference between devm_resource_remove and devm_remove_resource.

2016-03-03 22:49:29

by Dan Williams

[permalink] [raw]

Subject: Re: [PATCH v2 4/4] ACPI: Change NFIT driver to insert new resource

On Wed, Mar 2, 2016 at 2:50 PM, Toshi Kani <[email protected]> wrote:
> ACPI 6.0 defines persistent memory (PMEM) ranges in multiple
> firmware interfaces, e820, EFI, and ACPI NFIT table. This EFI
> change, however, leads to hit a bug in the grub bootloader, which
> treats EFI_PERSISTENT_MEMORY type as regular memory and corrupts
> stored user data [1].
>
> Therefore, BIOS may set generic reserved type in e820 and EFI
> to cover PMEM ranges. The kernel can initialize PMEM ranges
> from ACPI NFIT table alone.
>
> This scheme causes a problem in the iomem table, though. On x86,
> for instance, e820_reserve_resources() initializes top-level entries
> (iomem_resource.child) from the e820 table at early boot-time.
> This creates "reserved" entry for a PMEM range, which does not allow
> region_intersects() to check with PMEM type.
>
> Change acpi_nfit_register_region() to call acpi_nfit_insert_resource(),
> which calls devm_insert_resource() to insert a PMEM entry from NFIT
> when the iomem table does not have a PMEM entry already. That is,
> when a PMEM range is marked as reserved type in e820, it inserts
> "Persistent Memory" entry, which results as follows.
>
> + "Persistent Memory"
> + "reserved"
>
> This allows the EINJ driver, which calls region_intersects() to
> check PMEM ranges, to work continuously even if BIOS sets reserved
> type (or sets nothing) to PMEM ranges in e820 and EFI.
>
> [1]: https://lists.gnu.org/archive/html/grub-devel/2015-11/msg00209.html
> Signed-off-by: Toshi Kani <[email protected]>
> Cc: Rafael J. Wysocki <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Andrew Morton <[email protected]>
> ---
> drivers/acpi/nfit.c | 30 ++++++++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
>
> diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> index fb53db1..d97b53f 100644
> --- a/drivers/acpi/nfit.c
> +++ b/drivers/acpi/nfit.c
> @@ -1571,6 +1571,30 @@ static int ars_status_process_records(struct nvdimm_bus *nvdimm_bus,
> return 0;
> }
>
> +static int acpi_nfit_insert_resource(struct acpi_nfit_desc *acpi_desc,
> + struct nd_region_desc *ndr_desc)
> +{
> + struct resource *res, *nd_res = ndr_desc->res;
> + size_t size = nd_res->end - nd_res->start + 1;
> +
> + /* No operation if the region is already registered as PMEM */
> + if (region_intersects(nd_res->start, size, IORESOURCE_MEM,
> + IORES_DESC_PERSISTENT_MEMORY) == REGION_INTERSECTS)
> + return 0;
> +
> + res = devm_kzalloc(acpi_desc->dev, sizeof(*res), GFP_KERNEL);

How about allocating this resource on the stack and then have
devm_insert_resource handle the dynamic allocation (memdup) so we have
one less failure point to handle in the driver.

2016-03-03 22:56:10

by Kani, Toshimitsu

[permalink] [raw]

Subject: Re: [PATCH v2 3/4] resource: Add device-managed insert/remove_resource()

On Thu, 2016-03-03 at 14:45 -0800, Dan Williams wrote:
> On Wed, Mar 2, 2016 at 2:50 PM, Toshi Kani <[email protected]> wrote:
:
> >
> > diff --git a/kernel/resource.c b/kernel/resource.c
> > index effb6ee..b1a3394 100644
> > --- a/kernel/resource.c
> > +++ b/kernel/resource.c
> > @@ -1449,6 +1449,75 @@ void __devm_release_region(struct device *dev,
> > struct resource *parent,
> > EXPORT_SYMBOL(__devm_release_region);
> >
> > /*
> > + * Remove function for devm_insert_resource() and
> > devm_remove_resource().
> > + */
> > +static void devm_resource_remove(struct device *dev, void *ptr)
> > +{
> > +       struct resource **r = ptr;
> > +
> > +       remove_resource(*r);
> > +}
> > +
>
> Maybe call this __devm_remove_resource?  I think this makes it clearer
> that it is a private helper function.  I was initially confused about
> the difference between devm_resource_remove and devm_remove_resource.

Will do. I followed the naming convention of devm_resource_release() in
the same file, but I agree that this is confusing.

Thanks,
-Toshi

2016-03-03 23:19:24

by Kani, Toshimitsu

[permalink] [raw]

Subject: Re: [PATCH v2 4/4] ACPI: Change NFIT driver to insert new resource

On Thu, 2016-03-03 at 14:49 -0800, Dan Williams wrote:
> On Wed, Mar 2, 2016 at 2:50 PM, Toshi Kani <[email protected]> wrote:
:
> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> > index fb53db1..d97b53f 100644
> > --- a/drivers/acpi/nfit.c
> > +++ b/drivers/acpi/nfit.c
> > @@ -1571,6 +1571,30 @@ static int ars_status_process_records(struct
> > nvdimm_bus *nvdimm_bus,
> >         return 0;
> > }
> >
> > +static int acpi_nfit_insert_resource(struct acpi_nfit_desc *acpi_desc,
> > +               struct nd_region_desc *ndr_desc)
> > +{
> > +       struct resource *res, *nd_res = ndr_desc->res;
> > +       size_t size = nd_res->end - nd_res->start + 1;
> > +
> > +       /* No operation if the region is already registered as PMEM */
> > +       if (region_intersects(nd_res->start, size, IORESOURCE_MEM,
> > +                       IORES_DESC_PERSISTENT_MEMORY) ==
> > REGION_INTERSECTS)
> > +               return 0;
> > +
> > +       res = devm_kzalloc(acpi_desc->dev, sizeof(*res), GFP_KERNEL);
>
> How about allocating this resource on the stack and then have
> devm_insert_resource handle the dynamic allocation (memdup) so we have
> one less failure point to handle in the driver.

I like the idea, but existing callers of insert_resource() allocate a
resource either statically or dynamically. It may be contained by other
structure as well. So, I think devm_insert_resource() should be consistent
with insert_resource() on this regard.

Thanks,
-Toshi