| Message ID | 48b8ab49a54597d56b1732fc4e2955b5e726d4c9.1669153711.git.alison.schofield@intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | CXL Address Translation | expand |
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;
On Wed, Nov 30, 2022 at 04:42:35PM +0000, Jonathan Cameron wrote: > 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. Hi Jonathan, Belated thanks on this review. I've put this set aside and implemented the DPA -> HPA translation for the specific use case of adding an HPA field to the cxl_poison TRACE EVENT. I'm about to post that patch, but want to respond to your feedback, which I did take into the new patch. more... > > > > --- > > 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... > Understand about the HPA 0 now, and use ULLONG_MAX as failure on non-translateable - in future patch. > > + > > + 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? So, I've dropped this check on region create work entirely for now. > > + } > > + 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 Thanks > > > err: > > put_device(dev); > > return rc; >
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; + + 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; + } + 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); - err: put_device(dev); return rc;