[PATCH v2 0/2] cxl: add interleave capability check

[PATCH v2 0/2] cxl: add interleave capability check

[Yao Xingtao]

Changes:
v1 -> v2:
1. rename interleave_mask to ig_cap_mask
2. add a check for interleave granularity
3. update commit message of PATCH 2

Currently driver does not check the interleave capability of target, it can
attach target to region even if target does not support the interleave
granularity or interleave ways. Thus, applications access the memory
will occur unexpected behavior, such as segmentation fault.

Therefore, it is necessary to check the interleave capability of target
before attaching it to region. If the check fails, the attachment
operation should be stopped.

Yao Xingtao (2):
  cxl/core/hdm: rename interleave_mask to ig_cap_mask
  cxl/core/region: check interleave capability

 drivers/cxl/core/hdm.c    |  8 ++++++--
 drivers/cxl/core/region.c | 37 +++++++++++++++++++++++++++++++++++++
 drivers/cxl/cxl.h         |  2 ++
 drivers/cxl/cxlmem.h      |  3 ++-
 4 files changed, 47 insertions(+), 3 deletions(-)

-- 
2.37.3

[PATCH v2 1/2] cxl/core/hdm: rename interleave_mask to ig_cap_mask

[Yao Xingtao]

Since interleave contains interleave ways and interleave granularity,
using ig_cap_mask is better to describe this capability.

Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com>
---
 drivers/cxl/core/hdm.c | 4 ++--
 drivers/cxl/cxlmem.h   | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 7d97790b893d..9bb6a256cc6f 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -76,9 +76,9 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
 	cxlhdm->target_count =
 		FIELD_GET(CXL_HDM_DECODER_TARGET_COUNT_MASK, hdm_cap);
 	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_11_8, hdm_cap))
-		cxlhdm->interleave_mask |= GENMASK(11, 8);
+		cxlhdm->ig_cap_mask |= GENMASK(11, 8);
 	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_14_12, hdm_cap))
-		cxlhdm->interleave_mask |= GENMASK(14, 12);
+		cxlhdm->ig_cap_mask |= GENMASK(14, 12);
 }
 
 static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 20fb3b35e89e..b53f7ae0fdd6 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -852,7 +852,7 @@ struct cxl_hdm {
 	struct cxl_component_regs regs;
 	unsigned int decoder_count;
 	unsigned int target_count;
-	unsigned int interleave_mask;
+	unsigned int ig_cap_mask;
 	struct cxl_port *port;
 };
 
-- 
2.37.3

[PATCH v2 2/2] cxl/core/region: check interleave capability

[Yao Xingtao]

Since interleave capability is not checked, a target can be attached to
region successfully even it does not support the interleave ways or
interleave granularity.

When accessing the memory, unexpected behavior occurs due to converting
HPA to an error DPA:
$ numactl -m 2 ls
Segmentation fault (core dumped)

Link: https://lore.kernel.org/qemu-devel/20240327014653.26623-1-yaoxt.fnst@fujitsu.com
Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com>
---
 drivers/cxl/core/hdm.c    |  4 ++++
 drivers/cxl/core/region.c | 41 +++++++++++++++++++++++++++++++++++++++
 drivers/cxl/cxl.h         |  2 ++
 drivers/cxl/cxlmem.h      |  1 +
 4 files changed, 48 insertions(+)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 9bb6a256cc6f..1a99b138dbec 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -79,6 +79,10 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
 		cxlhdm->ig_cap_mask |= GENMASK(11, 8);
 	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_14_12, hdm_cap))
 		cxlhdm->ig_cap_mask |= GENMASK(14, 12);
+	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY, hdm_cap))
+		cxlhdm->iw_cap_mask |= BIT(3) | BIT(6) | BIT(12);
+	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_16_WAY, hdm_cap))
+		cxlhdm->iw_cap_mask |= BIT(16);
 }
 
 static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 5c186e0a39b9..25d178e14ed1 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1786,6 +1786,36 @@ static int cxl_region_sort_targets(struct cxl_region *cxlr)
 	return rc;
 }
 
