From: Alison Schofield <alison.schofield@intel.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>,
Jonathan Cameron <jonathan.cameron@huawei.com>,
Dave Jiang <dave.jiang@intel.com>,
Vishal Verma <vishal.l.verma@intel.com>,
Ira Weiny <ira.weiny@intel.com>,
linux-cxl@vger.kernel.org
Subject: Re: [PATCH 2/3] cxl/region: Verify target positions using the ordered target list
Date: Wed, 8 May 2024 10:30:14 -0700 [thread overview]
Message-ID: <Zju2pg7I3AfrLfbk@aschofie-mobl2> (raw)
In-Reply-To: <663177ea1ca3_10c21294f5@dwillia2-mobl3.amr.corp.intel.com.notmuch>
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;
--
I guess in some cases the above patch may be easier to apply to
prior kernels but we really want to replace the mechanism as
'this' patch does.
-- Alison
next prev parent reply other threads:[~2024-05-08 17:30 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-26 19:51 [PATCH 0/3] XOR Math Fixups: translation & position alison.schofield
2024-04-26 19:51 ` [PATCH 1/3] cxl/acpi: Restore XOR'd position bits during address translation alison.schofield
2024-04-27 1:07 ` Dan Williams
2024-05-01 3:41 ` Alison Schofield
2024-05-01 5:00 ` Alison Schofield
2024-05-02 4:34 ` Alison Schofield
2024-04-26 19:51 ` [PATCH 2/3] cxl/region: Verify target positions using the ordered target list alison.schofield
2024-04-30 22:59 ` Dan Williams
2024-05-08 17:30 ` Alison Schofield [this message]
2024-04-26 19:51 ` [PATCH 3/3] cxl: Remove defunct code calculating host bridge target positions alison.schofield
2024-04-30 23:04 ` Dan Williams
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Zju2pg7I3AfrLfbk@aschofie-mobl2 \
--to=alison.schofield@intel.com \
--cc=dan.j.williams@intel.com \
--cc=dave.jiang@intel.com \
--cc=dave@stgolabs.net \
--cc=ira.weiny@intel.com \
--cc=jonathan.cameron@huawei.com \
--cc=linux-cxl@vger.kernel.org \
--cc=vishal.l.verma@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).