| 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: <Z909uPbvRVlZ_J1Z@rric.localdomain>
Date: Fri, 21 Mar 2025 11:21:44 +0100
From: Robert Richter <rrichter@....com>
To: "Fabio M. De Francesco" <fabio.m.de.francesco@...ux.intel.com>
Cc: Davidlohr Bueso <dave@...olabs.net>,
Jonathan Cameron <jonathan.cameron@...wei.com>,
Dave Jiang <dave.jiang@...el.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>, ming.li@...omail.com,
linux-kernel@...r.kernel.org, linux-cxl@...r.kernel.org
Subject: Re: [PATCH 2/4 v3] cxl/core: Add helpers to detect Low memory Holes
on x86
On 14.03.25 12:36:31, Fabio M. De Francesco wrote:
> 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.
>
> 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.
>
> Introduce functions that indirectly detect x86 LMH's by comparing SPA's
> with corresponding HPA's. They will be used in the process of Regions
> creation and Endpoint attachments to prevent driver failures in a few
> steps of the above-mentioned process.
>
> 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
"... that for Intel with LMHs is 0x0", not true for AMD.
> be less than HPA's, SPA's range's size be less than 4G, HPA's size be
> aligned to the NIW * 256M rule.
>
> Also introduce a function to adjust the range end of the Regions to be
> created on x86 with LMH's.
>
> Cc: Alison Schofield <alison.schofield@...el.com>
> Cc: Dan Williams <dan.j.williams@...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/core/lmh.c | 56 ++++++++++++++++++++++++++++++++++++++++++
> drivers/cxl/core/lmh.h | 29 ++++++++++++++++++++++
The split of the code in patch #2 and #3 does not make sense. The
"interface" you introduce here is out of context which is patch #3.
And in patch #3 the functions are actually used, so you need to switch
back to this one. Other than that, the code is not enabled here at
all, it is even not built.
> 2 files changed, 85 insertions(+)
> create mode 100644 drivers/cxl/core/lmh.c
> create mode 100644 drivers/cxl/core/lmh.h
>
> diff --git a/drivers/cxl/core/lmh.c b/drivers/cxl/core/lmh.c
> new file mode 100644
> index 000000000000..2e32f867eb94
> --- /dev/null
> +++ b/drivers/cxl/core/lmh.c
> @@ -0,0 +1,56 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/range.h>
> +#include "lmh.h"
> +
> +/* Start of CFMWS range that end before x86 Low Memory Holes */
> +#define LMH_CFMWS_RANGE_START 0x0ULL
This looks arbitrary. An endpoint decoder's zero base address could
have other reasons too, e.g. the need of address translation. So you
need a different check for the mem hole.
In my previous review comment I have requested a platform check for
this code to enable.
> +
> +/*
> + * 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 arch_match_spa(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;
> + niw = cxled->cxld.interleave_ways;
> +
> + if (r1->start == LMH_CFMWS_RANGE_START && r1->start == r2->start &&
> + r1->end < (LMH_CFMWS_RANGE_START + SZ_4G) && r1->end < r2->end &&
How about removing LMH_CFMWS_RANGE_START at all? It is zero anyway and
would make this check much easier.
Can this being modified to reuse this codes for "holes" other than
below 4G?
> + IS_ALIGNED(range_len(r2), niw * SZ_256M))
> + return true;
> +
> + return false;
> +}
> +
> +/* Similar to arch_match_spa(), it matches regions and decoders */
> +bool arch_match_region(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 &&
Same here.
> + IS_ALIGNED(range_len(r), niw * SZ_256M))
> + return true;
> +
> + return false;
> +}
Right now the default check is:
(p->res->start == r->start && p->res->end == r->end)
Can't we just calculate a matching spa range for the decoder and
region and then check if both match? Both match functions would be
obsolete then.
> +
> +void arch_adjust_region_resource(struct resource *res,
> + struct cxl_root_decoder *cxlrd)
> +{
> + res->end = cxlrd->res->end;
> +}
This should be squashed with arch_match_spa(): same style and
interface as for cxl_extended_linear_cache_resize(). Please generalize
the interface of cxl_extended_linear_cache_resize() first.
> diff --git a/drivers/cxl/core/lmh.h b/drivers/cxl/core/lmh.h
> new file mode 100644
> index 000000000000..16746ceac1ed
> --- /dev/null
> +++ b/drivers/cxl/core/lmh.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include "cxl.h"
> +
> +#ifdef CONFIG_CXL_ARCH_LOW_MEMORY_HOLE
> +bool arch_match_spa(const struct cxl_root_decoder *cxlrd,
> + const struct cxl_endpoint_decoder *cxled);
> +bool arch_match_region(const struct cxl_region_params *p,
> + const struct cxl_decoder *cxld);
> +void arch_adjust_region_resource(struct resource *res,
> + struct cxl_root_decoder *cxlrd);
> +#else
> +static bool arch_match_spa(struct cxl_root_decoder *cxlrd,
> + struct cxl_endpoint_decoder *cxled)
> +{
> + return false;
> +}
> +
> +static bool arch_match_region(struct cxl_region_params *p,
> + struct cxl_decoder *cxld)
> +{
> + return false;
> +}
> +
> +static void arch_adjust_region_resource(struct resource *res,
> + struct cxl_root_decoder *cxlrd)
> +{
> +}
> +#endif /* CXL_ARCH_LOW_MEMORY_HOLE */
I don't think we will need all this helpers if there are platform
specific callbacks as suggested in a comment to v1.
-Robert
> --
> 2.48.1
>
Powered by blists - more mailing lists