From: Dan Williams <dan.j.williams@intel.com>
To: Ben Widawsky <ben.widawsky@intel.com>
Cc: linux-cxl@vger.kernel.org, patches@lists.linux.dev,
Alison Schofield <alison.schofield@intel.com>,
Ira Weiny <ira.weiny@intel.com>,
Jonathan Cameron <Jonathan.Cameron@huawei.com>,
Vishal Verma <vishal.l.verma@intel.com>,
Bjorn Helgaas <helgaas@kernel.org>,
Linux NVDIMM <nvdimm@lists.linux.dev>,
Linux PCI <linux-pci@vger.kernel.org>
Subject: Re: [PATCH v3 08/14] cxl/region: HB port config verification
Date: Fri, 18 Feb 2022 13:04:07 -0800 [thread overview]
Message-ID: <CAPcyv4i68KM52wXbeO_y9VHpbD_KQN1oZSZmps8dYYMuNYUc6w@mail.gmail.com> (raw)
In-Reply-To: <20220128002707.391076-9-ben.widawsky@intel.com>
On Thu, Jan 27, 2022 at 4:27 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> Host bridge root port verification determines if the device ordering in
> an interleave set can be programmed through the host bridges and
> switches.
>
> The algorithm implemented here is based on the CXL Type 3 Memory Device
> Software Guide, chapter 2.13.15. The current version of the guide does
> not yet support x3 interleave configurations, and so that's not
> supported here either.
Given x3 is in a released ECN lets go ahead and include it because it
may have a material effect on the design, but more importantly the
ABI.
>
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---
> .clang-format | 1 +
> drivers/cxl/core/port.c | 1 +
> drivers/cxl/cxl.h | 2 +
> drivers/cxl/region.c | 127 +++++++++++++++++++++++++++++++++++++++-
> 4 files changed, 130 insertions(+), 1 deletion(-)
>
> diff --git a/.clang-format b/.clang-format
> index 1221d53be90b..5e20206f905e 100644
> --- a/.clang-format
> +++ b/.clang-format
> @@ -171,6 +171,7 @@ ForEachMacros:
> - 'for_each_cpu_wrap'
> - 'for_each_cxl_decoder_target'
> - 'for_each_cxl_endpoint'
> + - 'for_each_cxl_endpoint_hb'
> - 'for_each_dapm_widgets'
> - 'for_each_dev_addr'
> - 'for_each_dev_scope'
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 0847e6ce19ef..1d81c5f56a3e 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -706,6 +706,7 @@ struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
> return ERR_PTR(-ENOMEM);
>
> INIT_LIST_HEAD(&dport->list);
> + INIT_LIST_HEAD(&dport->verify_link);
> dport->dport = dport_dev;
> dport->port_id = port_id;
> dport->component_reg_phys = component_reg_phys;
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index a291999431c7..ed984465b59c 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -350,6 +350,7 @@ struct cxl_port {
> * @component_reg_phys: downstream port component registers
> * @port: reference to cxl_port that contains this downstream port
> * @list: node for a cxl_port's list of cxl_dport instances
> + * @verify_link: node used for hb root port verification
> */
> struct cxl_dport {
> struct device *dport;
> @@ -357,6 +358,7 @@ struct cxl_dport {
> resource_size_t component_reg_phys;
> struct cxl_port *port;
> struct list_head list;
> + struct list_head verify_link;
Is this list also protected by the port device_lock?
> };
>
> /**
> diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
> index 562c8720da56..d2f6c990c8a8 100644
> --- a/drivers/cxl/region.c
> +++ b/drivers/cxl/region.c
> @@ -4,6 +4,7 @@
> #include <linux/genalloc.h>
> #include <linux/device.h>
> #include <linux/module.h>
> +#include <linux/sort.h>
> #include <linux/pci.h>
> #include "cxlmem.h"
> #include "region.h"
> @@ -36,6 +37,12 @@
> for (idx = 0, ep = (region)->config.targets[idx]; \
> idx < region_ways(region); ep = (region)->config.targets[++idx])
>
> +#define for_each_cxl_endpoint_hb(ep, region, hb, idx) \
> + for (idx = 0, (ep) = (region)->config.targets[idx]; \
> + idx < region_ways(region); \
> + idx++, (ep) = (region)->config.targets[idx]) \
> + if (get_hostbridge(ep) == (hb))
> +
> #define for_each_cxl_decoder_target(dport, decoder, idx) \
> for (idx = 0, dport = (decoder)->target[idx]; \
> idx < (decoder)->nr_targets - 1; \
> @@ -299,6 +306,59 @@ static bool region_xhb_config_valid(const struct cxl_region *cxlr,
> return true;
> }
>
> +static struct cxl_dport *get_rp(struct cxl_memdev *ep)
> +{
> + struct cxl_port *port, *parent_port = port = ep->port;
> + struct cxl_dport *dport;
> +
> + while (!is_cxl_root(port)) {
> + parent_port = to_cxl_port(port->dev.parent);
> + if (parent_port->depth == 1)
> + list_for_each_entry(dport, &parent_port->dports, list)
Locking?
> + if (dport->dport == port->uport->parent->parent)
This assumes no switches.
Effectively it is identical to what devm_cxl_enumerate_ports(), which
does support switches, is doing. To reduce maintenance burden it could
follow the same pattern of:
for (iter = dev; iter; iter = grandparent(iter))
...
if (dev_is_cxl_root_child(&port->dev))
...
> + return dport;
> + port = parent_port;
> + }
> +
> + BUG();
more kernel crashing... why?
> + return NULL;
> +}
> +
> +static int get_num_root_ports(const struct cxl_region *cxlr)
> +{
> + struct cxl_memdev *endpoint;
> + struct cxl_dport *dport, *tmp;
> + int num_root_ports = 0;
> + LIST_HEAD(root_ports);
> + int idx;
> +
> + for_each_cxl_endpoint(endpoint, cxlr, idx) {
> + struct cxl_dport *root_port = get_rp(endpoint);
> +
> + if (list_empty(&root_port->verify_link)) {
> + list_add_tail(&root_port->verify_link, &root_ports);
Doesn't this run into problems when there are multiple regions per root port?
> + num_root_ports++;
> + }
> + }
> +
> + list_for_each_entry_safe(dport, tmp, &root_ports, verify_link)
> + list_del_init(&dport->verify_link);
> +
> + return num_root_ports;
> +}
> +
> +static bool has_switch(const struct cxl_region *cxlr)
> +{
> + struct cxl_memdev *ep;
> + int i;
> +
> + for_each_cxl_endpoint(ep, cxlr, i)
> + if (ep->port->depth > 2)
> + return true;
> +
> + return false;
> +}
> +
> /**
> * region_hb_rp_config_valid() - determine root port ordering is correct
> * @cxlr: Region to validate
> @@ -312,7 +372,72 @@ static bool region_xhb_config_valid(const struct cxl_region *cxlr,
> static bool region_hb_rp_config_valid(const struct cxl_region *cxlr,
> const struct cxl_decoder *rootd)
> {
> - /* TODO: */
> + const int num_root_ports = get_num_root_ports(cxlr);
> + struct cxl_port *hbs[CXL_DECODER_MAX_INTERLEAVE];
In terms of stack usage, doesn't the caller also have one of these on
the stack when this is called?
> + int hb_count, i;
> +
> + hb_count = get_unique_hostbridges(cxlr, hbs);
> +
> + /* TODO: Switch support */
> + if (has_switch(cxlr))
> + return false;
> +
> + /*
> + * Are all devices in this region on the same CXL Host Bridge
> + * Root Port?
> + */
> + if (num_root_ports == 1 && !has_switch(cxlr))
> + return true;
How can this happen without any intervening switch?
> +
> + for (i = 0; i < hb_count; i++) {
> + int idx, position_mask;
> + struct cxl_dport *rp;
> + struct cxl_port *hb;
> +
> + /* Get next CXL Host Bridge this region spans */
> + hb = hbs[i];
> +
> + /*
> + * Calculate the position mask: NumRootPorts = 2^PositionMask
> + * for this region.
> + *
> + * XXX: pos_mask is actually (1 << PositionMask) - 1
> + */
> + position_mask = (1 << (ilog2(num_root_ports))) - 1;
Isn't "1 << ilog2(num_root_ports)" just "num_root_ports"?
> +
> + /*
> + * Calculate the PortGrouping for each device on this CXL Host
> + * Bridge Root Port:
> + * PortGrouping = RegionLabel.Position & PositionMask
Still confused what a port grouping is and what it means for the
algorithm especially since RegionLabels are not relevant to this part
of the algorithm. This assumes someone is familiar with "guide"
terminology?
> + *
> + * The following nest iterators effectively iterate over each
> + * root port in the region.
> + * for_each_unique_rootport(rp, cxlr)
> + */
> + list_for_each_entry(rp, &hb->dports, list) {
> + struct cxl_memdev *ep;
> + int port_grouping = -1;
> +
> + for_each_cxl_endpoint_hb(ep, cxlr, hb, idx) {
> + if (get_rp(ep) != rp)
> + continue;
> +
> + if (port_grouping == -1)
> + port_grouping = idx & position_mask;
> +
> + /*
> + * Do all devices in the region connected to this CXL
> + * Host Bridge Root Port have the same PortGrouping?
> + */
> + if ((idx & position_mask) != port_grouping) {
> + dev_dbg(&cxlr->dev,
> + "One or more devices are not connected to the correct Host Bridge Root Port\n");
> + return false;
> + }
> + }
> + }
> + }
> +
> return true;
> }
>
> --
> 2.35.0
>
next prev parent reply other threads:[~2022-02-18 21:04 UTC|newest]
Thread overview: 70+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-28 0:26 [PATCH v3 00/14] CXL Region driver Ben Widawsky
2022-01-28 0:26 ` [PATCH v3 01/14] cxl/region: Add region creation ABI Ben Widawsky
2022-01-28 18:14 ` Dan Williams
2022-01-28 18:59 ` Dan Williams
2022-02-02 18:26 ` Ben Widawsky
2022-02-02 18:28 ` Ben Widawsky
2022-02-02 18:48 ` Ben Widawsky
2022-02-02 19:00 ` Dan Williams
2022-02-02 19:02 ` Ben Widawsky
2022-02-02 19:15 ` Dan Williams
2022-02-01 22:42 ` Ben Widawsky
2022-02-01 15:53 ` Jonathan Cameron
2022-02-17 17:10 ` [PATCH v4 " Ben Widawsky
2022-02-17 17:19 ` [PATCH v5 01/15] " Ben Widawsky
2022-02-17 17:33 ` Ben Widawsky
2022-02-17 17:58 ` Dan Williams
2022-02-17 18:58 ` Ben Widawsky
2022-02-17 20:26 ` Dan Williams
2022-02-17 22:22 ` Ben Widawsky
2022-02-17 23:32 ` Dan Williams
2022-02-18 16:41 ` Ben Widawsky
2022-01-28 0:26 ` [PATCH v3 02/14] cxl/region: Introduce concept of region configuration Ben Widawsky
2022-01-29 0:25 ` Dan Williams
2022-02-01 14:59 ` Ben Widawsky
2022-02-03 5:06 ` Dan Williams
2022-02-01 23:11 ` Ben Widawsky
2022-02-03 17:48 ` Dan Williams
2022-02-03 22:23 ` Ben Widawsky
2022-02-03 23:27 ` Dan Williams
2022-02-04 0:19 ` Ben Widawsky
2022-02-04 2:45 ` Dan Williams
2022-02-17 18:36 ` Ben Widawsky
2022-02-17 19:57 ` Dan Williams
2022-02-17 20:20 ` Ben Widawsky
2022-02-17 21:12 ` Dan Williams
2022-02-23 21:49 ` Ben Widawsky
2022-02-23 22:24 ` Dan Williams
2022-02-23 22:31 ` Ben Widawsky
2022-02-23 22:42 ` Dan Williams
2022-01-28 0:26 ` [PATCH v3 03/14] cxl/mem: Cache port created by the mem dev Ben Widawsky
2022-02-17 1:20 ` Dan Williams
2022-01-28 0:26 ` [PATCH v3 04/14] cxl/region: Introduce a cxl_region driver Ben Widawsky
2022-02-01 16:21 ` Jonathan Cameron
2022-02-17 6:04 ` Dan Williams
2022-01-28 0:26 ` [PATCH v3 05/14] cxl/acpi: Handle address space allocation Ben Widawsky
2022-02-18 19:17 ` Dan Williams
2022-01-28 0:26 ` [PATCH v3 06/14] cxl/region: Address " Ben Widawsky
2022-02-18 19:51 ` Dan Williams
2022-01-28 0:27 ` [PATCH v3 07/14] cxl/region: Implement XHB verification Ben Widawsky
2022-02-18 20:23 ` Dan Williams
2022-01-28 0:27 ` [PATCH v3 08/14] cxl/region: HB port config verification Ben Widawsky
2022-02-14 16:20 ` Jonathan Cameron
2022-02-14 17:51 ` Ben Widawsky
2022-02-14 18:09 ` Jonathan Cameron
2022-02-15 16:35 ` Jonathan Cameron
2022-02-18 21:04 ` Dan Williams [this message]
2022-01-28 0:27 ` [PATCH v3 09/14] cxl/region: Add infrastructure for decoder programming Ben Widawsky
2022-02-01 18:16 ` Jonathan Cameron
2022-02-18 21:53 ` Dan Williams
2022-01-28 0:27 ` [PATCH v3 10/14] cxl/region: Collect host bridge decoders Ben Widawsky
2022-02-01 18:21 ` Jonathan Cameron
2022-02-18 23:42 ` Dan Williams
2022-01-28 0:27 ` [PATCH v3 11/14] cxl/region: Add support for single switch level Ben Widawsky
2022-02-01 18:26 ` Jonathan Cameron
2022-02-15 16:10 ` Jonathan Cameron
2022-02-18 18:23 ` Jonathan Cameron
2022-01-28 0:27 ` [PATCH v3 12/14] cxl: Program decoders for regions Ben Widawsky
2022-02-24 0:08 ` Dan Williams
2022-01-28 0:27 ` [PATCH v3 13/14] cxl/pmem: Convert nvdimm bridge API to use dev Ben Widawsky
2022-01-28 0:27 ` [PATCH v3 14/14] cxl/region: Create an nd_region Ben Widawsky
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAPcyv4i68KM52wXbeO_y9VHpbD_KQN1oZSZmps8dYYMuNYUc6w@mail.gmail.com \
--to=dan.j.williams@intel.com \
--cc=Jonathan.Cameron@huawei.com \
--cc=alison.schofield@intel.com \
--cc=ben.widawsky@intel.com \
--cc=helgaas@kernel.org \
--cc=ira.weiny@intel.com \
--cc=linux-cxl@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=nvdimm@lists.linux.dev \
--cc=patches@lists.linux.dev \
--cc=vishal.l.verma@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).