Re: [RFC PATCH] cxl/core: reenable Mem_Enable bit of DVSEC control when RR decodes outside platform ranges

[Gregory Price <gourry@gourry.net>]

On Sun, Apr 06, 2025 at 07:27:52PM +0800, Huaisheng Ye wrote:
> In some scenarios, the probe of endpoint ports would fail because of range
> register (RR) decodes outside platform defined CXL ranges.
> 
>   [kernel debug message]
>   cxl_hdm_decode_init:447: cxl_pci 0000:10:00.0: DVSEC Range0 denied by
>   platform
>   cxl_pci 0000:10:00.0: Range register decodes outside platform defined CXL
>   ranges.
>   cxl_bus_probe:2073: cxl_port endpoint3: probe: -6
>   call_driver_probe:590: cxl_port endpoint3: probe with driver cxl_port
>   rejects match -6
> 
> This defect could be found with Qemu CXL branch for a long while with a
> specified probability, even with the latest branch cxl-2025-03-20.
> 
> The root cause of this defect comes from that, bit CXL_DVSEC_MEM_ENABLE of
> DVSEC control has been set but in cxl_hdm_decode_init
> CXL_HDM_DECODER_ENABLE has NOT been set and also endpoint's dvsec_range is
> not covered by root decoder's hpa_range.
>

The explanation here is a bit confusing.  Please clarify if my
understanding of the issue is incorrect.

Observed problem:
   Some firmware/BIOS sets MEM_ENABLED, does not set HDM_DECODER_ENABLED,
   and does not program the Range registers.  This is possibly the
   result of defaulting MEM_ENABLED to 1 mistakenly, rather than a
   programming error / failure.

Suggested solution:
  Linux should detect this and reset the MEM_ENABLED bit and simply
  attempt to enable the hdm decoders accordingly.


Question: Is this only observed with QEMU? If so, can we just fix QEMU?


> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 013b869b66cb..5452bb285140 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -448,6 +440,29 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
>  		allowed++;
>  	}
>  
> +	if (info->mem_enabled && !allowed) {
> +		dev_warn(dev, "RR decodes outside ranges, have a try by disabling Mem_Enable bit.\n");
> +
> +		/*
> +		 * Instead of Return error when RR decodes outside platform ranges, reenable
> +		 * Mem_Enable bit of DVSEC control for a try.
> +		 */

Your comment says to "reenable mem_enable bit", but you clear it.
I think you mean to say "reset mem_enable bit, and try to enable hdm".

> +		rc = cxl_set_mem_enable(cxlds, 0);
> +		if (rc)
> +			return rc;
> +
> +		info->mem_enabled = 0;
> +		cxlhdm->decoder_count = cxlhdm->decoder_count_cap;
> +	}
> +

> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 2a25d1957ddb..60b538f8b677 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -855,6 +855,7 @@ int cxl_mem_sanitize(struct cxl_memdev *cxlmd, u16 cmd);
>   * struct cxl_hdm - HDM Decoder registers and cached / decoded capabilities
>   * @regs: mapped registers, see devm_cxl_setup_hdm()
>   * @decoder_count: number of decoders for this port
> + * @decoder_count_cap: number of decoders from HDM Decoder Capability
>   * @target_count: for switch decoders, max downstream port targets
>   * @interleave_mask: interleave granularity capability, see check_interleave_cap()
>   * @iw_cap_mask: bitmask of supported interleave ways, see check_interleave_cap()
> @@ -863,6 +864,7 @@ int cxl_mem_sanitize(struct cxl_memdev *cxlmd, u16 cmd);
>  struct cxl_hdm {
>  	struct cxl_component_regs regs;
>  	unsigned int decoder_count;
> +	unsigned int decoder_count_cap;

Why is this needed, as opposed to simply re-reading the count?

~Gregory