| Message ID | 20241122155226.2068287-3-fabio.m.de.francesco@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | cxl/core: Enable Region creation on x86 with Low Mem Hole | expand |
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. In some cases the size > of that hole is not compatible with the CXL hardware decoder constraint > that the size is always aligned to 256M * Interleave Ways. > > 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, cxl_add_to_region() fails and returns an error > because it can't find any CXL Window that matches a given CXL Endpoint > Decoder. > > Detect Low Memory Holes by comparing Root Decoders and Endpoint Decoders > ranges: both must start at physical address 0 and end below 4 GB, while > the Root Decoder ranges end at lower addresses than the matching Endpoint > Decoders which instead must always respect the above-mentioned CXL hardware > decoders HPA alignment constraint. > > Match misaligned CFMWS and CXL Regions with corresponding CXL Endpoint > Decoders if driver detects Low Memory Holes. > > Construct CXL Regions with HPA range's end trimmed by matching SPA. > > Allow the attach target process to complete by relaxing Decoder constraints > which would lead to failures. > > 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> Over all this looks good but I wonder if there is a slight optimization which could be done. See below. > --- > drivers/cxl/Kconfig | 5 ++++ > drivers/cxl/core/Makefile | 1 + > drivers/cxl/core/lmh.c | 53 +++++++++++++++++++++++++++++++++++++ > drivers/cxl/core/region.c | 55 ++++++++++++++++++++++++++++++++------- > drivers/cxl/cxl.h | 25 ++++++++++++++++++ > 5 files changed, 130 insertions(+), 9 deletions(-) > create mode 100644 drivers/cxl/core/lmh.c > > diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig > index 876469e23f7a..07b87f217e59 100644 > --- a/drivers/cxl/Kconfig > +++ b/drivers/cxl/Kconfig > @@ -128,6 +128,11 @@ config CXL_REGION > > If unsure say 'y' > > +config CXL_ARCH_LOW_MEMORY_HOLE > + def_bool y > + depends on CXL_REGION > + depends on X86 > + > config CXL_REGION_INVALIDATION_TEST > bool "CXL: Region Cache Management Bypass (TEST)" > depends on CXL_REGION > diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile > index 9259bcc6773c..6e80215e8444 100644 > --- a/drivers/cxl/core/Makefile > +++ b/drivers/cxl/core/Makefile > @@ -15,4 +15,5 @@ cxl_core-y += hdm.o > cxl_core-y += pmu.o > cxl_core-y += cdat.o > cxl_core-$(CONFIG_TRACING) += trace.o > +cxl_core-$(CONFIG_CXL_ARCH_LOW_MEMORY_HOLE) += lmh.o > cxl_core-$(CONFIG_CXL_REGION) += region.o > diff --git a/drivers/cxl/core/lmh.c b/drivers/cxl/core/lmh.c > new file mode 100644 > index 000000000000..da76b2a534ec > --- /dev/null > +++ b/drivers/cxl/core/lmh.c > @@ -0,0 +1,53 @@ > +// SPDX-License-Identifier: GPL-2.0-only > + > +#include <linux/range.h> > +#include "cxl.h" > + > +/* In x86 with memory hole, misaligned CFMWS range starts at 0x0 */ > +#define MISALIGNED_CFMWS_RANGE_BASE 0x0 > + > +/* > + * Match CXL Root and Endpoint Decoders by comparing SPA and HPA ranges. > + * > + * On x86, CFMWS ranges never intersect memory holes while endpoint decoders > + * HPA range sizes are always guaranteed aligned to NIW * 256MB; therefore, > + * the given endpoint decoder HPA range size is always expected aligned and > + * also larger than that of the matching root decoder > + */ > +bool arch_match_spa(struct cxl_root_decoder *cxlrd, > + struct cxl_endpoint_decoder *cxled) > +{ > + struct range *r1, *r2; > + int niw; > + > + r1 = &cxlrd->cxlsd.cxld.hpa_range; > + r2 = &cxled->cxld.hpa_range; > + niw = cxled->cxld.interleave_ways; > + > + if (r1->start == MISALIGNED_CFMWS_RANGE_BASE && > + r1->start == r2->start && r1->end < r2->end && Should this be 'r1->end <= r2->end'? And could that simplify the logic in match_switch_decoder_by_range()? See below... > + IS_ALIGNED(range_len(r2), niw * SZ_256M)) > + return true; > + return false; > +} > + > +/* Similar to arch_match_spa(), it matches regions and decoders */ > +bool arch_match_region(struct cxl_region_params *p, > + struct cxl_decoder *cxld) > +{ > + struct range *r = &cxld->hpa_range; > + struct resource *res = p->res; > + int niw = cxld->interleave_ways; > + > + if (res->start == MISALIGNED_CFMWS_RANGE_BASE && > + res->start == r->start && res->end < r->end && > + IS_ALIGNED(range_len(r), niw * SZ_256M)) > + return true; > + return false; > +} > + > +void arch_trim_hpa_by_spa(struct resource *res, > + struct cxl_root_decoder *cxlrd) > +{ > + res->end = cxlrd->res->end; > +} > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index ac2c486c16e9..3cb5a69e9731 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -836,8 +836,12 @@ static int match_auto_decoder(struct device *dev, void *data) > cxld = to_cxl_decoder(dev); > r = &cxld->hpa_range; > > - if (p->res && p->res->start == r->start && p->res->end == r->end) > - return 1; > + if (p->res) { > + if (p->res->start == r->start && p->res->end == r->end) > + return 1; > + if (arch_match_region(p, cxld)) > + return 1; > + } > > return 0; > } > @@ -1414,7 +1418,8 @@ static int cxl_port_setup_targets(struct cxl_port *port, > if (cxld->interleave_ways != iw || > cxld->interleave_granularity != ig || > cxld->hpa_range.start != p->res->start || > - cxld->hpa_range.end != p->res->end || > + (cxld->hpa_range.end != p->res->end && > + !arch_match_region(p, cxld)) || > ((cxld->flags & CXL_DECODER_F_ENABLE) == 0)) { > dev_err(&cxlr->dev, > "%s:%s %s expected iw: %d ig: %d %pr\n", > @@ -1726,6 +1731,7 @@ static int match_switch_decoder_by_range(struct device *dev, void *data) > { > struct cxl_endpoint_decoder *cxled = data; > struct cxl_switch_decoder *cxlsd; > + struct cxl_root_decoder *cxlrd; > struct range *r1, *r2; > > if (!is_switch_decoder(dev)) > @@ -1735,8 +1741,13 @@ static int match_switch_decoder_by_range(struct device *dev, void *data) > r1 = &cxlsd->cxld.hpa_range; > r2 = &cxled->cxld.hpa_range; > > - if (is_root_decoder(dev)) > - return range_contains(r1, r2); > + if (is_root_decoder(dev)) { > + if (range_contains(r1, r2)) > + return 1; > + cxlrd = to_cxl_root_decoder(dev); > + if (arch_match_spa(cxlrd, cxled)) > + return 1; > + } If the logic in arch_match_spa() was changed to allow for an equal end value could this block be simplified to? if (is_root_decoder(dev)) { cxlrd = to_cxl_root_decoder(dev); if (arch_match_spa(cxlrd, cxled)) return 1; } > return (r1->start == r2->start && r1->end == r2->end); > } > > @@ -1943,7 +1954,7 @@ static int cxl_region_attach(struct cxl_region *cxlr, > } > > if (resource_size(cxled->dpa_res) * p->interleave_ways != > - resource_size(p->res)) { > + resource_size(p->res) && !arch_match_spa(cxlrd, cxled)) { > dev_dbg(&cxlr->dev, > "%s:%s: decoder-size-%#llx * ways-%d != region-size-%#llx\n", > dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), > @@ -3199,7 +3210,13 @@ static int match_root_decoder_by_range(struct device *dev, void *data) > r1 = &cxlrd->cxlsd.cxld.hpa_range; > r2 = &cxled->cxld.hpa_range; > > - return range_contains(r1, r2); > + if (range_contains(r1, r2)) > + return true; > + > + if (arch_match_spa(cxlrd, cxled)) > + return true; Same question/simplification here? Ira > + > + return false; > } > > static int match_region_by_range(struct device *dev, void *data) > @@ -3217,8 +3234,12 @@ static int match_region_by_range(struct device *dev, void *data) > p = &cxlr->params; > > down_read(&cxl_region_rwsem); > - if (p->res && p->res->start == r->start && p->res->end == r->end) > - rc = 1; > + if (p->res) { > + if (p->res->start == r->start && p->res->end == r->end) > + rc = 1; > + if (arch_match_region(p, &cxled->cxld)) > + rc = 1; > + } > up_read(&cxl_region_rwsem); > > return rc; > @@ -3270,6 +3291,22 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, > > *res = DEFINE_RES_MEM_NAMED(hpa->start, range_len(hpa), > dev_name(&cxlr->dev)); > + > + /* > + * Trim the HPA retrieved from hardware to fit the SPA mapped by the > + * platform > + */ > + if (arch_match_spa(cxlrd, cxled)) { > + struct range *range = &cxlrd->cxlsd.cxld.hpa_range; > + > + arch_trim_hpa_by_spa(res, cxlrd); > + dev_dbg(cxlmd->dev.parent, > + "%s: Trim HPA (%s: %pr) by SPA (%s: %pr)\n", > + __func__, > + dev_name(&cxlrd->cxlsd.cxld.dev), range, > + dev_name(&cxled->cxld.dev), hpa); > + } > + > rc = insert_resource(cxlrd->res, res); > if (rc) { > /* > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 5406e3ab3d4a..a5ad4499381e 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -902,6 +902,31 @@ void cxl_coordinates_combine(struct access_coordinate *out, > > bool cxl_endpoint_decoder_reset_detected(struct cxl_port *port); > > +#ifdef CONFIG_CXL_ARCH_LOW_MEMORY_HOLE > +bool arch_match_spa(struct cxl_root_decoder *cxlrd, > + struct cxl_endpoint_decoder *cxled); > +bool arch_match_region(struct cxl_region_params *p, > + struct cxl_decoder *cxld); > +void arch_trim_hpa_by_spa(struct resource *res, > + struct cxl_root_decoder *cxlrd); > +#else > +bool arch_match_spa(struct cxl_root_decoder *cxlrd, > + struct cxl_endpoint_decoder *cxled) > +{ > + return false; > +} > + > +bool arch_match_region(struct cxl_region_params *p, > + struct cxl_decoder *cxld) > +{ > + return false; > +} > + > +void arch_trim_hpa_by_spa(struct resource *res, > + struct cxl_root_decoder *cxlrd) > +{ } > +#endif /* CXL_ARCH_LOW_MEMORY_HOLE */ > + > /* > * Unit test builds overrides this to __weak, find the 'strong' version > * of these symbols in tools/testing/cxl/. > -- > 2.46.2 >
Hi Fabio,
kernel test robot noticed the following build warnings:
[auto build test WARNING on cxl/next]
[also build test WARNING on linus/master cxl/pending v6.12 next-20241125]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Fabio-M-De-Francesco/cxl-core-Change-match_-_by_range-calling-convention/20241125-102754
base: https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git next
patch link: https://lore.kernel.org/r/20241122155226.2068287-3-fabio.m.de.francesco%40linux.intel.com
patch subject: [PATCH 2/3] cxl/core: Enable Region creation on x86 with Low Memory Hole
config: i386-buildonly-randconfig-001-20241125 (https://download.01.org/0day-ci/archive/20241125/202411251632.4eenIlnF-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241125/202411251632.4eenIlnF-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411251632.4eenIlnF-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/cxl/cxlmem.h:12,
from drivers/cxl/port.c:7:
>> drivers/cxl/cxl.h:921:6: warning: no previous prototype for 'arch_match_spa' [-Wmissing-prototypes]
921 | bool arch_match_spa(struct cxl_root_decoder *cxlrd,
| ^~~~~~~~~~~~~~
>> drivers/cxl/cxl.h:927:6: warning: no previous prototype for 'arch_match_region' [-Wmissing-prototypes]
927 | bool arch_match_region(struct cxl_region_params *p,
| ^~~~~~~~~~~~~~~~~
>> drivers/cxl/cxl.h:933:6: warning: no previous prototype for 'arch_trim_hpa_by_spa' [-Wmissing-prototypes]
933 | void arch_trim_hpa_by_spa(struct resource *res,
| ^~~~~~~~~~~~~~~~~~~~
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for GET_FREE_REGION
Depends on [n]: SPARSEMEM [=n]
Selected by [y]:
- RESOURCE_KUNIT_TEST [=y] && RUNTIME_TESTING_MENU [=y] && KUNIT [=y]
vim +/arch_match_spa +921 drivers/cxl/cxl.h
912
913 #ifdef CONFIG_CXL_ARCH_LOW_MEMORY_HOLE
914 bool arch_match_spa(struct cxl_root_decoder *cxlrd,
915 struct cxl_endpoint_decoder *cxled);
916 bool arch_match_region(struct cxl_region_params *p,
917 struct cxl_decoder *cxld);
918 void arch_trim_hpa_by_spa(struct resource *res,
919 struct cxl_root_decoder *cxlrd);
920 #else
> 921 bool arch_match_spa(struct cxl_root_decoder *cxlrd,
922 struct cxl_endpoint_decoder *cxled)
923 {
924 return false;
925 }
926
> 927 bool arch_match_region(struct cxl_region_params *p,
928 struct cxl_decoder *cxld)
929 {
930 return false;
931 }
932
> 933 void arch_trim_hpa_by_spa(struct resource *res,
934 struct cxl_root_decoder *cxlrd)
935 { }
936 #endif /* CXL_ARCH_LOW_MEMORY_HOLE */
937
On 11/22/2024 11:51 PM, 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. In some cases the size > of that hole is not compatible with the CXL hardware decoder constraint > that the size is always aligned to 256M * Interleave Ways. > > 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, cxl_add_to_region() fails and returns an error > because it can't find any CXL Window that matches a given CXL Endpoint > Decoder. > > Detect Low Memory Holes by comparing Root Decoders and Endpoint Decoders > ranges: both must start at physical address 0 and end below 4 GB, while > the Root Decoder ranges end at lower addresses than the matching Endpoint > Decoders which instead must always respect the above-mentioned CXL hardware > decoders HPA alignment constraint. Hi Fabio, Here mentions that the end address must be below 4GB, but I don't find any checking about that in the implementation, is it not needed? [snip] > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index ac2c486c16e9..3cb5a69e9731 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -836,8 +836,12 @@ static int match_auto_decoder(struct device *dev, void *data) > cxld = to_cxl_decoder(dev); > r = &cxld->hpa_range; > > - if (p->res && p->res->start == r->start && p->res->end == r->end) > - return 1; > + if (p->res) { > + if (p->res->start == r->start && p->res->end == r->end) > + return 1; > + if (arch_match_region(p, cxld)) > + return 1; > + } I think that it is better to check LMH cases before checking (p->res->start == r->start && p->res->end == r->end). Per the changelog and the implementation of arch_match_region(), below case should fails but current checking will succeed: the value of 'p->res->start' is MISALIGNED_CFMWS_RANGE_BASE and the the value of 'p->res->end' is equals to the value of 'r->end'. > > return 0; > } > @@ -1414,7 +1418,8 @@ static int cxl_port_setup_targets(struct cxl_port *port, > if (cxld->interleave_ways != iw || > cxld->interleave_granularity != ig || > cxld->hpa_range.start != p->res->start || > - cxld->hpa_range.end != p->res->end || > + (cxld->hpa_range.end != p->res->end && > + !arch_match_region(p, cxld)) || > ((cxld->flags & CXL_DECODER_F_ENABLE) == 0)) { > dev_err(&cxlr->dev, > "%s:%s %s expected iw: %d ig: %d %pr\n", > @@ -1726,6 +1731,7 @@ static int match_switch_decoder_by_range(struct device *dev, void *data) > { > struct cxl_endpoint_decoder *cxled = data; > struct cxl_switch_decoder *cxlsd; > + struct cxl_root_decoder *cxlrd; > struct range *r1, *r2; > > if (!is_switch_decoder(dev)) > @@ -1735,8 +1741,13 @@ static int match_switch_decoder_by_range(struct device *dev, void *data) > r1 = &cxlsd->cxld.hpa_range; > r2 = &cxled->cxld.hpa_range; > > - if (is_root_decoder(dev)) > - return range_contains(r1, r2); > + if (is_root_decoder(dev)) { > + if (range_contains(r1, r2)) > + return 1; > + cxlrd = to_cxl_root_decoder(dev); > + if (arch_match_spa(cxlrd, cxled)) > + return 1; Same as above, what will happen if the root decoder's address range still contains endpoint decoder's address range in LMH cases? should fails or succeed? > + } > return (r1->start == r2->start && r1->end == r2->end); > } > > @@ -1943,7 +1954,7 @@ static int cxl_region_attach(struct cxl_region *cxlr, > } > > if (resource_size(cxled->dpa_res) * p->interleave_ways != > - resource_size(p->res)) { > + resource_size(p->res) && !arch_match_spa(cxlrd, cxled)) { > dev_dbg(&cxlr->dev, > "%s:%s: decoder-size-%#llx * ways-%d != region-size-%#llx\n", > dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), > @@ -3199,7 +3210,13 @@ static int match_root_decoder_by_range(struct device *dev, void *data) > r1 = &cxlrd->cxlsd.cxld.hpa_range; > r2 = &cxled->cxld.hpa_range; > > - return range_contains(r1, r2); > + if (range_contains(r1, r2)) > + return true; > + > + if (arch_match_spa(cxlrd, cxled)) > + return true; > + > + return false; Same as above. > } > > static int match_region_by_range(struct device *dev, void *data) > @@ -3217,8 +3234,12 @@ static int match_region_by_range(struct device *dev, void *data) > p = &cxlr->params; > > down_read(&cxl_region_rwsem); > - if (p->res && p->res->start == r->start && p->res->end == r->end) > - rc = 1; > + if (p->res) { > + if (p->res->start == r->start && p->res->end == r->end) > + rc = 1; > + if (arch_match_region(p, &cxled->cxld)) > + rc = 1; > + } Same as above. Thanks Ming
On 11/23/2024 1:25 AM, Ira Weiny wrote: > 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. In some cases the size >> of that hole is not compatible with the CXL hardware decoder constraint >> that the size is always aligned to 256M * Interleave Ways. >> >> 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, cxl_add_to_region() fails and returns an error >> because it can't find any CXL Window that matches a given CXL Endpoint >> Decoder. >> >> Detect Low Memory Holes by comparing Root Decoders and Endpoint Decoders >> ranges: both must start at physical address 0 and end below 4 GB, while >> the Root Decoder ranges end at lower addresses than the matching Endpoint >> Decoders which instead must always respect the above-mentioned CXL hardware >> decoders HPA alignment constraint. >> >> Match misaligned CFMWS and CXL Regions with corresponding CXL Endpoint >> Decoders if driver detects Low Memory Holes. >> >> Construct CXL Regions with HPA range's end trimmed by matching SPA. >> >> Allow the attach target process to complete by relaxing Decoder constraints >> which would lead to failures. >> >> 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> > > Over all this looks good but I wonder if there is a slight optimization > which could be done. See below. > >> --- >> drivers/cxl/Kconfig | 5 ++++ >> drivers/cxl/core/Makefile | 1 + >> drivers/cxl/core/lmh.c | 53 +++++++++++++++++++++++++++++++++++++ >> drivers/cxl/core/region.c | 55 ++++++++++++++++++++++++++++++++------- >> drivers/cxl/cxl.h | 25 ++++++++++++++++++ >> 5 files changed, 130 insertions(+), 9 deletions(-) >> create mode 100644 drivers/cxl/core/lmh.c >> >> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig >> index 876469e23f7a..07b87f217e59 100644 >> --- a/drivers/cxl/Kconfig >> +++ b/drivers/cxl/Kconfig >> @@ -128,6 +128,11 @@ config CXL_REGION >> >> If unsure say 'y' >> >> +config CXL_ARCH_LOW_MEMORY_HOLE >> + def_bool y >> + depends on CXL_REGION >> + depends on X86 >> + >> config CXL_REGION_INVALIDATION_TEST >> bool "CXL: Region Cache Management Bypass (TEST)" >> depends on CXL_REGION >> diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile >> index 9259bcc6773c..6e80215e8444 100644 >> --- a/drivers/cxl/core/Makefile >> +++ b/drivers/cxl/core/Makefile >> @@ -15,4 +15,5 @@ cxl_core-y += hdm.o >> cxl_core-y += pmu.o >> cxl_core-y += cdat.o >> cxl_core-$(CONFIG_TRACING) += trace.o >> +cxl_core-$(CONFIG_CXL_ARCH_LOW_MEMORY_HOLE) += lmh.o >> cxl_core-$(CONFIG_CXL_REGION) += region.o >> diff --git a/drivers/cxl/core/lmh.c b/drivers/cxl/core/lmh.c >> new file mode 100644 >> index 000000000000..da76b2a534ec >> --- /dev/null >> +++ b/drivers/cxl/core/lmh.c >> @@ -0,0 +1,53 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> + >> +#include <linux/range.h> >> +#include "cxl.h" >> + >> +/* In x86 with memory hole, misaligned CFMWS range starts at 0x0 */ >> +#define MISALIGNED_CFMWS_RANGE_BASE 0x0 >> + >> +/* >> + * Match CXL Root and Endpoint Decoders by comparing SPA and HPA ranges. >> + * >> + * On x86, CFMWS ranges never intersect memory holes while endpoint decoders >> + * HPA range sizes are always guaranteed aligned to NIW * 256MB; therefore, >> + * the given endpoint decoder HPA range size is always expected aligned and >> + * also larger than that of the matching root decoder >> + */ >> +bool arch_match_spa(struct cxl_root_decoder *cxlrd, >> + struct cxl_endpoint_decoder *cxled) >> +{ >> + struct range *r1, *r2; >> + int niw; >> + >> + r1 = &cxlrd->cxlsd.cxld.hpa_range; >> + r2 = &cxled->cxld.hpa_range; >> + niw = cxled->cxld.interleave_ways; >> + >> + if (r1->start == MISALIGNED_CFMWS_RANGE_BASE && >> + r1->start == r2->start && r1->end < r2->end && > > Should this be 'r1->end <= r2->end'? > Hi Ira, I think that cannot just simply use 'r1->end <= r2->end' to instead of 'r1->end < r2->end' here, because there is a 'r1->start == MISALIGNED_CFMWS_RANGE_BASE' checking which means the 'if' checking is only for LMH cases. But maybe can put both LMH cases checkings and non-LMH cases checkings into one function. Thanks Ming [snip]
Hi Fabio, kernel test robot noticed the following build errors: [auto build test ERROR on cxl/next] [also build test ERROR on linus/master cxl/pending v6.12 next-20241125] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Fabio-M-De-Francesco/cxl-core-Change-match_-_by_range-calling-convention/20241125-102754 base: https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git next patch link: https://lore.kernel.org/r/20241122155226.2068287-3-fabio.m.de.francesco%40linux.intel.com patch subject: [PATCH 2/3] cxl/core: Enable Region creation on x86 with Low Memory Hole config: s390-allyesconfig (https://download.01.org/0day-ci/archive/20241125/202411252126.T7xKY88q-lkp@intel.com/config) compiler: s390-linux-gcc (GCC) 14.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241125/202411252126.T7xKY88q-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202411252126.T7xKY88q-lkp@intel.com/ All error/warnings (new ones prefixed by >>): In file included from drivers/dax/cxl.c:6: >> drivers/dax/../cxl/cxl.h:921:6: warning: no previous prototype for 'arch_match_spa' [-Wmissing-prototypes] 921 | bool arch_match_spa(struct cxl_root_decoder *cxlrd, | ^~~~~~~~~~~~~~ >> drivers/dax/../cxl/cxl.h:927:6: warning: no previous prototype for 'arch_match_region' [-Wmissing-prototypes] 927 | bool arch_match_region(struct cxl_region_params *p, | ^~~~~~~~~~~~~~~~~ >> drivers/dax/../cxl/cxl.h:933:6: warning: no previous prototype for 'arch_trim_hpa_by_spa' [-Wmissing-prototypes] 933 | void arch_trim_hpa_by_spa(struct resource *res, | ^~~~~~~~~~~~~~~~~~~~ -- s390-linux-ld: drivers/cxl/core/port.o: in function `arch_match_spa': >> port.c:(.text+0x87c0): multiple definition of `arch_match_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x200): first defined here s390-linux-ld: drivers/cxl/core/port.o: in function `arch_match_region': >> port.c:(.text+0x8840): multiple definition of `arch_match_region'; drivers/dax/cxl.o:cxl.c:(.text+0x280): first defined here s390-linux-ld: drivers/cxl/core/port.o: in function `arch_trim_hpa_by_spa': >> port.c:(.text+0x88c0): multiple definition of `arch_trim_hpa_by_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x300): first defined here s390-linux-ld: drivers/cxl/core/pmem.o: in function `arch_match_spa': pmem.c:(.text+0xe00): multiple definition of `arch_match_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x200): first defined here s390-linux-ld: drivers/cxl/core/pmem.o: in function `arch_match_region': pmem.c:(.text+0xe80): multiple definition of `arch_match_region'; drivers/dax/cxl.o:cxl.c:(.text+0x280): first defined here s390-linux-ld: drivers/cxl/core/pmem.o: in function `arch_trim_hpa_by_spa': pmem.c:(.text+0xf00): multiple definition of `arch_trim_hpa_by_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x300): first defined here s390-linux-ld: drivers/cxl/core/regs.o: in function `arch_match_spa': regs.c:(.text+0x1740): multiple definition of `arch_match_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x200): first defined here s390-linux-ld: drivers/cxl/core/regs.o: in function `arch_match_region': regs.c:(.text+0x17c0): multiple definition of `arch_match_region'; drivers/dax/cxl.o:cxl.c:(.text+0x280): first defined here s390-linux-ld: drivers/cxl/core/regs.o: in function `arch_trim_hpa_by_spa': regs.c:(.text+0x1840): multiple definition of `arch_trim_hpa_by_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x300): first defined here s390-linux-ld: drivers/cxl/core/memdev.o: in function `arch_match_spa': memdev.c:(.text+0x3ec0): multiple definition of `arch_match_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x200): first defined here s390-linux-ld: drivers/cxl/core/memdev.o: in function `arch_match_region': memdev.c:(.text+0x3f40): multiple definition of `arch_match_region'; drivers/dax/cxl.o:cxl.c:(.text+0x280): first defined here s390-linux-ld: drivers/cxl/core/memdev.o: in function `arch_trim_hpa_by_spa': memdev.c:(.text+0x3fc0): multiple definition of `arch_trim_hpa_by_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x300): first defined here s390-linux-ld: drivers/cxl/core/mbox.o: in function `arch_match_spa': mbox.c:(.text+0x4f80): multiple definition of `arch_match_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x200): first defined here s390-linux-ld: drivers/cxl/core/mbox.o: in function `arch_match_region': mbox.c:(.text+0x5000): multiple definition of `arch_match_region'; drivers/dax/cxl.o:cxl.c:(.text+0x280): first defined here s390-linux-ld: drivers/cxl/core/mbox.o: in function `arch_trim_hpa_by_spa': mbox.c:(.text+0x5080): multiple definition of `arch_trim_hpa_by_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x300): first defined here s390-linux-ld: drivers/cxl/core/pci.o: in function `arch_match_spa': pci.c:(.text+0x3e40): multiple definition of `arch_match_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x200): first defined here s390-linux-ld: drivers/cxl/core/pci.o: in function `arch_match_region': pci.c:(.text+0x3ec0): multiple definition of `arch_match_region'; drivers/dax/cxl.o:cxl.c:(.text+0x280): first defined here s390-linux-ld: drivers/cxl/core/pci.o: in function `arch_trim_hpa_by_spa': pci.c:(.text+0x3f40): multiple definition of `arch_trim_hpa_by_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x300): first defined here s390-linux-ld: drivers/cxl/core/hdm.o: in function `arch_match_spa': hdm.c:(.text+0x4880): multiple definition of `arch_match_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x200): first defined here s390-linux-ld: drivers/cxl/core/hdm.o: in function `arch_match_region': hdm.c:(.text+0x4900): multiple definition of `arch_match_region'; drivers/dax/cxl.o:cxl.c:(.text+0x280): first defined here s390-linux-ld: drivers/cxl/core/hdm.o: in function `arch_trim_hpa_by_spa': hdm.c:(.text+0x4980): multiple definition of `arch_trim_hpa_by_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x300): first defined here s390-linux-ld: drivers/cxl/core/pmu.o: in function `arch_match_spa': pmu.c:(.text+0x340): multiple definition of `arch_match_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x200): first defined here s390-linux-ld: drivers/cxl/core/pmu.o: in function `arch_match_region': pmu.c:(.text+0x3c0): multiple definition of `arch_match_region'; drivers/dax/cxl.o:cxl.c:(.text+0x280): first defined here s390-linux-ld: drivers/cxl/core/pmu.o: in function `arch_trim_hpa_by_spa': pmu.c:(.text+0x440): multiple definition of `arch_trim_hpa_by_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x300): first defined here s390-linux-ld: drivers/cxl/core/cdat.o: in function `arch_match_spa': cdat.c:(.text+0x19c0): multiple definition of `arch_match_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x200): first defined here s390-linux-ld: drivers/cxl/core/cdat.o: in function `arch_match_region': cdat.c:(.text+0x1a40): multiple definition of `arch_match_region'; drivers/dax/cxl.o:cxl.c:(.text+0x280): first defined here s390-linux-ld: drivers/cxl/core/cdat.o: in function `arch_trim_hpa_by_spa': cdat.c:(.text+0x1ac0): multiple definition of `arch_trim_hpa_by_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x300): first defined here s390-linux-ld: drivers/cxl/core/trace.o: in function `arch_match_spa': trace.c:(.text+0x8ec0): multiple definition of `arch_match_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x200): first defined here s390-linux-ld: drivers/cxl/core/trace.o: in function `arch_match_region': trace.c:(.text+0x8f40): multiple definition of `arch_match_region'; drivers/dax/cxl.o:cxl.c:(.text+0x280): first defined here s390-linux-ld: drivers/cxl/core/trace.o: in function `arch_trim_hpa_by_spa': trace.c:(.text+0x8fc0): multiple definition of `arch_trim_hpa_by_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x300): first defined here s390-linux-ld: drivers/cxl/core/region.o: in function `arch_match_spa': region.c:(.text+0xca00): multiple definition of `arch_match_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x200): first defined here s390-linux-ld: drivers/cxl/core/region.o: in function `arch_match_region': region.c:(.text+0xce00): multiple definition of `arch_match_region'; drivers/dax/cxl.o:cxl.c:(.text+0x280): first defined here s390-linux-ld: drivers/cxl/core/region.o: in function `arch_trim_hpa_by_spa': region.c:(.text+0x11a80): multiple definition of `arch_trim_hpa_by_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x300): first defined here s390-linux-ld: drivers/cxl/pci.o: in function `arch_match_spa': pci.c:(.text+0x4cc0): multiple definition of `arch_match_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x200): first defined here s390-linux-ld: drivers/cxl/pci.o: in function `arch_match_region': pci.c:(.text+0x4d40): multiple definition of `arch_match_region'; drivers/dax/cxl.o:cxl.c:(.text+0x280): first defined here s390-linux-ld: drivers/cxl/pci.o: in function `arch_trim_hpa_by_spa': pci.c:(.text+0x4dc0): multiple definition of `arch_trim_hpa_by_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x300): first defined here s390-linux-ld: drivers/cxl/mem.o: in function `arch_match_spa': mem.c:(.text+0xe40): multiple definition of `arch_match_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x200): first defined here s390-linux-ld: drivers/cxl/mem.o: in function `arch_match_region': mem.c:(.text+0xec0): multiple definition of `arch_match_region'; drivers/dax/cxl.o:cxl.c:(.text+0x280): first defined here s390-linux-ld: drivers/cxl/mem.o: in function `arch_trim_hpa_by_spa': mem.c:(.text+0xf40): multiple definition of `arch_trim_hpa_by_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x300): first defined here s390-linux-ld: drivers/cxl/pmem.o: in function `arch_match_spa': pmem.c:(.text+0x18c0): multiple definition of `arch_match_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x200): first defined here s390-linux-ld: drivers/cxl/pmem.o: in function `arch_match_region': pmem.c:(.text+0x1940): multiple definition of `arch_match_region'; drivers/dax/cxl.o:cxl.c:(.text+0x280): first defined here s390-linux-ld: drivers/cxl/pmem.o: in function `arch_trim_hpa_by_spa': pmem.c:(.text+0x19c0): multiple definition of `arch_trim_hpa_by_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x300): first defined here s390-linux-ld: drivers/cxl/security.o: in function `arch_match_spa': security.c:(.text+0xb00): multiple definition of `arch_match_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x200): first defined here s390-linux-ld: drivers/cxl/security.o: in function `arch_match_region': security.c:(.text+0xb80): multiple definition of `arch_match_region'; drivers/dax/cxl.o:cxl.c:(.text+0x280): first defined here s390-linux-ld: drivers/cxl/security.o: in function `arch_trim_hpa_by_spa': security.c:(.text+0xc00): multiple definition of `arch_trim_hpa_by_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x300): first defined here s390-linux-ld: drivers/cxl/port.o: in function `arch_match_spa': port.c:(.text+0x840): multiple definition of `arch_match_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x200): first defined here s390-linux-ld: drivers/cxl/port.o: in function `arch_match_region': port.c:(.text+0x8c0): multiple definition of `arch_match_region'; drivers/dax/cxl.o:cxl.c:(.text+0x280): first defined here s390-linux-ld: drivers/cxl/port.o: in function `arch_trim_hpa_by_spa': port.c:(.text+0x940): multiple definition of `arch_trim_hpa_by_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x300): first defined here s390-linux-ld: drivers/perf/cxl_pmu.o: in function `arch_match_spa': cxl_pmu.c:(.text+0x3380): multiple definition of `arch_match_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x200): first defined here s390-linux-ld: drivers/perf/cxl_pmu.o: in function `arch_match_region': cxl_pmu.c:(.text+0x3400): multiple definition of `arch_match_region'; drivers/dax/cxl.o:cxl.c:(.text+0x280): first defined here s390-linux-ld: drivers/perf/cxl_pmu.o: in function `arch_trim_hpa_by_spa': cxl_pmu.c:(.text+0x3480): multiple definition of `arch_trim_hpa_by_spa'; drivers/dax/cxl.o:cxl.c:(.text+0x300): first defined here
On Monday, November 25, 2024 9:42:56 AM GMT+1 Li Ming wrote: > > On 11/22/2024 11:51 PM, 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. In some cases the size > > of that hole is not compatible with the CXL hardware decoder constraint > > that the size is always aligned to 256M * Interleave Ways. > > > > 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, cxl_add_to_region() fails and returns an error > > because it can't find any CXL Window that matches a given CXL Endpoint > > Decoder. > > > > Detect Low Memory Holes by comparing Root Decoders and Endpoint Decoders > > ranges: both must start at physical address 0 and end below 4 GB, while > > the Root Decoder ranges end at lower addresses than the matching Endpoint > > Decoders which instead must always respect the above-mentioned CXL hardware > > decoders HPA alignment constraint. > > Hi Fabio, > > Here mentions that the end address must be below 4GB, but I don't find any checking about that in the implementation, is it not needed? > Hi Ming, Good question, thanks! While a first version of arch_match_spa() had a check of 'r2->end < SZ_4G', I dropped it for the final one. It ended up out of sync with the commit message. I think that we don't want that check. I'll rework the commit message for v2. If the hardware decoders HPA ranges extended beyond the end of Low Memory, the LMH would still need to be detected and the decoders capacity would still need to be trimmed to match their corresponding CFMWS range end. > > [snip] > > > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > > index ac2c486c16e9..3cb5a69e9731 100644 > > --- a/drivers/cxl/core/region.c > > +++ b/drivers/cxl/core/region.c > > @@ -836,8 +836,12 @@ static int match_auto_decoder(struct device *dev, void *data) > > cxld = to_cxl_decoder(dev); > > r = &cxld->hpa_range; > > > > - if (p->res && p->res->start == r->start && p->res->end == r->end) > > - return 1; > > + if (p->res) { > > + if (p->res->start == r->start && p->res->end == r->end) > > + return 1; > > + if (arch_match_region(p, cxld)) > > + return 1; > > + } > > I think that it is better to check LMH cases before checking (p->res->start == r->start && p->res->end == r->end). > Per the changelog and the implementation of arch_match_region(), below case should fails but current checking will succeed: > the value of 'p->res->start' is MISALIGNED_CFMWS_RANGE_BASE and the the value of 'p->res->end' is equals to the value of 'r->end'. > I think that the expected "normal" case should always come first. In the expected scenarios the driver deals either with SPA range == HPA range (e.g, in match_auto_decoder()) or with SPA range that fully contains the HPA range (match_decoder_by_range()). If either one of those two cases holds, arch_match_*() helper doesn't need to be called and the match must succeed. Besides, other architectures are free to define holes with many constraints that the driver doesn't want to check first if the expected case is already met. > > > > > return 0; > > } > > @@ -1414,7 +1418,8 @@ static int cxl_port_setup_targets(struct cxl_port *port, > > if (cxld->interleave_ways != iw || > > cxld->interleave_granularity != ig || > > cxld->hpa_range.start != p->res->start || > > - cxld->hpa_range.end != p->res->end || > > + (cxld->hpa_range.end != p->res->end && > > + !arch_match_region(p, cxld)) || > > ((cxld->flags & CXL_DECODER_F_ENABLE) == 0)) { > > dev_err(&cxlr->dev, > > "%s:%s %s expected iw: %d ig: %d %pr\n", > > @@ -1726,6 +1731,7 @@ static int match_switch_decoder_by_range(struct device *dev, void *data) > > { > > struct cxl_endpoint_decoder *cxled = data; > > struct cxl_switch_decoder *cxlsd; > > + struct cxl_root_decoder *cxlrd; > > struct range *r1, *r2; > > > > if (!is_switch_decoder(dev)) > > @@ -1735,8 +1741,13 @@ static int match_switch_decoder_by_range(struct device *dev, void *data) > > r1 = &cxlsd->cxld.hpa_range; > > r2 = &cxled->cxld.hpa_range; > > > > - if (is_root_decoder(dev)) > > - return range_contains(r1, r2); > > + if (is_root_decoder(dev)) { > > + if (range_contains(r1, r2)) > > + return 1; > > + cxlrd = to_cxl_root_decoder(dev); > > + if (arch_match_spa(cxlrd, cxled)) > > + return 1; > > Same as above, what will happen if the root decoder's address range still contains endpoint decoder's address range in LMH cases? should fails or succeed? > If r1 contains r2, there is no LMH and the driver is dealing with the regular, expected, case. It must succeed. Think of the arch_match_*() helpers like something that avoid unwanted failures if arch permits exceptions. Before returning errors when the "normal" tests fail, check if the arch allows any exceptions and make the driver ignore that "strange" SPA/HPA misalignment. > > [snip] > Thanks, Fabio
On Fri, Nov 22, 2024 at 04:51:53PM +0100, 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. In some cases the size > of that hole is not compatible with the CXL hardware decoder constraint > that the size is always aligned to 256M * Interleave Ways. > > 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, cxl_add_to_region() fails and returns an error > because it can't find any CXL Window that matches a given CXL Endpoint > Decoder. > > Detect Low Memory Holes by comparing Root Decoders and Endpoint Decoders > ranges: both must start at physical address 0 and end below 4 GB, while > the Root Decoder ranges end at lower addresses than the matching Endpoint > Decoders which instead must always respect the above-mentioned CXL hardware > decoders HPA alignment constraint. > > Match misaligned CFMWS and CXL Regions with corresponding CXL Endpoint > Decoders if driver detects Low Memory Holes. > > Construct CXL Regions with HPA range's end trimmed by matching SPA. > > Allow the attach target process to complete by relaxing Decoder constraints > which would lead to failures. This may be crisper in 2 patches so that we can separate a) introducing a method to handle arch specific constraints in region creation from b) use method a) to handle the x86 LMH specific constraint. Sometimes with an add like this, we'll say your the first, so go for it, and the next one will have to generic-ize. This time, we are aware that other holes may be lurking, so perhaps we can driver that generic soln from the start. snip > > diff --git a/drivers/cxl/core/lmh.c b/drivers/cxl/core/lmh.c snip > +bool arch_match_spa(struct cxl_root_decoder *cxlrd, > + struct cxl_endpoint_decoder *cxled) > +{ > + struct range *r1, *r2; > + int niw; > + > + r1 = &cxlrd->cxlsd.cxld.hpa_range; > + r2 = &cxled->cxld.hpa_range; > + niw = cxled->cxld.interleave_ways; > + > + if (r1->start == MISALIGNED_CFMWS_RANGE_BASE && > + r1->start == r2->start && r1->end < r2->end && > + IS_ALIGNED(range_len(r2), niw * SZ_256M)) > + return true; space before final return please > + return false; > +} > + > +/* Similar to arch_match_spa(), it matches regions and decoders */ > +bool arch_match_region(struct cxl_region_params *p, > + struct cxl_decoder *cxld) > +{ > + struct range *r = &cxld->hpa_range; > + struct resource *res = p->res; > + int niw = cxld->interleave_ways; > + > + if (res->start == MISALIGNED_CFMWS_RANGE_BASE && > + res->start == r->start && res->end < r->end && > + IS_ALIGNED(range_len(r), niw * SZ_256M)) > + return true; space before final return please > + return false; > +} > + > +void arch_trim_hpa_by_spa(struct resource *res, > + struct cxl_root_decoder *cxlrd) > +{ > + res->end = cxlrd->res->end; > +} I'm not so sure about the above function name. trim_by_spa implies to me that the hpa is trimmed by the amount of the spa. The hpa is trimmed to align with the spa and that is special to this LMH case. Would something completely generic suffice here, like: arch_adjust_region_resource() > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c snip > @@ -3270,6 +3291,22 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, > > *res = DEFINE_RES_MEM_NAMED(hpa->start, range_len(hpa), > dev_name(&cxlr->dev)); > + > + /* > + * Trim the HPA retrieved from hardware to fit the SPA mapped by the > + * platform > + */ > + if (arch_match_spa(cxlrd, cxled)) { > + struct range *range = &cxlrd->cxlsd.cxld.hpa_range; > + > + arch_trim_hpa_by_spa(res, cxlrd); > + dev_dbg(cxlmd->dev.parent, > + "%s: Trim HPA (%s: %pr) by SPA (%s: %pr)\n", > + __func__, > + dev_name(&cxlrd->cxlsd.cxld.dev), range, > + dev_name(&cxled->cxld.dev), hpa); > + } no need for __func__ as the dev_dbg() includes that. %pr works for struct resource, like the HPA, but not struct range. I'm guessing you'll v2 after v6.13-rc1 is out and then you can use the new struct range print format Ira added. When I tried this out I spew'd more. I'm not suggesting you print all I printed but maybe find a hint in here of what someone debugging will want to know. Maybe some of the debug should emit from the the lmh.c function so that it can be more LMH special> [] cxl_core:construct_region:3302: cxl region0: LMH: res was [mem 0xf010000000-0xf04fffffff flags 0x200] [] cxl_core:construct_region:3306: cxl region0: LMH: res now trimmed to [mem 0xf010000000-0xf03fffffff flags 0x200] [] cxl_core:construct_region:3307: cxl region0: LMH: res now matches root decoder decoder0.0 [0xf010000000 - 0xf03fffffff] [] cxl_core:construct_region:3309: cxl region0: LMH: res does not match endpoint decoder decoder15.0 [0xf010000000 - 0xf04fffffff] I wonder if we are overdoing the HPA SPA language to the point of confusion (maybe just me). By this point we've decided the root decoder range is good so we are trimming the region resource (typically derived from the endpoint decoder range) to match the root decoder range. snip > +#ifdef CONFIG_CXL_ARCH_LOW_MEMORY_HOLE > +bool arch_match_spa(struct cxl_root_decoder *cxlrd, > + struct cxl_endpoint_decoder *cxled); > +bool arch_match_region(struct cxl_region_params *p, > + struct cxl_decoder *cxld); > +void arch_trim_hpa_by_spa(struct resource *res, > + struct cxl_root_decoder *cxlrd); > +#else > +bool arch_match_spa(struct cxl_root_decoder *cxlrd, > + struct cxl_endpoint_decoder *cxled) > +{ > + return false; > +} > + > +bool arch_match_region(struct cxl_region_params *p, > + struct cxl_decoder *cxld) > +{ > + return false; > +} > + > +void arch_trim_hpa_by_spa(struct resource *res, > + struct cxl_root_decoder *cxlrd) > +{ } > +#endif /* CXL_ARCH_LOW_MEMORY_HOLE */ Some longish lines are needlessly split. git clang-format can help with that. -- Alison snip to end. >
Hi Fabio, kernel test robot noticed the following build errors: [auto build test ERROR on cxl/next] [also build test ERROR on linus/master cxl/pending v6.12 next-20241125] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Fabio-M-De-Francesco/cxl-core-Change-match_-_by_range-calling-convention/20241125-102754 base: https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git next patch link: https://lore.kernel.org/r/20241122155226.2068287-3-fabio.m.de.francesco%40linux.intel.com patch subject: [PATCH 2/3] cxl/core: Enable Region creation on x86 with Low Memory Hole config: i386-buildonly-randconfig-001-20241125 (https://download.01.org/0day-ci/archive/20241126/202411260633.Oq6ps76M-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241126/202411260633.Oq6ps76M-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202411260633.Oq6ps76M-lkp@intel.com/ All errors (new ones prefixed by >>): ld: drivers/cxl/core/pmem.o: in function `arch_match_spa': >> drivers/cxl/cxl.h:923: multiple definition of `arch_match_spa'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:923: first defined here ld: drivers/cxl/core/pmem.o: in function `arch_match_region': >> drivers/cxl/cxl.h:927: multiple definition of `arch_match_region'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:927: first defined here ld: drivers/cxl/core/pmem.o: in function `arch_trim_hpa_by_spa': >> drivers/cxl/cxl.h:935: multiple definition of `arch_trim_hpa_by_spa'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:935: first defined here ld: drivers/cxl/core/regs.o: in function `arch_match_spa': >> drivers/cxl/cxl.h:923: multiple definition of `arch_match_spa'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:923: first defined here ld: drivers/cxl/core/regs.o: in function `arch_match_region': >> drivers/cxl/cxl.h:927: multiple definition of `arch_match_region'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:927: first defined here ld: drivers/cxl/core/regs.o: in function `arch_trim_hpa_by_spa': >> drivers/cxl/cxl.h:935: multiple definition of `arch_trim_hpa_by_spa'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:935: first defined here ld: drivers/cxl/core/memdev.o: in function `arch_match_spa': >> drivers/cxl/cxl.h:923: multiple definition of `arch_match_spa'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:923: first defined here ld: drivers/cxl/core/memdev.o: in function `arch_match_region': >> drivers/cxl/cxl.h:927: multiple definition of `arch_match_region'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:927: first defined here ld: drivers/cxl/core/memdev.o: in function `arch_trim_hpa_by_spa': >> drivers/cxl/cxl.h:935: multiple definition of `arch_trim_hpa_by_spa'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:935: first defined here ld: drivers/cxl/core/mbox.o: in function `arch_match_spa': >> drivers/cxl/cxl.h:923: multiple definition of `arch_match_spa'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:923: first defined here ld: drivers/cxl/core/mbox.o: in function `arch_match_region': >> drivers/cxl/cxl.h:927: multiple definition of `arch_match_region'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:927: first defined here ld: drivers/cxl/core/mbox.o: in function `arch_trim_hpa_by_spa': >> drivers/cxl/cxl.h:935: multiple definition of `arch_trim_hpa_by_spa'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:935: first defined here ld: drivers/cxl/core/pci.o: in function `arch_match_spa': >> drivers/cxl/cxl.h:923: multiple definition of `arch_match_spa'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:923: first defined here ld: drivers/cxl/core/pci.o: in function `arch_match_region': >> drivers/cxl/cxl.h:927: multiple definition of `arch_match_region'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:927: first defined here ld: drivers/cxl/core/pci.o: in function `arch_trim_hpa_by_spa': >> drivers/cxl/cxl.h:935: multiple definition of `arch_trim_hpa_by_spa'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:935: first defined here ld: drivers/cxl/core/hdm.o: in function `arch_match_spa': >> drivers/cxl/cxl.h:923: multiple definition of `arch_match_spa'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:923: first defined here ld: drivers/cxl/core/hdm.o: in function `arch_match_region': >> drivers/cxl/cxl.h:927: multiple definition of `arch_match_region'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:927: first defined here ld: drivers/cxl/core/hdm.o: in function `arch_trim_hpa_by_spa': >> drivers/cxl/cxl.h:935: multiple definition of `arch_trim_hpa_by_spa'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:935: first defined here ld: drivers/cxl/core/pmu.o: in function `arch_match_spa': >> drivers/cxl/cxl.h:923: multiple definition of `arch_match_spa'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:923: first defined here ld: drivers/cxl/core/pmu.o: in function `arch_match_region': >> drivers/cxl/cxl.h:927: multiple definition of `arch_match_region'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:927: first defined here ld: drivers/cxl/core/pmu.o: in function `arch_trim_hpa_by_spa': drivers/cxl/cxl.h:935: multiple definition of `arch_trim_hpa_by_spa'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:935: first defined here ld: drivers/cxl/core/cdat.o: in function `arch_match_spa': drivers/cxl/cxl.h:923: multiple definition of `arch_match_spa'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:923: first defined here ld: drivers/cxl/core/cdat.o: in function `arch_match_region': drivers/cxl/cxl.h:927: multiple definition of `arch_match_region'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:927: first defined here ld: drivers/cxl/core/cdat.o: in function `arch_trim_hpa_by_spa': drivers/cxl/cxl.h:935: multiple definition of `arch_trim_hpa_by_spa'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:935: first defined here ld: drivers/cxl/core/trace.o: in function `arch_match_spa': drivers/cxl/cxl.h:923: multiple definition of `arch_match_spa'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:923: first defined here ld: drivers/cxl/core/trace.o: in function `arch_match_region': drivers/cxl/cxl.h:927: multiple definition of `arch_match_region'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:927: first defined here ld: drivers/cxl/core/trace.o: in function `arch_trim_hpa_by_spa': drivers/cxl/cxl.h:935: multiple definition of `arch_trim_hpa_by_spa'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:935: first defined here ld: drivers/cxl/port.o: in function `arch_match_spa': drivers/cxl/cxl.h:923: multiple definition of `arch_match_spa'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:923: first defined here ld: drivers/cxl/port.o: in function `arch_match_region': drivers/cxl/cxl.h:927: multiple definition of `arch_match_region'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:927: first defined here ld: drivers/cxl/port.o: in function `arch_trim_hpa_by_spa': drivers/cxl/cxl.h:935: multiple definition of `arch_trim_hpa_by_spa'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:935: first defined here ld: drivers/perf/cxl_pmu.o: in function `arch_match_spa': drivers/perf/../cxl/cxl.h:923: multiple definition of `arch_match_spa'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:923: first defined here ld: drivers/perf/cxl_pmu.o: in function `arch_match_region': drivers/perf/../cxl/cxl.h:927: multiple definition of `arch_match_region'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:927: first defined here ld: drivers/perf/cxl_pmu.o: in function `arch_trim_hpa_by_spa': drivers/perf/../cxl/cxl.h:935: multiple definition of `arch_trim_hpa_by_spa'; drivers/cxl/core/port.o:drivers/cxl/cxl.h:935: first defined here Kconfig warnings: (for reference only) WARNING: unmet direct dependencies detected for GET_FREE_REGION Depends on [n]: SPARSEMEM [=n] Selected by [y]: - RESOURCE_KUNIT_TEST [=y] && RUNTIME_TESTING_MENU [=y] && KUNIT [=y] vim +923 drivers/cxl/cxl.h 912 913 #ifdef CONFIG_CXL_ARCH_LOW_MEMORY_HOLE 914 bool arch_match_spa(struct cxl_root_decoder *cxlrd, 915 struct cxl_endpoint_decoder *cxled); 916 bool arch_match_region(struct cxl_region_params *p, 917 struct cxl_decoder *cxld); 918 void arch_trim_hpa_by_spa(struct resource *res, 919 struct cxl_root_decoder *cxlrd); 920 #else 921 bool arch_match_spa(struct cxl_root_decoder *cxlrd, 922 struct cxl_endpoint_decoder *cxled) > 923 { 924 return false; 925 } 926 > 927 bool arch_match_region(struct cxl_region_params *p, 928 struct cxl_decoder *cxld) 929 { 930 return false; 931 } 932 933 void arch_trim_hpa_by_spa(struct resource *res, 934 struct cxl_root_decoder *cxlrd) > 935 { } 936 #endif /* CXL_ARCH_LOW_MEMORY_HOLE */ 937
On 11/26/2024 1:22 AM, Fabio M. De Francesco wrote: > On Monday, November 25, 2024 9:42:56 AM GMT+1 Li Ming wrote: >> >> On 11/22/2024 11:51 PM, 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. In some cases the size >>> of that hole is not compatible with the CXL hardware decoder constraint >>> that the size is always aligned to 256M * Interleave Ways. >>> >>> 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, cxl_add_to_region() fails and returns an error >>> because it can't find any CXL Window that matches a given CXL Endpoint >>> Decoder. >>> >>> Detect Low Memory Holes by comparing Root Decoders and Endpoint Decoders >>> ranges: both must start at physical address 0 and end below 4 GB, while >>> the Root Decoder ranges end at lower addresses than the matching Endpoint >>> Decoders which instead must always respect the above-mentioned CXL hardware >>> decoders HPA alignment constraint. >> >> Hi Fabio, >> >> Here mentions that the end address must be below 4GB, but I don't find any checking about that in the implementation, is it not needed? >> > Hi Ming, > > Good question, thanks! > > While a first version of arch_match_spa() had a check of 'r2->end < SZ_4G', > I dropped it for the final one. It ended up out of sync with the commit message. > > I think that we don't want that check. I'll rework the commit message for v2. > > If the hardware decoders HPA ranges extended beyond the end of Low > Memory, the LMH would still need to be detected and the decoders capacity > would still need to be trimmed to match their corresponding CFMWS range end. >> >> [snip] >> >> >>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c >>> index ac2c486c16e9..3cb5a69e9731 100644 >>> --- a/drivers/cxl/core/region.c >>> +++ b/drivers/cxl/core/region.c >>> @@ -836,8 +836,12 @@ static int match_auto_decoder(struct device *dev, void *data) >>> cxld = to_cxl_decoder(dev); >>> r = &cxld->hpa_range; >>> >>> - if (p->res && p->res->start == r->start && p->res->end == r->end) >>> - return 1; >>> + if (p->res) { >>> + if (p->res->start == r->start && p->res->end == r->end) >>> + return 1; >>> + if (arch_match_region(p, cxld)) >>> + return 1; >>> + } >> >> I think that it is better to check LMH cases before checking (p->res->start == r->start && p->res->end == r->end). >> Per the changelog and the implementation of arch_match_region(), below case should fails but current checking will succeed: >> the value of 'p->res->start' is MISALIGNED_CFMWS_RANGE_BASE and the the value of 'p->res->end' is equals to the value of 'r->end'. >> > I think that the expected "normal" case should always come first. In the expected > scenarios the driver deals either with SPA range == HPA range > (e.g, in match_auto_decoder()) or with SPA range that fully contains the HPA range > (match_decoder_by_range()). > > If either one of those two cases holds, arch_match_*() helper doesn't need to be > called and the match must succeed. > > Besides, other architectures are free to define holes with many constraints that > the driver doesn't want to check first if the expected case is already met. >> >>> >>> return 0; >>> } >>> @@ -1414,7 +1418,8 @@ static int cxl_port_setup_targets(struct cxl_port *port, >>> if (cxld->interleave_ways != iw || >>> cxld->interleave_granularity != ig || >>> cxld->hpa_range.start != p->res->start || >>> - cxld->hpa_range.end != p->res->end || >>> + (cxld->hpa_range.end != p->res->end && >>> + !arch_match_region(p, cxld)) || >>> ((cxld->flags & CXL_DECODER_F_ENABLE) == 0)) { >>> dev_err(&cxlr->dev, >>> "%s:%s %s expected iw: %d ig: %d %pr\n", >>> @@ -1726,6 +1731,7 @@ static int match_switch_decoder_by_range(struct device *dev, void *data) >>> { >>> struct cxl_endpoint_decoder *cxled = data; >>> struct cxl_switch_decoder *cxlsd; >>> + struct cxl_root_decoder *cxlrd; >>> struct range *r1, *r2; >>> >>> if (!is_switch_decoder(dev)) >>> @@ -1735,8 +1741,13 @@ static int match_switch_decoder_by_range(struct device *dev, void *data) >>> r1 = &cxlsd->cxld.hpa_range; >>> r2 = &cxled->cxld.hpa_range; >>> >>> - if (is_root_decoder(dev)) >>> - return range_contains(r1, r2); >>> + if (is_root_decoder(dev)) { >>> + if (range_contains(r1, r2)) >>> + return 1; >>> + cxlrd = to_cxl_root_decoder(dev); >>> + if (arch_match_spa(cxlrd, cxled)) >>> + return 1; >> >> Same as above, what will happen if the root decoder's address range still contains endpoint decoder's address range in LMH cases? should fails or succeed? >> > If r1 contains r2, there is no LMH and the driver is dealing with the regular, > expected, case. It must succeed. > > Think of the arch_match_*() helpers like something that avoid unwanted > failures if arch permits exceptions. Before returning errors when the "normal" > tests fail, check if the arch allows any exceptions and make the driver > ignore that "strange" SPA/HPA misalignment. >> >> [snip] >> > Thanks, > > Fabio > > Understand it, thanks for your explanation. Ming > > >
diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig index 876469e23f7a..07b87f217e59 100644 --- a/drivers/cxl/Kconfig +++ b/drivers/cxl/Kconfig @@ -128,6 +128,11 @@ config CXL_REGION If unsure say 'y' +config CXL_ARCH_LOW_MEMORY_HOLE + def_bool y + depends on CXL_REGION + depends on X86 + config CXL_REGION_INVALIDATION_TEST bool "CXL: Region Cache Management Bypass (TEST)" depends on CXL_REGION diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile index 9259bcc6773c..6e80215e8444 100644 --- a/drivers/cxl/core/Makefile +++ b/drivers/cxl/core/Makefile @@ -15,4 +15,5 @@ cxl_core-y += hdm.o cxl_core-y += pmu.o cxl_core-y += cdat.o cxl_core-$(CONFIG_TRACING) += trace.o +cxl_core-$(CONFIG_CXL_ARCH_LOW_MEMORY_HOLE) += lmh.o cxl_core-$(CONFIG_CXL_REGION) += region.o diff --git a/drivers/cxl/core/lmh.c b/drivers/cxl/core/lmh.c new file mode 100644 index 000000000000..da76b2a534ec --- /dev/null +++ b/drivers/cxl/core/lmh.c @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: GPL-2.0-only + +#include <linux/range.h> +#include "cxl.h" + +/* In x86 with memory hole, misaligned CFMWS range starts at 0x0 */ +#define MISALIGNED_CFMWS_RANGE_BASE 0x0 + +/* + * Match CXL Root and Endpoint Decoders by comparing SPA and HPA ranges. + * + * On x86, CFMWS ranges never intersect memory holes while endpoint decoders + * HPA range sizes are always guaranteed aligned to NIW * 256MB; therefore, + * the given endpoint decoder HPA range size is always expected aligned and + * also larger than that of the matching root decoder + */ +bool arch_match_spa(struct cxl_root_decoder *cxlrd, + struct cxl_endpoint_decoder *cxled) +{ + struct range *r1, *r2; + int niw; + + r1 = &cxlrd->cxlsd.cxld.hpa_range; + r2 = &cxled->cxld.hpa_range; + niw = cxled->cxld.interleave_ways; + + if (r1->start == MISALIGNED_CFMWS_RANGE_BASE && + r1->start == r2->start && r1->end < r2->end && + IS_ALIGNED(range_len(r2), niw * SZ_256M)) + return true; + return false; +} + +/* Similar to arch_match_spa(), it matches regions and decoders */ +bool arch_match_region(struct cxl_region_params *p, + struct cxl_decoder *cxld) +{ + struct range *r = &cxld->hpa_range; + struct resource *res = p->res; + int niw = cxld->interleave_ways; + + if (res->start == MISALIGNED_CFMWS_RANGE_BASE && + res->start == r->start && res->end < r->end && + IS_ALIGNED(range_len(r), niw * SZ_256M)) + return true; + return false; +} + +void arch_trim_hpa_by_spa(struct resource *res, + struct cxl_root_decoder *cxlrd) +{ + res->end = cxlrd->res->end; +} diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index ac2c486c16e9..3cb5a69e9731 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -836,8 +836,12 @@ static int match_auto_decoder(struct device *dev, void *data) cxld = to_cxl_decoder(dev); r = &cxld->hpa_range; - if (p->res && p->res->start == r->start && p->res->end == r->end) - return 1; + if (p->res) { + if (p->res->start == r->start && p->res->end == r->end) + return 1; + if (arch_match_region(p, cxld)) + return 1; + } return 0; } @@ -1414,7 +1418,8 @@ static int cxl_port_setup_targets(struct cxl_port *port, if (cxld->interleave_ways != iw || cxld->interleave_granularity != ig || cxld->hpa_range.start != p->res->start || - cxld->hpa_range.end != p->res->end || + (cxld->hpa_range.end != p->res->end && + !arch_match_region(p, cxld)) || ((cxld->flags & CXL_DECODER_F_ENABLE) == 0)) { dev_err(&cxlr->dev, "%s:%s %s expected iw: %d ig: %d %pr\n", @@ -1726,6 +1731,7 @@ static int match_switch_decoder_by_range(struct device *dev, void *data) { struct cxl_endpoint_decoder *cxled = data; struct cxl_switch_decoder *cxlsd; + struct cxl_root_decoder *cxlrd; struct range *r1, *r2; if (!is_switch_decoder(dev)) @@ -1735,8 +1741,13 @@ static int match_switch_decoder_by_range(struct device *dev, void *data) r1 = &cxlsd->cxld.hpa_range; r2 = &cxled->cxld.hpa_range; - if (is_root_decoder(dev)) - return range_contains(r1, r2); + if (is_root_decoder(dev)) { + if (range_contains(r1, r2)) + return 1; + cxlrd = to_cxl_root_decoder(dev); + if (arch_match_spa(cxlrd, cxled)) + return 1; + } return (r1->start == r2->start && r1->end == r2->end); } @@ -1943,7 +1954,7 @@ static int cxl_region_attach(struct cxl_region *cxlr, } if (resource_size(cxled->dpa_res) * p->interleave_ways != - resource_size(p->res)) { + resource_size(p->res) && !arch_match_spa(cxlrd, cxled)) { dev_dbg(&cxlr->dev, "%s:%s: decoder-size-%#llx * ways-%d != region-size-%#llx\n", dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), @@ -3199,7 +3210,13 @@ static int match_root_decoder_by_range(struct device *dev, void *data) r1 = &cxlrd->cxlsd.cxld.hpa_range; r2 = &cxled->cxld.hpa_range; - return range_contains(r1, r2); + if (range_contains(r1, r2)) + return true; + + if (arch_match_spa(cxlrd, cxled)) + return true; + + return false; } static int match_region_by_range(struct device *dev, void *data) @@ -3217,8 +3234,12 @@ static int match_region_by_range(struct device *dev, void *data) p = &cxlr->params; down_read(&cxl_region_rwsem); - if (p->res && p->res->start == r->start && p->res->end == r->end) - rc = 1; + if (p->res) { + if (p->res->start == r->start && p->res->end == r->end) + rc = 1; + if (arch_match_region(p, &cxled->cxld)) + rc = 1; + } up_read(&cxl_region_rwsem); return rc; @@ -3270,6 +3291,22 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd, *res = DEFINE_RES_MEM_NAMED(hpa->start, range_len(hpa), dev_name(&cxlr->dev)); + + /* + * Trim the HPA retrieved from hardware to fit the SPA mapped by the + * platform + */ + if (arch_match_spa(cxlrd, cxled)) { + struct range *range = &cxlrd->cxlsd.cxld.hpa_range; + + arch_trim_hpa_by_spa(res, cxlrd); + dev_dbg(cxlmd->dev.parent, + "%s: Trim HPA (%s: %pr) by SPA (%s: %pr)\n", + __func__, + dev_name(&cxlrd->cxlsd.cxld.dev), range, + dev_name(&cxled->cxld.dev), hpa); + } + rc = insert_resource(cxlrd->res, res); if (rc) { /* diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 5406e3ab3d4a..a5ad4499381e 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -902,6 +902,31 @@ void cxl_coordinates_combine(struct access_coordinate *out, bool cxl_endpoint_decoder_reset_detected(struct cxl_port *port); +#ifdef CONFIG_CXL_ARCH_LOW_MEMORY_HOLE +bool arch_match_spa(struct cxl_root_decoder *cxlrd, + struct cxl_endpoint_decoder *cxled); +bool arch_match_region(struct cxl_region_params *p, + struct cxl_decoder *cxld); +void arch_trim_hpa_by_spa(struct resource *res, + struct cxl_root_decoder *cxlrd); +#else +bool arch_match_spa(struct cxl_root_decoder *cxlrd, + struct cxl_endpoint_decoder *cxled) +{ + return false; +} + +bool arch_match_region(struct cxl_region_params *p, + struct cxl_decoder *cxld) +{ + return false; +} + +void arch_trim_hpa_by_spa(struct resource *res, + struct cxl_root_decoder *cxlrd) +{ } +#endif /* CXL_ARCH_LOW_MEMORY_HOLE */ + /* * Unit test builds overrides this to __weak, find the 'strong' version * of these symbols in tools/testing/cxl/.
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. In some cases the size of that hole is not compatible with the CXL hardware decoder constraint that the size is always aligned to 256M * Interleave Ways. 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, cxl_add_to_region() fails and returns an error because it can't find any CXL Window that matches a given CXL Endpoint Decoder. Detect Low Memory Holes by comparing Root Decoders and Endpoint Decoders ranges: both must start at physical address 0 and end below 4 GB, while the Root Decoder ranges end at lower addresses than the matching Endpoint Decoders which instead must always respect the above-mentioned CXL hardware decoders HPA alignment constraint. Match misaligned CFMWS and CXL Regions with corresponding CXL Endpoint Decoders if driver detects Low Memory Holes. Construct CXL Regions with HPA range's end trimmed by matching SPA. Allow the attach target process to complete by relaxing Decoder constraints which would lead to failures. 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/Kconfig | 5 ++++ drivers/cxl/core/Makefile | 1 + drivers/cxl/core/lmh.c | 53 +++++++++++++++++++++++++++++++++++++ drivers/cxl/core/region.c | 55 ++++++++++++++++++++++++++++++++------- drivers/cxl/cxl.h | 25 ++++++++++++++++++ 5 files changed, 130 insertions(+), 9 deletions(-) create mode 100644 drivers/cxl/core/lmh.c