Message ID | a5121a8d37dd62581015452a77113f88df2ede25.1714159486.git.alison.schofield@intel.com |
---|---|
State | Superseded |
Headers | show |
Series | XOR Math Fixups: translation & position | expand |
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?
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 --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));