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 Monday, July 1, 2024 11:28:24 AM GMT+2 Fabio M. De Francesco wrote:
> 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.

Sorry, I wanted to say for "no translations" (i.e., for !cxlrd->translate) but 
with copy-paste something went wrong.

Fabio

> 
> 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);
> > 
> 
>