Re: [PATCH v10 04/12] cxl: Cache decoder settings on PCI devices

On 8/4/26 12:29 PM, Srirangan Madhavan wrote:
> Add CXL core plumbing to refresh a PCI device HDM decoder cache when
> decoders are enumerated, committed, or reset. PCI reset paths can use
> this snapshot to restore HDM programming without walking CXL topology
> during reset recovery.
> 
> The cache is populated by PCI-side discovery in a follow-on patch. Until
> then, the CXL core update path is a no-op when no PCI HDM cache is
> present.
> 
> Signed-off-by: Srirangan Madhavan <[email protected]>
> ---
>  drivers/cxl/core/hdm.c | 114 ++++++++++++++++++++++++++++++++++++++++-
>  include/cxl/cxl.h      |  12 +++++
>  include/linux/pci.h    |   6 +++
>  3 files changed, 131 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 6e132b4f092b..ec988e7b7c0c 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -84,6 +84,110 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
>  		cxlhdm->iw_cap_mask |= BIT(16);
>  }
>  
> +static int cxl_pci_hdm_info_match(struct pci_dev *pdev, int decoder_count,
> +				      bool *present)

Maybe this should just return a bool based on match.

bool cxl_pci_hdm_decoder_count_match()?

> +{
> +	struct cxl_hdm_info *info;
> +	int rc = 0;
> +
> +	*present = false;
> +	down_read(&cxl_rwsem.dpa);
> +	info = pdev->hdm;
> +	if (info) {
> +		*present = true;
> +		if (info->decoder_count != decoder_count) {
> +			pci_warn(pdev,
> +				 "CXL HDM cache decoder count mismatch: cached=%d hdm=%d\n",
> +				 info->decoder_count, decoder_count);
> +			rc = -ENXIO;
> +		}
> +	}
> +	up_read(&cxl_rwsem.dpa);
> +
> +	return rc;
> +}
> +
> +static int cxl_pci_setup_hdm_info(struct cxl_hdm *cxlhdm)

I'm not sure if this function name makes sense. Nothing is being setup here. I think the above function should just be __cxl_pci_hdm_decoder_count_match() and this function can just be cxl_pci_hdm_decoder_count_match(). Or just collpase the two into a single function.

> +{
> +	struct pci_dev *pdev __free(pci_dev_put) =
> +		cxl_port_get_uport_pci_dev(cxlhdm->port);
> +	bool present;
> +
> +	if (!pdev)
> +		return 0;
> +
> +	return cxl_pci_hdm_info_match(pdev, cxlhdm->decoder_count, &present);
> +}
> +
> +static u64 cxl_switch_target_list(struct cxl_switch_decoder *cxlsd)

cxl_switch_get_target_list()

> +{
> +	struct cxl_decoder *cxld = &cxlsd->cxld;
> +	u64 targets = 0;
> +	int ways = min(cxld->interleave_ways, cxlsd->nr_targets);
> +
> +	/* target_map[] holds the raw list before target[] is resolved. */
> +	for (int i = 0; i < ways && i < 8; i++) {
> +		u8 port_id;
> +
> +		if (cxlsd->target[i])
> +			port_id = cxlsd->target[i]->port_id;
> +		else
> +			port_id = cxld->target_map[i];
> +
> +		targets |= (u64)port_id << (i * 8);
> +	}
> +
> +	return targets;
> +}
> +
> +static void cxl_decoder_snapshot(struct cxl_decoder *cxld,
> +				 struct cxl_decoder_settings *settings)
> +{

Probably a good idea to add
lockdep_assert_held_write(&cxl_rwsem.dpa);

> +	*settings = (struct cxl_decoder_settings) {
> +		.id = cxld->id,
> +		.hpa_range = cxld->hpa_range,
> +		.interleave_ways = cxld->interleave_ways,
> +		.interleave_granularity = cxld->interleave_granularity,
> +		.target_type = cxld->target_type,
> +		.flags = cxld->flags,
> +	};
> +
> +	if (is_endpoint_decoder(&cxld->dev)) {
> +		struct cxl_endpoint_decoder *cxled =
> +			to_cxl_endpoint_decoder(&cxld->dev);
> +
> +		settings->target_or_skip = cxled->skip;
> +	} else if (is_switch_decoder(&cxld->dev)) {
> +		struct cxl_switch_decoder *cxlsd =
> +			to_cxl_switch_decoder(&cxld->dev);
> +
> +		settings->target_or_skip = cxl_switch_target_list(cxlsd);
> +	}
> +}
> +
> +static void cxl_hdm_info_set_decoder(struct cxl_hdm *cxlhdm,
> +				     struct cxl_decoder *cxld)

