Re: [PATCH v3 2/4] cxl: Restore XOR'd position bits during address translation

["Fabio M. De Francesco" <fabio.m.de.francesco@linux.intel.com>]

On Tuesday, June 25, 2024 2:55:53 AM GMT+2 alison.schofield@intel.com wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 

Hi Alison,

Below I have two questions... 

> When a device reports a DPA in events like poison, general_media,
> and dram, the driver translates that DPA back to an HPA. Presently,
> the CXL driver translation only considers the Modulo position and
> will report the wrong HPA for XOR configured root decoders.
> 
> Add a helper function that restores the XOR'd bits during DPA->HPA
> address translation. Plumb a root decoder callback to the new helper
> when XOR interleave arithmetic is in use. For Modulo arithmetic, just
> let the callback be NULL - as in no extra work required.
> 
> Upon completion of a DPA->HPA translation a couple of checks are
> performed on the result. One simply confirms that the calculated
> HPA is within the address range of the region. That test is useful
> for both Modulo and XOR interleave arithmetic decodes.
> 
> A second check confirms that the HPA is within an expected chunk
> based on the endpoints position in the region and the region
> granularity. An XOR decode disrupts the Modulo pattern making the
> chunk check useless.
> 
> To align the checks with the proper decode, pull the region range
> check inline and use the helper to do the chunk check for Modulo
> decodes only.
> 
> A cxl-test unit test of address translations is in upstream review.

Would it be helpful to provide a link to the test?

