Re: [PATCH v22 11/25] cxl/hdm: Add support for getting region from committed decoder

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Fri, 5 Dec 2025 11:52:34 +0000
<alejandro.lucero-palau@amd.com> wrote:

> From: Alejandro Lucero <alucerop@amd.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@amd.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.

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.

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

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.

> +
>  	/*
>  	 * 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.

> +{
> +	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 ()


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

> +	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__ */