Re: [PATCH 2/2] cxl/acpi: Support CXL XOR Interleave Math (CXIMS)

[Dan Williams <dan.j.williams@intel.com>]

Alison Schofield wrote:
> On Tue, Aug 09, 2022 at 04:36:14PM -0700, Dan Williams wrote:
> > alison.schofield@ wrote:
> > > From: Alison Schofield <alison.schofield@intel.com>
> > > 
> > > When the CFMWS is using XOR math, parse the corresponding
> > > CXIMS structure and store the xormaps in the root decoder.
> > > Use the xormaps in a new lookup, cxl_hb_xor(), to discover
> > > a targets entry in a host bridge interleave target list.
> > > 
> > > Defined in CXL Spec 3.0 Section: 9.17.1
> > > 
> > > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > > ---
> > >  drivers/cxl/cxl.h  |  2 +
> > >  drivers/cxl/acpi.c | 96 +++++++++++++++++++++++++++++++++++++++++++---
> > >  2 files changed, 93 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > > index f680450f0b16..0a17a7007bff 100644
> > > --- a/drivers/cxl/cxl.h
> > > +++ b/drivers/cxl/cxl.h
> > > @@ -330,12 +330,14 @@ struct cxl_switch_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
> > > + * @platform_data: platform specific configuration data
> > >   * @cxlsd: base cxl switch decoder
> > >   */
> > >  struct cxl_root_decoder {
> > >  	struct resource *res;
> > >  	atomic_t region_id;
> > >  	struct cxl_dport *(*calc_hb)(struct cxl_root_decoder *cxlrd, int pos);
> > > +	void *platform_data;
> > >  	struct cxl_switch_decoder cxlsd;
> > >  };
> > >  
> > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > > index fb649683dd3a..6ac6751c7f4e 100644
> > > --- a/drivers/cxl/acpi.c
> > > +++ b/drivers/cxl/acpi.c
> > > @@ -9,6 +9,79 @@
> > >  #include "cxlpci.h"
> > >  #include "cxl.h"
> > >  
> > > +struct cxims_data {
> > > +	int nr_maps;
> > > +	u64 xormaps[];
> > > +};
> > > +
> > > +static struct cxl_dport *cxl_hb_xor(struct cxl_root_decoder *cxlrd, int pos)
> > > +{
> > > +	struct cxl_switch_decoder *cxlsd = &cxlrd->cxlsd;
> > > +	struct cxims_data *cximsd = cxlrd->platform_data;
> > > +	struct cxl_decoder *cxld = &cxlsd->cxld;
> > > +	int ig = cxld->interleave_granularity;
> > > +	int i, n = 0;
> > > +	u64 hpa;
> > > +
> > > +	if (dev_WARN_ONCE(&cxld->dev,
> > > +			  cxld->interleave_ways != cxlsd->nr_targets,
> > > +			  "misconfigured root decoder\n"))
> > > +		return NULL;
> > > +	/*
> > > +	 * Find this targets entry (n) in the host bridge interleave
> > > +	 * list. Defined in CXL Spec 3.0 Section 9.17.1.3 Table 9-22
> > > +	 */
> > > +	hpa = cxlrd->res->start + pos * ig;
> > > +	for (i = 0; i < cximsd->nr_maps; i++)
> > > +		n |= (hweight64(hpa & cximsd->xormaps[i]) & 1) << i;
> > > +
> > > +	return cxlrd->cxlsd.target[n];
> > > +}
> > > +
> > > +struct cxl_cxims_context {
> > > +	struct device *dev;
> > > +	struct cxl_root_decoder *cxlrd;
> > > +};
> > > +
> > > +static int cxl_parse_cxims(union acpi_subtable_headers *header, void *arg,
> > > +			   const unsigned long end)
> > > +{
> > > +	struct acpi_cedt_cxims *cxims = (struct acpi_cedt_cxims *)header;
> > > +	struct cxl_cxims_context *ctx = arg;
> > > +	struct cxl_root_decoder *cxlrd = ctx->cxlrd;
> > > +	struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
> > > +	struct device *dev = ctx->dev;
> > > +	struct cxims_data *cximsd;
> > > +	unsigned int hbig;
> > > +	u8 eiw;
> > 
> > Per your comment on Dave's patch lets keep eiw as a shorthand for
> > "encoded interleave ways" i.e. the ilog2()'ish value of interleave-ways.
> > So just: s/eiw/iw/.
> 
> Then maybe it's intentionally wrong. The eiw tells the number of
> xormaps to store for use in cxl_hb_xor(). The 'iw', as stored in
> cxld->interleave_ways would be too many, so I translate back to the
> encoded version here.

Oh, what threw me off was the:

+               cximsd->nr_maps = eiw;

...because nr_maps is not @eiw. My suggestion to just change it back to
cxld->interleave_ways though is also wrong, sorry about that. 

The calculation for nr_maps is not ways_to_cxl() it is just plain
"ilog2(cxld->interleave_ways)" because the nr_maps needed for 3, 6, 12,
are 0, 1, 2 respectively, not 8, 9, 10 which is what @eiw would be.