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

[Alison Schofield <alison.schofield@intel.com>]

On Mon, Jul 01, 2024 at 11:28:24AM +0200, 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... 
> 
Thanks for reviewing -

snip

> > 
> > A cxl-test unit test of address translations is in upstream review.
> 
> Would it be helpful to provide a link to the test?

Sure. Will add lore link in next rev.

snip
> 
> > +

How about this comment:

	/* Predictable chunk guarantee only applies to modulo decodes */

> > +	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

Beyond that comment, I'd like readers to use git blame and look at
at the commit log:
	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.

I'm intentionally avoiding making the CXL driver and it's documentation
the source of XOR interleave education.

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