Re: [PATCH v17 17/19] PCI/CXL: Restore CXL state after CXL bus reset

[Dave Jiang]

On 11/5/26 11:59 AM, Srirangan Madhavan wrote:
> CXL bus reset can clear HDM decoder programming and CXL Device DVSEC
> control state. Restore the cached state after a successful cxl_bus reset
> while IOMMU exclusion remains active.
> 
> Restore PCI configuration first so HDM MMIO is accessible, and preserve a
> disabled state if restoration fails.
> 
> Copy the cached state before the reset, then refresh its decoder settings
> from the cache under the CXL region quiesce once the reset completes. A
> region decommit during the reset updates the cache, and restoring the
> earlier decoders would leave the endpoint decoding an HPA range that the
> CXL core considers free. Keep global and DVSEC control from before the
> reset, since a refresh during the reset can read them as all-ones.
> 
> The prepare/restore interface and decoder-refresh structure follow Dave
> Jiang's cxl-type2-reset reference branch.
> 
> Signed-off-by: Srirangan Madhavan <smadhavan@xxxxxxxxxx>
> Acked-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx> # pci/pci.c
> Assisted-by: LLM
> 
> ---
> Changes in v17:
> - Enable bus mastering only after the IOMMU is restored, via a separate
>   cxl_restore_command_after_pci_reset() step (Richard)
>   https://lore.kernel.org/linux-cxl/auRhJa-nzR0CnKf3@MWDK4CY14F/
> 
> Reference branch: https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/commit/?id=b93f0c8e67b198a63786fd3721b7177ab9df60be
> ---
>  drivers/cxl/core/hdm_state.c | 31 +++++++++++++
>  drivers/pci/cxl.c            | 85 ++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci.c            | 14 ++++++
>  drivers/pci/pci.h            | 24 ++++++++++
>  include/cxl/hdm.h            |  2 +
>  5 files changed, 156 insertions(+)
> 
> diff --git a/drivers/cxl/core/hdm_state.c b/drivers/cxl/core/hdm_state.c
> index 51df56377f7a..7e0903ce0b78 100644
> --- a/drivers/cxl/core/hdm_state.c
> +++ b/drivers/cxl/core/hdm_state.c
> @@ -182,6 +182,37 @@ struct cxl_hdm_info *cxl_hdm_cache_snapshot(struct cxl_hdm_info *const *slot)
>  	return copy;
>  }
>  
> +/**
> + * cxl_hdm_cache_copy_decoders() - refresh the decoders of a snapshot
> + * @slot: cache pointer owned by the device
> + * @dst: snapshot of the same device from cxl_hdm_cache_snapshot()
> + *
> + * Copy only the per-decoder settings, which region commit and teardown keep
> + * current. Leave the rest of @dst, including global and DVSEC control, as
> + * captured: a refresh during a reset may have read those registers from a
> + * device that returned all-ones.
> + *
> + * Return: 0 on success, -ENXIO when nothing is published, or -EINVAL when
> + * @dst holds a different number of decoders.
> + */
> +int cxl_hdm_cache_copy_decoders(struct cxl_hdm_info *const *slot,
> +				struct cxl_hdm_info *dst)

I think this function is a dead function for the series. By the end, nothing calls it.

DJ

