Re: [PATCH 09/14] cxl/mem: Fix CXL DVSEC Range Sizing

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Thu, 12 May 2022 11:15:05 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Per CXL 2.0 Section 8.1.3.8.4 "DVSEC CXL Range 1 Base Low" there is no
> way to specify decode sizes smaller than 256M. Fix cxl_dvsec_ranges()
> and cxl_hdm_decode_init() to account for that default decode range.

This is effectively the same as the discussion on patch 14.

My reading of the spec suggests that size can be 0 and that would mean
no access is passed on to the hardware.  It's a rather odd corner case
and would mean the device would only work with HDM decoders and might
well fail some compliance tests (I haven't checked)

It's not a corner case I care about... 

> Note, that this means that any BIOS implementation that sets mem_enable,
> but not HDM Decoder Capability enable will cause the driver to fail to
> attach. A later change validates the DVSEC ranges against platform CXL
> decode (CXL CFMWS) to make a decision about overriding the default DVSEC
> Range configuration.

> 
> Fixes: 560f78559006 ("cxl/pci: Retrieve CXL DVSEC memory info")
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/core/pci.c |   10 +++++++---
>  drivers/cxl/mem.c      |   10 +---------
>  2 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index f3e59f8b6621..f1c0677a4f52 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -236,7 +236,12 @@ int cxl_dvsec_ranges(struct cxl_dev_state *cxlds,
>  		if (rc)
>  			return rc;
>  
> -		size = (u64)temp << 32;
> +		/*
> +		 * Per CXL 2.0 Section 8.1.3.8.4 "DVSEC CXL Range 1 Base
> +		 * Low", the minimum decode size is 256MB
> +		 */
> +		size = SZ_256M;

This is not how I read the spec.  The match is base <= Addr < base + size.
If size == 0 then there are no matches as base = base + size and so the
right condition isn't met.  Maybe I'm missing something though...


> +		size |= (u64)temp << 32;
>  
>  		rc = pci_read_config_dword(
>  			pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(i), &temp);
> @@ -264,8 +269,7 @@ int cxl_dvsec_ranges(struct cxl_dev_state *cxlds,
>  			.end = base + size - 1
>  		};
>  
> -		if (size)
> -			ranges++;
> +		ranges++;
>  	}
>  
>  	info->ranges = ranges;
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 902d1f6e189e..af4a88d3c5fa 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -84,15 +84,7 @@ __mock bool cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
>  			    CXL_HDM_DECODER_CTRL_OFFSET);
>  	global_enable = global_ctrl & CXL_HDM_DECODER_ENABLE;
>  
> -	/*
> -	 * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 Base
> -	 * [High,Low] when HDM operation is enabled the range register values
> -	 * are ignored by the device, but the spec also recommends matching the
> -	 * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero info->ranges
> -	 * are expected even though Linux does not require or maintain that
> -	 * match.
> -	 */
> -	if (!global_enable && info->mem_enabled && info->ranges)
> +	if (!global_enable && info->mem_enabled)
>  		goto out;
>  
>  	retval = true;
>