Re: [PATCH v7 8/8] cxl: Allow auto-committed BI hdm decoders

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Auto-committed BI decoders can be improperly assembled into regions even if BI path setup fails, leading to fatal hardware routing errors or cache incoherence.
--

--- Patch [8]: [PATCH v7 8/8] cxl: Allow auto-committed BI hdm decoders ---
commit 5bfa0978968677982ba4863deb2a8257dcfab9b1
Author: Davidlohr Bueso <[email protected]>

cxl: Allow auto-committed BI hdm decoders

This commit allows auto-committed BI HDM decoders on capable platforms.
It ensures that committed BI decoders are not assembled into regions
with mismatched coherency models or non-BI windows.

> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1839,6 +1854,16 @@ static int cxl_region_attach_auto(struct cxl_region *cxlr,
>  		return -EINVAL;
>  	}
>  
> +	/* A committed decoder may only join a region of its own flavor */
> +	if (cxled_committed_bi(cxled) !=
> +	    (cxlr->type == CXL_DECODER_DEVMEM &&
> +	     cxl_root_decoder_is_bi(cxlr->cxlrd))) {
> +		dev_err(&cxlr->dev, "%s:%s coherency model mismatch\n",
> +			dev_name(&cxled_to_memdev(cxled)->dev),
> +			dev_name(&cxled->cxld.dev));
> +		return -ENXIO;
> +	}
> +

[ ... ]

> @@ -3806,6 +3831,14 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>  	if (part < 0)
>  		return ERR_PTR(-EBUSY);
>  
> +	/* avoid UB */
> +	if (cxled_committed_bi(cxled) && !cxl_root_decoder_is_bi(cxlrd)) {
> +		dev_err(cxlmd->dev.parent,
> +			"%s:%s BI decoder in a non-BI window\n",
> +			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev));
> +		return ERR_PTR(-ENXIO);
> +	}
> +

[ ... ]

> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -181,6 +181,10 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
>  	if (rc)
>  		return rc;
>  
> +	/*
> +	 * Must precede region discovery so that any firmware-committed BI
> +	 * decoder is adopted before its region is assembled.
> +	 */
>  	rc = cxl_bi_setup(port);
>  	if (rc)
>  		dev_dbg(&port->dev, "BI setup failed rc=%d\n", rc);

[Severity: High]
If cxl_bi_setup() fails in cxl_endpoint_port_probe() (for example, due to
register read/write failures on intermediate switches), the error is only
logged and probe continues.

Since cxl_region_attach_auto() and construct_region() authorize the decoder
to join a BI region based on cxled_committed_bi() checking the hardware state
instead of checking the software state (cxlds->bi), could an auto-committed
BI decoder be improperly assembled into a region even if its path setup failed?

Does this risk fatal hardware routing errors or silent cache incoherence if
the OS failed to properly configure or adopt the BI path?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8