| 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
| ||
|
Message-ID: <661065683552f_e9f9f2946@iweiny-mobl.notmuch>
Date: Fri, 5 Apr 2024 13:56:08 -0700
From: Ira Weiny <ira.weiny@...el.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>, <ira.weiny@...el.com>
CC: Dave Jiang <dave.jiang@...el.com>, Fan Ni <fan.ni@...sung.com>, "Navneet
Singh" <navneet.singh@...el.com>, Dan Williams <dan.j.williams@...el.com>,
Davidlohr Bueso <dave@...olabs.net>, Alison Schofield
<alison.schofield@...el.com>, 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 06/26] cxl/port: Add Dynamic Capacity mode support to
endpoint decoders
Jonathan Cameron wrote:
> On Sun, 24 Mar 2024 16:18:09 -0700
> ira.weiny@...el.com wrote:
>
> > From: Navneet Singh <navneet.singh@...el.com>
> >
[snip]
> > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> > index fff2581b8033..8b3efaf6563c 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-cxl
> > +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> > @@ -316,23 +316,24 @@ Description:
> >
> >
> > What: /sys/bus/cxl/devices/decoderX.Y/mode
> > -Date: May, 2022
> > -KernelVersion: v6.0
> > +Date: May, 2022, June 2024
> > +KernelVersion: v6.0, v6.10 (dcY)
> > Contact: linux-cxl@...r.kernel.org
> > Description:
> > (RW) When a CXL decoder is of devtype "cxl_decoder_endpoint" it
> > translates from a host physical address range, to a device local
> > address range. Device-local address ranges are further split
> > - into a 'ram' (volatile memory) range and 'pmem' (persistent
> > - memory) range. The 'mode' attribute emits one of 'ram', 'pmem',
> > - 'mixed', or 'none'. The 'mixed' indication is for error cases
> > - when a decoder straddles the volatile/persistent partition
> > - boundary, and 'none' indicates the decoder is not actively
> > - decoding, or no DPA allocation policy has been set.
> > + into a 'ram' (volatile memory) range, 'pmem' (persistent
> > + memory) range, or Dynamic Capacity (DC) range. The 'mode'
> > + attribute emits one of 'ram', 'pmem', 'dcY', 'mixed', or
> > + 'none'. The 'mixed' indication is for error cases when a
> > + decoder straddles the volatile/persistent partition boundary,
>
> I love corners. What happen if no persistent and decoder straddles
> volatile / dc0? Would only happen if the bios was having fun I think..
Yep same issue with a wonky bios... I did not change this text. Only got reformatted. But it is worth changing to:
.. The 'mixed' indication is for error cases when a decoder
straddles partition boundaries, ...
>
> > + and 'none' indicates the decoder is not actively decoding, or
> > + no DPA allocation policy has been set.
> >
> > 'mode' can be written, when the decoder is in the 'disabled'
> > - state, with either 'ram' or 'pmem' to set the boundaries for the
> > - next allocation.
> > + state, with 'ram', 'pmem', or 'dcY' to set the boundaries for
> > + the next allocation.
> >
> >
> > What: /sys/bus/cxl/devices/decoderX.Y/dpa_resource
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index 66b8419fd0c3..e22b6f4f7145 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -255,6 +255,14 @@ static void devm_cxl_dpa_release(struct cxl_endpoint_decoder *cxled)
> > __cxl_dpa_release(cxled);
> > }
> >
> > +static int dc_mode_to_region_index(enum cxl_decoder_mode mode)
> > +{
> > + if (mode < CXL_DECODER_DC0 || CXL_DECODER_DC7 < mode)
> > + return -EINVAL;
> > +
> > + return mode - CXL_DECODER_DC0;
> > +}
> > +
> > static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
> > resource_size_t base, resource_size_t len,
> > resource_size_t skipped)
> > @@ -411,6 +419,7 @@ int cxl_dpa_set_mode(struct cxl_endpoint_decoder *cxled,
> > struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> > struct cxl_dev_state *cxlds = cxlmd->cxlds;
> > struct device *dev = &cxled->cxld.dev;
> > + int rc;
> >
> > guard(rwsem_write)(&cxl_dpa_rwsem);
> > if (cxled->cxld.flags & CXL_DECODER_F_ENABLE)
> > @@ -433,6 +442,16 @@ int cxl_dpa_set_mode(struct cxl_endpoint_decoder *cxled,
> > return -ENXIO;
> > }
> > break;
> > + case CXL_DECODER_DC0 ... CXL_DECODER_DC7:
> > + rc = dc_mode_to_region_index(mode);
> > + if (rc < 0)
> > + return rc;
>
> Can't fail, so you could not bother checking.. Seems very unlikely
> that function will gain other error cases in the future.
Sure, done.
>
> > +
> > + if (resource_size(&cxlds->dc_res[rc]) == 0) {
> > + dev_dbg(dev, "no available dynamic capacity\n");
> > + return -ENXIO;
> > + }
> > + break;
> > default:
> > dev_dbg(dev, "unsupported mode: %d\n", mode);
> > return -EINVAL;
> > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> > index e59d9d37aa65..80c0651794eb 100644
> > --- a/drivers/cxl/core/port.c
> > +++ b/drivers/cxl/core/port.c
> > @@ -208,6 +208,22 @@ static ssize_t mode_store(struct device *dev, struct device_attribute *attr,
> > mode = CXL_DECODER_PMEM;
> > else if (sysfs_streq(buf, "ram"))
> > mode = CXL_DECODER_RAM;
> > + else if (sysfs_streq(buf, "dc0"))
> > + mode = CXL_DECODER_DC0;
> > + else if (sysfs_streq(buf, "dc1"))
> > + mode = CXL_DECODER_DC1;
> > + else if (sysfs_streq(buf, "dc2"))
> > + mode = CXL_DECODER_DC2;
> > + else if (sysfs_streq(buf, "dc3"))
> > + mode = CXL_DECODER_DC3;
> > + else if (sysfs_streq(buf, "dc4"))
> > + mode = CXL_DECODER_DC4;
> > + else if (sysfs_streq(buf, "dc5"))
> > + mode = CXL_DECODER_DC5;
> > + else if (sysfs_streq(buf, "dc6"))
> > + mode = CXL_DECODER_DC6;
> > + else if (sysfs_streq(buf, "dc7"))
> > + mode = CXL_DECODER_DC7;
>
> Fully agree with the comment that a string + enum table and search
> is probably appropriate here.
>
Ok, done.
Ira
Powered by blists - more mailing lists