Re: [PATCH v3 2/4] cxl/region: Introduce SPA to DPA address translation

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Wed, 16 Jul 2025 14:26:41 -0700
Alison Schofield <alison.schofield@intel.com> wrote:

> On Wed, Jul 16, 2025 at 11:52:26AM +0100, Jonathan Cameron wrote:
> > On Sat, 12 Jul 2025 19:37:55 -0700
> > alison.schofield@intel.com wrote:
> >   
> > > From: Alison Schofield <alison.schofield@intel.com>
> > > 
> > > Add infrastructure to translate System Physical Addresses (SPA) to
> > > Device Physical Addresses (DPA) within CXL regions. This capability
> > > will be used by follow-on patches that add poison inject and clear
> > > operations at the region level.
> > > 
> > > The SPA-to-DPA translation process follows these steps:
> > > 1. Apply root decoder transformations (SPA to HPA) if configured.
> > > 2. Extract the position in region interleave from the HPA offset.
> > > 3. Extract the DPA offset from the HPA offset.
> > > 4. Use position to find endpoint decoder.
> > > 5. Use endpoint decoder to find memdev and calculate DPA from offset.
> > > 6. Return the result - a memdev and a DPA.
> > > 
> > > It is Step 1 above that makes this a driver level operation and not
> > > work we can push to user space. Rather than exporting the XOR maps for
> > > root decoders configured with XOR interleave, the driver performs this
> > > complex calculation for the user.
> > > 
> > > Steps 2 and 3 follow the CXL Spec 3.2 Section 8.2.4.20.13
> > > Implementation Note: Device Decode Logic.
> > > 
> > > These calculations mirror much of the logic introduced earlier in DPA
> > > to SPA translation, see cxl_dpa_to_hpa(), where the driver needed to
> > > reverse the spec defined 'Deivce Decode Logic'.  
> > 
> > Device
> >   
> > > 
> > > Signed-off-by: Alison Schofield <alison.schofield@intel.com>  
> > Hi Alison,
> > 
> > A few minor things inline.   
> > > ---
> > > 
> > > Changes in v3:
> > > Check return of ways_to_eiw() & granularity_to_eig() (Jonathan)
> > > Collapse a header comment into pos and offset blocks (Jonathan)
> > > Calc pos for 3,6,12 using actual ways, not always 3 (Jonathan)
> > > Mask off bottom bits in < 8 offset case (Jonathan)
> > > Wrap code comments closer to column 80 (Jonathan)
> > > Use a continue to un-nest a for loop. (Jonathan)
> > > 
> > >  drivers/cxl/core/region.c | 93 +++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 93 insertions(+)
> > > 
> > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > > index a2ba19151d4f..404f69864d61 100644
> > > --- a/drivers/cxl/core/region.c
> > > +++ b/drivers/cxl/core/region.c
> > > @@ -2956,6 +2956,99 @@ u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> > >  	return hpa;
> > >  }
> > >  
> > > +struct dpa_result {
> > > +	struct cxl_memdev *cxlmd;
> > > +	u64 dpa;
> > > +};
> > > +
> > > +static int __maybe_unused region_offset_to_dpa_result(struct cxl_region *cxlr,
> > > +						      u64 offset,
> > > +						      struct dpa_result *result)
> > > +{
> > > +	struct cxl_region_params *p = &cxlr->params;
> > > +	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
> > > +	struct cxl_endpoint_decoder *cxled;
> > > +	u64 hpa, hpa_offset, dpa_offset;
> > > +	u64 bits_upper, bits_lower;
> > > +	u16 eig = 0;
> > > +	u8 eiw = 0;
> > > +	int pos;
> > > +
> > > +	lockdep_assert_held(&cxl_rwsem.region);
> > > +	lockdep_assert_held(&cxl_rwsem.dpa);
> > > +
> > > +	if (granularity_to_eig(p->interleave_granularity, &eig) ||
> > > +	    ways_to_eiw(p->interleave_ways, &eiw))
> > > +		return -ENXIO;  
> > 
> > Why eat return values to replace with ENXIO? I'd prefer
> > 
> > 	rc = granularity_to_eig(p->interleave_granularity, &eig);
> > 	if (rc)
> > 		return rc;
> > 
> > 	rc = ways_to_eiw(p->interleave_ways, &eiw);
> > 	if (rc)
> > 		return rc;  
> 
> Ah, I should never have tried to appease you with a shortcut.
> Let's revisit.
> 
> In v1,v2 it was this-
> +       ways_to_eiw(p->interleave_ways, &eiw);
> +       granularity_to_eig(p->interleave_granularity, &eig);
> 
> And you asked to check the return value. I *should* have said "No, the
> input validation ensures these are valid. That is, when storing these
> values in the region params, we do eiw_to_ways() and check that rc
> value when storing. I'll note that this is not the first instance of
> using these functions without checking return value. 
> 
> How about one of these:
> 
> Original:
> +       ways_to_eiw(p->interleave_ways, &eiw);
> +       granularity_to_eig(p->interleave_granularity, &eig);
> 
> Original w comment:
> + 	/* Input validation ensures valid ways and gran */
> +       ways_to_eiw(p->interleave_ways, &eiw);
> +       granularity_to_eig(p->interleave_granularity, &eig);

That's fine.

Personally I'd just check them separately and return the errors.
Sometimes it's less effort to add long hand error checks that
never fail (in slow paths!) than it is to explain why not ;)

Jonathan

> 
> Original w caution:
> +	if (!p->interleave_ways || !p->interleave_granularity)
> +		return -ENXIO;
> +       ways_to_eiw(p->interleave_ways, &eiw);
> +       granularity_to_eig(p->interleave_granularity, &eig);
> 
> 
> -- Alison
> 
>