Re: [PATCH v17 18/19] PCI/CXL: Flush host and device caches for CXL bus reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] PCI/CXL: cxl_restore_state_after_pci_reset() enables cache before flush
- [Medium] PCI/CXL: cxl_reset_quiesced() restores caching and skips disable on error
- [Medium] PCI/CXL: cxl_reset_bus_function() disables device on pre-reset -ENOTTY
- [Medium] PCI/CXL: TOCTOU race on snapshot in pci_cxl_reset_prepare()
- [Low] PCI/CXL: missing cxl_region_quiesce context annotations on reset helpers
- [Low] cxl: unused cxl_hdm_cache_copy_decoders() after bus reset quiesce change
--

commit 6351634e948c8a95d981c82600454247642192c2
Author: Srirangan Madhavan <smadhavan@nvidia.com>

PCI/CXL: Flush host and device caches for CXL bus reset

This commit adds host and device cache flushing, HPA range reservation, and
CXL region quiesce locking around CXL bus resets when cached HDM state is
present. It also preserves and restores the device cache policy across the
reset and leaves the device disabled if recovery fails after an attempted
reset.

> diff --git a/drivers/pci/cxl.c b/drivers/pci/cxl.c
> index a9aa2e1c631d1..c5cdad4b05906 100644
> --- a/drivers/pci/cxl.c
> +++ b/drivers/pci/cxl.c
> @@ -393,74 +393,174 @@ static int pci_cxl_restore_state(struct pci_dev *pdev,
>  	return cxl_restore_state(pdev, snapshot);
>  }
>  
> +static int cxl_reset_restore_cache_policy(struct pci_dev *pdev, int dvsec,
> +					  u16 saved_ctrl2);
> +static int cxl_reset_cache_prepare(struct pci_dev *pdev, int dvsec, u16 cap,
> +				   const struct cxl_hdm_info *snapshot,
> +				   struct cxl_pci_cache *cache);

[ ... ]

