| Message ID | 7709896423244c523e810d9f5d2ef625e7babf3b.1698254338.git.alison.schofield@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl/region: Autodiscovery position repair | expand |
alison.schofield@ wrote: > From: Alison Schofield <alison.schofield@intel.com> > > Introduce a calculation to find a target's position in a region > interleave. Perform a self-test of the calculation on user-defined > regions. > > The region driver uses the kernel sort() function to put region > targets in relative order. Positions are assigned based on each > target's index in that sorted list. That relative sort doesn't > consider the offset of a port into its parent port which causes > some auto-discovered regions to fail creation. In one failure case, > a 2 + 2 config (2 host bridges each with 2 endpoints), the sort > puts all the targets of one port ahead of another port when they > were expected to be interleaved. > > In preparation for repairing the autodiscovery region assembly, > introduce a new method for discovering a target position in the > region interleave. > > cxl_calc_interleave_pos() adds a method to find the target position by > ascending from an endpoint to a root decoder. The calculation starts > with the endpoint's local position and position in the parent port. It > traverses towards the root decoder and examines both position and ways > in order to allow the position to be refined all the way to the root > decoder. > > This calculation: position = position * parent_ways + parent_pos; > applied iteratively yields the correct position. > > Include a self-test that exercises this new position calculation against > every successfully configured user-defined region. > > Signed-off-by: Alison Schofield <alison.schofield@intel.com> > --- > drivers/cxl/core/region.c | 142 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 142 insertions(+) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index eea8e89a6860..bea43da7e8f2 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -1501,6 +1501,128 @@ static int match_switch_decoder_by_range(struct device *dev, void *data) > return (r1->start == r2->start && r1->end == r2->end); > } > > +/** > + * cxl_find_pos_and_ways() - find the position of a port in a parent > + * and the parent interleave ways Given that this function is local to this file I do not think the cxl_ prefix is needed, and it does not match the below which is still find_pos_and_ways(). > + * > + * @port: the port in search of parent info > + * @range: the hpa range that the parent must map > + * @pos: the position of @port in the parent decoder target list > + * @ways: the interleave ways of the parent decoder > + * > + * This helper function for cxl_calc_interleave_pos() provides the > + * @pos and @ways used to calculate the endpoint position in a region > + * interleave. > + * > + * Use @range to find @port's parent port and follow that to the > + * parent switch decoder. Extract @ways from the switch decoder and > + * lookup the @port @pos in the switch decoder target list. Hmm, per your concern in the cover letter, yes, this find_pos_and_ways()() commentary is not adding much for describing the theory of operation. Lets just go with the calc_interleave_pos() comment block which is answering all my concerns about documenting what is going on here. > + * > + * Returns: 0: success, @pos and @ways are updated > + * -ENXIO: failed to find @pos or @ways > + */ > +static int find_pos_and_ways(struct cxl_port *port, struct range *range, > + int *pos, int *ways) > +{ > + struct cxl_switch_decoder *cxlsd; > + struct cxl_port *parent; > + struct device *dev; > + int rc = -ENXIO; > + > + parent = next_port(port); > + if (!parent) > + return rc; > + > + dev = device_find_child(&parent->dev, range, > + match_switch_decoder_by_range); > + if (!dev) { > + dev_err(port->uport_dev, > + "failed to find decoder mapping %#llx-%#llx\n", > + range->start, range->end); > + return rc; > + } > + cxlsd = to_cxl_switch_decoder(dev); > + *ways = cxlsd->cxld.interleave_ways; > + > + for (int i = 0; i < *ways; i++) { > + if (cxlsd->target[i] == port->parent_dport) { > + *pos = i; > + rc = 0; > + break; > + } > + } > + put_device(dev); > + > + return rc; > +} > + > +/** > + * cxl_calc_interleave_pos() - calculate an endpoint position in a region > + * @cxled: the endpoint decoder > + * > + * The endpoint position is calculated by traversing from the endpoint to > + * the root decoder and iteratively applying this calculation: > + * position = position * parent_ways + parent_pos; > + * > + * For example, the expected interleave order of the 4-way region shown > + * below is: mem0, mem2, mem1, mem3 > + * > + * root_port > + * / \ > + * host_bridge_0 host_bridge_1 > + * | | | | > + * mem0 mem1 mem2 mem3 > + * > + * In the example the calculator will iterate twice. The first iteration > + * uses the mem position in the host-bridge and the ways of the host- > + * bridge to generate the first, or local, position. The second iteration > + * uses the host-bridge position in the root_port and the ways of the > + * root_port to refine the position. > + * > + * A trace of the calculation per endpoint looks like this: > + * mem0: pos = 0 * 2 + 0 mem2: pos = 0 * 2 + 0 > + * pos = 0 * 2 + 0 pos = 0 * 2 + 1 > + * pos: 0 pos: 1 > + * > + * mem1: pos = 0 * 2 + 1 mem3: pos = 0 * 2 + 1 > + * pos = 1 * 2 + 0 pos = 1 * 2 + 1 > + * pos: 2 pos = 3 > + * > + * Note that while this example is simple, the method applies to more > + * complex topologies, including those with switches. Looks good. Just delete the find_pos_and_ways() kdoc and we can call this one good to go.
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index eea8e89a6860..bea43da7e8f2 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -1501,6 +1501,128 @@ static int match_switch_decoder_by_range(struct device *dev, void *data) return (r1->start == r2->start && r1->end == r2->end); } +/** + * cxl_find_pos_and_ways() - find the position of a port in a parent + * and the parent interleave ways + * + * @port: the port in search of parent info + * @range: the hpa range that the parent must map + * @pos: the position of @port in the parent decoder target list + * @ways: the interleave ways of the parent decoder + * + * This helper function for cxl_calc_interleave_pos() provides the + * @pos and @ways used to calculate the endpoint position in a region + * interleave. + * + * Use @range to find @port's parent port and follow that to the + * parent switch decoder. Extract @ways from the switch decoder and + * lookup the @port @pos in the switch decoder target list. + * + * Returns: 0: success, @pos and @ways are updated + * -ENXIO: failed to find @pos or @ways + */ +static int find_pos_and_ways(struct cxl_port *port, struct range *range, + int *pos, int *ways) +{ + struct cxl_switch_decoder *cxlsd; + struct cxl_port *parent; + struct device *dev; + int rc = -ENXIO; + + parent = next_port(port); + if (!parent) + return rc; + + dev = device_find_child(&parent->dev, range, + match_switch_decoder_by_range); + if (!dev) { + dev_err(port->uport_dev, + "failed to find decoder mapping %#llx-%#llx\n", + range->start, range->end); + return rc; + } + cxlsd = to_cxl_switch_decoder(dev); + *ways = cxlsd->cxld.interleave_ways; + + for (int i = 0; i < *ways; i++) { + if (cxlsd->target[i] == port->parent_dport) { + *pos = i; + rc = 0; + break; + } + } + put_device(dev); + + return rc; +} + +/** + * cxl_calc_interleave_pos() - calculate an endpoint position in a region + * @cxled: the endpoint decoder + * + * The endpoint position is calculated by traversing from the endpoint to + * the root decoder and iteratively applying this calculation: + * position = position * parent_ways + parent_pos; + * + * For example, the expected interleave order of the 4-way region shown + * below is: mem0, mem2, mem1, mem3 + * + * root_port + * / \ + * host_bridge_0 host_bridge_1 + * | | | | + * mem0 mem1 mem2 mem3 + * + * In the example the calculator will iterate twice. The first iteration + * uses the mem position in the host-bridge and the ways of the host- + * bridge to generate the first, or local, position. The second iteration + * uses the host-bridge position in the root_port and the ways of the + * root_port to refine the position. + * + * A trace of the calculation per endpoint looks like this: + * mem0: pos = 0 * 2 + 0 mem2: pos = 0 * 2 + 0 + * pos = 0 * 2 + 0 pos = 0 * 2 + 1 + * pos: 0 pos: 1 + * + * mem1: pos = 0 * 2 + 1 mem3: pos = 0 * 2 + 1 + * pos = 1 * 2 + 0 pos = 1 * 2 + 1 + * pos: 2 pos = 3 + * + * Note that while this example is simple, the method applies to more + * complex topologies, including those with switches. + * + * Return: position >= 0 on success + * -ENXIO on failure + */ + +static int cxl_calc_interleave_pos(struct cxl_endpoint_decoder *cxled) +{ + struct cxl_port *iter, *port = cxled_to_port(cxled); + struct cxl_memdev *cxlmd = cxled_to_memdev(cxled); + struct range *range = &cxled->cxld.hpa_range; + int parent_ways = 0, parent_pos = 0, pos = 0; + int rc; + + /* Iterate from endpoint to root_port refining the position */ + for (iter = port; iter; iter = next_port(iter)) { + if (is_cxl_root(iter)) + break; + + rc = find_pos_and_ways(iter, range, &parent_pos, &parent_ways); + if (rc) + return rc; + + pos = pos * parent_ways + parent_pos; + } + + dev_dbg(&cxlmd->dev, + "decoder:%s parent:%s port:%s range:%#llx-%#llx pos:%d\n", + dev_name(&cxled->cxld.dev), dev_name(cxlmd->dev.parent), + dev_name(&port->dev), range->start, range->end, pos); + + return pos; +} + static void find_positions(const struct cxl_switch_decoder *cxlsd, const struct cxl_port *iter_a, const struct cxl_port *iter_b, int *a_pos, @@ -1764,6 +1886,26 @@ static int cxl_region_attach(struct cxl_region *cxlr, .end = p->res->end, }; + if (p->nr_targets != p->interleave_ways) + return 0; + + /* + * Test the auto-discovery position calculator function + * against this successfully created user-defined region. + * A fail message here means that this interleave config + * will fail when presented as CXL_REGION_F_AUTO. + */ + for (int i = 0; i < p->nr_targets; i++) { + struct cxl_endpoint_decoder *cxled = p->targets[i]; + int test_pos; + + test_pos = cxl_calc_interleave_pos(cxled); + dev_dbg(&cxled->cxld.dev, + "Test cxl_calc_interleave_pos(): %s test_pos:%d cxled->pos:%d\n", + (test_pos == cxled->pos) ? "success" : "fail", + test_pos, cxled->pos); + } + return 0; err_decrement: