Re: [PATCH 01/20] cxl: Introduce cxl_get_hdm_reg_info()

[Jonathan Cameron]

On Thu, 12 Mar 2026 02:04:21 +0530
mhonap@xxxxxxxxxx wrote:

> From: Manish Honap <mhonap@xxxxxxxxxx>
> 
> CXL core has the information of what CXL register groups a device has.
> When initializing the device, the CXL core probes the register groups
> and saves the information. The probing sequence is quite complicated.
> 
> vfio-cxl requires the HDM register information to emulate the HDM decoder
> registers.
> 
> Introduce cxl_get_hdm_reg_info() for vfio-cxl to leverage the HDM
> register information in the CXL core. Thus, it doesn't need to implement
> its own probing sequence.
> 
> Co-developed-by: Zhi Wang <zhiw@xxxxxxxxxx>
> Signed-off-by: Zhi Wang <zhiw@xxxxxxxxxx>
> Signed-off-by: Manish Honap <mhonap@xxxxxxxxxx>

Hi Manish, Zhi,

Taking a first pass look at this so likely comments will be trivial
whilst I get my head around it.

Jonathan

> ---
>  drivers/cxl/core/pci.c | 45 ++++++++++++++++++++++++++++++++++++++++++
>  include/cxl/cxl.h      |  3 +++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index ba2d393c540a..52ed0b4f5e78 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -449,6 +449,51 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_hdm_decode_init, "CXL");
>  
> +/**
> + * cxl_get_hdm_reg_info - Get HDM decoder register block location and count
> + * @cxlds: CXL device state (must have component regs enumerated)
> + * @count: Output: number of HDM decoders (from DVSEC cap). Only set when
> + *         the device has a valid HDM decoder capability.
> + * @offset: Output: byte offset of the HDM decoder register block within the
> + *          component register BAR. Only set when valid.
> + * @size: Output: size in bytes of the HDM decoder register block. Only set
> + *        when valid.
> + *
> + * Reads the CXL component register map and DVSEC capability to return the
> + * Host Managed Device Memory (HDM) decoder register block offset and size,
> + * and the number of HDM decoders.

No it doesn't, see below.

> This function requires cxlds->cxl_dvsec
> + * to be non-zero.
> + *
> + * Return: 0 on success. A negative errno is returned when config read
> + * failure or when the decoder registers are not valid.
> + */
> +int cxl_get_hdm_reg_info(struct cxl_dev_state *cxlds, u32 *count,

If we are going to use a fixed size for count (which is fine) maybe pick a
smaller one.  It's never bigger than a u8.  Actually never bigger than 2...
See below.

> +			 resource_size_t *offset, resource_size_t *size)
> +{
> +	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> +	struct cxl_component_reg_map *map =
> +		&cxlds->reg_map.component_map;

Trivial: Fits on one < 80 char line.

> +	int d = cxlds->cxl_dvsec;
> +	u16 cap;
> +	int rc;
> +
> +	/* HDM decoder registers not implemented */
> +	if (!map->hdm_decoder.valid || !d)
> +		return -ENODEV;
> +
> +	rc = pci_read_config_word(pdev,
> +				  d + PCI_DVSEC_CXL_CAP, &cap);

Single line.

Why do you want this?  You are using HDM decoders, but checking the
number of ranges. Historical artifact before the HDM Decoder stuff
was added to the spec. That could really do with a rename!

In theory the spec recommends keeping these HDM ranges aligned
with the first 2 HDM decoders, but it doesn't require it and
functionally that doesn't matter so I'd not bother.

> +	if (rc)
> +		return rc;
> +
> +	*count = FIELD_GET(PCI_DVSEC_CXL_HDM_COUNT, cap);
> +	*offset = map->hdm_decoder.offset;
> +	*size = map->hdm_decoder.size;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_get_hdm_reg_info, "CXL");
> +
>  #define CXL_DOE_TABLE_ACCESS_REQ_CODE		0x000000ff
>  #define   CXL_DOE_TABLE_ACCESS_REQ_CODE_READ	0
>  #define CXL_DOE_TABLE_ACCESS_TABLE_TYPE		0x0000ff00
> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
> index 50acbd13bcf8..8456177b523e 100644
> --- a/include/cxl/cxl.h
> +++ b/include/cxl/cxl.h
> @@ -284,4 +284,7 @@ int cxl_dpa_free(struct cxl_endpoint_decoder *cxled);
>  struct cxl_region *cxl_create_region(struct cxl_root_decoder *cxlrd,
>  				     struct cxl_endpoint_decoder **cxled,
>  				     int ways);
> +
> +int cxl_get_hdm_reg_info(struct cxl_dev_state *cxlds, u32 *count,
> +			 resource_size_t *offset, resource_size_t *size);
>  #endif /* __CXL_CXL_H__ */