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

Dan Williams
2025-01-21T22:42:10Z

Alejandro Lucero Palau wrote:
>
> On 1/18/25 01:40, Dan Williams wrote:
> > alejandro.lucero-palau@ wrote:
> >> From: Alejandro Lucero <alucerop@amd.com>
> >>
> >> 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 <alucerop@amd.com>
> >> 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.