| Message ID | 20220621201259.1547474-1-dave@stgolabs.net |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cxl/acpi: Verify CHBS consistency | expand |
Davidlohr Bueso wrote: > Similarly to verifying the cfwms, have a cxl_acpi_chbs_verify(), > as described by the CXL T3 Memory Device Software Guide > for CXL 2.0 platforms. > > Also while at it, tuck the rc check for nvdimm bridge into > the pmem branch. > > Signed-off-by: Davidlohr Bueso <dave@stgolabs.net> > --- > > drivers/cxl/acpi.c | 64 +++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 61 insertions(+), 3 deletions(-) > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > index 40286f5df812..33b5f362c9f1 100644 > --- a/drivers/cxl/acpi.c > +++ b/drivers/cxl/acpi.c > @@ -187,14 +187,65 @@ static int add_host_bridge_uport(struct device *match, void *arg) > struct cxl_chbs_context { > struct device *dev; > unsigned long long uid; > + struct cxl_port *root_port; > resource_size_t chbcr; > }; > > +static inline bool range_overlaps(struct range *r1, struct range *r2) > +{ > + return r1->start <= r2->end && r2->start <= r1->end; > +} > + > +static int cxl_acpi_chbs_verify(struct cxl_chbs_context *cxt, > + struct acpi_cedt_chbs *chbs) > +{ > + struct device *dev = cxt->dev; > + struct cxl_dport *dport; > + struct cxl_port *root_port = cxt->root_port; > + struct range chbs_range = { > + .start = chbs->base, > + .end = chbs->base + chbs->length - 1, > + }; > + > + if (chbs->cxl_version > 1) { > + dev_err(dev, "CHBS Unsupported CXL Version\n"); > + return -EINVAL; > + } > + > + if (chbs->cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11) > + return 0; > + > + if (chbs->cxl_version == ACPI_CEDT_CHBS_VERSION_CXL20 && > + (chbs->length != ACPI_CEDT_CHBS_LENGTH_CXL20)) { > + dev_err(dev, "Platform does not support CXL 2.0\n"); > + return -EINVAL; > + } > + > + device_lock(&root_port->dev); > + list_for_each_entry(dport, &root_port->dports, list) { > + struct range dport_range = { > + .start = dport->component_reg_phys, > + .end = dport->component_reg_phys + > + CXL_COMPONENT_REG_BLOCK_SIZE - 1, > + }; > + > + if (range_overlaps(&chbs_range, &dport_range)) { > + device_unlock(&root_port->dev); > + dev_err(dev, "CHBS overlapping Base and Length pair\n"); > + return -EINVAL; > + } For cxl_port objects this happens "for free" as a side effect of the: crb = devm_cxl_iomap_block(dev, port->component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE); ...call in devm_cxl_setup_hdm(), where it tries to exclusively claim the component register block for that cxl_port driver instance. I.e. if the CHBS provides overlapping / duplicated ranges the failure is localized to the cxl_port_probe() event for that port, and can be debugged further by disabling one of the conflicts. > + } > + device_unlock(&root_port->dev); > + > + return 0; > +} > + > static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg, > const unsigned long end) > { > struct cxl_chbs_context *ctx = arg; > struct acpi_cedt_chbs *chbs; > + int ret; > > if (ctx->chbcr) > return 0; > @@ -203,6 +254,11 @@ static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg, > > if (ctx->uid != chbs->uid) > return 0; > + > + ret = cxl_acpi_chbs_verify(ctx, chbs); > + if (ret) > + return ret; > + > ctx->chbcr = chbs->base; > > return 0; > @@ -232,6 +288,7 @@ static int add_host_bridge_dport(struct device *match, void *arg) > ctx = (struct cxl_chbs_context) { > .dev = host, > .uid = uid, > + .root_port = root_port, > }; > acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbcr, &ctx); > > @@ -321,11 +378,12 @@ static int cxl_acpi_probe(struct platform_device *pdev) > if (rc < 0) > return rc; > > - if (IS_ENABLED(CONFIG_CXL_PMEM)) > + if (IS_ENABLED(CONFIG_CXL_PMEM)) { > rc = device_for_each_child(&root_port->dev, root_port, > add_root_nvdimm_bridge); > - if (rc < 0) > - return rc; > + if (rc < 0) > + return rc; > + } No need to move this inside the "if (IS_ENABLED(CONFIG_CXL_PMEM))" that I can see.
On Tue, 21 Jun 2022, Dan Williams wrote: >For cxl_port objects this happens "for free" as a side effect of the: > > crb = devm_cxl_iomap_block(dev, port->component_reg_phys, > CXL_COMPONENT_REG_BLOCK_SIZE); > >...call in devm_cxl_setup_hdm(), where it tries to exclusively claim the >component register block for that cxl_port driver instance. Fair enough, I had noticed this. > >I.e. if the CHBS provides overlapping / duplicated ranges the failure is >localized to the cxl_port_probe() event for that port, and can be >debugged further by disabling one of the conflicts. Ok. Although imo it does make sense for failing directly in the cxl_acpi driver at an early stage instead of bogusly passing it down the hierarchy. So is a v2 still worth it without this check? Thanks, Davidlohr
Davidlohr Bueso wrote: > On Tue, 21 Jun 2022, Dan Williams wrote: > > >For cxl_port objects this happens "for free" as a side effect of the: > > > > crb = devm_cxl_iomap_block(dev, port->component_reg_phys, > > CXL_COMPONENT_REG_BLOCK_SIZE); > > > >...call in devm_cxl_setup_hdm(), where it tries to exclusively claim the > >component register block for that cxl_port driver instance. > > Fair enough, I had noticed this. > > > > >I.e. if the CHBS provides overlapping / duplicated ranges the failure is > >localized to the cxl_port_probe() event for that port, and can be > >debugged further by disabling one of the conflicts. > > Ok. Although imo it does make sense for failing directly in the cxl_acpi > driver at an early stage instead of bogusly passing it down the hierarchy. You could make that argument for almost any resource range advertised to the kernel. The expected place where they finally collide is at request_region() time. > So is a v2 still worth it without this check? I do notice that you also added a CXL 1.1 version check. That seems useful to break out, but probably needs a rationale for what that means for CXL 2.0 device on CXL 1.1 host compatibility. So a patch per new CHBS consistency check is my preference, but I think only the CXL 1.1 check is still open.
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c index 40286f5df812..33b5f362c9f1 100644 --- a/drivers/cxl/acpi.c +++ b/drivers/cxl/acpi.c @@ -187,14 +187,65 @@ static int add_host_bridge_uport(struct device *match, void *arg) struct cxl_chbs_context { struct device *dev; unsigned long long uid; + struct cxl_port *root_port; resource_size_t chbcr; }; +static inline bool range_overlaps(struct range *r1, struct range *r2) +{ + return r1->start <= r2->end && r2->start <= r1->end; +} + +static int cxl_acpi_chbs_verify(struct cxl_chbs_context *cxt, + struct acpi_cedt_chbs *chbs) +{ + struct device *dev = cxt->dev; + struct cxl_dport *dport; + struct cxl_port *root_port = cxt->root_port; + struct range chbs_range = { + .start = chbs->base, + .end = chbs->base + chbs->length - 1, + }; + + if (chbs->cxl_version > 1) { + dev_err(dev, "CHBS Unsupported CXL Version\n"); + return -EINVAL; + } + + if (chbs->cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11) + return 0; + + if (chbs->cxl_version == ACPI_CEDT_CHBS_VERSION_CXL20 && + (chbs->length != ACPI_CEDT_CHBS_LENGTH_CXL20)) { + dev_err(dev, "Platform does not support CXL 2.0\n"); + return -EINVAL; + } + + device_lock(&root_port->dev); + list_for_each_entry(dport, &root_port->dports, list) { + struct range dport_range = { + .start = dport->component_reg_phys, + .end = dport->component_reg_phys + + CXL_COMPONENT_REG_BLOCK_SIZE - 1, + }; + + if (range_overlaps(&chbs_range, &dport_range)) { + device_unlock(&root_port->dev); + dev_err(dev, "CHBS overlapping Base and Length pair\n"); + return -EINVAL; + } + } + device_unlock(&root_port->dev); + + return 0; +} + static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg, const unsigned long end) { struct cxl_chbs_context *ctx = arg; struct acpi_cedt_chbs *chbs; + int ret; if (ctx->chbcr) return 0; @@ -203,6 +254,11 @@ static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg, if (ctx->uid != chbs->uid) return 0; + + ret = cxl_acpi_chbs_verify(ctx, chbs); + if (ret) + return ret; + ctx->chbcr = chbs->base; return 0; @@ -232,6 +288,7 @@ static int add_host_bridge_dport(struct device *match, void *arg) ctx = (struct cxl_chbs_context) { .dev = host, .uid = uid, + .root_port = root_port, }; acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbcr, &ctx); @@ -321,11 +378,12 @@ static int cxl_acpi_probe(struct platform_device *pdev) if (rc < 0) return rc; - if (IS_ENABLED(CONFIG_CXL_PMEM)) + if (IS_ENABLED(CONFIG_CXL_PMEM)) { rc = device_for_each_child(&root_port->dev, root_port, add_root_nvdimm_bridge); - if (rc < 0) - return rc; + if (rc < 0) + return rc; + } /* In case PCI is scanned before ACPI re-trigger memdev attach */ return cxl_bus_rescan();
Similarly to verifying the cfwms, have a cxl_acpi_chbs_verify(), as described by the CXL T3 Memory Device Software Guide for CXL 2.0 platforms. Also while at it, tuck the rc check for nvdimm bridge into the pmem branch. Signed-off-by: Davidlohr Bueso <dave@stgolabs.net> --- drivers/cxl/acpi.c | 64 +++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 61 insertions(+), 3 deletions(-)