For the implementation of address translation it might not be possible
to determine the root decoder in the early enumeration state since the
SPA range is still unknown. Instead, the endpoint's HPA range is known
and from there the topology can be traversed up to the root port while
the memory range is adjusted from one memory domain to the next up to
the root port.

In a first step, use endpoint's HPA range to find the port's decoder.
Without address translation there is HPA == SPA. Then, the HPA range
of the endpoint can be used instead of the root decoder's range as
both are the same.

Signed-off-by: Robert Richter <rrichter@amd.com>
---
 drivers/cxl/core/region.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index b7f6d8a83e4e..23b86de3d4e7 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -861,9 +861,8 @@ static int cxl_port_calc_hpa(struct cxl_port *port, struct cxl_decoder *cxld,
 
 static int match_auto_decoder(struct device *dev, void *data)
 {
-	struct cxl_region_params *p = data;
+	struct range *r, *hpa = data;
 	struct cxl_decoder *cxld;
-	struct range *r;
 
 	if (!is_switch_decoder(dev))
 		return 0;
@@ -871,7 +870,7 @@ static int match_auto_decoder(struct device *dev, void *data)
 	cxld = to_cxl_decoder(dev);
 	r = &cxld->hpa_range;
 
-	if (p->res && p->res->start == r->start && p->res->end == r->end)
+	if (hpa && hpa->start == r->start && hpa->end == r->end)
 		return 1;
 
 	return 0;
@@ -888,7 +887,7 @@ cxl_find_decoder_early(struct cxl_port *port,
 		return &cxled->cxld;
 
 	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags))
-		dev = device_find_child(&port->dev, &cxlr->params,
+		dev = device_find_child(&port->dev, &cxled->cxld.hpa_range,
 					match_auto_decoder);
 	else
 		dev = device_find_child(&port->dev, NULL, match_free_decoder);
-- 
2.39.5

On 1/7/25 8:10 AM, Robert Richter wrote:
> For the implementation of address translation it might not be possible
> to determine the root decoder in the early enumeration state since the
> SPA range is still unknown. Instead, the endpoint's HPA range is known
> and from there the topology can be traversed up to the root port while
> the memory range is adjusted from one memory domain to the next up to
> the root port.
> 
> In a first step, use endpoint's HPA range to find the port's decoder.
> Without address translation there is HPA == SPA. Then, the HPA range
> of the endpoint can be used instead of the root decoder's range as
> both are the same.

I think this can be clearer. Something like:

"In a first step, use endpoint's HPA range to find the port's decoder.
Without address translation HPA == SPA, so the endpoint's HPA range can
be used since it is the same as the root decoder's.

> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
>  drivers/cxl/core/region.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index b7f6d8a83e4e..23b86de3d4e7 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -861,9 +861,8 @@ static int cxl_port_calc_hpa(struct cxl_port *port, struct cxl_decoder *cxld,
>  
>  static int match_auto_decoder(struct device *dev, void *data)
>  {
> -	struct cxl_region_params *p = data;
> +	struct range *r, *hpa = data;
>  	struct cxl_decoder *cxld;
> -	struct range *r;
>  
>  	if (!is_switch_decoder(dev))
>  		return 0;
> @@ -871,7 +870,7 @@ static int match_auto_decoder(struct device *dev, void *data)
>  	cxld = to_cxl_decoder(dev);
>  	r = &cxld->hpa_range;
>  
> -	if (p->res && p->res->start == r->start && p->res->end == r->end)
> +	if (hpa && hpa->start == r->start && hpa->end == r->end)
>  		return 1;
>  
>  	return 0;
> @@ -888,7 +887,7 @@ cxl_find_decoder_early(struct cxl_port *port,
>  		return &cxled->cxld;
>  
>  	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags))
> -		dev = device_find_child(&port->dev, &cxlr->params,
> +		dev = device_find_child(&port->dev, &cxled->cxld.hpa_range,
>  					match_auto_decoder);
>  	else
>  		dev = device_find_child(&port->dev, NULL, match_free_decoder);

On 17.01.25 15:31:51, Ben Cheatham wrote:
> On 1/7/25 8:10 AM, Robert Richter wrote:
> > For the implementation of address translation it might not be possible
> > to determine the root decoder in the early enumeration state since the
> > SPA range is still unknown. Instead, the endpoint's HPA range is known
> > and from there the topology can be traversed up to the root port while
> > the memory range is adjusted from one memory domain to the next up to
> > the root port.
> > 
> > In a first step, use endpoint's HPA range to find the port's decoder.
> > Without address translation there is HPA == SPA. Then, the HPA range
> > of the endpoint can be used instead of the root decoder's range as
> > both are the same.
> 
> I think this can be clearer. Something like:
> 
> "In a first step, use endpoint's HPA range to find the port's decoder.
> Without address translation HPA == SPA, so the endpoint's HPA range can
> be used since it is the same as the root decoder's.

Changed, thanks.

-Robert

On Tue, Jan 07, 2025 at 03:10:05PM +0100, Robert Richter wrote:
> For the implementation of address translation it might not be possible
> to determine the root decoder in the early enumeration state since the
> SPA range is still unknown. Instead, the endpoint's HPA range is known
> and from there the topology can be traversed up to the root port while
> the memory range is adjusted from one memory domain to the next up to
> the root port.
> 
> In a first step, use endpoint's HPA range to find the port's decoder.
> Without address translation there is HPA == SPA. Then, the HPA range
> of the endpoint can be used instead of the root decoder's range as
> both are the same.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>

I'll make a note to test up to this patch in particular in HPA==SPA
mode without the follow ons.

The functional change here is the move from cxlr->params to
cxled->cxld.hpa_range. 

What was the likely value of cxlr->params previously? Undefined?

Otherwise I don't immediately see any issues.

Reviewed-by: Gregory Price <gourry@gourry.net>

> ---
>  drivers/cxl/core/region.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index b7f6d8a83e4e..23b86de3d4e7 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -861,9 +861,8 @@ static int cxl_port_calc_hpa(struct cxl_port *port, struct cxl_decoder *cxld,
>  
>  static int match_auto_decoder(struct device *dev, void *data)
>  {
> -	struct cxl_region_params *p = data;
> +	struct range *r, *hpa = data;
>  	struct cxl_decoder *cxld;
> -	struct range *r;
>  
>  	if (!is_switch_decoder(dev))
>  		return 0;
> @@ -871,7 +870,7 @@ static int match_auto_decoder(struct device *dev, void *data)
>  	cxld = to_cxl_decoder(dev);
>  	r = &cxld->hpa_range;
>  
> -	if (p->res && p->res->start == r->start && p->res->end == r->end)
> +	if (hpa && hpa->start == r->start && hpa->end == r->end)
>  		return 1;
>  
>  	return 0;
> @@ -888,7 +887,7 @@ cxl_find_decoder_early(struct cxl_port *port,
>  		return &cxled->cxld;
>  
>  	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags))
> -		dev = device_find_child(&port->dev, &cxlr->params,
> +		dev = device_find_child(&port->dev, &cxled->cxld.hpa_range,
>  					match_auto_decoder);
>  	else
>  		dev = device_find_child(&port->dev, NULL, match_free_decoder);
> -- 
> 2.39.5
>

On 07.01.25 17:18:49, Gregory Price wrote:
> On Tue, Jan 07, 2025 at 03:10:05PM +0100, Robert Richter wrote:
> > For the implementation of address translation it might not be possible
> > to determine the root decoder in the early enumeration state since the
> > SPA range is still unknown. Instead, the endpoint's HPA range is known
> > and from there the topology can be traversed up to the root port while
> > the memory range is adjusted from one memory domain to the next up to
> > the root port.
> > 
> > In a first step, use endpoint's HPA range to find the port's decoder.
> > Without address translation there is HPA == SPA. Then, the HPA range
> > of the endpoint can be used instead of the root decoder's range as
> > both are the same.
> > 
> > Signed-off-by: Robert Richter <rrichter@amd.com>
> 
> I'll make a note to test up to this patch in particular in HPA==SPA
> mode without the follow ons.
> 
> The functional change here is the move from cxlr->params to
> cxled->cxld.hpa_range. 
> 
> What was the likely value of cxlr->params previously? Undefined?

The region's hpa range was set up with the same values. Thus it can be
used interchangeable.

Call chain:

cxl_endpoint_decoder_add
  hpa = &cxled->cxld.hpa_range;
  cxl_find_region_by_range(..., hpa);
    match_region_by_range(..., hpa);
  construct_region
    hpa = &cxled->cxld.hpa_range;
    *res = DEFINE_RES_MEM_NAMED(hpa->start, range_len(hpa), ...)
  attach_target
    cxl_region_attach
      cxl_region_attach_position
        cxl_port_attach_region
          cxl_find_decoder_early
            hpa = &cxled->cxld.hpa_range
            match_auto_decoder(..., hpa)

Note I have moved out the switch to spa_range of "[PATCH v1 15/29]
cxl/region: Use an endpoint's SPA range to find a region" to a later
patch for a better understanding of the changes, but technically it is
the same as there is still spa == hpa at this point.

> 
> Otherwise I don't immediately see any issues.
> 
> Reviewed-by: Gregory Price <gourry@gourry.net>

Thanks,

-Robert