Re: [PATCH v6 04/31] PCI/CXL: Let the driver that owns the HDM range run CXL Reset

[Alex Williamson]

On Fri, 16 Oct 2026 21:48:11 +0530
<mhonap@xxxxxxxxxx> wrote:

> From: Manish Honap <mhonap@xxxxxxxxxx>
> 
> vfio-cxl passes a CXL Type-2 device to a guest and must run CXL Reset at
> the vfio reset points. The cxl_reset PCI reset method does not fit that
> caller:
> 
> - It requests every HPA range the device decodes. vfio-cxl holds that
>   range with request_mem_region_exclusive() while it exposes the memory,
>   so the request fails with -EBUSY.
> - When the reset cannot run, the method returns -ENOTTY and the PCI core
>   moves on to the next method. That is FLR, which leaves device memory
>   intact for the next user.
> - The snapshot and restore helpers that vfio-cxl needs after a bus reset
>   and on return from low power are not exported.
> 
> Let cxl_hdm_quiesce_prepare() take the resource the caller already holds
> and skip the request for ranges inside it. pci_cxl_reset_owned() accepts
> only a resource marked busy and exclusive, as
> request_mem_region_exclusive() leaves it. The CPU caches are still
> written back and invalidated for every range before and after the reset,
> as the reset method does.
> 
> Add:
> - pci_cxl_reset_owned(): runs the reset method's sequence for a caller that
>   holds the device lock and returns any failure to that caller
> 
> - pci_cxl_reset_capable(): checks the cached HDM state and the CPU cache
>   invalidation that the reset-method probe skips.
> 
> - pci_cxl_hdm_snapshot(): copies the HDM state cached at enumeration
>   without requiring the device lock. A driver needs this copy in probe,
>   which the PCI core may run in a worker on the device's NUMA node, where
>   device_lock_assert() would warn although the binding task holds the
>   lock. pci_cxl_reset_prepare() becomes that copy plus the lock assertion.
> 
> Export:
> - pci_cxl_reset_owned()
> - pci_cxl_reset_capable()
> - pci_cxl_hdm_snapshot()
> - cxl_restore_state_after_pci_reset()
> 
> to the vfio-cxl module only, and declare them in include/cxl/pci.h.
> 
> CXL Reset always clears memory, so there is no caller Memory Clear choice.
> 
> Assisted-by: LLM
> Signed-off-by: Manish Honap <mhonap@xxxxxxxxxx>
> ---
>  drivers/cxl/core/hdm_state.c |  22 ++++--
>  drivers/pci/cxl.c            | 136 ++++++++++++++++++++++++++++-------
>  drivers/pci/pci.h            |  18 +----
>  include/cxl/hdm.h            |   3 +-
>  include/cxl/pci.h            |  39 ++++++++++
>  5 files changed, 171 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm_state.c b/drivers/cxl/core/hdm_state.c
> index 95daeda0ef80..9452047e3b97 100644
> --- a/drivers/cxl/core/hdm_state.c
> +++ b/drivers/cxl/core/hdm_state.c
> @@ -267,7 +267,8 @@ static int cxl_hdm_ranges_collect(struct cxl_hdm_quiesce *q, struct device *dev,
>  	return 0;
>  }
>  
> -static int cxl_hdm_ranges_request(struct cxl_hdm_quiesce *q)
> +static int cxl_hdm_ranges_request(struct cxl_hdm_quiesce *q,
> +				  const struct resource *owned)
>  {
>  	struct cxl_hdm_range *range;
>  
> @@ -275,6 +276,12 @@ static int cxl_hdm_ranges_request(struct cxl_hdm_quiesce *q)
>  
>  	list_for_each_entry(range, &q->ranges, list) {
>  		const struct range *hpa_range = &range->hpa_range;
> +		struct resource res = DEFINE_RES_MEM(hpa_range->start,
> +						     range->len);
> +
> +		/* The caller already holds this range exclusively. */
> +		if (owned && resource_contains(owned, &res))
> +			continue;
>  
>  		range->res = request_mem_region(hpa_range->start, range->len,
>  						"cxl_reset");
> @@ -316,15 +323,20 @@ static int cxl_hdm_ranges_invalidate(struct cxl_hdm_quiesce *q)
>   * @dev: device for error messages
>   * @q: quiesce state to initialize
>   * @snapshot: HDM state from cxl_hdm_cache_snapshot()
> + * @owned: resource the caller holds exclusively over the ranges, or NULL
>   *
>   * Reserve every enabled system-physical range in @snapshot exclusively, then
> - * write back and invalidate CPU caches for them. Call
> - * cxl_hdm_quiesce_destroy() afterwards whether or not this succeeds.
> + * write back and invalidate CPU caches for them. A range inside @owned is
> + * not requested again; the caller's reservation already excludes other
> + * users.
> + *
> + * Call cxl_hdm_quiesce_destroy() afterwards whether or not this succeeds.
>   *
>   * Return: 0 on success, or a negative errno.
>   */
>  int cxl_hdm_quiesce_prepare(struct device *dev, struct cxl_hdm_quiesce *q,
> -			    const struct cxl_hdm_info *snapshot)
> +			    const struct cxl_hdm_info *snapshot,
> +			    const struct resource *owned)
>  {
>  	int rc;
>  
> @@ -340,7 +352,7 @@ int cxl_hdm_quiesce_prepare(struct device *dev, struct cxl_hdm_quiesce *q,
>  	if (rc)
>  		return rc;
>  
> -	rc = cxl_hdm_ranges_request(q);
> +	rc = cxl_hdm_ranges_request(q, owned);
>  	if (rc)
>  		return rc;
>  
> diff --git a/drivers/pci/cxl.c b/drivers/pci/cxl.c
> index ed0b6f619c70..b4d467506c54 100644
> --- a/drivers/pci/cxl.c
> +++ b/drivers/pci/cxl.c
> @@ -372,9 +372,32 @@ static int cxl_reset_save_restored_state(struct pci_dev *pdev, u16 command)
>  	return rc;
>  }
>  
> +/**
> + * pci_cxl_hdm_snapshot() - copy the HDM state cached at enumeration
> + * @pdev: CXL device
> + *
> + * The cache is read under cxl_rwsem.dpa, so the caller needs no device
> + * lock. A driver can call this from probe, which the PCI core may run in a
> + * worker on the device's NUMA node while the binding task holds the lock.
> + *
> + * Return: a copy to free with kfree(); NULL when @pdev has no cached HDM
> + * state; or an ERR_PTR() on failure.
> + */
> +struct cxl_hdm_info *pci_cxl_hdm_snapshot(struct pci_dev *pdev)
> +{
> +	struct cxl_hdm_info *snapshot;
> +
> +	snapshot = cxl_hdm_cache_snapshot(&pdev->hdm);
> +	if (IS_ERR(snapshot) && PTR_ERR(snapshot) == -ENXIO)
> +		return NULL;
> +
> +	return snapshot;
> +}
> +EXPORT_SYMBOL_FOR_MODULES(pci_cxl_hdm_snapshot, "vfio-cxl");

Why are any of these module locked?  What great risk do they expose
that only vfio-cxl should have access to versus just a NS export?

We shouldn't set a precedent for vfio specific APIs and I certainly
don't want to be defending such a thing down the road.

It's unfortunate that we're intermixing "snapshot" from CXL but using
it like a "save state" from PCI.  Maybe we could bridge the gap at the
PCI API?

> +
>  /**
>   * pci_cxl_reset_prepare() - allocate HDM state storage ahead of a bus reset
> - * @pdev: device about to be reset
> + * @pdev: device about to be reset, with its device lock held
>   *
>   * Copy the cached HDM state before the reset, so that restoring cannot fail
>   * for lack of memory. The decoder settings are refreshed when the state is
> @@ -386,14 +409,8 @@ static int cxl_reset_save_restored_state(struct pci_dev *pdev, u16 command)
>   */
>  struct cxl_hdm_info *pci_cxl_reset_prepare(struct pci_dev *pdev)
>  {
> -	struct cxl_hdm_info *snapshot;
> -
>  	device_lock_assert(&pdev->dev);
> -	snapshot = cxl_hdm_cache_snapshot(&pdev->hdm);
> -	if (IS_ERR(snapshot) && PTR_ERR(snapshot) == -ENXIO)
> -		return NULL;
> -
> -	return snapshot;
> +	return pci_cxl_hdm_snapshot(pdev);
>  }
>  
>  /**
> @@ -433,6 +450,7 @@ int cxl_restore_state_after_pci_reset(struct pci_dev *pdev,
>  	cxl_region_quiesce_unlock();
>  	return rc;
>  }
> +EXPORT_SYMBOL_FOR_MODULES(cxl_restore_state_after_pci_reset, "vfio-cxl");
>  
>  /*
>   * CXL r4.0 sec 9.7.2 defines the reset completion timeout encodings.
> @@ -642,7 +660,8 @@ static int __cxl_reset_execute(struct pci_dev *pdev, int dvsec, u16 cap,
>  	return rc;
>  }
>  
> -static int cxl_reset_quiesced(struct pci_dev *pdev, int dvsec, u16 cap)
> +static int cxl_reset_quiesced(struct pci_dev *pdev, int dvsec, u16 cap,
> +			      const struct resource *owned)
>  	__must_hold(cxl_region_quiesce)
>  {
>  	struct cxl_hdm_info *snapshot __free(kfree) = NULL;
> @@ -663,7 +682,7 @@ static int cxl_reset_quiesced(struct pci_dev *pdev, int dvsec, u16 cap)
>  		return PTR_ERR(snapshot);
>  	}
>  
> -	rc = cxl_hdm_quiesce_prepare(&pdev->dev, &q, snapshot);
> +	rc = cxl_hdm_quiesce_prepare(&pdev->dev, &q, snapshot, owned);
>  	if (rc) {
>  		cxl_hdm_quiesce_destroy(&q);
>  		return rc;
> @@ -686,32 +705,40 @@ static int cxl_reset_quiesced(struct pci_dev *pdev, int dvsec, u16 cap)
>  	return cxl_reset_save_restored_state(pdev, command);
>  }
>  
> -static int cxl_reset_execute(struct pci_dev *pdev, int dvsec, u16 cap)
> +static int cxl_reset_execute(struct pci_dev *pdev, int dvsec, u16 cap,
> +			     const struct resource *owned)
>  {
>  	int rc;
>  
> +	/* Raw reset callers may not have saved the PCI state needed for restore. */
> +	if (!pdev->state_saved) {
> +		pci_err(pdev, "CXL reset requires saved PCI state\n");
> +		return -EINVAL;
> +	}
> +
>  	cxl_region_quiesce_lock();
> -	rc = cxl_reset_quiesced(pdev, dvsec, cap);
> +	rc = cxl_reset_quiesced(pdev, dvsec, cap, owned);
>  	cxl_region_quiesce_unlock();
>  
>  	return rc;
>  }
>  
> -int cxl_reset_function(struct pci_dev *pdev, bool probe)
> +static int cxl_reset_check(struct pci_dev *pdev, bool probe, u16 *dvsec,
> +			   u16 *cap)
>  {
> -	u16 dvsec, cap, ctrl;
> +	u16 ctrl;
>  	int rc;
>  
> -	rc = cxl_pci_device_dvsec(pdev, &dvsec, &cap);
> +	rc = cxl_pci_device_dvsec(pdev, dvsec, cap);
>  	if (rc)
>  		return probe ? -ENOTTY : rc;
> -	if (!(cap & PCI_DVSEC_CXL_CACHE_CAPABLE) ||
> -	    !(cap & PCI_DVSEC_CXL_MEM_CAPABLE) ||
> -	    !(cap & PCI_DVSEC_CXL_RST_CAPABLE) ||
> -	    !(cap & PCI_DVSEC_CXL_RST_MEM_CLR_CAPABLE))
> +	if (!(*cap & PCI_DVSEC_CXL_CACHE_CAPABLE) ||
> +	    !(*cap & PCI_DVSEC_CXL_MEM_CAPABLE) ||
> +	    !(*cap & PCI_DVSEC_CXL_RST_CAPABLE) ||
> +	    !(*cap & PCI_DVSEC_CXL_RST_MEM_CLR_CAPABLE))
>  		return -ENOTTY;
>  
> -	rc = pci_read_config_word(pdev, dvsec + PCI_DVSEC_CXL_CTRL, &ctrl);
> +	rc = pci_read_config_word(pdev, *dvsec + PCI_DVSEC_CXL_CTRL, &ctrl);
>  	if (rc)
>  		return probe ? -ENOTTY : pcibios_err_to_errno(rc);
>  	if (PCI_POSSIBLE_ERROR(ctrl))
> @@ -737,11 +764,70 @@ int cxl_reset_function(struct pci_dev *pdev, bool probe)
>  	if (!cpu_cache_has_invalidate_memregion())
>  		return -ENOTTY;
>  
> -	/* Raw reset callers may not have saved the PCI state needed for restore. */
> -	if (!pdev->state_saved) {
> -		pci_err(pdev, "CXL reset requires saved PCI state\n");
> +	return 0;
> +}
> +
> +int cxl_reset_function(struct pci_dev *pdev, bool probe)
> +{
> +	u16 dvsec, cap;
> +	int rc;
> +
> +	rc = cxl_reset_check(pdev, probe, &dvsec, &cap);
> +	if (rc || probe)
> +		return rc;
> +
> +	return cxl_reset_execute(pdev, dvsec, cap, NULL);
> +}
> +
> +/**
> + * pci_cxl_reset_capable() - check whether CXL Reset can run on a device now
> + * @pdev: CXL device
> + *
> + * Unlike the reset-method probe, this also checks the cached HDM state and
> + * the CPU cache invalidation that the reset needs.
> + *
> + * Return: true when pci_cxl_reset_owned() can attempt the reset.
> + */
> +bool pci_cxl_reset_capable(struct pci_dev *pdev)
> +{
> +	u16 dvsec, cap;
> +
> +	return !cxl_reset_check(pdev, false, &dvsec, &cap);
> +}
> +EXPORT_SYMBOL_FOR_MODULES(pci_cxl_reset_capable, "vfio-cxl");
> +
> +/**
> + * pci_cxl_reset_owned() - CXL Reset for the driver that owns the HDM range
> + * @pdev: CXL device, with its device lock held and PCI state saved
> + * @owned: resource the caller reserved over the device's HPA range with
> + *	   request_mem_region_exclusive(), or NULL
> + *
> + * Run the same sequence as the cxl_reset method. Ranges inside @owned are not
> + * requested again, so a driver that keeps the HPA range reserved while it
> + * exposes the memory can reset the device. The CPU caches are still
> + * invalidated for every range before and after the reset.
> + *
> + * Return: 0 on success, or a negative errno. Unlike the reset method, a
> + * device that cannot be reset returns an error to the caller instead of
> + * falling through to another reset method. -EINVAL if @owned is not a busy,
> + * exclusive reservation.
> + */
> +int pci_cxl_reset_owned(struct pci_dev *pdev, const struct resource *owned)
> +{
> +	const unsigned long held = IORESOURCE_BUSY | IORESOURCE_EXCLUSIVE;
> +	u16 dvsec, cap;
> +	int rc;
> +
> +	device_lock_assert(&pdev->dev);
> +
> +	/* Only a busy, exclusive reservation keeps other users off the range. */
> +	if (owned && WARN_ON_ONCE((owned->flags & held) != held))
>  		return -EINVAL;
> -	}
>  
> -	return cxl_reset_execute(pdev, dvsec, cap);
> +	rc = cxl_reset_check(pdev, false, &dvsec, &cap);
> +	if (rc)
> +		return rc;
> +
> +	return cxl_reset_execute(pdev, dvsec, cap, owned);
>  }

There's a lot of asymmetric sketchiness through this path.
pdev->state_saved is required to be set by the caller, but the state
gets restored and state_saved invalided deep inside
cxl_restore_pci_state_for_hdm_restore() on this path. That's not at all
symmetric to the PCI conventions and the split ownership model is poor.
Even worse, it's vfio-pci-core doing the pci_save_state() on behalf of
the cxl_ops.reset callback.

Where does bus-master get cleared?  Seems we've only got half of
pci_dev_save_and_disable() implemented here and the save has become the
caller's responsibility.  Thanks,

Alex