[PATCH] cxl: set root decoder granularity based on region params

[PATCH] cxl: set root decoder granularity based on region params

[Jim Harris]

The granularity for downstream targets is all based on descending the
topology from the root. So we need to set the root decoder's granularity
based on the region params.

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:
        "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 |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 6d63b8798c29..f14110e35f79 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1691,6 +1691,11 @@ static int cxl_region_attach(struct cxl_region *cxlr,
 		return -EINVAL;
 	}
 
+	/* Set root decoder's granularity now, so that we can use it to calculate
+	 * granularity for the downstream targets in cxl_region_setup_targets().
+	 */
+	cxlrd->cxlsd.cxld.interleave_granularity = cxlr->params.interleave_granularity;
+
 	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
 		int i;
 

RE: [PATCH] cxl: set root decoder granularity based on region params

[Dan Williams]

Jim Harris wrote:
> The granularity for downstream targets is all based on descending the
> topology from the root. So we need to set the root decoder's granularity
> based on the region params.
> 
> 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:
>         "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 |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 6d63b8798c29..f14110e35f79 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1691,6 +1691,11 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  		return -EINVAL;
>  	}
>  
> +	/* Set root decoder's granularity now, so that we can use it to calculate
> +	 * granularity for the downstream targets in cxl_region_setup_targets().
> +	 */

Minor, and I mean minor, nit, the cxl subsystem is using:

/*
 * Comment...
 */

...block-comment style. Yes, it varies across subsystems, yes it's
arbitrary, but general rule is follow local customs.


> +	cxlrd->cxlsd.cxld.interleave_granularity = cxlr->params.interleave_granularity;
> +

So I think this is only valid in the cxlrd->cxlsd.cxld.interleave_ways == 1
case as interleave_granularity_store() forbids regions that do not match
the granularity of the root.

If a BIOS tries to ship such a config in production that's when I expect
that policy needs to be revisited, but outside of being forced to
reconsider that stance the complexity reduction is the benefit.

In the meantime maybe add an "effective granularity" concept for x1 root
decoders and x1 switches so the math can use that value?

Re: [PATCH] cxl: set root decoder granularity based on region params

[Jim Harris]

> On Oct 4, 2023, at 6:18 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> 
> Jim Harris wrote:
>> 
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index 6d63b8798c29..f14110e35f79 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -1691,6 +1691,11 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>> return -EINVAL;
>> }
>> 
>> + /* Set root decoder's granularity now, so that we can use it to calculate
>> + * granularity for the downstream targets in cxl_region_setup_targets().
>> + */
> 
> Minor, and I mean minor, nit, the cxl subsystem is using:
> 
> /*
> * Comment...
> */
> 
> ...block-comment style. Yes, it varies across subsystems, yes it's
> arbitrary, but general rule is follow local customs.

Ack.

>> + cxlrd->cxlsd.cxld.interleave_granularity = cxlr->params.interleave_granularity;
>> +
> 
> So I think this is only valid in the cxlrd->cxlsd.cxld.interleave_ways == 1
> case as interleave_granularity_store() forbids regions that do not match
> the granularity of the root.

That’s correct. And now I think it’s better to put this assignment in
interleave_granularity_store() instead since we are already doing
root decoder related checking there, and we would then just be setting
this granularity once instead of once per memdev.

> If a BIOS tries to ship such a config in production that's when I expect
> that policy needs to be revisited, but outside of being forced to
> reconsider that stance the complexity reduction is the benefit.
> 
> In the meantime maybe add an "effective granularity" concept for x1 root
> decoders and x1 switches so the math can use that value?

Ack. Use “effective_granularity” (EG) for the topology math and
“interleave_granularity” (IG) for programming the decoders.

For the x1 switches, is your intent here to set IG = 256 since the IG is
really a don’t care from the decoder’s perspective? Or is there another
reason we would need to track different IG v. EG for x1 switches?

Re: [PATCH] cxl: set root decoder granularity based on region params

[Dan Williams]

Jim Harris wrote:
[..]
> > If a BIOS tries to ship such a config in production that's when I expect
> > that policy needs to be revisited, but outside of being forced to
> > reconsider that stance the complexity reduction is the benefit.
> > 
> > In the meantime maybe add an "effective granularity" concept for x1 root
> > decoders and x1 switches so the math can use that value?
> 
> Ack. Use “effective_granularity” (EG) for the topology math and
> “interleave_granularity” (IG) for programming the decoders.
> 
> For the x1 switches, is your intent here to set IG = 256 since the IG is
> really a don’t care from the decoder’s perspective? Or is there another
> reason we would need to track different IG v. EG for x1 switches?

Hmm, I see what you are saying. If the value is a don't care, might as well
just update interleave_granularity to make the math friendly. So "effective
granularity" is probably just a documentation update to the 'struct
cxl_decoder' kdoc so people are not surprised about why this may differ
from CFMWS value.

Re: [PATCH] cxl: set root decoder granularity based on region params

[Dan Williams]

Jim Harris wrote:
[..]
> > So I think this is only valid in the cxlrd->cxlsd.cxld.interleave_ways == 1
> > case as interleave_granularity_store() forbids regions that do not match
> > the granularity of the root.
> 
> That’s correct. And now I think it’s better to put this assignment in
> interleave_granularity_store() instead since we are already doing
> root decoder related checking there, and we would then just be setting
> this granularity once instead of once per memdev.

