Message-ID: <Z-Zlrm8emmOtQjhu@rric.localdomain>
Date: Fri, 28 Mar 2025 10:02:38 +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 0/4 v3] cxl/core: Enable Region creation on x86 with Low
 Mem Hole

On 25.03.25 17:13:50, Fabio M. De Francesco wrote:
> On Friday, March 21, 2025 11:34:35 AM Central European Standard Time Robert Richter wrote:
> > Fabio,
> > 
> > On 14.03.25 12:36:29, 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.1 - 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. On those systems, 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 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
> > > with Low Memory Holes, the driver fails and returns an error because it
> > > expects that the CXL Endpoint Decoder range is a subset of the Root
> > > Decoder's (SPA >= HPA). On x86 with LMH's, it happens that SPA < HPA.
> > > 
> > > Therefore, detect x86 Low Memory Holes, match CXL Root and Endpoint
> > > Decoders or already made CXL Regions and Decoders to allow the
> > > construction of new CXL Regions and the attachment of Endpoint Decoders,
> > > even if SPA < HPA. If needed because of LMH's, adjust the Endpoint Decoder
> > > range end to match Root Decoder's.
> > > 
> > > - Patch 1/4 changes the calling conventions of three match_*_by_range()
> > >   helpers in preparation of 3/4.
> > > - Patch 2/4 Introduces helpers to detect LMH's and also one to adjust
> > >   the HPA range end for CXL Regions construction.
> > > - Patch 3/4 enables CXL Regions construction and Endpoint Decoders
> > >   attachment by matching Root Decoders or Regions with Endpoint
> > >   Decoders, adjusting Endpoint Decoders HPA range end, and relaxing
> > >   constraints while Endpoints decoders' attachment.
> > > - Patch 4/4 simulates a LMH for the CXL tests on patched CXL driver.
> > > 
> > > Many thanks to Alison, Dan, and Ira for their help and for their reviews
> > > of my RFC on Intel's internal ML.
> > > 
> > > Commenting on v1, Alison wrote a couple of observations on what users
> > > will see. I suggest anyone interested to see how this series affect
> > > users to take a look at her observations.[0] Thank you!
> > > 
> > > Changes for v3:
> > > 
> > >   Re-base the series on cxl/next.
> > > 
> > >   1/4 - 2/4:
> > > 	Constify local variables.
> > >   3/4:
> > > 	Call arch_match_region() from region_res_match_cxl_range().
> > >   4/4:
> > > 	arch_match_region() - Check that region end is under start + 4G;
> > > 	arch_match_spa() - Check that SPA range start is cfmws_range_start.
> > 
> > I have sent comments for version 1 and suggested a simpler approach
> > for this to implement.
> >
> I replied to your comments for version 1. Please find my message at 
> https://lore.kernel.org/linux-cxl/9930375.eNJFYEL58v@fdefranc-mobl3/.

The outcome was "I'll think about it.".

> >
> > My comments haven't been addressed yet,
> >
> I think it's not possible to detect an LMH while still in cxl_port_add().

Why is that not possible? I have outlined a solution before: Implement
a function to run platform specific port setup. Run a platform check.
Enable a platform dependend callback. Setup the port using platform
specifics.

> Therefore, I think that this is the best way to go. It works to prevent
> the driver to fail to create Regions and attach Endpoint Decoders on x86
> with Low Memory Holes, provided that the lower SPA range starts at 0x0.

An alternative works as well, that is not an argument.

> 
> On platforms with other kind of holes, the driver will continue to fail
> as it currently does. 

And those platform will then add more specific code and more
complexity in the main path other need to run? See, the code needs to
be encapsulated.

> >
> > but we
> > need better isolation to reduce interference with other platforms and
> > archs. Please take a look again.
> >
> Interference? Do you mean that this series would make the driver fail on 
> other platforms? 

No, other platforms must deal with that specific code and constrains.

> 
> Of course I don't want anything like that. I'm not clear about it...
> Would you please describe how would this series interfere and what
> would happen on other platforms?

Other platforms should not care about platform-specifics of others. So
again, use a platform check and only enable that code there necessary.
And this requires a well defined interface to common code.

Thanks,

-Robert

> 
> Thanks,
> 
> Fabio
> >
> > Many thanks,
> > 
> > -Robert
> > 
> 
> 
> 
> 

Powered by blists - more mailing lists