Re: [PATCH v4 8/9] cxl/region, dax/hmem: Tear down CXL regions when HMEM reclaims Soft Reserved

[<dan.j.williams@intel.com>]

Smita Koralahalli wrote:
> If CXL regions do not fully cover a Soft Reserved span, HMEM takes
> ownership. Tear down overlapping CXL regions before allowing HMEM to
> register and online the memory.
> 
> Add cxl_region_teardown() to walk CXL regions overlapping a span and
> unregister them via devm_release_action() and unregister_region().
> 
> Force the region state back to CXL_CONFIG_ACTIVE before unregistering to
> prevent the teardown path from resetting decoders HMEM still relies on
> to create its dax and online memory.
> 
> Co-developed-by: Alison Schofield <alison.schofield@intel.com>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
> ---
>  drivers/cxl/core/region.c | 38 ++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/cxl.h         |  5 +++++
>  drivers/dax/hmem/hmem.c   |  4 +++-
>  3 files changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 38e7ec6a087b..266b24028df0 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3784,6 +3784,44 @@ struct cxl_range_ctx {
>  	bool found;
>  };
>  
> +static int cxl_region_teardown_cb(struct device *dev, void *data)
> +{
> +	struct cxl_range_ctx *ctx = data;
> +	struct cxl_root_decoder *cxlrd;
> +	struct cxl_region_params *p;
> +	struct cxl_region *cxlr;
> +	struct cxl_port *port;
> +
> +	cxlr = cxlr_overlapping_range(dev, ctx->start, ctx->end);
> +	if (!cxlr)
> +		return 0;
> +
> +	cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
> +	port = cxlrd_to_port(cxlrd);
> +	p = &cxlr->params;
> +
> +	/* Force the region state back to CXL_CONFIG_ACTIVE so that

Minor, and moot given the follow on comments below, but please keep
consistent comment-style and lead with a /*, i.e.:

/*
 * Force the region...
 
> +	 * unregister_region() does not run the full decoder reset path
> +	 * which would invalidate the decoder programming that HMEM
> +	 * relies on to create its DAX device and online the underlying
> +	 * memory.
> +	 */
> +	scoped_guard(rwsem_write, &cxl_rwsem.region)
> +		p->state = min(p->state, CXL_CONFIG_ACTIVE);

I think the thickness of the above comment belies that this is too much
of a layering violation and likely to cause problems. For minimizing the
mental load of analyzing future bug reports, I want all regions gone
when any handshake with the platform firmware and dax-hmem occurs.  When
that happens it may mean destroying regions that were dynamically
created while waiting the wait_for_initial_probe() to timeout, who
knows. The simple policy is "CXL subsystem understands everything, or
touches nothing."

For this reset determination, what I think makes more sense, and is
generally useful for shutting down CXL even outside of the hmem deferral
trickery, is to always record whether decoders were idle or not at the
time of region creation. In fact we already have that flag, it is called
CXL_REGION_F_AUTO.

If CXL_REGION_F_AUTO is still set at detach_target() time, it means that
we are giving up on auto-assembly and leaving the decoders alone.

If the administrator actually wants to destroy and reclaim that
physical address space then they need to forcefully de-commit that
auto-assembled region via the @commit sysfs attribute. So that means
commit_store() needs to clear CXL_REGION_F_AUTO to get the decoder reset
to happen. 

[..]
>  void cxl_endpoint_parse_cdat(struct cxl_port *port);
> diff --git a/drivers/dax/hmem/hmem.c b/drivers/dax/hmem/hmem.c
> index b9312e0f2e62..7d874ee169ac 100644
> --- a/drivers/dax/hmem/hmem.c
> +++ b/drivers/dax/hmem/hmem.c
> @@ -158,8 +158,10 @@ static int handle_deferred_cxl(struct device *host, int target_nid,
>  		if (cxl_regions_fully_map(res->start, res->end)) {
>  			dax_cxl_mode = DAX_CXL_MODE_DROP;
>  			cxl_register_dax(res->start, res->end);
> -		} else
> +		} else {
>  			dax_cxl_mode = DAX_CXL_MODE_REGISTER;
> +			cxl_region_teardown(res->start, res->end);
> +		}

Like I alluded to above, I am not on board with making a range-by range
decision on teardown. The check for "all clear" vs "abort" should be a
global event before proceeding with either allowing cxl_region instances
to attach or all of them get destroyed. Recall that if
cxl_dax_region_probe() is globally rejecting all cxl_dax_region devices
until dax_cxl_mode moves to DAX_CXL_MODE_DROP then it keeps a consistent
behavior of all regions attach or none attach.