+static int
+check_interleave_cap(struct cxl_decoder *cxld, int iw, int ig)
+{
+	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
+	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
+	u8 eiw;
+	u16 eig;
+	int rc;
+
+	rc = ways_to_eiw(iw, &eiw);
+	if (rc)
+		return rc;
+
+	if (eiw > 3 && !(cxlhdm->iw_cap_mask & BIT(iw))) {
+		dev_dbg(&cxld->dev, "iw: %d is not supported\n", iw);
+		return -EOPNOTSUPP;
+	}
+
+	rc = granularity_to_eig(ig, &eig);
+	if (rc)
+		return rc;
+
+	if (!(BIT(eig + 8) & cxlhdm->ig_cap_mask)) {
+		dev_dbg(&cxld->dev, "ig: %d is not supported\n", ig);
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
 static int cxl_region_attach(struct cxl_region *cxlr,
 			     struct cxl_endpoint_decoder *cxled, int pos)
 {
@@ -1796,6 +1826,17 @@ static int cxl_region_attach(struct cxl_region *cxlr,
 	struct cxl_dport *dport;
 	int rc = -ENXIO;
 
+	rc = check_interleave_cap(&cxled->cxld, p->interleave_ways,
+				  p->interleave_granularity);
+	if (rc) {
+		dev_dbg(&cxlr->dev,
+			"%s with region iw: %d, ig: %d is not supported\n",
+			dev_name(&cxled->cxld.dev),
+			p->interleave_ways,
+			p->interleave_granularity);
+		return rc;
+	}
+
 	if (cxled->mode != cxlr->mode) {
 		dev_dbg(&cxlr->dev, "%s region mode: %d mismatch: %d\n",
 			dev_name(&cxled->cxld.dev), cxlr->mode, cxled->mode);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 534e25e2f0a4..da8a487ededa 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -45,6 +45,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 b53f7ae0fdd6..979c22955246 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -853,6 +853,7 @@ struct cxl_hdm {
 	unsigned int decoder_count;
 	unsigned int target_count;
 	unsigned int ig_cap_mask;
+	unsigned int iw_cap_mask;
 	struct cxl_port *port;
 };
 
-- 
2.37.3

Re: [PATCH v2 1/2] cxl/core/hdm: rename interleave_mask to ig_cap_mask

[Jonathan Cameron]

On Tue,  2 Apr 2024 22:17:46 -0400
Yao Xingtao <yaoxt.fnst@fujitsu.com> wrote:

> Since interleave contains interleave ways and interleave granularity,
> using ig_cap_mask is better to describe this capability.

Hi Yao,

I don't follow the reasoning of this.

ig suggests interleave granularity which whilst related to the address
bits, they aren't masking the granularity as such.

I agree current naming isn't particularly meaningful, but don't see the
suggested change as an improvement.

Jonathan
> 
> Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com>
> ---
>  drivers/cxl/core/hdm.c | 4 ++--
>  drivers/cxl/cxlmem.h   | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 7d97790b893d..9bb6a256cc6f 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -76,9 +76,9 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
>  	cxlhdm->target_count =
>  		FIELD_GET(CXL_HDM_DECODER_TARGET_COUNT_MASK, hdm_cap);
>  	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_11_8, hdm_cap))
> -		cxlhdm->interleave_mask |= GENMASK(11, 8);
> +		cxlhdm->ig_cap_mask |= GENMASK(11, 8);
>  	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_14_12, hdm_cap))
> -		cxlhdm->interleave_mask |= GENMASK(14, 12);
> +		cxlhdm->ig_cap_mask |= GENMASK(14, 12);
>  }
>  
>  static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 20fb3b35e89e..b53f7ae0fdd6 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -852,7 +852,7 @@ struct cxl_hdm {
>  	struct cxl_component_regs regs;
>  	unsigned int decoder_count;
>  	unsigned int target_count;
> -	unsigned int interleave_mask;
> +	unsigned int ig_cap_mask;
>  	struct cxl_port *port;
>  };
>  

