Re: [PATCH v14 09/16] cxl: Refresh cached PCI HDM decoder settings

[Dave Jiang <dave.jiang@intel.com>]

On 10/1/26 2:22 AM, Srirangan Madhavan wrote:
> Early PCI discovery creates the HDM cache, while later CXL enumeration and
> decoder operations provide updated programming state.
> 
> Refresh the PCI snapshot when decoders are enumerated, committed, or reset
> so reset recovery need not walk the CXL topology. Ignore updates when no
> cache exists and reject decoder-count mismatches.
> 
> Signed-off-by: Srirangan Madhavan <smadhavan@nvidia.com>
> ---
>  drivers/cxl/core/hdm.c | 58 ++++++++++++++++++++++++++++++++++++++++++
>  include/cxl/cxl.h      | 22 ++++++++++++++++
>  include/linux/pci.h    |  6 +++++
>  3 files changed, 86 insertions(+)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index b6a8fe83d336..d6b3afbffa3e 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -753,6 +753,44 @@ static void cxl_decoder_snapshot(struct cxl_decoder *cxld,
>  	}
>  }
>  
> +static void cxl_hdm_refresh_decoder(struct cxl_hdm *cxlhdm,
> +				    struct cxl_decoder *cxld)
> +{
> +	struct cxl_port *port = cxlhdm->port;
> +	void __iomem *hdm = cxlhdm->regs.hdm_decoder;

reverse xmas tree pls.

> +	struct cxl_decoder_settings *settings;
> +	struct cxl_hdm_info *info;
> +	struct pci_dev *pdev __free(pci_dev_put) =
> +		cxl_port_get_uport_pci_dev(port);

You can move this outside of declaration block. It's one of the exceptions. 

Also, is there any reason to reference a PCI device specifically? Can't you just get the 'struct device' and use exclusively that in this function? Prefer to keep the function neutral here rather than hard code it to PCI and I don't see anything specific to PCI that needs the hard coding. core/hdm.c should use 'struct device'. It does not have a PCI dependency before this. Also you may break cxl_test as it uses platform devices instead of PCI devices.

As an aside, can you please do me a favor and run cxl_test for your series? Having it passing all regression tests is one of the requirements for us to accept CXL patches. We will help you fix any issues crop up if you need it. Of course adding new regression tests for reset to test the new core functions would be awesome and much appreciated. 

> +
> +	if (!pdev || !hdm)
> +		return;

Go ahead and do the block like:

	struct pci_dev *pdev __free(pci_dev_put) =
		cxl_port_get_uport_pci_dev(port);
	if (!pdev)
		return;

	if (!hdm)
		return;


> +
> +	guard(rwsem_write)(&cxl_rwsem.dpa);
> +	info = pdev->hdm;
> +	if (!info)
> +		return;

Could use a blank line here.

> +	if (cxld->config.id < 0 || cxld->config.id >= info->decoder_count) {

Maybe just create a local ptr for cxld->config given all the places you need to access its members.

> +		pci_warn(pdev, "CXL HDM decoder %d exceeds cached count %d\n",
> +			 cxld->config.id, info->decoder_count);
> +		return;
> +	}
> +
> +	info->global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
> +	settings = &info->settings[cxld->config.id];
> +
> +	/*
> +	 * A disabled decoder's software object may retain its old range and
> +	 * target state. Leave only the decoder id in the cached settings so stale
> +	 * state is not restored as an enabled decode.
> +	 */
> +	*settings = (struct cxl_decoder_settings) {
> +		.config.id = cxld->config.id,
> +	};
> +	if (cxld->config.flags & CXL_DECODER_F_ENABLE)
> +		cxl_decoder_snapshot(cxld, settings);
> +}
> +
>  static int cxl_decoder_commit(struct cxl_decoder *cxld)
>  {
>  	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> @@ -804,6 +842,7 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
>  	}
>  	port->commit_end++;
>  	cxld->config.flags |= CXL_DECODER_F_ENABLE;
> +	cxl_hdm_refresh_decoder(cxlhdm, cxld);
>  
>  	return 0;
>  }
> @@ -876,6 +915,7 @@ static void cxl_decoder_reset(struct cxl_decoder *cxld)
>  	writel(0, hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(id));
>  
>  	cxld->config.flags &= ~CXL_DECODER_F_ENABLE;
> +	cxl_hdm_refresh_decoder(cxlhdm, cxld);
>  
>  	/* Userspace is now responsible for reconfiguring this decoder */
>  	if (is_endpoint_decoder(&cxld->dev)) {
> @@ -1086,6 +1126,23 @@ static int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
>  	int i;
>  	u64 dpa_base = 0;
>  
> +	if (is_cxl_endpoint(port) && hdm) {
> +		struct pci_dev *pdev __free(pci_dev_put) =
> +			cxl_port_get_uport_pci_dev(port);
> +
> +		if (pdev) {
> +			guard(rwsem_read)(&cxl_rwsem.dpa);
> +
> +			if (pdev->hdm &&
> +			    pdev->hdm->decoder_count != cxlhdm->decoder_count) {
> +				pci_warn(pdev,
> +					 "CXL HDM cache decoder count mismatch: cached=%d hdm=%d\n",
> +					 pdev->hdm->decoder_count, cxlhdm->decoder_count);
> +				return -ENXIO;
> +			}
> +		}

So we can't have a 'struct pci_dev' in core/hdm.c without breaking cxl_test. I think we can move this block into a helper function that's in core/pci.c. Maybe call it something like cxl_check_cached_decoder_settings(). I'm open to ideas. That way cxl_test can create a mock function and override it. See something like cxl_await_media_ready() in tools/testing/cxl/ on how to fixup cxl_test. Until we add support code for decoder saving etc in cxl_test, it can just do nothing now.

DJ
> +	}
> +
>  	cxl_settle_decoders(cxlhdm);
>  
>  	for (i = 0; i < cxlhdm->decoder_count; i++) {
> @@ -1124,6 +1181,7 @@ static int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
>  			put_device(&cxld->dev);
>  			return rc;
>  		}
> +		cxl_hdm_refresh_decoder(cxlhdm, cxld);
>  		rc = add_hdm_decoder(port, cxld);
>  		if (rc) {
>  			dev_warn(&port->dev,
> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
> index 67c81be47fbb..e200c5c56b92 100644
> --- a/include/cxl/cxl.h
> +++ b/include/cxl/cxl.h
> @@ -82,6 +82,28 @@ struct cxl_decoder_settings {
>  	u64 target_or_skip_reg_val;
>  };
>  
> +/**
> + * struct cxl_hdm_info - cached CXL HDM state for a PCI device
> + * @decoder_count: number of entries in @settings
> + * @hdm_bar: PCI BAR containing the HDM decoder capability
> + * @hdm_offset: offset of the HDM decoder capability in @hdm_bar
> + * @hdm_size: size of the HDM decoder register block
> + * @global_ctrl: HDM decoder global control register
> + * @dvsec_ctrl: CXL DVSEC control register
> + * @dvsec_ctrl_valid: whether @dvsec_ctrl contains valid state
> + * @settings: per-decoder programming state
> + */
> +struct cxl_hdm_info {
> +	int decoder_count;
> +	int hdm_bar;
> +	resource_size_t hdm_offset;
> +	resource_size_t hdm_size;
> +	u32 global_ctrl;
> +	u16 dvsec_ctrl;
> +	bool dvsec_ctrl_valid;
> +	struct cxl_decoder_settings settings[] __counted_by(decoder_count);
> +};
> +
>  /*
>   * Using struct_group() allows for per register-block-type helper routines,
>   * without requiring block-type agnostic code to include the prefix.
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index d31a8d107b1e..7bb37fcb556d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -339,6 +339,9 @@ struct pcie_link_state;
>  struct pci_sriov;
>  struct pci_p2pdma;
>  struct rcec_ea;
> +#ifdef CONFIG_CXL_RESET
> +struct cxl_hdm_info;
> +#endif
>  
>  /* struct pci_dev - describes a PCI device
>   *
> @@ -566,6 +569,9 @@ struct pci_dev {
>  #ifdef CONFIG_PCI_DOE
>  	struct xarray	doe_mbs;	/* Data Object Exchange mailboxes */
>  #endif
> +#ifdef CONFIG_CXL_RESET
> +	struct cxl_hdm_info *hdm;	/* CXL HDM decoder state */
> +#endif
>  #ifdef CONFIG_PCI_NPEM
>  	struct npem	*npem;		/* Native PCIe Enclosure Management */
>  #endif