diff mbox series

[2/3] cxl/region: Verify target positions using the ordered target list

Message ID a5121a8d37dd62581015452a77113f88df2ede25.1714159486.git.alison.schofield@intel.com
State Superseded
Headers show
Series XOR Math Fixups: translation & position | expand

Commit Message

Alison Schofield April 26, 2024, 7:51 p.m. UTC 
From: Alison Schofield <alison.schofield@intel.com>

When a root decoder is configured the interleave target list is read
from the BIOS populated CFMWS structure. Per the CXL spec 3.1 Table
9-22 the target list is in interleave order. The CXL driver populates
its decoder target list in the same order and stores it in 'struct
cxl_switch_decoder' field "@target: active ordered target list in
current decoder configuration"

Given the promise of an ordered list, the driver can stop duplicating
the work of BIOS and simply check target positions against the ordered
list during region configuration.

The simplified check against the ordered list is presented here.
A follow-on patch will remove the unused code.

For Modulo arithmetic this is not a fix, only a simplification.
For XOR arithmetic this is a fix for HB IW of 6,12.

Fixes: f9db85bfec0d ("cxl/acpi: Support CXL XOR Interleave Math (CXIMS)")
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/region.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Dan Williams April 30, 2024, 10:59 p.m. UTC | #1 
alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> When a root decoder is configured the interleave target list is read
> from the BIOS populated CFMWS structure. Per the CXL spec 3.1 Table
> 9-22 the target list is in interleave order. The CXL driver populates
> its decoder target list in the same order and stores it in 'struct
> cxl_switch_decoder' field "@target: active ordered target list in
> current decoder configuration"
> 
> Given the promise of an ordered list, the driver can stop duplicating
> the work of BIOS and simply check target positions against the ordered
> list during region configuration.
> 
> The simplified check against the ordered list is presented here.
> A follow-on patch will remove the unused code.
> 
> For Modulo arithmetic this is not a fix, only a simplification.
> For XOR arithmetic this is a fix for HB IW of 6,12.
> 
> Fixes: f9db85bfec0d ("cxl/acpi: Support CXL XOR Interleave Math (CXIMS)")
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  drivers/cxl/core/region.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 5c186e0a39b9..3c20f8364b26 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1559,10 +1559,17 @@ static int cxl_region_attach_position(struct cxl_region *cxlr,
>  				      const struct cxl_dport *dport, int pos)
>  {
>  	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> +	struct cxl_switch_decoder *cxlsd = &cxlrd->cxlsd;
> +	struct cxl_decoder *cxld = &cxlsd->cxld;
> +	int iw = cxld->interleave_ways;
>  	struct cxl_port *iter;
>  	int rc;
>  
> -	if (cxlrd->calc_hb(cxlrd, pos) != dport) {
> +	if (dev_WARN_ONCE(&cxld->dev, iw != cxlsd->nr_targets,
> +			  "misconfigured root decoder\n"))
> +		return -ENXIO;

This is a nop because root-decoders by definition have all of their
targets covered in the interleave, and the driver passes the CFWMS
interleaves_ways setting directly to the @nr_targets parameter of
cxl_switch_decoder_init().

So drop this warning, which in retrospect was never needed , and it all
gets cleaned up in your next patch.

> +
> +	if (dport != cxlrd->cxlsd.target[pos % iw]) {

Looks ok, but I don't understand why this patch is tagged as a fix?
There should be no end user visible change of this conversion, right?
Alison Schofield May 8, 2024, 5:30 p.m. UTC | #2 
On Tue, Apr 30, 2024 at 03:59:54PM -0700, Dan Williams wrote:
> alison.schofield@ wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > When a root decoder is configured the interleave target list is read
> > from the BIOS populated CFMWS structure. Per the CXL spec 3.1 Table
> > 9-22 the target list is in interleave order. The CXL driver populates
> > its decoder target list in the same order and stores it in 'struct
> > cxl_switch_decoder' field "@target: active ordered target list in
> > current decoder configuration"
> > 
> > Given the promise of an ordered list, the driver can stop duplicating
> > the work of BIOS and simply check target positions against the ordered
> > list during region configuration.
> > 
> > The simplified check against the ordered list is presented here.
> > A follow-on patch will remove the unused code.
> > 
> > For Modulo arithmetic this is not a fix, only a simplification.
> > For XOR arithmetic this is a fix for HB IW of 6,12.
> > 
> > Fixes: f9db85bfec0d ("cxl/acpi: Support CXL XOR Interleave Math (CXIMS)")
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  drivers/cxl/core/region.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 5c186e0a39b9..3c20f8364b26 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -1559,10 +1559,17 @@ static int cxl_region_attach_position(struct cxl_region *cxlr,
> >  				      const struct cxl_dport *dport, int pos)
> >  {
> >  	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> > +	struct cxl_switch_decoder *cxlsd = &cxlrd->cxlsd;
> > +	struct cxl_decoder *cxld = &cxlsd->cxld;
> > +	int iw = cxld->interleave_ways;
> >  	struct cxl_port *iter;
> >  	int rc;
> >  
> > -	if (cxlrd->calc_hb(cxlrd, pos) != dport) {
> > +	if (dev_WARN_ONCE(&cxld->dev, iw != cxlsd->nr_targets,
> > +			  "misconfigured root decoder\n"))
> > +		return -ENXIO;
> 
> This is a nop because root-decoders by definition have all of their
> targets covered in the interleave, and the driver passes the CFWMS
> interleaves_ways setting directly to the @nr_targets parameter of
> cxl_switch_decoder_init().
> 
> So drop this warning, which in retrospect was never needed , and it all
> gets cleaned up in your next patch.

Got it.

> 
> > +
> > +	if (dport != cxlrd->cxlsd.target[pos % iw]) {
> 
> Looks ok, but I don't understand why this patch is tagged as a fix?
> There should be no end user visible change of this conversion, right?

In the commit log I note that this is a 'Fix' for XOR, not for MODULO.

It is a fix for XOR because the previous calculation for the by 3's
can fail to find the correct Host Bridge position.

The diff belows shows the fix needed to properly mask the hpa prior
to the MOD 3 operation. (Described in Table 9-22 HPA[51:NIW+HBIG])

---
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 571069863c62..611e5b995d46 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -30,6 +30,7 @@ static int cxl_xor_calc_n(u64 hpa, struct cxl_cxims_data *cximsd, int iw,
 			  int ig)
 {
 	int i = 0, n = 0;
+	u16 eig;
 	u8 eiw;
 
 	/* IW: 2,4,6,8,12,16 begin building 'n' using xormaps */
@@ -37,11 +38,14 @@ static int cxl_xor_calc_n(u64 hpa, struct cxl_cxims_data *cximsd, int iw,
 		for (i = 0; i < cximsd->nr_maps; i++)
 			n |= (hweight64(hpa & cximsd->xormaps[i]) & 1) << i;
 	}
+
 	/* IW: 3,6,12 add a modulo calculation to 'n' */
 	if (!is_power_of_2(iw)) {
 		if (ways_to_eiw(iw, &eiw))
 			return -1;
-		hpa &= GENMASK_ULL(51, eiw + ig);
+		if (granularity_to_eig(ig, &eig))
+			return -1;
+		hpa = (hpa & GENMASK_ULL(51, eiw + eig)) >> (eiw + eig);
 		n |= do_div(hpa, 3) << i;
 	}
 	return n;
diff mbox series

Patch

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 5c186e0a39b9..3c20f8364b26 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1559,10 +1559,17 @@  static int cxl_region_attach_position(struct cxl_region *cxlr,
 				      const struct cxl_dport *dport, int pos)
 {
 	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
+	struct cxl_switch_decoder *cxlsd = &cxlrd->cxlsd;
+	struct cxl_decoder *cxld = &cxlsd->cxld;
+	int iw = cxld->interleave_ways;
 	struct cxl_port *iter;
 	int rc;
 
-	if (cxlrd->calc_hb(cxlrd, pos) != dport) {
+	if (dev_WARN_ONCE(&cxld->dev, iw != cxlsd->nr_targets,
+			  "misconfigured root decoder\n"))
+		return -ENXIO;
+
+	if (dport != cxlrd->cxlsd.target[pos % iw]) {
 		dev_dbg(&cxlr->dev, "%s:%s invalid target position for %s\n",
 			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
 			dev_name(&cxlrd->cxlsd.cxld.dev));