Re: [PATCH v2] cxl/region: Translate DPA->HPA in unaligned MOD3 regions

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

On Wed, Oct 29, 2025 at 12:03:57PM +0000, Jonathan Cameron wrote:
> On Mon, 13 Oct 2025 23:28:48 -0700
> Alison Schofield <alison.schofield@intel.com> wrote:
> 
> > Suggested-by: Qing Huang <qing.huang@intel.com>
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> Hi Alison,
> 
> A few minor inline you might want to tweak a little. 
> 
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

Thanks for the review...
> 
> >  drivers/cxl/core/region.c | 147 ++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 140 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index e14c1d305b22..3dc6f0ae9f19 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -2934,13 +2934,124 @@ static bool has_spa_to_hpa(struct cxl_root_decoder *cxlrd)
> >  	return cxlrd->ops && cxlrd->ops->spa_to_hpa;
> >  }
> >  
> > +static int decode_pos(int reg_ways, int hb_ways, int pos, int *pos_port,
> > +		      int *pos_hb)
> > +{
> > +	int devices_per_hb;
> > +
> > +	/*
> > +	 * Decode for 3-6-12 way interleaves as defined in the CXL
> > +	 * Spec 3.2 9.13.1.1 Legal Interleaving Configurations.
> > +	 */
> > +	switch (hb_ways) {
> > +	case 3: /* Supports 3-way, 6-way, or 12-way regions */
> 
> Does no harm I guess, but the comment seems a little unnecessary given
> the line that immediately follows!

I've removed my excessive narration. Thanks.

> 
> > +		if (reg_ways != 3 && reg_ways != 6 && reg_ways != 12)
> > +			return -EINVAL;
> > +
> > +		devices_per_hb = reg_ways / 3;
> You could drop this out of the switch as
> 
> 	devices_per_hb = reg_ways / hb_ways;
> 
> Then the switch is simply checking for a valid config.

Nice, done!

> 
> > +		break;
> > +
> > +	case 6: /* Supports 6-way or 12-way regions */
> > +		if (reg_ways != 6 && reg_ways != 12)
> > +			return -EINVAL;
> > +
> > +		devices_per_hb = reg_ways / 6;
> > +		break;
> > +
> > +	case 12: /* Supports 12-way regions */
> > +		if (reg_ways != 12)
> > +			return -EINVAL;
> > +
> > +		devices_per_hb = 1;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +	/* Calculate port and host bridge positions */
> > +	*pos_port = pos % devices_per_hb;
> > +	*pos_hb = pos / devices_per_hb;
> > +
> > +	return 0;
> > +}
> 
> ...
> 
> > +/*
> > + * unaligned_dpa_to_hpa() translates a DPA to HPA when the region resource
> > + * start address is not aligned at Host Bridge Interleave Ways * 256MB.
> > + *
> > + * Unaligned start addresses only occur with MOD3 interleaves. All power-
> > + * of-two interleaves are guaranteed aligned.
> > + */
> > +static u64 unaligned_dpa_to_hpa(struct cxl_decoder *cxld,
> > +				struct cxl_region_params *p, int pos, u64 dpa)
> > +{
> > +	int ways_port = p->interleave_ways / cxld->interleave_ways;
> > +	int gran_port = p->interleave_granularity;
> > +	int gran_hb = cxld->interleave_granularity;
> > +	int ways_hb = cxld->interleave_ways;
> > +	int pos_port, pos_hb, gran_shift;
> > +	u64 shifted, hpa, hpa_port = 0;
> 
> Trivial but I really don't like mixing assignment and non assignments
> in these.  Also can reduce scope of hpa and shifted which is probably a good
> idea for readability.

Done.
Moved shifted and hpa to their smaller scope.
Un-mixed assigns and non-assign declarations.

> 
> > +
> > +	/* Decode an endpoint 'pos' into port and host-bridge components */
> > +	if (decode_pos(p->interleave_ways, ways_hb, pos, &pos_port, &pos_hb)) {
> > +		dev_dbg(&cxld->dev, "not supported for region ways:%d\n",
> > +			p->interleave_ways);
> > +		return ULLONG_MAX;
> > +	}
> 
> Trivial but I'd put a blank line here if you end up respinning.

Done

> 
> > +	/* Restore the port parent address if needed */
> > +	if (gran_hb != gran_port)
> > +		hpa_port = restore_parent(dpa, pos_port, gran_port, ways_port);
> > +	else
> > +		hpa_port = dpa;
> > +
> > +	/*
> > +	 * Complete the HPA reconstruction by restoring the address as if
> > +	 * each HB position is a candidate. Test against expected pos_hb
> > +	 * to confirm match.
> > +	 */
> > +	gran_shift = ilog2(gran_hb);
> > +	for (int index = 0; index < ways_hb; index++) {
> Why not just i for the index?

hmm...I was ready to just switch to i for brevity, but since you
asked 'why', I stared longer and realized it should be renamed 'pos'.
That is what it is and that matches the restore_parent() definition.

> 
> 		u64 hpa = restore_parent(hpa_port, index, gran_hb, ways_hb);
> 

Thanks for reviewing!
--Alison

>