* [PATCH v2] cxl: Check for invalid addresses returned from translation functions on errors
@ 2026-01-06 17:23 Robert Richter
2026-01-06 17:45 ` Dave Jiang
2026-01-06 18:42 ` Alison Schofield
0 siblings, 2 replies; 6+ messages in thread
From: Robert Richter @ 2026-01-06 17:23 UTC (permalink / raw)
To: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Alison Schofield,
Vishal Verma, Ira Weiny, Dan Williams
Cc: Robert Richter, linux-cxl, linux-kernel
Translation functions may return an invalid address in case of errors.
If the address is not checked the further use of the invalid value
will cause an address corruption.
Consistently check for a valid address returned by translation
functions. Use RESOURCE_SIZE_MAX to indicate an invalid address for
type resource_size_t. Depending on the type either RESOURCE_SIZE_MAX
or ULLONG_MAX is used to indicate an address error.
Signed-off-by: Robert Richter <rrichter@amd.com>
---
v2:
* separated from this patch series (Alison):
[PATCH v8 00/13] cxl: ACPI PRM Address Translation Support and AMD Zen5 enablement
* improved error handling logic and early return on error in
region_offset_to_dpa_result() (Dave),
* use RESOURCE_SIZE_MAX to indicate an invalid address for
resource_size_t types (Alison, kernel test robot),
* improved patch description (Alison),
* added line wrap for code >80 chars.
---
Signed-off-by: Robert Richter <rrichter@amd.com>
---
drivers/cxl/core/hdm.c | 2 +-
drivers/cxl/core/region.c | 34 ++++++++++++++++++++------
tools/testing/cxl/test/cxl_translate.c | 5 ++--
3 files changed, 30 insertions(+), 11 deletions(-)
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 1c5d2022c87a..031672e92b0b 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -530,7 +530,7 @@ resource_size_t cxl_dpa_size(struct cxl_endpoint_decoder *cxled)
resource_size_t cxl_dpa_resource_start(struct cxl_endpoint_decoder *cxled)
{
- resource_size_t base = -1;
+ resource_size_t base = RESOURCE_SIZE_MAX;
lockdep_assert_held(&cxl_rwsem.dpa);
if (cxled->dpa_res)
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index fc36a5413d3f..5bd1213737fa 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3118,7 +3118,7 @@ u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
struct cxl_region_params *p = &cxlr->params;
struct cxl_endpoint_decoder *cxled = NULL;
- u64 dpa_offset, hpa_offset, hpa;
+ u64 base, dpa_offset, hpa_offset, hpa;
u16 eig = 0;
u8 eiw = 0;
int pos;
@@ -3136,8 +3136,14 @@ u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
ways_to_eiw(p->interleave_ways, &eiw);
granularity_to_eig(p->interleave_granularity, &eig);
- dpa_offset = dpa - cxl_dpa_resource_start(cxled);
+ base = cxl_dpa_resource_start(cxled);
+ if (base == RESOURCE_SIZE_MAX)
+ return ULLONG_MAX;
+
+ dpa_offset = dpa - base;
hpa_offset = cxl_calculate_hpa_offset(dpa_offset, pos, eiw, eig);
+ if (hpa_offset == ULLONG_MAX)
+ return ULLONG_MAX;
/* Apply the hpa_offset to the region base address */
hpa = hpa_offset + p->res->start + p->cache_size;
@@ -3146,6 +3152,9 @@ u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
if (cxlrd->ops.hpa_to_spa)
hpa = cxlrd->ops.hpa_to_spa(cxlrd, hpa);
+ if (hpa == ULLONG_MAX)
+ return ULLONG_MAX;
+
if (!cxl_resource_contains_addr(p->res, hpa)) {
dev_dbg(&cxlr->dev,
"Addr trans fail: hpa 0x%llx not in region\n", hpa);
@@ -3170,7 +3179,8 @@ static int region_offset_to_dpa_result(struct cxl_region *cxlr, u64 offset,
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 hpa_offset = offset;
+ u64 dpa, dpa_offset;
u16 eig = 0;
u8 eiw = 0;
int pos;
@@ -3187,10 +3197,13 @@ static int region_offset_to_dpa_result(struct cxl_region *cxlr, u64 offset,
* CXL HPA is assumed to equal SPA.
*/
if (cxlrd->ops.spa_to_hpa) {
- hpa = cxlrd->ops.spa_to_hpa(cxlrd, p->res->start + offset);
- hpa_offset = hpa - p->res->start;
- } else {
- hpa_offset = offset;
+ hpa_offset = cxlrd->ops.spa_to_hpa(cxlrd, p->res->start + offset);
+ if (hpa_offset == ULLONG_MAX) {
+ dev_dbg(&cxlr->dev, "HPA not found for %pr offset %#llx\n",
+ p->res, offset);
+ return -ENXIO;
+ }
+ hpa_offset -= p->res->start;
}
pos = cxl_calculate_position(hpa_offset, eiw, eig);
@@ -3207,8 +3220,13 @@ static int region_offset_to_dpa_result(struct cxl_region *cxlr, u64 offset,
cxled = p->targets[i];
if (cxled->pos != pos)
continue;
+
+ dpa = cxl_dpa_resource_start(cxled);
+ if (dpa != RESOURCE_SIZE_MAX)
+ dpa += dpa_offset;
+
result->cxlmd = cxled_to_memdev(cxled);
- result->dpa = cxl_dpa_resource_start(cxled) + dpa_offset;
+ result->dpa = dpa;
return 0;
}
diff --git a/tools/testing/cxl/test/cxl_translate.c b/tools/testing/cxl/test/cxl_translate.c
index 2200ae21795c..c2af918b853e 100644
--- a/tools/testing/cxl/test/cxl_translate.c
+++ b/tools/testing/cxl/test/cxl_translate.c
@@ -69,7 +69,7 @@ static u64 to_hpa(u64 dpa_offset, int pos, u8 r_eiw, u16 r_eig, u8 hb_ways,
/* Calculate base HPA offset from DPA and position */
hpa_offset = cxl_calculate_hpa_offset(dpa_offset, pos, r_eiw, r_eig);
- if (math == XOR_MATH) {
+ if (hpa_offset != ULLONG_MAX && math == XOR_MATH) {
cximsd->nr_maps = hbiw_to_nr_maps[hb_ways];
if (cximsd->nr_maps)
return cxl_do_xormap_calc(cximsd, hpa_offset, hb_ways);
@@ -262,7 +262,8 @@ static int test_random_params(void)
reverse_dpa = cxl_calculate_dpa_offset(hpa, eiw, eig);
reverse_pos = cxl_calculate_position(hpa, eiw, eig);
- if (reverse_dpa != dpa || reverse_pos != pos) {
+ if (hpa == ULLONG_MAX || reverse_dpa != dpa ||
+ reverse_pos != pos) {
pr_err("test random iter %d FAIL hpa=%llu, dpa=%llu reverse_dpa=%llu, pos=%d reverse_pos=%d eiw=%u eig=%u\n",
i, hpa, dpa, reverse_dpa, pos, reverse_pos, eiw,
eig);
base-commit: 88c72bab77aaf389beccf762e112828253ca0564
--
2.47.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] cxl: Check for invalid addresses returned from translation functions on errors
2026-01-06 17:23 [PATCH v2] cxl: Check for invalid addresses returned from translation functions on errors Robert Richter
@ 2026-01-06 17:45 ` Dave Jiang
2026-01-06 18:42 ` Alison Schofield
1 sibling, 0 replies; 6+ messages in thread
From: Dave Jiang @ 2026-01-06 17:45 UTC (permalink / raw)
To: Robert Richter, Davidlohr Bueso, Jonathan Cameron,
Alison Schofield, Vishal Verma, Ira Weiny, Dan Williams
Cc: linux-cxl, linux-kernel
On 1/6/26 10:23 AM, Robert Richter wrote:
> Translation functions may return an invalid address in case of errors.
> If the address is not checked the further use of the invalid value
> will cause an address corruption.
>
> Consistently check for a valid address returned by translation
> functions. Use RESOURCE_SIZE_MAX to indicate an invalid address for
> type resource_size_t. Depending on the type either RESOURCE_SIZE_MAX
> or ULLONG_MAX is used to indicate an address error.
>
> Signed-off-by: Robert Richter <rrichter@amd.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v2:
> * separated from this patch series (Alison):
> [PATCH v8 00/13] cxl: ACPI PRM Address Translation Support and AMD Zen5 enablement
> * improved error handling logic and early return on error in
> region_offset_to_dpa_result() (Dave),
> * use RESOURCE_SIZE_MAX to indicate an invalid address for
> resource_size_t types (Alison, kernel test robot),
> * improved patch description (Alison),
> * added line wrap for code >80 chars.
> ---
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
> drivers/cxl/core/hdm.c | 2 +-
> drivers/cxl/core/region.c | 34 ++++++++++++++++++++------
> tools/testing/cxl/test/cxl_translate.c | 5 ++--
> 3 files changed, 30 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 1c5d2022c87a..031672e92b0b 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -530,7 +530,7 @@ resource_size_t cxl_dpa_size(struct cxl_endpoint_decoder *cxled)
>
> resource_size_t cxl_dpa_resource_start(struct cxl_endpoint_decoder *cxled)
> {
> - resource_size_t base = -1;
> + resource_size_t base = RESOURCE_SIZE_MAX;
>
> lockdep_assert_held(&cxl_rwsem.dpa);
> if (cxled->dpa_res)
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index fc36a5413d3f..5bd1213737fa 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3118,7 +3118,7 @@ u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
> struct cxl_region_params *p = &cxlr->params;
> struct cxl_endpoint_decoder *cxled = NULL;
> - u64 dpa_offset, hpa_offset, hpa;
> + u64 base, dpa_offset, hpa_offset, hpa;
> u16 eig = 0;
> u8 eiw = 0;
> int pos;
> @@ -3136,8 +3136,14 @@ u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> ways_to_eiw(p->interleave_ways, &eiw);
> granularity_to_eig(p->interleave_granularity, &eig);
>
> - dpa_offset = dpa - cxl_dpa_resource_start(cxled);
> + base = cxl_dpa_resource_start(cxled);
> + if (base == RESOURCE_SIZE_MAX)
> + return ULLONG_MAX;
> +
> + dpa_offset = dpa - base;
> hpa_offset = cxl_calculate_hpa_offset(dpa_offset, pos, eiw, eig);
> + if (hpa_offset == ULLONG_MAX)
> + return ULLONG_MAX;
>
> /* Apply the hpa_offset to the region base address */
> hpa = hpa_offset + p->res->start + p->cache_size;
> @@ -3146,6 +3152,9 @@ u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> if (cxlrd->ops.hpa_to_spa)
> hpa = cxlrd->ops.hpa_to_spa(cxlrd, hpa);
>
> + if (hpa == ULLONG_MAX)
> + return ULLONG_MAX;
> +
> if (!cxl_resource_contains_addr(p->res, hpa)) {
> dev_dbg(&cxlr->dev,
> "Addr trans fail: hpa 0x%llx not in region\n", hpa);
> @@ -3170,7 +3179,8 @@ static int region_offset_to_dpa_result(struct cxl_region *cxlr, u64 offset,
> 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 hpa_offset = offset;
> + u64 dpa, dpa_offset;
> u16 eig = 0;
> u8 eiw = 0;
> int pos;
> @@ -3187,10 +3197,13 @@ static int region_offset_to_dpa_result(struct cxl_region *cxlr, u64 offset,
> * CXL HPA is assumed to equal SPA.
> */
> if (cxlrd->ops.spa_to_hpa) {
> - hpa = cxlrd->ops.spa_to_hpa(cxlrd, p->res->start + offset);
> - hpa_offset = hpa - p->res->start;
> - } else {
> - hpa_offset = offset;
> + hpa_offset = cxlrd->ops.spa_to_hpa(cxlrd, p->res->start + offset);
> + if (hpa_offset == ULLONG_MAX) {
> + dev_dbg(&cxlr->dev, "HPA not found for %pr offset %#llx\n",
> + p->res, offset);
> + return -ENXIO;
> + }
> + hpa_offset -= p->res->start;
> }
>
> pos = cxl_calculate_position(hpa_offset, eiw, eig);
> @@ -3207,8 +3220,13 @@ static int region_offset_to_dpa_result(struct cxl_region *cxlr, u64 offset,
> cxled = p->targets[i];
> if (cxled->pos != pos)
> continue;
> +
> + dpa = cxl_dpa_resource_start(cxled);
> + if (dpa != RESOURCE_SIZE_MAX)
> + dpa += dpa_offset;
> +
> result->cxlmd = cxled_to_memdev(cxled);
> - result->dpa = cxl_dpa_resource_start(cxled) + dpa_offset;
> + result->dpa = dpa;
>
> return 0;
> }
> diff --git a/tools/testing/cxl/test/cxl_translate.c b/tools/testing/cxl/test/cxl_translate.c
> index 2200ae21795c..c2af918b853e 100644
> --- a/tools/testing/cxl/test/cxl_translate.c
> +++ b/tools/testing/cxl/test/cxl_translate.c
> @@ -69,7 +69,7 @@ static u64 to_hpa(u64 dpa_offset, int pos, u8 r_eiw, u16 r_eig, u8 hb_ways,
> /* Calculate base HPA offset from DPA and position */
> hpa_offset = cxl_calculate_hpa_offset(dpa_offset, pos, r_eiw, r_eig);
>
> - if (math == XOR_MATH) {
> + if (hpa_offset != ULLONG_MAX && math == XOR_MATH) {
> cximsd->nr_maps = hbiw_to_nr_maps[hb_ways];
> if (cximsd->nr_maps)
> return cxl_do_xormap_calc(cximsd, hpa_offset, hb_ways);
> @@ -262,7 +262,8 @@ static int test_random_params(void)
> reverse_dpa = cxl_calculate_dpa_offset(hpa, eiw, eig);
> reverse_pos = cxl_calculate_position(hpa, eiw, eig);
>
> - if (reverse_dpa != dpa || reverse_pos != pos) {
> + if (hpa == ULLONG_MAX || reverse_dpa != dpa ||
> + reverse_pos != pos) {
> pr_err("test random iter %d FAIL hpa=%llu, dpa=%llu reverse_dpa=%llu, pos=%d reverse_pos=%d eiw=%u eig=%u\n",
> i, hpa, dpa, reverse_dpa, pos, reverse_pos, eiw,
> eig);
>
> base-commit: 88c72bab77aaf389beccf762e112828253ca0564
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] cxl: Check for invalid addresses returned from translation functions on errors
2026-01-06 17:23 [PATCH v2] cxl: Check for invalid addresses returned from translation functions on errors Robert Richter
2026-01-06 17:45 ` Dave Jiang
@ 2026-01-06 18:42 ` Alison Schofield
2026-01-07 12:03 ` Robert Richter
1 sibling, 1 reply; 6+ messages in thread
From: Alison Schofield @ 2026-01-06 18:42 UTC (permalink / raw)
To: Robert Richter
Cc: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Vishal Verma,
Ira Weiny, Dan Williams, linux-cxl, linux-kernel
On Tue, Jan 06, 2026 at 06:23:58PM +0100, Robert Richter wrote:
> Translation functions may return an invalid address in case of errors.
> If the address is not checked the further use of the invalid value
> will cause an address corruption.
>
> Consistently check for a valid address returned by translation
> functions. Use RESOURCE_SIZE_MAX to indicate an invalid address for
> type resource_size_t. Depending on the type either RESOURCE_SIZE_MAX
> or ULLONG_MAX is used to indicate an address error.
Thanks for separating this out. A couple of comments below:
>
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
> v2:
> * separated from this patch series (Alison):
> [PATCH v8 00/13] cxl: ACPI PRM Address Translation Support and AMD Zen5 enablement
> * improved error handling logic and early return on error in
> region_offset_to_dpa_result() (Dave),
> * use RESOURCE_SIZE_MAX to indicate an invalid address for
> resource_size_t types (Alison, kernel test robot),
> * improved patch description (Alison),
> * added line wrap for code >80 chars.
> ---
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
> drivers/cxl/core/hdm.c | 2 +-
> drivers/cxl/core/region.c | 34 ++++++++++++++++++++------
> tools/testing/cxl/test/cxl_translate.c | 5 ++--
> 3 files changed, 30 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 1c5d2022c87a..031672e92b0b 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -530,7 +530,7 @@ resource_size_t cxl_dpa_size(struct cxl_endpoint_decoder *cxled)
>
> resource_size_t cxl_dpa_resource_start(struct cxl_endpoint_decoder *cxled)
> {
> - resource_size_t base = -1;
> + resource_size_t base = RESOURCE_SIZE_MAX;
>
> lockdep_assert_held(&cxl_rwsem.dpa);
> if (cxled->dpa_res)
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index fc36a5413d3f..5bd1213737fa 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3118,7 +3118,7 @@ u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
> struct cxl_region_params *p = &cxlr->params;
> struct cxl_endpoint_decoder *cxled = NULL;
> - u64 dpa_offset, hpa_offset, hpa;
> + u64 base, dpa_offset, hpa_offset, hpa;
> u16 eig = 0;
> u8 eiw = 0;
> int pos;
> @@ -3136,8 +3136,14 @@ u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> ways_to_eiw(p->interleave_ways, &eiw);
> granularity_to_eig(p->interleave_granularity, &eig);
>
> - dpa_offset = dpa - cxl_dpa_resource_start(cxled);
> + base = cxl_dpa_resource_start(cxled);
> + if (base == RESOURCE_SIZE_MAX)
> + return ULLONG_MAX;
> +
> + dpa_offset = dpa - base;
> hpa_offset = cxl_calculate_hpa_offset(dpa_offset, pos, eiw, eig);
> + if (hpa_offset == ULLONG_MAX)
> + return ULLONG_MAX;
>
> /* Apply the hpa_offset to the region base address */
> hpa = hpa_offset + p->res->start + p->cache_size;
> @@ -3146,6 +3152,9 @@ u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> if (cxlrd->ops.hpa_to_spa)
> hpa = cxlrd->ops.hpa_to_spa(cxlrd, hpa);
>
> + if (hpa == ULLONG_MAX)
> + return ULLONG_MAX;
> +
> if (!cxl_resource_contains_addr(p->res, hpa)) {
> dev_dbg(&cxlr->dev,
> "Addr trans fail: hpa 0x%llx not in region\n", hpa);
> @@ -3170,7 +3179,8 @@ static int region_offset_to_dpa_result(struct cxl_region *cxlr, u64 offset,
> 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 hpa_offset = offset;
> + u64 dpa, dpa_offset;
> u16 eig = 0;
> u8 eiw = 0;
> int pos;
> @@ -3187,10 +3197,13 @@ static int region_offset_to_dpa_result(struct cxl_region *cxlr, u64 offset,
> * CXL HPA is assumed to equal SPA.
> */
> if (cxlrd->ops.spa_to_hpa) {
> - hpa = cxlrd->ops.spa_to_hpa(cxlrd, p->res->start + offset);
> - hpa_offset = hpa - p->res->start;
> - } else {
> - hpa_offset = offset;
> + hpa_offset = cxlrd->ops.spa_to_hpa(cxlrd, p->res->start + offset);
> + if (hpa_offset == ULLONG_MAX) {
> + dev_dbg(&cxlr->dev, "HPA not found for %pr offset %#llx\n",
> + p->res, offset);
> + return -ENXIO;
> + }
> + hpa_offset -= p->res->start;
> }
>
> pos = cxl_calculate_position(hpa_offset, eiw, eig);
> @@ -3207,8 +3220,13 @@ static int region_offset_to_dpa_result(struct cxl_region *cxlr, u64 offset,
> cxled = p->targets[i];
> if (cxled->pos != pos)
> continue;
> +
> + dpa = cxl_dpa_resource_start(cxled);
We want to return -ENXIO, not 0 in this case.
So jump out here immediately - right?
if (dpa == REsOURCE_SIZE_MAX)
return -ENXIO;
> + if (dpa != RESOURCE_SIZE_MAX)
> + dpa += dpa_offset;
> +
> result->cxlmd = cxled_to_memdev(cxled);
> - result->dpa = cxl_dpa_resource_start(cxled) + dpa_offset;
> + result->dpa = dpa;
>
> return 0;
> }
> diff --git a/tools/testing/cxl/test/cxl_translate.c b/tools/testing/cxl/test/cxl_translate.c
> index 2200ae21795c..c2af918b853e 100644
> --- a/tools/testing/cxl/test/cxl_translate.c
> +++ b/tools/testing/cxl/test/cxl_translate.c
> @@ -69,7 +69,7 @@ static u64 to_hpa(u64 dpa_offset, int pos, u8 r_eiw, u16 r_eig, u8 hb_ways,
> /* Calculate base HPA offset from DPA and position */
> hpa_offset = cxl_calculate_hpa_offset(dpa_offset, pos, r_eiw, r_eig);
>
> - if (math == XOR_MATH) {
> + if (hpa_offset != ULLONG_MAX && math == XOR_MATH) {
Prefer no compound statement. ie.
if (hpa_offset == ULLONG_MAX)
return ULLONG_MAX;
then continue as is.
> cximsd->nr_maps = hbiw_to_nr_maps[hb_ways];
> if (cximsd->nr_maps)
> return cxl_do_xormap_calc(cximsd, hpa_offset, hb_ways);
> @@ -262,7 +262,8 @@ static int test_random_params(void)
Here we continue to do calcs after we may already know that hpa is
ULLONG_MAX. Need to skip calculating the reverse_dpa,pos and go right
to adding this as a failure.
> reverse_dpa = cxl_calculate_dpa_offset(hpa, eiw, eig);
> reverse_pos = cxl_calculate_position(hpa, eiw, eig);
>
> - if (reverse_dpa != dpa || reverse_pos != pos) {
> + if (hpa == ULLONG_MAX || reverse_dpa != dpa ||
> + reverse_pos != pos) {
> pr_err("test random iter %d FAIL hpa=%llu, dpa=%llu reverse_dpa=%llu, pos=%d reverse_pos=%d eiw=%u eig=%u\n",
> i, hpa, dpa, reverse_dpa, pos, reverse_pos, eiw,
> eig);
>
> base-commit: 88c72bab77aaf389beccf762e112828253ca0564
> --
> 2.47.3
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] cxl: Check for invalid addresses returned from translation functions on errors
2026-01-06 18:42 ` Alison Schofield
@ 2026-01-07 12:03 ` Robert Richter
2026-01-08 18:07 ` Alison Schofield
0 siblings, 1 reply; 6+ messages in thread
From: Robert Richter @ 2026-01-07 12:03 UTC (permalink / raw)
To: Alison Schofield
Cc: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Vishal Verma,
Ira Weiny, Dan Williams, linux-cxl, linux-kernel
On 06.01.26 10:42:36, Alison Schofield wrote:
> On Tue, Jan 06, 2026 at 06:23:58PM +0100, Robert Richter wrote:
> > @@ -3207,8 +3220,13 @@ static int region_offset_to_dpa_result(struct cxl_region *cxlr, u64 offset,
> > cxled = p->targets[i];
> > if (cxled->pos != pos)
> > continue;
> > +
> > + dpa = cxl_dpa_resource_start(cxled);
>
> We want to return -ENXIO, not 0 in this case.
> So jump out here immediately - right?
>
> if (dpa == REsOURCE_SIZE_MAX)
> return -ENXIO;
The users of region_offset_to_dpa_result() handle the
RESOURCE_SIZE_MAX case. For that reason the dpa is not checked here.
I did not want to change the function interface with that fix.
Thanks for review.
-Robert
>
>
> > + if (dpa != RESOURCE_SIZE_MAX)
> > + dpa += dpa_offset;
> > +
> > result->cxlmd = cxled_to_memdev(cxled);
> > - result->dpa = cxl_dpa_resource_start(cxled) + dpa_offset;
> > + result->dpa = dpa;
> >
> > return 0;
> > }
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] cxl: Check for invalid addresses returned from translation functions on errors
2026-01-07 12:03 ` Robert Richter
@ 2026-01-08 18:07 ` Alison Schofield
2026-01-12 12:07 ` Robert Richter
0 siblings, 1 reply; 6+ messages in thread
From: Alison Schofield @ 2026-01-08 18:07 UTC (permalink / raw)
To: Robert Richter
Cc: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Vishal Verma,
Ira Weiny, Dan Williams, linux-cxl, linux-kernel
On Wed, Jan 07, 2026 at 01:03:00PM +0100, Robert Richter wrote:
> On 06.01.26 10:42:36, Alison Schofield wrote:
> > On Tue, Jan 06, 2026 at 06:23:58PM +0100, Robert Richter wrote:
>
> > > @@ -3207,8 +3220,13 @@ static int region_offset_to_dpa_result(struct cxl_region *cxlr, u64 offset,
> > > cxled = p->targets[i];
> > > if (cxled->pos != pos)
> > > continue;
> > > +
> > > + dpa = cxl_dpa_resource_start(cxled);
> >
> > We want to return -ENXIO, not 0 in this case.
> > So jump out here immediately - right?
> >
> > if (dpa == REsOURCE_SIZE_MAX)
> > return -ENXIO;
>
> The users of region_offset_to_dpa_result() handle the
> RESOURCE_SIZE_MAX case. For that reason the dpa is not checked here.
> I did not want to change the function interface with that fix.
region_offset_to_dpa_result() intends to return an rc when it knows
it would return a bad result. This is that case. With this proposed
change, we avoid adding to a RESOURCE_SIZE_MAX, which is defensive,
but isn't quitting immediately correct? These fixups are all about
stopping when any ingredient to the calc looks bad. It looks bad,
let's stop right away.
Seeing that the callsites do this:
rc = region_offset_to_dpa_result(cxlr, offset, &result);
if (rc || !result.cxlmd || result.dpa == ULLONG_MAX) {
...this failed
shows that they do some defensive programming and don't rely only
on the rc. But the inverse does not seem right - relying on that.
Staring at it more, I guess you could refactor the function to be
a void and get rid of the rc entirely.
Is that the real simplification needed here?
-- Alison
>
> Thanks for review.
>
> -Robert
>
> >
> >
> > > + if (dpa != RESOURCE_SIZE_MAX)
> > > + dpa += dpa_offset;
> > > +
> > > result->cxlmd = cxled_to_memdev(cxled);
> > > - result->dpa = cxl_dpa_resource_start(cxled) + dpa_offset;
> > > + result->dpa = dpa;
> > >
> > > return 0;
> > > }
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] cxl: Check for invalid addresses returned from translation functions on errors
2026-01-08 18:07 ` Alison Schofield
@ 2026-01-12 12:07 ` Robert Richter
0 siblings, 0 replies; 6+ messages in thread
From: Robert Richter @ 2026-01-12 12:07 UTC (permalink / raw)
To: Alison Schofield
Cc: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Vishal Verma,
Ira Weiny, Dan Williams, linux-cxl, linux-kernel
On Thu, Jan 08, 2026 at 10:07:06AM -0800, Alison Schofield wrote:
> On Wed, Jan 07, 2026 at 01:03:00PM +0100, Robert Richter wrote:
> > On 06.01.26 10:42:36, Alison Schofield wrote:
> > > On Tue, Jan 06, 2026 at 06:23:58PM +0100, Robert Richter wrote:
> >
> > > > @@ -3207,8 +3220,13 @@ static int region_offset_to_dpa_result(struct cxl_region *cxlr, u64 offset,
> > > > cxled = p->targets[i];
> > > > if (cxled->pos != pos)
> > > > continue;
> > > > +
> > > > + dpa = cxl_dpa_resource_start(cxled);
> > >
> > > We want to return -ENXIO, not 0 in this case.
> > > So jump out here immediately - right?
> > >
> > > if (dpa == REsOURCE_SIZE_MAX)
> > > return -ENXIO;
> >
> > The users of region_offset_to_dpa_result() handle the
> > RESOURCE_SIZE_MAX case. For that reason the dpa is not checked here.
> > I did not want to change the function interface with that fix.
>
> region_offset_to_dpa_result() intends to return an rc when it knows
> it would return a bad result. This is that case. With this proposed
> change, we avoid adding to a RESOURCE_SIZE_MAX, which is defensive,
> but isn't quitting immediately correct? These fixups are all about
> stopping when any ingredient to the calc looks bad. It looks bad,
> let's stop right away.
>
> Seeing that the callsites do this:
>
> rc = region_offset_to_dpa_result(cxlr, offset, &result);
> if (rc || !result.cxlmd || result.dpa == ULLONG_MAX) {
> ...this failed
>
> shows that they do some defensive programming and don't rely only
> on the rc. But the inverse does not seem right - relying on that.
The logic here says that it is not an error to have result.dpa ==
ULLONG_MAX, e.g. you still could take result.cxlmd.
>
> Staring at it more, I guess you could refactor the function to be
> a void and get rid of the rc entirely.
>
> Is that the real simplification needed here?
This patch aims to only fix the use of bad address values. No rework
at all.
-Robert
>
> -- Alison
>
> >
> > Thanks for review.
> >
> > -Robert
> >
> > >
> > >
> > > > + if (dpa != RESOURCE_SIZE_MAX)
> > > > + dpa += dpa_offset;
> > > > +
> > > > result->cxlmd = cxled_to_memdev(cxled);
> > > > - result->dpa = cxl_dpa_resource_start(cxled) + dpa_offset;
> > > > + result->dpa = dpa;
> > > >
> > > > return 0;
> > > > }
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-01-12 12:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-01-06 17:23 [PATCH v2] cxl: Check for invalid addresses returned from translation functions on errors Robert Richter
2026-01-06 17:45 ` Dave Jiang
2026-01-06 18:42 ` Alison Schofield
2026-01-07 12:03 ` Robert Richter
2026-01-08 18:07 ` Alison Schofield
2026-01-12 12:07 ` Robert Richter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox