Re: [PATCH 2/3] cxl/region: Introduce HPA to DPA address translation

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

On Mon, 23 Jun 2025 17:53:35 -0700
alison.schofield@intel.com wrote:

> From: Alison Schofield <alison.schofield@intel.com>
> 
> Add infrastructure to translate Host Physical Addresses (HPA) to Device
> Physical Addresses (DPA) within CXL regions. This capability is being
> introduced for use by follow-on patches that will add poison inject
> and clear operations at the region level.
> 
> The HPA-to-DPA translation process involves several steps:
> 1. Apply root decoder transformations (HPA to SPA) if configured
> 2. Calculate the relative offset within the region's address space
> 3. Decode the interleave position using the region's interleave ways
>    and granularity settings
> 4. Identify the target memdev based on the decoded position
> 5. Compute the final DPA by adding the decoded offset to the memdev's
>    DPA base address
> 
> 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.
> 
> While not immediately apparent in this diff, broader examination of
> the region.c code shows that this work is basically the reverse of
> previous work where a DPA is translated to an HPA. This is notable
> because it demonstrates that these calculations reuse existing logic
> rather than introducing new algorithms.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Hi Alison,

This maths still gives me a headache, but I don't follow at least some of it.
Also I'm missing the application of the XOR Map referred to above.

> ---
>  drivers/cxl/core/region.c | 85 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 85 insertions(+)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 6e5e1460068d..d2d904c4b427 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2972,6 +2972,91 @@ u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
>  	return hpa;
>  }
>  
> +struct hpa_decode_result {
> +	u64 dpa_offset;
> +	int pos;
> +};
> +
> +struct cxl_dpa_result {
> +	u64 dpa;
> +	struct cxl_memdev *cxlmd;
> +};
> +
> +static struct hpa_decode_result decode_hpa_to_dpa(u64 hpa_offset, u8 eiw,
Having a function called hpa_to_dpa that takes an hpa_offset seems inconsistent.

decode_hpa_offset_to_dpa() maybe?

> +						  u16 eig)
> +{
> +	struct hpa_decode_result result;
> +	u64 bits_upper;
> +
> +	if (eiw < 8) {
Could add a spec reference. Other than that, this block looks fine to me.
> +		result.pos = (hpa_offset >> (eig + 8)) & GENMASK(eiw - 1, 0);
> +		hpa_offset &= ~((u64)GENMASK(eiw - 1, 0) << (eig + 8));
> +		result.dpa_offset = hpa_offset >> eiw;
> +	} else {

This block isn't lining up for me with what is documented in step 2 of the
implementation note on decoding.

> +		bits_upper = hpa_offset >> (eig + 8);
ignoring pos calc as that is confusingly in a very different place
in the spec, we should have
DPA_OFFSET[51:IG + 8] = HPA_OFFSET[51:IG + IW] / 3 

So I'm missing a shift by IW somewhere.
Perhaps some more comments would help relate this to the maths.

> +		result.pos = bits_upper % 3;
> +		bits_upper /= 3;
> +		result.dpa_offset = bits_upper << (eig + 8);



> +	}
> +
> +	result.dpa_offset |= hpa_offset & GENMASK_ULL(eig + 7, 0);

I wonder if it is simpler to keep a copy of these bottom bits from
before all the manipulation above.  They make it through untouched
but we could avoid the reader having to figure that out.


> +
> +	return result;
> +}
> +
> +static struct cxl_dpa_result __maybe_unused
> +cxl_hpa_to_dpa(struct cxl_region *cxlr, u64 hpa)
> +{
> +	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
> +	struct cxl_dpa_result result = { .dpa = ULLONG_MAX, .cxlmd = NULL };
> +	struct cxl_region_params *p = &cxlr->params;
> +	struct hpa_decode_result decode;
> +	u64 hpa_offset;
> +	u16 eig = 0;
> +	u8 eiw = 0;
> +
> +	lockdep_assert_held(&cxl_region_rwsem);
> +	lockdep_assert_held(&cxl_dpa_rwsem);
> +
> +	if (hpa < p->res->start || hpa > p->res->end) {
> +		dev_err_once(&cxlr->dev,
> +			     "HPA 0x%llx not in region [0x%llx-0x%llx]\n", hpa,
> +			     p->res->start, p->res->end);
> +
> +		return result;
> +	}
> +
> +	/* Apply root decoder translation */
> +	if (cxlrd->hpa_to_spa)
> +		hpa = cxlrd->hpa_to_spa(cxlrd, hpa);

Is this in the right direction?  I was rather expecting opposite of what
is going on in cxl_dpa_to_hpa()

Perhaps separate spa and hpa in here to improve readability with
spa == hpa for the case where we don't have a transaltion.

> +
> +	ways_to_eiw(p->interleave_ways, &eiw);
> +	granularity_to_eig(p->interleave_granularity, &eig);
> +	hpa_offset = hpa - p->res->start;

Given people will line this code up with cxl_dpa_to_hpa() maybe a
comment to rule out subtracting cache_size?

> +
> +	decode = decode_hpa_to_dpa(hpa_offset, eiw, eig);
> +	if (decode.pos >= p->nr_targets) {
> +		dev_err(&cxlr->dev, "Invalid position %d for %d targets\n",
> +			decode.pos, p->nr_targets);
> +
> +		return result;
> +	}
> +
> +	for (int i = 0; i < p->nr_targets; i++) {
> +		struct cxl_endpoint_decoder *cxled = p->targets[i];
> +
> +		if (cxled->pos == decode.pos) {
> +			result.cxlmd = cxled_to_memdev(cxled);
> +			result.dpa = decode.dpa_offset + cxl_dpa_resource_start(cxled);
> +
> +			return result;
> +		}
> +	}
> +	dev_err(&cxlr->dev, "No device found for position %d\n", decode.pos);
> +
> +	return result;
> +}
> +
>  static struct lock_class_key cxl_pmem_region_key;
>  
>  static int cxl_pmem_region_alloc(struct cxl_region *cxlr)