Re: [PATCH v21 08/23] cxl/sfc: Map cxl component regs

[PJ Waskiewicz <ppwaskie@kernel.org>]

On Wed, 2025-11-19 at 19:22 +0000, alejandro.lucero-palau@amd.com
wrote:

Hi Alejandro,

> 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>
> Acked-by: Edward Cree <ecree.xilinx@gmail.com>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> Reviewed-by: Ben Cheatham <benjamin.cheatham@amd.com>
> ---
>  drivers/cxl/core/pci.c             |  1 +
>  drivers/cxl/core/pci_drv.c         |  1 +
>  drivers/cxl/core/port.c            |  1 +
>  drivers/cxl/core/regs.c            |  1 +
>  drivers/cxl/cxl.h                  |  7 ------
>  drivers/cxl/cxlpci.h               | 12 ----------
>  drivers/net/ethernet/sfc/efx_cxl.c | 35
> ++++++++++++++++++++++++++++++
>  include/cxl/cxl.h                  | 19 ++++++++++++++++
>  include/cxl/pci.h                  | 21 ++++++++++++++++++
>  9 files changed, 79 insertions(+), 19 deletions(-)
>  create mode 100644 include/cxl/pci.h
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 566d57ba0579..90a0763e72c4 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -6,6 +6,7 @@
>  #include <linux/delay.h>
>  #include <linux/pci.h>
>  #include <linux/pci-doe.h>
> +#include <cxl/pci.h>
>  #include <linux/aer.h>
>  #include <cxlpci.h>
>  #include <cxlmem.h>
> diff --git a/drivers/cxl/core/pci_drv.c b/drivers/cxl/core/pci_drv.c
> index a35e746e6303..4c767e2471b8 100644
> --- a/drivers/cxl/core/pci_drv.c
> +++ b/drivers/cxl/core/pci_drv.c
> @@ -11,6 +11,7 @@
>  #include <linux/pci.h>
>  #include <linux/aer.h>
>  #include <linux/io.h>
> +#include <cxl/pci.h>
>  #include <cxl/mailbox.h>
>  #include <cxl/cxl.h>
>  #include "cxlmem.h"
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index d19ebf052d76..7c828c75e7b8 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/core/regs.c b/drivers/cxl/core/regs.c
> index fc7fbd4f39d2..dcf444f1fe48 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -4,6 +4,7 @@
>  #include <linux/device.h>
>  #include <linux/slab.h>
>  #include <linux/pci.h>
> +#include <cxl/pci.h>
>  #include <cxlmem.h>
>  #include <cxlpci.h>
>  #include <pmu.h>
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 536c9d99e0e6..d7ddca6f7115 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -39,10 +39,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)
> @@ -206,9 +202,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 24aba9ff6d2e..53760ce31af8 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
>   *
> @@ -100,6 +90,4 @@ static inline void
> cxl_uport_init_ras_reporting(struct cxl_port *port,
>  						struct device *host)
> { }
>  #endif
>  
> -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 8e0481d8dced..34126bc4826c 100644
> --- a/drivers/net/ethernet/sfc/efx_cxl.c
> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
> @@ -7,6 +7,8 @@
>  
>  #include <linux/pci.h>
>  
> +#include <cxl/cxl.h>
> +#include <cxl/pci.h>
>  #include "net_driver.h"
>  #include "efx_cxl.h"
>  
> @@ -18,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;
>  
> @@ -44,6 +47,38 @@ 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) {
> +		pci_err(pci_dev, "No component registers\n");
> +		return rc;
> +	}
> +
> +	if (!cxl->cxlds.reg_map.component_map.hdm_decoder.valid) {
> +		pci_err(pci_dev, "Expected HDM component register
> not found\n");
> +		return -ENODEV;
> +	}
> +
> +	if (!cxl->cxlds.reg_map.component_map.ras.valid) {
> +		pci_err(pci_dev, "Expected RAS component register
> not found\n");
> +		return -ENODEV;
> +	}
> +
> +	rc = cxl_map_component_regs(&cxl->cxlds.reg_map,
> +				    &cxl->cxlds.regs.component,
> +				    BIT(CXL_CM_CAP_CAP_ID_RAS));

I'm going to reiterate a previous concern here with this.  When all of
this was in the CXL core, the CXL core owned whatever BAR these
registers were in in its entirety.  Now with a Type2 device, splitting
this out has implications.

The cxl_map_component_regs() is going to try and map the register map
you request as a reserved resource, which will fail if this Type2
driver has the BAR mapped (which basically all of these drivers do).

I think it's worth either a big comment or something explicit in the
patch description that calls this limitation or restriction out.  
Hardware designers will be caught off-guard if they design their
hardware where the CXL component regs are in a BAR shared by other
register maps in their devices.  If they land the CXL regs in the
middle of that BAR, they will have to do some serious gymnastics in the
drivers to map pieces of their BAR to allow the kernel to map the
component regs.  OR...they can have some breadcrumbs to try and design
the HW where the CXL component regs are at the very beginning or very
end of their BAR.  That way drivers have an easier way to reserve a
subset of a contiguous BAR, and allow the kernel to grab the remainder
for CXL access and management.

I think this is a pretty serious implication that I don't see a way
around.  But letting a HW designer fall into this hole and realize they
can only fix it with a horrible set of driver hacks, or a silicon
respin, really sucks.

Cheers,
-PJ

> +	if (rc) {
> +		pci_err(pci_dev, "Failed to map RAS capability.\n");
> +		return rc;
> +	}
> +
> +	/*
> +	 * Set media ready explicitly as there are neither mailbox
> for checking
> +	 * this state nor the CXL register involved, both not
> mandatory for
> +	 * type2.
> +	 */
> +	cxl->cxlds.media_ready = true;
> +
>  	probe_data->cxl = cxl;
>  
>  	return 0;
> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
> index 13d448686189..7f2e23bce1f7 100644
> --- a/include/cxl/cxl.h
> +++ b/include/cxl/cxl.h
> @@ -70,6 +70,10 @@ struct cxl_regs {
>  	);
>  };
>  
> +#define   CXL_CM_CAP_CAP_ID_RAS 0x2
> +#define   CXL_CM_CAP_CAP_ID_HDM 0x5
> +#define   CXL_CM_CAP_CAP_HDM_VERSION 1
> +
>  struct cxl_reg_map {
>  	bool valid;
>  	int id;
> @@ -223,4 +227,19 @@ struct cxl_dev_state
> *_devm_cxl_dev_state_create(struct device *dev,
>  		(drv_struct *)_devm_cxl_dev_state_create(parent,
> type, serial, dvsec,	\
>  						     
> sizeof(drv_struct), mbox);	\
>  	})
> +
> +/**
> + * cxl_map_component_regs - map cxl component registers
> + *
> + * @map: cxl register map to update with the mappings
> + * @regs: cxl component registers to work with
> + * @map_mask: cxl component regs to map
> + *
> + * Returns integer: success (0) or error (-ENOMEM)
> + *
> + * Made public for Type2 driver support.
> + */
> +int cxl_map_component_regs(const struct cxl_register_map *map,
> +			   struct cxl_component_regs *regs,
> +			   unsigned long map_mask);
>  #endif /* __CXL_CXL_H__ */
> diff --git a/include/cxl/pci.h b/include/cxl/pci.h
> new file mode 100644
> index 000000000000..a172439f08c6
> --- /dev/null
> +++ b/include/cxl/pci.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright(c) 2020 Intel Corporation. All rights reserved. */
> +
> +#ifndef __CXL_CXL_PCI_H__
> +#define __CXL_CXL_PCI_H__
> +
> +/* 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
> +};
> +
> +struct cxl_register_map;
> +
> +int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type
> type,
> +		       struct cxl_register_map *map);
> +#endif