Re: [PATCH 1/4] cxl/region: Add a DPA to HPA translation helper

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

On Wed, Nov 30, 2022 at 04:27:36PM +0000, Jonathan Cameron wrote:
> On Tue, 22 Nov 2022 15:07:48 -0800
> alison.schofield@intel.com wrote:
> 
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > CXL devices may report errors and events using their DPA (device
> > physical address). When a CXL device contributes capacity to a
> > CXL region, the device's physical addresses are mapped to HPA's.
> > (host physical addresses)
> > 
> > Provide a helper to calculate the HPA when given a DPA, a region,
> > and the devices position in the region interleave.
> 
> device's 
> 
> > 
> > Verify that the HPA is within the expected ranges that this device
> > contributes to the region interleave set.
> > 
> > The initial use case is translating the DPAs that CXL devices
> > report in media error records.
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> 
> I'm not following the maths for the 3/6/12 way interleave cases.
> 
Yeah, they were off. I reworked this to strictly follow the 
spec.

> > ---
> >  drivers/cxl/core/core.h   |  3 ++
> >  drivers/cxl/core/region.c | 80 +++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 83 insertions(+)
> > 
> > diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> > index 1d8f87be283f..72b58e53c394 100644
> > --- a/drivers/cxl/core/core.h
> > +++ b/drivers/cxl/core/core.h
> > @@ -67,6 +67,9 @@ static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port,
> >  	return xa_load(&port->endpoints, (unsigned long)&cxlmd->dev);
> >  }
> >  
> > +u64 cxl_dpa_to_hpa(u64 dpa, struct cxl_region *cxlr,
> > +		   struct cxl_endpoint_decoder *cxled);
> > +
> >  int cxl_memdev_init(void);
> >  void cxl_memdev_exit(void);
> >  void cxl_mbox_init(void);
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index f9ae5ad284ff..c847517e766c 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -1865,6 +1865,86 @@ static void cxlr_pmem_unregister(void *dev)
> >  	device_unregister(dev);
> >  }
> >  
> > +static bool cxl_is_hpa_in_range(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 stride;
> > +
> > +	/* 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 stride for its pos(-ition) */
> > +	stride = p->res->start + pos * gran;
> > +	do {
> > +		if (hpa >= stride && hpa <= stride + gran - 1)
> 	< stride + gran seems simpler.
> 

Got it.

> > +			return true;
> > +
> > +		stride = stride + ways * gran;
> > +	} while (stride < p->res->end);
> 
> That's going to take a 'while' with a large region.  Can we do something quicker
> along the lines of...
> 
> Something like (untested)
> 	u64 offset = hpa - p->res_start;
> 
> 	/* Offset within the right 'stride' */
> 	offset = offset % (gran * ways);
> 
> 	if (offset >= pos * gran && offset < (pos + 1) * gran)
> 		return true;
> 
> ..

I did adopt this calc you offered up here.

I kept this range check function in the new patch, so you'll see
it again.

> > +
> > +	dev_dbg(&cxlr->dev,
> > +		"Addr trans fail: hpa 0x%llx not in any stride\n", hpa);
> > +
> > +	return false;
> > +}
> > +
> > +u64 cxl_dpa_to_hpa(u64 dpa, struct cxl_region *cxlr,
> > +		   struct cxl_endpoint_decoder *cxled)
> > +{
> > +	struct cxl_region_params *p = &cxlr->params;
> > +	u64 dpa_offset, hpa_offset, hpa;
> > +	int rc, pos = cxled->pos;
> > +	u16 eig;
> > +	u8 eiw;
> > +
> > +	rc = ways_to_cxl(p->interleave_ways, &eiw);
> I lost track a bit, but I think we ended up with einw?

Yes. Above and below are updated w latest names:
ways_to_eiw() and granularity_to_eig()

> > +	if (rc)
> > +		return rc;
> > +	rc = granularity_to_cxl(p->interleave_granularity, &eig);
> > +	if (rc)
> > +		return rc;
> > +
> > +	/*
> > +	 * Reverse the HPA->DPA decode logic defined
> > +	 * in the CXL Spec 3.0 Section 8.2.4.19.13
> > +	 * Implementation Note: Device Decode Logic
> Short line wrap. Probably want to go nearer 80 chars
> 
> > +	 *
> > +	 * The device position in the region interleave
> > +	 * set was removed in the HPA->DPA translation.
> > +	 * Put it back to reconstruct the HPA.
> > +	 */
> > +
> > +	/* Remove the dpa base */
> > +	dpa_offset = dpa - cxl_dpa_resource_start(cxled);
> > +
> > +	/* Restore the position */
> > +	if (eiw <= 8) {
> 
> This looks to be the power of 2 version, so should that be eiw < 8?
> 

This calc section got a rewrite w 2 cases eiw < 8 and 'all others'.

> > +		hpa_offset = (dpa_offset & GENMASK_ULL(51, eig + 8)) << eiw;
> > +		hpa_offset |= GENMASK_ULL(eig + 8 + eiw, eig + 8)
> > +			      & (pos << (eig + 8));
> 
> Why is the masking needed here?  I think pos should always fit in the iw bits.
Not needed.

> 
> > +	}
> > +	if (eiw == 9)
> 
> else if would show clearly that only one of these ifs is true.
>  
> > +		hpa_offset |= BIT(eig + eiw) & (pos & 0x01);
> 
> I don't follow this logic. hpa_offset hasn't been set to anything before
> this point + the right hand side of the above will always be 0 (I think...)
> 
> 
> > +	if (eiw == 10)
> else if
> > +		hpa_offset |= GENMASK_ULL(eig + eiw, eig + 8) & (pos & 0x03);

Rework has 2 cases (eiw < 8), else all others.
I hope you can pick up the review in the new patch.

Thanks Jonathan!
Alison

> > +
> > +	/* The lower bits remain unchanged */
> > +	hpa_offset |= dpa_offset & GENMASK_ULL(eig + 7, 0);
> > +
> > +	/* Apply the hpa_offset to region base address */
> > +	hpa = hpa_offset + p->res->start;
> > +
> > +	if (!cxl_is_hpa_in_range(hpa, cxlr, cxled->pos))
> > +		return 0;
> > +
> > +	return hpa;
> > +}
> > +
> >  /**
> >   * devm_cxl_add_pmem_region() - add a cxl_region-to-nd_region bridge
> >   * @cxlr: parent CXL region for this pmem region bridge device
>