[v2,1/2] cxl/region: Try to add a region resource to a soft reserved parent

Commit Message

Alison Schofield Aug. 4, 2023, 7:31 p.m. UTC

From: Alison Schofield <[e-mail]>

During region autodiscovery, the region driver always inserts the
region resource as a child of the root decoder, a CXL WINDOW. It
has the effect of making a soft reserved resource, with an exactly
matching address range, a child of the region resource.

It looks like this in /proc/iomem:

2080000000-29dbfffffff : CXL Window 0
  2080000000-247fffffff : region0
    2080000000-247fffffff : Soft Reserved

Search for soft reserved resources that include the region resource,
and add the new region resource as a child of that found resource.
If a soft reserved resource is not found, insert to the root decoder
as usual.

With this change, it looks like this:

2080000000-29dbfffffff : CXL Window 0
  2080000000-247fffffff : Soft Reserved
    2080000000-247fffffff : region0

This odd parenting only occurs when the resources are an exact match.
When the region resource only uses part of a soft reserved resource,
the parenting appears more logical like this:

2080000000-29dbfffffff : CXL Window 0
  2080000000-287fffffff : Soft Reserved
    2080000000-247fffffff : region0

Aside from the more logical appearance, this change is in preparation
for further cleanup in region teardown. A follow-on patch intends to
find and free soft reserved resources upon region teardown.

Signed-off-by: Alison Schofield <[e-mail]>
---
 drivers/cxl/core/region.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

Comments

Jonathan Cameron Aug. 7, 2023, 2:24 p.m. UTC | #1

On Fri,  4 Aug 2023 12:31:39 -0700
[e-mail] wrote:

> From: Alison Schofield <[e-mail]>
> 
> During region autodiscovery, the region driver always inserts the
> region resource as a child of the root decoder, a CXL WINDOW. It
> has the effect of making a soft reserved resource, with an exactly
> matching address range, a child of the region resource.
> 
> It looks like this in /proc/iomem:
> 
> 2080000000-29dbfffffff : CXL Window 0
>   2080000000-247fffffff : region0
>     2080000000-247fffffff : Soft Reserved
> 
> Search for soft reserved resources that include the region resource,
> and add the new region resource as a child of that found resource.
> If a soft reserved resource is not found, insert to the root decoder
> as usual.
> 
> With this change, it looks like this:
> 
> 2080000000-29dbfffffff : CXL Window 0
>   2080000000-247fffffff : Soft Reserved
>     2080000000-247fffffff : region0
> 
> This odd parenting only occurs when the resources are an exact match.
> When the region resource only uses part of a soft reserved resource,
> the parenting appears more logical like this:
> 
> 2080000000-29dbfffffff : CXL Window 0
>   2080000000-287fffffff : Soft Reserved
>     2080000000-247fffffff : region0
> 
> Aside from the more logical appearance, this change is in preparation
> for further cleanup in region teardown. A follow-on patch intends to
> find and free soft reserved resources upon region teardown.
> 
> Signed-off-by: Alison Schofield <[e-mail]>

Hi Alison

A comment and a question inline.

Thanks,

Jonathan

