[PATCH 0/3] Add sanity check for interleave setup

[PATCH 0/3] Add sanity check for interleave setup

[Dave Jiang]

The small series adds sanity check for the combination of interleave ways
and interleave granularity during region and port configuration. The
calculation references CXL spec 3.0 8.2.4.19.13 implementation note #3. The
checks also added HDM CAP retrieval for the support of new interleave ways
where 3, 6, and 12 ways support as well as 16 ways support.

---

Dave Jiang (3):
      cxl: Add check for result of interleave ways plus granularity combo
      cxl: Add CXL spec v3.0 interleave support
      tools/testing/cxl: Add interleave check support to mock cxl port device


 drivers/cxl/core/hdm.c       |  4 +++
 drivers/cxl/core/region.c    | 17 +++++++++-
 drivers/cxl/cxl.h            | 13 ++++++++
 drivers/cxl/cxlmem.h         | 60 ++++++++++++++++++++++++++++++++++++
 tools/testing/cxl/test/cxl.c |  2 ++
 5 files changed, 95 insertions(+), 1 deletion(-)

--

[PATCH 1/3] cxl: Add check for result of interleave ways plus granularity combo

[Dave Jiang]

Add a helper function to check the combination of interleave ways and
interleave granularity together is sane against the interleave mask from
the HDM decoder. Add the check to cxl_region_attach() to make sure the
region config is sane. Add the check to cxl_port_setup_targets() to make
sure the port setup config is also sane.

Calculation refers to CXL spec v3 8.2.4.19.13 implementation note #3.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/region.c |   17 ++++++++++++++++-
 drivers/cxl/cxl.h         |   11 +++++++++++
 drivers/cxl/cxlmem.h      |   31 +++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index cf5d5811fe4c..a209a8de31fd 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1081,6 +1081,13 @@ static int cxl_port_setup_targets(struct cxl_port *port,
 		return rc;
 	}
 
