Re: [PATCH v15 09/16] PCI/CXL: Cache endpoint HDM state during PCI enumeration

[Alison Schofield <alison.schofield@intel.com>]

On Sun, Oct 11, 2026 at 02:14:15AM +0000, Srirangan Madhavan wrote:
> Cache HDM state in pci_dev->hdm after BAR resource setup and before driver
> binding, so reset can restore it without a bound CXL driver.
> 
> Save the HDM register location, decoder settings, and Device DVSEC state.
> Validate capability reads and decoder counts, and restore PCI_COMMAND
> after temporary MMIO access, reporting restoration failures.
> 
> Keep PCI discovery separate from shared HDM helpers. Move cxl_rwsem into
> built-in state code and protect cache publication with its DPA lock, so
> the cache remains usable when cxl_core is modular.

Hi Srirangan,

I felt the large-ness of this patch, so I'm coming back to the top
here to add this comment after doing the inline review below.

The commit message doesn't mention much of the preparation work that's
included. Could that prep be presented first in prep patches, and then
add the new code?  Something like:

cxl: Build register block helpers into the kernel
- regs.o moves from cxl_core into the built-in objects
- devm_cxl_iomap_block(), cxl_rcrb_to_aer(), __rcrb_to_component()
  and cxl_rcd_component_reg_phys() gain exports
- cxl_find_regblock() and cxl_setup_regs() move to include/cxl/pci.h
- struct cxl_register_map gains bar and bar_offset

cxl: Move cxl_rwsem into built-in code
- a new hdm_state.c holds cxl_rwsem and its export, and hdm.c drops it


More feedback inline below -

> 
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/commit/?id=b12baa1105f33cc1a10f9c9c0c6376194397d9f3
> Suggested-by: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: Srirangan Madhavan <smadhavan@nvidia.com>
> Assisted-by: LLM
> ---
>  MAINTAINERS                  |   1 +
>  drivers/cxl/core/Makefile    |   3 +-
>  drivers/cxl/core/hdm.c       |   5 -
>  drivers/cxl/core/hdm_regs.c  |  99 ++++++++++++++++
>  drivers/cxl/core/hdm_state.c |  64 ++++++++++
>  drivers/cxl/core/pci.c       |  17 ++-
>  drivers/cxl/core/regs.c      |   9 ++
>  drivers/cxl/cxl.h            |   3 -
>  drivers/pci/Makefile         |   1 +
>  drivers/pci/bus.c            |   1 +
>  drivers/pci/cxl.c            | 223 +++++++++++++++++++++++++++++++++++
>  drivers/pci/pci.h            |   8 ++
>  drivers/pci/probe.c          |   1 +
>  include/cxl/cxl.h            |   6 +
>  include/cxl/hdm.h            |  12 ++
>  include/cxl/pci.h            |  13 ++
>  tools/testing/cxl/Kbuild     |   1 -
>  17 files changed, 454 insertions(+), 13 deletions(-)
>  create mode 100644 drivers/cxl/core/hdm_state.c
>  create mode 100644 drivers/pci/cxl.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c2414447892c..b874340b0265 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6572,6 +6572,7 @@ S:	Maintained
>  F:	Documentation/driver-api/cxl
>  F:	Documentation/userspace-api/fwctl/fwctl-cxl.rst
>  F:	drivers/cxl/
> +F:	drivers/pci/cxl.c
>  F:	include/cxl/
>  F:	include/uapi/linux/cxl_mem.h
>  F:	tools/testing/cxl/
> diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
> index 6703c1729396..9a77e68d44ce 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-$(subst m,y,$(CONFIG_CXL_BUS)) += hdm_regs.o
> +obj-$(subst m,y,$(CONFIG_CXL_BUS)) += hdm_regs.o hdm_state.o regs.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

Does all of reg.c need to be built in? I'm implying that that you'd
only build in the part that PCI core needs and leave the rest in
cxl_core. Maybe moving more code around is not worth it. Just noticing.


snip


> +static int cxl_hdm_read_decoder_settings(struct device *dev,
> +					 struct cxl_endpoint_decoder_settings *settings,
> +					 void __iomem *hdm, int id)
> +{
> +	u64 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));
> +	rc = cxl_hdm_unpack_decoder(&settings->config, id, ctrl, base, size);


This duplicates work of init_hdm_decoder().
Could the new shared helper _unpack_decoder() grow to do the register
reads also, so the code is shared. Also, then you could do the reads
in low then high order, like init_hdm_decoder() does and like the
rest of the CXL code reads its Low/High register pairs.

snip

> +/* A published cache is usable only if its HDM register block was mapped. */
> +bool cxl_hdm_cache_present(struct cxl_hdm_info *const *slot)
> +{
> +	guard(rwsem_read)(&cxl_rwsem.dpa);
> +	return *slot && (*slot)->hdm_size;
> +}

Add a blank line before the final return please. (subsystem style)

snip

>  
> +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;



Could hdm_state.c add accessors for this, like the existing
cxl_hdm_cache_present() and cxl_hdm_cache_publish()? That way cxl_core
doesn't write pdev->hdm directly and the dpa locking stays in one place.
The same could be done for cxl_hdm_get_cached_info() and
cxl_hdm_check_decoder_count() added in Patch 8/16.


