Message-ID: <f56f7a6b-7931-4264-8d42-50603ce81cba@amd.com>
Date: Thu, 18 Dec 2025 11:52:58 +0000
From: Alejandro Lucero Palau <alucerop@....com>
To: Jonathan Cameron <jonathan.cameron@...wei.com>,
 alejandro.lucero-palau@....com
Cc: linux-cxl@...r.kernel.org, netdev@...r.kernel.org,
 dan.j.williams@...el.com, edward.cree@....com, davem@...emloft.net,
 kuba@...nel.org, pabeni@...hat.com, edumazet@...gle.com, dave.jiang@...el.com
Subject: Re: [PATCH v22 11/25] cxl/hdm: Add support for getting region from
 committed decoder

Hi Jonathan,


On 12/15/25 13:50, Jonathan Cameron wrote:
> On Fri, 5 Dec 2025 11:52:34 +0000
> <alejandro.lucero-palau@....com> wrote:
>
>> From: Alejandro Lucero <alucerop@....com>
>>
>> A Type2 device configured by the BIOS can already have its HDM
>> committed. Add a cxl_get_committed_decoder() function for cheking
> checking if this is so after memdev creation.
>
>> so after memdev creation. A CXL region should have been created
>> during memdev initialization, therefore a Type2 driver can ask for
>> such a region for working with the HPA. If the HDM is not committed,
>> a Type2 driver will create the region after obtaining proper HPA
>> and DPA space.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@....com>
> Hi Alejandro,
>
> I'm in two minds about this.  In general there are devices that have
> been configured by the BIOS because they are already in use. I'm not sure
> the driver you are working with here is necessarily set up to survive
> that sort of live setup without interrupting data flows.


This is not mainly about my driver/device but something PJ and Dan agree 
on support along this type2 patchset.

You can see the v21 discussions, but basically PJ can not have his 
driver using the committed decoders from BIOS. So this change addresses 
that situation which my driver/device can also benefit from as current 
BIOS available is committing decoders regardless of UEFI flags like 
EFI_RESERVED_TYPE.


Neither in my case nor in PJ case the device will be in use before 
kernel is executing, although PJ should confirm this.


>
> If it is fair enough to support this, otherwise my inclination is tear
> down whatever the bios did and start again (unless locked - in which
> case go grumble at your BIOS folk). Reasoning being that we then only
> have to handle the equivalent of the hotplug flow in both cases rather
> than having to handle 2.


Well, the automatic discovery region used for Type3 can be reused for 
Type2 in this scenario, so we do not need to tear down what the BIOS 
did. However, the argument is what we should do when the driver exits 
which the current functionality added with the patchset being tearing 
down the device and CXL bridge decoders. Dan seems to be keen on not 
doing this tear down even if the HDMs are not locked.


What I can say is I have tested this patchset with an AMD system and 
with the BIOS committing the HDM decoders for my device, and the first 
time the driver loads it gets the region from the automatic discovery 
while creating memdev, and the driver does tear down the HDMs when 
exiting. Subsequent driver loads do the HDM configuration as this 
patchset had been doing from day one. So all works as expected.


I'm inclined to leave the functionality as it is now, and your 
suggestion or Dan's one for keeping the HDMs, as they were configured by 
the BIOS, when driver exits should require, IMO, a good reason behind it.


> There are also the TSP / encrypted link cases where we need to be careful.
> I have no idea if that applies here.


I would say, let's wait until this support is completed, but as far as I 
know, this is not a requirement for current Type2 clients (sfc and jump 
trading).


