From 7701c8bef4f14bd9f7940c6ed0e6a73584115a96 Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Fri, 14 Apr 2023 11:53:55 -0700 Subject: [PATCH 1/5] cxl/hdm: Fail upon detecting 0-sized decoders Decoders committed with 0-size lead to later crashes on shutdown as __cxl_dpa_release() assumes a 'struct resource' has been established in the in 'cxlds->dpa_res'. Just fail the driver load in this instance since there are deeper problems with the enumeration or the setup when this happens. Fixes: 9c57cde0dcbd ("cxl/hdm: Enumerate allocated DPA") Cc: Reviewed-by: Dave Jiang Reviewed-by: Alison Schofield Link: https://lore.kernel.org/r/168149843516.792294.11872242648319572632.stgit@dwillia2-xfh.jf.intel.com Signed-off-by: Dan Williams --- drivers/cxl/core/hdm.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c index 02cc2c38b44b..35b338b716fe 100644 --- a/drivers/cxl/core/hdm.c +++ b/drivers/cxl/core/hdm.c @@ -269,8 +269,11 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled, lockdep_assert_held_write(&cxl_dpa_rwsem); - if (!len) - goto success; + if (!len) { + dev_warn(dev, "decoder%d.%d: empty reservation attempted\n", + port->id, cxled->cxld.id); + return -EINVAL; + } if (cxled->dpa_res) { dev_dbg(dev, "decoder%d.%d: existing allocation %pr assigned\n", @@ -323,7 +326,6 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled, cxled->mode = CXL_DECODER_MIXED; } -success: port->hdm_end++; get_device(&cxled->cxld.dev); return 0; @@ -833,6 +835,13 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, port->id, cxld->id); return -ENXIO; } + + if (size == 0) { + dev_warn(&port->dev, + "decoder%d.%d: Committed with zero size\n", + port->id, cxld->id); + return -ENXIO; + } port->commit_end = cxld->id; } else { /* unless / until type-2 drivers arrive, assume type-3 */ From 1423885c84a5b3a53b79bcf241b18124d0d7cba6 Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Fri, 14 Apr 2023 11:54:00 -0700 Subject: [PATCH 2/5] cxl/hdm: Use 4-byte reads to retrieve HDM decoder base+limit The CXL specification mandates that 4-byte registers must be accessed with 4-byte access cycles. CXL 3.0 8.2.3 "Component Register Layout and Definition" states that the behavior is undefined if (2) 32-bit registers are accessed as an 8-byte quantity. It turns out that at least one hardware implementation is sensitive to this in practice. The @size variable results in zero with: size = readq(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which)); ...and the correct size with: lo = readl(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which)); hi = readl(hdm + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(which)); size = (hi << 32) + lo; Fixes: d17d0540a0db ("cxl/core/hdm: Add CXL standard decoder enumeration to the core") Cc: Reviewed-by: Dave Jiang Reviewed-by: Alison Schofield Link: https://lore.kernel.org/r/168149844056.792294.8224490474529733736.stgit@dwillia2-xfh.jf.intel.com Signed-off-by: Dan Williams --- drivers/cxl/core/hdm.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c index 35b338b716fe..6fdf7981ddc7 100644 --- a/drivers/cxl/core/hdm.c +++ b/drivers/cxl/core/hdm.c @@ -1,6 +1,5 @@ // SPDX-License-Identifier: GPL-2.0-only /* Copyright(c) 2022 Intel Corporation. All rights reserved. */ -#include #include #include #include @@ -785,8 +784,8 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, int *target_map, void __iomem *hdm, int which, u64 *dpa_base, struct cxl_endpoint_dvsec_info *info) { + u64 size, base, skip, dpa_size, lo, hi; struct cxl_endpoint_decoder *cxled; - u64 size, base, skip, dpa_size; bool committed; u32 remainder; int i, rc; @@ -801,8 +800,12 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, which, info); ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(which)); - base = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which)); - size = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which)); + lo = readl(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which)); + hi = readl(hdm + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(which)); + base = (hi << 32) + lo; + lo = readl(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which)); + hi = readl(hdm + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(which)); + size = (hi << 32) + lo; committed = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED); cxld->commit = cxl_decoder_commit; cxld->reset = cxl_decoder_reset; @@ -865,8 +868,9 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, return rc; if (!info) { - target_list.value = - ioread64_hi_lo(hdm + CXL_HDM_DECODER0_TL_LOW(which)); + lo = readl(hdm + CXL_HDM_DECODER0_TL_LOW(which)); + hi = readl(hdm + CXL_HDM_DECODER0_TL_HIGH(which)); + target_list.value = (hi << 32) + lo; for (i = 0; i < cxld->interleave_ways; i++) target_map[i] = target_list.target_id[i]; @@ -883,7 +887,9 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, port->id, cxld->id, size, cxld->interleave_ways); return -ENXIO; } - skip = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SKIP_LOW(which)); + lo = readl(hdm + CXL_HDM_DECODER0_SKIP_LOW(which)); + hi = readl(hdm + CXL_HDM_DECODER0_SKIP_HIGH(which)); + skip = (hi << 32) + lo; cxled = to_cxl_endpoint_decoder(&cxld->dev); rc = devm_cxl_dpa_reserve(cxled, *dpa_base + skip, dpa_size, skip); if (rc) { From 104087a8aaf0f46d89376917eca977fad972cc93 Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Fri, 14 Apr 2023 11:54:06 -0700 Subject: [PATCH 3/5] cxl/core: Drop unused io-64-nonatomic-lo-hi.h After the discovery of a case where an implementation misbehaves with register reads larger than the definition of the register the other usages of readq() were audited and found to be correct, but some cases where the io-64-nonatomic-lo-hi.h include is not needed were discovered, delete them. Reviewed-by: Dave Jiang Reviewed-by: Alison Schofield Link: https://lore.kernel.org/r/168149844596.792294.8273108394688012953.stgit@dwillia2-xfh.jf.intel.com Signed-off-by: Dan Williams --- drivers/cxl/core/mbox.c | 1 - drivers/cxl/core/port.c | 1 - 2 files changed, 2 deletions(-) diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index f2addb457172..6198637cb0bb 100644 --- a/drivers/cxl/core/mbox.c +++ b/drivers/cxl/core/mbox.c @@ -1,6 +1,5 @@ // SPDX-License-Identifier: GPL-2.0-only /* Copyright(c) 2020 Intel Corporation. All rights reserved. */ -#include #include #include #include diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c index 4d1f9c5b5029..b060757c4000 100644 --- a/drivers/cxl/core/port.c +++ b/drivers/cxl/core/port.c @@ -1,6 +1,5 @@ // SPDX-License-Identifier: GPL-2.0-only /* Copyright(c) 2020 Intel Corporation. All rights reserved. */ -#include #include #include #include From 7bba261e0aa6e8e5f28a3a3def8338b6512534ee Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Fri, 14 Apr 2023 11:54:11 -0700 Subject: [PATCH 4/5] cxl/port: Scan single-target ports for decoders Do not assume that a single-target port falls back to a passthrough decoder configuration. Scan for decoders and only fallback after probing that the HDM decoder capability is not present. One user visible affect of this bug is the inability to enumerate present CXL regions as the decoder settings for the present decoders are skipped. Fixes: d17d0540a0db ("cxl/core/hdm: Add CXL standard decoder enumeration to the core") Reported-by: Jonathan Cameron Link: http://lore.kernel.org/r/20230227153128.8164-1-Jonathan.Cameron@huawei.com Cc: Reviewed-by: Jonathan Cameron Reviewed-by: Dave Jiang Reviewed-by: Alison Schofield Link: https://lore.kernel.org/r/168149845130.792294.3210421233937427962.stgit@dwillia2-xfh.jf.intel.com Signed-off-by: Dan Williams --- drivers/cxl/core/hdm.c | 5 +++-- drivers/cxl/port.c | 20 ++++++++++++++------ 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c index 6fdf7981ddc7..abe3877cfa63 100644 --- a/drivers/cxl/core/hdm.c +++ b/drivers/cxl/core/hdm.c @@ -92,8 +92,9 @@ static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb, cxl_probe_component_regs(&port->dev, crb, &map.component_map); if (!map.component_map.hdm_decoder.valid) { - dev_err(&port->dev, "HDM decoder registers invalid\n"); - return -ENXIO; + dev_dbg(&port->dev, "HDM decoder registers not implemented\n"); + /* unique error code to indicate no HDM decoder capability */ + return -ENODEV; } return cxl_map_component_regs(&port->dev, regs, &map, diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c index 22a7ab2bae7c..eb57324c4ad4 100644 --- a/drivers/cxl/port.c +++ b/drivers/cxl/port.c @@ -66,14 +66,22 @@ static int cxl_switch_port_probe(struct cxl_port *port) if (rc < 0) return rc; - if (rc == 1) - return devm_cxl_add_passthrough_decoder(port); - cxlhdm = devm_cxl_setup_hdm(port, NULL); - if (IS_ERR(cxlhdm)) - return PTR_ERR(cxlhdm); + if (!IS_ERR(cxlhdm)) + return devm_cxl_enumerate_decoders(cxlhdm, NULL); - return devm_cxl_enumerate_decoders(cxlhdm, NULL); + if (PTR_ERR(cxlhdm) != -ENODEV) { + dev_err(&port->dev, "Failed to map HDM decoder capability\n"); + return PTR_ERR(cxlhdm); + } + + if (rc == 1) { + dev_dbg(&port->dev, "Fallback to passthrough decoder\n"); + return devm_cxl_add_passthrough_decoder(port); + } + + dev_err(&port->dev, "HDM decoder capability not found\n"); + return -ENXIO; } static int cxl_endpoint_port_probe(struct cxl_port *port) From c841ecd8277154c9297dd9ac959494f6deb61e76 Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Fri, 14 Apr 2023 11:54:16 -0700 Subject: [PATCH 5/5] cxl/hdm: Add more HDM decoder debug messages at startup A recent debug session yielded a couple debug messages that were useful for determining the reason why the driver was or was not falling back to CXL range register emulation, and for identifying decoder setting enumeration problems. Reviewed-by: Dave Jiang Reviewed-by: Alison Schofield Link: https://lore.kernel.org/r/168149845668.792294.11814353796371419167.stgit@dwillia2-xfh.jf.intel.com Signed-off-by: Dan Williams --- drivers/cxl/core/hdm.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c index abe3877cfa63..7889ff203a34 100644 --- a/drivers/cxl/core/hdm.c +++ b/drivers/cxl/core/hdm.c @@ -130,6 +130,14 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info) */ for (i = 0; i < cxlhdm->decoder_count; i++) { ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(i)); + dev_dbg(&info->port->dev, + "decoder%d.%d: committed: %ld base: %#x_%.8x size: %#x_%.8x\n", + info->port->id, i, + FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl), + readl(hdm + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(i)), + readl(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(i)), + readl(hdm + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(i)), + readl(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(i))); if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl)) return false; } @@ -868,6 +876,10 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld, if (rc) return rc; + dev_dbg(&port->dev, "decoder%d.%d: range: %#llx-%#llx iw: %d ig: %d\n", + port->id, cxld->id, cxld->hpa_range.start, cxld->hpa_range.end, + cxld->interleave_ways, cxld->interleave_granularity); + if (!info) { lo = readl(hdm + CXL_HDM_DECODER0_TL_LOW(which)); hi = readl(hdm + CXL_HDM_DECODER0_TL_HIGH(which));