> Fixes: 28a3ae4ff66c ("cxl/trace: Add an HPA to cxl_poison trace events")
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> Tested-by: Diego Garcia Rodriguez <diego.garcia.rodriguez@intel.com>
> ---
>  drivers/cxl/acpi.c        | 48 ++++++++++++++++++++++++++++++++++++---
>  drivers/cxl/core/port.c   |  5 +++-
>  drivers/cxl/core/region.c | 22 ++++++++++--------
>  drivers/cxl/cxl.h         |  6 ++++-
>  4 files changed, 67 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 571069863c62..010741da0176 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -74,6 +74,43 @@ static struct cxl_dport *cxl_hb_xor(struct 
cxl_root_decoder *cxlrd, int pos)
>  	return cxlrd->cxlsd.target[n];
>  }
>  
> +static u64 cxl_xor_translate(struct cxl_root_decoder *cxlrd, u64 hpa)
> +{
> +	struct cxl_cxims_data *cximsd = cxlrd->platform_data;
> +	int hbiw = cxlrd->cxlsd.nr_targets;
> +	u64 val;
> +	int pos;
> +
> +	/* No xormaps for host bridge interleave ways of 1 or 3 */
> +	if (hbiw == 1 || hbiw == 3)
> +		return hpa;
> +
> +	/*
> +	 * For root decoders using xormaps (hbiw: 2,4,6,8,12,16) restore
> +	 * the position bit to its value before the xormap was applied at
> +	 * HPA->DPA translation.
> +	 *
> +	 * pos is the lowest set bit in an XORMAP
> +	 * val is the XORALLBITS(HPA & XORMAP)
> +	 *
> +	 * XORALLBITS: The CXL spec (3.1 Table 9-22) defines XORALLBITS
> +	 * as an operation that outputs a single bit by XORing all the
> +	 * bits in the input (hpa & xormap). Implement XORALLBITS using
> +	 * hweight64(). If the hamming weight is even the XOR of those
> +	 * bits results in val==0, if odd the XOR result is val==1.
> +	 */
> +
> +	for (int i = 0; i < cximsd->nr_maps; i++) {
> +		if (!cximsd->xormaps[i])
> +			continue;
> +		pos = __ffs(cximsd->xormaps[i]);
> +		val = (hweight64(hpa & cximsd->xormaps[i]) & 1);
> +		hpa = (hpa & ~(1ULL << pos)) | (val << pos);
> +	}
> +
> +	return hpa;
> +}
> +
>  struct cxl_cxims_context {
>  	struct device *dev;
>  	struct cxl_root_decoder *cxlrd;
> @@ -362,6 +399,7 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws 
*cfmws,
>  	struct cxl_cxims_context cxims_ctx;
>  	struct device *dev = ctx->dev;
>  	cxl_calc_hb_fn cxl_calc_hb;
> +	cxl_translate_fn translate;
>  	struct cxl_decoder *cxld;
>  	unsigned int ways, i, ig;
>  	int rc;
> @@ -389,13 +427,17 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws 
*cfmws,
>  	if (rc)
>  		return rc;
>  
> -	if (cfmws->interleave_arithmetic == 
ACPI_CEDT_CFMWS_ARITHMETIC_MODULO)
> +	if (cfmws->interleave_arithmetic == 
ACPI_CEDT_CFMWS_ARITHMETIC_MODULO) {
>  		cxl_calc_hb = cxl_hb_modulo;
> -	else
> +		translate = NULL;
> +
> +	} else {
>  		cxl_calc_hb = cxl_hb_xor;
> +		translate = cxl_xor_translate;
> +	}
>  
>  	struct cxl_root_decoder *cxlrd __free(put_cxlrd) =
> -		cxl_root_decoder_alloc(root_port, ways, cxl_calc_hb);
> +		cxl_root_decoder_alloc(root_port, ways, cxl_calc_hb, 
translate);
>  	if (IS_ERR(cxlrd))
>  		return PTR_ERR(cxlrd);
>  
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 887ed6e358fb..e5d5f7783857 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1808,6 +1808,7 @@ static int cxl_switch_decoder_init(struct cxl_port 
*port,
>   * @port: owning CXL root of this decoder
>   * @nr_targets: static number of downstream targets
>   * @calc_hb: which host bridge covers the n'th position by granularity
> + * @translate: decoder specific address translation function
>   *
>   * Return: A new cxl decoder to be registered by cxl_decoder_add(). A
>   * 'CXL root' decoder is one that decodes from a top-level / static 
platform
> @@ -1816,7 +1817,8 @@ static int cxl_switch_decoder_init(struct cxl_port 
*port,
>   */
>  struct cxl_root_decoder *cxl_root_decoder_alloc(struct cxl_port *port,
>  						unsigned 
int nr_targets,
> -						
cxl_calc_hb_fn calc_hb)
> +						
cxl_calc_hb_fn calc_hb,
> +						
cxl_translate_fn translate)
>  {
>  	struct cxl_root_decoder *cxlrd;
>  	struct cxl_switch_decoder *cxlsd;
> @@ -1839,6 +1841,7 @@ struct cxl_root_decoder *cxl_root_decoder_alloc(struct 
cxl_port *port,
>  	}
>  
>  	cxlrd->calc_hb = calc_hb;
> +	cxlrd->translate = translate;
>  	mutex_init(&cxlrd->range_lock);
>  
>  	cxld = &cxlsd->cxld;
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 237c28d5f2cc..bdb06dbe98a8 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2723,20 +2723,13 @@ struct cxl_region *cxl_dpa_to_region(const struct 
cxl_memdev *cxlmd, u64 dpa)
>  	return ctx.cxlr;
>  }
>  
> -static bool cxl_is_hpa_in_range(u64 hpa, struct cxl_region *cxlr, int pos)
> +static bool cxl_is_hpa_in_chunk(u64 hpa, struct cxl_region *cxlr, int pos)
>  {
>  	struct cxl_region_params *p = &cxlr->params;
>  	int gran = p->interleave_granularity;
>  	int ways = p->interleave_ways;
>  	u64 offset;
>  
> -	/* Is the hpa within this region at all */
> -	if (hpa < p->res->start || hpa > p->res->end) {
> -		dev_dbg(&cxlr->dev,
> -			"Addr trans fail: hpa 0x%llx not in 
region\n", hpa);
> -		return false;
> -	}
> -
>  	/* Is the hpa in an expected chunk for its pos(-ition) */
>  	offset = hpa - p->res->start;
>  	offset = do_div(offset, gran * ways);
> @@ -2752,6 +2745,7 @@ static bool cxl_is_hpa_in_range(u64 hpa, struct 
cxl_region *cxlr, int pos)
>  u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
>  		   u64 dpa)
>  {
> +	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr-
>dev.parent);
>  	u64 dpa_offset, hpa_offset, bits_upper, mask_upper, hpa;
>  	struct cxl_region_params *p = &cxlr->params;
>  	struct cxl_endpoint_decoder *cxled = NULL;
> @@ -2801,7 +2795,17 @@ u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const 
struct cxl_memdev *cxlmd,
>  	/* Apply the hpa_offset to the region base address */
>  	hpa = hpa_offset + p->res->start;
>  
> -	if (!cxl_is_hpa_in_range(hpa, cxlr, cxled->pos))
> +	/* Root decoder translation overrides typical modulo decode */
> +	if (cxlrd->translate)
> +		hpa = cxlrd->translate(cxlrd, hpa);
> +
> +	if (hpa < p->res->start || hpa > p->res->end) {
> +		dev_dbg(&cxlr->dev,
> +			"Addr trans fail: hpa 0x%llx not in 
region\n", hpa);
> +		return ULLONG_MAX;
> +	}
> +
> +	if (!cxlrd->translate && (!cxl_is_hpa_in_chunk(hpa, cxlr, pos)))
>  		return ULLONG_MAX;

I needed some time to understand this. It was not immediately clear why, for 
XOR translations, this chunk check is skipped.

Wouldn't it be helpful to add a comment to explain why that check is skipped?

Thanks,

Fabio

> 
>  	return hpa;
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 603c0120cff8..3678235fc9ce 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -434,12 +434,14 @@ struct cxl_switch_decoder {
>  struct cxl_root_decoder;
>  typedef struct cxl_dport *(*cxl_calc_hb_fn)(struct cxl_root_decoder *cxlrd,
>  					    int pos);
> +typedef u64 (*cxl_translate_fn)(struct cxl_root_decoder *cxlrd, u64 hpa);
>  
>  /**
>   * struct cxl_root_decoder - Static platform CXL address decoder
>   * @res: host / parent resource for region allocations
>   * @region_id: region id for next region provisioning event
>   * @calc_hb: which host bridge covers the n'th position by granularity
> + * @translate: decoder specific address translation function
>   * @platform_data: platform specific configuration data
>   * @range_lock: sync region autodiscovery by address range
>   * @qos_class: QoS performance class cookie
> @@ -449,6 +451,7 @@ struct cxl_root_decoder {
>  	struct resource *res;
>  	atomic_t region_id;
>  	cxl_calc_hb_fn calc_hb;
> +	cxl_translate_fn translate;
>  	void *platform_data;
>  	struct mutex range_lock;
>  	int qos_class;
> @@ -773,7 +776,8 @@ bool is_switch_decoder(struct device *dev);
>  bool is_endpoint_decoder(struct device *dev);
>  struct cxl_root_decoder *cxl_root_decoder_alloc(struct cxl_port *port,
>  						unsigned 
int nr_targets,
> -						
cxl_calc_hb_fn calc_hb);
> +						
cxl_calc_hb_fn calc_hb,
> +						
cxl_translate_fn translate);
>  struct cxl_dport *cxl_hb_modulo(struct cxl_root_decoder *cxlrd, int pos);
>  struct cxl_switch_decoder *cxl_switch_decoder_alloc(struct cxl_port *port,
>  						    
unsigned int nr_targets);
>