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

[Dan Williams <dan.j.williams@intel.com>]

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.