Re: [PATCH v2 2/2] cxl/core/region: check interleave capability

[Jonathan Cameron]

On Tue,  2 Apr 2024 22:17:47 -0400
Yao Xingtao <yaoxt.fnst@fujitsu.com> wrote:

> Since interleave capability is not checked, a target can be attached to
> region successfully even it does not support the interleave ways or
> interleave granularity.
> 
> When accessing the memory, unexpected behavior occurs due to converting
> HPA to an error DPA:
> $ numactl -m 2 ls
> Segmentation fault (core dumped)
> 
> Link: https://lore.kernel.org/qemu-devel/20240327014653.26623-1-yaoxt.fnst@fujitsu.com
> Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com>

I argued on the CXL opensource sync call last night that we'd get an
hdm commit fail (on working hardware - unlike current qemu) if this check
wasn't present.  Having thought more I think I was wrong and this is a
necessary fix because a device that doesn't support one of these ways
treats the HDM Decoder n Control Register / Interleave Ways (IW) values
as 'reserved'. Is it guaranteed to not just do that by fixing the higher
bits to zero?

If that's a possible implementation and the decoder was set to 6-way (0x9)
maybe the device would interpret that as 2-way (0x1) and give us the wrong
decode.

With that in mind I think this is a fix and needs a Fixes tag.


> ---
>  drivers/cxl/core/hdm.c    |  4 ++++
>  drivers/cxl/core/region.c | 41 +++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/cxl.h         |  2 ++
>  drivers/cxl/cxlmem.h      |  1 +
>  4 files changed, 48 insertions(+)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 9bb6a256cc6f..1a99b138dbec 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -79,6 +79,10 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
>  		cxlhdm->ig_cap_mask |= GENMASK(11, 8);
>  	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_14_12, hdm_cap))
>  		cxlhdm->ig_cap_mask |= GENMASK(14, 12);
> +	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY, hdm_cap))
> +		cxlhdm->iw_cap_mask |= BIT(3) | BIT(6) | BIT(12);
> +	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_16_WAY, hdm_cap))
> +		cxlhdm->iw_cap_mask |= BIT(16);

Whilst it doesn't matter as such (because they are always valid) I think
we should also st the bits for 1,2,4,8 so that all relevant bits are
enabled.

