| 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
| ||
|
Message-ID: <7c11a5d6-0d4e-484f-827c-a93d0b8b0fb4@intel.com>
Date: Fri, 5 Sep 2025 15:13:06 -0700
From: Dave Jiang <dave.jiang@...el.com>
To: "Fabio M. De Francesco" <fabio.m.de.francesco@...ux.intel.com>,
linux-cxl@...r.kernel.org
Cc: Davidlohr Bueso <dave@...olabs.net>,
Jonathan Cameron <jonathan.cameron@...wei.com>,
Alison Schofield <alison.schofield@...el.com>,
Vishal Verma <vishal.l.verma@...el.com>, Ira Weiny <ira.weiny@...el.com>,
Dan Williams <dan.j.williams@...el.com>, Robert Richter <rrichter@....com>,
ming.li@...omail.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4 v4] cxl/core: Add helpers to detect Low Memory Holes
on x86
On 7/24/25 7:20 AM, Fabio M. De Francesco wrote:
s/Holes/Hole/ for subject. Only 1 right?
> In x86 with Low memory Hole, the BIOS may publishes CFMWS that describe
> SPA ranges which are subsets of the corresponding CXL Endpoint Decoders> HPA's because the CFMWS never intersects LMH's while EP Decoders HPA's
> ranges are always guaranteed to align to the NIW * 256M rule.
On a x86 platform with a low memory hole (LHM), the BIOS may publish CFMWS that describes
SPA ranges. The SPA ranges are subsets of the corresponding CXL endpoint decoder's
HPA ranges because the CFMWS never intersects the LHM while the endpoint decoder's HPA
ranges are always guaranteed to align to the "NIW * 256M" rule.
>
> In order to construct Regions and attach Decoders, the driver needs to
> match Root Decoders and Regions with Endpoint Decoders, but it fails and> the entire process returns errors because it doesn't expect to deal with
> SPA range lengths smaller than corresponding HPA's.
To construct regions and attach decoders, the driver needs to match root
decoders and regions with endpoint decoders. The process fails and returns
errors because it isn't expected to deal with SPA range smaller than the
corresponding HPA range.
>
> Introduce functions that indirectly detect x86 LMH's by comparing SPA's
s/LMH's/LMH/
s/SPA/SPA range/
> with corresponding HPA's. They will be used in the process of Regions
s/HPA's/HPA range/
s/Regions/region/
> creation and Endpoint attachments to prevent driver failures in a few
s/Endpoint/endpoints/
> steps of the above-mentioned process.
s/above-mentioned process/the process mentioned above/
>
> The helpers return true when HPA/SPA misalignments are detected under
> specific conditions: both the SPA and HPA ranges must start at
> LMH_CFMWS_RANGE_START (that in x86 with LMH's is 0x0), SPA range sizes
> be less than HPA's, SPA's range's size be less than 4G, HPA's size be
> aligned to the NIW * 256M rule.
I would make it clearer to read by listing them:
1. ....
2. ....
>
> Also introduce a function to adjust the range end of the Regions to be
s/Regions/region/ singular right?
> created on x86 with LMH's.
s/LMH's/LMH/
>
> Cc: Alison Schofield <alison.schofield@...el.com>
> Cc: Dan Williams <dan.j.williams@...el.com>
> Cc: Dave Jiang <dave.jiang@...el.com>
> Cc: Ira Weiny <ira.weiny@...el.com>
> Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@...ux.intel.com>
> ---
> drivers/cxl/Kconfig | 5 +++
> drivers/cxl/core/Makefile | 1 +
> drivers/cxl/core/platform.c | 85 +++++++++++++++++++++++++++++++++++++
> drivers/cxl/core/platform.h | 32 ++++++++++++++
> 4 files changed, 123 insertions(+)
> create mode 100644 drivers/cxl/core/platform.c
> create mode 100644 drivers/cxl/core/platform.h
>
> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> index 48b7314afdb8..eca90baeac10 100644
> --- a/drivers/cxl/Kconfig
> +++ b/drivers/cxl/Kconfig
> @@ -211,6 +211,11 @@ config CXL_REGION
>
> If unsure say 'y'
>
> +config CXL_PLATFORM_QUIRKS
> + def_bool y
> + depends on CXL_REGION
> + depends on X86
> +
> config CXL_REGION_INVALIDATION_TEST
> bool "CXL: Region Cache Management Bypass (TEST)"
> depends on CXL_REGION
> diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
> index 79e2ef81fde8..4be729fb7d64 100644
> --- a/drivers/cxl/core/Makefile
> +++ b/drivers/cxl/core/Makefile
> @@ -18,6 +18,7 @@ cxl_core-y += ras.o
> cxl_core-y += acpi.o
> cxl_core-$(CONFIG_TRACING) += trace.o
> cxl_core-$(CONFIG_CXL_REGION) += region.o
> +cxl_core-$(CONFIG_CXL_PLATFORM_QUIRKS) += platform.o
Given that you are creating a quirk, make the object obvious it's a quirk.
platform_quirks.o
> cxl_core-$(CONFIG_CXL_MCE) += mce.o
> cxl_core-$(CONFIG_CXL_FEATURES) += features.o
> cxl_core-$(CONFIG_CXL_EDAC_MEM_FEATURES) += edac.o
> diff --git a/drivers/cxl/core/platform.c b/drivers/cxl/core/platform.c
> new file mode 100644
> index 000000000000..8202750742d0
> --- /dev/null
> +++ b/drivers/cxl/core/platform.c
> @@ -0,0 +1,85 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/range.h>
> +#include "platform.h"
> +#include "cxlmem.h"
> +#include "core.h"
> +
> +/* Start of CFMWS range that end before x86 Low Memory Holes */
> +#define LMH_CFMWS_RANGE_START 0x0ULL
> +
> +/*
> + * Match CXL Root and Endpoint Decoders by comparing SPA and HPA ranges.
> + *
> + * On x86, CFMWS ranges never intersect memory holes while endpoint decoders
> + * HPA range sizes are always guaranteed aligned to NIW * 256MB; therefore,
> + * the given endpoint decoder HPA range size is always expected aligned and
> + * also larger than that of the matching root decoder. If there are LMH's,
> + * the root decoder range end is always less than SZ_4G.
> + */
> +bool platform_root_decoder_contains(const struct cxl_root_decoder *cxlrd,
> + const struct cxl_endpoint_decoder *cxled)
> +{
> + const struct range *r1, *r2;
> + int niw;
> +
> + r1 = &cxlrd->cxlsd.cxld.hpa_range;
> + r2 = &cxled->cxld.hpa_range;
I would name it rd_r and ed_r so it's easier to remember which is which below when you do compare below.
> + niw = cxled->cxld.interleave_ways;
Just make this 'align', and you can just do:
align = cxled->cxld.interleave_ways * SZ_256M;
That way it's cleaner below when you do compare.
> +
> + if (r1->start == LMH_CFMWS_RANGE_START && r1->start == r2->start &&
May as well make it 'r2->start == LMH_CFMWS_RANGE_START'?
> + r1->end < (LMH_CFMWS_RANGE_START + SZ_4G) && r1->end < r2->end &&
> + IS_ALIGNED(range_len(r2), niw * SZ_256M))
> + return true;
Maybe have 1 conditional per line to make it easier to read since this is a big one
> +
> + return false;
> +}
> +
> +/*
> + * Similar to platform_root_decoder_contains(), it matches regions and
> + * decoders
> + */
> +bool platform_region_contains(const struct cxl_region_params *p,
> + const struct cxl_decoder *cxld)
> +{
> + const struct range *r = &cxld->hpa_range;
> + const struct resource *res = p->res;
> + int niw = cxld->interleave_ways;
> +
> + if (res->start == LMH_CFMWS_RANGE_START && res->start == r->start &&
> + res->end < (LMH_CFMWS_RANGE_START + SZ_4G) && res->end < r->end &&
> + IS_ALIGNED(range_len(r), niw * SZ_256M))
> + return true;
similar comments as the previous function
> +
> + return false;
> +}
> +
> +void platform_res_adjust(struct resource *res,
> + struct cxl_endpoint_decoder *cxled,
> + const struct cxl_root_decoder *cxlrd)
> +{
> + if (!platform_root_decoder_contains(cxlrd, cxled))
> + return;
> +
> + guard(rwsem_write)(&cxl_dpa_rwsem);
> + dev_info(cxled_to_memdev(cxled)->dev.parent,
> + "(LMH) Resources were (%s: %pr, %pr)\n",
> + dev_name(&cxled->cxld.dev), res, cxled->dpa_res);
> + if (res) {
Do you ever expect a scenario where the 'res' passed in is NULL?
> + /*
> + * A region must be constructed with Endpoint Decoder's
> + * HPA range end adjusted to Root Decoder's resource end
> + */
> + res->end = cxlrd->res->end;
> + }
> + /*
> + * The Endpoint Decoder's dpa_res->end must be adjusted with Root
> + * Decoder's resource end
> + */
> + cxled->dpa_res->end =
> + cxled->dpa_res->start +
> + resource_size(cxlrd->res) / cxled->cxld.interleave_ways - 1;
> + dev_info(cxled_to_memdev(cxled)->dev.parent,
> + "(LMH) Resources have been adjusted (%s: %pr, %pr)\n",
> + dev_name(&cxled->cxld.dev), res, cxled->dpa_res);
If the res passed in is ever NULL, this line would crash when accessing res here.
> +}
> diff --git a/drivers/cxl/core/platform.h b/drivers/cxl/core/platform.h
> new file mode 100644
> index 000000000000..0baa39938729
> --- /dev/null
> +++ b/drivers/cxl/core/platform.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include "cxl.h"
> +
> +#ifdef CONFIG_CXL_PLATFORM_QUIRKS
> +bool platform_root_decoder_contains(const struct cxl_root_decoder *cxlrd,
> + const struct cxl_endpoint_decoder *cxled);
> +bool platform_region_contains(const struct cxl_region_params *p,
> + const struct cxl_decoder *cxld);
> +void platform_res_adjust(struct resource *res,
> + struct cxl_endpoint_decoder *cxled,
> + const struct cxl_root_decoder *cxlrd);
> +#else
> +static bool
> +platform_root_decoder_contains(const struct cxl_root_decoder *cxlrd,
> + const struct cxl_endpoint_decoder *cxled)
> +{
> + return false;
> +}
> +
> +static bool platform_region_contains(const struct cxl_region_params *p,
> + const struct cxl_decoder *cxld)
> +{
> + return false;
> +}
> +
> +void platform_res_adjust(struct resource *res,
> + struct cxl_endpoint_decoder *cxled,
> + const struct cxl_root_decoder *cxlrd)
> +{
> +}
these need inline
> +#endif /* CONFIG_CXL_PLATFORM_QUIRKS */
Powered by blists - more mailing lists