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

["Cheatham, Benjamin" <benjamin.cheatham@amd.com>]

On 3/23/2026 6:31 AM, 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
> 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>
> ---
>  drivers/cxl/core/hdm.c | 39 +++++++++++++++++++++++++++++++++++++++
>  include/cxl/cxl.h      |  3 +++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index c222e98ae736..006be88efaa5 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -686,6 +686,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_endpoint_decoder(struct device *dev, const void *data)
> +{
> +	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;
> +}

I am still not convinced this is the right check. I have two reasons:

1. I would be surprised if just always taking the last endpoint decoder is what we want here.
There's no guarantee that the last endpoint decoder is programmed to match the range, which is only
now an issue since you're not looking at uncommitted decoders anymore. Suggestion below...

2. I'm sure it's possible, but I'm not aware of any case (outside of interleaving) that there
would be multiple endpoint decoders above a singular port. So, it may be that the whole check is
overkill and we can just cast the endpoint port into the endpoint decoder and be done with it.
If the rest of the set hasn't changed from v23, interleaving is still broken. At that point,
I'd say ditch the interleaving support for now and we can complicate all of this later.

Suggestion: If you don't want to abandon interleaving or I'm wrong about the topology, checking
if the decoder is committed by checking the HDM decoder registers directly is probably the
correct way to do this.

That gets you *a* decoder, but I'm not sure it's the right one. Getting the right one would require
checking the resource attached to the decoder and verifying it matches the HPA range expected by
the accelerator and/or region. I don't know if we expect the accelerator to know the HPA range
at this point, in which case this is pretty easy. I have an inkling of what to do otherwise, but
I'd need time to think it through.
> +
> +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_endpoint_decoder);
> +
> +	if (!cxled_dev)
> +		return NULL;
> +
> +	cxled = to_cxl_endpoint_decoder(cxled_dev);
> +	*cxlr = cxled->cxld.region;

I think the region type should be set already at this point, so I'd check it's set to
CXL_DECODER_DEVMEM here as a sanity check.

> +
> +	put_device(cxled_dev);
> +	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 10a9b8fa2f6b..2a61138e2a73 100644
> --- a/include/cxl/cxl.h
> +++ b/include/cxl/cxl.h
> @@ -231,4 +231,7 @@ struct cxl_dev_state *_devm_cxl_dev_state_create(struct device *dev,
>  int cxl_set_capacity(struct cxl_dev_state *cxlds, u64 capacity);
>  struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds,
>  				       const struct cxl_memdev_attach *attach);
> +struct cxl_region;
> +struct cxl_endpoint_decoder *cxl_get_committed_decoder(struct cxl_memdev *cxlmd,
> +						       struct cxl_region **cxlr);
>  #endif /* __CXL_CXL_H__ */