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

[Dan Williams]

On Wed, Mar 2, 2016 at 2:50 PM, Toshi Kani <toshi.kani@xxxxxxx> 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@xxxxxxx>
> Cc: Rafael J. Wysocki <rjw@xxxxxxxxxxxxx>
> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Borislav Petkov <bp@xxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> ---
>  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.