Re: [PATCH v5 4/5] cxl: Add HDM-DB region creation

On Mon, 15 Jun 2026, [email protected] wrote:

>Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>- [High] Auto-discovered regions fail to inherit their coherency type from the root decoder, violating the design rule and potentially creating invalid region configurations.

False positive. As is already documented in the cover letter and patch 3,
a decoder committed with the BI bit set is rejected at enumeration time.

>- [Medium] Spurious `dev_WARN_ONCE` triggers in `cxl_rr_assign_decoder()` when attaching an endpoint decoder with a stale `DEVMEM` target type to an HDM-H (non-BI) region.

Yes, I was able to trigger this at least on qemu with two windows:
     https://github.com/davidlohr/cxl-bi-tests/blob/main/topos/t15-decoder-reuse.sh

See the fix below.

>- [Medium] `bi_show` incorrectly reports that an auto-discovered endpoint decoder is configured for HDM-DB (BI=1) when it is operating with BI=0 in hardware.

Yes. An auto-discovered HDM-D on BI device can make this show bogus
1 afaict. Adding a check for the region's root decoder bi addresses
this and also makes the check more robust: (&& cxl_root_decoder_is_bi()).

Another alternative is to cache the BI bit upon committing, but imo this
file should show 1 as soon as it is attached successfully.

>--
>
>commit 913c2a59fed5b8381543ceb7345cd15ac2ea9d17
>Author: Davidlohr Bueso <[email protected]>
>
>cxl: Add HDM-DB region creation
>
>A region inherits its coherency from the chosen root decoder: HDM-DB
>if the root has CXL_DECODER_F_BI, otherwise HDM-H.
>
>[ ... ]
>
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index c5aacd7054f1d..71801dfd614b1 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>
>[ ... ]
>
>> @@ -233,6 +234,19 @@ static ssize_t mode_store(struct device *dev, struct device_attribute *attr,
>>  }
>>  static DEVICE_ATTR_RW(mode);
>>
>> +static ssize_t bi_show(struct device *dev, struct device_attribute *attr,
>> +		       char *buf)
>> +{
>> +	struct cxl_endpoint_decoder *cxled = to_cxl_endpoint_decoder(dev);
>> +	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
>> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>> +
>> +	guard(rwsem_read)(&cxl_rwsem.region);
>> +	return sysfs_emit(buf, "%d\n", cxlds->bi && cxled->cxld.region &&
>> +			  cxled->cxld.target_type == CXL_DECODER_DEVMEM);
>> +}
>
>[Severity: Medium]
>Will this attribute show '1' for an auto-discovered DEVMEM endpoint decoder
>even if it operates with BI=0 in hardware?
>
>If Linux auto-discovers a DEVMEM decoder that intrinsically supports BI
>(cxlds->bi is true), init_hdm_decoder() currently rejects it if the BI
>control bit is set, so the hardware must be operating with BI=0. However,
>this attribute seems to evaluate the state dynamically based on the device
>capability rather than the hardware's actual BI configuration, contradicting
>the sysfs documentation that says it shows '1' when "currently configured".
>
>> +static DEVICE_ATTR_RO(bi);
>
>[ ... ]
>
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index e50dc716d4e82..6eb852b514d3a 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -1132,11 +1132,13 @@ static int cxl_rr_assign_decoder(struct cxl_port *port, struct cxl_region *cxlr,
>>	/*
>>	 * Endpoints should already match the region type, but backstop that
>>	 * assumption with an assertion. Switch-decoders change mapping-type
>> -	 * based on what is mapped when they are assigned to a region.
>> +	 * based on what is mapped when they are assigned to a region. HDM-DB
>> +	 * endpoint decoders inherit their type from cxlr->type below.
>>	 */
>>	dev_WARN_ONCE(&cxlr->dev,
>>		      port == cxled_to_port(cxled) &&
>> -			      cxld->target_type != cxlr->type,
>> +			      cxld->target_type != cxlr->type &&
>> +			      !cxl_root_decoder_is_bi(cxlr->cxlrd),
>>		      "%s:%s mismatch decoder type %d -> %d\n",
>>		      dev_name(&cxled_to_memdev(cxled)->dev),
>>		      dev_name(&cxld->dev), cxld->target_type, cxlr->type);
>
>[Severity: Medium]
>Does this logic trigger a spurious dev_WARN_ONCE() when attaching an
>endpoint decoder with a stale DEVMEM target type to an HDM-H region?
>
>When a region is dismantled, __cxl_decoder_detach() clears the decoder's
>region assignment but doesn't reset its target_type, leaving it with a
>stale DEVMEM target type. If it is later attached to an HDM-H (non-BI)
>region, the check for cxld->target_type != cxlr->type evaluates to true,
>and the !cxl_root_decoder_is_bi(cxlr->cxlrd) check also evaluates to true.
>
>Could this generate a spurious warning stack trace in dmesg during valid
>user reconfigurations, potentially causing a panic if panic_on_warn is set?

I ended up introducing a default type helper:

+/*
+ * HDMs that advertise support for both coherency modes
+ * (CXL_HDM_DECODER_COHERENCY_BOTH) default to host-only; the region
+ * attach path switches target_type to device-coherent if the region's
+ * root decoder has the CFMWS BI bit set. Only HDMs that strictly
+ * support device-coherent mode default to HDM-DB.
+ */
+enum cxl_decoder_type cxled_default_type(struct cxl_endpoint_decoder *cxled)
+{
+       struct cxl_dev_state *cxlds = cxled_to_memdev(cxled)->cxlds;
+       struct cxl_port *port = cxled_to_port(cxled);
+       struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
+
+       if (cxlds->type == CXL_DEVTYPE_CLASSMEM &&
+           cxlhdm->supported_coherency != CXL_HDM_DECODER_COHERENCY_DEV)
+               return CXL_DECODER_HOSTONLYMEM;
+
+       return CXL_DECODER_DEVMEM;
+}

... then use it in both init_hdm_decoder() (which we do already) and
__cxl_decoder_detach(), to reset.

>
>[ ... ]
>
>> @@ -2772,6 +2796,7 @@ static ssize_t create_region_store(struct device *dev, const char *buf,
>>				   size_t len, enum cxl_partition_mode mode)
>>  {
>>	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev);
>> +	enum cxl_decoder_type target_type;
>>	struct cxl_region *cxlr;
>>	int rc, id;
>>
>> @@ -2779,7 +2804,14 @@ static ssize_t create_region_store(struct device *dev, const char *buf,
>>	if (rc != 1)
>>		return -EINVAL;
>>
>> -	cxlr = __create_region(cxlrd, mode, id, CXL_DECODER_HOSTONLYMEM);
>> +	/*
>> +	 * The CFMWS dictates endpoint coherency: a BI-restricted Window
>> +	 * produces an HDM-DB region; otherwise HDM-H.
>> +	 */
>> +	target_type = cxl_root_decoder_is_bi(cxlrd) ?
>> +		CXL_DECODER_DEVMEM : CXL_DECODER_HOSTONLYMEM;
>> +
>> +	cxlr = __create_region(cxlrd, mode, id, target_type);
>
>[Severity: High]
>Does the auto-discovery path in construct_region() still blindly create
>regions using the endpoint decoder's type?
>
>The commit message dictates a strict design rule that a region inherits its
>coherency from the chosen root decoder. The sysfs creation path correctly
>enforces this here, but construct_region() appears to miss this update:
>
>drivers/cxl/core/region.c:construct_region() {
>    ...
>		cxlr = __create_region(cxlrd, cxlds->part[part].mode,
>				       atomic_read(&cxlrd->region_id),
>				       cxled->cxld.target_type);
>    ...
>}
>
>If firmware incorrectly programs an endpoint decoder as DEVMEM under an HDM-H
>root decoder, could construct_region() create a DEVMEM region? Because both
>the region and the endpoint have target_type == DEVMEM, the mismatch warning
>in cxl_rr_assign_decoder() would be bypassed, resulting in an invalid HDM-DB
>region under a root decoder that cannot generate Back-Invalidates.
>
>>	if (IS_ERR(cxlr))
>>		return PTR_ERR(cxlr);
>
>--
>Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4