| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 13 Mar 2023 15:44:16 -0700
From: Ira Weiny <ira.weiny@...el.com>
To: <alison.schofield@...el.com>,
Dan Williams <dan.j.williams@...el.com>,
"Ira Weiny" <ira.weiny@...el.com>,
Vishal Verma <vishal.l.verma@...el.com>,
"Dave Jiang" <dave.jiang@...el.com>,
Ben Widawsky <bwidawsk@...nel.org>,
"Steven Rostedt" <rostedt@...dmis.org>
CC: Alison Schofield <alison.schofield@...el.com>,
<linux-cxl@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v8 4/6] cxl/region: Provide region info to the cxl_poison
trace event
alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@...el.com>
>
> User space may need to know which region, if any, maps the poison
> address(es) logged in a cxl_poison trace event. Since the mapping
> of DPAs (device physical addresses) to a region can change, the
> kernel must provide this information at the time the poison list
> is read. The event informs user space that at event <timestamp>
> this <region> mapped to this <DPA>, which is poisoned.
>
> The cxl_poison trace event is already wired up to log the region
> name and uuid if it receives param 'struct cxl_region'.
>
> In order to provide that cxl_region, add another method for gathering
> poison - by committed endpoint decoder mappings. This method is only
> available with CONFIG_CXL_REGION and is only used if a region actually
> maps the memdev where poison is being read. After the poison list is
> read for all the mapped resources, poison is read for the unmapped
> resources, and those events are logged without the region info.
>
> Mixed mode decoders are not currently supported in Linux. Add a debug
> message to the poison request path. That will serve as an alert that
> poison list retrieval needs to add support for mixed mode.
>
> The default method remains: read the poison by memdev resource.
>
> Signed-off-by: Alison Schofield <alison.schofield@...el.com>
> ---
> drivers/cxl/core/core.h | 5 ++
> drivers/cxl/core/memdev.c | 17 ++++++-
> drivers/cxl/core/region.c | 96 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 117 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index cde475e13216..4f507cb85926 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -25,7 +25,12 @@ void cxl_decoder_kill_region(struct cxl_endpoint_decoder *cxled);
> #define CXL_DAX_REGION_TYPE(x) (&cxl_dax_region_type)
> int cxl_region_init(void);
> void cxl_region_exit(void);
> +int cxl_get_poison_by_endpoint(struct device *dev, void *data);
> #else
> +static inline int cxl_get_poison_by_endpoint(struct device *dev, void *data)
> +{
> + return 0;
> +}
> static inline void cxl_decoder_kill_region(struct cxl_endpoint_decoder *cxled)
> {
> }
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index ea996057815e..5e65818d2171 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -139,14 +139,29 @@ static ssize_t trigger_poison_list_store(struct device *dev,
> const char *buf, size_t len)
> {
> struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> + struct cxl_port *port;
> bool trigger;
> int rc;
>
> if (kstrtobool(buf, &trigger) || !trigger)
> return -EINVAL;
>
> + port = dev_get_drvdata(&cxlmd->dev);
> + if (!port || !is_cxl_endpoint(port))
> + return -EINVAL;
> +
> down_read(&cxl_dpa_rwsem);
> - rc = cxl_get_poison_by_memdev(cxlmd);
> + if (port->commit_end == -1) {
> + /* No regions mapped to this memdev */
> + rc = cxl_get_poison_by_memdev(cxlmd);
> + } else {
> + /* Regions mapped, collect poison by endpoint */
> + rc = device_for_each_child(&port->dev, port,
> + cxl_get_poison_by_endpoint);
> + if (rc == 1)
> + rc = 0;
> + }
> +
> up_read(&cxl_dpa_rwsem);
>
> return rc ? rc : len;
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index f29028148806..1a558adfe32d 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2213,6 +2213,102 @@ struct cxl_pmem_region *to_cxl_pmem_region(struct device *dev)
> }
> EXPORT_SYMBOL_NS_GPL(to_cxl_pmem_region, CXL);
>
> +int cxl_get_poison_by_endpoint(struct device *dev, void *data)
> +{
> + struct cxl_endpoint_decoder *cxled;
> + struct cxl_port *port = data;
> + struct cxl_dev_state *cxlds;
> + struct cxl_memdev *cxlmd;
> + u64 offset, length;
> + int rc = 0;
> +
> + down_read(&cxl_dpa_rwsem);
> +
> + if (!is_endpoint_decoder(dev))
> + goto out;
> +
> + cxled = to_cxl_endpoint_decoder(dev);
> + if (!cxled->dpa_res || !resource_size(cxled->dpa_res))
> + goto out;
> +
> + /*
> + * Regions are only created with single mode decoders: pmem or ram.
> + * Linux does not currently support mixed mode decoders. This means
> + * that reading poison per endpoint decoder adheres to the spec
> + * requirement that poison reads of pmem and ram must be separated.
> + * CXL 3.0 Spec 8.2.9.8.4.1
> + *
> + * Watch for future support of mixed with a dev_dbg() msg.
> + */
> + if (cxled->mode == CXL_DECODER_MIXED) {
> + dev_dbg(dev, "poison list read unsupported in mixed mode\n");
> + goto out;
> + }
> +
> + cxlmd = cxled_to_memdev(cxled);
> + if (cxled->skip) {
> + offset = cxled->dpa_res->start - cxled->skip;
> + length = cxled->skip;
> + rc = cxl_mem_get_poison(cxlmd, offset, length, NULL);
> + if (rc == -EFAULT && cxled->mode == CXL_DECODER_RAM)
> + rc = 0;
> + if (rc)
> + goto out;
> + }
> +
> + offset = cxled->dpa_res->start;
> + length = cxled->dpa_res->end - offset + 1;
> + rc = cxl_mem_get_poison(cxlmd, offset, length, cxled->cxld.region);
> + if (rc == -EFAULT && cxled->mode == CXL_DECODER_RAM)
> + rc = 0;
> + if (rc)
> + goto out;
> +
> + /* Iterate until commit_end is reached */
> + if (cxled->cxld.id < port->commit_end)
> + goto out;
> +
> + /*
> + * Reach here with the last committed decoder only.
> + * Knowing that PMEM must always follow RAM, get poison
> + * for unmapped ranges based on the last decoder's mode:
> + * ram: scan remains of ram range, then scan for pmem
> + * pmem: scan remains of pmem range
> + */
> + cxlds = cxlmd->cxlds;
I wonder if this is the best place to put this final logic vs in
trigger_poison_list_store()?
I see that you need that last cxled info... So I think it is probably ok.
But it was odd trying to understand here.
Reviewed-by: Ira Weiny <ira.weiny@...el.com>
> +
> + if (cxled->mode == CXL_DECODER_RAM) {
> + offset = cxled->dpa_res->end + 1;
> + length = resource_size(&cxlds->ram_res) - offset;
> + rc = cxl_mem_get_poison(cxlmd, offset, length, NULL);
> + if (rc == -EFAULT)
> + rc = 0;
> + if (rc)
> + goto out;
> + }
> + if (cxled->mode == CXL_DECODER_PMEM) {
> + offset = cxled->dpa_res->end + 1;
> + length = resource_size(&cxlds->dpa_res) - offset;
> + if (!length) {
> + rc = 1;
> + goto out;
> + }
> + } else if (resource_size(&cxlds->pmem_res)) {
> + offset = cxlds->pmem_res.start;
> + length = resource_size(&cxlds->pmem_res);
> + } else {
> + rc = 1;
> + goto out;
> + }
> + /* Final get poison call. Return rc or 1 to stop iteration. */
> + rc = cxl_mem_get_poison(cxlmd, offset, length, NULL);
> + if (!rc)
> + rc = 1;
> +out:
> + up_read(&cxl_dpa_rwsem);
> + return rc;
> +}
> +
> static struct lock_class_key cxl_pmem_region_key;
>
> static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr)
> --
> 2.37.3
>
Powered by blists - more mailing lists