Re: [PATCH 2/4] cxl/region: Check addr trans at pmem region create (debug only)

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

On Tue, 22 Nov 2022 15:07:49 -0800
alison.schofield@intel.com wrote:

> From: Alison Schofield <alison.schofield@intel.com>
> 
> At the time of region creation, device physical addresses (DPA)
> are mapped to host physical addresses (HPA). The CXL driver
> translates DPA's to HPA's for user space consumption. In order
> to prove and exercise that translation functionality, perform
> a small sample of DPA to HPA translations whenever a pmem region
> is created in a debug kernel.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
I'd drop this patch - it isn't doing a particularly useful check
as it only checks the resulting HPA is in range, not that it's
actually correct.


> ---
>  drivers/cxl/core/region.c | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index c847517e766c..32216b5fe450 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1945,6 +1945,36 @@ u64 cxl_dpa_to_hpa(u64 dpa, struct cxl_region *cxlr,
>  	return hpa;
>  }
>  
> +static bool cxl_check_addrtrans(struct cxl_region *cxlr)
> +{
> +	struct cxl_region_params *p = &cxlr->params;
> +	struct cxl_endpoint_decoder *cxled;
> +	u64 start, end, dpa;
> +
> +	/*
> +	 * Translate a few DPAs (start,mid,end) to HPAs
> +	 * for each contributing endpoint decoder.
> +	 */
> +	for (int i = 0; i <  p->nr_targets; i++) {
> +		cxled = p->targets[i];
> +		start = cxl_dpa_resource_start(cxled);
> +		end = start + cxl_dpa_size(cxled) - 1;
> +
> +		dpa = start;
> +		if (!cxl_dpa_to_hpa(dpa, cxlr, cxled))
> +			return false;
Hmm. This verifies they are in range, but not that the
address is right.  Not sure that is particularly helpful.
Also - in theory, hpa address 0 is valid value, or does something
stop people putting a CFMWS at HPA 0 onwards?
Sure it's unlikely anyone will do so, but not impossible...

> +
> +		dpa = start + cxl_dpa_size(cxled) / 2;
> +		if (!cxl_dpa_to_hpa(dpa, cxlr, cxled))
> +			return false;
> +
> +		dpa = end;
> +		if (!cxl_dpa_to_hpa(dpa, cxlr, cxled))
> +			return false;
Maybe more useful to check 1 higher and trip the 'out of range'
return?
> +	}
> +	return true;
> +}
> +
>  /**
>   * devm_cxl_add_pmem_region() - add a cxl_region-to-nd_region bridge
>   * @cxlr: parent CXL region for this pmem region bridge device
> @@ -1973,8 +2003,10 @@ static int devm_cxl_add_pmem_region(struct cxl_region *cxlr)
>  	dev_dbg(&cxlr->dev, "%s: register %s\n", dev_name(dev->parent),
>  		dev_name(dev));
>  
> +	dev_dbg(&cxlr->dev, "Address translation check: %s\n",
> +		cxl_check_addrtrans(cxlr) ? "Pass" : "Fail");
> +
>  	return devm_add_action_or_reset(&cxlr->dev, cxlr_pmem_unregister, dev);
> -

stray change

>  err:
>  	put_device(dev);
>  	return rc;