Re: [PATCH v18 04/20] cxl: allow Type2 drivers to map cxl component regs

[PJ Waskiewicz <ppwaskie@kernel.org>]

Hi Alejandro,

On Thu, 2025-09-18 at 10:17 +0100, alejandro.lucero-palau@amd.com
wrote:
> From: Alejandro Lucero <alucerop@amd.com>
> 
> Export cxl core functions for a Type2 driver being able to discover
> and
> map the device component registers.
> 
> Use it in sfc driver cxl initialization.
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/cxl/core/port.c            |  1 +
>  drivers/cxl/cxl.h                  |  7 -------
>  drivers/cxl/cxlpci.h               | 12 -----------
>  drivers/net/ethernet/sfc/efx_cxl.c | 33
> ++++++++++++++++++++++++++++++
>  include/cxl/cxl.h                  | 20 ++++++++++++++++++
>  include/cxl/pci.h                  | 15 ++++++++++++++
>  6 files changed, 69 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index bb326dc95d5f..240c3c5bcdc8 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -11,6 +11,7 @@
>  #include <linux/idr.h>
>  #include <linux/node.h>
>  #include <cxl/einj.h>
> +#include <cxl/pci.h>
>  #include <cxlmem.h>
>  #include <cxlpci.h>
>  #include <cxl.h>
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index e197c36c7525..793d4dfe51a2 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -38,10 +38,6 @@ extern const struct nvdimm_security_ops
> *cxl_security_ops;
>  #define   CXL_CM_CAP_HDR_ARRAY_SIZE_MASK GENMASK(31, 24)
>  #define CXL_CM_CAP_PTR_MASK GENMASK(31, 20)
>  
> -#define   CXL_CM_CAP_CAP_ID_RAS 0x2
> -#define   CXL_CM_CAP_CAP_ID_HDM 0x5
> -#define   CXL_CM_CAP_CAP_HDM_VERSION 1
> -
>  /* HDM decoders CXL 2.0 8.2.5.12 CXL HDM Decoder Capability
> Structure */
>  #define CXL_HDM_DECODER_CAP_OFFSET 0x0
>  #define   CXL_HDM_DECODER_COUNT_MASK GENMASK(3, 0)
> @@ -205,9 +201,6 @@ void cxl_probe_component_regs(struct device *dev,
> void __iomem *base,
>  			      struct cxl_component_reg_map *map);
>  void cxl_probe_device_regs(struct device *dev, void __iomem *base,
>  			   struct cxl_device_reg_map *map);
> -int cxl_map_component_regs(const struct cxl_register_map *map,
> -			   struct cxl_component_regs *regs,
> -			   unsigned long map_mask);
>  int cxl_map_device_regs(const struct cxl_register_map *map,
>  			struct cxl_device_regs *regs);
>  int cxl_map_pmu_regs(struct cxl_register_map *map, struct
> cxl_pmu_regs *regs);
> diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
> index 4b11757a46ab..2247823acf6f 100644
> --- a/drivers/cxl/cxlpci.h
> +++ b/drivers/cxl/cxlpci.h
> @@ -13,16 +13,6 @@
>   */
>  #define CXL_PCI_DEFAULT_MAX_VECTORS 16
>  
> -/* Register Block Identifier (RBI) */
> -enum cxl_regloc_type {
> -	CXL_REGLOC_RBI_EMPTY = 0,
> -	CXL_REGLOC_RBI_COMPONENT,
> -	CXL_REGLOC_RBI_VIRT,
> -	CXL_REGLOC_RBI_MEMDEV,
> -	CXL_REGLOC_RBI_PMU,
> -	CXL_REGLOC_RBI_TYPES
> -};
> -
>  /*
>   * Table Access DOE, CDAT Read Entry Response
>   *
> @@ -90,6 +80,4 @@ struct cxl_dev_state;
>  int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm
> *cxlhdm,
>  			struct cxl_endpoint_dvsec_info *info);
>  void read_cdat_data(struct cxl_port *port);
> -int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type
> type,
> -		       struct cxl_register_map *map);
>  #endif /* __CXL_PCI_H__ */
> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c
> b/drivers/net/ethernet/sfc/efx_cxl.c
> index 56d148318636..cdfbe546d8d8 100644
> --- a/drivers/net/ethernet/sfc/efx_cxl.c
> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
> @@ -5,6 +5,7 @@
>   * Copyright (C) 2025, Advanced Micro Devices, Inc.
>   */
>  
> +#include <cxl/cxl.h>
>  #include <cxl/pci.h>
>  #include <linux/pci.h>
>  
> @@ -19,6 +20,7 @@ int efx_cxl_init(struct efx_probe_data *probe_data)
>  	struct pci_dev *pci_dev = efx->pci_dev;
>  	struct efx_cxl *cxl;
>  	u16 dvsec;
> +	int rc;
>  
>  	probe_data->cxl_pio_initialised = false;
>  
> @@ -45,6 +47,37 @@ int efx_cxl_init(struct efx_probe_data
> *probe_data)
>  	if (!cxl)
>  		return -ENOMEM;
>  
> +	rc = cxl_pci_setup_regs(pci_dev, CXL_REGLOC_RBI_COMPONENT,
> +				&cxl->cxlds.reg_map);
> +	if (rc) {
> +		dev_err(&pci_dev->dev, "No component registers
> (err=%d)\n", rc);
> +		return rc;
> +	}
> +
> +	if (!cxl->cxlds.reg_map.component_map.hdm_decoder.valid) {
> +		dev_err(&pci_dev->dev, "Expected HDM component
> register not found\n");
> +		return -ENODEV;
> +	}
> +
> +	if (!cxl->cxlds.reg_map.component_map.ras.valid)
> +		return dev_err_probe(&pci_dev->dev, -ENODEV,
> +				     "Expected RAS component
> register not found\n");
> +
> +	rc = cxl_map_component_regs(&cxl->cxlds.reg_map,
> +				    &cxl->cxlds.regs.component,
> +				    BIT(CXL_CM_CAP_CAP_ID_RAS));
> +	if (rc) {
> +		dev_err(&pci_dev->dev, "Failed to map RAS
> capability.\n");
> +		return rc;
> +	}

I've finally made some serious headway integrating v17 into my
environment to better comment on this flow.

I'm running into what I'm seeing as a fundamental issue of resource
ownership between a device driver, and the CXL driver core.  I'm having
a hard time trying to resolve this.

If I do the above and call cxl_map_component_regs() with a valid CAP_ID
(RAS, HDM, etc.), that eventually calls devm_cxl_iomap_block() from
inside the CXL core drivers.  That calls devm_request_mem_region(), and
this is where things get interesting.

If my device happens to land the CXL component registers inside of a
BAR that has other items needed by my Type 2 device's driver, then we
have a conflict.  My driver and the CXL core drivers cannot hold the
same regions mapped.  i.e. I can't call pci_request_region() on my BAR,
and then call the above.  One loses, and then we all lose.

Curious if you have any ideas how we can improve this?

Cheers,
-PJ