Message-ID: <e9810a8b-b1c4-4f84-926f-3b8c05b2099a@amd.com>
Date: Fri, 6 Jun 2025 15:03:27 +0100
From: Alejandro Lucero Palau <alucerop@....com>
To: Dan Williams <dan.j.williams@...el.com>, alejandro.lucero-palau@....com,
 linux-cxl@...r.kernel.org, netdev@...r.kernel.org, edward.cree@....com,
 davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
 edumazet@...gle.com, dave.jiang@...el.com
Cc: Zhi Wang <zhiw@...dia.com>, Jonathan Cameron <Jonathan.Cameron@...wei.com>
Subject: Re: [PATCH v16 21/22] cxl: Add function for obtaining region range


On 5/21/25 22:31, Dan Williams wrote:
> alejandro.lucero-palau@ wrote:
>> From: Alejandro Lucero <alucerop@....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@....com>
>> Reviewed-by: Zhi Wang <zhiw@...dia.com>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
>> Reviewed-by: Dave Jiang <dave.jiang@...el.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.


I assumed the owner of the region could be confident the region would be 
there ...


This is more of the same problem discussed in previous patches ... where 
having cxl_acquire_endpoint makes sense. But it makes me wonder if we 
should allow cxl_acpi or cxl_mem to be removed at all while clients 
depend on them. Is there a case for this apart from the fact that 
current implementation with kernel modules allow it?


>
> 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).


Not sure I follow this logic. So Type3 devices can still be accessed 
after cxl_acpi or cxl_mem are removed? How does it work when those 
modules are loaded again?


>
> 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".


Part of that expected model is fine as long as the sfc driver does exit 
doing the cxl unwinding like cxl_accel_region_detach. That should leave 
the SFC CXL HDM decoder as in a fresh start ... . Anyway, I got what you 
are warning about here and in previous patches, so I will try to address 
them or at least identify most of the potential situations using your 
reviews as the starting point. Maybe it is a good moment for going back 
to my statement about the patchset being about "basic" support ...


>
> 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.

Powered by blists - more mailing lists