linux-kernel - Re: [PATCH 2/3] cxl/core: Enable Region creation on x86 with Low Memory Hole

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <VI1PR10MB20168EDC5126CC53204B44E0CE2E2@VI1PR10MB2016.EURPRD10.PROD.OUTLOOK.COM>
Date: Mon, 25 Nov 2024 19:23:07 +0800
From: Li Ming <ming4.li@...look.com>
To: Ira Weiny <ira.weiny@...el.com>,
 "Fabio M. De Francesco" <fabio.m.de.francesco@...ux.intel.com>,
 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>,
 Dan Williams <dan.j.williams@...el.com>, Huang Ying <ying.huang@...el.com>,
 Yao Xingtao <yaoxt.fnst@...itsu.com>, Li Ming <ming4.li@...el.com>,
 linux-kernel@...r.kernel.org, linux-cxl@...r.kernel.org
Subject: Re: [PATCH 2/3] cxl/core: Enable Region creation on x86 with Low
 Memory Hole



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@...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>
> 
> 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]