Message-ID: <3089527.UnXabflUDm@fdefranc-mobl3>
Date: Sat, 29 Mar 2025 11:16:09 +0100
From: "Fabio M. De Francesco" <fabio.m.de.francesco@...ux.intel.com>
To: Dan Williams <dan.j.williams@...el.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>,
 Robert Richter <rrichter@....com>, ming.li@...omail.com,
 linux-kernel@...r.kernel.org, linux-cxl@...r.kernel.org
Subject:
 Re: [PATCH 4/4 v3] cxl/test: Simulate an x86 Low Memory Hole for tests

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@...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               | 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.
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists