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

[sashiko-bot@kernel.org]

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.
- [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.
- [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.
--

commit 913c2a59fed5b8381543ceb7345cd15ac2ea9d17
Author: Davidlohr Bueso <dave@stgolabs.net>

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?

[ ... ]

> @@ -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/20260615145529.13848-1-dave@stgolabs.net?part=4