Re: [PATCH 4/4 v3] cxl/test: Simulate an x86 Low Memory Hole for tests

Fabio M. De Francesco
2025-03-29T22:01:36Z

[-- Attachment #1: Type: text/plain, Size: 6027 bytes --]

On Saturday, March 29, 2025 11:16:09 AM Central European Standard Time Fabio M. De Francesco wrote:
> On Saturday, March 29, 2025 12:40:47 AM Central European Standard Time Dan Williams wrote:
> > Fabio M. De Francesco wrote:
> > > Simulate an x86 Low Memory Hole for the CXL tests by changing the first
> > > mock CFMWS range size to 768MB and the CXL Endpoint Decoder HPA range sizes
> > > to 1GB.
> > >
> > > Since the auto-created region of cxl-test uses mock_cfmws[0], whose range
> > > base address is typically different from the one published by the BIOS on
> > > real hardware, the driver would fail to create and attach CXL Regions if
> > > it was run on the mock environment created by cxl-tests.
> > >
> > > Therefore, save the mock_cfmsw[0] range base_hpa and reuse it to match CXL
> > > Root Decoders and Regions with Endpoint Decoders when the driver is run on
> > > mock devices.
> > >
> > > Since the auto-created region of cxl-test uses mock_cfmws[0], the
> > > LMH path in the CXL Driver will be exercised every time the cxl-test
> > > module is loaded. Executing unit test: cxl-topology.sh, confirms the
> > > region created successfully with a LMH.
> > >
> > > Cc: Alison Schofield <alison.schofield@intel.com>
> > > Cc: Dan Williams <dan.j.williams@intel.com>
> > > Cc: Ira Weiny <ira.weiny@intel.com>
> > > Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
> > > ---
> > > drivers/cxl/core/lmh.c | 35 ++++++++++++++++++++++++----
> > > drivers/cxl/core/lmh.h | 2 ++
> > > tools/testing/cxl/cxl_core_exports.c | 2 ++
> > > tools/testing/cxl/test/cxl.c | 10 ++++++++
> > > 4 files changed, 45 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/cxl/core/lmh.c b/drivers/cxl/core/lmh.c
> > > index 2e32f867eb94..9c55670c1c84 100644
> > > --- a/drivers/cxl/core/lmh.c
> > > +++ b/drivers/cxl/core/lmh.c
> > > @@ -1,11 +1,28 @@
> > > // SPDX-License-Identifier: GPL-2.0-only
> > >
> > > #include <linux/range.h>
> > > +#include <linux/pci.h>
> > > +
> > > #include "lmh.h"
> > >
> > > /* Start of CFMWS range that end before x86 Low Memory Holes */
> > > #define LMH_CFMWS_RANGE_START 0x0ULL
> > >
> > > +static u64 mock_cfmws0_range_start = ULLONG_MAX;
> > > +
> > > +void set_mock_cfmws0_range_start(const u64 start)
> > > +{
> > > + mock_cfmws0_range_start = start;
> > > +}
> > > +
> > > +static u64 get_cfmws_range_start(const struct device *dev)
> > > +{
> > > + if (dev_is_pci(dev))
> > > + return LMH_CFMWS_RANGE_START;
> > > +
> > > + return mock_cfmws0_range_start;
> > > +}
> > > +
> >
> > cxl_test should never result in "mock" infrastructure appearing outside
> > of tools/testing/cxl/
> >
> > > /*
> > > * Match CXL Root and Endpoint Decoders by comparing SPA and HPA ranges.
> > > *
> > > @@ -19,14 +36,19 @@ bool arch_match_spa(const struct cxl_root_decoder *cxlrd,
> > > const struct cxl_endpoint_decoder *cxled)
> > > {
> > > const struct range *r1, *r2;
> > > + u64 cfmws_range_start;
> > > int niw;
> > >
> > > + cfmws_range_start = get_cfmws_range_start(&cxled->cxld.dev);
> > > + if (cfmws_range_start == ULLONG_MAX)
> > > + return false;
> > > +
> > > 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 &&
> > > + if (r1->start == cfmws_range_start && r1->start == r2->start &&
> > > + r1->end < (cfmws_range_start + SZ_4G) && r1->end < r2->end &&
> > > IS_ALIGNED(range_len(r2), niw * SZ_256M))
> > > return true;
> > >
> > > @@ -40,9 +62,14 @@ bool arch_match_region(const struct cxl_region_params *p,
> > > const struct range *r = &cxld->hpa_range;
> > > const struct resource *res = p->res;
> > > int niw = cxld->interleave_ways;
> > > + u64 cfmws_range_start;
> > > +
> > > + cfmws_range_start = get_cfmws_range_start(&cxld->dev);
> > > + if (cfmws_range_start == ULLONG_MAX)
> > > + return false;
> > >
> > > - if (res->start == LMH_CFMWS_RANGE_START && res->start == r->start &&
> > > - res->end < (LMH_CFMWS_RANGE_START + SZ_4G) && res->end < r->end &&
> > > + if (res->start == cfmws_range_start && res->start == r->start &&
> > > + res->end < (cfmws_range_start + SZ_4G) && res->end < r->end &&
> > > IS_ALIGNED(range_len(r), niw * SZ_256M))
> > > return true;
> >
> > Someone should be able to read the straight line CXL driver code and
> > never know that an alternate implementation exists for changing these
> > details.
> >
> > So, the mock interface for this stuff should intercept at the
> > arch_match_spa() and arch_match_region() level.
> >
> > To me that looks like mark these implementations with the __mock
> > attribute, similar to to_cxl_host_bridge(). Then define strong versions
> > in tools/testing/cxl/mock_lmh.c.
> >
> > The strong versions would apply memory hole semantics to both windows
> > starting at zero and whatever cxl_test window you choose.
> >
> I thought the same and wanted to use the strong/weak mechanism, but then
> I noticed that the strong version (in tools/testing/cxl/mock_lmh.c) was never
> called. I think it never happens because of the weak version is called from
> cxl_core. I think that all functions called from cxl_core can't be override
> from cxl_test.
>
> Is that deduction unfounded? Am I missing something?
>
> Thanks,
>
> Fabio
>
> P.S.: Please notice that to_cxl_host_bridge() is never used in cxl_core.
>
I mistakenly thought you were suggesting something like the wrap approach
that is not possible if the caller of the wrapped function is internal to
the CXL core.[1]

Fabio

[1] https://lore.kernel.org/all/6711b7c0c0b53_3ee2294a6@dwillia2-xfh.jf.intel.com.notmuch/

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]