Re: [PATCH v10 04/10] 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:
- [Medium] cxl/acpi: cxl_acpi_cfmws_verify() rejects Host-only + BI windows on QEMU 11.0+
- [Medium] cxl: stale cxlds->bi state on reset in cxl_region_decode_commit()
- [Low] cxl/acpi: unreachable third coherency check in cxl_acpi_cfmws_verify()
--

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

cxl: Add HDM-DB region creation

This commit adds support for creating HDM-DB (device-coherent with
back-invalidate) CXL regions via sysfs and auto-assembly, introduces
CFMWS coherency verification in cxl_acpi_cfmws_verify(), and exposes the
cap_back_invalidate and back_invalidate sysfs attributes.

> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index b7ce6b59c680b..cc7c99d3066d8 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -152,6 +152,8 @@ static unsigned long cfmws_to_decoder_flags(int restrictions)
>  		flags |= CXL_DECODER_F_PMEM;
>  	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_FIXED)
>  		flags |= CXL_DECODER_F_LOCK;
> +	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_BI)
> +		flags |= CXL_DECODER_F_BI;
>  
>  	return flags;
>  }
> @@ -198,6 +200,24 @@ static int cxl_acpi_cfmws_verify(struct device *dev,
>  		dev_dbg(dev, "CFMWS length %d greater than expected %d\n",
>  			cfmws->header.length, expected_len);
>  
> +	if ((cfmws->restrictions & ACPI_CEDT_CFMWS_RESTRICT_HOSTONLYMEM) &&
> +	    (cfmws->restrictions & ACPI_CEDT_CFMWS_RESTRICT_BI)) {
> +		dev_err(dev, "CFMWS cannot have both HDM-H and HDM-DB\n");
> +		return -EINVAL;
> +	}

[Severity: Medium]
As reported by Alison Schofield on v9 [1], does rejecting the entire CFMWS
window in cxl_acpi_cfmws_verify() when both ACPI_CEDT_CFMWS_RESTRICT_HOSTONLYMEM
and ACPI_CEDT_CFMWS_RESTRICT_BI are set cause a regression on released QEMU
v11.0+ systems?

During cxl_acpi_probe() -> acpi_table_parse_cedt() -> cxl_parse_cfmws() ->
__cxl_parse_cfmws() -> cxl_acpi_cfmws_verify(), returning -EINVAL for this
combination aborts root decoder registration completely. Because released
QEMU versions from v11.0 onward set both restriction bits by default, CXL
windows are rejected at boot and manual HDM-H region creation is lost on
windows where BI was previously ignored.

Could cfmws_to_decoder_flags() withhold CXL_DECODER_F_BI when
ACPI_CEDT_CFMWS_RESTRICT_HOSTONLYMEM is set (or when
ACPI_CEDT_CFMWS_RESTRICT_DEVMEM is not set) instead of failing the entire
window in cxl_acpi_cfmws_verify()?

> +
> +	if (!(cfmws->restrictions & (ACPI_CEDT_CFMWS_RESTRICT_DEVMEM |
> +				     ACPI_CEDT_CFMWS_RESTRICT_HOSTONLYMEM))) {
> +		dev_err(dev, "CFMWS has no coherency model\n");
> +		return -EINVAL;
> +	}
> +
> +	if ((cfmws->restrictions & ACPI_CEDT_CFMWS_RESTRICT_BI) &&
> +	    !(cfmws->restrictions & ACPI_CEDT_CFMWS_RESTRICT_DEVMEM)) {
> +		dev_err(dev, "CFMWS BI requires device-coherent\n");
> +		return -EINVAL;
> +	}

[Severity: Low]
Can this third check in cxl_acpi_cfmws_verify() ever be reached?

For (cfmws->restrictions & ACPI_CEDT_CFMWS_RESTRICT_BI) &&
!(cfmws->restrictions & ACPI_CEDT_CFMWS_RESTRICT_DEVMEM) to evaluate to
true, ACPI_CEDT_CFMWS_RESTRICT_HOSTONLYMEM must be either set or clear:

- If ACPI_CEDT_CFMWS_RESTRICT_HOSTONLYMEM is set, the first check
  ((cfmws->restrictions & ACPI_CEDT_CFMWS_RESTRICT_HOSTONLYMEM) &&
  (cfmws->restrictions & ACPI_CEDT_CFMWS_RESTRICT_BI)) already returns
  -EINVAL.

- If ACPI_CEDT_CFMWS_RESTRICT_HOSTONLYMEM is clear (with
  ACPI_CEDT_CFMWS_RESTRICT_DEVMEM also clear), the second check
  (!(cfmws->restrictions & (ACPI_CEDT_CFMWS_RESTRICT_DEVMEM |
  ACPI_CEDT_CFMWS_RESTRICT_HOSTONLYMEM))) already logs "CFMWS has no
  coherency model" and returns -EINVAL.

> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index a048bd5c06cb6..28c61feef06d2 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -313,8 +313,25 @@ static int commit_decoder(struct cxl_decoder *cxld)
>  static int cxl_region_decode_commit(struct cxl_region *cxlr)
>  {
>  	struct cxl_region_params *p = &cxlr->params;
> +	bool hdm_db;
>  	int i, rc = 0;
>  
> +	hdm_db = cxlr->type == CXL_DECODER_DEVMEM &&
> +		 cxl_root_decoder_is_bi(cxlr->cxlrd);
> +
> +	/* a reset since attach may have invalidated cxlds->bi */
> +	for (i = 0; i < p->nr_targets; i++) {
> +		struct cxl_endpoint_decoder *cxled = p->targets[i];
> +		struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> +
> +		if (hdm_db && !cxlmd->cxlds->bi) {
> +			dev_err(&cxlr->dev, "%s:%s BI not enabled on device\n",
> +				dev_name(&cxlmd->dev),
> +				dev_name(&cxled->cxld.dev));
> +			return -ENXIO;
> +		}
> +	}
> +
>  	for (i = 0; i < p->nr_targets; i++) {
>  		struct cxl_endpoint_decoder *cxled = p->targets[i];
>  		struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> @@ -346,6 +363,21 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
>  			cxled->cxld.reset(&cxled->cxld);
>  			goto err;
>  		}
> +
> +		/*
> +		 * A reset after the check above wipes BI Enable, but
> +		 * cxlds->bi is only refreshed from .reset_done, which may
> +		 * not have run. Ask the hardware.
> +		 */
> +		if (hdm_db && !cxl_bi_decoder_enabled(cxled_to_port(cxled))) {
> +			dev_err(&cxlr->dev,
> +				"%s:%s BI disabled by reset during commit\n",
> +				dev_name(&cxlmd->dev),
> +				dev_name(&cxled->cxld.dev));
> +			rc = -ENXIO;
> +			i++; /* this target committed, so undo it too */
> +			goto err;
> +		}

[Severity: Medium]
When a reset clears CXL_BI_DECODER_CTRL_BI_ENABLE in hardware without
.reset_done (cxl_bi_reset_detected()) having updated cxlmd->cxlds->bi (for
example, during a concurrent reset before .reset_done runs, a secondary bus
reset that does not invoke .reset_done, or on a Type-2 device driver that
does not call cxl_bi_reset_detected()), doesn't this leave cxlmd->cxlds->bi
stale and transiently commit decoders before rolling them back?

First, in the pre-commit loop at the top of cxl_region_decode_commit(), only
the software flag !cxlmd->cxlds->bi is checked rather than
cxl_bi_decoder_enabled(cxled_to_port(cxled)). If hardware BI Enable was
already cleared while cxlmd->cxlds->bi is still true,
cxl_region_decode_commit() programs and commits the endpoint and switch HDM
decoders in hardware with CXL_HDM_DECODER0_CTRL_BI set before catching the
mismatch in the post-commit check and rolling back via
cxl_region_decode_reset().

Second, when the post-commit check detects
!cxl_bi_decoder_enabled(cxled_to_port(cxled)), it does not clear
cxlmd->cxlds->bi = false. If .reset_done never runs, cxlmd->cxlds->bi
remains true indefinitely: back_invalidate_show() in
drivers/cxl/core/port.c continues to report 1 while the decoder is attached,
cxl_region_check_coherency() continues to allow attaching the endpoint to
HDM-DB regions, subsequent commit_store() calls repeat the hardware commit
and rollback cycle, and cxl_bi_release() attempts to disable BI on teardown.

Should the pre-commit check also verify
cxl_bi_decoder_enabled(cxled_to_port(cxled)), and should cxlmd->cxlds->bi be
updated when hardware BI Enable is found cleared?

[1] https://lore.kernel.org/r/ar3TU4IM_TtcGgwl@aschofie-mobl2.lan

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793567979.git.dave@stgolabs.net?part=4