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, Wonjae Lee <wj28.lee@samsung.com>
Subject: Re: [RFC PATCH] cxl/region: Allow out of order assembly of autodiscovered regions
Date: Thu, 8 Feb 2024 12:52:04 -0800 [thread overview]
Message-ID: <ZcU+9BxBD8IRsnTu@aschofie-mobl2> (raw)
In-Reply-To: <65a980249f50f_3b8e294a3@dwillia2-xfh.jf.intel.com.notmuch>
On Thu, Jan 18, 2024 at 11:46:44AM -0800, Dan Williams wrote:
- snip to decoder replay patch -
>
> So one of the largest roadblocks for creating arbitrary region assembly
> scenarios with cxl_test is the inability to save and restore decoder
> settings.
>
> The patch below adds support for recording decoder settings and skipping
> the reset of those values when unloading the driver. Then, when the
> driver is reloaded, it simulates the case of BIOS created CXL regions
> prior to OS boot.
>
> We can go after even finer grained cases once the uunit effort settles,
> but in the meantime cxl_test can add auto-assembly regression tests with
> the current topology.
>
> With the below you can simply trigger "cxl {en,dis}able-memdev" in the
> proper order to cause the violation.
>
> -- >8 --
> From 5839392a3dbb64dbbac0630d930b1f48a8ef7ea6 Mon Sep 17 00:00:00 2001
> From: Dan Williams <dan.j.williams@intel.com>
> Date: Wed, 17 Jan 2024 20:56:20 -0800
> Subject: [PATCH] tools/testing/cxl: Add decoder save/restore support
>
> Record decoder values at init and mock_decoder_commit() time, and
> restore them at the next invocation of mock_init_hdm_decoder(). Add 2
> attributes to the cxl_test "cxl_acpi" device to optionally flush the
> cache of topology decoder values, or disable updating the decoder at
> mock_decoder_reset() time.
>
> This enables replaying a saved decoder configuration when re-triggering
> a topology scan by re-binding the cxl_acpi driver to "cxl_acpi.0" (the
> cxl_test emulation of an ACPI0017 instance).
Hi Dan,
Sorry it's taken a while to come back on this. I did find it useful
in testing the auto-assembly order issue as you suggested.
I didn't use this one: &dev_attr_decoder_registry_invalidate.attr,
I just reloaded the cxl-test module to do same.
This I used: &dev_attr_decoder_registry_reset_disable.attr,
with your decoders state fixup to set CXL_DECODER_STATE_AUTO,
and a work-around to avoid pmem_probe failures on pmem region
auto create.
More generally, I'm wondering about the implementation of the
'registry_save'. Here it continuously updates during all
cxl-test usage. Did you consider only creating the registry upon
user request and then at the next mock_init_hmd_decoder() look
for and use that registry if it exists.
Usage would be:
# echo 1 > /sys/bus/platform/devices/cxl_acpi.0/decoder_registry_create
# echo cxl_acpi.0 > /sys/bus/platform/drivers/cxl_acpi/unbind
# echo cxl_acpi.0 > /sys/bus/platform/drivers/cxl_acpi/bind
And then maybe, for folks who like to acpi/unbind,bind, rather
than reload module, we could offer a decoder_registry_remove attr.
Am I missing something regarding the need to keep it updated
on the fly?
Alison
>
> # modprobe cxl_test
> # cxl list -RB -b cxl_test -u
> {
> "bus":"root3",
> "provider":"cxl_test",
> "regions:root3":[
> {
> "region":"region5",
> "resource":"0xf010000000",
> "size":"512.00 MiB (536.87 MB)",
> "type":"ram",
> "interleave_ways":2,
> "interleave_granularity":4096,
> "decode_state":"commit"
> }
> ]
> }
> # echo 1 > /sys/bus/platform/devices/cxl_acpi.0/decoder_registry_reset_disable
> # echo cxl_acpi.0 > /sys/bus/platform/drivers/cxl_acpi/unbind
> # cxl list -RB -b cxl_test -u
> # echo cxl_acpi.0 > /sys/bus/platform/drivers/cxl_acpi/bind
> # cxl list -RB -b cxl_test -u
> {
> "bus":"root3",
> "provider":"cxl_test",
> "regions:root3":[
> {
> "region":"region5",
> "resource":"0xf010000000",
> "size":"512.00 MiB (536.87 MB)",
> "type":"ram",
> "interleave_ways":2,
> "interleave_granularity":4096,
> "decode_state":"commit"
> }
> ]
> }
>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> tools/testing/cxl/test/cxl.c | 268 +++++++++++++++++++++++++++++++++++
> 1 file changed, 268 insertions(+)
>
> diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
> index f4e517a0c774..3f333d6a57be 100644
> --- a/tools/testing/cxl/test/cxl.c
> +++ b/tools/testing/cxl/test/cxl.c
> @@ -44,6 +44,9 @@ struct platform_device *cxl_mem_single[NR_MEM_SINGLE];
> static struct platform_device *cxl_rch[NR_CXL_RCH];
> static struct platform_device *cxl_rcd[NR_CXL_RCH];
>
> +static DEFINE_XARRAY(decoder_registry);
> +static bool decoder_registry_reset_disable;
> +
> static inline bool is_multi_bridge(struct device *dev)
> {
> int i;
> @@ -660,6 +663,153 @@ static int map_targets(struct device *dev, void *data)
> return 0;
> }
>
> +static unsigned long cxld_registry_index(struct cxl_decoder *cxld)
> +{
> + struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> +
> + /*
> + * Upper nibble of a kernel pointer is 0xff, chop that to make
> + * space for a cxl_decoder id which should be less than 128
> + * given decoder count is a 4-bit field.
> + *
> + * While @port is reallocated each enumeration, @port->uport_dev
> + * is stable.
> + */
> + dev_WARN_ONCE(&port->dev, cxld->id >= 128,
> + "decoder id:%d out of range\n", cxld->id);
> + return (((unsigned long) port->uport_dev) << 4) | cxld->id;
> +}
> +
> +struct cxl_test_decoder {
> + union {
> + struct cxl_switch_decoder cxlsd;
> + struct cxl_endpoint_decoder cxled;
> + };
> + union {
> + struct cxl_dport *targets[CXL_DECODER_MAX_INTERLEAVE];
> + struct range dpa_range;
> + };
> +};
> +
> +static struct cxl_test_decoder *cxld_registry_find(struct cxl_decoder *cxld)
> +{
> + return xa_load(&decoder_registry, cxld_registry_index(cxld));
> +}
> +
> +#define dbg_cxld(port, msg, cxld) \
> + do { \
> + struct cxl_decoder *___d = (cxld); \
> + dev_dbg((port)->uport_dev, \
> + "decoder%d: %s range: %#llx-%#llx iw: %d ig: %d flags: %#lx\n", \
> + ___d->id, msg, ___d->hpa_range.start, \
> + ___d->hpa_range.end + 1, ___d->interleave_ways, \
> + ___d->interleave_granularity, ___d->flags); \
> + } while (0)
> +
> +static int mock_decoder_commit(struct cxl_decoder *cxld);
> +static int mock_decoder_reset(struct cxl_decoder *cxld);
> +
> +static void cxld_copy(struct cxl_decoder *a, struct cxl_decoder *b)
> +{
> + a->id = b->id;
> + a->hpa_range = b->hpa_range;
> + a->interleave_ways = b->interleave_ways;
> + a->interleave_granularity = b->interleave_granularity;
> + a->target_type = b->target_type;
> + a->flags = b->flags;
> + a->commit = mock_decoder_commit;
> + a->reset = mock_decoder_reset;
> +}
> +
> +static void cxld_registry_restore(struct cxl_decoder *cxld, struct cxl_test_decoder *td)
> +{
> + struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> +
> + if (is_switch_decoder(&cxld->dev)) {
> + struct cxl_switch_decoder *cxlsd = to_cxl_switch_decoder(&cxld->dev);
> +
> + dbg_cxld(port, "restore", &td->cxlsd.cxld);
> + cxld_copy(cxld, &td->cxlsd.cxld);
> + WARN_ON(cxlsd->nr_targets != td->cxlsd.nr_targets);
> +
> + /* convert saved dport devs to dports */
> + for (int i = 0; i < cxlsd->nr_targets; i++) {
> + struct cxl_dport *dport;
> +
> + if (!td->cxlsd.target[i])
> + continue;
> + dport = cxl_find_dport_by_dev(
> + port, (struct device *)td->cxlsd.target[i]);
> + WARN_ON(!dport);
> + cxlsd->target[i] = dport;
> + }
> + } else {
> + struct cxl_endpoint_decoder *cxled = to_cxl_endpoint_decoder(&cxld->dev);
> +
> + dbg_cxld(port, "restore", &td->cxled.cxld);
> + cxld_copy(cxld, &td->cxled.cxld);
> + cxled->state = td->cxled.state;
> + cxled->skip = td->cxled.skip;
> + if (range_len(&td->dpa_range))
> + devm_cxl_dpa_reserve(cxled, td->dpa_range.start,
> + range_len(&td->dpa_range),
> + td->cxled.skip);
> + if (cxld->flags & CXL_DECODER_F_ENABLE)
> + port->commit_end = cxld->id;
> + }
> +}
> +
> +static void __cxld_registry_save(struct cxl_test_decoder *td,
> + struct cxl_decoder *cxld)
> +{
> + if (is_switch_decoder(&cxld->dev)) {
> + struct cxl_switch_decoder *cxlsd = to_cxl_switch_decoder(&cxld->dev);
> +
> + cxld_copy(&td->cxlsd.cxld, cxld);
> + td->cxlsd.nr_targets = cxlsd->nr_targets;
> +
> + /* save dport devs as a stable placeholder for dports */
> + for (int i = 0; i < cxlsd->nr_targets; i++) {
> + if (!cxlsd->target[i])
> + continue;
> + td->cxlsd.target[i] =
> + (struct cxl_dport *)cxlsd->target[i]->dport_dev;
> + }
> + } else {
> + struct cxl_endpoint_decoder *cxled = to_cxl_endpoint_decoder(&cxld->dev);
> +
> + cxld_copy(&td->cxled.cxld, cxld);
> + td->cxled.state = cxled->state;
> + td->cxled.skip = cxled->skip;
> + if (cxled->dpa_res) {
> + td->dpa_range.start = cxled->dpa_res->start;
> + td->dpa_range.end = cxled->dpa_res->end;
> + } else {
> + td->dpa_range.start = 0;
> + td->dpa_range.end = -1;
> + }
> + }
> +}
> +
> +static void cxld_registry_save(struct cxl_test_decoder *td, struct cxl_decoder *cxld)
> +{
> + struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> +
> + dbg_cxld(port, "save", cxld);
> + __cxld_registry_save(td, cxld);
> +}
> +
> +static void cxld_registry_update(struct cxl_decoder *cxld)
> +{
> + struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> + struct cxl_test_decoder *td = cxld_registry_find(cxld);
> +
> + dev_WARN_ONCE(port->uport_dev, !td, "%s failed\n", __func__);
> +
> + dbg_cxld(port, "update", cxld);
> + __cxld_registry_save(td, cxld);
> +}
> +
> static int mock_decoder_commit(struct cxl_decoder *cxld)
> {
> struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> @@ -679,6 +829,7 @@ static int mock_decoder_commit(struct cxl_decoder *cxld)
>
> port->commit_end++;
> cxld->flags |= CXL_DECODER_F_ENABLE;
> + cxld_registry_update(cxld);
>
> return 0;
> }
> @@ -701,10 +852,43 @@ static int mock_decoder_reset(struct cxl_decoder *cxld)
>
> port->commit_end--;
> cxld->flags &= ~CXL_DECODER_F_ENABLE;
> + if (decoder_registry_reset_disable)
> + dev_dbg(port->uport_dev, "decoder%d: skip registry update\n",
> + cxld->id);
> + else
> + cxld_registry_update(cxld);
>
> return 0;
> }
>
> +static void cxld_registry_invalidate(void)
> +{
> + unsigned long index;
> + void *entry;
> +
> + xa_for_each(&decoder_registry, index, entry) {
> + xa_erase(&decoder_registry, index);
> + kfree(entry);
> + }
> +}
> +
> +static struct cxl_test_decoder *cxld_registry_new(struct cxl_decoder *cxld)
> +{
> + struct cxl_test_decoder *td __free(kfree) = kzalloc(sizeof(*td), GFP_KERNEL);
> +
> + if (!td)
> + return NULL;
> +
> + if (xa_insert(&decoder_registry, cxld_registry_index(cxld), td,
> + GFP_KERNEL)) {
> + WARN_ON(1);
> + return NULL;
> + }
> +
> + cxld_registry_save(td, cxld);
> + return no_free_ptr(td);
> +}
> +
> static void default_mock_decoder(struct cxl_decoder *cxld)
> {
> cxld->hpa_range = (struct range){
> @@ -717,6 +901,9 @@ static void default_mock_decoder(struct cxl_decoder *cxld)
> cxld->target_type = CXL_DECODER_HOSTONLYMEM;
> cxld->commit = mock_decoder_commit;
> cxld->reset = mock_decoder_reset;
> +
> + if (!cxld_registry_new(cxld))
> + dev_dbg(&cxld->dev, "failed to add to registry\n");
> }
>
> static int first_decoder(struct device *dev, void *data)
> @@ -738,6 +925,7 @@ static void mock_init_hdm_decoder(struct cxl_decoder *cxld)
> struct cxl_endpoint_decoder *cxled;
> struct cxl_switch_decoder *cxlsd;
> struct cxl_port *port, *iter;
> + struct cxl_test_decoder *td;
> const int size = SZ_512M;
> struct cxl_memdev *cxlmd;
> struct cxl_dport *dport;
> @@ -767,6 +955,12 @@ static void mock_init_hdm_decoder(struct cxl_decoder *cxld)
> port = cxled_to_port(cxled);
> }
>
> + td = cxld_registry_find(cxld);
> + if (td) {
> + cxld_registry_restore(cxld, td);
> + return;
> + }
> +
> /*
> * The first decoder on the first 2 devices on the first switch
> * attached to host-bridge0 mock a fake / static RAM region. All
> @@ -795,6 +989,8 @@ static void mock_init_hdm_decoder(struct cxl_decoder *cxld)
> devm_cxl_dpa_reserve(cxled, 0, size / cxld->interleave_ways, 0);
> cxld->commit = mock_decoder_commit;
> cxld->reset = mock_decoder_reset;
> + if (!cxld_registry_new(cxld))
> + dev_dbg(&cxld->dev, "failed to add to registry\n");
>
> /*
> * Now that endpoint decoder is set up, walk up the hierarchy
> @@ -837,6 +1033,7 @@ static void mock_init_hdm_decoder(struct cxl_decoder *cxld)
> .start = base,
> .end = base + size - 1,
> };
> + cxld_registry_update(cxld);
> put_device(dev);
> }
> }
> @@ -1233,6 +1430,73 @@ static void cxl_single_exit(void)
> }
> }
>
> +static ssize_t decoder_registry_invalidate_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + unsigned long index;
> + bool empty = true;
> + void *entry;
> +
> + xa_for_each(&decoder_registry, index, entry) {
> + empty = false;
> + break;
> + }
> +
> + return sysfs_emit(buf, "%d\n", !empty);
> +}
> +
> +static ssize_t decoder_registry_invalidate_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + bool invalidate;
> + int rc;
> +
> + rc = kstrtobool(buf, &invalidate);
> + if (rc)
> + return rc;
> +
> + guard(device)(dev);
> +
> + if (dev->driver)
> + return -EBUSY;
> +
> + cxld_registry_invalidate();
> + return count;
> +}
> +
> +static DEVICE_ATTR_RW(decoder_registry_invalidate);
> +
> +static ssize_t
> +decoder_registry_reset_disable_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return sysfs_emit(buf, "%d\n", decoder_registry_reset_disable);
> +}
> +
> +static ssize_t
> +decoder_registry_reset_disable_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + int rc;
> +
> + rc = kstrtobool(buf, &decoder_registry_reset_disable);
> + if (rc)
> + return rc;
> + return count;
> +}
> +
> +static DEVICE_ATTR_RW(decoder_registry_reset_disable);
> +
> +static struct attribute *cxl_acpi_attrs[] = {
> + &dev_attr_decoder_registry_invalidate.attr,
> + &dev_attr_decoder_registry_reset_disable.attr,
> + NULL
> +};
> +ATTRIBUTE_GROUPS(cxl_acpi);
> +
> static __init int cxl_test_init(void)
> {
> int rc, i;
> @@ -1377,6 +1641,7 @@ static __init int cxl_test_init(void)
>
> mock_companion(&acpi0017_mock, &cxl_acpi->dev);
> acpi0017_mock.dev.bus = &platform_bus_type;
> + cxl_acpi->dev.groups = cxl_acpi_groups;
>
> rc = platform_device_add(cxl_acpi);
> if (rc)
> @@ -1446,6 +1711,9 @@ static __exit void cxl_test_exit(void)
> depopulate_all_mock_resources();
> gen_pool_destroy(cxl_mock_pool);
> unregister_cxl_mock_ops(&cxl_mock_ops);
> +
> + cxld_registry_invalidate();
> + xa_destroy(&decoder_registry);
> }
>
> module_param(interleave_arithmetic, int, 0444);
> --
> 2.43.0
next prev parent reply other threads:[~2024-02-08 20:52 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-13 5:04 [RFC PATCH] cxl/region: Allow out of order assembly of autodiscovered regions alison.schofield
2024-01-14 23:38 ` kernel test robot
2024-01-15 0:38 ` kernel test robot
2024-01-15 2:33 ` kernel test robot
2024-01-18 19:46 ` Dan Williams
2024-02-08 20:52 ` Alison Schofield [this message]
2024-02-08 22:57 ` Dan Williams
2024-02-09 0:23 ` Alison Schofield
2024-02-09 5:25 ` Dan Williams
[not found] ` <CGME20240113050447epcas2p2097a5c49c1f0f9318ec4202843e169b8@epcms2p8>
2024-01-26 8:51 ` Wonjae Lee
2024-01-30 4:37 ` Alison Schofield
2024-01-31 1:02 ` Wonjae Lee
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=ZcU+9BxBD8IRsnTu@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 \
--cc=wj28.lee@samsung.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.