Re: [PATCH v13 09/15] cxl: Cache endpoint HDM state during PCI enumeration

["Cheatham, Benjamin" <benjamin.cheatham@amd.com>]

On 9/22/2026 3:39 AM, Srirangan Madhavan wrote:
> PCI capability initialization runs before BAR resources are finalized,
> while driver binding is too late for driver-independent reset support.
> Create pci_dev->hdm during pci_bus_add_device(), after PCI resource setup
> and before driver binding.
> 
> Cache BAR-relative HDM location, global control, decoder settings, and CXL
> Device DVSEC Control, then publish the completed cache under cxl_rwsem.dpa.
> Restore PCI_COMMAND after temporary MMIO access and reject decoder-count
> changes.
> 
> Signed-off-by: Srirangan Madhavan <smadhavan@nvidia.com>
> ---
>  drivers/cxl/core/Makefile   |   3 +-
>  drivers/cxl/core/pci.c      |  15 ++-
>  drivers/cxl/core/regs.c     |   9 ++
>  drivers/cxl/core/resource.c | 255 ++++++++++++++++++++++++++++++++++++
>  drivers/pci/bus.c           |   2 +
>  drivers/pci/probe.c         |   2 +
>  include/cxl/cxl.h           |  21 +++
>  tools/testing/cxl/Kbuild    |   1 -
>  8 files changed, 304 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
> index 30470799e059..2335059f687a 100644
> --- a/drivers/cxl/core/Makefile
> +++ b/drivers/cxl/core/Makefile
> @@ -1,6 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_CXL_BUS) += cxl_core.o
> -obj-$(CONFIG_CXL_RESET) += resource.o
> +obj-$(CONFIG_CXL_RESET) += regs.o resource.o
>  obj-$(CONFIG_CXL_SUSPEND) += suspend.o
>  
>  ccflags-y += -I$(srctree)/drivers/cxl
> @@ -8,7 +8,6 @@ CFLAGS_trace.o = -DTRACE_INCLUDE_PATH=. -I$(src)
>  
>  cxl_core-y := port.o
>  cxl_core-y += pmem.o
> -cxl_core-y += regs.o
>  cxl_core-y += memdev.o
>  cxl_core-y += mbox.o
>  cxl_core-y += pci.o
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index bf7fc77626d9..cf7fefd595a2 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -180,6 +180,16 @@ int cxl_await_media_ready(struct cxl_dev_state *cxlds)
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_await_media_ready, "CXL");
>  
> +static void cxl_hdm_cache_dvsec_ctrl(struct pci_dev *pdev, u16 ctrl)
> +{
> +	guard(rwsem_write)(&cxl_rwsem.dpa);
> +	if (!pdev->hdm)
> +		return;
> +
> +	pdev->hdm->dvsec_ctrl = ctrl;
> +	pdev->hdm->dvsec_ctrl_valid = true;
> +}
> +
>  static int cxl_set_mem_enable(struct cxl_dev_state *cxlds, u16 val)
>  {
>  	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> @@ -191,8 +201,10 @@ static int cxl_set_mem_enable(struct cxl_dev_state *cxlds, u16 val)
>  	if (rc)
>  		return pcibios_err_to_errno(rc);
>  
> -	if ((ctrl & PCI_DVSEC_CXL_MEM_ENABLE) == val)
> +	if ((ctrl & PCI_DVSEC_CXL_MEM_ENABLE) == val) {
> +		cxl_hdm_cache_dvsec_ctrl(pdev, ctrl);
>  		return 1;
> +	}
>  	ctrl &= ~PCI_DVSEC_CXL_MEM_ENABLE;
>  	ctrl |= val;
>  
> @@ -200,6 +212,7 @@ static int cxl_set_mem_enable(struct cxl_dev_state *cxlds, u16 val)
>  	if (rc)
>  		return pcibios_err_to_errno(rc);
>  
> +	cxl_hdm_cache_dvsec_ctrl(pdev, ctrl);
>  	return 0;
>  }
>  
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index 20c2d9fbcfe7..ba1f942d20e1 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -200,6 +200,7 @@ void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
>  
>  	return ret_val;
>  }
> +EXPORT_SYMBOL_NS_GPL(devm_cxl_iomap_block, "CXL");
>  
>  int cxl_map_component_regs(const struct cxl_register_map *map,
>  			   struct cxl_component_regs *regs,
> @@ -285,6 +286,8 @@ static bool cxl_decode_regblock(struct pci_dev *pdev, u32 reg_lo, u32 reg_hi,
>  	}
>  
>  	map->reg_type = reg_type;
> +	map->bar = bar;
> +	map->bar_offset = offset;
>  	map->resource = pci_resource_start(pdev, bar) + offset;
>  	map->max_size = pci_resource_len(pdev, bar) - offset;
>  	return true;
> @@ -309,6 +312,7 @@ static int __cxl_find_regblock_instance(struct pci_dev *pdev, enum cxl_regloc_ty
>  	*map = (struct cxl_register_map) {
>  		.host = &pdev->dev,
>  		.resource = CXL_RESOURCE_NONE,
> +		.bar = -1,
>  	};
>  
>  	regloc = pci_find_dvsec_capability(pdev, PCI_VENDOR_ID_CXL,
> @@ -339,6 +343,8 @@ static int __cxl_find_regblock_instance(struct pci_dev *pdev, enum cxl_regloc_ty
>  	}
>  
>  	map->resource = CXL_RESOURCE_NONE;
> +	map->bar = -1;
> +	map->bar_offset = 0;
>  	if (index == CXL_INSTANCES_COUNT)
>  		return instance;
>  
> @@ -518,6 +524,7 @@ u16 cxl_rcrb_to_aer(struct device *dev, resource_size_t rcrb)
>  
>  	return offset;
>  }
> +EXPORT_SYMBOL_NS_GPL(cxl_rcrb_to_aer, "CXL");
>  
>  static resource_size_t cxl_rcrb_to_linkcap(struct device *dev, struct cxl_dport *dport)
>  {
> @@ -634,6 +641,7 @@ resource_size_t __rcrb_to_component(struct device *dev, struct cxl_rcrb_info *ri
>  
>  	return component_reg_phys;
>  }
> +EXPORT_SYMBOL_NS_GPL(__rcrb_to_component, "CXL");
>  
>  resource_size_t cxl_rcd_component_reg_phys(struct device *dev,
>  					   struct cxl_dport *dport)
> @@ -642,3 +650,4 @@ resource_size_t cxl_rcd_component_reg_phys(struct device *dev,
>  		return CXL_RESOURCE_NONE;
>  	return __rcrb_to_component(dev, &dport->rcrb, CXL_RCRB_UPSTREAM);
>  }
> +EXPORT_SYMBOL_NS_GPL(cxl_rcd_component_reg_phys, "CXL");
> diff --git a/drivers/cxl/core/resource.c b/drivers/cxl/core/resource.c
> index 7eea2bc38d94..b5f8c28e613f 100644
> --- a/drivers/cxl/core/resource.c
> +++ b/drivers/cxl/core/resource.c
> @@ -2,9 +2,17 @@
>  /* Copyright (c) 2026 NVIDIA Corporation & Affiliates */
>  #include <linux/delay.h>
>  #include <linux/bug.h>
> +#include <linux/bitfield.h>
> +#include <linux/cleanup.h>
>  #include <linux/errno.h>
>  #include <linux/export.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
>  #include <linux/kernel.h>
> +#include <linux/pci.h>
> +#include <linux/slab.h>
> +
> +#include <cxl/pci.h>
>  
>  #include "cxl.h"
>  #include "core.h"
> @@ -164,3 +172,250 @@ int cxl_hdm_unpack_decoder(struct cxl_decoder_settings *settings, int id,
>  	return 0;
>  }
>  EXPORT_SYMBOL_FOR_MODULES(cxl_hdm_unpack_decoder, "cxl_core");
> +
> +static int cxl_pci_hdm_dvsec(struct pci_dev *pdev)
> +{
> +	u16 cap;
> +	int dvsec;
> +	int rc;
> +
> +	dvsec = pci_find_dvsec_capability(pdev, PCI_VENDOR_ID_CXL,
> +					  PCI_DVSEC_CXL_DEVICE);
> +	if (!dvsec)
> +		return -ENOTTY;

Why -ENOTTY here and below? I would've expected -ENXIO for a missing capability.

> +
> +	rc = pci_read_config_word(pdev, dvsec + PCI_DVSEC_CXL_CAP, &cap);
> +	if (rc)
> +		return pcibios_err_to_errno(rc);
> +	if (!(cap & PCI_DVSEC_CXL_MEM_CAPABLE) ||
> +	    !FIELD_GET(PCI_DVSEC_CXL_HDM_COUNT, cap))
> +		return -ENOTTY;
> +
> +	return dvsec;
> +}
> +
> +static int cxl_pci_hdm_read_decoder(struct pci_dev *pdev,
> +				    struct cxl_decoder_settings *settings,
> +				    void __iomem *hdm, int id)
> +{
> +	u64 target_or_skip, base, size;
> +	u32 ctrl;
> +	int rc;
> +
> +	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(id));
> +	base = ((u64)readl(hdm + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(id)) << 32) |
> +	       readl(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(id));
> +	size = ((u64)readl(hdm + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(id)) << 32) |
> +	       readl(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(id));
> +	target_or_skip =
> +		((u64)readl(hdm + CXL_HDM_DECODER0_TL_HIGH(id)) << 32) |
> +		readl(hdm + CXL_HDM_DECODER0_TL_LOW(id));
> +
> +	rc = cxl_hdm_unpack_decoder(settings, id, ctrl, base, size,
> +				    target_or_skip);
> +	if (rc) {
> +		pci_err(pdev,
> +			"CXL HDM decoder %d has invalid configuration: %d\n",
> +			id, rc);
> +		return rc;
> +	}
> +
> +	if (!(settings->config.flags & CXL_DECODER_F_ENABLE))
> +		*settings = (struct cxl_decoder_settings) {
> +			.config.id = id,
> +		};
> +
> +	return 0;
> +}
> +
> +static int __cxl_pci_hdm_read_info(struct pci_dev *pdev,
> +				   struct cxl_register_map *map, int dvsec,
> +				   struct cxl_hdm_info **out)
> +{
> +	struct cxl_reg_map *hdm_map = &map->component_map.hdm_decoder;
> +	struct cxl_hdm_info *info = NULL;
> +	resource_size_t hdm_offset;
> +	void __iomem *hdm;
> +	int decoder_count;
> +	size_t required;
> +	u16 dvsec_ctrl;
> +	int rc;
> +
> +	rc = cxl_setup_regs(map);
> +	if (rc)
> +		return rc;
> +	if (!hdm_map->valid)
> +		return -ENODEV;
> +	if (hdm_map->offset > map->max_size ||
> +	    hdm_map->size > map->max_size - hdm_map->offset)
> +		return -ENXIO;
> +	if (check_add_overflow(map->bar_offset,
> +			       (resource_size_t)hdm_map->offset, &hdm_offset))
> +		return -EOVERFLOW;
> +
> +	hdm = ioremap(map->resource + hdm_map->offset, hdm_map->size);
> +	if (!hdm)
> +		return -ENOMEM;
> +
> +	decoder_count = cxl_hdm_decoder_count(readl(hdm + CXL_HDM_DECODER_CAP_OFFSET));
> +	if (decoder_count < 0) {
> +		rc = decoder_count;
> +		goto out_unmap;
> +	}
> +
> +	required = CXL_HDM_DECODER0_TL_HIGH(decoder_count - 1) +
> +		   sizeof(u32);
> +	if (required > hdm_map->size) {
> +		pci_err(pdev,
> +			"CXL HDM decoder count exceeds mapped register block\n");
> +		rc = -ENXIO;
> +		goto out_unmap;
> +	}
> +
> +	rc = pci_read_config_word(pdev, dvsec + PCI_DVSEC_CXL_CTRL,
> +				  &dvsec_ctrl);
> +	if (rc) {
> +		rc = pcibios_err_to_errno(rc);
> +		goto out_unmap;
> +	}
> +
> +	info = kzalloc(struct_size(info, settings, decoder_count), GFP_KERNEL);
> +	if (!info) {
> +		rc = -ENOMEM;
> +		goto out_unmap;
> +	}
> +
> +	info->decoder_count = decoder_count;
> +	info->hdm_bar = map->bar;
> +	info->hdm_offset = hdm_offset;
> +	info->hdm_size = hdm_map->size;
> +	info->global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
> +	info->dvsec_ctrl = dvsec_ctrl;
> +	info->dvsec_ctrl_valid = true;
> +
> +	for (int i = 0; i < decoder_count; i++) {
> +		rc = cxl_pci_hdm_read_decoder(pdev, &info->settings[i], hdm, i);
> +		if (rc)
> +			goto out_unmap;
> +	}
> +
> +	/*
> +	 * A changed decoder count means the snapshot no longer describes the
> +	 * complete hardware state. Reject it rather than publish a partial cache.
> +	 */
> +	rc = cxl_hdm_decoder_count(readl(hdm + CXL_HDM_DECODER_CAP_OFFSET));
> +	if (rc != decoder_count) {
> +		pci_err(pdev, "CXL HDM decoder count changed from %d to %d\n",
> +			decoder_count, rc);
> +		rc = -ENXIO;
> +		goto out_unmap;
> +	}
> +
> +	*out = info;
> +	info = NULL;
> +	rc = 0;
> +
> +out_unmap:
> +	kfree(info);
> +	iounmap(hdm);
> +	return rc;
> +}
> +
> +static int cxl_pci_hdm_read_info(struct pci_dev *pdev,
> +				 struct cxl_register_map *map, int dvsec,
> +				 struct cxl_hdm_info **out)
> +{
> +	bool restore_command = false;
> +	u16 command;
> +	int rc, rc2;
> +
> +	guard(pci_dev)(pdev);
> +
> +	rc = pci_read_config_word(pdev, PCI_COMMAND, &command);
> +	if (rc)
> +		return pcibios_err_to_errno(rc);
> +
> +	if (!(command & PCI_COMMAND_MEMORY)) {
> +		rc = pci_write_config_word(pdev, PCI_COMMAND,
> +					   command | PCI_COMMAND_MEMORY);
> +		if (rc)
> +			return pcibios_err_to_errno(rc);
> +		restore_command = true;
> +	}
> +
> +	rc = __cxl_pci_hdm_read_info(pdev, map, dvsec, out);
> +	if (!restore_command)
> +		return rc;
> +
> +	if (!rc) {
> +		rc = pci_write_config_word(pdev, PCI_COMMAND, command);
> +		return pcibios_err_to_errno(rc);
> +	}
> +
> +	rc2 = pci_write_config_word(pdev, PCI_COMMAND, command);
> +	if (rc2)
> +		pci_err(pdev,
> +			"failed to restore PCI_COMMAND after CXL HDM cache init: %d\n",
> +			pcibios_err_to_errno(rc2));
> +
> +	return rc;


I think the above would flow better as:

rc = __cxl_pci_hdm_read_info(pdev, map, dvsec, out);
if (rc) {
	if (!restore_command)
		return rc;

	rc2 = pci_write_config_word(pdev, PCI_COMMAND, command);
	if (rc2)
		...
} else {
	rc = pci_write_config_word(pdev, PCI_COMMAND, command);
	return pcibios_err_to_errno(rc);
}

return rc;

> +}
> +
> +static int __pci_cxl_hdm_cache_init(struct pci_dev *pdev)
> +{
> +	struct cxl_hdm_info *info __free(kfree) = NULL;
> +	struct cxl_register_map map = { };
> +	struct resource *bar;
> +	int dvsec;
> +	int rc;
> +
> +	scoped_guard(rwsem_read, &cxl_rwsem.dpa)
> +		if (pdev->hdm)
> +			return 0;
> +
> +	dvsec = cxl_pci_hdm_dvsec(pdev);
> +	if (dvsec < 0)
> +		return dvsec;

Might be better to just return 0 here. If the device doesn't have a CXL capability
then I wouldn't consider that an error from the perspective of the PCI subsystem.

> +
> +	rc = cxl_find_regblock(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
> +	if (rc)
> +		return rc;
> +	if (map.bar < 0 || map.bar >= PCI_STD_NUM_BARS)
> +		return -ENODEV;
> +
> +	bar = &pdev->resource[map.bar];
> +	if (!pci_resource_len(pdev, map.bar) ||
> +	    (bar->flags & (IORESOURCE_UNSET | IORESOURCE_DISABLED)) ||
> +	    resource_type(bar) != IORESOURCE_MEM || !bar->start || !bar->end)
> +		return -ENODEV;
> +
> +	rc = cxl_pci_hdm_read_info(pdev, &map, dvsec, &info);
> +	if (rc)
> +		return rc;
> +
> +	guard(rwsem_write)(&cxl_rwsem.dpa);
> +	if (!pdev->hdm)
> +		pdev->hdm = no_free_ptr(info);
> +
> +	return 0;
> +}
> +
> +void pci_cxl_hdm_cache_init(struct pci_dev *pdev)
> +{
> +	int rc;
> +
> +	rc = __pci_cxl_hdm_cache_init(pdev);
> +	if (rc && rc != -ENOTTY && rc != -ENODEV)
> +		pci_dbg(pdev, "CXL HDM cache init failed: %d\n", rc);

I don't know if it's worth filtering out -ENOTTY and -ENODEV for a debug message,
but that's more of a question for the PCI people.

> +}
> +EXPORT_SYMBOL_FOR_MODULES(pci_cxl_hdm_cache_init, "cxl_core");
> +
> +void pci_cxl_hdm_cache_release(struct pci_dev *pdev)
> +{
> +	struct cxl_hdm_info *info;
> +
> +	guard(rwsem_write)(&cxl_rwsem.dpa);
> +	info = pdev->hdm;
> +	pdev->hdm = NULL;
> +	kfree(info);
> +}
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 655ed53436d3..d866452222b6 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -18,6 +18,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/proc_fs.h>
>  #include <linux/slab.h>
> +#include <cxl/cxl.h>
>  
>  #include "pci.h"
>  
> @@ -359,6 +360,7 @@ void pci_bus_add_device(struct pci_dev *dev)
>  
>  	/* Save config space for error recoverability */
>  	pci_save_state(dev);
> +	pci_cxl_hdm_cache_init(dev);
>  
>  	/*
>  	 * Enable runtime PM, which potentially allows the device to
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 27008e2ea5af..752347464a4d 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -24,6 +24,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/bitfield.h>
>  #include <trace/events/pci.h>
> +#include <cxl/cxl.h>
>  #include "pci.h"
>  
>  static struct resource busn_resource = {
> @@ -2484,6 +2485,7 @@ static void pci_release_dev(struct device *dev)
>  	struct pci_dev *pci_dev;
>  
>  	pci_dev = to_pci_dev(dev);
> +	pci_cxl_hdm_cache_release(pci_dev);
>  	pci_release_capabilities(pci_dev);
>  	pci_release_of_node(pci_dev);
>  	pcibios_release_device(pci_dev);
> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
> index 005259e38f4b..4bad41e74f74 100644
> --- a/include/cxl/cxl.h
> +++ b/include/cxl/cxl.h
> @@ -26,6 +26,7 @@ enum cxl_devtype {
>  };
>  
>  struct cxl_region;
> +struct pci_dev;
>  
>  enum cxl_decoder_type {
>  	CXL_DECODER_DEVMEM = 2,
> @@ -87,6 +88,9 @@ struct cxl_decoder_settings {
>   * @dvsec_ctrl: CXL DVSEC control register
>   * @dvsec_ctrl_valid: whether @dvsec_ctrl contains valid state
>   * @settings: per-decoder programming state
> + *
> + * Publication through pci_dev.hdm and all mutable fields are protected by
> + * cxl_rwsem.dpa.
>   */
>  struct cxl_hdm_info {
>  	int decoder_count;
> @@ -172,6 +176,8 @@ struct cxl_pmu_reg_map {
>   * @base: virtual base of the register-block-BAR + @block_offset
>   * @resource: physical resource base of the register block
>   * @max_size: maximum mapping size to perform register search
> + * @bar: PCI BAR containing the register block
> + * @bar_offset: register block offset relative to @bar
>   * @reg_type: see enum cxl_regloc_type
>   * @component_map: cxl_reg_map for component registers
>   * @device_map: cxl_reg_maps for device registers
> @@ -182,6 +188,8 @@ struct cxl_register_map {
>  	void __iomem *base;
>  	resource_size_t resource;
>  	resource_size_t max_size;
> +	int bar;
> +	resource_size_t bar_offset;
>  	u8 reg_type;
>  	union {
>  		struct cxl_component_reg_map component_map;
> @@ -190,6 +198,19 @@ struct cxl_register_map {
>  	};
>  };
>  
> +#ifdef CONFIG_CXL_RESET
> +void pci_cxl_hdm_cache_init(struct pci_dev *pdev);
> +void pci_cxl_hdm_cache_release(struct pci_dev *pdev);
> +#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)
> +{
> +}
> +#endif
> +
>  /**
>   * struct cxl_dpa_perf - DPA performance property entry
>   * @dpa_range: range for DPA address
> diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
> index 2be1df80fcc9..e80500f457a9 100644
> --- a/tools/testing/cxl/Kbuild
> +++ b/tools/testing/cxl/Kbuild
> @@ -55,7 +55,6 @@ obj-m += cxl_core.o
>  
>  cxl_core-y := $(CXL_CORE_SRC)/port.o
>  cxl_core-y += $(CXL_CORE_SRC)/pmem.o
> -cxl_core-y += $(CXL_CORE_SRC)/regs.o

Did you run CXL test with this change? I don't think there are any test symbols in regs.c, but you should
make sure this doesn't break anything there. You may not need to even fix anything if it does, but just
make a note of the regression.

>  cxl_core-y += $(CXL_CORE_SRC)/memdev.o
>  cxl_core-y += $(CXL_CORE_SRC)/mbox.o
>  cxl_core-y += $(CXL_CORE_SRC)/pci.o