> +{
> +	struct cxl_hdm_info *info;
> +
> +	lockdep_assert_held_write(&cxl_rwsem.region);
> +	guard(rwsem_read)(&cxl_rwsem.dpa);
> +	info = *slot;
> +	if (!info)
> +		return -ENXIO;
> +	if (info->decoder_count != dst->decoder_count)
> +		return -EINVAL;
> +
> +	memcpy(dst->settings, info->settings,
> +	       flex_array_size(info, settings, info->decoder_count));
> +	return 0;
> +}
> +
>  void cxl_region_quiesce_lock(void)
>  	__acquires(cxl_region_quiesce)
>  	__context_unsafe(token maps to cxl_rwsem.region)
> diff --git a/drivers/pci/cxl.c b/drivers/pci/cxl.c
> index c68b08f7f95b..a9aa2e1c631d 100644
> --- a/drivers/pci/cxl.c
> +++ b/drivers/pci/cxl.c
> @@ -393,6 +393,91 @@ static int pci_cxl_restore_state(struct pci_dev *pdev,
>  	return cxl_restore_state(pdev, snapshot);
>  }
>  
> +/**
> + * pci_cxl_reset_prepare() - allocate HDM state storage ahead of a bus reset
> + * @pdev: device about to be reset
> + *
> + * 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
> + * restored.
> + *
> + * Return: storage for cxl_restore_state_after_pci_reset(), to be freed with
> + * kfree(); NULL when @pdev has no cached HDM state; or an ERR_PTR() on
> + * failure.
> + */
> +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))
> +		return PTR_ERR(snapshot) == -ENXIO ? NULL : snapshot;
> +
> +	/* 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);
> +	}
> +
> +	return snapshot;
> +}
> +
> +/**
> + * cxl_restore_state_after_pci_reset() - restore CXL state after a bus reset
> + * @pdev: device that was reset
> + * @snapshot: storage from pci_cxl_reset_prepare()
> + * @command: returns the saved PCI_COMMAND value to apply once the IOMMU is
> + *	     restored
> + *
> + * Under the region quiesce, refresh the decoder settings in @snapshot from the
> + * cache and restore: first the PCI configuration needed to reach HDM MMIO,
> + * then the HDM decoder and CXL Device DVSEC state. Leave @pdev disabled on
> + * failure.
> + *
> + * Bus mastering stays off, because the caller has not yet restored IOMMU
> + * translation and a DMA request now would hit the blocking domain. The caller
> + * calls cxl_restore_command_after_pci_reset() once the IOMMU is restored.
> + *
> + * Refresh the decoders now rather than rely on the copy taken before the
> + * reset: a region commit or teardown during the reset has updated the cache,
> + * and restoring older decoders would leave the device decoding ranges the
> + * CXL core considers free.
> + */
> +int cxl_restore_state_after_pci_reset(struct pci_dev *pdev,
> +				      struct cxl_hdm_info *snapshot,
> +				      u16 *command)
> +{
> +	int rc;
> +
> +	device_lock_assert(&pdev->dev);
> +	cxl_region_quiesce_lock();
> +
> +	rc = cxl_hdm_cache_copy_decoders(&pdev->hdm, snapshot);
> +	if (!rc)
> +		rc = pci_cxl_restore_state(pdev, snapshot, command);
> +	if (rc)
> +		cxl_reset_save_disabled_state(pdev);
> +
> +	cxl_region_quiesce_unlock();
> +	return rc;
> +}
> +
> +/**
> + * cxl_restore_command_after_pci_reset() - re-enable the device after a bus reset
> + * @pdev: device that was reset
> + * @command: PCI_COMMAND value from cxl_restore_state_after_pci_reset()
> + *
> + * Call after the IOMMU is restored: this can enable bus mastering. Save the
> + * restored PCI state, and leave @pdev disabled on failure.
> + */
> +int cxl_restore_command_after_pci_reset(struct pci_dev *pdev, u16 command)
> +{
> +	device_lock_assert(&pdev->dev);
> +	return cxl_reset_save_restored_state(pdev, command);
> +}
> +
>  /*
>   * CXL r4.0 sec 9.7.2 defines the reset completion timeout encodings.
>   * Sec 9.7.3 leaves config-space access behavior undefined for 100 ms after
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 426b5ee38d04..1d4ba90a6976 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5014,8 +5014,10 @@ static int pci_reset_bus_function(struct pci_dev *dev, bool probe)
>  
>  static int cxl_reset_bus_function(struct pci_dev *dev, bool probe)
>  {
> +	struct cxl_hdm_info *snapshot __free(kfree) = NULL;
>  	struct pci_dev *bridge;
>  	u16 dvsec, reg, val;
> +	u16 command;
>  	int rc;
>  
>  	/*
> @@ -5040,6 +5042,10 @@ static int cxl_reset_bus_function(struct pci_dev *dev, bool probe)
>  	if (rc)
>  		return -ENOTTY;
>  
> +	snapshot = pci_cxl_reset_prepare(dev);
> +	if (IS_ERR(snapshot))
> +		return PTR_ERR(snapshot);
> +
>  	rc = pci_dev_reset_iommu_prepare(dev);
>  	if (rc) {
>  		pci_err(dev, "failed to stop IOMMU for a PCI reset: %d\n", rc);
> @@ -5060,7 +5066,15 @@ 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);
> +
>  	pci_dev_reset_iommu_done(dev);
> +
> +	/* Enable bus mastering only once the IOMMU translates again. */
> +	if (!rc && snapshot)
> +		rc = cxl_restore_command_after_pci_reset(dev, command);
> +
>  	return rc;
>  }
>  
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 743e65abd758..2f819e1fbead 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -730,10 +730,16 @@ static inline void pci_doe_destroy(struct pci_dev *pdev) { }
>  static inline void pci_doe_disconnected(struct pci_dev *pdev) { }
>  #endif
>  
> +struct cxl_hdm_info;
>  #ifdef CONFIG_CXL_RESET
>  void pci_cxl_hdm_cache_init(struct pci_dev *pdev);
>  void pci_cxl_hdm_cache_release(struct pci_dev *pdev);
>  int cxl_reset_function(struct pci_dev *pdev, bool probe);
> +struct cxl_hdm_info *pci_cxl_reset_prepare(struct pci_dev *pdev);
> +int cxl_restore_state_after_pci_reset(struct pci_dev *pdev,
> +				      struct cxl_hdm_info *snapshot,
> +				      u16 *command);
> +int cxl_restore_command_after_pci_reset(struct pci_dev *pdev, u16 command);
>  #else
>  static inline void pci_cxl_hdm_cache_init(struct pci_dev *pdev) { }
>  static inline void pci_cxl_hdm_cache_release(struct pci_dev *pdev) { }
> @@ -741,6 +747,24 @@ static inline int cxl_reset_function(struct pci_dev *pdev, bool probe)
>  {
>  	return -ENOTTY;
>  }
> +
> +static inline struct cxl_hdm_info *pci_cxl_reset_prepare(struct pci_dev *pdev)
> +{
> +	return NULL;
> +}
> +
> +static inline int
> +cxl_restore_state_after_pci_reset(struct pci_dev *pdev,
> +				  struct cxl_hdm_info *snapshot, u16 *command)
> +{
> +	return 0;
> +}
> +
> +static inline int cxl_restore_command_after_pci_reset(struct pci_dev *pdev,
> +						      u16 command)
> +{
> +	return 0;
> +}
>  #endif
>  
>  #ifdef CONFIG_PCI_NPEM
> diff --git a/include/cxl/hdm.h b/include/cxl/hdm.h
> index 94a8f8a1a65a..81767f21052a 100644
> --- a/include/cxl/hdm.h
> +++ b/include/cxl/hdm.h
> @@ -64,6 +64,8 @@ void cxl_hdm_cache_set_normalized(struct pci_dev *pdev, int id);
>  void cxl_hdm_cache_release(struct cxl_hdm_info **slot);
>  bool cxl_hdm_cache_dvsec_decode(struct cxl_hdm_info *const *slot);
>  struct cxl_hdm_info *cxl_hdm_cache_snapshot(struct cxl_hdm_info *const *slot);
> +int cxl_hdm_cache_copy_decoders(struct cxl_hdm_info *const *slot,
> +				struct cxl_hdm_info *dst);
>  
>  /*
>   * Region quiesce blocks CXL region commit and teardown, and with them decoder