Re: [PATCH v16 21/22] cxl: Add function for obtaining region range

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

alejandro.lucero-palau@ wrote:
> From: Alejandro Lucero <alucerop@amd.com>
> 
> A CXL region struct contains the physical address to work with.
> 
> Type2 drivers can create a CXL region but have not access to the
> related struct as it is defined as private by the kernel CXL core.
> Add a function for getting the cxl region range to be used for mapping
> such memory range by a Type2 driver.
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> Reviewed-by: Zhi Wang <zhiw@nvidia.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/region.c | 23 +++++++++++++++++++++++
>  include/cxl/cxl.h         |  2 ++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 06647bae210f..9b7c6b8304d6 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2726,6 +2726,29 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
>  	return ERR_PTR(rc);
>  }
>  
> +/**
> + * cxl_get_region_range - obtain range linked to a CXL region
> + *
> + * @region: a pointer to struct cxl_region
> + * @range: a pointer to a struct range to be set
> + *
> + * Returns 0 or error.
> + */
> +int cxl_get_region_range(struct cxl_region *region, struct range *range)
> +{
> +	if (WARN_ON_ONCE(!region))
> +		return -ENODEV;
> +
> +	if (!region->params.res)
> +		return -ENOSPC;
> +
> +	range->start = region->params.res->start;
> +	range->end = region->params.res->end;

Region params are only consistent under cxl_region_rwsem. Whatever is
consuming this will want to have that consistent snapshot and some
coordination with the region shutdown / de-commit flow.

This again raises the question, what do you expect to happen after the
->remove(&cxlmd->dev) event?

For Type-3 the expectation is leave all the decoders in place and
reassemble the region from past hardware settings (as if the decode
range had been established by platform firmware).

Another model could be "never trust an existing decoder and always reset
the configuration when the driver loads. That would also involve walking
the topology to reset any upstream switch decoders that were decoding
the old configuration.

The current model in these patches is "unwind nothing at cxl_mem detach
time, hope that probe_data->cxl->cxlmd is attached immediately upon
return from devm_add_cxl_memdev(), hope that it remains attached until
efx_cxl_exit() runs, and always assume a fresh "from scratch" HDM decode
configuration at efx_cxl_init() time".

I do often cringe at the complexity of the CXL subsystem, but it is all
complexity that the CXL programming model mandates. Specifically, CXL
window capacity being a dynamically assigned limited resource that needs
runtime re-configuration across multiple devices/switches, and resources
that can interleave host-bridges and endpoints. Compare that to PCIe
that mostly statically assigns MMIO resources throughout the topology,
rarely needs to reassign that, and never interleaves.

Yes, there is some self-inflicted complexity in the CXL subsystem
introduced by allowing drivers like cxl_mem and cxl_acpi to logically
detach at runtime. However given cxl_mem needs to be prepared for
physical detachment there is no simple escape from handling the "dynamic
CXL HDM decode teardown" problem.