>  }
>  
>  static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 5c186e0a39b9..25d178e14ed1 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1786,6 +1786,36 @@ static int cxl_region_sort_targets(struct cxl_region *cxlr)
>  	return rc;
>  }
>  
> +static int
> +check_interleave_cap(struct cxl_decoder *cxld, int iw, int ig)
> +{
> +	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> +	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
> +	u8 eiw;
> +	u16 eig;
> +	int rc;
> +
> +	rc = ways_to_eiw(iw, &eiw);
> +	if (rc)
> +		return rc;
> +
> +	if (eiw > 3 && !(cxlhdm->iw_cap_mask & BIT(iw))) {
If you have all the bits set then you can avoid using eiw > 3 just
to check we aren't 1,2,4,8

> +		dev_dbg(&cxld->dev, "iw: %d is not supported\n", iw);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	rc = granularity_to_eig(ig, &eig);
> +	if (rc)
> +		return rc;
> +
> +	if (!(BIT(eig + 8) & cxlhdm->ig_cap_mask)) {

This seems too simple.  Need to look at the calculations in
IMPLEMENTATON NOTE: CXL Host Bridge and Upstream Switch Port Decode Flow.

For a decode of more than 2 ways you need more bits to be supported.

> +		dev_dbg(&cxld->dev, "ig: %d is not supported\n", ig);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
>  static int cxl_region_attach(struct cxl_region *cxlr,
>  			     struct cxl_endpoint_decoder *cxled, int pos)
>  {
> @@ -1796,6 +1826,17 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  	struct cxl_dport *dport;
>  	int rc = -ENXIO;
>  
> +	rc = check_interleave_cap(&cxled->cxld, p->interleave_ways,
> +				  p->interleave_granularity);
> +	if (rc) {
> +		dev_dbg(&cxlr->dev,
> +			"%s with region iw: %d, ig: %d is not supported\n",
> +			dev_name(&cxled->cxld.dev),
> +			p->interleave_ways,
> +			p->interleave_granularity);
> +		return rc;
> +	}
> +
>  	if (cxled->mode != cxlr->mode) {
>  		dev_dbg(&cxlr->dev, "%s region mode: %d mismatch: %d\n",
>  			dev_name(&cxled->cxld.dev), cxlr->mode, cxled->mode);
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 534e25e2f0a4..da8a487ededa 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -45,6 +45,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 b53f7ae0fdd6..979c22955246 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -853,6 +853,7 @@ struct cxl_hdm {
>  	unsigned int decoder_count;
>  	unsigned int target_count;
>  	unsigned int ig_cap_mask;
> +	unsigned int iw_cap_mask;
>  	struct cxl_port *port;
>  };
>  

RE: [PATCH v2 1/2] cxl/core/hdm: rename interleave_mask to ig_cap_mask

[Xingtao Yao (Fujitsu)]

> -----Original Message-----
> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Sent: Wednesday, April 3, 2024 10:06 PM
> To: Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com>
> Cc: dave@stgolabs.net; dave.jiang@intel.com; alison.schofield@intel.com;
> vishal.l.verma@intel.com; ira.weiny@intel.com; dan.j.williams@intel.com;
> jim.harris@samsung.com; linux-cxl@vger.kernel.org
> Subject: Re: [PATCH v2 1/2] cxl/core/hdm: rename interleave_mask to
> ig_cap_mask
> 
> On Tue,  2 Apr 2024 22:17:46 -0400
> Yao Xingtao <yaoxt.fnst@fujitsu.com> wrote:
> 
> > Since interleave contains interleave ways and interleave granularity,
> > using ig_cap_mask is better to describe this capability.
> 
> Hi Yao,
> 
> I don't follow the reasoning of this.
> 
> ig suggests interleave granularity which whilst related to the address
> bits, they aren't masking the granularity as such.
> 
> I agree current naming isn't particularly meaningful, but don't see the
> suggested change as an improvement.
this is my misunderstanding of these bits, I will drop this modification.

> 
> Jonathan
> >
> > Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com>
> > ---
> >  drivers/cxl/core/hdm.c | 4 ++--
> >  drivers/cxl/cxlmem.h   | 2 +-
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index 7d97790b893d..9bb6a256cc6f 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -76,9 +76,9 @@ static void parse_hdm_decoder_caps(struct cxl_hdm
> *cxlhdm)
> >  	cxlhdm->target_count =
> >  		FIELD_GET(CXL_HDM_DECODER_TARGET_COUNT_MASK,
> hdm_cap);
> >  	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_11_8, hdm_cap))
> > -		cxlhdm->interleave_mask |= GENMASK(11, 8);
> > +		cxlhdm->ig_cap_mask |= GENMASK(11, 8);
> >  	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_14_12, hdm_cap))
> > -		cxlhdm->interleave_mask |= GENMASK(14, 12);
> > +		cxlhdm->ig_cap_mask |= GENMASK(14, 12);
> >  }
> >
> >  static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
> > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > index 20fb3b35e89e..b53f7ae0fdd6 100644
> > --- a/drivers/cxl/cxlmem.h
> > +++ b/drivers/cxl/cxlmem.h
> > @@ -852,7 +852,7 @@ struct cxl_hdm {
> >  	struct cxl_component_regs regs;
> >  	unsigned int decoder_count;
> >  	unsigned int target_count;
> > -	unsigned int interleave_mask;
> > +	unsigned int ig_cap_mask;
> >  	struct cxl_port *port;
> >  };
> >