> +}
> +
>  static int cxl_set_mem_enable(struct cxl_dev_state *cxlds, u16 val)
>  {
>  	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> @@ -261,8 +271,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;
>  
> @@ -270,6 +282,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;


Can this function end in a single return, with the 0 or 1 result
in a variable, so that the cache is updated in one place?

snip

> @@ -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");


Why not EXPORT_SYMBOL_FOR_MODULES(..., "cxl_core") here, like the hdm_regs.c
helpers?

snip

> +static struct cxl_hdm_info *__cxl_pci_hdm_read_info(struct pci_dev *pdev,
> +						    struct cxl_register_map *map,
> +						    int dvsec)
> +{
> +	struct cxl_reg_map *hdm_map = &map->component_map.hdm_decoder;
> +	resource_size_t hdm_offset;
> +	void __iomem *hdm;
> +	int decoder_count;
> +	u16 dvsec_ctrl;
> +	int rc;
> +
> +	hdm = cxl_pci_hdm_map_regs(pdev, map, &hdm_offset, &decoder_count);
> +	if (IS_ERR(hdm))
> +		return ERR_CAST(hdm);
> +
> +	rc = pci_read_config_word(pdev, dvsec + PCI_DVSEC_CXL_CTRL,
> +				  &dvsec_ctrl);
> +	if (rc) {
> +		rc = pcibios_err_to_errno(rc);
> +		goto out_unmap;
> +	}
> +
> +	struct cxl_hdm_info *info =
> +		kzalloc_flex(*info, settings, decoder_count, GFP_KERNEL);

Move declaration to top of func.
checkpatch should be complaining about mixing statement & declarations.


> +	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->dvsec = dvsec;
> +	info->dvsec_ctrl = dvsec_ctrl;
> +	info->dvsec_ctrl_valid = true;
> +
> +	rc = cxl_hdm_read_state(&pdev->dev, hdm, info);
> +	if (rc)
> +		goto out_free;
> +
> +	iounmap(hdm);
> +	return info;
> +
> +out_free:
> +	kfree(info);
> +out_unmap:
> +	iounmap(hdm);
> +	return ERR_PTR(rc);
> +}
> +
> +static struct cxl_hdm_info *cxl_pci_hdm_read_info(struct pci_dev *pdev,
> +						  struct cxl_register_map *map,
> +						  int dvsec)
> +{
> +	struct cxl_hdm_info *info;
> +	u16 command;
> +	int rc;
> +
> +	guard(pci_dev)(pdev);
> +
> +	rc = pci_read_config_word(pdev, PCI_COMMAND, &command);
> +	if (rc)
> +		return ERR_PTR(pcibios_err_to_errno(rc));
> +
> +	if (command & PCI_COMMAND_MEMORY)
> +		return __cxl_pci_hdm_read_info(pdev, map, dvsec);
> +
> +	rc = pci_write_config_word(pdev, PCI_COMMAND,
> +				   command | PCI_COMMAND_MEMORY);
> +	if (rc)
> +		return ERR_PTR(pcibios_err_to_errno(rc));
> +
> +	info = __cxl_pci_hdm_read_info(pdev, map, dvsec);
> +	rc = pci_write_config_word(pdev, PCI_COMMAND, command);
> +	if (!rc)
> +		return info;
> +
> +	pci_err(pdev,
> +		"failed to restore PCI_COMMAND after CXL HDM cache init: %d\n",
> +		pcibios_err_to_errno(rc));
> +	if (IS_ERR(info))
> +		return info;
> +
> +	kfree(info);
> +	return ERR_PTR(pcibios_err_to_errno(rc));


This returns the __cxl_pci_hdm_read_info() result from two places, and then
has three more returns after a failed PCI_COMMAND restore. Try to end in a
single return. Li Ming's suggestion to warn and keep the info on a failed
restore would also reduce the tail to one return info


> +}
> +
> +static int __pci_cxl_hdm_cache_init(struct pci_dev *pdev)
> +{
> +	struct cxl_register_map map = { };
> +	u16 dvsec, cap;
> +	int rc;
> +
> +	/* Avoid duplicate MMIO reads when the cache is already present. */
> +	if (cxl_hdm_cache_present(&pdev->hdm))
> +		return 0;
> +
> +	rc = cxl_pci_device_dvsec(pdev, &dvsec, &cap);
> +	if (rc == -ENOTTY)
> +		return 0;
> +	if (rc)
> +		return rc;
> +	if (!(cap & PCI_DVSEC_CXL_MEM_CAPABLE) ||
> +	    !cxl_dvsec_hdm_count_valid(FIELD_GET(PCI_DVSEC_CXL_HDM_COUNT, cap)))
> +		return 0;
> +

This cache is built for every function w CXL.mem, not just type2.
So I believe we are needlessly reading Type decoders at PCI enumeration.

Can this check the capability bits like cxl_reset_function() does 
and limit this work to type2 devices that can use it?

snip to end...

-- Alison