> So I'm not against this in general, just not sure there is an argument
> for this approach 'yet'. If there is, give more breadcrumbs to it in this
> commit message.
>
> A few comments inline.
>
>> ---
>>   drivers/cxl/core/hdm.c | 44 ++++++++++++++++++++++++++++++++++++++++++
>>   include/cxl/cxl.h      |  3 +++
>>   2 files changed, 47 insertions(+)
>>
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index d3a094ca01ad..fa99657440d1 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
>> @@ -92,6 +92,7 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
>>   static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
>>   {
>>   	struct cxl_hdm *cxlhdm;
>> +	struct cxl_port *port;
>>   	void __iomem *hdm;
>>   	u32 ctrl;
>>   	int i;
>> @@ -105,6 +106,10 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
>>   	if (!hdm)
>>   		return true;
>>   
>> +	port = cxlhdm->port;
>> +	if (is_cxl_endpoint(port))
>> +		return false;
> Why this change?  If it was valid before this patch as an early exit
> then do it in a patch that justifies that not buried in here.


Good catch. I needed this hack for the functionality described, because 
the second time the driver loads this check turns to be positive because 
the memory state. I think I understand the reason behind this decoders 
emulation, but being honest, I do not understand what such emulation 
depends on. I would say once the device advertises HDM, it should never 
depend on other things, what seems to be the case now. I will explain 
more about the problem in the following days.


>> +
>>   	/*
>>   	 * If HDM decoders are present and the driver is in control of
>>   	 * Mem_Enable skip DVSEC based emulation
>> @@ -686,6 +691,45 @@ int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, u64 size)
>>   	return devm_add_action_or_reset(&port->dev, cxl_dpa_release, cxled);
>>   }
>>   
>> +static int find_committed_decoder(struct device *dev, const void *data)
> Function name rather suggests it finds committed decoders on 'whatever'
> but it only works for the endpoint decoders.  Rename it to avoid this
> confusion.


OK


>
>> +{
>> +	struct cxl_endpoint_decoder *cxled;
>> +	struct cxl_port *port;
>> +
>> +	if (!is_endpoint_decoder(dev))
>> +		return 0;
>> +
>> +	cxled = to_cxl_endpoint_decoder(dev);
>> +	port = cxled_to_port(cxled);
>> +
>> +	return cxled->cxld.id == (port->hdm_end);
> Drop the ()


Sure.


>
>> +}
>> +
>> +struct cxl_endpoint_decoder *cxl_get_committed_decoder(struct cxl_memdev *cxlmd,
>> +						       struct cxl_region **cxlr)
>> +{
>> +	struct cxl_port *endpoint = cxlmd->endpoint;
>> +	struct cxl_endpoint_decoder *cxled;
>> +	struct device *cxled_dev;
>> +
>> +	if (!endpoint)
>> +		return NULL;
>> +
>> +	guard(rwsem_read)(&cxl_rwsem.dpa);
>> +	cxled_dev = device_find_child(&endpoint->dev, NULL,
>> +				      find_committed_decoder);
>> +
>> +	if (!cxled_dev)
>> +		return NULL;
>> +
>> +	cxled = to_cxl_endpoint_decoder(cxled_dev);
>> +	*cxlr = cxled->cxld.region;
>> +
>> +	put_device(cxled_dev);
> Probably use a __free() for this.


I'll think about it.


Thanks!


>> +	return cxled;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_get_committed_decoder, "CXL");
>> +
>>   static void cxld_set_interleave(struct cxl_decoder *cxld, u32 *ctrl)
>>   {
>>   	u16 eig;
>> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
>> index 043fc31c764e..2ff3c19c684c 100644
>> --- a/include/cxl/cxl.h
>> +++ b/include/cxl/cxl.h
>> @@ -250,4 +250,7 @@ int cxl_set_capacity(struct cxl_dev_state *cxlds, u64 capacity);
>>   struct cxl_memdev *devm_cxl_add_memdev(struct device *host,
>>   				       struct cxl_dev_state *cxlds,
>>   				       const struct cxl_memdev_ops *ops);
>> +struct cxl_region;
>> +struct cxl_endpoint_decoder *cxl_get_committed_decoder(struct cxl_memdev *cxlmd,
>> +						       struct cxl_region **cxlr);
>>   #endif /* __CXL_CXL_H__ */

Powered by blists - more mailing lists