[PATCH] cxl/core/region: check interleave way capability

[PATCH] cxl/core/region: check interleave way capability

[Yao Xingtao]

I used qemu to emulaute the cxl.mem and attmpted to create a 6-way
region, the region was created successfully, but I could not access the
memory properly:
$ numactl -m 2 ls
Segmentation fault (core dumped)

I found the root cause is that the logic of converting HPA to DPA for
3-way, 6-way and 12-way were not implimented on qemu side.

But qemu has already disable the capability, so we should not create
region in such ways.

So I think we should check whether the interleave way is supported by
the target while attaching it to region.

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

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 5c186e0a39b9..dde66b7c9e3f 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1786,6 +1786,24 @@ static int cxl_region_sort_targets(struct cxl_region *cxlr)
 	return rc;
 }
 
+static bool check_iw_capability(struct cxl_endpoint_decoder *cxled, u8 iw)
+{
+	struct cxl_port *port = to_cxl_port(cxled->cxld.dev.parent);
+	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
+	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
+	u32 cap;
+
+	cap = readl(hdm + CXL_HDM_DECODER_CAP_OFFSET);
+	if (!FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY, cap) &&
+	    (iw == 3 || iw == 6 || iw == 12))
+		return false;
+
+	if (!FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_16_WAY, cap) && iw == 16)
+		return false;
+
+	return true;
+}
+
 static int cxl_region_attach(struct cxl_region *cxlr,
 			     struct cxl_endpoint_decoder *cxled, int pos)
 {
@@ -1796,6 +1814,13 @@ static int cxl_region_attach(struct cxl_region *cxlr,
 	struct cxl_dport *dport;
 	int rc = -ENXIO;
 
+	if (!check_iw_capability(cxled, p->interleave_ways)) {
+		dev_dbg(&cxlr->dev,
+			"%s with region interleave ways: %d is not supported\n",
+			dev_name(&cxled->cxld.dev), p->interleave_ways);
+		return -EOPNOTSUPP;
+	}
+
 	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)
-- 
2.37.3

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

[Alison Schofield]

On Mon, Apr 01, 2024 at 03:56:35AM -0400, Yao Xingtao wrote:
> I used qemu to emulaute the cxl.mem and attmpted to create a 6-way
> region, the region was created successfully, but I could not access the
> memory properly:
> $ numactl -m 2 ls
> Segmentation fault (core dumped)
> 
> I found the root cause is that the logic of converting HPA to DPA for
> 3-way, 6-way and 12-way were not implimented on qemu side.
> 
> But qemu has already disable the capability, so we should not create
> region in such ways.
> 
> So I think we should check whether the interleave way is supported by
> the target while attaching it to region.

Nice find!

I'm wondering if this may be done where other hdm caps are parsed,
at parse_hdm_decoder_caps() and saved as a capability of the port,
as opposed to reading the register on region creation attempts.

--Alison



> 
> Link: https://lore.kernel.org/qemu-devel/20240327014653.26623-1-yaoxt.fnst@fujitsu.com/T/#r
> Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com>
> ---
>  drivers/cxl/core/region.c | 25 +++++++++++++++++++++++++
>  drivers/cxl/cxl.h         |  2 ++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 5c186e0a39b9..dde66b7c9e3f 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1786,6 +1786,24 @@ static int cxl_region_sort_targets(struct cxl_region *cxlr)
>  	return rc;
>  }
>  
> +static bool check_iw_capability(struct cxl_endpoint_decoder *cxled, u8 iw)
> +{
> +	struct cxl_port *port = to_cxl_port(cxled->cxld.dev.parent);
> +	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
> +	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
> +	u32 cap;
> +
> +	cap = readl(hdm + CXL_HDM_DECODER_CAP_OFFSET);
> +	if (!FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY, cap) &&
> +	    (iw == 3 || iw == 6 || iw == 12))
> +		return false;
> +
> +	if (!FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_16_WAY, cap) && iw == 16)
> +		return false;
> +
> +	return true;
> +}
> +
>  static int cxl_region_attach(struct cxl_region *cxlr,
>  			     struct cxl_endpoint_decoder *cxled, int pos)
>  {
> @@ -1796,6 +1814,13 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  	struct cxl_dport *dport;
>  	int rc = -ENXIO;
>  
> +	if (!check_iw_capability(cxled, p->interleave_ways)) {
> +		dev_dbg(&cxlr->dev,
> +			"%s with region interleave ways: %d is not supported\n",
> +			dev_name(&cxled->cxld.dev), p->interleave_ways);
> +		return -EOPNOTSUPP;
> +	}
> +
>  	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)
> -- 
> 2.37.3
> 

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

[Dan Williams]

