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

[Richard Cheng <icheng@nvidia.com>]

On Thu, Oct 29, 2026 at 02:56:58PM +0800, Srirangan Madhavan wrote:
> Refuse reset unless enabled system-physical HDM ranges can be reserved
> exclusively and CPU-cache invalidation is available. Invalidate before
> reset and again before ending IOMMU exclusion, holding range reservations
> until the second invalidation completes. A later patch places state
> restoration before the second invalidation.
> 
> Reject normalized-addressing decoders because their cached ranges are not
> system physical addresses. Ignore zero-size decoders because they map no
> address range. Refuse reset when the device decodes memory through its
> DVSEC ranges instead of HDM decoders, since the cache holds no range to
> reserve for that memory.
> 
> Hold the global cxl_rwsem.region write lock across snapshot, reset and
> restoration to keep the decoder state and reserved ranges stable. This also
> blocks region and decoder sysfs, region probe, poison operations, sanitize
> and region-aware event tracing across CXL devices. The Memory Clear wait is
> capped at 20 seconds; cache flushes and other reset work extend the hold.
> 
> Allow the existing CXL_REGION_INVALIDATION_TEST option to bypass missing
> CPU cache invalidation for test environments without a provider.
> 
> Signed-off-by: Srirangan Madhavan <smadhavan@nvidia.com>
> Assisted-by: LLM
> 
> ---
> Reference branch: https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/commit/?id=5944c6e61f14c0dd28d54537be15edb2cadb0e8b
> ---
>  drivers/cxl/Kconfig          |   3 +-
>  drivers/cxl/core/hdm_state.c | 245 ++++++++++++++++++++++++++++++++++-
>  drivers/pci/cxl.c            |  86 ++++++++++--
>  include/cxl/hdm.h            |  44 ++++++-
>  4 files changed, 367 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> index 8b08bfdc4982..e927e69a3f7b 100644
> --- a/drivers/cxl/Kconfig
> +++ b/drivers/cxl/Kconfig
> @@ -226,7 +226,8 @@ config CXL_REGION_INVALIDATION_TEST
>  	  invalidation mechanism. For example usage of wbinvd is restricted to
>  	  bare metal x86. However, for testing purposes toggling this option
>  	  can disable that data integrity safety and proceed with enabling
> -	  regions when there might be conflicting contents in the CPU cache.
> +	  regions or performing CXL reset when there might be conflicting
> +	  contents in the CPU cache.
>  
>  	  If unsure, or if this kernel is meant for production environments,
>  	  say N.
> diff --git a/drivers/cxl/core/hdm_state.c b/drivers/cxl/core/hdm_state.c
> index 1140006de50d..cb7ec0f86ab1 100644
> --- a/drivers/cxl/core/hdm_state.c
> +++ b/drivers/cxl/core/hdm_state.c
> @@ -1,8 +1,14 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /* Copyright (c) 2026 NVIDIA Corporation & Affiliates */
>  #include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/errno.h>
>  #include <linux/export.h>
>  #include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/list.h>
> +#include <linux/memregion.h>
> +#include <linux/overflow.h>
>  #include <linux/pci.h>
>  #include <linux/rwsem.h>
>  #include <linux/slab.h>
> @@ -13,7 +19,8 @@
>  
>  /*
>   * CXL state that must stay available while no CXL driver is bound: the CXL
> - * locks and the per-device HDM cache filled during PCI enumeration.
> + * locks, the per-device HDM cache filled during PCI enumeration, and the
> + * region quiesce used around a reset.
>   */
>  struct cxl_rwsem cxl_rwsem = {
>  	.region = __RWSEM_INITIALIZER(cxl_rwsem.region),
> @@ -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);
> +}
> +
> +/**
> + * cxl_hdm_cache_snapshot() - copy the published HDM state
> + * @slot: cache pointer owned by the device
> + *
> + * Return: a copy the caller frees with kfree(), ERR_PTR(-ENXIO) when nothing
> + * is published, or another ERR_PTR() on failure.
> + */
> +struct cxl_hdm_info *cxl_hdm_cache_snapshot(struct cxl_hdm_info *const *slot)
> +{
> +	struct cxl_hdm_info *info, *copy;
> +	size_t size;
> +
> +	guard(rwsem_read)(&cxl_rwsem.dpa);
> +	info = *slot;
> +	if (!info)
> +		return ERR_PTR(-ENXIO);
> +	if (info->decoder_count <= 0)
> +		return ERR_PTR(-EINVAL);
> +
> +	size = struct_size(info, settings, info->decoder_count);
> +	if (size == SIZE_MAX)
> +		return ERR_PTR(-EOVERFLOW);
> +
> +	copy = kmemdup(info, size, GFP_KERNEL);
> +	if (!copy)
> +		return ERR_PTR(-ENOMEM);
> +
> +	return copy;
> +}
> +
> +void cxl_region_quiesce_lock(void)
> +	__acquires(cxl_region_quiesce)
> +	__context_unsafe(token maps to cxl_rwsem.region)
> +{
> +	down_write(&cxl_rwsem.region);
> +	__acquire(cxl_region_quiesce);
> +}
> +
> +void cxl_region_quiesce_unlock(void)
> +	__releases(cxl_region_quiesce)
> +	__context_unsafe(token maps to cxl_rwsem.region)
> +{
> +	__release(cxl_region_quiesce);
> +	up_write(&cxl_rwsem.region);
> +}
> +
> +/*
> + * Bound the range twice: request_mem_region() takes resource_size_t while
> + * cpu_cache_invalidate_memregion() takes size_t, and the two differ on
> + * 32-bit builds with CONFIG_PHYS_ADDR_T_64BIT. range_len() can also reach
> + * RESOURCE_SIZE_MAX + 1 for a full-width range, and wraps to zero when
> + * resource_size_t is 64-bit, which the !len test catches.
> + */
> +static int cxl_hdm_range_validate(const struct range *hpa_range)
> +{
> +	u64 len = range_len(hpa_range);
> +
> +	if (!len)
> +		return -EINVAL;
> +	if (hpa_range->end > RESOURCE_SIZE_MAX || len > RESOURCE_SIZE_MAX ||
> +	    len > SIZE_MAX)
> +		return -EOVERFLOW;
> +
> +	return 0;
> +}
> +
> +static int cxl_hdm_range_add(struct cxl_hdm_quiesce *q,
> +			     const struct range *hpa_range)
> +{
> +	struct cxl_hdm_range *range, *next, *new_range;
> +	int rc;
> +
> +	rc = cxl_hdm_range_validate(hpa_range);
> +	if (rc)
> +		return rc;
> +
> +	list_for_each_entry(range, &q->ranges, list)
> +		if (range_contains(&range->hpa_range, hpa_range))
> +			return 0;
> +
> +	new_range = kzalloc_obj(*new_range);
> +	if (!new_range)
> +		return -ENOMEM;
> +
> +	new_range->hpa_range = *hpa_range;
> +	new_range->len = range_len(hpa_range);
> +
> +	list_for_each_entry_safe(range, next, &q->ranges, list) {
> +		if (range_contains(hpa_range, &range->hpa_range)) {
> +			list_del(&range->list);
> +			kfree(range);
> +		}
> +	}
> +	list_add_tail(&new_range->list, &q->ranges);
> +
> +	return 0;
> +}
> +
> +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;
> +
> +		rc = cxl_hdm_range_add(q, &config->hpa_range);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cxl_hdm_ranges_request(struct cxl_hdm_quiesce *q)
> +{
> +	struct cxl_hdm_range *range;
> +
> +	lockdep_assert_held_write(&cxl_rwsem.region);
> +
> +	list_for_each_entry(range, &q->ranges, list) {
> +		const struct range *hpa_range = &range->hpa_range;
> +
> +		range->res = request_mem_region(hpa_range->start, range->len,
> +						"cxl_reset");
> +		if (!range->res)
> +			return -EBUSY;
> +	}
> +

Hi Srirangan,

The spec explicitly requires no outstanding or new CXL.mem transaction after
the affected HDM ranges are offlined.

For this part, I don't see any guarantee that a successful request_mem_region()
means all outstanding memory access have stopped and no new access can occur.

I see you refuse to reset when resource reservation conflict, but nothing
offline memory or directly stop CXL.mem traffic.

I wonder if the implementation provide enough evidence that when request_mem_region()
success means no new mem access or CXL.mem traffic will pop up ?

Best regards,
Richard Cheng.

> +	return 0;
> +}
> +
> +static int cxl_hdm_ranges_invalidate(struct cxl_hdm_quiesce *q)
> +{
> +	struct cxl_hdm_range *range;
> +	int rc = 0;
> +
> +	lockdep_assert_held_write(&cxl_rwsem.region);
> +
> +	if (!cpu_cache_has_invalidate_memregion() &&
> +	    IS_ENABLED(CONFIG_CXL_REGION_INVALIDATION_TEST)) {
> +		pr_info_once("CXL reset: bypassing CPU cache invalidation for testing\n");
> +		return 0;
> +	}
> +
> +	list_for_each_entry(range, &q->ranges, list) {
> +		const struct range *hpa_range = &range->hpa_range;
> +		int rc2;
> +
> +		rc2 = cpu_cache_invalidate_memregion(hpa_range->start, range->len);
> +		rc = rc ?: rc2;
> +	}
> +
> +	return rc;
> +}
> +
> +/**
> + * cxl_hdm_quiesce_prepare() - take the HPA ranges of a device out of use
> + * @q: quiesce state to initialize
> + * @snapshot: HDM state from cxl_hdm_cache_snapshot()
> + *
> + * 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.
> + *
> + * Return: 0 on success, or a negative errno.
> + */
> +int cxl_hdm_quiesce_prepare(struct cxl_hdm_quiesce *q,
> +			    const struct cxl_hdm_info *snapshot)
> +{
> +	int rc;
> +
> +	lockdep_assert_held_write(&cxl_rwsem.region);
> +	INIT_LIST_HEAD(&q->ranges);
> +
> +	if (!cpu_cache_has_invalidate_memregion() &&
> +	    !IS_ENABLED(CONFIG_CXL_REGION_INVALIDATION_TEST))
> +		return -ENXIO;
> +
> +	rc = cxl_hdm_ranges_collect(q, snapshot);
> +	if (rc)
> +		return rc;
> +
> +	rc = cxl_hdm_ranges_request(q);
> +	if (rc)
> +		return rc;
> +
> +	return cxl_hdm_ranges_invalidate(q);
> +}
> +
> +/* Evict lines fetched during reset before DMA exclusion ends. */
> +int cxl_hdm_quiesce_flush(struct cxl_hdm_quiesce *q)
> +{
> +	return cxl_hdm_ranges_invalidate(q);
> +}
> +
> +void cxl_hdm_quiesce_destroy(struct cxl_hdm_quiesce *q)
> +{
> +	struct cxl_hdm_range *range, *next;
> +
> +	list_for_each_entry_safe(range, next, &q->ranges, list) {
> +		list_del(&range->list);
> +		if (range->res)
> +			release_mem_region(range->hpa_range.start,
> +					   resource_size(range->res));
> +		kfree(range);
> +	}
> +}
> diff --git a/drivers/pci/cxl.c b/drivers/pci/cxl.c
> index bfc31f821d01..4a8969145f8b 100644
> --- a/drivers/pci/cxl.c
> +++ b/drivers/pci/cxl.c
> @@ -15,6 +15,7 @@
>  #include <linux/iommu.h>
>  #include <linux/jiffies.h>
>  #include <linux/kernel.h>
> +#include <linux/memregion.h>
>  #include <linux/pci.h>
>  #include <linux/slab.h>
>  
> @@ -23,6 +24,13 @@
>  
>  #include "pci.h"
>  
> +struct cxl_pci_cache {
> +	struct cxl_hdm_quiesce quiesce;
> +	u16 saved_ctrl2;
> +	/* A cache-policy write was attempted and may need undoing. */
> +	bool cache_disabled;
> +};
> +
>  /* Find the CXL Device DVSEC and read its capability register. */
>  static int cxl_pci_device_dvsec(struct pci_dev *pdev, u16 *dvsec, u16 *cap)
>  {
> @@ -331,6 +339,23 @@ static int cxl_reset_restore_cache_policy(struct pci_dev *pdev, int dvsec,
>  	return rc;
>  }
>  
> +/* Shared range reservation, host invalidation, and device cache writeback. */
> +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)
> +	__must_hold(cxl_region_quiesce)
> +{
> +	int rc;
> +
> +	rc = cxl_hdm_quiesce_prepare(&cache->quiesce, snapshot);
> +	if (rc)
> +		return rc;
> +
> +	/* Track an attempted change so partial failures still restore policy. */
> +	cache->cache_disabled = true;
> +	return cxl_reset_disable_cache(pdev, dvsec, cap);
> +}
> +
>  static int cxl_reset_wait_done(struct pci_dev *pdev, int dvsec, u16 cap)
>  {
>  	unsigned long deadline;
> @@ -386,7 +411,8 @@ static int cxl_clear_memory(struct pci_dev *pdev, int dvsec, bool initiate)
>  				      PCI_DVSEC_CXL_RST_MEM_CLR_EN);
>  }
>  
> -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,
> +			      struct cxl_hdm_quiesce *q)
>  {
>  	int rc, rc2;
>  
> @@ -411,29 +437,61 @@ static int __cxl_reset_execute(struct pci_dev *pdev, int dvsec, u16 cap)
>  		pci_err(pdev, "failed to clear CXL Reset Memory Clear: %d\n", rc2);
>  	rc = rc ?: rc2;
>  
> +	rc2 = cxl_hdm_quiesce_flush(q);
> +	if (rc2)
> +		pci_err(pdev, "failed to invalidate CPU cache after CXL reset: %d\n", rc2);
> +	rc = rc ?: rc2;
> +
>  	pci_dev_reset_iommu_done(pdev);
>  	return rc;
>  }
>  
> -static int cxl_reset_execute(struct pci_dev *pdev, int dvsec, u16 cap)
> +static int cxl_reset_quiesced(struct pci_dev *pdev, int dvsec, u16 cap)
> +	__must_hold(cxl_region_quiesce)
>  {
> -	u16 saved_ctrl2;
> +	struct cxl_hdm_info *snapshot __free(kfree) = NULL;
> +	struct cxl_pci_cache cache = {};
>  	int rc, rc2;
>  
> -	rc = pci_read_config_word(pdev, dvsec + PCI_DVSEC_CXL_CTRL2, &saved_ctrl2);
> +	rc = pci_read_config_word(pdev, dvsec + PCI_DVSEC_CXL_CTRL2,
> +				  &cache.saved_ctrl2);
>  	if (rc)
>  		return pcibios_err_to_errno(rc);
> -	if (PCI_POSSIBLE_ERROR(saved_ctrl2))
> +	if (PCI_POSSIBLE_ERROR(cache.saved_ctrl2))
>  		return -ENODEV;
>  
> -	rc = cxl_reset_disable_cache(pdev, dvsec, cap);
> +	snapshot = cxl_hdm_cache_snapshot(&pdev->hdm);
> +	if (IS_ERR(snapshot)) {
> +		pci_err(pdev, "CXL HDM decoder state unavailable: %ld\n",
> +			PTR_ERR(snapshot));
> +		return PTR_ERR(snapshot);
> +	}
> +
> +	rc = cxl_reset_cache_prepare(pdev, dvsec, cap, snapshot, &cache);
> +	if (rc && !cache.cache_disabled) {
> +		pci_err(pdev, "failed to prepare CXL reset ranges: %d\n", rc);
> +		cxl_hdm_quiesce_destroy(&cache.quiesce);
> +		return rc;
> +	}
>  	if (!rc)
> -		rc = __cxl_reset_execute(pdev, dvsec, cap);
> +		rc = __cxl_reset_execute(pdev, dvsec, cap, &cache.quiesce);
>  	/* Restore cache policy after any attempt to disable caching. */
> -	rc2 = cxl_reset_restore_cache_policy(pdev, dvsec, saved_ctrl2);
> +	rc2 = cxl_reset_restore_cache_policy(pdev, dvsec, cache.saved_ctrl2);
> +	cxl_hdm_quiesce_destroy(&cache.quiesce);
>  	return rc ?: rc2;
>  }
>  
> +static int cxl_reset_execute(struct pci_dev *pdev, int dvsec, u16 cap)
> +{
> +	int rc;
> +
> +	cxl_region_quiesce_lock();
> +	rc = cxl_reset_quiesced(pdev, dvsec, cap);
> +	cxl_region_quiesce_unlock();
> +
> +	return rc;
> +}
> +
>  int cxl_reset_function(struct pci_dev *pdev, bool probe)
>  {
>  	u16 dvsec, cap, ctrl;
> @@ -460,5 +518,17 @@ int cxl_reset_function(struct pci_dev *pdev, bool probe)
>  	if (probe)
>  		return 0;
>  
> +	/* The cache is owned by @pdev and does not require a bound CXL driver. */
> +	if (!cxl_hdm_cache_present(&pdev->hdm))
> +		return -ENOTTY;
> +
> +	/* Memory decoded through DVSEC ranges has no HPA ranges to quiesce. */
> +	if (cxl_hdm_cache_dvsec_decode(&pdev->hdm))
> +		return -ENOTTY;
> +
> +	if (!cpu_cache_has_invalidate_memregion() &&
> +	    !IS_ENABLED(CONFIG_CXL_REGION_INVALIDATION_TEST))
> +		return -ENOTTY;
> +
>  	return cxl_reset_execute(pdev, dvsec, cap);
>  }
> diff --git a/include/cxl/hdm.h b/include/cxl/hdm.h
> index cfb55b10f8ac..7919d872a34d 100644
> --- a/include/cxl/hdm.h
> +++ b/include/cxl/hdm.h
> @@ -3,10 +3,14 @@
>  #ifndef __CXL_HDM_H__
>  #define __CXL_HDM_H__
>  
> +#include <linux/list.h>
>  #include <linux/types.h>
> +
>  #include <cxl/cxl.h>
>  
> +struct device;
>  struct pci_dev;
> +struct resource;
>  struct cxl_endpoint_decoder;
>  
>  /**
> @@ -45,7 +49,6 @@ struct cxl_hdm_info {
>  	bool dvsec_ctrl_valid;
>  	struct cxl_endpoint_decoder_settings settings[] __counted_by(decoder_count);
>  };
> -struct device;
>  
>  int cxl_hdm_decoder_count_mapped(struct device *dev, void __iomem *hdm,
>  				 resource_size_t size);
> @@ -59,5 +62,44 @@ int cxl_hdm_cache_check_decoder_count(struct pci_dev *pdev, int count);
>  void cxl_hdm_cache_publish(struct cxl_hdm_info **slot,
>  			   struct cxl_hdm_info *info);
>  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);
> +
> +/*
> + * Region quiesce blocks CXL region commit and teardown, and with them decoder
> + * reprogramming, while reset handling captures or restores HDM decoder state.
> + */
> +token_context_lock(cxl_region_quiesce);
> +
> +/**
> + * struct cxl_hdm_range - HPA range held exclusively across a reset
> + * @list: entry in &cxl_hdm_quiesce.ranges
> + * @hpa_range: system physical address range decoded by the device
> + * @len: length of @hpa_range
> + * @res: reservation of @hpa_range, once requested
> + */
> +struct cxl_hdm_range {
> +	struct list_head list;
> +	struct range hpa_range;
> +	u64 len;
> +	struct resource *res;
> +};
> +
> +/**
> + * struct cxl_hdm_quiesce - HPA ranges held exclusively across a reset
> + * @ranges: list of &struct cxl_hdm_range, see cxl_hdm_quiesce_prepare()
> + */
> +struct cxl_hdm_quiesce {
> +	struct list_head ranges;
> +};
> +
> +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);
>  
>  #endif /* __CXL_HDM_H__ */
> -- 
> 2.43.0
>