| Message ID | 20241230214445.27602-5-alejandro.lucero-palau@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | cxl: add type2 device basic support | expand |
On Mon, 30 Dec 2024 21:44:22 +0000 alejandro.lucero-palau@amd.com 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> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
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.
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. 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.
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.
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"); diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c index 144ae9eb6253..6432a784f08b 100644 --- a/drivers/cxl/core/regs.c +++ b/drivers/cxl/core/regs.c @@ -446,15 +446,6 @@ static int cxl_probe_regs(struct cxl_register_map *map, unsigned long *caps) case CXL_REGLOC_RBI_MEMDEV: dev_map = &map->device_map; cxl_probe_device_regs(host, base, dev_map, caps); - if (!dev_map->status.valid || !dev_map->mbox.valid || - !dev_map->memdev.valid) { - dev_err(host, "registers not found: %s%s%s\n", - !dev_map->status.valid ? "status " : "", - !dev_map->mbox.valid ? "mbox " : "", - !dev_map->memdev.valid ? "memdev " : ""); - return -ENXIO; - } - dev_dbg(host, "Probing device registers...\n"); break; default: diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index dbc1cd9bec09..9e790382496a 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -903,6 +903,8 @@ __ATTRIBUTE_GROUPS(cxl_rcd); static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) { struct pci_host_bridge *host_bridge = pci_find_host_bridge(pdev->bus); + DECLARE_BITMAP(expected, CXL_MAX_CAPS); + DECLARE_BITMAP(found, CXL_MAX_CAPS); struct cxl_memdev_state *mds; struct cxl_dev_state *cxlds; struct cxl_register_map map; @@ -964,6 +966,28 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) if (rc) dev_dbg(&pdev->dev, "Failed to map RAS capability.\n"); + bitmap_clear(expected, 0, CXL_MAX_CAPS); + + /* + * These are the mandatory capabilities for a Type3 device. + * Only checking capabilities used by current Linux drivers. + */ + set_bit(CXL_DEV_CAP_HDM, expected); + set_bit(CXL_DEV_CAP_DEV_STATUS, expected); + set_bit(CXL_DEV_CAP_MAILBOX_PRIMARY, expected); + set_bit(CXL_DEV_CAP_MEMDEV, expected); + + /* + * Checking mandatory caps are there as, at least, a subset of those + * found. + */ + if (!cxl_pci_check_caps(cxlds, expected, found)) { + dev_err(&pdev->dev, + "Expected mandatory capabilities not found: (%pb - %pb)\n", + expected, found); + return -ENXIO; + } + rc = cxl_pci_type3_init_mailbox(cxlds); if (rc) return rc; diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h index c5e4b6233baa..464e5fb006ba 100644 --- a/include/cxl/cxl.h +++ b/include/cxl/cxl.h @@ -40,4 +40,7 @@ void cxl_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec); void cxl_set_serial(struct cxl_dev_state *cxlds, u64 serial); int cxl_set_resource(struct cxl_dev_state *cxlds, struct resource res, enum cxl_resource); +bool cxl_pci_check_caps(struct cxl_dev_state *cxlds, + unsigned long *expected_caps, + unsigned long *current_caps); #endif