| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <64a36a4a39351_8e178294c5@dwillia2-xfh.jf.intel.com.notmuch>
Date: Mon, 3 Jul 2023 17:39:38 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: Terry Bowman <terry.bowman@....com>, <alison.schofield@...el.com>,
<vishal.l.verma@...el.com>, <ira.weiny@...el.com>,
<bwidawsk@...nel.org>, <dan.j.williams@...el.com>,
<dave.jiang@...el.com>, <Jonathan.Cameron@...wei.com>,
<linux-cxl@...r.kernel.org>
CC: <terry.bowman@....com>, <rrichter@....com>,
<linux-kernel@...r.kernel.org>, <bhelgaas@...gle.com>
Subject: RE: [PATCH v8 03/14] cxl/hdm: Use stored Component Register mappings
to map HDM decoder capability
Terry Bowman wrote:
> From: Robert Richter <rrichter@....com>
>
> Now, that the Component Register mappings are stored, use them to
> enable and map the HDM decoder capabilities. The Component Registers
> do not need to be probed again for this, remove probing code.
>
> The HDM capability applies to Endpoints, USPs and VH Host Bridges. The
> Endpoint's component register mappings are located in the cxlds and
> else in the port's structure. Provide a helper function
> cxl_port_get_comp_map() to locate the mappings depending on the
> component's type.
This causes a regression that cxl_test tripped over. It's likely
something you could reproduce without cxl_test just be reloading the
cxl_port driver. Root cause below, but here is what I see on a test run:
# meson test -C build --suite cxl
ninja: no work to do.
ninja: Entering directory `/root/git/ndctl/build'
[113/113] Linking target ndctl/ndctl
1/6 ndctl:cxl / cxl-topology.sh OK 12.78s
2/6 ndctl:cxl / cxl-region-sysfs.sh OK 7.72s
3/6 ndctl:cxl / cxl-labels.sh FAIL 1.53s (exit status 129 or signal 1 SIGHUP)
>>> LD_LIBRARY_PATH=/root/git/ndctl/build/cxl/lib:/root/git/ndctl/build/ndctl/lib:/root/git/ndctl/build/daxctl/lib NDCTL=/root/git/ndctl/build/ndctl/ndctl DAXCTL=/root/git/ndctl/build/daxctl/daxctl MALLOC_PERTURB_=67 TEST_PATH=/root/git/ndctl/build/test DATA_PATH=/root/git/ndctl/test /bin/bash /root/git/ndctl/test/cxl-labels.sh
4/6 ndctl:cxl / cxl-create-region.sh OK 17.05s
5/6 ndctl:cxl / cxl-xor-region.sh OK 5.18s
6/6 ndctl:cxl / cxl-security.sh OK 4.68s
Ok: 5
Expected Fail: 0
Fail: 1
Unexpected Pass: 0
Skipped: 0
Timeout: 0
Full log written to /root/git/ndctl/build/meson-logs/testlog.txt
It's not that cxl-labels.sh causes the error, it is loading and
unloading the port driver trips over the problem below:
>
> Signed-off-by: Robert Richter <rrichter@....com>
> Signed-off-by: Terry Bowman <terry.bowman@....com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> Reviewed-by: Dave Jiang <dave.jiang@...el.com>
> ---
> drivers/cxl/core/hdm.c | 64 +++++++++++++++++++++++-------------------
> 1 file changed, 35 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 4449b34a80cc..b0f59e63e0d2 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -81,26 +81,6 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
> cxlhdm->interleave_mask |= GENMASK(14, 12);
> }
>
> -static int map_hdm_decoder_regs(struct cxl_port *port, void __iomem *crb,
> - struct cxl_component_regs *regs)
> -{
> - struct cxl_register_map map = {
> - .dev = &port->dev,
> - .resource = port->component_reg_phys,
> - .base = crb,
> - .max_size = CXL_COMPONENT_REG_BLOCK_SIZE,
> - };
> -
> - cxl_probe_component_regs(&port->dev, crb, &map.component_map);
> - if (!map.component_map.hdm_decoder.valid) {
> - 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(&map, regs, BIT(CXL_CM_CAP_CAP_ID_HDM));
> -}
> -
> static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
> {
> struct cxl_hdm *cxlhdm;
> @@ -145,6 +125,22 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
> return true;
> }
>
> +static struct cxl_register_map *cxl_port_get_comp_map(struct cxl_port *port)
> +{
> + /*
> + * HDM capability applies to Endpoints, USPs and VH Host
> + * Bridges. The Endpoint's component register mappings are
> + * located in the cxlds.
> + */
> + if (is_cxl_endpoint(port)) {
> + struct cxl_memdev *memdev = to_cxl_memdev(port->uport_dev);
> +
> + return &memdev->cxlds->comp_map;
...but why? The issue here is that the @dev argument in that map is the
memdev parent PCI device. However, in this context the @dev for devm
operations wants to be &port->dev.
> + }
> +
> + return &port->comp_map;
...so this is fine, and folding in the following resolves the test
failure.
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index b0f59e63e0d2..6f111f487795 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -125,22 +125,6 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
return true;
}
-static struct cxl_register_map *cxl_port_get_comp_map(struct cxl_port *port)
-{
- /*
- * HDM capability applies to Endpoints, USPs and VH Host
- * Bridges. The Endpoint's component register mappings are
- * located in the cxlds.
- */
- if (is_cxl_endpoint(port)) {
- struct cxl_memdev *memdev = to_cxl_memdev(port->uport_dev);
-
- return &memdev->cxlds->comp_map;
- }
-
- return &port->comp_map;
-}
-
/**
* devm_cxl_setup_hdm - map HDM decoder component registers
* @port: cxl_port to map
@@ -160,8 +144,7 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port,
cxlhdm->port = port;
dev_set_drvdata(dev, cxlhdm);
- comp_map = cxl_port_get_comp_map(port);
-
+ comp_map = &port->comp_map;
if (comp_map->resource == CXL_RESOURCE_NONE) {
if (info && info->mem_enabled) {
cxlhdm->decoder_count = info->ranges;
Am I missing why the cxlds->comp_map needs to be reused?
Note that I am out and may not be able to respond further until next
week.
Powered by blists - more mailing lists