[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aOcsGbRlMesYgAyV@aschofie-mobl2.lan>
Date: Wed, 8 Oct 2025 20:29:29 -0700
From: Alison Schofield <alison.schofield@...el.com>
To: "Fabio M. De Francesco" <fabio.m.de.francesco@...ux.intel.com>
CC: <linux-cxl@...r.kernel.org>, Davidlohr Bueso <dave@...olabs.net>,
"Jonathan Cameron" <jonathan.cameron@...wei.com>, Dave Jiang
<dave.jiang@...el.com>, Vishal Verma <vishal.l.verma@...el.com>, Ira Weiny
<ira.weiny@...el.com>, "Dan Williams" <dan.j.williams@...el.com>, Jonathan
Corbet <corbet@....net>, <linux-doc@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, Gregory Price <gourry@...rry.net>, Robert
Richter <rrichter@....com>, Cheatham Benjamin <benjamin.cheatham@....com>
Subject: Re: [PATCH 3/4 v5] cxl/core: Enable Region creation on x86 with LMH
On Mon, Oct 06, 2025 at 05:58:06PM +0200, Fabio M. De Francesco wrote:
> The CXL Fixed Memory Window Structure (CFMWS) describes zero or more
> Host Physical Address (HPA) windows that are associated with each CXL
> Host Bridge. Each window represents a contiguous HPA that may be
> interleaved with one or more targets (CXL v3.2 - 9.18.1.3).
>
> The Low Memory Hole (LMH) of x86 is a range of addresses of physical low
> memory to which systems cannot send transactions. In some case the size
> of that hole is not compatible with the constraint that the CFMWS size
> shall be multiple of Interleave Ways * 256 MB. (CXL v3.2 - Table 9-22).
>
> On those systems, the BIOS publishes CFMWS which communicate the active
> System Physical Address (SPA) ranges that map to a subset of the Host
> Physical Address (HPA) ranges. The SPA range trims out the hole, and the
> capacity in the endpoint is lost with no SPA to map to CXL HPA in that
> hole.
>
> In the early stages of CXL regions construction and attach on platforms
> that have Low Memory Holes, cxl_add_to_region() fails and returns an
> error for it can't find any CFMWS range that matches a given endpoint
> decoder.
>
> Detect an LMH by comparing root decoder and endpoint decoder range.
> Match root decoders HPA range and constructed region with the
> corresponding endpoint decoders. Construct CXL region with the end of
> its HPA ranges end adjusted to the matching SPA and adjust the DPA
> resource end of the hardware decoders to fit the region. Allow the
> attach target process to complete by allowing regions and decoders to
> bypass the constraints that don't hold when an LMH is present.[1]
>
> [1] commit 7a81173f3 ("cxl: Documentation/driver-api/cxl: Describe the x86 Low Memory Hole solution")
>
> 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/core/region.c | 47 ++++++++++++++++++++++++++++++++-------
> tools/testing/cxl/Kbuild | 1 +
> 2 files changed, 40 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 43a854036202..9a499bfca23d 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -14,6 +14,7 @@
> #include <linux/string_choices.h>
> #include <cxlmem.h>
> #include <cxl.h>
> +#include "platform_quirks.h"
> #include "core.h"
>
snip
> @@ -3479,6 +3498,12 @@ static int __construct_region(struct cxl_region *cxlr,
> *res = DEFINE_RES_MEM_NAMED(hpa->start, range_len(hpa),
> dev_name(&cxlr->dev));
>
> + /*
> + * Trim the HPA retrieved from hardware to fit the SPA mapped by the
> + * platform
> + */
> + platform_res_adjust(res, cxled, cxlrd);
> +
Noted this a bit in other patch, not so sure about that comment.
But anyway, do we really want to say what it is doing or let it be
a mystery of the quirks. I'm really not clear on where we are going
with these quirks and the naming of the helper functions.
If you split into 2 helpers, you can try something like:
*res = platform_adjust_region_resource(...);
And then later, do the endpoint adjust. See below:
> rc = cxl_extended_linear_cache_resize(cxlr, res);
> if (rc && rc != -EOPNOTSUPP) {
> /*
> @@ -3588,6 +3613,12 @@ int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
> cxl_find_region_by_range(cxlrd, cxled);
> if (!cxlr)
> cxlr = construct_region(cxlrd, cxled);
> + else
> + /*
> + * Adjust the Endpoint Decoder's dpa_res to fit the Region which
> + * it has to be attached to
> + */
> + platform_res_adjust(NULL, cxled, cxlrd);
Following from above, would it work to skip the else, and knowing
that the region resource was adjusted in construct_region(), only
do this here for every cxled that attaches.
cxled = platform_adjust_endpoint_resource(...)
snip to end.
Powered by blists - more mailing lists