Re: [PATCH v9 03/27] cxl: add capabilities field to cxl_dev_state and cxl_port

On 1/18/25 01:37, Dan Williams wrote:

alejandro.lucero-palau@ wrote:

quoted

From: Alejandro Lucero <redacted>

Type2 devices have some Type3 functionalities as optional like an mbox
or an hdm decoder, and CXL core needs a way to know what an CXL accelerator
implements.

Add a new field to cxl_dev_state for keeping device capabilities as
discovered during initialization. Add same field to cxl_port as registers
discovery is also used during port initialization.

Signed-off-by: Alejandro Lucero <redacted>
Reviewed-by: Ben Cheatham <redacted>
Reviewed-by: Fan Ni <redacted>
---
  drivers/cxl/core/port.c | 11 +++++++----
  drivers/cxl/core/regs.c | 23 ++++++++++++++++-------
  drivers/cxl/cxl.h       |  9 ++++++---
  drivers/cxl/cxlmem.h    |  2 ++
  drivers/cxl/pci.c       | 10 ++++++----
  include/cxl/cxl.h       | 19 +++++++++++++++++++
  6 files changed, 56 insertions(+), 18 deletions(-)

[..]

quoted

@@ -113,11 +118,12 @@ EXPORT_SYMBOL_NS_GPL(cxl_probe_component_regs, "CXL");
   * @dev: Host device of the @base mapping
   * @base: Mapping of CXL 2.0 8.2.8 CXL Device Register Interface
   * @map: Map object describing the register block information found
+ * @caps: capabilities to be set when discovered
   *
   * Probe for device register information and return it in map object.
   */
  void cxl_probe_device_regs(struct device *dev, void __iomem *base,
-			   struct cxl_device_reg_map *map)
+			   struct cxl_device_reg_map *map, unsigned long *caps)
  {
  	int cap, cap_count;
  	u64 cap_array;

@@ -146,10 +152,12 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base,
  		case CXLDEV_CAP_CAP_ID_DEVICE_STATUS:
  			dev_dbg(dev, "found Status capability (0x%x)\n", offset);
  			rmap = &map->status;
+			set_bit(CXL_DEV_CAP_DEV_STATUS, caps);
  			break;
  		case CXLDEV_CAP_CAP_ID_PRIMARY_MAILBOX:
  			dev_dbg(dev, "found Mailbox capability (0x%x)\n", offset);
  			rmap = &map->mbox;
+			set_bit(CXL_DEV_CAP_MAILBOX_PRIMARY, caps);
  			break;
  		case CXLDEV_CAP_CAP_ID_SECONDARY_MAILBOX:
  			dev_dbg(dev, "found Secondary Mailbox capability (0x%x)\n", offset);

@@ -157,6 +165,7 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base,
  		case CXLDEV_CAP_CAP_ID_MEMDEV:
  			dev_dbg(dev, "found Memory Device capability (0x%x)\n", offset);
  			rmap = &map->memdev;
+			set_bit(CXL_DEV_CAP_MEMDEV, caps);

I do not understand the rationale for a capability bitmap. There is
already a 'valid' flag in 'struct cxl_reg_map' for all register blocks.
Any optional core functionality should key off those existing flags.

The current code is based on Type3 and the registers and capabilities are defined as mandatory, I think except RAS.

With Type2 we have optional capabilities like mailbox and hdm, and the code probing the regs should not make any assumption about what should be there.

With this patchset the capabilities to expect are set by the accel driver and compared with those discovered when probing CXL regs. Although the capabilities check could use the cxl_reg_map, I consider it is convenient to have a capability bitmap for keeping those discovered and easily checking them against those expected by the accel driver, and reporting them (if necessary) as well without further processing.