[PATCH v2 0/3] CXL Region Provisioning Fixes

[PATCH v2 0/3] CXL Region Provisioning Fixes

[Dan Williams]

Changes and new fixes since v1 [1]:

While reviewing "cxl/region: Constrain region granularity scaling
factor" [2] it became clear that the attempt to support region granularity >
window (ACPI CFMWS entry) granularity was misguided. Update the
implementation to disallow that configuration, but also allow
unrestricted granularity settings when the window interleave-ways is 1.

Speaking of window interleave-ways == 1 add a fix to
cxl_port_setup_targets() for the case where it does not need to assign
addition address bits to the decode.

Lastly, fix a regression with the original version of "cxl/region: Move
HPA setup to cxl_region_attach()" that broke the establishment of
->hpa_range in switch decoders.

An expansion of cxl_test to backstop these fixes will be posted shortly.

These are based on the updated state of the cxl/pending branch that has
pulled in the reviewed fixes from Dan and Bagas.

[1]: https://lore.kernel.org/r/165853775181.2430596.3054032756974329979.stgit@dwillia2-xfh.jf.intel.com/
[2]: https://lore.kernel.org/r/165853778028.2430596.7493880465382850752.stgit@dwillia2-xfh.jf.intel.com/

---

Dan Williams (3):
      cxl/region: Move HPA setup to cxl_region_attach()
      cxl/region: Fix x1 interleave to greater than x1 interleave routing
      cxl/region: Disallow region granularity != window granularity


 drivers/cxl/core/hdm.c    |   26 ++------------------------
 drivers/cxl/core/region.c |   43 ++++++++++++++++++++++++++++++++++---------
 2 files changed, 36 insertions(+), 33 deletions(-)

base-commit: 2901c8bdedca19e5efdab2ea55b465102231b315

[PATCH v2 1/3] cxl/region: Move HPA setup to cxl_region_attach()

[Dan Williams]

A recent bug fix added the setup of the endpoint decoder interleave
geometry settings to cxl_region_attach(). Move the HPA setup there as
well to keep all endpoint decoder parameter setting in a central
location.

For symmetry, move endpoint HPA teardown to cxl_region_detach(), and for
switches move HPA setup / teardown to cxl_port_{setup,reset}_targets().

Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/hdm.c    |   26 ++------------------------
 drivers/cxl/core/region.c |   24 ++++++++++++++++++++++--
 2 files changed, 24 insertions(+), 26 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 8143e2615957..e096f74e19df 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -499,28 +499,6 @@ static void cxld_set_type(struct cxl_decoder *cxld, u32 *ctrl)
 			  CXL_HDM_DECODER0_CTRL_TYPE);
 }
 
-static void cxld_set_hpa(struct cxl_decoder *cxld, u64 *base, u64 *size)
-{
-	struct cxl_region *cxlr = cxld->region;
-	struct cxl_region_params *p = &cxlr->params;
-
-	cxld->hpa_range = (struct range) {
-		.start = p->res->start,
-		.end = p->res->end,
-	};
-
-	*base = p->res->start;
-	*size = resource_size(p->res);
-}
-
-static void cxld_clear_hpa(struct cxl_decoder *cxld)
-{
-	cxld->hpa_range = (struct range) {
-		.start = 0,
-		.end = -1,
-	};
-}
-
 static int cxlsd_set_targets(struct cxl_switch_decoder *cxlsd, u64 *tgt)
 {
 	struct cxl_dport **t = &cxlsd->target[0];
@@ -601,7 +579,8 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
 	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
 	cxld_set_interleave(cxld, &ctrl);
 	cxld_set_type(cxld, &ctrl);
-	cxld_set_hpa(cxld, &base, &size);
+	base = cxld->hpa_range.start;
+	size = range_len(&cxld->hpa_range);
 
 	writel(upper_32_bits(base), hdm + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(id));
 	writel(lower_32_bits(base), hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(id));
@@ -674,7 +653,6 @@ static int cxl_decoder_reset(struct cxl_decoder *cxld)
 	ctrl &= ~CXL_HDM_DECODER0_CTRL_COMMIT;
 	writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(id));
 