RE: [PATCH v2 2/2] cxl/core/region: check interleave capability

[Xingtao Yao (Fujitsu)]

> -----Original Message-----
> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Sent: Wednesday, April 3, 2024 10:28 PM
> To: Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com>
> Cc: dave@stgolabs.net; dave.jiang@intel.com; alison.schofield@intel.com;
> vishal.l.verma@intel.com; ira.weiny@intel.com; dan.j.williams@intel.com;
> jim.harris@samsung.com; linux-cxl@vger.kernel.org
> Subject: Re: [PATCH v2 2/2] cxl/core/region: check interleave capability
> 
> On Tue,  2 Apr 2024 22:17:47 -0400
> Yao Xingtao <yaoxt.fnst@fujitsu.com> wrote:
> 
> > Since interleave capability is not checked, a target can be attached to
> > region successfully even it does not support the interleave ways or
> > interleave granularity.
> >
> > When accessing the memory, unexpected behavior occurs due to converting
> > HPA to an error DPA:
> > $ numactl -m 2 ls
> > Segmentation fault (core dumped)
> >
> > Link:
> https://lore.kernel.org/qemu-devel/20240327014653.26623-1-yaoxt.fnst@fujitsu.c
> om
> > Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com>
> 
> I argued on the CXL opensource sync call last night that we'd get an
> hdm commit fail (on working hardware - unlike current qemu) if this check
> wasn't present.  Having thought more I think I was wrong and this is a
> necessary fix because a device that doesn't support one of these ways
> treats the HDM Decoder n Control Register / Interleave Ways (IW) values
> as 'reserved'. Is it guaranteed to not just do that by fixing the higher
> bits to zero?
> 
> If that's a possible implementation and the decoder was set to 6-way (0x9)
> maybe the device would interpret that as 2-way (0x1) and give us the wrong
> decode.
> 
> With that in mind I think this is a fix and needs a Fixes tag.
> 
> 
> > ---
> >  drivers/cxl/core/hdm.c    |  4 ++++
> >  drivers/cxl/core/region.c | 41
> +++++++++++++++++++++++++++++++++++++++
> >  drivers/cxl/cxl.h         |  2 ++
> >  drivers/cxl/cxlmem.h      |  1 +
> >  4 files changed, 48 insertions(+)
> >
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index 9bb6a256cc6f..1a99b138dbec 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -79,6 +79,10 @@ static void parse_hdm_decoder_caps(struct cxl_hdm
> *cxlhdm)
> >  		cxlhdm->ig_cap_mask |= GENMASK(11, 8);
> >  	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_14_12, hdm_cap))
> >  		cxlhdm->ig_cap_mask |= GENMASK(14, 12);
> > +	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY,
> hdm_cap))
> > +		cxlhdm->iw_cap_mask |= BIT(3) | BIT(6) | BIT(12);
> > +	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_16_WAY, hdm_cap))
> > +		cxlhdm->iw_cap_mask |= BIT(16);
> 
> Whilst it doesn't matter as such (because they are always valid) I think
> we should also st the bits for 1,2,4,8 so that all relevant bits are
> enabled.
good idea!

