Re: [PATCH v16 13/18] cxl: Validate and synchronize HDM ranges around reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] cxl: unpropagated normalized addressing flag in cxl_hdm_ranges_collect()
- [High] cxl: stale cache state and unlocked pre-checks in cxl_reset_function()
- [Low] cxl: missing context lock annotations on cxl_region_quiesce_lock()
--

commit 2ab2ba1a8c9580e8171daa685fd9f57270c18ea2
Author: Srirangan Madhavan <smadhavan@nvidia.com>

cxl: Validate and synchronize HDM ranges around reset

This commit validates and reserves enabled system-physical HDM ranges and
invalidates CPU caches before and after a CXL reset. It also holds
cxl_rwsem.region across snapshot, reset, and restoration while rejecting
resets on devices using normalized addressing or DVSEC range decoding.

> diff --git a/drivers/cxl/core/hdm_state.c b/drivers/cxl/core/hdm_state.c
> index 1140006de50d9..cb7ec0f86ab15 100644
> --- a/drivers/cxl/core/hdm_state.c
> +++ b/drivers/cxl/core/hdm_state.c

[ ... ]

> @@ -132,3 +139,239 @@ void cxl_hdm_cache_release(struct cxl_hdm_info **slot)
>  	}
>  	kfree(info);
>  }
> +
> +/**
> + * cxl_hdm_cache_dvsec_decode() - check for memory decoded by DVSEC ranges
> + * @slot: cache pointer owned by the device
> + *
> + * Return: true if the published state shows CXL.mem enabled while HDM
> + * decoders are globally disabled, so that the device decodes memory through
> + * its DVSEC ranges, which the cache does not describe.
> + */
> +bool cxl_hdm_cache_dvsec_decode(struct cxl_hdm_info *const *slot)
> +{
> +	struct cxl_hdm_info *info;
> +
> +	guard(rwsem_read)(&cxl_rwsem.dpa);
> +	info = *slot;
> +	return info && (info->dvsec_ctrl & PCI_DVSEC_CXL_MEM_ENABLE) &&
> +	       !(info->global_ctrl & CXL_HDM_DECODER_ENABLE);
> +}

[Severity: High]
Can info->dvsec_ctrl and info->global_ctrl be stale relative to the hardware
state when cxl_reset_function() calls cxl_hdm_cache_dvsec_decode()?

First, cxl_reset_function() reads the live PCI_DVSEC_CXL_CTRL register into
ctrl and confirms PCI_DVSEC_CXL_MEM_ENABLE is set, but
cxl_hdm_cache_dvsec_decode() checks the cached info->dvsec_ctrl instead.
info->dvsec_ctrl is only populated during PCI enumeration in
pci_cxl_hdm_cache_init() or updated via cxl_set_mem_enable(). If
PCI_DVSEC_CXL_MEM_ENABLE was clear at enumeration and later enabled without
cxl_set_mem_enable(), or if cxl_hdm_decode_init() takes the active DVSEC
range return path without calling devm_cxl_enable_mem() (where
devm_cxl_enumerate_decoders() also skips cxl_hdm_refresh_decoder() because
emulated is true):

drivers/cxl/core/pci.c:cxl_hdm_decode_init() {
	...
	if (allowed && info->mem_enabled)
		return 0;
	...
}

then info->dvsec_ctrl remains 0 while the device actively decodes memory
through DVSEC ranges, causing cxl_hdm_cache_dvsec_decode() to return false
and allowing the reset to proceed without quiescing those ranges.

Second, neither devm_cxl_enable_hdm() nor its devres teardown callback
disable_hdm() in drivers/cxl/core/pci.c updates pdev->hdm->global_ctrl:

drivers/cxl/core/pci.c:disable_hdm() {
	...
	global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
	writel(global_ctrl & ~CXL_HDM_DECODER_ENABLE,
	       hdm + CXL_HDM_DECODER_CTRL_OFFSET);
}

When cxl_port unbinds, disable_hdm() clears CXL_HDM_DECODER_ENABLE in
hardware while info->global_ctrl keeps CXL_HDM_DECODER_ENABLE set in the
cache. Conversely, during cxl_endpoint_port_probe(), devm_cxl_enable_mem()
sets PCI_DVSEC_CXL_MEM_ENABLE in info->dvsec_ctrl before
devm_cxl_enumerate_decoders() refreshes info->global_ctrl.

