Re: [PATCH v9 04/27] cxl/pci: add check for validating capabilities

Alejandro Lucero Palau wrote:

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

quoted

alejandro.lucero-palau@ wrote:

quoted

From: Alejandro Lucero <redacted>

During CXL device initialization supported capabilities by the device
are discovered. Type3 and Type2 devices have different mandatory
capabilities and a Type2 expects a specific set including optional
capabilities.

Add a function for checking expected capabilities against those found
during initialization and allow those mandatory/expected capabilities to
be a subset of the capabilities found.

Rely on this function for validating capabilities instead of when CXL
regs are probed.

Signed-off-by: Alejandro Lucero <redacted>
Reviewed-by: Zhi Wang <zhiw@nvidia.com>
---
  drivers/cxl/core/pci.c  | 16 ++++++++++++++++
  drivers/cxl/core/regs.c |  9 ---------
  drivers/cxl/pci.c       | 24 ++++++++++++++++++++++++
  include/cxl/cxl.h       |  3 +++
  4 files changed, 43 insertions(+), 9 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index ec57caf5b2d7..57318cdc368a 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c

@@ -8,6 +8,7 @@
  #include <linux/pci.h>
  #include <linux/pci-doe.h>
  #include <linux/aer.h>
+#include <cxl/cxl.h>
  #include <cxlpci.h>
  #include <cxlmem.h>
  #include <cxl.h>

@@ -1055,3 +1056,18 @@ int cxl_pci_get_bandwidth(struct pci_dev *pdev, struct access_coordinate *c)
  
  	return 0;
  }
+
+bool cxl_pci_check_caps(struct cxl_dev_state *cxlds, unsigned long *expected_caps,
+			unsigned long *current_caps)
+{
+
+	if (current_caps)
+		bitmap_copy(current_caps, cxlds->capabilities, CXL_MAX_CAPS);
+
+	dev_dbg(cxlds->dev, "Checking cxlds caps 0x%pb vs expected caps 0x%pb\n",
+		cxlds->capabilities, expected_caps);
+
+	/* Checking a minimum of mandatory/expected capabilities */
+	return bitmap_subset(expected_caps, cxlds->capabilities, CXL_MAX_CAPS);
+}
+EXPORT_SYMBOL_NS_GPL(cxl_pci_check_caps, "CXL");

cxl_setup_regs() is already exported from the core. Just make the caller
of cxl_setup_regs() responsible for checking the valid bits per its
constraints rather a new mechanism.

I prefer to keep the regs setup separated from the checks, and I think 
your suggestion involves a higher impact on the current code.

Yes, it moves complexity to the leaf consumers where it belongs.

Note this is the API for accel drivers and by design what the accel 
driver can do with cxl structs is restricted. The patchset adds a new 
function in patch 6 for regs setup  by accel drivers.

"restricted" as in "least privilege design"? There is nothing in 'struct
cxl_reg_map' that needs least privilege restrictions.