cxl_hdm_cache(or save)_decoder_info()?

> +{
> +	struct pci_dev *pdev __free(pci_dev_put) =
> +		cxl_port_get_uport_pci_dev(cxlhdm->port);
> +	struct cxl_hdm_info *info;
> +
> +	if (!pdev)
> +		return;
> +
> +	guard(rwsem_write)(&cxl_rwsem.dpa);
> +	info = pdev->hdm;
> +	if (!info || cxld->id >= info->decoder_count)
> +		return;
> +
> +	if (cxld->flags & CXL_DECODER_F_ENABLE)
> +		cxl_decoder_snapshot(cxld, &info->settings[cxld->id]);
> +	else
> +		info->settings[cxld->id] = (struct cxl_decoder_settings) {
> +			.id = cxld->id,
> +		};

Use {} for if/else since the else part is multi-lines. Although given that you have to set the id either way, maybe have a helper function cxl_decoder_settings_init() that does it so you don't need the else branch.

DJ

> +}
> +
>  static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
>  {
>  	struct cxl_hdm *cxlhdm;
> @@ -767,6 +871,7 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
>  	}
>  	port->commit_end++;
>  	cxld->flags |= CXL_DECODER_F_ENABLE;
> +	cxl_hdm_info_set_decoder(cxlhdm, cxld);
>  
>  	return 0;
>  }
> @@ -839,6 +944,7 @@ static void cxl_decoder_reset(struct cxl_decoder *cxld)
>  	writel(0, hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(id));
>  
>  	cxld->flags &= ~CXL_DECODER_F_ENABLE;
> +	cxl_hdm_info_set_decoder(cxlhdm, cxld);
>  
>  	/* Userspace is now responsible for reconfiguring this decoder */
>  	if (is_endpoint_decoder(&cxld->dev)) {
> @@ -1058,11 +1164,16 @@ static int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
>  	struct cxl_port *port = cxlhdm->port;
>  	int i;
>  	u64 dpa_base = 0;
> +	int rc;
>  
>  	cxl_settle_decoders(cxlhdm);
>  
> +	rc = cxl_pci_setup_hdm_info(cxlhdm);
> +	if (rc)
> +		return rc;
> +
>  	for (i = 0; i < cxlhdm->decoder_count; i++) {
> -		int rc, target_count = cxlhdm->target_count;
> +		int target_count = cxlhdm->target_count;
>  		struct cxl_decoder *cxld;
>  
>  		if (is_cxl_endpoint(port)) {
> @@ -1097,6 +1208,7 @@ static int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
>  			put_device(&cxld->dev);
>  			return rc;
>  		}
> +		cxl_hdm_info_set_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 85e895d9b31c..703285966946 100644
> --- a/include/cxl/cxl.h
> +++ b/include/cxl/cxl.h
> @@ -133,6 +133,18 @@ struct cxl_regs {
>  	);
>  };
>  
> +#define CXL_HDM_DECODER_MAX_COUNT 32
> +
> +/**
> + * struct cxl_hdm_info - PCI device HDM decoder programming cache
> + * @decoder_count: number of decoder settings entries
> + * @settings: cached per-decoder programming state
> + */
> +struct cxl_hdm_info {
> +	int decoder_count;
> +	struct cxl_decoder_settings settings[CXL_HDM_DECODER_MAX_COUNT];
> +};
> +
>  struct cxl_reg_map {
>  	bool valid;
>  	int id;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 64b308b6e61c..a8e5cec96bae 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -335,6 +335,9 @@ struct pcie_link_state;
>  struct pci_sriov;
>  struct pci_p2pdma;
>  struct rcec_ea;
> +#ifdef CONFIG_CXL_HDM
> +struct cxl_hdm_info;
> +#endif
>  
>  /* struct pci_dev - describes a PCI device
>   *
> @@ -562,6 +565,9 @@ struct pci_dev {
>  #ifdef CONFIG_PCI_DOE
>  	struct xarray	doe_mbs;	/* Data Object Exchange mailboxes */
>  #endif
> +#ifdef CONFIG_CXL_HDM
> +	struct cxl_hdm_info *hdm;	/* CXL HDM decoder reset state */
> +#endif
>  #ifdef CONFIG_PCI_NPEM
>  	struct npem	*npem;		/* Native PCIe Enclosure Management */
>  #endif