Finally, cxl_reset_function() calls cxl_hdm_cache_present() and
cxl_hdm_cache_dvsec_decode() in separate cxl_rwsem.dpa read-lock sections
before cxl_reset_execute() acquires cxl_region_quiesce_lock(), and neither
cxl_hdm_cache_snapshot() nor cxl_hdm_quiesce_prepare() re-checks the
captured snapshot for DVSEC decode mode while cxl_endpoint_port_probe() and
port unbind run under device_lock(&port->dev) without holding
cxl_rwsem.region.

[ ... ]

> +static int cxl_hdm_ranges_collect(struct cxl_hdm_quiesce *q,
> +				  const struct cxl_hdm_info *snapshot)
> +{
> +	int rc;
> +
> +	for (int i = 0; i < snapshot->decoder_count; i++) {
> +		const struct cxl_decoder_config *config =
> +			&snapshot->settings[i].config;
> +
> +		/* A committed zero-size decoder maps no HPA. */
> +		if (!(config->flags & CXL_DECODER_F_ENABLE) ||
> +		    !range_len(&config->hpa_range))
> +			continue;
> +
> +		if (config->flags & CXL_DECODER_F_NORMALIZED_ADDRESSING)
> +			return -EOPNOTSUPP;

[Severity: High]
Can CXL_DECODER_F_NORMALIZED_ADDRESSING ever be set in
snapshot->settings[i].config.flags here?

CXL_DECODER_F_NORMALIZED_ADDRESSING is a software flag rather than a
hardware HDM register bit, so pci_cxl_hdm_cache_init() ->
cxl_hdm_read_state() -> cxl_hdm_unpack_decoder() never sets it during PCI
enumeration.

When the endpoint port driver probes in cxl_endpoint_port_probe(), it first
runs devm_cxl_endpoint_decoders_setup() -> devm_cxl_enumerate_decoders(),
which refreshes pdev->hdm before region discovery runs:

drivers/cxl/port.c:cxl_endpoint_port_probe() {
	...
	rc = devm_cxl_endpoint_decoders_setup(port);
	if (rc)
		return rc;
	...
	device_for_each_child(&port->dev, NULL, discover_region);
	...
}

CXL_DECODER_F_NORMALIZED_ADDRESSING is only set later during
discover_region() -> cxl_add_to_region() -> get_cxl_root_decoder() ->
cxl_root_setup_translation() -> cxl_prm_setup_root():

drivers/cxl/core/atl.c:cxl_prm_setup_root() {
	...
	cxld->config.flags |= CXL_DECODER_F_LOCK;
	cxld->config.flags |= CXL_DECODER_F_NORMALIZED_ADDRESSING;
	...
}

Because nothing calls cxl_hdm_refresh_decoder() after cxl_prm_setup_root()
sets that flag (and the auto-discovered decoder is already enabled and
locked, so cxl_decoder_commit() and cxl_decoder_reset() will not run on it),
pdev->hdm->settings[i].config.flags never receives
CXL_DECODER_F_NORMALIZED_ADDRESSING. Would a reset on a
normalized-addressing endpoint bypass this -EOPNOTSUPP check and instead
reserve and invalidate the untranslated normalized address range?

[ ... ]

> diff --git a/include/cxl/hdm.h b/include/cxl/hdm.h
> index cfb55b10f8acf..7919d872a34de 100644
> --- a/include/cxl/hdm.h
> +++ b/include/cxl/hdm.h

[ ... ]

> +void cxl_region_quiesce_lock(void);
> +void cxl_region_quiesce_unlock(void);
> +int cxl_hdm_quiesce_prepare(struct cxl_hdm_quiesce *q,
> +			    const struct cxl_hdm_info *snapshot)
> +	__must_hold(cxl_region_quiesce);
> +int cxl_hdm_quiesce_flush(struct cxl_hdm_quiesce *q)
> +	__must_hold(cxl_region_quiesce);
> +void cxl_hdm_quiesce_destroy(struct cxl_hdm_quiesce *q);

[Severity: Low]
Should the declarations of cxl_region_quiesce_lock() and
cxl_region_quiesce_unlock() in include/cxl/hdm.h carry the
__acquires(cxl_region_quiesce) and __releases(cxl_region_quiesce)
annotations that are present on their definitions in
drivers/cxl/core/hdm_state.c, and should __cxl_reset_execute() in
drivers/pci/cxl.c also be annotated with __must_hold(cxl_region_quiesce)?

Without those annotations visible in drivers/pci/cxl.c, context analysis
(-Wthread-safety) cannot see that cxl_reset_execute() acquires
cxl_region_quiesce before calling cxl_reset_quiesced(), or that
__cxl_reset_execute() holds cxl_region_quiesce when calling
cxl_hdm_quiesce_flush().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029145703.2185231-1-smadhavan@nvidia.com?part=13