From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: <linux-cxl@vger.kernel.org>
Subject: Re: [PATCH 5/5] cxl/region: Constrain region granularity scaling factor
Date: Wed, 3 Aug 2022 17:17:32 +0100 [thread overview]
Message-ID: <20220803171732.0000482d@huawei.com> (raw)
In-Reply-To: <165853778028.2430596.7493880465382850752.stgit@dwillia2-xfh.jf.intel.com>
On Fri, 22 Jul 2022 17:56:20 -0700
Dan Williams <dan.j.williams@intel.com> wrote:
> Consider the scenario where a platform provides for a x2 host-bridge
> interleave at a granularity of 256 bytes.
Hi Dan,
Terminology not clear to me. Is this interleave across 2 host bridges?
I'm lost in the explanation.
> The only permitted region
> granularities for that configuration are 256 or 512. Anything larger
> than 512 results in unmapped capacity within a given decoder. Also, if
> the region granularity is 512 then the interleave_ways for the region
> must be 4 to keep the scaling matched.
>
> Here are the translations for the first (4) 256-byte blocks where an
> endpoint decoder is configured for a 512-byte granularity:
>
> Block[0] => HB0 => DPA: 0
> Block[1] => HB1 => DPA: 0
> Block[2] => HB0 => DPA: 0
> Block[3] => HB1 => DPA: 0
>
> In order for those translations to not alias the region interleave ways
> must be 4 resulting in:
>
> Block[0] => HB0 => Dev0 => DPA: 0
> Block[1] => HB1 => Dev1 => DPA: 0
> Block[2] => HB0 => Dev2 => DPA: 0
> Block[3] => HB1 => Dev3 => DPA: 0
Block[4] => HBA0 => Dev0 => DPA: 512 ?
which means we are only using alternate 256 blocks of the EP. Does that make
sense?
>
> ...not 2, resulting in:
>
> Block[0] => HB0 => Dev0 => DPA: 0
> Block[1] => HB1 => Dev1 => DPA: 0
> Block[2] => HB0 => Dev0 => DPA: 0 !
> Block[3] => HB1 => Dev1 => DPA: 0 !
>
> Given tooling is already being built around this ABI allow for
> granularity and ways to be set in either order and validate the
> combination once both are set.
>
> Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Typo inline.
> ---
> drivers/cxl/core/region.c | 63 +++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 60 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 05b6212e6399..a34e537e4cb2 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -348,6 +348,25 @@ static ssize_t interleave_ways_store(struct device *dev,
> goto out;
> }
>
> + /*
> + * If region granularity has been set and xHB interleave is active,
> + * validate that granularity is compatible with specified ways.
> + * Otherwise allow ways to be set now and depend on
> + * interleave_granularity_store() to validate this constraint.
> + */
> + if (cxld->interleave_ways > 1 &&
> + p->interleave_granularity > cxld->interleave_granularity &&
> + p->interleave_granularity / cxld->interleave_granularity !=
> + val / cxld->interleave_ways) {
> + dev_dbg(dev,
> + "ways scaling factor %d mismatch with granularity %d\n",
> + val / cxld->interleave_ways,
> + p->interleave_granularity /
> + cxld->interleave_granularity);
> + rc = -EINVAL;
> + goto out;
> + }
> +
> save = p->interleave_ways;
> p->interleave_ways = val;
> rc = sysfs_update_group(&cxlr->dev.kobj, get_cxl_region_target_group());
> @@ -386,7 +405,7 @@ static ssize_t interleave_granularity_store(struct device *dev,
> struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
> struct cxl_region *cxlr = to_cxl_region(dev);
> struct cxl_region_params *p = &cxlr->params;
> - int rc, val;
> + int rc, val, ways;
> u16 ig;
>
> rc = kstrtoint(buf, 0, &val);
> @@ -403,9 +422,29 @@ static ssize_t interleave_granularity_store(struct device *dev,
> * granularity less than the root interleave result in needing
> * multiple endpoints to support a single slot in the
> * interleave.
> + *
> + * When the root interleave ways is 1 then the root granularity is a
> + * don't care.
> + *
> + * Limit region granularity to cxld->interleave_granularity *
> + * rounddown_pow_of_two(cxld->interleave_ways) otherwise holes result in
> + * the decode at each endpoint. Note that rounddown_pow_of_two()
> + * accounts for x3, x6, and x9 root intereleave.
interleave
> */
> - if (val < cxld->interleave_granularity)
> - return -EINVAL;
> + ways = rounddown_pow_of_two(cxld->interleave_ways);
> + if (ways > 1) {
> + if (val < cxld->interleave_granularity) {
> + dev_dbg(dev, "granularity %d must be >= %d\n", val,
> + cxld->interleave_granularity);
> + return -EINVAL;
> + }
> +
> + if (val > cxld->interleave_granularity * ways) {
> + dev_dbg(dev, "granularity %d must be <= %d\n", val,
> + cxld->interleave_granularity * ways);
> + return -EINVAL;
> + }
> + }
>
> rc = down_write_killable(&cxl_region_rwsem);
> if (rc)
> @@ -415,6 +454,24 @@ static ssize_t interleave_granularity_store(struct device *dev,
> goto out;
> }
>
> + /*
> + * If region ways has been set and xHB interleave is active, validate
> + * that ways is compatible with specified granularity. Otherwise allow
> + * granularity to be set now and depend on interleave_ways_store() to
> + * validate this constraint.
> + */
> + if (cxld->interleave_ways > 1 && p->interleave_ways &&
> + val > cxld->interleave_granularity &&
> + p->interleave_ways / cxld->interleave_ways !=
> + val / cxld->interleave_granularity) {
> + dev_dbg(dev,
> + "granularity scaling factor %d mismatch with ways %d\n",
> + val / cxld->interleave_granularity,
> + p->interleave_ways / cxld->interleave_ways);
> + rc = -EINVAL;
> + goto out;
> + }
> +
> p->interleave_granularity = val;
> out:
> up_write(&cxl_region_rwsem);
>
>
next prev parent reply other threads:[~2022-08-03 16:17 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-23 0:55 [PATCH 0/5] CXL Region Provisioning Fixes Dan Williams
2022-07-23 0:55 ` [PATCH 1/5] cxl/acpi: Autoload driver for 'cxl_acpi' test devices Dan Williams
2022-08-01 19:24 ` Verma, Vishal L
2022-07-23 0:56 ` [PATCH 2/5] cxl/region: Delete 'region' attribute from root decoders Dan Williams
2022-08-01 19:32 ` Alison Schofield
2022-08-01 19:38 ` Verma, Vishal L
2022-08-01 19:40 ` Verma, Vishal L
2022-08-01 21:32 ` Dan Williams
2022-08-01 21:32 ` Dan Williams
2022-07-23 0:56 ` [PATCH 3/5] cxl/acpi: Minimize granularity for x1 interleaves Dan Williams
2022-08-01 19:35 ` Alison Schofield
2022-08-01 19:45 ` Verma, Vishal L
2022-08-01 21:34 ` Dan Williams
2022-08-02 15:56 ` Jonathan Cameron
2022-08-02 16:52 ` Jonathan Cameron
2022-08-02 17:33 ` Dan Williams
2022-08-03 16:00 ` Jonathan Cameron
2022-08-03 17:18 ` Dan Williams
2022-08-04 9:32 ` Jonathan Cameron
2022-07-23 0:56 ` [PATCH 4/5] cxl/region: Stop initializing interleave granularity Dan Williams
2022-08-01 19:41 ` Alison Schofield
2022-08-01 19:47 ` Verma, Vishal L
2022-07-23 0:56 ` [PATCH 5/5] cxl/region: Constrain region granularity scaling factor Dan Williams
2022-08-01 19:43 ` Alison Schofield
2022-08-01 20:55 ` Verma, Vishal L
2022-08-03 16:17 ` Jonathan Cameron [this message]
2022-08-04 16:33 ` Dan Williams
2022-08-04 17:57 ` 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=20220803171732.0000482d@huawei.com \
--to=jonathan.cameron@huawei.com \
--cc=dan.j.williams@intel.com \
--cc=linux-cxl@vger.kernel.org \
/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 NNTP newsgroup(s).