> 
> >  }
> >
> >  static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info)
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 5c186e0a39b9..25d178e14ed1 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -1786,6 +1786,36 @@ static int cxl_region_sort_targets(struct cxl_region
> *cxlr)
> >  	return rc;
> >  }
> >
> > +static int
> > +check_interleave_cap(struct cxl_decoder *cxld, int iw, int ig)
> > +{
> > +	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> > +	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
> > +	u8 eiw;
> > +	u16 eig;
> > +	int rc;
> > +
> > +	rc = ways_to_eiw(iw, &eiw);
> > +	if (rc)
> > +		return rc;
> > +
> > +	if (eiw > 3 && !(cxlhdm->iw_cap_mask & BIT(iw))) {
> If you have all the bits set then you can avoid using eiw > 3 just
> to check we aren't 1,2,4,8
> 
> > +		dev_dbg(&cxld->dev, "iw: %d is not supported\n", iw);
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	rc = granularity_to_eig(ig, &eig);
> > +	if (rc)
> > +		return rc;
> > +
> > +	if (!(BIT(eig + 8) & cxlhdm->ig_cap_mask)) {
> 
> This seems too simple.  Need to look at the calculations in
> IMPLEMENTATON NOTE: CXL Host Bridge and Upstream Switch Port Decode
> Flow.
> 
> For a decode of more than 2 ways you need more bits to be supported.
this is my misunderstanding of interleave bits, I will change this logical.

> 
> > +		dev_dbg(&cxld->dev, "ig: %d is not supported\n", ig);
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int cxl_region_attach(struct cxl_region *cxlr,
> >  			     struct cxl_endpoint_decoder *cxled, int pos)
> >  {
> > @@ -1796,6 +1826,17 @@ static int cxl_region_attach(struct cxl_region *cxlr,
> >  	struct cxl_dport *dport;
> >  	int rc = -ENXIO;
> >
> > +	rc = check_interleave_cap(&cxled->cxld, p->interleave_ways,
> > +				  p->interleave_granularity);
> > +	if (rc) {
> > +		dev_dbg(&cxlr->dev,
> > +			"%s with region iw: %d, ig: %d is not supported\n",
> > +			dev_name(&cxled->cxld.dev),
> > +			p->interleave_ways,
> > +			p->interleave_granularity);
> > +		return rc;
> > +	}
> > +
> >  	if (cxled->mode != cxlr->mode) {
> >  		dev_dbg(&cxlr->dev, "%s region mode: %d mismatch: %d\n",
> >  			dev_name(&cxled->cxld.dev), cxlr->mode,
> cxled->mode);
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index 534e25e2f0a4..da8a487ededa 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> > @@ -45,6 +45,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 b53f7ae0fdd6..979c22955246 100644
> > --- a/drivers/cxl/cxlmem.h
> > +++ b/drivers/cxl/cxlmem.h
> > @@ -853,6 +853,7 @@ struct cxl_hdm {
> >  	unsigned int decoder_count;
> >  	unsigned int target_count;
> >  	unsigned int ig_cap_mask;
> > +	unsigned int iw_cap_mask;
> >  	struct cxl_port *port;
> >  };
> >

RE: [PATCH v2 2/2] cxl/core/region: check interleave capability

[Dan Williams]

Xingtao Yao (Fujitsu) wrote:
[..]
> > > +		dev_dbg(&cxld->dev, "iw: %d is not supported\n", iw);
> > > +		return -EOPNOTSUPP;
> > > +	}
> > > +
> > > +	rc = granularity_to_eig(ig, &eig);
> > > +	if (rc)
> > > +		return rc;
> > > +
> > > +	if (!(BIT(eig + 8) & cxlhdm->ig_cap_mask)) {
> > 
> > This seems too simple.  Need to look at the calculations in
> > IMPLEMENTATON NOTE: CXL Host Bridge and Upstream Switch Port Decode
> > Flow.
> > 
> > For a decode of more than 2 ways you need more bits to be supported.
> this is my misunderstanding of interleave bits, I will change this logical.

My expectation is that interleave_mask is an address mask and can only
be validated by multiplying it against interleave_ways. For example, for
power-of-2 interleave_ways:

	region_interleave_mask = GENMASK(eiw - 1 + eig + 8, eig + 8);

...that is the address bits required to decode the interleave, and if
that mask is not fully supported by the device-address mask then the
device can not be attached to that region.

However, above is untested, please double check my math.