linux-kernel - Re: [PATCH v2 6/6] ACPI: HMAT: Attach a device for each soft-reserved range

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 24 Mar 2020 19:41:02 +0000
From:   Joao Martins <joao.m.martins@...cle.com>
To:     Dan Williams <dan.j.williams@...el.com>
Cc:     linux-acpi@...r.kernel.org,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Brice Goglin <Brice.Goglin@...ia.fr>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>, peterz@...radead.org,
        dave.hansen@...ux.intel.com, linux-nvdimm@...ts.01.org,
        linux-kernel@...r.kernel.org, x86@...nel.org
Subject: Re: [PATCH v2 6/6] ACPI: HMAT: Attach a device for each soft-reserved
 range

On 3/22/20 4:12 PM, Dan Williams wrote:
> The hmem enabling in commit 'cf8741ac57ed ("ACPI: NUMA: HMAT: Register
> "soft reserved" memory as an "hmem" device")' only registered ranges to
> the hmem driver for each soft-reservation that also appeared in the
> HMAT. While this is meant to encourage platform firmware to "do the
> right thing" and publish an HMAT, the corollary is that platforms that
> fail to publish an accurate HMAT will strand memory from Linux usage.
> Additionally, the "efi_fake_mem" kernel command line option enabling
> will strand memory by default without an HMAT.
> 
> Arrange for "soft reserved" memory that goes unclaimed by HMAT entries
> to be published as raw resource ranges for the hmem driver to consume.
> 
> Include a module parameter to disable either this fallback behavior, or
> the hmat enabling from creating hmem devices. The module parameter
> requires the hmem device enabling to have unique name in the module
> namespace: "device_hmem".
> 
> Rather than mark this x86-only, include an interim phys_to_target_node()
> implementation for arm64.
> 
> Cc: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> Cc: Brice Goglin <Brice.Goglin@...ia.fr>
> Cc: Ard Biesheuvel <ard.biesheuvel@...aro.org>
> Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>
> Cc: Jeff Moyer <jmoyer@...hat.com>
> Cc: Catalin Marinas <catalin.marinas@....com>
> Cc: Will Deacon <will@...nel.org>
> Signed-off-by: Dan Williams <dan.j.williams@...el.com>
> ---
>  arch/arm64/mm/numa.c      |   13 +++++++++++++
>  drivers/dax/Kconfig       |    1 +
>  drivers/dax/hmem/Makefile |    3 ++-
>  drivers/dax/hmem/device.c |   33 +++++++++++++++++++++++++++++++++
>  4 files changed, 49 insertions(+), 1 deletion(-)
> 

[...]

> diff --git a/drivers/dax/hmem/device.c b/drivers/dax/hmem/device.c
> index 99bc15a8b031..f9c5fa8b1880 100644
> --- a/drivers/dax/hmem/device.c
> +++ b/drivers/dax/hmem/device.c
> @@ -4,6 +4,9 @@
>  #include <linux/module.h>
>  #include <linux/mm.h>
>  
> +static bool nohmem;
> +module_param_named(disable, nohmem, bool, 0444);
> +
>  void hmem_register_device(int target_nid, struct resource *r)
>  {
>  	/* define a clean / non-busy resource for the platform device */
> @@ -16,6 +19,9 @@ void hmem_register_device(int target_nid, struct resource *r)
>  	struct memregion_info info;
>  	int rc, id;
>  
> +	if (nohmem)
> +		return;
> +
>  	rc = region_intersects(res.start, resource_size(&res), IORESOURCE_MEM,
>  			IORES_DESC_SOFT_RESERVED);
>  	if (rc != REGION_INTERSECTS)
> @@ -62,3 +68,30 @@ void hmem_register_device(int target_nid, struct resource *r)
>  out_pdev:
>  	memregion_free(id);
>  }
> +
> +static __init int hmem_register_one(struct resource *res, void *data)
> +{
> +	/*
> +	 * If the resource is not a top-level resource it was already
> +	 * assigned to a device by the HMAT parsing.
> +	 */
> +	if (res->parent != &iomem_resource)
> +		return 0;
> +
> +	hmem_register_device(phys_to_target_node(res->start), res);
> +
> +	return 0;

Should we add an error returning value to hmem_register_device() perhaps this
ought to be reflected in hmem_register_one().

> +}
> +
> +static __init int hmem_init(void)
> +{
> +	walk_iomem_res_desc(IORES_DESC_SOFT_RESERVED,
> +			IORESOURCE_MEM, 0, -1, NULL, hmem_register_one);
> +	return 0;
> +}
> +

(...) and then perhaps here returning in the initcall if any of the resources
failed hmem registration?

> +/*
> + * As this is a fallback for address ranges unclaimed by the ACPI HMAT
> + * parsing it must be at an initcall level greater than hmat_init().
> + */
> +late_initcall(hmem_init);

Regardless of the nit (which ties in to patch 4), looks good. So, FWIW:

  Reviewed-by: Joao Martins <joao.m.martins@...cle.com>

For the hmem changes.

	Joao