> ---
>  drivers/cxl/core/region.c | 32 ++++++++++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index e115ba382e04..36c0c0dd5697 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2709,6 +2709,28 @@ static int match_region_by_range(struct device *dev, void *data)
>  	return rc;
>  }
>  
> +static int insert_resource_soft_reserved(struct resource *soft_res, void *arg)
> +{
> +	struct resource *parent, *new, *res = arg;
> +	bool found = false;
> +
> +	parent = soft_res->parent;
> +	if (!parent)
> +		return 0;
> +
> +	/* Caller provides a copy of soft_res. Find the actual resource. */
> +	for (new = parent->child; new; new = new->sibling) {
> +		if (resource_contains(new, soft_res)) {
> +			found = true;
> +			break;
> +		}
> +	}
> +	if (found)
> +		return insert_resource(new, res) == 0;

Why not do this in the the if above then directly return there?
Also, should we get an error here, does it make sense to carry on?
(I don't think we can get an error, but if that's the case we shouldn't
 be checking for one)  If it's a corner we aren't sure about, good
to add a comment on what the error case might be and what we are doing
as a result.

> +
> +	return 0;
> +}
> +
>  /* Establish an empty region covering the given HPA range */
>  static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>  					   struct cxl_endpoint_decoder *cxled)
> @@ -2719,7 +2741,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>  	struct cxl_region_params *p;
>  	struct cxl_region *cxlr;
>  	struct resource *res;
> -	int rc;
> +	int rc = 0;
>  
>  	do {
>  		cxlr = __create_region(cxlrd, cxled->mode,
> @@ -2755,7 +2777,13 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>  
>  	*res = DEFINE_RES_MEM_NAMED(hpa->start, range_len(hpa),
>  				    dev_name(&cxlr->dev));
> -	rc = insert_resource(cxlrd->res, res);
> +
> +	/* Try inserting to a Soft Reserved parent, fallback to root decoder */
> +	if (walk_iomem_res_desc(IORES_DESC_SOFT_RESERVED, 0,
> +				res->start, res->end, res,
> +				insert_resource_soft_reserved) != 1)
> +		rc = insert_resource(cxlrd->res, res);
> +
>  	if (rc) {
>  		/*
>  		 * Platform-firmware may not have split resources like "System

Dave Jiang Aug. 9, 2023, 11:22 p.m. UTC | #2

On 8/4/23 12:31, [e-mail] wrote:
> From: Alison Schofield <[e-mail]>
> 
> During region autodiscovery, the region driver always inserts the
> region resource as a child of the root decoder, a CXL WINDOW. It
> has the effect of making a soft reserved resource, with an exactly
> matching address range, a child of the region resource.
> 
> It looks like this in /proc/iomem:
> 
> 2080000000-29dbfffffff : CXL Window 0
>    2080000000-247fffffff : region0
>      2080000000-247fffffff : Soft Reserved
> 
> Search for soft reserved resources that include the region resource,
> and add the new region resource as a child of that found resource.

Don't think the comma is needed here.

> If a soft reserved resource is not found, insert to the root decoder
> as usual.
> 
> With this change, it looks like this:
> 
> 2080000000-29dbfffffff : CXL Window 0
>    2080000000-247fffffff : Soft Reserved
>      2080000000-247fffffff : region0
> 
> This odd parenting only occurs when the resources are an exact match.
> When the region resource only uses part of a soft reserved resource,
> the parenting appears more logical like this:
> 
> 2080000000-29dbfffffff : CXL Window 0
>    2080000000-287fffffff : Soft Reserved
>      2080000000-247fffffff : region0
> 
> Aside from the more logical appearance, this change is in preparation
> for further cleanup in region teardown. A follow-on patch intends to
> find and free soft reserved resources upon region teardown.
> 
> Signed-off-by: Alison Schofield <[e-mail]>
> ---
>   drivers/cxl/core/region.c | 32 ++++++++++++++++++++++++++++++--
>   1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index e115ba382e04..36c0c0dd5697 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2709,6 +2709,28 @@ static int match_region_by_range(struct device *dev, void *data)
>   	return rc;
>   }
>   
> +static int insert_resource_soft_reserved(struct resource *soft_res, void *arg)
> +{
> +	struct resource *parent, *new, *res = arg;
> +	bool found = false;
> +
> +	parent = soft_res->parent;
> +	if (!parent)
> +		return 0;
> +
> +	/* Caller provides a copy of soft_res. Find the actual resource. */
> +	for (new = parent->child; new; new = new->sibling) {
> +		if (resource_contains(new, soft_res)) {
> +			found = true;
> +			break;
> +		}
> +	}
> +	if (found)
> +		return insert_resource(new, res) == 0;
> +
> +	return 0;
> +}
> +
>   /* Establish an empty region covering the given HPA range */
>   static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>   					   struct cxl_endpoint_decoder *cxled)
> @@ -2719,7 +2741,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>   	struct cxl_region_params *p;
>   	struct cxl_region *cxlr;
>   	struct resource *res;
> -	int rc;
> +	int rc = 0;
>   
>   	do {
>   		cxlr = __create_region(cxlrd, cxled->mode,
> @@ -2755,7 +2777,13 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>   
>   	*res = DEFINE_RES_MEM_NAMED(hpa->start, range_len(hpa),
>   				    dev_name(&cxlr->dev));
> -	rc = insert_resource(cxlrd->res, res);
> +
> +	/* Try inserting to a Soft Reserved parent, fallback to root decoder */
> +	if (walk_iomem_res_desc(IORES_DESC_SOFT_RESERVED, 0,
> +				res->start, res->end, res,
> +				insert_resource_soft_reserved) != 1)
> +		rc = insert_resource(cxlrd->res, res);
> +
>   	if (rc) {
>   		/*
>   		 * Platform-firmware may not have split resources like "System

Alison Schofield Aug. 15, 2023, 12:10 a.m. UTC | #3

On Mon, Aug 07, 2023 at 03:24:07PM +0100, Jonathan Cameron wrote:
> On Fri,  4 Aug 2023 12:31:39 -0700
> [e-mail] wrote:
> 
> > From: Alison Schofield <[e-mail]>
> > 
> > During region autodiscovery, the region driver always inserts the
> > region resource as a child of the root decoder, a CXL WINDOW. It
> > has the effect of making a soft reserved resource, with an exactly
> > matching address range, a child of the region resource.
> > 
> > It looks like this in /proc/iomem:
> > 
> > 2080000000-29dbfffffff : CXL Window 0
> >   2080000000-247fffffff : region0
> >     2080000000-247fffffff : Soft Reserved
> > 
> > Search for soft reserved resources that include the region resource,
> > and add the new region resource as a child of that found resource.
> > If a soft reserved resource is not found, insert to the root decoder
> > as usual.
> > 
> > With this change, it looks like this:
> > 
> > 2080000000-29dbfffffff : CXL Window 0
> >   2080000000-247fffffff : Soft Reserved
> >     2080000000-247fffffff : region0
> > 
> > This odd parenting only occurs when the resources are an exact match.
> > When the region resource only uses part of a soft reserved resource,
> > the parenting appears more logical like this:
> > 
> > 2080000000-29dbfffffff : CXL Window 0
> >   2080000000-287fffffff : Soft Reserved
> >     2080000000-247fffffff : region0
> > 
> > Aside from the more logical appearance, this change is in preparation
> > for further cleanup in region teardown. A follow-on patch intends to
> > find and free soft reserved resources upon region teardown.
> > 
> > Signed-off-by: Alison Schofield <[e-mail]>
> 
> Hi Alison
> 
> A comment and a question inline.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/cxl/core/region.c | 32 ++++++++++++++++++++++++++++++--
> >  1 file changed, 30 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index e115ba382e04..36c0c0dd5697 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -2709,6 +2709,28 @@ static int match_region_by_range(struct device *dev, void *data)
> >  	return rc;
> >  }
> >  
> > +static int insert_resource_soft_reserved(struct resource *soft_res, void *arg)
> > +{
> > +	struct resource *parent, *new, *res = arg;
> > +	bool found = false;
> > +
> > +	parent = soft_res->parent;
> > +	if (!parent)
> > +		return 0;
> > +
> > +	/* Caller provides a copy of soft_res. Find the actual resource. */
> > +	for (new = parent->child; new; new = new->sibling) {
> > +		if (resource_contains(new, soft_res)) {
> > +			found = true;
> > +			break;
> > +		}
> > +	}
> > +	if (found)
> > +		return insert_resource(new, res) == 0;
> 

Thanks for the review Jonathan,

> Why not do this in the the if above then directly return there?
That would be better. Thanks.


> Also, should we get an error here, does it make sense to carry on?
> (I don't think we can get an error, but if that's the case we shouldn't
>  be checking for one)  If it's a corner we aren't sure about, good
> to add a comment on what the error case might be and what we are doing
> as a result.'


I see now this is hiding 2 cases in one error return
1) no soft reserved found
2) soft reserved found but failed to insert.

I think it can get an error if firmware set up soft reserved on bad
boundaries.

...scroll down...

> 
> > +
> > +	return 0;
> > +}
> > +
> >  /* Establish an empty region covering the given HPA range */
> >  static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
> >  					   struct cxl_endpoint_decoder *cxled)
> > @@ -2719,7 +2741,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
> >  	struct cxl_region_params *p;
> >  	struct cxl_region *cxlr;
> >  	struct resource *res;
> > -	int rc;
> > +	int rc = 0;
> >  
> >  	do {
> >  		cxlr = __create_region(cxlrd, cxled->mode,
> > @@ -2755,7 +2777,13 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
> >  
> >  	*res = DEFINE_RES_MEM_NAMED(hpa->start, range_len(hpa),
> >  				    dev_name(&cxlr->dev));
> > -	rc = insert_resource(cxlrd->res, res);
> > +
> > +	/* Try inserting to a Soft Reserved parent, fallback to root decoder */
> > +	if (walk_iomem_res_desc(IORES_DESC_SOFT_RESERVED, 0,
> > +				res->start, res->end, res,
> > +				insert_resource_soft_reserved) != 1)
> > +		rc = insert_resource(cxlrd->res, res);
> > +

I'll rework this error handling to not try to insert to root decoder if
insert to soft reserved fails. Only try to insert to root decoder if soft
reserved does not exist.

> >  	if (rc) {
> >  		/*
> >  		 * Platform-firmware may not have split resources like "System
>

I think the insert to soft reserved can fail as describe here:
(Let me show this complete error case above)

	if (rc) {
                /*
                 * Platform-firmware may not have split resources like "System
                 * RAM" on CXL window boundaries see cxl_region_iomem_release()
                 */
                dev_warn(cxlmd->dev.parent,
                         "%s:%s: %s %s cannot insert resource\n",
                         dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
                         __func__, dev_name(&cxlr->dev));
        }

Alison Schofield Aug. 15, 2023, 12:12 a.m. UTC | #4

On Wed, Aug 09, 2023 at 04:22:03PM -0700, Dave Jiang wrote:
> 
> 
> On 8/4/23 12:31, [e-mail] wrote:
> > From: Alison Schofield <[e-mail]>
> > 
> > During region autodiscovery, the region driver always inserts the
> > region resource as a child of the root decoder, a CXL WINDOW. It
> > has the effect of making a soft reserved resource, with an exactly
> > matching address range, a child of the region resource.
> > 
> > It looks like this in /proc/iomem:
> > 
> > 2080000000-29dbfffffff : CXL Window 0
> >    2080000000-247fffffff : region0
> >      2080000000-247fffffff : Soft Reserved
> > 
> > Search for soft reserved resources that include the region resource,
> > and add the new region resource as a child of that found resource.
> 
> Don't think the comma is needed here.

Got it. Thanks!


> 
> > If a soft reserved resource is not found, insert to the root decoder
> > as usual.
> > 
> > With this change, it looks like this:
> > 
> > 2080000000-29dbfffffff : CXL Window 0
> >    2080000000-247fffffff : Soft Reserved
> >      2080000000-247fffffff : region0
> > 
> > This odd parenting only occurs when the resources are an exact match.
> > When the region resource only uses part of a soft reserved resource,
> > the parenting appears more logical like this:
> > 
> > 2080000000-29dbfffffff : CXL Window 0
> >    2080000000-287fffffff : Soft Reserved
> >      2080000000-247fffffff : region0
> > 
> > Aside from the more logical appearance, this change is in preparation
> > for further cleanup in region teardown. A follow-on patch intends to
> > find and free soft reserved resources upon region teardown.
> > 
> > Signed-off-by: Alison Schofield <[e-mail]>
> > ---
> >   drivers/cxl/core/region.c | 32 ++++++++++++++++++++++++++++++--
> >   1 file changed, 30 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index e115ba382e04..36c0c0dd5697 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -2709,6 +2709,28 @@ static int match_region_by_range(struct device *dev, void *data)
> >   	return rc;
> >   }
> > +static int insert_resource_soft_reserved(struct resource *soft_res, void *arg)
> > +{
> > +	struct resource *parent, *new, *res = arg;
> > +	bool found = false;
> > +
> > +	parent = soft_res->parent;
> > +	if (!parent)
> > +		return 0;
> > +
> > +	/* Caller provides a copy of soft_res. Find the actual resource. */
> > +	for (new = parent->child; new; new = new->sibling) {
> > +		if (resource_contains(new, soft_res)) {
> > +			found = true;
> > +			break;
> > +		}
> > +	}
> > +	if (found)
> > +		return insert_resource(new, res) == 0;
> > +
> > +	return 0;
> > +}
> > +
> >   /* Establish an empty region covering the given HPA range */
> >   static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
> >   					   struct cxl_endpoint_decoder *cxled)
> > @@ -2719,7 +2741,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
> >   	struct cxl_region_params *p;
> >   	struct cxl_region *cxlr;
> >   	struct resource *res;
> > -	int rc;
> > +	int rc = 0;
> >   	do {
> >   		cxlr = __create_region(cxlrd, cxled->mode,
> > @@ -2755,7 +2777,13 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
> >   	*res = DEFINE_RES_MEM_NAMED(hpa->start, range_len(hpa),
> >   				    dev_name(&cxlr->dev));
> > -	rc = insert_resource(cxlrd->res, res);
> > +
> > +	/* Try inserting to a Soft Reserved parent, fallback to root decoder */
> > +	if (walk_iomem_res_desc(IORES_DESC_SOFT_RESERVED, 0,
> > +				res->start, res->end, res,
> > +				insert_resource_soft_reserved) != 1)
> > +		rc = insert_resource(cxlrd->res, res);
> > +
> >   	if (rc) {
> >   		/*
> >   		 * Platform-firmware may not have split resources like "System

Patch

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index e115ba382e04..36c0c0dd5697 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2709,6 +2709,28 @@  static int match_region_by_range(struct device *dev, void *data)
 	return rc;
 }
 
+static int insert_resource_soft_reserved(struct resource *soft_res, void *arg)
+{
+	struct resource *parent, *new, *res = arg;
+	bool found = false;
+
+	parent = soft_res->parent;
+	if (!parent)
+		return 0;
+
+	/* Caller provides a copy of soft_res. Find the actual resource. */
+	for (new = parent->child; new; new = new->sibling) {
+		if (resource_contains(new, soft_res)) {
+			found = true;
+			break;
+		}
+	}
+	if (found)
+		return insert_resource(new, res) == 0;
+
+	return 0;
+}
+
 /* Establish an empty region covering the given HPA range */
 static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 					   struct cxl_endpoint_decoder *cxled)
@@ -2719,7 +2741,7 @@  static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 	struct cxl_region_params *p;
 	struct cxl_region *cxlr;
 	struct resource *res;
-	int rc;
+	int rc = 0;
 
 	do {
 		cxlr = __create_region(cxlrd, cxled->mode,
@@ -2755,7 +2777,13 @@  static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 
 	*res = DEFINE_RES_MEM_NAMED(hpa->start, range_len(hpa),
 				    dev_name(&cxlr->dev));
-	rc = insert_resource(cxlrd->res, res);
+
+	/* Try inserting to a Soft Reserved parent, fallback to root decoder */
+	if (walk_iomem_res_desc(IORES_DESC_SOFT_RESERVED, 0,
+				res->start, res->end, res,
+				insert_resource_soft_reserved) != 1)
+		rc = insert_resource(cxlrd->res, res);
+
 	if (rc) {
 		/*
 		 * Platform-firmware may not have split resources like "System