Re: [PATCH v3 1/2] cxl/region: Try to add a region resource to a soft reserved parent

[Dave Jiang <dave.jiang@intel.com>]

On 8/21/23 11:14, alison.schofield@intel.com wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> 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 <alison.schofield@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>   drivers/cxl/core/region.c | 57 ++++++++++++++++++++++++++++++++-------
>   1 file changed, 47 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index e115ba382e04..5c487aab15ad 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2709,6 +2709,31 @@ 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;
> +	int rc = 0;
> +
> +	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)) {
> +			rc = insert_resource(new, res);
> +			found = true;
> +			break;
> +		}
> +	}
> +	/* Caller handles failure to find or insert resource */
> +	if (!found || rc)
> +		return rc;
> +
> +	return 1;
> +}
> +
>   /* 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)
> @@ -2755,16 +2780,28 @@ 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);
> -	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));
> +
> +	/* Try inserting to a Soft Reserved parent. Fallback to root decoder */
> +	rc = walk_iomem_res_desc(IORES_DESC_SOFT_RESERVED, 0, res->start,
> +				 res->end, res, insert_resource_soft_reserved);
> +	if (!rc || rc == -EBUSY)
> +		dev_dbg(&cxlmd->dev,
> +			"insert %pr to soft reserved parent failed rc:%d\n",
> +			res, rc);
> +	if (rc != 1) {
> +		rc = insert_resource(cxlrd->res, res);
> +		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));
> +		}
>   	}
>   
>   	p->res = res;