| Message ID | 169660931834.684402.2774329392272976121.stgit@bgt-140510-bm03.eng.stellus.in |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl/region: use region (not root decoder) granularity for calculations | expand |
Jim Harris wrote: > Root decoder granularity must match value from CFWMS, which may not > be the region's granularity for non-interleaved root decoders. > > So when calculating granularities for host bridge decoders, use the > region's granularity instead of the root decoder's granularity to ensure > the correct granularities are set for the host bridge decoders and any > downstream switch decoders. [..] > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 6d63b8798c29..70f7c66ee2ce 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -1133,7 +1133,7 @@ static int cxl_port_setup_targets(struct cxl_port *port, > } > > if (is_cxl_root(parent_port)) { > - parent_ig = cxlrd->cxlsd.cxld.interleave_granularity; > + parent_ig = p->interleave_granularity; > parent_iw = cxlrd->cxlsd.cxld.interleave_ways; So the reason I went looking for the interleave_granularity_store() version of the patch was only because this one when viewed in isolation has the problem that the root-decoder granularity can only be overridden to the region granularity when root-decoder interleave ways is 1. Is this assumed safe because interleave_granularity_store() is enforcing that regions must match root-decoder granularity? If "yes", then this likely wants a "/* See interleave_granularity_store() ... */" like comment to describe why this override is safe, if "no" then I am missing something. What I liked about the idea of putting this in interleave_granularity_store() is keeping all the simplification shortcuts and commentary in one place, in case the region interleave != root-decoder interleave ban is lifted. I don't mind doing this fixup in cxl_port_setup_targets(), but would want the comment per above.
On Tue, Oct 24, 2023 at 07:11:55PM -0700, Dan Williams wrote: > > So the reason I went looking for the interleave_granularity_store() > version of the patch was only because this one when viewed in isolation > has the problem that the root-decoder granularity can only be overridden > to the region granularity when root-decoder interleave ways is 1. > > Is this assumed safe because interleave_granularity_store() is enforcing > that regions must match root-decoder granularity? If "yes", then this > likely wants a "/* See interleave_granularity_store() ... */" like > comment to describe why this override is safe, if "no" then I am missing > something. The answer is "yes". I'll add a comment in the v2. > What I liked about the idea of putting this in > interleave_granularity_store() is keeping all the simplification > shortcuts and commentary in one place, in case the region interleave != > root-decoder interleave ban is lifted. > > I don't mind doing this fixup in cxl_port_setup_targets(), but would > want the comment per above. I also debated this after identifiying the 1-line code fix. This new approach avoids the whole "effective_granularity" special case for the root decoders for now at least. It would need to be revisited if we support interleaved host-bridges with region IG < root IG in the future.
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 6d63b8798c29..70f7c66ee2ce 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -1133,7 +1133,7 @@ static int cxl_port_setup_targets(struct cxl_port *port, } if (is_cxl_root(parent_port)) { - parent_ig = cxlrd->cxlsd.cxld.interleave_granularity; + parent_ig = p->interleave_granularity; parent_iw = cxlrd->cxlsd.cxld.interleave_ways; /* * For purposes of address bit routing, use power-of-2 math for
Root decoder granularity must match value from CFWMS, which may not be the region's granularity for non-interleaved root decoders. So when calculating granularities for host bridge decoders, use the region's granularity instead of the root decoder's granularity to ensure the correct granularities are set for the host bridge decoders and any downstream switch decoders. Test configuration is 1 host bridge * 2 switches * 2 endpoints per switch. Region created with 2048 granularity using following command line: cxl create-region -m -d decoder0.0 -w 4 mem0 mem2 mem1 mem3 \ -g 2048 -s 2048M Use "cxl list -PDE | grep granularity" to get a view of the granularity set at each level of the topology. Before this patch: "interleave_granularity":2048, "interleave_granularity":2048, "interleave_granularity":512, "interleave_granularity":2048, "interleave_granularity":2048, "interleave_granularity":512, "interleave_granularity":256, After: "interleave_granularity":2048, "interleave_granularity":2048, "interleave_granularity":4096, "interleave_granularity":2048, "interleave_granularity":2048, "interleave_granularity":4096, "interleave_granularity":2048, Signed-off-by: Jim Harris <jim.harris@samsung.com> --- drivers/cxl/core/region.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)