From: Dan Williams <dan.j.williams@intel.com>
To: <alison.schofield@intel.com>, 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>,
Dan Williams <dan.j.williams@intel.com>
Cc: <linux-cxl@vger.kernel.org>
Subject: Re: [PATCH 1/3] cxl/acpi: Restore XOR'd position bits during address translation
Date: Fri, 26 Apr 2024 18:07:59 -0700 [thread overview]
Message-ID: <662c4fef695d6_b6e029460@dwillia2-mobl3.amr.corp.intel.com.notmuch> (raw)
In-Reply-To: <d4e689f9a9645cbddb943188d508807da33bf91f.1714159486.git.alison.schofield@intel.com>
alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
>
> When a CXL region is created in a CXL Window (CFMWS) that uses XOR
> interleave arithmetic XOR maps are applied during the HPA->DPA
> translation. The XOR function changes the interleave selector
> bit (aka position bit) in the HPA thereby varying which host bridge
> services an HPA. The purpose is to minimize hot spots thereby
> improving performance.
>
> When a device reports a DPA in events such as poison, general_media,
> and dram, the driver translates that DPA back to an HPA. Presently,
> the CXL driver translation only considers the modulo position and
> will report the wrong HPA for XOR configured CFMWS's.
>
> Add a helper function that restores the XOR'd bits during DPA->HPA
> address translation. Plumb a root decoder callback to the new helper
> when XOR interleave arithmetic is in use. For MODULO arithmetic, just
> let the callback be NULL - as in no extra work required.
>
> Fixes: 28a3ae4ff66c ("cxl/trace: Add an HPA to cxl_poison trace events")
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
> drivers/cxl/acpi.c | 49 +++++++++++++++++++++++++++++++++++++---
> drivers/cxl/core/port.c | 5 +++-
> drivers/cxl/core/trace.c | 5 ++++
> drivers/cxl/cxl.h | 6 ++++-
> 4 files changed, 60 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index af5cb818f84d..519e933b5a4b 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -74,6 +74,44 @@ static struct cxl_dport *cxl_hb_xor(struct cxl_root_decoder *cxlrd, int pos)
> return cxlrd->cxlsd.target[n];
> }
>
> +static u64 restore_xor_pos(u64 hpa, u64 map)
> +{
> + int restore_value, restore_pos = 0;
> +
> + /*
> + * Restore the position bit to its value before the
> + * xormap was applied at HPA->DPA translation.
> + *
> + * restore_pos is the lowest set bit in the map
> + * restore_value is the XORALLBITS in (hpa AND map)
Might be worth finally clarifying why there is no "XOR" operation in
this xor_pos routine, i.e. that XORALLBITS is identical to asking if the
hweight of the (hpa & map) is odd or even.
> + */
> +
> + while ((map & (1ULL << restore_pos)) == 0)
> + restore_pos++;
This is just open coded ffs()?
> +
> + restore_value = (hweight64(hpa & map) & 1);
> + if (restore_value)
> + hpa |= (1ULL << restore_pos);
> + else
> + hpa &= ~(1ULL << restore_pos);
It feels like this conditional mask / set can just be an xor operation?
hpa ^= ((hweight64(hpa & map) & 1) << restore_pos);
Otherwise I question the & and | operations relative to the HPA bit
position already being 0 or 1.
> +
> + return hpa;
> +}
> +
> +static u64 cxl_xor_trans(struct cxl_root_decoder *cxlrd, u64 hpa, int iw)
Ok, so the driver has cxl_trace_hpa() and now cxl_xor_trans() and
"addr_trans". Can these all just be called "translate" because "trace"
feels like tracing, "trans" is only saving 4 characters, and "addr" is
redundant as nothing else needs translating besides addresses in CXL
land.
[..]
> diff --git a/drivers/cxl/core/trace.c b/drivers/cxl/core/trace.c
> index d0403dc3c8ab..a7ea4a256036 100644
> --- a/drivers/cxl/core/trace.c
> +++ b/drivers/cxl/core/trace.c
> @@ -36,6 +36,7 @@ static bool cxl_is_hpa_in_range(u64 hpa, struct cxl_region *cxlr, int pos)
> static u64 cxl_dpa_to_hpa(u64 dpa, struct cxl_region *cxlr,
> struct cxl_endpoint_decoder *cxled)
> {
> + struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
> u64 dpa_offset, hpa_offset, bits_upper, mask_upper, hpa;
> struct cxl_region_params *p = &cxlr->params;
> int pos = cxled->pos;
> @@ -75,6 +76,10 @@ static u64 cxl_dpa_to_hpa(u64 dpa, struct cxl_region *cxlr,
> /* Apply the hpa_offset to the region base address */
> hpa = hpa_offset + p->res->start;
>
> + /* An addr_trans helper is defined for XOR math */
Rather then calling out XOR math since that is an ACPI'ism I would just
say something like: "root decoder overrides typical modulo decode"
next prev parent reply other threads:[~2024-04-27 1:08 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 [this message]
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
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=662c4fef695d6_b6e029460@dwillia2-mobl3.amr.corp.intel.com.notmuch \
--to=dan.j.williams@intel.com \
--cc=alison.schofield@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.