| 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: Wed, 10 Apr 2024 11:49:11 -0700
From: Alison Schofield <alison.schofield@...el.com>
To: ira.weiny@...el.com
Cc: Dave Jiang <dave.jiang@...el.com>, Fan Ni <fan.ni@...sung.com>,
Jonathan Cameron <Jonathan.Cameron@...wei.com>,
Navneet Singh <navneet.singh@...el.com>,
Dan Williams <dan.j.williams@...el.com>,
Davidlohr Bueso <dave@...olabs.net>,
Vishal Verma <vishal.l.verma@...el.com>,
linux-btrfs@...r.kernel.org, linux-cxl@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 02/26] cxl/core: Separate region mode from decoder mode
On Sun, Mar 24, 2024 at 04:18:05PM -0700, Ira Weiny wrote:
> From: Navneet Singh <navneet.singh@...el.com>
>
> Until now region modes and decoder modes were equivalent in that they
> were either PMEM or RAM. With the upcoming addition of Dynamic Capacity
> regions (which will represent an array of device regions [better named
> partitions] the index of which could be different on different
> interleaved devices), the mode of an endpoint decoder and a region will
> no longer be equivalent.
>
> Define a new region mode enumeration and adjust the code for it.
>
> Suggested-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> Signed-off-by: Navneet Singh <navneet.singh@...el.com>
> Co-developed-by: Ira Weiny <ira.weiny@...el.com>
> Signed-off-by: Ira Weiny <ira.weiny@...el.com>
>
> ---
> Changes for v1
> <none>
> ---
> drivers/cxl/core/region.c | 77 +++++++++++++++++++++++++++++++++++------------
> drivers/cxl/cxl.h | 26 ++++++++++++++--
> 2 files changed, 81 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 4c7fd2d5cccb..1723d17f121e 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -40,7 +40,7 @@ static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
> rc = down_read_interruptible(&cxl_region_rwsem);
> if (rc)
> return rc;
> - if (cxlr->mode != CXL_DECODER_PMEM)
> + if (cxlr->mode != CXL_REGION_PMEM)
> rc = sysfs_emit(buf, "\n");
> else
> rc = sysfs_emit(buf, "%pUb\n", &p->uuid);
> @@ -353,7 +353,7 @@ static umode_t cxl_region_visible(struct kobject *kobj, struct attribute *a,
> * Support tooling that expects to find a 'uuid' attribute for all
> * regions regardless of mode.
> */
> - if (a == &dev_attr_uuid.attr && cxlr->mode != CXL_DECODER_PMEM)
> + if (a == &dev_attr_uuid.attr && cxlr->mode != CXL_REGION_PMEM)
> return 0444;
> return a->mode;
> }
> @@ -516,7 +516,7 @@ static ssize_t mode_show(struct device *dev, struct device_attribute *attr,
> {
> struct cxl_region *cxlr = to_cxl_region(dev);
>
> - return sysfs_emit(buf, "%s\n", cxl_decoder_mode_name(cxlr->mode));
> + return sysfs_emit(buf, "%s\n", cxl_region_mode_name(cxlr->mode));
> }
> static DEVICE_ATTR_RO(mode);
>
> @@ -542,7 +542,7 @@ static int alloc_hpa(struct cxl_region *cxlr, resource_size_t size)
>
> /* ways, granularity and uuid (if PMEM) need to be set before HPA */
> if (!p->interleave_ways || !p->interleave_granularity ||
> - (cxlr->mode == CXL_DECODER_PMEM && uuid_is_null(&p->uuid)))
> + (cxlr->mode == CXL_REGION_PMEM && uuid_is_null(&p->uuid)))
> return -ENXIO;
>
> div64_u64_rem(size, (u64)SZ_256M * p->interleave_ways, &remainder);
> @@ -1683,6 +1683,17 @@ static int cxl_region_sort_targets(struct cxl_region *cxlr)
> return rc;
> }
>
> +static bool cxl_modes_compatible(enum cxl_region_mode rmode,
> + enum cxl_decoder_mode dmode)
> +{
Perhaps is_region_mode_compatible() ?
Seems we have precedence for asking these questions that have
boolean responses. I picked 'region' because it is the region
we are trying to construct.
> + if (rmode == CXL_REGION_RAM && dmode == CXL_DECODER_RAM)
> + return true;
> + if (rmode == CXL_REGION_PMEM && dmode == CXL_DECODER_PMEM)
> + return true;
> +
> + return false;
> +}
> +
> static int cxl_region_attach(struct cxl_region *cxlr,
> struct cxl_endpoint_decoder *cxled, int pos)
> {
> @@ -1693,9 +1704,11 @@ static int cxl_region_attach(struct cxl_region *cxlr,
> struct cxl_dport *dport;
> int rc = -ENXIO;
>
> - if (cxled->mode != cxlr->mode) {
> - dev_dbg(&cxlr->dev, "%s region mode: %d mismatch: %d\n",
> - dev_name(&cxled->cxld.dev), cxlr->mode, cxled->mode);
> + if (!cxl_modes_compatible(cxlr->mode, cxled->mode)) {
> + dev_dbg(&cxlr->dev, "%s region mode: %s mismatch decoder: %s\n",
> + dev_name(&cxled->cxld.dev),
> + cxl_region_mode_name(cxlr->mode),
> + cxl_decoder_mode_name(cxled->mode));
> return -EINVAL;
> }
Does the above return bypass this next code segment (not in your diff):
if (cxled->mode == CXL_DECODER_DEAD) {
dev_dbg(&cxlr->dev, "%s dead\n", dev_name(&cxled->cxld.dev));
return -ENODEV;
}
It seems we are changing the return value on DEAD.
More below where a new check for DEAD is added ...
snip
> /* Establish an empty region covering the given HPA range */
> static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
> struct cxl_endpoint_decoder *cxled)
> @@ -2808,12 +2840,17 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
> struct cxl_port *port = cxlrd_to_port(cxlrd);
> struct range *hpa = &cxled->cxld.hpa_range;
> struct cxl_region_params *p;
> + enum cxl_region_mode mode;
> struct cxl_region *cxlr;
> struct resource *res;
> int rc;
>
> + if (cxled->mode == CXL_DECODER_DEAD)
> + return ERR_PTR(-EINVAL);
I see this addition, but it is in a different place and with
a different return value. Help me understand that this is no
change in behavior.
> +
> + mode = cxl_decoder_to_region_mode(cxled->mode);
> do {
> - cxlr = __create_region(cxlrd, cxled->mode,
> + cxlr = __create_region(cxlrd, mode,
> atomic_read(&cxlrd->region_id));
> } while (IS_ERR(cxlr) && PTR_ERR(cxlr) == -EBUSY);
>
snip
> /*
> * Track whether this decoder is reserved for region autodiscovery, or
> * free for userspace provisioning.
> @@ -511,7 +532,8 @@ struct cxl_region_params {
> * struct cxl_region - CXL region
> * @dev: This region's device
> * @id: This region's id. Id is globally unique across all regions
> - * @mode: Endpoint decoder allocation / access mode
> + * @mode: Region mode which defines which endpoint decoder mode the region is
> + * compatible with
Maybe...
@mode: Region mode used for decoder compatibility check
snip to end
--Alison
>
Powered by blists - more mailing lists