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

["Fabio M. De Francesco" <fabio.m.de.francesco@linux.intel.com>]

[-- 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 --]