+	rc = cxl_interleave_verify(port, iw, ig);
+	if (rc) {
+		dev_dbg(&cxlr->dev, "%s:%s: invalid interleave & granularity combo: %d\n",
+			dev_name(port->uport), dev_name(&port->dev), rc);
+		return rc;
+	}
+
 	cxld->interleave_ways = iw;
 	cxld->interleave_granularity = ig;
 	dev_dbg(&cxlr->dev, "%s:%s iw: %d ig: %d\n", dev_name(port->uport),
@@ -1218,6 +1225,15 @@ static int cxl_region_attach(struct cxl_region *cxlr,
 		return -EBUSY;
 	}
 
+	ep_port = cxled_to_port(cxled);
+	rc = cxl_interleave_verify(ep_port, p->interleave_ways,
+				   p->interleave_granularity);
+	if (rc) {
+		dev_dbg(&cxlr->dev, "%s: invalid interleave & granularity combo: %d\n",
+			dev_name(&cxlmd->dev), rc);
+		return rc;
+	}
+
 	for (i = 0; i < p->interleave_ways; i++) {
 		struct cxl_endpoint_decoder *cxled_target;
 		struct cxl_memdev *cxlmd_target;
@@ -1236,7 +1252,6 @@ static int cxl_region_attach(struct cxl_region *cxlr,
 		}
 	}
 
-	ep_port = cxled_to_port(cxled);
 	root_port = cxlrd_to_port(cxlrd);
 	dport = cxl_find_dport_by_dev(root_port, ep_port->host_bridge);
 	if (!dport) {
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index bc604b7e44fb..275979fbd15a 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -61,6 +61,17 @@
 #define CXL_HDM_DECODER0_SKIP_LOW(i) CXL_HDM_DECODER0_TL_LOW(i)
 #define CXL_HDM_DECODER0_SKIP_HIGH(i) CXL_HDM_DECODER0_TL_HIGH(i)
 
+enum {
+	CXL_INTERLEAVE_1_WAY = 0,
+	CXL_INTERLEAVE_2_WAY,
+	CXL_INTERLEAVE_4_WAY,
+	CXL_INTERLEAVE_8_WAY,
+	CXL_INTERLEAVE_16_WAY,
+	CXL_INTERLEAVE_3_WAY = 8,
+	CXL_INTERLEAVE_6_WAY,
+	CXL_INTERLEAVE_12_WAY
+};
+
 static inline int cxl_hdm_decoder_count(u32 cap_hdr)
 {
 	int val = FIELD_GET(CXL_HDM_DECODER_COUNT_MASK, cap_hdr);
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 88e3a8e54b6a..d5f872ca62f9 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -401,6 +401,37 @@ struct cxl_hdm {
 	struct cxl_port *port;
 };
 
+static inline int cxl_interleave_verify(struct cxl_port *port,
+					int ways, int granularity)
+{
+	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
+	unsigned int addr_mask;
+	u16 ig;
+	u8 iw;
+	int rc;
+
+	rc = granularity_to_cxl(granularity, &ig);
+	if (rc)
+		return rc;
+
+	rc = ways_to_cxl(ways, &iw);
+	if (rc)
+		return rc;
+
+	if (iw == 0)
+		return 0;
+
+	if (iw < CXL_INTERLEAVE_3_WAY)
+		addr_mask = GENMASK(ig + 8 + iw - 1, ig + 8);
+	else
+		addr_mask = GENMASK((ig + iw) / 3 - 1, ig + 8);
+
+	if (~cxlhdm->interleave_mask & addr_mask)
+		return -EINVAL;
+
+	return 0;
+}
+
 struct seq_file;
 struct dentry *cxl_debugfs_create_dir(const char *dir);
 void cxl_dpa_debug(struct seq_file *file, struct cxl_dev_state *cxlds);

[PATCH 2/3] cxl: Add CXL spec v3.0 interleave support

[Dave Jiang]

CXL spec v3.0 added 2 CAP bits to the CXL HDM Decoder Capability Register.
CXL spec v3.0 8.2.4.19.1. Bit 11 indicates that 3, 6, and 12 way interleave
is capable. Bit 12 indicates that 16 way interleave is capable.

Add code to parse_hdm_decoder_caps() to cache those new bits. Add check in
cxl_interleave_verify() call to make sure those CAP bits matches the passed
in interleave value.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/hdm.c |    4 ++++
 drivers/cxl/cxl.h      |    2 ++
 drivers/cxl/cxlmem.h   |   29 +++++++++++++++++++++++++++++
 3 files changed, 35 insertions(+)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 8143e2615957..50ff7387e425 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -80,6 +80,10 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
 		cxlhdm->interleave_mask |= GENMASK(11, 8);
 	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_14_12, hdm_cap))
 		cxlhdm->interleave_mask |= GENMASK(14, 12);
+	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY, hdm_cap))
+		cxlhdm->interleave_3_6_12 = true;
+	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_16_WAY, hdm_cap))
+		cxlhdm->interleave_16 = true;
 }
 
 static void __iomem *map_hdm_decoder_regs(struct cxl_port *port,
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 275979fbd15a..db9631d09dd0 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -42,6 +42,8 @@
 #define   CXL_HDM_DECODER_TARGET_COUNT_MASK GENMASK(7, 4)
 #define   CXL_HDM_DECODER_INTERLEAVE_11_8 BIT(8)
 #define   CXL_HDM_DECODER_INTERLEAVE_14_12 BIT(9)
+#define   CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY BIT(11)
+#define   CXL_HDM_DECODER_INTERLEAVE_16_WAY BIT(12)
 #define CXL_HDM_DECODER_CTRL_OFFSET 0x4
 #define   CXL_HDM_DECODER_ENABLE BIT(1)
 #define CXL_HDM_DECODER0_BASE_LOW_OFFSET(i) (0x20 * (i) + 0x10)
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index d5f872ca62f9..9b4b23b3b78a 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -398,9 +398,35 @@ struct cxl_hdm {
 	unsigned int decoder_count;
 	unsigned int target_count;
 	unsigned int interleave_mask;
+	bool interleave_3_6_12;
+	bool interleave_16;
 	struct cxl_port *port;
 };
 
+static inline bool valid_interleave(struct cxl_hdm *cxlhdm, u8 iw)
+{
+	switch (iw) {
+	case CXL_INTERLEAVE_1_WAY:
+	case CXL_INTERLEAVE_2_WAY:
+	case CXL_INTERLEAVE_4_WAY:
+	case CXL_INTERLEAVE_8_WAY:
+		return true;
+	case CXL_INTERLEAVE_16_WAY:
+		if (!cxlhdm->interleave_16)
+			return false;
+		return true;
+	case CXL_INTERLEAVE_3_WAY:
+	case CXL_INTERLEAVE_6_WAY:
+	case CXL_INTERLEAVE_12_WAY:
+		if (!cxlhdm->interleave_3_6_12)
+			return false;
+		return true;
+	default:
+	};
+
+	return false;
+}
+
 static inline int cxl_interleave_verify(struct cxl_port *port,
 					int ways, int granularity)
 {
@@ -421,6 +447,9 @@ static inline int cxl_interleave_verify(struct cxl_port *port,
 	if (iw == 0)
 		return 0;
 
+	if (!valid_interleave(cxlhdm, iw))
+		return -EINVAL;
+
 	if (iw < CXL_INTERLEAVE_3_WAY)
 		addr_mask = GENMASK(ig + 8 + iw - 1, ig + 8);
 	else

[PATCH 3/3] tools/testing/cxl: Add interleave check support to mock cxl port device

[Dave Jiang]

Attach the cxl mock hdm to the port device to allow cxl_interleave_verify()
to check the interleave configuration. Set the interleave_mask as well
to support the new verification code.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 tools/testing/cxl/test/cxl.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
index a072b2d3e726..5a9f33703ee7 100644
--- a/tools/testing/cxl/test/cxl.c
+++ b/tools/testing/cxl/test/cxl.c
@@ -398,6 +398,8 @@ static struct cxl_hdm *mock_cxl_setup_hdm(struct cxl_port *port)
 		return ERR_PTR(-ENOMEM);
 
 	cxlhdm->port = port;
+	cxlhdm->interleave_mask = GENMASK(14, 8);
+	dev_set_drvdata(&port->dev, cxlhdm);
 	return cxlhdm;
 }
 

RE: [PATCH 1/3] cxl: Add check for result of interleave ways plus granularity combo

[Dan Williams]

Dave Jiang wrote:
> Add a helper function to check the combination of interleave ways and
> interleave granularity together is sane against the interleave mask from
> the HDM decoder. Add the check to cxl_region_attach() to make sure the
> region config is sane. Add the check to cxl_port_setup_targets() to make
> sure the port setup config is also sane.
> 
> Calculation refers to CXL spec v3 8.2.4.19.13 implementation note #3.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/region.c |   17 ++++++++++++++++-
>  drivers/cxl/cxl.h         |   11 +++++++++++
>  drivers/cxl/cxlmem.h      |   31 +++++++++++++++++++++++++++++++
>  3 files changed, 58 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index cf5d5811fe4c..a209a8de31fd 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1081,6 +1081,13 @@ static int cxl_port_setup_targets(struct cxl_port *port,
>  		return rc;
>  	}
>  
> +	rc = cxl_interleave_verify(port, iw, ig);

There are multiple "interleave verify" actions, this function is just
handling the interleave address bit capability, so how about:

s/cxl_interleave_verify/cxl_interleave_capable/

> +	if (rc) {
> +		dev_dbg(&cxlr->dev, "%s:%s: invalid interleave & granularity combo: %d\n",

If this fires I would want to know the iw and ig settings, something
like:

    "%s:%s interleave (ig: %d iw: %d mask: %#x) exceeds capability (mask: %#x)\n"

Likely that message would need to move internal to
cxl_interleave_capable() where you have the address masks available.


> +			dev_name(port->uport), dev_name(&port->dev), rc);
> +		return rc;
> +	}
> +
>  	cxld->interleave_ways = iw;
>  	cxld->interleave_granularity = ig;
>  	dev_dbg(&cxlr->dev, "%s:%s iw: %d ig: %d\n", dev_name(port->uport),
> @@ -1218,6 +1225,15 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  		return -EBUSY;
>  	}
>  
> +	ep_port = cxled_to_port(cxled);
> +	rc = cxl_interleave_verify(ep_port, p->interleave_ways,
> +				   p->interleave_granularity);
> +	if (rc) {
> +		dev_dbg(&cxlr->dev, "%s: invalid interleave & granularity combo: %d\n",
> +			dev_name(&cxlmd->dev), rc);

...and then you don't need to duplicate the message if it is internal to
cxl_interleave_capable().

> +		return rc;
> +	}
> +
>  	for (i = 0; i < p->interleave_ways; i++) {
>  		struct cxl_endpoint_decoder *cxled_target;
>  		struct cxl_memdev *cxlmd_target;
> @@ -1236,7 +1252,6 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  		}
>  	}
>  
> -	ep_port = cxled_to_port(cxled);
>  	root_port = cxlrd_to_port(cxlrd);
>  	dport = cxl_find_dport_by_dev(root_port, ep_port->host_bridge);
>  	if (!dport) {
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index bc604b7e44fb..275979fbd15a 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -61,6 +61,17 @@
>  #define CXL_HDM_DECODER0_SKIP_LOW(i) CXL_HDM_DECODER0_TL_LOW(i)
>  #define CXL_HDM_DECODER0_SKIP_HIGH(i) CXL_HDM_DECODER0_TL_HIGH(i)
>  
> +enum {
> +	CXL_INTERLEAVE_1_WAY = 0,
> +	CXL_INTERLEAVE_2_WAY,
> +	CXL_INTERLEAVE_4_WAY,
> +	CXL_INTERLEAVE_8_WAY,
> +	CXL_INTERLEAVE_16_WAY,
> +	CXL_INTERLEAVE_3_WAY = 8,
> +	CXL_INTERLEAVE_6_WAY,
> +	CXL_INTERLEAVE_12_WAY

I'm not sure this new enum is worth it given only one of these will ever
be used.

> +};
> +
>  static inline int cxl_hdm_decoder_count(u32 cap_hdr)
>  {
>  	int val = FIELD_GET(CXL_HDM_DECODER_COUNT_MASK, cap_hdr);
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 88e3a8e54b6a..d5f872ca62f9 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -401,6 +401,37 @@ struct cxl_hdm {
>  	struct cxl_port *port;
>  };
>  
> +static inline int cxl_interleave_verify(struct cxl_port *port,
> +					int ways, int granularity)
> +{
> +	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
> +	unsigned int addr_mask;
> +	u16 ig;
> +	u8 iw;
> +	int rc;
> +
> +	rc = granularity_to_cxl(granularity, &ig);
> +	if (rc)
> +		return rc;
> +
> +	rc = ways_to_cxl(ways, &iw);
> +	if (rc)
> +		return rc;
> +
> +	if (iw == 0)
> +		return 0;
> +
> +	if (iw < CXL_INTERLEAVE_3_WAY)

...just do "is_power_of_2(iw)" here instead.

> +		addr_mask = GENMASK(ig + 8 + iw - 1, ig + 8);
> +	else
> +		addr_mask = GENMASK((ig + iw) / 3 - 1, ig + 8);

RE: [PATCH 2/3] cxl: Add CXL spec v3.0 interleave support

[Dan Williams]

Dave Jiang wrote:
> CXL spec v3.0 added 2 CAP bits to the CXL HDM Decoder Capability Register.
> CXL spec v3.0 8.2.4.19.1. Bit 11 indicates that 3, 6, and 12 way interleave
> is capable. Bit 12 indicates that 16 way interleave is capable.
> 
> Add code to parse_hdm_decoder_caps() to cache those new bits. Add check in
> cxl_interleave_verify() call to make sure those CAP bits matches the passed
> in interleave value.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/hdm.c |    4 ++++
>  drivers/cxl/cxl.h      |    2 ++
>  drivers/cxl/cxlmem.h   |   29 +++++++++++++++++++++++++++++
>  3 files changed, 35 insertions(+)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 8143e2615957..50ff7387e425 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -80,6 +80,10 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
>  		cxlhdm->interleave_mask |= GENMASK(11, 8);
>  	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_14_12, hdm_cap))
>  		cxlhdm->interleave_mask |= GENMASK(14, 12);
> +	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY, hdm_cap))
> +		cxlhdm->interleave_3_6_12 = true;
> +	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_16_WAY, hdm_cap))
> +		cxlhdm->interleave_16 = true;
>  }
>  
>  static void __iomem *map_hdm_decoder_regs(struct cxl_port *port,
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 275979fbd15a..db9631d09dd0 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -42,6 +42,8 @@
>  #define   CXL_HDM_DECODER_TARGET_COUNT_MASK GENMASK(7, 4)
>  #define   CXL_HDM_DECODER_INTERLEAVE_11_8 BIT(8)
>  #define   CXL_HDM_DECODER_INTERLEAVE_14_12 BIT(9)
> +#define   CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY BIT(11)
> +#define   CXL_HDM_DECODER_INTERLEAVE_16_WAY BIT(12)
>  #define CXL_HDM_DECODER_CTRL_OFFSET 0x4
>  #define   CXL_HDM_DECODER_ENABLE BIT(1)
>  #define CXL_HDM_DECODER0_BASE_LOW_OFFSET(i) (0x20 * (i) + 0x10)
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index d5f872ca62f9..9b4b23b3b78a 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -398,9 +398,35 @@ struct cxl_hdm {
>  	unsigned int decoder_count;
>  	unsigned int target_count;
>  	unsigned int interleave_mask;
> +	bool interleave_3_6_12;
> +	bool interleave_16;
>  	struct cxl_port *port;
>  };
>  
> +static inline bool valid_interleave(struct cxl_hdm *cxlhdm, u8 iw)
> +{
> +	switch (iw) {
> +	case CXL_INTERLEAVE_1_WAY:
> +	case CXL_INTERLEAVE_2_WAY:
> +	case CXL_INTERLEAVE_4_WAY:
> +	case CXL_INTERLEAVE_8_WAY:
> +		return true;
> +	case CXL_INTERLEAVE_16_WAY:
> +		if (!cxlhdm->interleave_16)
> +			return false;
> +		return true;
> +	case CXL_INTERLEAVE_3_WAY:
> +	case CXL_INTERLEAVE_6_WAY:
> +	case CXL_INTERLEAVE_12_WAY:
> +		if (!cxlhdm->interleave_3_6_12)
> +			return false;
> +		return true;
> +	default:
> +	};
> +
> +	return false;

I'd prefer something more compact like

    test_bit(iw, &port->interleave_cap)

...where interleave_cap is just a mask with bits 1,2,3,4,6,8,12,16
optionally set.

RE: [PATCH 3/3] tools/testing/cxl: Add interleave check support to mock cxl port device

[Dan Williams]

Dave Jiang wrote:
> Attach the cxl mock hdm to the port device to allow cxl_interleave_verify()
> to check the interleave configuration. Set the interleave_mask as well
> to support the new verification code.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  tools/testing/cxl/test/cxl.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
> index a072b2d3e726..5a9f33703ee7 100644
> --- a/tools/testing/cxl/test/cxl.c
> +++ b/tools/testing/cxl/test/cxl.c
> @@ -398,6 +398,8 @@ static struct cxl_hdm *mock_cxl_setup_hdm(struct cxl_port *port)
>  		return ERR_PTR(-ENOMEM);
>  
>  	cxlhdm->port = port;
> +	cxlhdm->interleave_mask = GENMASK(14, 8);
> +	dev_set_drvdata(&port->dev, cxlhdm);

This would also need to set cxlhdm->interleave_cap.

I accidentally called it port->interleave_cap in the last mail, but it
belong in cxlhdm.

Re: [PATCH 1/3] cxl: Add check for result of interleave ways plus granularity combo

[Alison Schofield]

Dave - Haven't reviewed yet, but wanted to drop this tidbit - 

On Mon, Aug 08, 2022 at 02:06:57PM -0700, Dave Jiang wrote:
> Add a helper function to check the combination of interleave ways and
> interleave granularity together is sane against the interleave mask from
> the HDM decoder. Add the check to cxl_region_attach() to make sure the
> region config is sane. Add the check to cxl_port_setup_targets() to make
> sure the port setup config is also sane.
> 
> Calculation refers to CXL spec v3 8.2.4.19.13 implementation note #3.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

snip

> +static inline int cxl_interleave_verify(struct cxl_port *port,
> +					int ways, int granularity)
> +{
> +	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
> +	unsigned int addr_mask;
> +	u16 ig;
> +	u8 iw;

s/ig/eig
s/iw/eiw
For consistency, let's use the "e" version of these names to mean
encoded value.  

> +	int rc;
> +
> +	rc = granularity_to_cxl(granularity, &ig);
> +	if (rc)
> +		return rc;
> +
> +	rc = ways_to_cxl(ways, &iw);
> +	if (rc)
> +		return rc;
> +
> +	if (iw == 0)
> +		return 0;
> +
> +	if (iw < CXL_INTERLEAVE_3_WAY)
> +		addr_mask = GENMASK(ig + 8 + iw - 1, ig + 8);
> +	else
> +		addr_mask = GENMASK((ig + iw) / 3 - 1, ig + 8);
> +
> +	if (~cxlhdm->interleave_mask & addr_mask)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  struct seq_file;
>  struct dentry *cxl_debugfs_create_dir(const char *dir);
>  void cxl_dpa_debug(struct seq_file *file, struct cxl_dev_state *cxlds);
> 
> 

Re: [PATCH 1/3] cxl: Add check for result of interleave ways plus granularity combo

[Dave Jiang]

On 8/9/2022 9:18 AM, Dan Williams wrote:
> Dave Jiang wrote:
>> Add a helper function to check the combination of interleave ways and
>> interleave granularity together is sane against the interleave mask from
>> the HDM decoder. Add the check to cxl_region_attach() to make sure the
>> region config is sane. Add the check to cxl_port_setup_targets() to make
>> sure the port setup config is also sane.
>>
>> Calculation refers to CXL spec v3 8.2.4.19.13 implementation note #3.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   drivers/cxl/core/region.c |   17 ++++++++++++++++-
>>   drivers/cxl/cxl.h         |   11 +++++++++++
>>   drivers/cxl/cxlmem.h      |   31 +++++++++++++++++++++++++++++++
>>   3 files changed, 58 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index cf5d5811fe4c..a209a8de31fd 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -1081,6 +1081,13 @@ static int cxl_port_setup_targets(struct cxl_port *port,
>>   		return rc;
>>   	}
>>   
>> +	rc = cxl_interleave_verify(port, iw, ig);
> There are multiple "interleave verify" actions, this function is just
> handling the interleave address bit capability, so how about:
>
> s/cxl_interleave_verify/cxl_interleave_capable/
ok
>
>> +	if (rc) {
>> +		dev_dbg(&cxlr->dev, "%s:%s: invalid interleave & granularity combo: %d\n",
> If this fires I would want to know the iw and ig settings, something
> like:
>
>      "%s:%s interleave (ig: %d iw: %d mask: %#x) exceeds capability (mask: %#x)\n"
>
> Likely that message would need to move internal to
> cxl_interleave_capable() where you have the address masks available.
Will move it internally
>
>
>> +			dev_name(port->uport), dev_name(&port->dev), rc);
>> +		return rc;
>> +	}
>> +
>>   	cxld->interleave_ways = iw;
>>   	cxld->interleave_granularity = ig;
>>   	dev_dbg(&cxlr->dev, "%s:%s iw: %d ig: %d\n", dev_name(port->uport),
>> @@ -1218,6 +1225,15 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>>   		return -EBUSY;
>>   	}
>>   
>> +	ep_port = cxled_to_port(cxled);
>> +	rc = cxl_interleave_verify(ep_port, p->interleave_ways,
>> +				   p->interleave_granularity);
>> +	if (rc) {
>> +		dev_dbg(&cxlr->dev, "%s: invalid interleave & granularity combo: %d\n",
>> +			dev_name(&cxlmd->dev), rc);
> ...and then you don't need to duplicate the message if it is internal to
> cxl_interleave_capable().
>
>> +		return rc;
>> +	}
>> +
>>   	for (i = 0; i < p->interleave_ways; i++) {
>>   		struct cxl_endpoint_decoder *cxled_target;
>>   		struct cxl_memdev *cxlmd_target;
>> @@ -1236,7 +1252,6 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>>   		}
>>   	}
>>   
>> -	ep_port = cxled_to_port(cxled);
>>   	root_port = cxlrd_to_port(cxlrd);
>>   	dport = cxl_find_dport_by_dev(root_port, ep_port->host_bridge);
>>   	if (!dport) {
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index bc604b7e44fb..275979fbd15a 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -61,6 +61,17 @@
>>   #define CXL_HDM_DECODER0_SKIP_LOW(i) CXL_HDM_DECODER0_TL_LOW(i)
>>   #define CXL_HDM_DECODER0_SKIP_HIGH(i) CXL_HDM_DECODER0_TL_HIGH(i)
>>   
>> +enum {
>> +	CXL_INTERLEAVE_1_WAY = 0,
>> +	CXL_INTERLEAVE_2_WAY,
>> +	CXL_INTERLEAVE_4_WAY,
>> +	CXL_INTERLEAVE_8_WAY,
>> +	CXL_INTERLEAVE_16_WAY,
>> +	CXL_INTERLEAVE_3_WAY = 8,
>> +	CXL_INTERLEAVE_6_WAY,
>> +	CXL_INTERLEAVE_12_WAY
> I'm not sure this new enum is worth it given only one of these will ever
> be used.
Will remove
>
>> +};
>> +
>>   static inline int cxl_hdm_decoder_count(u32 cap_hdr)
>>   {
>>   	int val = FIELD_GET(CXL_HDM_DECODER_COUNT_MASK, cap_hdr);
>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
>> index 88e3a8e54b6a..d5f872ca62f9 100644
>> --- a/drivers/cxl/cxlmem.h
>> +++ b/drivers/cxl/cxlmem.h
>> @@ -401,6 +401,37 @@ struct cxl_hdm {
>>   	struct cxl_port *port;
>>   };
>>   
>> +static inline int cxl_interleave_verify(struct cxl_port *port,
>> +					int ways, int granularity)
>> +{
>> +	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
>> +	unsigned int addr_mask;
>> +	u16 ig;
>> +	u8 iw;
>> +	int rc;
>> +
>> +	rc = granularity_to_cxl(granularity, &ig);
>> +	if (rc)
>> +		return rc;
>> +
>> +	rc = ways_to_cxl(ways, &iw);
>> +	if (rc)
>> +		return rc;
>> +
>> +	if (iw == 0)
>> +		return 0;
>> +
>> +	if (iw < CXL_INTERLEAVE_3_WAY)
> ...just do "is_power_of_2(iw)" here instead.

ok

>
>> +		addr_mask = GENMASK(ig + 8 + iw - 1, ig + 8);
>> +	else
>> +		addr_mask = GENMASK((ig + iw) / 3 - 1, ig + 8);

Re: [PATCH 2/3] cxl: Add CXL spec v3.0 interleave support

[Dave Jiang]

On 8/9/2022 9:20 AM, Dan Williams wrote:
> Dave Jiang wrote:
>> CXL spec v3.0 added 2 CAP bits to the CXL HDM Decoder Capability Register.
>> CXL spec v3.0 8.2.4.19.1. Bit 11 indicates that 3, 6, and 12 way interleave
>> is capable. Bit 12 indicates that 16 way interleave is capable.
>>
>> Add code to parse_hdm_decoder_caps() to cache those new bits. Add check in
>> cxl_interleave_verify() call to make sure those CAP bits matches the passed
>> in interleave value.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   drivers/cxl/core/hdm.c |    4 ++++
>>   drivers/cxl/cxl.h      |    2 ++
>>   drivers/cxl/cxlmem.h   |   29 +++++++++++++++++++++++++++++
>>   3 files changed, 35 insertions(+)
>>
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index 8143e2615957..50ff7387e425 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
>> @@ -80,6 +80,10 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
>>   		cxlhdm->interleave_mask |= GENMASK(11, 8);
>>   	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_14_12, hdm_cap))
>>   		cxlhdm->interleave_mask |= GENMASK(14, 12);
>> +	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY, hdm_cap))
>> +		cxlhdm->interleave_3_6_12 = true;
>> +	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_16_WAY, hdm_cap))
>> +		cxlhdm->interleave_16 = true;
>>   }
>>   
>>   static void __iomem *map_hdm_decoder_regs(struct cxl_port *port,
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index 275979fbd15a..db9631d09dd0 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -42,6 +42,8 @@
>>   #define   CXL_HDM_DECODER_TARGET_COUNT_MASK GENMASK(7, 4)
>>   #define   CXL_HDM_DECODER_INTERLEAVE_11_8 BIT(8)
>>   #define   CXL_HDM_DECODER_INTERLEAVE_14_12 BIT(9)
>> +#define   CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY BIT(11)
>> +#define   CXL_HDM_DECODER_INTERLEAVE_16_WAY BIT(12)
>>   #define CXL_HDM_DECODER_CTRL_OFFSET 0x4
>>   #define   CXL_HDM_DECODER_ENABLE BIT(1)
>>   #define CXL_HDM_DECODER0_BASE_LOW_OFFSET(i) (0x20 * (i) + 0x10)
>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
>> index d5f872ca62f9..9b4b23b3b78a 100644
>> --- a/drivers/cxl/cxlmem.h
>> +++ b/drivers/cxl/cxlmem.h
>> @@ -398,9 +398,35 @@ struct cxl_hdm {
>>   	unsigned int decoder_count;
>>   	unsigned int target_count;
>>   	unsigned int interleave_mask;
>> +	bool interleave_3_6_12;
>> +	bool interleave_16;
>>   	struct cxl_port *port;
>>   };
>>   
>> +static inline bool valid_interleave(struct cxl_hdm *cxlhdm, u8 iw)
>> +{
>> +	switch (iw) {
>> +	case CXL_INTERLEAVE_1_WAY:
>> +	case CXL_INTERLEAVE_2_WAY:
>> +	case CXL_INTERLEAVE_4_WAY:
>> +	case CXL_INTERLEAVE_8_WAY:
>> +		return true;
>> +	case CXL_INTERLEAVE_16_WAY:
>> +		if (!cxlhdm->interleave_16)
>> +			return false;
>> +		return true;
>> +	case CXL_INTERLEAVE_3_WAY:
>> +	case CXL_INTERLEAVE_6_WAY:
>> +	case CXL_INTERLEAVE_12_WAY:
>> +		if (!cxlhdm->interleave_3_6_12)
>> +			return false;
>> +		return true;
>> +	default:
>> +	};
>> +
>> +	return false;
> I'd prefer something more compact like
>
>      test_bit(iw, &port->interleave_cap)
>
> ...where interleave_cap is just a mask with bits 1,2,3,4,6,8,12,16
> optionally set.

ok

Re: [PATCH 3/3] tools/testing/cxl: Add interleave check support to mock cxl port device

[Dave Jiang]

On 8/9/2022 9:21 AM, Dan Williams wrote:
> Dave Jiang wrote:
>> Attach the cxl mock hdm to the port device to allow cxl_interleave_verify()
>> to check the interleave configuration. Set the interleave_mask as well
>> to support the new verification code.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   tools/testing/cxl/test/cxl.c |    2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
>> index a072b2d3e726..5a9f33703ee7 100644
>> --- a/tools/testing/cxl/test/cxl.c
>> +++ b/tools/testing/cxl/test/cxl.c
>> @@ -398,6 +398,8 @@ static struct cxl_hdm *mock_cxl_setup_hdm(struct cxl_port *port)
>>   		return ERR_PTR(-ENOMEM);
>>   
>>   	cxlhdm->port = port;
>> +	cxlhdm->interleave_mask = GENMASK(14, 8);
>> +	dev_set_drvdata(&port->dev, cxlhdm);
> This would also need to set cxlhdm->interleave_cap.
Yes with the suggested change in the previous patch, we need to set the 
default 1, 2, 4, 8 bits.
>
> I accidentally called it port->interleave_cap in the last mail, but it
> belong in cxlhdm.

Re: [PATCH 1/3] cxl: Add check for result of interleave ways plus granularity combo

[Dave Jiang]

On 8/9/2022 10:06 AM, Alison Schofield wrote:
> Dave - Haven't reviewed yet, but wanted to drop this tidbit -
>
> On Mon, Aug 08, 2022 at 02:06:57PM -0700, Dave Jiang wrote:
>> Add a helper function to check the combination of interleave ways and
>> interleave granularity together is sane against the interleave mask from
>> the HDM decoder. Add the check to cxl_region_attach() to make sure the
>> region config is sane. Add the check to cxl_port_setup_targets() to make
>> sure the port setup config is also sane.
>>
>> Calculation refers to CXL spec v3 8.2.4.19.13 implementation note #3.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> snip
>
>> +static inline int cxl_interleave_verify(struct cxl_port *port,
>> +					int ways, int granularity)
>> +{
>> +	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
>> +	unsigned int addr_mask;
>> +	u16 ig;
>> +	u8 iw;
> s/ig/eig
> s/iw/eiw
> For consistency, let's use the "e" version of these names to mean
> encoded value.

Ok. Good suggestion.


>    
>
>> +	int rc;
>> +
>> +	rc = granularity_to_cxl(granularity, &ig);
>> +	if (rc)
>> +		return rc;
>> +
>> +	rc = ways_to_cxl(ways, &iw);
>> +	if (rc)
>> +		return rc;
>> +
>> +	if (iw == 0)
>> +		return 0;
>> +
>> +	if (iw < CXL_INTERLEAVE_3_WAY)
>> +		addr_mask = GENMASK(ig + 8 + iw - 1, ig + 8);
>> +	else
>> +		addr_mask = GENMASK((ig + iw) / 3 - 1, ig + 8);
>> +
>> +	if (~cxlhdm->interleave_mask & addr_mask)
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>>   struct seq_file;
>>   struct dentry *cxl_debugfs_create_dir(const char *dir);
>>   void cxl_dpa_debug(struct seq_file *file, struct cxl_dev_state *cxlds);
>>
>>