| Message ID | 1456959056-12316-5-git-send-email-toshi.kani@hpe.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Wed, Mar 2, 2016 at 2:50 PM, Toshi Kani <toshi.kani@hpe.com> 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 <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 | 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. -- 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
On Thu, 2016-03-03 at 14:49 -0800, Dan Williams wrote: > On Wed, Mar 2, 2016 at 2:50 PM, Toshi Kani <toshi.kani@hpe.com> 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 -- 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
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,
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 <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 | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) -- 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