The calculation of an endpoint's position in a region traverses all
ports up to the root port and determines the corresponding decoders
for that particular address range. For address translation the HPA
range must be recalculated between ports. In order to prepare the
implementation of address translation, move code to
cxl_endpoint_decoder_initialize() and reuse the existing iterator
there.
Signed-off-by: Robert Richter <rrichter@amd.com>
---
drivers/cxl/core/region.c | 25 +++++++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index ad4a6ce37216..6f106bfa115f 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1903,7 +1903,6 @@ static int cxl_region_sort_targets(struct cxl_region *cxlr)
for (i = 0; i < p->nr_targets; i++) {
struct cxl_endpoint_decoder *cxled = p->targets[i];
- cxled->pos = cxl_calc_interleave_pos(cxled);
/*
* Record that sorting failed, but still continue to calc
* cxled->pos so that follow-on code paths can reliably
@@ -3264,10 +3263,22 @@ static int cxl_endpoint_decoder_initialize(struct cxl_endpoint_decoder *cxled)
struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
struct cxl_port *iter = cxled_to_port(cxled);
struct cxl_decoder *root, *cxld = &cxled->cxld;
- struct range *hpa = &cxld->hpa_range;
+ struct range hpa = cxld->hpa_range;
+ struct cxl_interleave_context ctx;
+ int rc;
- while (iter && !is_cxl_root(iter))
- iter = to_cxl_port(iter->dev.parent);
+ ctx = (struct cxl_interleave_context) {
+ .hpa_range = &hpa,
+ };
+
+ while (iter && !is_cxl_root(iter)) {
+ /* Convert interleave settings to next port upstream. */
+ rc = cxl_port_calc_interleave(iter, &ctx);
+ if (rc < 0)
+ return rc;
+
+ iter = parent_port_of(iter);
+ }
if (!iter)
return -ENXIO;
@@ -3281,7 +3292,13 @@ static int cxl_endpoint_decoder_initialize(struct cxl_endpoint_decoder *cxled)
return -ENXIO;
}
+ dev_dbg(cxld->dev.parent,
+ "%s:%s: range:%#llx-%#llx pos:%d\n",
+ dev_name(&cxled->cxld.dev), dev_name(&cxld->dev),
+ hpa.start, hpa.end, ctx.pos);
+
cxled->cxlrd = to_cxl_root_decoder(&root->dev);
+ cxled->pos = ctx.pos;
return 0;
}
--
2.39.5On Tue, Feb 18, 2025 at 02:23:45PM +0100, Robert Richter wrote:
> The calculation of an endpoint's position in a region traverses all
> ports up to the root port and determines the corresponding decoders
> for that particular address range. For address translation the HPA
> range must be recalculated between ports. In order to prepare the
> implementation of address translation, move code to
> cxl_endpoint_decoder_initialize() and reuse the existing iterator
> there.
>
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
> drivers/cxl/core/region.c | 25 +++++++++++++++++++++----
> 1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index ad4a6ce37216..6f106bfa115f 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1903,7 +1903,6 @@ static int cxl_region_sort_targets(struct cxl_region *cxlr)
> for (i = 0; i < p->nr_targets; i++) {
> struct cxl_endpoint_decoder *cxled = p->targets[i];
>
> - cxled->pos = cxl_calc_interleave_pos(cxled);
> /*
This change breaks the entire sort process, because in
cxl_region_attach_auto the positions are temporarily overwritten
To make this work, I had to drop over-write of the position when doing
the attach process - but this is just incorrect as we now have a
cxled->pos that indexes incorrectly into p->targets[N]
The result of the above change (without the below hack) is that decoder
probe order causes a race condition - whoever shows up first gets
position 0, and the sort does nothing (because the above line is
dropped).
TL;DR: This line should just be left as-is. It's perfectly fine to
re-calculate the position.
~Gregory
--- DO NOT USE - added for context ---
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 902b04b875b3..e75eb1c815f1 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1855,7 +1855,6 @@ static int cxl_region_attach_auto(struct cxl_region *cxlr,
*/
pos = p->nr_targets;
p->targets[pos] = cxled;
- cxled->pos = pos;
p->nr_targets++;
return 0;
On 2/18/25 6:23 AM, Robert Richter wrote:
> The calculation of an endpoint's position in a region traverses all
> ports up to the root port and determines the corresponding decoders
> for that particular address range. For address translation the HPA
> range must be recalculated between ports. In order to prepare the
> implementation of address translation, move code to
> cxl_endpoint_decoder_initialize() and reuse the existing iterator
> there.
>
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
> drivers/cxl/core/region.c | 25 +++++++++++++++++++++----
> 1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index ad4a6ce37216..6f106bfa115f 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1903,7 +1903,6 @@ static int cxl_region_sort_targets(struct cxl_region *cxlr)
> for (i = 0; i < p->nr_targets; i++) {
> struct cxl_endpoint_decoder *cxled = p->targets[i];
>
> - cxled->pos = cxl_calc_interleave_pos(cxled);
> /*
> * Record that sorting failed, but still continue to calc
> * cxled->pos so that follow-on code paths can reliably
Do the comments need to be updated here with the deletion of that line above?
DJ
> @@ -3264,10 +3263,22 @@ static int cxl_endpoint_decoder_initialize(struct cxl_endpoint_decoder *cxled)
> struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> struct cxl_port *iter = cxled_to_port(cxled);
> struct cxl_decoder *root, *cxld = &cxled->cxld;
> - struct range *hpa = &cxld->hpa_range;
> + struct range hpa = cxld->hpa_range;
> + struct cxl_interleave_context ctx;
> + int rc;
>
> - while (iter && !is_cxl_root(iter))
> - iter = to_cxl_port(iter->dev.parent);
> + ctx = (struct cxl_interleave_context) {
> + .hpa_range = &hpa,
> + };
> +
> + while (iter && !is_cxl_root(iter)) {
> + /* Convert interleave settings to next port upstream. */
> + rc = cxl_port_calc_interleave(iter, &ctx);
> + if (rc < 0)
> + return rc;
> +
> + iter = parent_port_of(iter);
> + }
>
> if (!iter)
> return -ENXIO;
> @@ -3281,7 +3292,13 @@ static int cxl_endpoint_decoder_initialize(struct cxl_endpoint_decoder *cxled)
> return -ENXIO;
> }
>
> + dev_dbg(cxld->dev.parent,
> + "%s:%s: range:%#llx-%#llx pos:%d\n",
> + dev_name(&cxled->cxld.dev), dev_name(&cxld->dev),
> + hpa.start, hpa.end, ctx.pos);
> +
> cxled->cxlrd = to_cxl_root_decoder(&root->dev);
> + cxled->pos = ctx.pos;
>
> return 0;
> }
On Tue, Feb 18, 2025 at 02:23:45PM +0100, Robert Richter wrote:
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index ad4a6ce37216..6f106bfa115f 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1903,7 +1903,6 @@ static int cxl_region_sort_targets(struct cxl_region *cxlr)
> for (i = 0; i < p->nr_targets; i++) {
> struct cxl_endpoint_decoder *cxled = p->targets[i];
>
> - cxled->pos = cxl_calc_interleave_pos(cxled);
> /*
> * Record that sorting failed, but still continue to calc
> * cxled->pos so that follow-on code paths can reliably
> @@ -3264,10 +3263,22 @@ static int cxl_endpoint_decoder_initialize(struct cxl_endpoint_decoder *cxled)
> struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> struct cxl_port *iter = cxled_to_port(cxled);
> struct cxl_decoder *root, *cxld = &cxled->cxld;
> - struct range *hpa = &cxld->hpa_range;
> + struct range hpa = cxld->hpa_range;
> + struct cxl_interleave_context ctx;
> + int rc;
>
> - while (iter && !is_cxl_root(iter))
> - iter = to_cxl_port(iter->dev.parent);
> + ctx = (struct cxl_interleave_context) {
> + .hpa_range = &hpa,
> + };
> +
> + while (iter && !is_cxl_root(iter)) {
> + /* Convert interleave settings to next port upstream. */
> + rc = cxl_port_calc_interleave(iter, &ctx);
Thinking about it a bit more, you still have cxl_port_calc_interleave
returning the position, but you have the position captured in ctx.
I think you just want to return 0/ERR now since ctx will have the pos.
This makes me think patch 3 and 4 should just be 1 patch.
> + if (rc < 0)
> + return rc;
> +
> + iter = parent_port_of(iter);
> + }
>
> if (!iter)
> return -ENXIO;
> @@ -3281,7 +3292,13 @@ static int cxl_endpoint_decoder_initialize(struct cxl_endpoint_decoder *cxled)
> return -ENXIO;
> }
>
> + dev_dbg(cxld->dev.parent,
> + "%s:%s: range:%#llx-%#llx pos:%d\n",
> + dev_name(&cxled->cxld.dev), dev_name(&cxld->dev),
> + hpa.start, hpa.end, ctx.pos);
> +
> cxled->cxlrd = to_cxl_root_decoder(&root->dev);
> + cxled->pos = ctx.pos;
>
> return 0;
> }
> --
> 2.39.5
>
On Tue, Feb 18, 2025 at 02:23:45PM +0100, Robert Richter wrote:
> The calculation of an endpoint's position in a region traverses all
> ports up to the root port and determines the corresponding decoders
> for that particular address range. For address translation the HPA
> range must be recalculated between ports. In order to prepare the
> implementation of address translation, move code to
> cxl_endpoint_decoder_initialize() and reuse the existing iterator
> there.
>
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
> drivers/cxl/core/region.c | 25 +++++++++++++++++++++----
> 1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index ad4a6ce37216..6f106bfa115f 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1903,7 +1903,6 @@ static int cxl_region_sort_targets(struct cxl_region *cxlr)
> for (i = 0; i < p->nr_targets; i++) {
> struct cxl_endpoint_decoder *cxled = p->targets[i];
>
> - cxled->pos = cxl_calc_interleave_pos(cxled);
> /*
> * Record that sorting failed, but still continue to calc
> * cxled->pos so that follow-on code paths can reliably
> @@ -3264,10 +3263,22 @@ static int cxl_endpoint_decoder_initialize(struct cxl_endpoint_decoder *cxled)
> struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> struct cxl_port *iter = cxled_to_port(cxled);
> struct cxl_decoder *root, *cxld = &cxled->cxld;
> - struct range *hpa = &cxld->hpa_range;
> + struct range hpa = cxld->hpa_range;
I believe you have a build error here:
drivers/cxl/core/region.c: In function ‘cxl_endpoint_decoder_initialize’:
drivers/cxl/core/region.c:3286:51: error: incompatible type for argument 2 of ‘cxl_port_find_switch_decoder’
3286 | root = cxl_port_find_switch_decoder(iter, hpa);
| ^~~
| |
| struct range
drivers/cxl/core/region.c:3244:67: note: expected ‘struct range *’ but argument is of type ‘struct range’
3244 | cxl_port_find_switch_decoder(struct cxl_port *port, struct range *hpa)
| ~~~~~~~~~~~~~~^~~
Just a missed & later in the function
~Gregory
There are at least 3 bugs at this point in the patch series.
1. cxl_calc_interleave_pos should be using cxled->spa_range
2. cxl_calc_interleave_pos should be returning ctx->pos
3. cxl_region_sort_targets still needs to call cxl_calc_interleave_pos
The auto decoder probe proess overwrites the endpoint position
temporarily to record its temporary location in the region target list.
This patch restores the pos recalculation during the sort target process
so that decoder probe order doesn't affect region probe.
Signed-off-by: Gregory Price <gourry@gourry.net>
---
drivers/cxl/core/region.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 934dcb2daa15..5c9e2b747731 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1879,7 +1879,7 @@ static int cxl_calc_interleave_pos(struct cxl_endpoint_decoder *cxled)
int pos = 0;
ctx = (struct cxl_interleave_context) {
- .hpa_range = &cxled->cxld.hpa_range,
+ .hpa_range = &cxled->spa_range,
};
for (iter = cxled_to_port(cxled); pos >= 0 && iter;
@@ -1892,7 +1892,7 @@ static int cxl_calc_interleave_pos(struct cxl_endpoint_decoder *cxled)
dev_name(&port->dev), ctx.hpa_range->start, ctx.hpa_range->end,
ctx.pos);
- return pos;
+ return ctx.pos;
}
static int cxl_region_sort_targets(struct cxl_region *cxlr)
@@ -1903,6 +1903,7 @@ static int cxl_region_sort_targets(struct cxl_region *cxlr)
for (i = 0; i < p->nr_targets; i++) {
struct cxl_endpoint_decoder *cxled = p->targets[i];
+ cxled->pos = cxl_calc_interleave_pos(cxled);
/*
* Record that sorting failed, but still continue to calc
* cxled->pos so that follow-on code paths can reliably
--
2.47.1On Fri, Apr 04, 2025 at 10:35:20PM -0400, Gregory Price wrote:
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 934dcb2daa15..5c9e2b747731 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1879,7 +1879,7 @@ static int cxl_calc_interleave_pos(struct cxl_endpoint_decoder *cxled)
> int pos = 0;
>
> ctx = (struct cxl_interleave_context) {
> - .hpa_range = &cxled->cxld.hpa_range,
> + .hpa_range = &cxled->spa_range,
> };
>
Realizing just now that this line belongs to the next patch.
~Gregory
The auto decoder probe proess overwrites the endpoint position
temporarily to record its temporary location in the region target list.
This patch restores the pos recalculation during the sort target process
so that decoder probe order doesn't affect region probe.
Signed-off-by: Gregory Price <gourry@gourry.net>
---
drivers/cxl/core/region.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index e1ef0d577b35..8c79c0a39d56 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2061,6 +2061,7 @@ static int cxl_region_sort_targets(struct cxl_region *cxlr)
for (i = 0; i < p->nr_targets; i++) {
struct cxl_endpoint_decoder *cxled = p->targets[i];
+ cxled->pos = cxl_calc_interleave_pos(cxled);
/*
* Record that sorting failed, but still continue to calc
* cxled->pos so that follow-on code paths can reliably
--
2.47.1On Fri, Apr 04, 2025 at 10:36:49AM -0500, Gregory Price wrote: > The auto decoder probe proess overwrites the endpoint position > temporarily to record its temporary location in the region target list. > This patch restores the pos recalculation during the sort target process > so that decoder probe order doesn't affect region probe. > > Signed-off-by: Gregory Price <gourry@gourry.net> Disregard this patch, it appears to break is subtly different ways now - sigh :[. ~Gregory
© 2016 - 2025 Red Hat, Inc.