Re: [PATCH 2/4 v4] cxl/core: Add helpers to detect Low Memory Holes on x86

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

Hi Benjamin,

I thought again and decided to do a few more changes that you suggested... 

On Friday, August 1, 2025 10:04:16 PM Central European Summer Time Cheatham, Benjamin wrote:
> On 7/24/2025 9:20 AM, Fabio M. De Francesco wrote:
> > In x86 with Low memory Hole, the BIOS may publishes CFMWS that describe
> 
> s/publishes/publish
> 
> > SPA ranges which are subsets of the corresponding CXL Endpoint Decoders
> > HPA's because the CFMWS never intersects LMH's while EP Decoders HPA's
> > ranges are always guaranteed to align to the NIW * 256M rule.
> 
> s/to align to/to align due to/
> 
> Also a spec reference for the rule would be helpful (same with next patch).
> 
> > 
> > In order to construct Regions and attach Decoders, the driver needs to
> > match Root Decoders and Regions with Endpoint Decoders, but it fails and
> > the entire process returns errors because it doesn't expect to deal with
> > SPA range lengths smaller than corresponding HPA's.
> > 
> > Introduce functions that indirectly detect x86 LMH's by comparing SPA's
> > with corresponding HPA's. They will be used in the process of Regions
> > creation and Endpoint attachments to prevent driver failures in a few
> > steps of the above-mentioned process.
> > 
> > The helpers return true when HPA/SPA misalignments are detected under
> > specific conditions: both the SPA and HPA ranges must start at
> > LMH_CFMWS_RANGE_START (that in x86 with LMH's is 0x0), SPA range sizes
> 
> maybe sub "that in x86 with LMH's is 0x0" with "0x0 on x86 with LMH's" also
> s/SPA range sizes/SPA range's size/.
> 
> > be less than HPA's, SPA's range's size be less than 4G, HPA's size be
> > aligned to the NIW * 256M rule.
> 
> s/be/is/ in above list.
> 
> > 
> > Also introduce a function to adjust the range end of the Regions to be
> > created on x86 with LMH's.
> > 
> > Cc: Alison Schofield <alison.schofield@intel.com>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: Dave Jiang <dave.jiang@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/Kconfig         |  5 +++
> >  drivers/cxl/core/Makefile   |  1 +
> >  drivers/cxl/core/platform.c | 85 +++++++++++++++++++++++++++++++++++++
> >  drivers/cxl/core/platform.h | 32 ++++++++++++++
> >  4 files changed, 123 insertions(+)
> >  create mode 100644 drivers/cxl/core/platform.c
> >  create mode 100644 drivers/cxl/core/platform.h
> > 
> > diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> > index 48b7314afdb8..eca90baeac10 100644
> > --- a/drivers/cxl/Kconfig
> > +++ b/drivers/cxl/Kconfig
> > @@ -211,6 +211,11 @@ config CXL_REGION
> >  
> >  	  If unsure say 'y'
> >  
> > +config CXL_PLATFORM_QUIRKS
> > +	def_bool y
> > +	depends on CXL_REGION
> > +	depends on X86
> > +
> 
> I'm confused on the intention behind this symbol. The naming suggests it's for all platform quirks,
> but the code and dependencies make this x86-specific.
> 
> I'm going to suggest making this x86-specific for now. I'm not aware of any other platforms with quirks
> (someone correct me if I'm wrong), so making this x86-specific is fine for now. I would rename this
> symbol to CXL_X86_QUIRKS, leave dependencies as-is, and rename platform.c to something like platform_x86.c.
> Then, if someone comes along with other platform quirks they can do their own symbol and file (or come
> up with a generic scheme).
> 
I'll make it x86-specific for now. platform.c will be renamed to platform_x86.c
and CXL_PLATFORM_QUIRKS to CXL_X86_QUIRKS.

Thanks,

Fabio