-	cxld_clear_hpa(cxld);
 	writel(0, hdm + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(id));
 	writel(0, hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(id));
 	writel(0, hdm + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(id));
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 40f04c543e41..e71077beb021 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1044,6 +1044,10 @@ static int cxl_port_setup_targets(struct cxl_port *port,
 
 	cxld->interleave_ways = iw;
 	cxld->interleave_granularity = ig;
+	cxld->hpa_range = (struct range) {
+		.start = p->res->start,
+		.end = p->res->end,
+	};
 	dev_dbg(&cxlr->dev, "%s:%s iw: %d ig: %d\n", dev_name(port->uport),
 		dev_name(&port->dev), iw, ig);
 add_target:
@@ -1070,13 +1074,21 @@ static void cxl_port_reset_targets(struct cxl_port *port,
 				   struct cxl_region *cxlr)
 {
 	struct cxl_region_ref *cxl_rr = cxl_rr_load(port, cxlr);
+	struct cxl_decoder *cxld;
 
 	/*
 	 * After the last endpoint has been detached the entire cxl_rr may now
 	 * be gone.
 	 */
-	if (cxl_rr)
-		cxl_rr->nr_targets_set = 0;
+	if (!cxl_rr)
+		return;
+	cxl_rr->nr_targets_set = 0;
+
+	cxld = cxl_rr->decoder;
+	cxld->hpa_range = (struct range) {
+		.start = 0,
+		.end = -1,
+	};
 }
 
 static void cxl_region_teardown_targets(struct cxl_region *cxlr)
@@ -1257,6 +1269,10 @@ static int cxl_region_attach(struct cxl_region *cxlr,
 
 	cxled->cxld.interleave_ways = p->interleave_ways;
 	cxled->cxld.interleave_granularity = p->interleave_granularity;
+	cxled->cxld.hpa_range = (struct range) {
+		.start = p->res->start,
+		.end = p->res->end,
+	};
 
 	return 0;
 
@@ -1315,6 +1331,10 @@ static int cxl_region_detach(struct cxl_endpoint_decoder *cxled)
 	}
 	p->targets[cxled->pos] = NULL;
 	p->nr_targets--;
+	cxled->cxld.hpa_range = (struct range) {
+		.start = 0,
+		.end = -1,
+	};
 
 	/* notify the region driver that one of its targets has departed */
 	up_write(&cxl_region_rwsem);

[PATCH v2 2/3] cxl/region: Fix x1 interleave to greater than x1 interleave routing

[Dan Williams]

In cases where the decode fans out as it traverses downstream, the
interleave granularity needs to increment to identify the port selector
bits out of the remaining address bits. For example, recall that with an
x2 parent port intereleave (IW == 1), the downstream decode for children
of those ports will either see address bit IG+8 always set, or address
bit IG+8 always clear. So if the child port needs to select a downstream
port it can only use address bits starting at IG+9 (where IG and IW are
the CXL encoded values for interleave granularity (ilog2(ig) - 8) and
ways (ilog2(iw))).

When the parent port interleave is x1 no such masking occurs and the
child port can maintain the granularity that was routed to the parent
port.

Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/region.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index e71077beb021..641bc6344a4a 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1025,7 +1025,11 @@ static int cxl_port_setup_targets(struct cxl_port *port,
 		return rc;
 	}
 
-	if (cxl_rr->nr_targets > 1) {
+	/*
+	 * If @parent_port is masking address bits, pick the next unused address
+	 * bit to route @port's targets.
+	 */
+	if (parent_iw > 1 && cxl_rr->nr_targets > 1) {
 		u32 address_bit = max(peig + peiw, eiw + peig);
 
 		eig = address_bit - eiw + 1;

[PATCH v2 3/3] cxl/region: Disallow region granularity != window granularity

[Dan Williams]

The endpoint decode granularity must be <= the window granularity
otherwise capacity in the endpoints is lost in the decode. Consider an
attempt to have a region granularity of 512 with 4 devices within a
window that maps 2 host bridges at a granularity of 256 bytes:

HPA	DPA Offset	HB	Port	EP
0x0	0x0		0	0	0
0x100	0x0		1	0	2
0x200	0x100		0	0	0
0x300	0x100		1	0	2
0x400	0x200		0	1	1
0x500	0x200		1	1	3
0x600	0x300		0	1	1
0x700	0x300		1	1	3
0x800	0x400		0	0	0
0x900	0x400		1	0	2
0xA00	0x500		0	0	0
0xB00	0x500		1	0	2

Notice how endpoint0 maps HPA 0x0 and 0x200 correctly, but then HPA
0x800 results in DPA 0x200 to 0x400 on endpoint0 being not skipped.

Fix this by restricing the region granularity to be equal to the window
granularity resulting in the following for a x4 region under a x2 window
at a granularity of 256.

HPA	DPA Offset	HB	Port	EP
0x0	0x0		0	0	0
0x100	0x0		1	0	2
0x200	0x0		0	1	1
0x300	0x0		1	1	3
0x400	0x100		0	0	0
0x500	0x100		1	0	2
0x600	0x100		0	1	1
0x700	0x100		1	1	3

Not that it ever made practical sense to support region granularity >
window granularity. The window rotates host bridges causing endpoints to
never see a consecutive stream of requests at the desired granularity
without breaks to issue cycles to the other host bridge.

Fixes: 80d10a6cee05 ("cxl/region: Add interleave geometry attributes")
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/region.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 641bc6344a4a..401148016978 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -395,13 +395,14 @@ static ssize_t interleave_granularity_store(struct device *dev,
 		return rc;
 
 	/*
-	 * Disallow region granularity less than root granularity to
-	 * simplify the implementation. Otherwise, region's with a
-	 * granularity less than the root interleave result in needing
-	 * multiple endpoints to support a single slot in the
-	 * interleave.
+	 * When the host-bridge is interleaved, disallow region granularity !=
+	 * root granularity. Regions with a granularity less than the root
+	 * interleave result in needing multiple endpoints to support a single
+	 * slot in the interleave (possible to suport in the future). Regions
+	 * with a granularity greater than the root interleave result in invalid
+	 * DPA translations (invalid to support).
 	 */
-	if (val < cxld->interleave_granularity)
+	if (cxld->interleave_ways > 1 && val != cxld->interleave_granularity)
 		return -EINVAL;
 
 	rc = down_write_killable(&cxl_region_rwsem);

Re: [PATCH v2 2/3] cxl/region: Fix x1 interleave to greater than x1 interleave routing

[Verma, Vishal L]

On Fri, 2022-08-05 at 13:27 -0700, Dan Williams wrote:
> In cases where the decode fans out as it traverses downstream, the
> interleave granularity needs to increment to identify the port selector
> bits out of the remaining address bits. For example, recall that with an
> x2 parent port intereleave (IW == 1), the downstream decode for children
> of those ports will either see address bit IG+8 always set, or address
> bit IG+8 always clear. So if the child port needs to select a downstream
> port it can only use address bits starting at IG+9 (where IG and IW are
> the CXL encoded values for interleave granularity (ilog2(ig) - 8) and
> ways (ilog2(iw))).
> 
> When the parent port interleave is x1 no such masking occurs and the
> child port can maintain the granularity that was routed to the parent
> port.
> 
> Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/core/region.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

Makes sense to me,

Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index e71077beb021..641bc6344a4a 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1025,7 +1025,11 @@ static int cxl_port_setup_targets(struct cxl_port *port,
>                 return rc;
>         }
>  
> -       if (cxl_rr->nr_targets > 1) {
> +       /*
> +        * If @parent_port is masking address bits, pick the next unused address
> +        * bit to route @port's targets.
> +        */
> +       if (parent_iw > 1 && cxl_rr->nr_targets > 1) {
>                 u32 address_bit = max(peig + peiw, eiw + peig);
>  
>                 eig = address_bit - eiw + 1;
> 

Re: [PATCH v2 1/3] cxl/region: Move HPA setup to cxl_region_attach()

[Ira Weiny]

On Fri, Aug 05, 2022 at 01:27:40PM -0700, Dan Williams wrote:
> A recent bug fix added the setup of the endpoint decoder interleave
> geometry settings to cxl_region_attach(). Move the HPA setup there as
> well to keep all endpoint decoder parameter setting in a central
> location.
> 
> For symmetry, move endpoint HPA teardown to cxl_region_detach(), and for
> switches move HPA setup / teardown to cxl_port_{setup,reset}_targets().
> 
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> ---
>  drivers/cxl/core/hdm.c    |   26 ++------------------------
>  drivers/cxl/core/region.c |   24 ++++++++++++++++++++++--
>  2 files changed, 24 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 8143e2615957..e096f74e19df 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -499,28 +499,6 @@ static void cxld_set_type(struct cxl_decoder *cxld, u32 *ctrl)
>  			  CXL_HDM_DECODER0_CTRL_TYPE);
>  }
>  
> -static void cxld_set_hpa(struct cxl_decoder *cxld, u64 *base, u64 *size)
> -{
> -	struct cxl_region *cxlr = cxld->region;
> -	struct cxl_region_params *p = &cxlr->params;
> -
> -	cxld->hpa_range = (struct range) {
> -		.start = p->res->start,
> -		.end = p->res->end,
> -	};
> -
> -	*base = p->res->start;
> -	*size = resource_size(p->res);
> -}
> -
> -static void cxld_clear_hpa(struct cxl_decoder *cxld)
> -{
> -	cxld->hpa_range = (struct range) {
> -		.start = 0,
> -		.end = -1,
> -	};
> -}
> -
>  static int cxlsd_set_targets(struct cxl_switch_decoder *cxlsd, u64 *tgt)
>  {
>  	struct cxl_dport **t = &cxlsd->target[0];
> @@ -601,7 +579,8 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
>  	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
>  	cxld_set_interleave(cxld, &ctrl);
>  	cxld_set_type(cxld, &ctrl);
> -	cxld_set_hpa(cxld, &base, &size);
> +	base = cxld->hpa_range.start;
> +	size = range_len(&cxld->hpa_range);
>  
>  	writel(upper_32_bits(base), hdm + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(id));
>  	writel(lower_32_bits(base), hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(id));
> @@ -674,7 +653,6 @@ static int cxl_decoder_reset(struct cxl_decoder *cxld)
>  	ctrl &= ~CXL_HDM_DECODER0_CTRL_COMMIT;
>  	writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(id));
>  
> -	cxld_clear_hpa(cxld);
>  	writel(0, hdm + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(id));
>  	writel(0, hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(id));
>  	writel(0, hdm + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(id));
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 40f04c543e41..e71077beb021 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1044,6 +1044,10 @@ static int cxl_port_setup_targets(struct cxl_port *port,
>  
>  	cxld->interleave_ways = iw;
>  	cxld->interleave_granularity = ig;
> +	cxld->hpa_range = (struct range) {
> +		.start = p->res->start,
> +		.end = p->res->end,
> +	};
>  	dev_dbg(&cxlr->dev, "%s:%s iw: %d ig: %d\n", dev_name(port->uport),
>  		dev_name(&port->dev), iw, ig);
>  add_target:
> @@ -1070,13 +1074,21 @@ static void cxl_port_reset_targets(struct cxl_port *port,
>  				   struct cxl_region *cxlr)
>  {
>  	struct cxl_region_ref *cxl_rr = cxl_rr_load(port, cxlr);
> +	struct cxl_decoder *cxld;
>  
>  	/*
>  	 * After the last endpoint has been detached the entire cxl_rr may now
>  	 * be gone.
>  	 */
> -	if (cxl_rr)
> -		cxl_rr->nr_targets_set = 0;
> +	if (!cxl_rr)
> +		return;
> +	cxl_rr->nr_targets_set = 0;
> +
> +	cxld = cxl_rr->decoder;
> +	cxld->hpa_range = (struct range) {
> +		.start = 0,
> +		.end = -1,
> +	};
>  }
>  
>  static void cxl_region_teardown_targets(struct cxl_region *cxlr)
> @@ -1257,6 +1269,10 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  
>  	cxled->cxld.interleave_ways = p->interleave_ways;
>  	cxled->cxld.interleave_granularity = p->interleave_granularity;
> +	cxled->cxld.hpa_range = (struct range) {
> +		.start = p->res->start,
> +		.end = p->res->end,
> +	};
>  
>  	return 0;
>  
> @@ -1315,6 +1331,10 @@ static int cxl_region_detach(struct cxl_endpoint_decoder *cxled)
>  	}
>  	p->targets[cxled->pos] = NULL;
>  	p->nr_targets--;
> +	cxled->cxld.hpa_range = (struct range) {
> +		.start = 0,
> +		.end = -1,
> +	};
>  
>  	/* notify the region driver that one of its targets has departed */
>  	up_write(&cxl_region_rwsem);
> 

Re: [PATCH v2 2/3] cxl/region: Fix x1 interleave to greater than x1 interleave routing

[Ira Weiny]

On Fri, Aug 05, 2022 at 01:27:45PM -0700, Dan Williams wrote:
> In cases where the decode fans out as it traverses downstream, the
> interleave granularity needs to increment to identify the port selector
> bits out of the remaining address bits. For example, recall that with an
> x2 parent port intereleave (IW == 1), the downstream decode for children
> of those ports will either see address bit IG+8 always set, or address
> bit IG+8 always clear. So if the child port needs to select a downstream
> port it can only use address bits starting at IG+9 (where IG and IW are
> the CXL encoded values for interleave granularity (ilog2(ig) - 8) and
> ways (ilog2(iw))).
> 
> When the parent port interleave is x1 no such masking occurs and the
> child port can maintain the granularity that was routed to the parent
> port.
> 
> Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> ---
>  drivers/cxl/core/region.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index e71077beb021..641bc6344a4a 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1025,7 +1025,11 @@ static int cxl_port_setup_targets(struct cxl_port *port,
>  		return rc;
>  	}
>  
> -	if (cxl_rr->nr_targets > 1) {
> +	/*
> +	 * If @parent_port is masking address bits, pick the next unused address
> +	 * bit to route @port's targets.
> +	 */
> +	if (parent_iw > 1 && cxl_rr->nr_targets > 1) {
>  		u32 address_bit = max(peig + peiw, eiw + peig);
>  
>  		eig = address_bit - eiw + 1;
> 

Re: [PATCH v2 3/3] cxl/region: Disallow region granularity != window granularity

[Ira Weiny]

On Fri, Aug 05, 2022 at 01:27:51PM -0700, Dan Williams wrote:
> The endpoint decode granularity must be <= the window granularity
> otherwise capacity in the endpoints is lost in the decode. Consider an
> attempt to have a region granularity of 512 with 4 devices within a
> window that maps 2 host bridges at a granularity of 256 bytes:
> 
> HPA	DPA Offset	HB	Port	EP
> 0x0	0x0		0	0	0
> 0x100	0x0		1	0	2
> 0x200	0x100		0	0	0
> 0x300	0x100		1	0	2
> 0x400	0x200		0	1	1
> 0x500	0x200		1	1	3
> 0x600	0x300		0	1	1
> 0x700	0x300		1	1	3
> 0x800	0x400		0	0	0
> 0x900	0x400		1	0	2
> 0xA00	0x500		0	0	0
> 0xB00	0x500		1	0	2
> 
> Notice how endpoint0 maps HPA 0x0 and 0x200 correctly, but then HPA
> 0x800 results in DPA 0x200 to 0x400 on endpoint0 being not skipped.
> 
> Fix this by restricing the region granularity to be equal to the window
> granularity resulting in the following for a x4 region under a x2 window
> at a granularity of 256.
> 
> HPA	DPA Offset	HB	Port	EP
> 0x0	0x0		0	0	0
> 0x100	0x0		1	0	2
> 0x200	0x0		0	1	1
> 0x300	0x0		1	1	3
> 0x400	0x100		0	0	0
> 0x500	0x100		1	0	2
> 0x600	0x100		0	1	1
> 0x700	0x100		1	1	3
> 
> Not that it ever made practical sense to support region granularity >
> window granularity. The window rotates host bridges causing endpoints to
> never see a consecutive stream of requests at the desired granularity
> without breaks to issue cycles to the other host bridge.
> 
> Fixes: 80d10a6cee05 ("cxl/region: Add interleave geometry attributes")
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/core/region.c |   13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 641bc6344a4a..401148016978 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -395,13 +395,14 @@ static ssize_t interleave_granularity_store(struct device *dev,
>  		return rc;
>  
>  	/*
> -	 * Disallow region granularity less than root granularity to
> -	 * simplify the implementation. Otherwise, region's with a
> -	 * granularity less than the root interleave result in needing
> -	 * multiple endpoints to support a single slot in the
> -	 * interleave.
> +	 * When the host-bridge is interleaved, disallow region granularity !=
> +	 * root granularity. Regions with a granularity less than the root
> +	 * interleave result in needing multiple endpoints to support a single
> +	 * slot in the interleave (possible to suport in the future). Regions
> +	 * with a granularity greater than the root interleave result in invalid
> +	 * DPA translations (invalid to support).
>  	 */
> -	if (val < cxld->interleave_granularity)
> +	if (cxld->interleave_ways > 1 && val != cxld->interleave_granularity)
>  		return -EINVAL;
>  
>  	rc = down_write_killable(&cxl_region_rwsem);
> 

Re: [PATCH v2 2/3] cxl/region: Fix x1 interleave to greater than x1 interleave routing

[Jonathan Cameron]

On Fri, 05 Aug 2022 13:27:45 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> In cases where the decode fans out as it traverses downstream, the
> interleave granularity needs to increment to identify the port selector
> bits out of the remaining address bits. For example, recall that with an
> x2 parent port intereleave (IW == 1), the downstream decode for children
> of those ports will either see address bit IG+8 always set, or address
> bit IG+8 always clear. So if the child port needs to select a downstream
> port it can only use address bits starting at IG+9 (where IG and IW are
> the CXL encoded values for interleave granularity (ilog2(ig) - 8) and
> ways (ilog2(iw))).
> 
> When the parent port interleave is x1 no such masking occurs and the
> child port can maintain the granularity that was routed to the parent
> port.
> 
> Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Tested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> #via qemu

Now this is resolved, I'll get qemu fix sent out (hopefully later today).

Thanks,

Jonathan

> ---
>  drivers/cxl/core/region.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index e71077beb021..641bc6344a4a 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1025,7 +1025,11 @@ static int cxl_port_setup_targets(struct cxl_port *port,
>  		return rc;
>  	}
>  
> -	if (cxl_rr->nr_targets > 1) {
> +	/*
> +	 * If @parent_port is masking address bits, pick the next unused address
> +	 * bit to route @port's targets.
> +	 */
> +	if (parent_iw > 1 && cxl_rr->nr_targets > 1) {
>  		u32 address_bit = max(peig + peiw, eiw + peig);
>  
>  		eig = address_bit - eiw + 1;
> 

Re: [PATCH v2 1/3] cxl/region: Move HPA setup to cxl_region_attach()

[Jonathan Cameron]

On Fri, 05 Aug 2022 13:27:40 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> A recent bug fix added the setup of the endpoint decoder interleave
> geometry settings to cxl_region_attach(). Move the HPA setup there as
> well to keep all endpoint decoder parameter setting in a central
> location.
> 
> For symmetry, move endpoint HPA teardown to cxl_region_detach(), and for
> switches move HPA setup / teardown to cxl_port_{setup,reset}_targets().
> 
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
LGTM

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

I'm not yet hammering the detach path in my local testing, but
this looks unlikely to break anything.

Jonathan

> ---
>  drivers/cxl/core/hdm.c    |   26 ++------------------------
>  drivers/cxl/core/region.c |   24 ++++++++++++++++++++++--
>  2 files changed, 24 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 8143e2615957..e096f74e19df 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -499,28 +499,6 @@ static void cxld_set_type(struct cxl_decoder *cxld, u32 *ctrl)
>  			  CXL_HDM_DECODER0_CTRL_TYPE);
>  }
>  
> -static void cxld_set_hpa(struct cxl_decoder *cxld, u64 *base, u64 *size)
> -{
> -	struct cxl_region *cxlr = cxld->region;
> -	struct cxl_region_params *p = &cxlr->params;
> -
> -	cxld->hpa_range = (struct range) {
> -		.start = p->res->start,
> -		.end = p->res->end,
> -	};
> -
> -	*base = p->res->start;
> -	*size = resource_size(p->res);
> -}
> -
> -static void cxld_clear_hpa(struct cxl_decoder *cxld)
> -{
> -	cxld->hpa_range = (struct range) {
> -		.start = 0,
> -		.end = -1,
> -	};
> -}
> -
>  static int cxlsd_set_targets(struct cxl_switch_decoder *cxlsd, u64 *tgt)
>  {
>  	struct cxl_dport **t = &cxlsd->target[0];
> @@ -601,7 +579,8 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
>  	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
>  	cxld_set_interleave(cxld, &ctrl);
>  	cxld_set_type(cxld, &ctrl);
> -	cxld_set_hpa(cxld, &base, &size);
> +	base = cxld->hpa_range.start;
> +	size = range_len(&cxld->hpa_range);
>  
>  	writel(upper_32_bits(base), hdm + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(id));
>  	writel(lower_32_bits(base), hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(id));
> @@ -674,7 +653,6 @@ static int cxl_decoder_reset(struct cxl_decoder *cxld)
>  	ctrl &= ~CXL_HDM_DECODER0_CTRL_COMMIT;
>  	writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(id));
>  
> -	cxld_clear_hpa(cxld);
>  	writel(0, hdm + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(id));
>  	writel(0, hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(id));
>  	writel(0, hdm + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(id));
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 40f04c543e41..e71077beb021 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1044,6 +1044,10 @@ static int cxl_port_setup_targets(struct cxl_port *port,
>  
>  	cxld->interleave_ways = iw;
>  	cxld->interleave_granularity = ig;
> +	cxld->hpa_range = (struct range) {
> +		.start = p->res->start,
> +		.end = p->res->end,
> +	};
>  	dev_dbg(&cxlr->dev, "%s:%s iw: %d ig: %d\n", dev_name(port->uport),
>  		dev_name(&port->dev), iw, ig);
>  add_target:
> @@ -1070,13 +1074,21 @@ static void cxl_port_reset_targets(struct cxl_port *port,
>  				   struct cxl_region *cxlr)
>  {
>  	struct cxl_region_ref *cxl_rr = cxl_rr_load(port, cxlr);
> +	struct cxl_decoder *cxld;
>  
>  	/*
>  	 * After the last endpoint has been detached the entire cxl_rr may now
>  	 * be gone.
>  	 */
> -	if (cxl_rr)
> -		cxl_rr->nr_targets_set = 0;
> +	if (!cxl_rr)
> +		return;
> +	cxl_rr->nr_targets_set = 0;
> +
> +	cxld = cxl_rr->decoder;
> +	cxld->hpa_range = (struct range) {
> +		.start = 0,
> +		.end = -1,
> +	};
>  }
>  
>  static void cxl_region_teardown_targets(struct cxl_region *cxlr)
> @@ -1257,6 +1269,10 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  
>  	cxled->cxld.interleave_ways = p->interleave_ways;
>  	cxled->cxld.interleave_granularity = p->interleave_granularity;
> +	cxled->cxld.hpa_range = (struct range) {
> +		.start = p->res->start,
> +		.end = p->res->end,
> +	};
>  
>  	return 0;
>  
> @@ -1315,6 +1331,10 @@ static int cxl_region_detach(struct cxl_endpoint_decoder *cxled)
>  	}
>  	p->targets[cxled->pos] = NULL;
>  	p->nr_targets--;
> +	cxled->cxld.hpa_range = (struct range) {
> +		.start = 0,
> +		.end = -1,
> +	};
>  
>  	/* notify the region driver that one of its targets has departed */
>  	up_write(&cxl_region_rwsem);
> 

Re: [PATCH v2 3/3] cxl/region: Disallow region granularity != window granularity

[Jonathan Cameron]

On Fri, 05 Aug 2022 13:27:51 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> The endpoint decode granularity must be <= the window granularity
> otherwise capacity in the endpoints is lost in the decode. Consider an
> attempt to have a region granularity of 512 with 4 devices within a
> window that maps 2 host bridges at a granularity of 256 bytes:
> 
> HPA	DPA Offset	HB	Port	EP
> 0x0	0x0		0	0	0
> 0x100	0x0		1	0	2
> 0x200	0x100		0	0	0
> 0x300	0x100		1	0	2
> 0x400	0x200		0	1	1
> 0x500	0x200		1	1	3
> 0x600	0x300		0	1	1
> 0x700	0x300		1	1	3
> 0x800	0x400		0	0	0
> 0x900	0x400		1	0	2
> 0xA00	0x500		0	0	0
> 0xB00	0x500		1	0	2
> 
> Notice how endpoint0 maps HPA 0x0 and 0x200 correctly, but then HPA
> 0x800 results in DPA 0x200 to 0x400 on endpoint0 being not skipped.
> 
> Fix this by restricing the region granularity to be equal to the window

restricting

> granularity resulting in the following for a x4 region under a x2 window
> at a granularity of 256.
> 
> HPA	DPA Offset	HB	Port	EP
> 0x0	0x0		0	0	0
> 0x100	0x0		1	0	2
> 0x200	0x0		0	1	1
> 0x300	0x0		1	1	3
> 0x400	0x100		0	0	0
> 0x500	0x100		1	0	2
> 0x600	0x100		0	1	1
> 0x700	0x100		1	1	3
> 
> Not that it ever made practical sense to support region granularity >
> window granularity. The window rotates host bridges causing endpoints to
> never see a consecutive stream of requests at the desired granularity
> without breaks to issue cycles to the other host bridge.

Possibly worth description mentioning the (possible to support in the
future) case you have in the comment in the code.  I'm not that fussed
though either way.

>              (Regions with a granularity less than the root
> +	 * interleave result in needing multiple endpoints to support a single
> +	 * slot in the interleave)

Last bit is interesting and based on region granularity is always
constant across different EPs (currently interface enforces that - not as
far as I can tell the spec) I think the spec would allow:
HB0: 2 EP, HB1 1EP with HB interleave at 256 bytes (interleave irrelevant
for HB1 as it only has one port) and CFMWS at 512.

HPA	DPA Offset	HB	Port	EP
0x0	0x0		0	0	0
0x100	0x0		0	1	1
0x200	0x0		1	0	2
0x300	0x100		1	0	2
0x400	0x100		0	0	0
0x500	0x100		0	1	1
0x600	0x200		1	0	2
0x700	0x300		1	0	2
0x800	0x200		0	0	0
0x900	0x200		0	1	1
0xa00	0x400		1	0	2
0xb00	0x500		1	0	2

Fun bit is region ends up having granularity 256 on some devices and 512 on
others.  Perhaps better to not mention this (assuming I haven't made
an error in the maths) :) So I'm fine with leaving the comment as is
given it's correct for current combination of software model and what
the spec allows.

> 
> Fixes: 80d10a6cee05 ("cxl/region: Add interleave geometry attributes")
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/core/region.c |   13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 641bc6344a4a..401148016978 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -395,13 +395,14 @@ static ssize_t interleave_granularity_store(struct device *dev,
>  		return rc;
>  
>  	/*
> -	 * Disallow region granularity less than root granularity to
> -	 * simplify the implementation. Otherwise, region's with a
> -	 * granularity less than the root interleave result in needing
> -	 * multiple endpoints to support a single slot in the
> -	 * interleave.
> +	 * When the host-bridge is interleaved, disallow region granularity !=
> +	 * root granularity. Regions with a granularity less than the root
> +	 * interleave result in needing multiple endpoints to support a single
> +	 * slot in the interleave (possible to suport in the future). Regions
> +	 * with a granularity greater than the root interleave result in invalid
> +	 * DPA translations (invalid to support).
>  	 */
> -	if (val < cxld->interleave_granularity)
> +	if (cxld->interleave_ways > 1 && val != cxld->interleave_granularity)
>  		return -EINVAL;
>  
>  	rc = down_write_killable(&cxl_region_rwsem);
> 

Re: [PATCH v2 2/3] cxl/region: Fix x1 interleave to greater than x1 interleave routing

[Dan Williams]

Jonathan Cameron wrote:
> On Fri, 05 Aug 2022 13:27:45 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > In cases where the decode fans out as it traverses downstream, the
> > interleave granularity needs to increment to identify the port selector
> > bits out of the remaining address bits. For example, recall that with an
> > x2 parent port intereleave (IW == 1), the downstream decode for children
> > of those ports will either see address bit IG+8 always set, or address
> > bit IG+8 always clear. So if the child port needs to select a downstream
> > port it can only use address bits starting at IG+9 (where IG and IW are
> > the CXL encoded values for interleave granularity (ilog2(ig) - 8) and
> > ways (ilog2(iw))).
> > 
> > When the parent port interleave is x1 no such masking occurs and the
> > child port can maintain the granularity that was routed to the parent
> > port.
> > 
> > Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Tested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> #via qemu

Thanks Jonathan.

In the interests of being able to make a:

"these commits have appeared in Linux next with no known outstanding
issues"

...I'd like to note these review and test tags in the merge message.
They will still be in the history just not in the commits directly.
Similar to what I did here:

https://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git/tag/?h=libnvdimm-for-5.17

The current state of cxl/next hit linux-next last night. Let me know if
you have any heartburn about that.

> Now this is resolved, I'll get qemu fix sent out (hopefully later today).

Good to hear, thanks.

Re: [PATCH v2 2/3] cxl/region: Fix x1 interleave to greater than x1 interleave routing

[Jonathan Cameron]

On Mon, 8 Aug 2022 12:28:36 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Jonathan Cameron wrote:
> > On Fri, 05 Aug 2022 13:27:45 -0700
> > Dan Williams <dan.j.williams@intel.com> wrote:
> >   
> > > In cases where the decode fans out as it traverses downstream, the
> > > interleave granularity needs to increment to identify the port selector
> > > bits out of the remaining address bits. For example, recall that with an
> > > x2 parent port intereleave (IW == 1), the downstream decode for children
> > > of those ports will either see address bit IG+8 always set, or address
> > > bit IG+8 always clear. So if the child port needs to select a downstream
> > > port it can only use address bits starting at IG+9 (where IG and IW are
> > > the CXL encoded values for interleave granularity (ilog2(ig) - 8) and
> > > ways (ilog2(iw))).
> > > 
> > > When the parent port interleave is x1 no such masking occurs and the
> > > child port can maintain the granularity that was routed to the parent
> > > port.
> > > 
> > > Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>  
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Tested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> #via qemu  
> 
> Thanks Jonathan.
> 
> In the interests of being able to make a:
> 
> "these commits have appeared in Linux next with no known outstanding
> issues"
> 
> ...I'd like to note these review and test tags in the merge message.
> They will still be in the history just not in the commits directly.
> Similar to what I did here:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git/tag/?h=libnvdimm-for-5.17
> 
> The current state of cxl/next hit linux-next last night. Let me know if
> you have any heartburn about that.

Fine by me.

I normally don't even do that if I'm rushing something in. Just rely on
the link to the thread in the actual patch description for anyone who cares.
Maybe I'll start doing something like you are, though can be a pain to gather them
up on a large pull request.

Jonathan

> 
> > Now this is resolved, I'll get qemu fix sent out (hopefully later today).  
> 
> Good to hear, thanks.