Yao Xingtao wrote:
> I used qemu to emulaute the cxl.mem and attmpted to create a 6-way
> region, the region was created successfully, but I could not access the
> memory properly:
> $ numactl -m 2 ls
> Segmentation fault (core dumped)
> 
> I found the root cause is that the logic of converting HPA to DPA for
> 3-way, 6-way and 12-way were not implimented on qemu side.
> 
> But qemu has already disable the capability, so we should not create
> region in such ways.
> 
> So I think we should check whether the interleave way is supported by
> the target while attaching it to region.

Good find!

Not only does this need to happen for 3, 6, 12, but it also needs to be
checked to make sure the device can support interleaves on address bits
11to8 and 14to12 (CXL_HDM_DECODER_INTERLEAVE_11_8,
CXL_HDM_DECODER_INTERLEAVE_14_12).

As it stands the 'struct cxl_hdm' 'interleave_mask' attribute is unused,
and IIRC was meant to support this capabilty check.

> Link: https://lore.kernel.org/qemu-devel/20240327014653.26623-1-yaoxt.fnst@fujitsu.com/T/#r

This can be trimmed to:

Link: http://lore.kernel.org/r/20240327014653.26623-1-yaoxt.fnst@fujitsu.com

> Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com>
> ---
>  drivers/cxl/core/region.c | 25 +++++++++++++++++++++++++
>  drivers/cxl/cxl.h         |  2 ++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 5c186e0a39b9..dde66b7c9e3f 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1786,6 +1786,24 @@ static int cxl_region_sort_targets(struct cxl_region *cxlr)
>  	return rc;
>  }
>  
> +static bool check_iw_capability(struct cxl_endpoint_decoder *cxled, u8 iw)
> +{
> +	struct cxl_port *port = to_cxl_port(cxled->cxld.dev.parent);
> +	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
> +	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
> +	u32 cap;
> +
> +	cap = readl(hdm + CXL_HDM_DECODER_CAP_OFFSET);
> +	if (!FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY, cap) &&
> +	    (iw == 3 || iw == 6 || iw == 12))
> +		return false;
> +
> +	if (!FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_16_WAY, cap) && iw == 16)
> +		return false;
> +
> +	return true;
> +}
> +
>  static int cxl_region_attach(struct cxl_region *cxlr,
>  			     struct cxl_endpoint_decoder *cxled, int pos)
>  {
> @@ -1796,6 +1814,13 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  	struct cxl_dport *dport;
>  	int rc = -ENXIO;
>  
> +	if (!check_iw_capability(cxled, p->interleave_ways)) {
> +		dev_dbg(&cxlr->dev,
> +			"%s with region interleave ways: %d is not supported\n",
> +			dev_name(&cxled->cxld.dev), p->interleave_ways);
> +		return -EOPNOTSUPP;
> +	}

In order to validate the address-bit capabilities of the endpoint this
routine will also need the 'interleave_granularity' argument.

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

[Xingtao Yao (Fujitsu)]

> -----Original Message-----
> From: Alison Schofield <alison.schofield@intel.com>
> Sent: Tuesday, April 2, 2024 12:51 AM
> To: Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com>
> Cc: dave@stgolabs.net; jonathan.cameron@huawei.com; dave.jiang@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] cxl/core/region: check interleave way capability
> 
> On Mon, Apr 01, 2024 at 03:56:35AM -0400, Yao Xingtao wrote:
> > I used qemu to emulaute the cxl.mem and attmpted to create a 6-way
> > region, the region was created successfully, but I could not access the
> > memory properly:
> > $ numactl -m 2 ls
> > Segmentation fault (core dumped)
> >
> > I found the root cause is that the logic of converting HPA to DPA for
> > 3-way, 6-way and 12-way were not implimented on qemu side.
> >
> > But qemu has already disable the capability, so we should not create
> > region in such ways.
> >
> > So I think we should check whether the interleave way is supported by
> > the target while attaching it to region.
> 
> Nice find!
> 
> I'm wondering if this may be done where other hdm caps are parsed,
> at parse_hdm_decoder_caps() and saved as a capability of the port,
> as opposed to reading the register on region creation attempts.
thanks for your advice!
I will move this operation into parse_hdm_decoder_caps() in the next version.