> +struct cxl_hdm_info *pci_cxl_reset_prepare(struct pci_dev *pdev,
> +					struct cxl_pci_cache **cachep)
>  {
>  	struct cxl_hdm_info *snapshot;
> +	struct cxl_pci_cache *cache;
> +	u16 dvsec, cap, command;
> +	int rc;
>  
>  	device_lock_assert(&pdev->dev);
> -	snapshot = cxl_hdm_cache_snapshot(&pdev->hdm);
> -	if (IS_ERR(snapshot))
> -		return PTR_ERR(snapshot) == -ENXIO ? NULL : snapshot;
> +	*cachep = NULL;
> +	if (!pdev->hdm)
> +		return NULL;
>  
> +	rc = cxl_pci_device_dvsec(pdev, &dvsec, &cap);
> +	if (rc == -ENOTTY)
> +		return ERR_PTR(-ENODEV);
> +	if (rc)
> +		return ERR_PTR(rc);
>  	/* Raw reset callers may not have saved the PCI state needed for restore. */
>  	if (!pdev->state_saved) {
>  		pci_err(pdev, "CXL bus reset requires saved PCI state\n");
> -		kfree(snapshot);
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> +	cache = kzalloc_obj(*cache);
> +	if (!cache)
> +		return ERR_PTR(-ENOMEM);
> +	cxl_region_quiesce_lock();
> +	snapshot = cxl_hdm_cache_snapshot(&pdev->hdm);
> +	if (IS_ERR(snapshot)) {
> +		rc = PTR_ERR(snapshot);
> +		goto unlock;
> +	}
> +	if (!snapshot->dvsec_ctrl_valid) {
> +		rc = -ENXIO;
> +		goto free_snapshot;
> +	}
> +	if (cxl_hdm_cache_dvsec_decode(&pdev->hdm) ||
> +	    (snapshot->dvsec_ctrl & PCI_DVSEC_CXL_CACHE_ENABLE &&
> +	     !(cap & PCI_DVSEC_CXL_CACHE_WBI_CAPABLE))) {

[Severity: Medium]
In drivers/pci/cxl.c, pci_cxl_reset_prepare() calls
cxl_hdm_cache_snapshot(&pdev->hdm) under a cxl_rwsem.dpa read lock to copy
dvsec_ctrl and global_ctrl into snapshot, and then calls
cxl_hdm_cache_dvsec_decode(&pdev->hdm) in a separate cxl_rwsem.dpa read
section on the live pdev->hdm pointer rather than checking snapshot.

Meanwhile, endpoint port probe and teardown on &port->dev run under
device_lock(&port->dev) without holding device_lock(&pdev->dev) or
cxl_rwsem.region:

  clear_mem_enable()
    -> cxl_set_mem_enable()
      -> cxl_hdm_cache_set_dvsec_ctrl()

and

  devm_cxl_enumerate_decoders()
    -> cxl_hdm_refresh_decoder()
      -> cxl_hdm_cache_refresh_decoder()

both mutate pdev->hdm->dvsec_ctrl and pdev->hdm->global_ctrl under the
cxl_rwsem.dpa write lock.

If clear_mem_enable() clears PCI_DVSEC_CXL_MEM_ENABLE in
pdev->hdm->dvsec_ctrl (or cxl_hdm_cache_refresh_decoder() sets
CXL_HDM_DECODER_ENABLE in pdev->hdm->global_ctrl) between
cxl_hdm_cache_snapshot() and cxl_hdm_cache_dvsec_decode(), wouldn't
cxl_hdm_cache_dvsec_decode(&pdev->hdm) return false even though snapshot
captured PCI_DVSEC_CXL_MEM_ENABLE set with CXL_HDM_DECODER_ENABLE clear,
allowing the reset to proceed and later write stale snapshot->global_ctrl
and snapshot->dvsec_ctrl back to hardware in cxl_restore_state()?

Should this check evaluate snapshot->dvsec_ctrl and snapshot->global_ctrl
directly instead of re-reading pdev->hdm?

> +		rc = -EOPNOTSUPP;
> +		goto free_snapshot;
> +	}

[ ... ]

>  int cxl_restore_state_after_pci_reset(struct pci_dev *pdev,
> -				      struct cxl_hdm_info *snapshot,
> -				      u16 *command)
> +				      const struct cxl_hdm_info *snapshot,
> +				      struct cxl_pci_cache *cache,
> +				      bool reset_attempted, int rc, u16 *command)
>  {
> -	int rc;
> +	int rc2, policy_rc = 0;
>  
> +	*command = 0;
> +	if (!cache)
> +		return rc;
>  	device_lock_assert(&pdev->dev);
> -	cxl_region_quiesce_lock();
> -
> -	rc = cxl_hdm_cache_copy_decoders(&pdev->hdm, snapshot);

[Severity: Low]
Removing this call in cxl_restore_state_after_pci_reset() in
drivers/pci/cxl.c removes the only caller of cxl_hdm_cache_copy_decoders()
in the tree. Should the definition of cxl_hdm_cache_copy_decoders() in
drivers/cxl/core/hdm_state.c and its declaration in include/cxl/hdm.h also
be removed so dead code is not left behind?

> -	if (!rc)
> +	if (reset_attempted && !rc)
>  		rc = pci_cxl_restore_state(pdev, snapshot, command);
> -	if (rc)
> -		cxl_reset_save_disabled_state(pdev);
>  
> +	/* Complete host invalidation before ending IOMMU exclusion. */
> +	rc2 = cxl_hdm_quiesce_flush(&cache->quiesce);
> +	if (rc2)
> +		pci_err(pdev, "failed to invalidate CPU cache after CXL bus reset: %d\n", rc2);
> +	/* Undo preparation on abort; enable caching after reset only if safe. */
> +	if (cache->cache_disabled && (!reset_attempted || (!rc && !rc2)))
> +		policy_rc = cxl_reset_restore_cache_policy(pdev, snapshot->dvsec,
> +							  cache->saved_ctrl2);

[Severity: High]
During a Secondary Bus Reset (Hot Reset), the non-sticky Disable_Caching
bit (PCI_DVSEC_CXL_DISABLE_CACHING) in PCI_DVSEC_CXL_CTRL2 is reset to 0
by hardware along with PCI_DVSEC_CXL_CTRL.

In cxl_restore_state_after_pci_reset() in drivers/pci/cxl.c, when
reset_attempted is true and rc is 0, pci_cxl_restore_state() calls
cxl_restore_state() -> cxl_restore_dvsec_ctrl(), which writes
snapshot->dvsec_ctrl (including PCI_DVSEC_CXL_CACHE_ENABLE and
PCI_DVSEC_CXL_MEM_ENABLE) to PCI_DVSEC_CXL_CTRL before
cxl_hdm_quiesce_flush() and cxl_reset_restore_cache_policy() run.

Because PCI_DVSEC_CXL_DISABLE_CACHING was cleared to 0 by the bus reset,
doesn't writing snapshot->dvsec_ctrl inside pci_cxl_restore_state()
immediately enable device caching before cxl_hdm_quiesce_flush() has
invalidated the host CPU caches, transiently enable caching even when
cache->saved_ctrl2 originally had PCI_DVSEC_CXL_DISABLE_CACHING set, and
leave CXL.cache and CXL.mem enabled in hardware if cxl_hdm_quiesce_flush()
fails (since cxl_reset_save_disabled_state() only clears MSE and BME in
PCI_COMMAND)?

> +	if (reset_attempted) {
> +		rc = rc ?: rc2 ?: policy_rc;
> +		if (rc)
> +			cxl_reset_save_disabled_state(pdev);
> +	} else {
> +		/* An aborted reset alone must not poison the saved PCI command. */
> +		if (rc2 || policy_rc)
> +			cxl_reset_save_disabled_state(pdev);
> +		rc = rc ?: rc2 ?: policy_rc;
> +	}
> +
> +	cxl_hdm_quiesce_destroy(&cache->quiesce);
>  	cxl_region_quiesce_unlock();
> +	kfree(cache);
>  	return rc;
>  }

[Severity: Low]
In include/cxl/hdm.h, cxl_region_quiesce is declared with
token_context_lock(cxl_region_quiesce), and cxl_region_quiesce_lock(),
cxl_region_quiesce_unlock(), cxl_hdm_quiesce_prepare(),
cxl_hdm_quiesce_flush(), pci_cxl_restore_state(), and the definition of
cxl_reset_cache_prepare() carry __acquires, __releases, or __must_hold
annotations for cxl_region_quiesce.

In drivers/pci/cxl.c:
1. The new forward declaration of cxl_reset_cache_prepare() omits the
   __must_hold(cxl_region_quiesce) annotation present on its definition.
2. pci_cxl_reset_prepare() calls cxl_region_quiesce_lock() and returns with
   cxl_region_quiesce held on its non-NULL success path (or releases it via
   cxl_restore_state_after_pci_reset() on the finish path) without context
   annotations.
3. cxl_restore_state_after_pci_reset() calls the __must_hold functions
   pci_cxl_restore_state() and cxl_hdm_quiesce_flush() as well as
   cxl_region_quiesce_unlock() when cache is non-NULL without context
   annotations.

When building with Clang static context analysis enabled
(CONFIG_WARN_CONTEXT_ANALYSIS_ALL=y), won't these missing annotations
trigger -Wthread-safety warnings in drivers/pci/cxl.c?

[ ... ]

> @@ -731,7 +831,9 @@ static int cxl_reset_quiesced(struct pci_dev *pdev, int dvsec, u16 cap)
>  		rc = __cxl_reset_execute(pdev, dvsec, cap, snapshot, &cache.quiesce,
>  					 &command);
>  	/* Restore cache policy after any attempt to disable caching. */
> -	rc2 = cxl_reset_restore_cache_policy(pdev, dvsec, cache.saved_ctrl2);
> +	rc2 = 0;
> +	if (cache.cache_disabled)
> +		rc2 = cxl_reset_restore_cache_policy(pdev, dvsec, cache.saved_ctrl2);
>  	cxl_hdm_quiesce_destroy(&cache.quiesce);
>  	if (rc)
>  		return rc;

[Severity: Medium]
In cxl_restore_state_after_pci_reset() in drivers/pci/cxl.c,
cxl_reset_restore_cache_policy() is guarded with
(!reset_attempted || (!rc && !rc2)) so caching is not re-enabled when an
attempted reset, HDM restoration, or post-reset cache flush fails, and on
an aborted reset cxl_reset_save_disabled_state() is called if
cxl_reset_restore_cache_policy() fails.

In cxl_reset_quiesced() in drivers/pci/cxl.c, however:
1. If __cxl_reset_execute() fails during CXL Reset, pci_cxl_restore_state(),
   or cxl_hdm_quiesce_flush(), cxl_reset_quiesced() still unconditionally
   calls cxl_reset_restore_cache_policy() whenever cache.cache_disabled is
   true, re-enabling caching in PCI_DVSEC_CXL_CTRL2 against unrestored HDM
   state after pci_dev_reset_iommu_done() has already ended IOMMU exclusion.
2. Because if (rc) return rc; runs before checking rc2:

	if (rc)
		return rc;
	if (rc2) {
		cxl_reset_save_disabled_state(pdev);
		return rc2;
	}

   if cxl_reset_cache_prepare() fails after setting cache.cache_disabled =
   true (or __cxl_reset_execute() fails in
   pci_wait_for_pending_transaction() or pci_dev_reset_iommu_prepare()
   before reaching cxl_reset_save_disabled_state()) and
   cxl_reset_restore_cache_policy() also fails (rc2 != 0),
   cxl_reset_quiesced() returns rc without calling
   cxl_reset_save_disabled_state(pdev).

Should cxl_reset_quiesced() follow the same error-handling rules as
cxl_restore_state_after_pci_reset() for restoring the cache policy and
saving the disabled state?

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 1d4ba90a69762..b29a6a43249dd 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c

[ ... ]

> @@ -5066,8 +5068,9 @@ static int cxl_reset_bus_function(struct pci_dev *dev, bool probe)
>  		pci_write_config_word(bridge, dvsec + PCI_DVSEC_CXL_PORT_CTL,
>  				      reg);
>  
> -	if (!rc && snapshot)
> -		rc = cxl_restore_state_after_pci_reset(dev, snapshot, &command);
> +	/* Reset-helper failure does not prove that no hardware reset occurred. */
> +	rc = cxl_restore_state_after_pci_reset(dev, snapshot, cache, true, rc,
> +					       &command);

[Severity: Medium]
In cxl_reset_bus_function() in drivers/pci/pci.c, when probe is true, the
function only checks for VFs/multifunction, pci_upstream_bridge(dev), and
cxl_port_dvsec(bridge) without probing pci_dev_reset_slot_function() or
pci_parent_bus_reset().

When cxl_reset_bus_function(dev, false) runs on a CXL device with cached
HDM state, pci_cxl_reset_prepare() disables device caching in
PCI_DVSEC_CXL_CTRL2, and then pci_reset_bus_function(dev, false) is called.
If pci_reset_bus_function(dev, false) returns -ENOTTY from its prerequisite
checks without touching hardware (for example, when cxl_sbr_masked(bridge)
remains true because PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR is read-only, or
when both pci_dev_reset_slot_function() and pci_parent_bus_reset() return
-ENOTTY because PCI_DEV_FLAGS_NO_BUS_RESET is set, dev->subordinate is
non-NULL, or another device shares dev->bus->devices),
cxl_reset_bus_function() still passes reset_attempted = true and
rc = -ENOTTY to cxl_restore_state_after_pci_reset().

When reset_attempted is true and rc is non-zero,
cxl_restore_state_after_pci_reset() skips cxl_reset_restore_cache_policy()
and calls cxl_reset_save_disabled_state(dev), which leaves
PCI_DVSEC_CXL_DISABLE_CACHING set and clears MSE and BME in
dev->saved_config_space.

Should cxl_reset_bus_function() verify that slot or parent bus reset is
supported before preparing the reset (or treat -ENOTTY from
pci_reset_bus_function() as an unattempted reset) so an unsupported bus
reset does not disable an untouched device?

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