Did you ever send the interleave_granularity_store() version of this
patch? I can't seem to find it.

Re: [PATCH] cxl: set root decoder granularity based on region params

[Jim Harris]

On Tue, Oct 24, 2023 at 05:55:53PM -0700, Dan Williams wrote:
> Jim Harris wrote:
> [..]
> > > So I think this is only valid in the cxlrd->cxlsd.cxld.interleave_ways == 1
> > > case as interleave_granularity_store() forbids regions that do not match
> > > the granularity of the root.
> > 
> > That’s correct. And now I think it’s better to put this assignment in
> > interleave_granularity_store() instead since we are already doing
> > root decoder related checking there, and we would then just be setting
> > this granularity once instead of once per memdev.
> 
> Did you ever send the interleave_granularity_store() version of this
> patch? I can't seem to find it.

I found a third approach that was much simpler than the effective_granularity
approach. It's in this thread with a completely different title.

Should I have still marked it v2, even though the commit title and patch was
radically different?

Re: [PATCH] cxl: set root decoder granularity based on region params

[Dan Williams]

Jim Harris wrote:
> On Tue, Oct 24, 2023 at 05:55:53PM -0700, Dan Williams wrote:
> > Jim Harris wrote:
> > [..]
> > > > So I think this is only valid in the cxlrd->cxlsd.cxld.interleave_ways == 1
> > > > case as interleave_granularity_store() forbids regions that do not match
> > > > the granularity of the root.
> > > 
> > > That’s correct. And now I think it’s better to put this assignment in
> > > interleave_granularity_store() instead since we are already doing
> > > root decoder related checking there, and we would then just be setting
> > > this granularity once instead of once per memdev.
> > 
> > Did you ever send the interleave_granularity_store() version of this
> > patch? I can't seem to find it.
> 
> I found a third approach that was much simpler than the effective_granularity
> approach. It's in this thread with a completely different title.

Oh, this one
http://lore.kernel.org/r/169660931834.684402.2774329392272976121.stgit@bgt-140510-bm03.eng.stellus.in?

I'll go comment.

[PATCH] cxl/region: use region (not root decoder) granularity for calculations

[Jim Harris]

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(-)

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

RE: [PATCH] cxl/region: use region (not root decoder) granularity for calculations

[Dan Williams]

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.

Re: [PATCH] cxl/region: use region (not root decoder) granularity for calculations

[Jim Harris]

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.

[PATCH v2] cxl/region: use region (not root decoder) granularity for calculations

[Jim Harris]

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 |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 6d63b8798c29..c25be26d0bdf 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1133,7 +1133,14 @@ static int cxl_port_setup_targets(struct cxl_port *port,
 	}
 
 	if (is_cxl_root(parent_port)) {
-		parent_ig = cxlrd->cxlsd.cxld.interleave_granularity;
+		/*
+		 * Root decoder IG is always set to value in CFMWS which
+		 * may be different than this region's IG.  We can use the
+		 * region's IG here since interleave_granularity_store()
+		 * does not allow interleaved host-bridges with
+		 * root IG != region IG.
+		 */
+		parent_ig = p->interleave_granularity;
 		parent_iw = cxlrd->cxlsd.cxld.interleave_ways;
 		/*
 		 * For purposes of address bit routing, use power-of-2 math for

RE: [PATCH v2] cxl/region: use region (not root decoder) granularity for calculations

[Dan Williams]

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.
> 
> 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>

Applied, I did end up changing the subject to:

   "cxl/region: Fix x1 root-decoder granularity calculations"

...and added:

    Fixes: 27b3f8d13830 ("cxl/region: Program target lists")
    Cc: <stable@vger.kernel.org>

Thanks, Jim!

RE: [PATCH v2] cxl/region: use region (not root decoder) granularity for calculations

[Dan Williams]

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.
> 
> 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 |    9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 6d63b8798c29..c25be26d0bdf 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1133,7 +1133,14 @@ static int cxl_port_setup_targets(struct cxl_port *port,
>  	}
>  
>  	if (is_cxl_root(parent_port)) {
> -		parent_ig = cxlrd->cxlsd.cxld.interleave_granularity;
> +		/*
> +		 * Root decoder IG is always set to value in CFMWS which
> +		 * may be different than this region's IG.  We can use the
> +		 * region's IG here since interleave_granularity_store()
> +		 * does not allow interleaved host-bridges with
> +		 * root IG != region IG.
> +		 */
> +		parent_ig = p->interleave_granularity;
>  		parent_iw = cxlrd->cxlsd.cxld.interleave_ways;

This causes the kernel to correctly detect that cxl_test was programming
an incorrect host-bridge level granularity value. Fixup the test.

diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
index fb6ab9cef84f..b88546299902 100644
--- a/tools/testing/cxl/test/cxl.c
+++ b/tools/testing/cxl/test/cxl.c
@@ -831,7 +831,7 @@ static void mock_init_hdm_decoder(struct cxl_decoder *cxld)
                        cxld->interleave_ways = 2;
                else
                        cxld->interleave_ways = 1;
-               cxld->interleave_granularity = 256;
+               cxld->interleave_granularity = 4096;
                cxld->hpa_range = (struct range) {
                        .start = base,
                        .end = base + size - 1,