> 
> --Alison
> 
> 
> 
> >
> > Link:
> https://lore.kernel.org/qemu-devel/20240327014653.26623-1-yaoxt.fnst@fujitsu.c
> om/T/#r
> > Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com>
> > ---
> >  drivers/cxl/core/region.c | 25 +++++++++++++++++++++++++
> >  drivers/cxl/cxl.h         |  2 ++
> >  2 files changed, 27 insertions(+)
> >
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 5c186e0a39b9..dde66b7c9e3f 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -1786,6 +1786,24 @@ static int cxl_region_sort_targets(struct cxl_region
> *cxlr)
> >  	return rc;
> >  }
> >
> > +static bool check_iw_capability(struct cxl_endpoint_decoder *cxled, u8 iw)
> > +{
> > +	struct cxl_port *port = to_cxl_port(cxled->cxld.dev.parent);
> > +	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
> > +	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
> > +	u32 cap;
> > +
> > +	cap = readl(hdm + CXL_HDM_DECODER_CAP_OFFSET);
> > +	if (!FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY, cap)
> &&
> > +	    (iw == 3 || iw == 6 || iw == 12))
> > +		return false;
> > +
> > +	if (!FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_16_WAY, cap) && iw
> == 16)
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> >  static int cxl_region_attach(struct cxl_region *cxlr,
> >  			     struct cxl_endpoint_decoder *cxled, int pos)
> >  {
> > @@ -1796,6 +1814,13 @@ static int cxl_region_attach(struct cxl_region *cxlr,
> >  	struct cxl_dport *dport;
> >  	int rc = -ENXIO;
> >
> > +	if (!check_iw_capability(cxled, p->interleave_ways)) {
> > +		dev_dbg(&cxlr->dev,
> > +			"%s with region interleave ways: %d is not supported\n",
> > +			dev_name(&cxled->cxld.dev), p->interleave_ways);
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> >  	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)
> > --
> > 2.37.3
> >

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

[Xingtao Yao (Fujitsu)]

> -----Original Message-----
> From: Dan Williams <dan.j.williams@intel.com>
> Sent: Tuesday, April 2, 2024 2:34 AM
> To: Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com>; dave@stgolabs.net;
> jonathan.cameron@huawei.com; 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
> Cc: linux-cxl@vger.kernel.org; Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com>
> Subject: Re: [PATCH] cxl/core/region: check interleave way capability
> 
> Yao Xingtao wrote:
> > I used qemu to emulaute the cxl.mem and attmpted to create a 6-way
> > region, the region was created successfully, but I could not access the
> > memory properly:
> > $ numactl -m 2 ls
> > Segmentation fault (core dumped)
> >
> > I found the root cause is that the logic of converting HPA to DPA for
> > 3-way, 6-way and 12-way were not implimented on qemu side.
> >
> > But qemu has already disable the capability, so we should not create
> > region in such ways.
> >
> > So I think we should check whether the interleave way is supported by
> > the target while attaching it to region.
> 
> Good find!
> 
> Not only does this need to happen for 3, 6, 12, but it also needs to be
> checked to make sure the device can support interleaves on address bits
> 11to8 and 14to12 (CXL_HDM_DECODER_INTERLEAVE_11_8,
> CXL_HDM_DECODER_INTERLEAVE_14_12).
> 
> As it stands the 'struct cxl_hdm' 'interleave_mask' attribute is unused,
> and IIRC was meant to support this capabilty check.
I will add this check in the next version.

> 
> > Link:
> https://lore.kernel.org/qemu-devel/20240327014653.26623-1-yaoxt.fnst@fujitsu.c
> om/T/#r
> 
> This can be trimmed to:
> 
> Link: http://lore.kernel.org/r/20240327014653.26623-1-yaoxt.fnst@fujitsu.com
I will change the comment in the next version

> 
> > Signed-off-by: Yao Xingtao <yaoxt.fnst@fujitsu.com>
> > ---
> >  drivers/cxl/core/region.c | 25 +++++++++++++++++++++++++
> >  drivers/cxl/cxl.h         |  2 ++
> >  2 files changed, 27 insertions(+)
> >
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 5c186e0a39b9..dde66b7c9e3f 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -1786,6 +1786,24 @@ static int cxl_region_sort_targets(struct cxl_region
> *cxlr)
> >  	return rc;
> >  }
> >
> > +static bool check_iw_capability(struct cxl_endpoint_decoder *cxled, u8 iw)
> > +{
> > +	struct cxl_port *port = to_cxl_port(cxled->cxld.dev.parent);
> > +	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
> > +	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
> > +	u32 cap;
> > +
> > +	cap = readl(hdm + CXL_HDM_DECODER_CAP_OFFSET);
> > +	if (!FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_3_6_12_WAY, cap)
> &&
> > +	    (iw == 3 || iw == 6 || iw == 12))
> > +		return false;
> > +
> > +	if (!FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_16_WAY, cap) && iw
> == 16)
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> >  static int cxl_region_attach(struct cxl_region *cxlr,
> >  			     struct cxl_endpoint_decoder *cxled, int pos)
> >  {
> > @@ -1796,6 +1814,13 @@ static int cxl_region_attach(struct cxl_region *cxlr,
> >  	struct cxl_dport *dport;
> >  	int rc = -ENXIO;
> >
> > +	if (!check_iw_capability(cxled, p->interleave_ways)) {
> > +		dev_dbg(&cxlr->dev,
> > +			"%s with region interleave ways: %d is not supported\n",
> > +			dev_name(&cxled->cxld.dev), p->interleave_ways);
> > +		return -EOPNOTSUPP;
> > +	}
> 
> In order to validate the address-bit capabilities of the endpoint this
> routine will also need the 'interleave_granularity' argument.