Re: [PATCH 3/3] acpi,srat: reduce memory block size if CFMWS has a smaller alignment

[Ira Weiny <ira.weiny@intel.com>]

Gregory Price wrote:
> The CXL Fixed Memory Window allows for memory aligned down to the
> size of 256MB.  However, by default on x86, memory blocks increase
> in size as total System RAM capacity increases. On x86, this caps
> out at 2G when 64GB of System RAM is reached.
> 
> When the CFMWS regions are not aligned to memory block size, this
> results in lost capacity on either side of the alignment.
> 
> Parse all CFMWS to detect the largest common denomenator among all
> regions, and reduce the block size accordingly.
> 
> This can only be done when MEMORY_HOTPLUG and SPARSEMEM configs are
> enabled, but the surrounding code may not necessarily require these
> configs, so build accordingly.
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Gregory Price <gourry@gourry.net>
> ---
>  drivers/acpi/numa/srat.c | 48 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c
> index 44f91f2c6c5d..9367d36eba9a 100644
> --- a/drivers/acpi/numa/srat.c
> +++ b/drivers/acpi/numa/srat.c
> @@ -14,6 +14,7 @@
>  #include <linux/errno.h>
>  #include <linux/acpi.h>
>  #include <linux/memblock.h>
> +#include <linux/memory.h>
>  #include <linux/numa.h>
>  #include <linux/nodemask.h>
>  #include <linux/topology.h>
> @@ -333,6 +334,37 @@ acpi_parse_memory_affinity(union acpi_subtable_headers *header,
>  	return 0;
>  }
>  
> +#if defined(CONFIG_MEMORY_HOTPLUG)

Generally we avoid config defines in *.c files...  See more below.

> +/*
> + * CXL allows CFMW to be aligned along 256MB boundaries, but large memory
> + * systems default to larger alignments (2GB on x86). Misalignments can
> + * cause some capacity to become unreachable. Calculate the largest supported
> + * alignment for all CFMW to maximize the amount of mappable capacity.
> + */
> +static int __init acpi_align_cfmws(union acpi_subtable_headers *header,
> +				   void *arg, const unsigned long table_end)
> +{
> +	struct acpi_cedt_cfmws *cfmws = (struct acpi_cedt_cfmws *)header;
> +	u64 start = cfmws->base_hpa;
> +	u64 size = cfmws->window_size;
> +	unsigned long *fin_bz = arg;
> +	unsigned long bz;
> +
> +	for (bz = SZ_64T; bz >= SZ_256M; bz >>= 1) {
> +		if (IS_ALIGNED(start, bz) && IS_ALIGNED(size, bz))
> +			break;
> +	}
> +
> +	/* Only adjust downward, we never want to increase block size */
> +	if (bz < *fin_bz && bz >= SZ_256M)
> +		*fin_bz = bz;
> +	else if (bz < SZ_256M)
> +		pr_err("CFMWS: [BIOS BUG] base/size alignment violates spec\n");
> +
> +	return 0;
> +}
> +#endif /* defined(CONFIG_MEMORY_HOTPLUG) */
> +
>  static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
>  				   void *arg, const unsigned long table_end)
>  {
> @@ -501,6 +533,10 @@ acpi_table_parse_srat(enum acpi_srat_type id,
>  int __init acpi_numa_init(void)
>  {
>  	int i, fake_pxm, cnt = 0;
> +#if defined(CONFIG_MEMORY_HOTPLUG)
> +	unsigned long block_sz = memory_block_size_bytes();

To help address David's comment as well;

Is there a way to scan all the alignments of the windows and pass the
desired alignment to the arch in a new call and have the arch determine if
changing the order is ok?

Also the call to the arch would be a noop for !CONFIG_MEMORY_HOTPLUG which
cleans up this function WRT CONFIG_MEMORY_HOTPLUG.

Ira

> +	unsigned long cfmw_align = block_sz;
> +#endif /* defined(CONFIG_MEMORY_HOTPLUG) */
>  
>  	if (acpi_disabled)
>  		return -EINVAL;
> @@ -552,6 +588,18 @@ int __init acpi_numa_init(void)
>  	}
>  	last_real_pxm = fake_pxm;
>  	fake_pxm++;
> +
> +#if defined(CONFIG_MEMORY_HOTPLUG)
> +	/* Calculate and set largest supported memory block size alignment */
> +	acpi_table_parse_cedt(ACPI_CEDT_TYPE_CFMWS, acpi_align_cfmws,
> +			      &cfmw_align);
> +	if (cfmw_align < block_sz && cfmw_align >= SZ_256M) {
> +		if (set_memory_block_size_order(ffs(cfmw_align)-1))
> +			pr_warn("CFMWS: Unable to adjust memory block size\n");
> +	}
> +#endif /* defined(CONFIG_MEMORY_HOTPLUG) */
> +
> +	/* Then parse and fill the numa nodes with the described memory */
>  	acpi_table_parse_cedt(ACPI_CEDT_TYPE_CFMWS, acpi_parse_cfmws,
>  			      &fake_pxm);
>  
> -- 
> 2.43.0
> 
>