| Message ID | 167564537678.847146.4066579806086171091.stgit@dwillia2-xfh.jf.intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | CXL RAM and the 'Soft Reserved' => 'System RAM' default | expand |
On Sun, 05 Feb 2023 17:02:56 -0800 Dan Williams <dan.j.williams@intel.com> wrote: > Expand the region creation infrastructure to enable 'ram' > (volatile-memory) regions. The internals of create_pmem_region_store() > and create_pmem_region_show() are factored out into helpers > __create_region() and __create_region_show() for the 'ram' case to > reuse. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Entirely trivial comments inline. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > Documentation/ABI/testing/sysfs-bus-cxl | 22 +++++----- > drivers/cxl/core/core.h | 1 > drivers/cxl/core/port.c | 14 ++++++ > drivers/cxl/core/region.c | 71 +++++++++++++++++++++++++------ > 4 files changed, 82 insertions(+), 26 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl > index 4c4e1cbb1169..3acf2f17a73f 100644 > --- a/Documentation/ABI/testing/sysfs-bus-cxl > +++ b/Documentation/ABI/testing/sysfs-bus-cxl > @@ -285,20 +285,20 @@ Description: > interleave_granularity). > > > -What: /sys/bus/cxl/devices/decoderX.Y/create_pmem_region > -Date: May, 2022 > -KernelVersion: v6.0 > +What: /sys/bus/cxl/devices/decoderX.Y/create_{pmem,ram}_region > +Date: May, 2022, January, 2023 > +KernelVersion: v6.0 (pmem), v6.3 (ram) > Contact: linux-cxl@vger.kernel.org > Description: > (RW) Write a string in the form 'regionZ' to start the process > - of defining a new persistent memory region (interleave-set) > - within the decode range bounded by root decoder 'decoderX.Y'. > - The value written must match the current value returned from > - reading this attribute. An atomic compare exchange operation is > - done on write to assign the requested id to a region and > - allocate the region-id for the next creation attempt. EBUSY is > - returned if the region name written does not match the current > - cached value. > + of defining a new persistent, or volatile memory region > + (interleave-set) within the decode range bounded by root decoder > + 'decoderX.Y'. The value written must match the current value > + returned from reading this attribute. An atomic compare exchange > + operation is done on write to assign the requested id to a > + region and allocate the region-id for the next creation attempt. > + EBUSY is returned if the region name written does not match the > + current cached value. > > > What: /sys/bus/cxl/devices/decoderX.Y/delete_region > diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h > index 8c04672dca56..5eb873da5a30 100644 > --- a/drivers/cxl/core/core.h > +++ b/drivers/cxl/core/core.h > @@ -11,6 +11,7 @@ extern struct attribute_group cxl_base_attribute_group; > > #ifdef CONFIG_CXL_REGION > extern struct device_attribute dev_attr_create_pmem_region; > +extern struct device_attribute dev_attr_create_ram_region; > extern struct device_attribute dev_attr_delete_region; > extern struct device_attribute dev_attr_region; > extern const struct device_type cxl_pmem_region_type; > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > index 8566451cb22f..47e450c3a5a9 100644 > --- a/drivers/cxl/core/port.c > +++ b/drivers/cxl/core/port.c > @@ -294,6 +294,7 @@ static struct attribute *cxl_decoder_root_attrs[] = { > &dev_attr_cap_type3.attr, > &dev_attr_target_list.attr, > SET_CXL_REGION_ATTR(create_pmem_region) > + SET_CXL_REGION_ATTR(create_ram_region) > SET_CXL_REGION_ATTR(delete_region) > NULL, > }; > @@ -305,6 +306,13 @@ static bool can_create_pmem(struct cxl_root_decoder *cxlrd) > return (cxlrd->cxlsd.cxld.flags & flags) == flags; > } > > +static bool can_create_ram(struct cxl_root_decoder *cxlrd) > +{ > + unsigned long flags = CXL_DECODER_F_TYPE3 | CXL_DECODER_F_RAM; > + > + return (cxlrd->cxlsd.cxld.flags & flags) == flags; > +} > + > static umode_t cxl_root_decoder_visible(struct kobject *kobj, struct attribute *a, int n) > { > struct device *dev = kobj_to_dev(kobj); > @@ -313,7 +321,11 @@ static umode_t cxl_root_decoder_visible(struct kobject *kobj, struct attribute * > if (a == CXL_REGION_ATTR(create_pmem_region) && !can_create_pmem(cxlrd)) > return 0; > > - if (a == CXL_REGION_ATTR(delete_region) && !can_create_pmem(cxlrd)) > + if (a == CXL_REGION_ATTR(create_ram_region) && !can_create_ram(cxlrd)) > + return 0; > + > + if (a == CXL_REGION_ATTR(delete_region) && > + !(can_create_pmem(cxlrd) || can_create_ram(cxlrd))) > return 0; > > return a->mode; > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 53d6dbe4de6d..8dea49c021b8 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -1685,6 +1685,15 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd, > struct device *dev; > int rc; > > + switch (mode) { > + case CXL_DECODER_RAM: > + case CXL_DECODER_PMEM: > + break; > + default: > + dev_err(&cxlrd->cxlsd.cxld.dev, "unsupported mode %d\n", mode); > + return ERR_PTR(-EINVAL); > + } > + > cxlr = cxl_region_alloc(cxlrd, id); > if (IS_ERR(cxlr)) > return cxlr; > @@ -1713,12 +1722,38 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd, > return ERR_PTR(rc); > } > > +static ssize_t __create_region_show(struct cxl_root_decoder *cxlrd, char *buf) > +{ > + return sysfs_emit(buf, "region%u\n", atomic_read(&cxlrd->region_id)); > +} > + > static ssize_t create_pmem_region_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > - struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev); > + return __create_region_show(to_cxl_root_decoder(dev), buf); > +} > > - return sysfs_emit(buf, "region%u\n", atomic_read(&cxlrd->region_id)); > +static ssize_t create_ram_region_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + return __create_region_show(to_cxl_root_decoder(dev), buf); > +} > + > +static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd, > + enum cxl_decoder_mode mode, int id) > +{ > + int rc; > + > + rc = memregion_alloc(GFP_KERNEL); > + if (rc < 0) > + return ERR_PTR(rc); > + > + if (atomic_cmpxchg(&cxlrd->region_id, id, rc) != id) { > + memregion_free(rc); > + return ERR_PTR(-EBUSY); > + } > + > + return devm_cxl_add_region(cxlrd, id, mode, CXL_DECODER_EXPANDER); > } > > static ssize_t create_pmem_region_store(struct device *dev, > @@ -1727,29 +1762,37 @@ static ssize_t create_pmem_region_store(struct device *dev, > { > struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev); > struct cxl_region *cxlr; > - int id, rc; > + int rc, id; > > rc = sscanf(buf, "region%d\n", &id); > if (rc != 1) > return -EINVAL; > > - rc = memregion_alloc(GFP_KERNEL); > - if (rc < 0) > - return rc; > + cxlr = __create_region(cxlrd, CXL_DECODER_PMEM, id); > + if (IS_ERR(cxlr)) > + return PTR_ERR(cxlr); I'd have a blank line here - see below. > + return len; > +} > +DEVICE_ATTR_RW(create_pmem_region); > > - if (atomic_cmpxchg(&cxlrd->region_id, id, rc) != id) { > - memregion_free(rc); > - return -EBUSY; > - } > +static ssize_t create_ram_region_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t len) > +{ > + struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev); > + struct cxl_region *cxlr; > + int rc, id; > > - cxlr = devm_cxl_add_region(cxlrd, id, CXL_DECODER_PMEM, > - CXL_DECODER_EXPANDER); > + rc = sscanf(buf, "region%d\n", &id); > + if (rc != 1) > + return -EINVAL; > + > + cxlr = __create_region(cxlrd, CXL_DECODER_RAM, id); > if (IS_ERR(cxlr)) > return PTR_ERR(cxlr); > - Just so you know I read to the end! Spurious unrelated white space change :) > return len; > } > -DEVICE_ATTR_RW(create_pmem_region); > +DEVICE_ATTR_RW(create_ram_region); > > static ssize_t region_show(struct device *dev, struct device_attribute *attr, > char *buf) >
On Sun, Feb 05, 2023 at 05:02:56PM -0800, Dan Williams wrote: > Expand the region creation infrastructure to enable 'ram' > (volatile-memory) regions. The internals of create_pmem_region_store() > and create_pmem_region_show() are factored out into helpers > __create_region() and __create_region_show() for the 'ram' case to > reuse. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > [...] > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > index 8566451cb22f..47e450c3a5a9 100644 > --- a/drivers/cxl/core/port.c > +++ b/drivers/cxl/core/port.c > @@ -294,6 +294,7 @@ static struct attribute *cxl_decoder_root_attrs[] = { > &dev_attr_cap_type3.attr, > &dev_attr_target_list.attr, > SET_CXL_REGION_ATTR(create_pmem_region) > + SET_CXL_REGION_ATTR(create_ram_region) > SET_CXL_REGION_ATTR(delete_region) > NULL, > }; > @@ -305,6 +306,13 @@ static bool can_create_pmem(struct cxl_root_decoder *cxlrd) > return (cxlrd->cxlsd.cxld.flags & flags) == flags; > } > > +static bool can_create_ram(struct cxl_root_decoder *cxlrd) > +{ > + unsigned long flags = CXL_DECODER_F_TYPE3 | CXL_DECODER_F_RAM; > + > + return (cxlrd->cxlsd.cxld.flags & flags) == flags; > +} > + does this collide with either CXL_DECODE_F_ENABLE or CXL_DECODER_F_AUTO? I think obviously if it's already enabled creating new regions in a decoder doesn't make sense, but if F_AUTO is set, does that imply the region settings cannot be changed?
Jonathan Cameron wrote: > On Sun, 05 Feb 2023 17:02:56 -0800 > Dan Williams <dan.j.williams@intel.com> wrote: > > > Expand the region creation infrastructure to enable 'ram' > > (volatile-memory) regions. The internals of create_pmem_region_store() > > and create_pmem_region_show() are factored out into helpers > > __create_region() and __create_region_show() for the 'ram' case to > > reuse. > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > Entirely trivial comments inline. > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> [..] > > @@ -1727,29 +1762,37 @@ static ssize_t create_pmem_region_store(struct device *dev, > > { > > struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev); > > struct cxl_region *cxlr; > > - int id, rc; > > + int rc, id; > > > > rc = sscanf(buf, "region%d\n", &id); > > if (rc != 1) > > return -EINVAL; > > > > - rc = memregion_alloc(GFP_KERNEL); > > - if (rc < 0) > > - return rc; > > + cxlr = __create_region(cxlrd, CXL_DECODER_PMEM, id); > > + if (IS_ERR(cxlr)) > > + return PTR_ERR(cxlr); > > I'd have a blank line here - see below. > [..] > > if (IS_ERR(cxlr)) > > return PTR_ERR(cxlr); > > - > > Just so you know I read to the end! > > Spurious unrelated white space change :) ...and I can't even blame that on clang-format! Will fix.
Dan Williams wrote: > Expand the region creation infrastructure to enable 'ram' > (volatile-memory) regions. The internals of create_pmem_region_store() > and create_pmem_region_show() are factored out into helpers > __create_region() and __create_region_show() for the 'ram' case to > reuse. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
On Mon, Feb 06, 2023 at 01:57:05PM -0800, Dan Williams wrote: > Gregory Price wrote: > > On Sun, Feb 05, 2023 at 05:02:56PM -0800, Dan Williams wrote: > > > Expand the region creation infrastructure to enable 'ram' > > > (volatile-memory) regions. The internals of create_pmem_region_store() > > > and create_pmem_region_show() are factored out into helpers > > > __create_region() and __create_region_show() for the 'ram' case to > > > reuse. > > > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > > --- > > > > [..] > > > @@ -305,6 +306,13 @@ static bool can_create_pmem(struct cxl_root_decoder *cxlrd) > > > return (cxlrd->cxlsd.cxld.flags & flags) == flags; > > > } > > > > > > +static bool can_create_ram(struct cxl_root_decoder *cxlrd) > > > +{ > > > + unsigned long flags = CXL_DECODER_F_TYPE3 | CXL_DECODER_F_RAM; > > > + > > > + return (cxlrd->cxlsd.cxld.flags & flags) == flags; > > > +} > > > + > > > > does this collide with either CXL_DECODE_F_ENABLE or CXL_DECODER_F_AUTO? > > > > I think obviously if it's already enabled creating new regions in a > > decoder doesn't make sense, but if F_AUTO is set, does that imply > > the region settings cannot be changed? > > That just cares if the root decoder supports TYPE3 and RAM independent > of ENABLE or AUTO. Root decoders are always enabled. The AUTO flag, > which is not applicable to root decoders, is just there to hold off > userspace racing the attachment of endpoint decoders to a region until > the autodiscovery process has completed. Once that completes and the > region has been enabled then it can be destroyed to clear AUTO. Reviewed-by: Gregory Price <gregory.price@memverge.com>
Gregory Price wrote: > On Sun, Feb 05, 2023 at 05:02:56PM -0800, Dan Williams wrote: > > Expand the region creation infrastructure to enable 'ram' > > (volatile-memory) regions. The internals of create_pmem_region_store() > > and create_pmem_region_show() are factored out into helpers > > __create_region() and __create_region_show() for the 'ram' case to > > reuse. > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > --- > > [..] > > @@ -305,6 +306,13 @@ static bool can_create_pmem(struct cxl_root_decoder *cxlrd) > > return (cxlrd->cxlsd.cxld.flags & flags) == flags; > > } > > > > +static bool can_create_ram(struct cxl_root_decoder *cxlrd) > > +{ > > + unsigned long flags = CXL_DECODER_F_TYPE3 | CXL_DECODER_F_RAM; > > + > > + return (cxlrd->cxlsd.cxld.flags & flags) == flags; > > +} > > + > > does this collide with either CXL_DECODE_F_ENABLE or CXL_DECODER_F_AUTO? > > I think obviously if it's already enabled creating new regions in a > decoder doesn't make sense, but if F_AUTO is set, does that imply > the region settings cannot be changed? That just cares if the root decoder supports TYPE3 and RAM independent of ENABLE or AUTO. Root decoders are always enabled. The AUTO flag, which is not applicable to root decoders, is just there to hold off userspace racing the attachment of endpoint decoders to a region until the autodiscovery process has completed. Once that completes and the region has been enabled then it can be destroyed to clear AUTO.
On 2/5/23 6:02 PM, Dan Williams wrote: > Expand the region creation infrastructure to enable 'ram' > (volatile-memory) regions. The internals of create_pmem_region_store() > and create_pmem_region_show() are factored out into helpers > __create_region() and __create_region_show() for the 'ram' case to > reuse. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Minor comment below, otherwise: Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > Documentation/ABI/testing/sysfs-bus-cxl | 22 +++++----- > drivers/cxl/core/core.h | 1 > drivers/cxl/core/port.c | 14 ++++++ > drivers/cxl/core/region.c | 71 +++++++++++++++++++++++++------ > 4 files changed, 82 insertions(+), 26 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl > index 4c4e1cbb1169..3acf2f17a73f 100644 > --- a/Documentation/ABI/testing/sysfs-bus-cxl > +++ b/Documentation/ABI/testing/sysfs-bus-cxl > @@ -285,20 +285,20 @@ Description: > interleave_granularity). > > > -What: /sys/bus/cxl/devices/decoderX.Y/create_pmem_region > -Date: May, 2022 > -KernelVersion: v6.0 > +What: /sys/bus/cxl/devices/decoderX.Y/create_{pmem,ram}_region > +Date: May, 2022, January, 2023 > +KernelVersion: v6.0 (pmem), v6.3 (ram) > Contact: linux-cxl@vger.kernel.org > Description: > (RW) Write a string in the form 'regionZ' to start the process > - of defining a new persistent memory region (interleave-set) > - within the decode range bounded by root decoder 'decoderX.Y'. > - The value written must match the current value returned from > - reading this attribute. An atomic compare exchange operation is > - done on write to assign the requested id to a region and > - allocate the region-id for the next creation attempt. EBUSY is > - returned if the region name written does not match the current > - cached value. > + of defining a new persistent, or volatile memory region > + (interleave-set) within the decode range bounded by root decoder > + 'decoderX.Y'. The value written must match the current value > + returned from reading this attribute. An atomic compare exchange > + operation is done on write to assign the requested id to a > + region and allocate the region-id for the next creation attempt. > + EBUSY is returned if the region name written does not match the -EBUSY? DJ > + current cached value. > > > What: /sys/bus/cxl/devices/decoderX.Y/delete_region > diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h > index 8c04672dca56..5eb873da5a30 100644 > --- a/drivers/cxl/core/core.h > +++ b/drivers/cxl/core/core.h > @@ -11,6 +11,7 @@ extern struct attribute_group cxl_base_attribute_group; > > #ifdef CONFIG_CXL_REGION > extern struct device_attribute dev_attr_create_pmem_region; > +extern struct device_attribute dev_attr_create_ram_region; > extern struct device_attribute dev_attr_delete_region; > extern struct device_attribute dev_attr_region; > extern const struct device_type cxl_pmem_region_type; > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > index 8566451cb22f..47e450c3a5a9 100644 > --- a/drivers/cxl/core/port.c > +++ b/drivers/cxl/core/port.c > @@ -294,6 +294,7 @@ static struct attribute *cxl_decoder_root_attrs[] = { > &dev_attr_cap_type3.attr, > &dev_attr_target_list.attr, > SET_CXL_REGION_ATTR(create_pmem_region) > + SET_CXL_REGION_ATTR(create_ram_region) > SET_CXL_REGION_ATTR(delete_region) > NULL, > }; > @@ -305,6 +306,13 @@ static bool can_create_pmem(struct cxl_root_decoder *cxlrd) > return (cxlrd->cxlsd.cxld.flags & flags) == flags; > } > > +static bool can_create_ram(struct cxl_root_decoder *cxlrd) > +{ > + unsigned long flags = CXL_DECODER_F_TYPE3 | CXL_DECODER_F_RAM; > + > + return (cxlrd->cxlsd.cxld.flags & flags) == flags; > +} > + > static umode_t cxl_root_decoder_visible(struct kobject *kobj, struct attribute *a, int n) > { > struct device *dev = kobj_to_dev(kobj); > @@ -313,7 +321,11 @@ static umode_t cxl_root_decoder_visible(struct kobject *kobj, struct attribute * > if (a == CXL_REGION_ATTR(create_pmem_region) && !can_create_pmem(cxlrd)) > return 0; > > - if (a == CXL_REGION_ATTR(delete_region) && !can_create_pmem(cxlrd)) > + if (a == CXL_REGION_ATTR(create_ram_region) && !can_create_ram(cxlrd)) > + return 0; > + > + if (a == CXL_REGION_ATTR(delete_region) && > + !(can_create_pmem(cxlrd) || can_create_ram(cxlrd))) > return 0; > > return a->mode; > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 53d6dbe4de6d..8dea49c021b8 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -1685,6 +1685,15 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd, > struct device *dev; > int rc; > > + switch (mode) { > + case CXL_DECODER_RAM: > + case CXL_DECODER_PMEM: > + break; > + default: > + dev_err(&cxlrd->cxlsd.cxld.dev, "unsupported mode %d\n", mode); > + return ERR_PTR(-EINVAL); > + } > + > cxlr = cxl_region_alloc(cxlrd, id); > if (IS_ERR(cxlr)) > return cxlr; > @@ -1713,12 +1722,38 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd, > return ERR_PTR(rc); > } > > +static ssize_t __create_region_show(struct cxl_root_decoder *cxlrd, char *buf) > +{ > + return sysfs_emit(buf, "region%u\n", atomic_read(&cxlrd->region_id)); > +} > + > static ssize_t create_pmem_region_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > - struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev); > + return __create_region_show(to_cxl_root_decoder(dev), buf); > +} > > - return sysfs_emit(buf, "region%u\n", atomic_read(&cxlrd->region_id)); > +static ssize_t create_ram_region_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + return __create_region_show(to_cxl_root_decoder(dev), buf); > +} > + > +static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd, > + enum cxl_decoder_mode mode, int id) > +{ > + int rc; > + > + rc = memregion_alloc(GFP_KERNEL); > + if (rc < 0) > + return ERR_PTR(rc); > + > + if (atomic_cmpxchg(&cxlrd->region_id, id, rc) != id) { > + memregion_free(rc); > + return ERR_PTR(-EBUSY); > + } > + > + return devm_cxl_add_region(cxlrd, id, mode, CXL_DECODER_EXPANDER); > } > > static ssize_t create_pmem_region_store(struct device *dev, > @@ -1727,29 +1762,37 @@ static ssize_t create_pmem_region_store(struct device *dev, > { > struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev); > struct cxl_region *cxlr; > - int id, rc; > + int rc, id; > > rc = sscanf(buf, "region%d\n", &id); > if (rc != 1) > return -EINVAL; > > - rc = memregion_alloc(GFP_KERNEL); > - if (rc < 0) > - return rc; > + cxlr = __create_region(cxlrd, CXL_DECODER_PMEM, id); > + if (IS_ERR(cxlr)) > + return PTR_ERR(cxlr); > + return len; > +} > +DEVICE_ATTR_RW(create_pmem_region); > > - if (atomic_cmpxchg(&cxlrd->region_id, id, rc) != id) { > - memregion_free(rc); > - return -EBUSY; > - } > +static ssize_t create_ram_region_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t len) > +{ > + struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev); > + struct cxl_region *cxlr; > + int rc, id; > > - cxlr = devm_cxl_add_region(cxlrd, id, CXL_DECODER_PMEM, > - CXL_DECODER_EXPANDER); > + rc = sscanf(buf, "region%d\n", &id); > + if (rc != 1) > + return -EINVAL; > + > + cxlr = __create_region(cxlrd, CXL_DECODER_RAM, id); > if (IS_ERR(cxlr)) > return PTR_ERR(cxlr); > - > return len; > } > -DEVICE_ATTR_RW(create_pmem_region); > +DEVICE_ATTR_RW(create_ram_region); > > static ssize_t region_show(struct device *dev, struct device_attribute *attr, > char *buf) >
Dave Jiang wrote: > > > On 2/5/23 6:02 PM, Dan Williams wrote: > > Expand the region creation infrastructure to enable 'ram' > > (volatile-memory) regions. The internals of create_pmem_region_store() > > and create_pmem_region_show() are factored out into helpers > > __create_region() and __create_region_show() for the 'ram' case to > > reuse. > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > Minor comment below, otherwise: > Reviewed-by: Dave Jiang <dave.jiang@intel.com> > [..] > > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl > > index 4c4e1cbb1169..3acf2f17a73f 100644 > > --- a/Documentation/ABI/testing/sysfs-bus-cxl > > +++ b/Documentation/ABI/testing/sysfs-bus-cxl > > @@ -285,20 +285,20 @@ Description: > > interleave_granularity). > > > > > > -What: /sys/bus/cxl/devices/decoderX.Y/create_pmem_region > > -Date: May, 2022 > > -KernelVersion: v6.0 > > +What: /sys/bus/cxl/devices/decoderX.Y/create_{pmem,ram}_region > > +Date: May, 2022, January, 2023 > > +KernelVersion: v6.0 (pmem), v6.3 (ram) > > Contact: linux-cxl@vger.kernel.org > > Description: > > (RW) Write a string in the form 'regionZ' to start the process > > - of defining a new persistent memory region (interleave-set) > > - within the decode range bounded by root decoder 'decoderX.Y'. > > - The value written must match the current value returned from > > - reading this attribute. An atomic compare exchange operation is > > - done on write to assign the requested id to a region and > > - allocate the region-id for the next creation attempt. EBUSY is > > - returned if the region name written does not match the current > > - cached value. > > + of defining a new persistent, or volatile memory region > > + (interleave-set) within the decode range bounded by root decoder > > + 'decoderX.Y'. The value written must match the current value > > + returned from reading this attribute. An atomic compare exchange > > + operation is done on write to assign the requested id to a > > + region and allocate the region-id for the next creation attempt. > > + EBUSY is returned if the region name written does not match the > > -EBUSY? Userspace errno values are positive. So "$?" after "echo $region > create_pmem_region" will be 16 not -16 in the busy case.
On Sun, 2023-02-05 at 17:02 -0800, Dan Williams wrote: > Expand the region creation infrastructure to enable 'ram' > (volatile-memory) regions. The internals of create_pmem_region_store() > and create_pmem_region_show() are factored out into helpers > __create_region() and __create_region_show() for the 'ram' case to > reuse. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > Documentation/ABI/testing/sysfs-bus-cxl | 22 +++++----- > drivers/cxl/core/core.h | 1 > drivers/cxl/core/port.c | 14 ++++++ > drivers/cxl/core/region.c | 71 +++++++++++++++++++++++++------ > 4 files changed, 82 insertions(+), 26 deletions(-) Looks good, Reviewed-by: Vishal Verma <vishal.l.verma@intel.com> > > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl > index 4c4e1cbb1169..3acf2f17a73f 100644 > --- a/Documentation/ABI/testing/sysfs-bus-cxl > +++ b/Documentation/ABI/testing/sysfs-bus-cxl > @@ -285,20 +285,20 @@ Description: > interleave_granularity). > > > -What: /sys/bus/cxl/devices/decoderX.Y/create_pmem_region > -Date: May, 2022 > -KernelVersion: v6.0 > +What: /sys/bus/cxl/devices/decoderX.Y/create_{pmem,ram}_region > +Date: May, 2022, January, 2023 > +KernelVersion: v6.0 (pmem), v6.3 (ram) > Contact: linux-cxl@vger.kernel.org > Description: > (RW) Write a string in the form 'regionZ' to start the process > - of defining a new persistent memory region (interleave-set) > - within the decode range bounded by root decoder 'decoderX.Y'. > - The value written must match the current value returned from > - reading this attribute. An atomic compare exchange operation is > - done on write to assign the requested id to a region and > - allocate the region-id for the next creation attempt. EBUSY is > - returned if the region name written does not match the current > - cached value. > + of defining a new persistent, or volatile memory region > + (interleave-set) within the decode range bounded by root decoder > + 'decoderX.Y'. The value written must match the current value > + returned from reading this attribute. An atomic compare exchange > + operation is done on write to assign the requested id to a > + region and allocate the region-id for the next creation attempt. > + EBUSY is returned if the region name written does not match the > + current cached value. > > > What: /sys/bus/cxl/devices/decoderX.Y/delete_region > diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h > index 8c04672dca56..5eb873da5a30 100644 > --- a/drivers/cxl/core/core.h > +++ b/drivers/cxl/core/core.h > @@ -11,6 +11,7 @@ extern struct attribute_group cxl_base_attribute_group; > > #ifdef CONFIG_CXL_REGION > extern struct device_attribute dev_attr_create_pmem_region; > +extern struct device_attribute dev_attr_create_ram_region; > extern struct device_attribute dev_attr_delete_region; > extern struct device_attribute dev_attr_region; > extern const struct device_type cxl_pmem_region_type; > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > index 8566451cb22f..47e450c3a5a9 100644 > --- a/drivers/cxl/core/port.c > +++ b/drivers/cxl/core/port.c > @@ -294,6 +294,7 @@ static struct attribute *cxl_decoder_root_attrs[] = { > &dev_attr_cap_type3.attr, > &dev_attr_target_list.attr, > SET_CXL_REGION_ATTR(create_pmem_region) > + SET_CXL_REGION_ATTR(create_ram_region) > SET_CXL_REGION_ATTR(delete_region) > NULL, > }; > @@ -305,6 +306,13 @@ static bool can_create_pmem(struct cxl_root_decoder *cxlrd) > return (cxlrd->cxlsd.cxld.flags & flags) == flags; > } > > +static bool can_create_ram(struct cxl_root_decoder *cxlrd) > +{ > + unsigned long flags = CXL_DECODER_F_TYPE3 | CXL_DECODER_F_RAM; > + > + return (cxlrd->cxlsd.cxld.flags & flags) == flags; > +} > + > static umode_t cxl_root_decoder_visible(struct kobject *kobj, struct attribute *a, int n) > { > struct device *dev = kobj_to_dev(kobj); > @@ -313,7 +321,11 @@ static umode_t cxl_root_decoder_visible(struct kobject *kobj, struct attribute * > if (a == CXL_REGION_ATTR(create_pmem_region) && !can_create_pmem(cxlrd)) > return 0; > > - if (a == CXL_REGION_ATTR(delete_region) && !can_create_pmem(cxlrd)) > + if (a == CXL_REGION_ATTR(create_ram_region) && !can_create_ram(cxlrd)) > + return 0; > + > + if (a == CXL_REGION_ATTR(delete_region) && > + !(can_create_pmem(cxlrd) || can_create_ram(cxlrd))) > return 0; > > return a->mode; > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index 53d6dbe4de6d..8dea49c021b8 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -1685,6 +1685,15 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd, > struct device *dev; > int rc; > > + switch (mode) { > + case CXL_DECODER_RAM: > + case CXL_DECODER_PMEM: > + break; > + default: > + dev_err(&cxlrd->cxlsd.cxld.dev, "unsupported mode %d\n", mode); > + return ERR_PTR(-EINVAL); > + } > + > cxlr = cxl_region_alloc(cxlrd, id); > if (IS_ERR(cxlr)) > return cxlr; > @@ -1713,12 +1722,38 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd, > return ERR_PTR(rc); > } > > +static ssize_t __create_region_show(struct cxl_root_decoder *cxlrd, char *buf) > +{ > + return sysfs_emit(buf, "region%u\n", atomic_read(&cxlrd->region_id)); > +} > + > static ssize_t create_pmem_region_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > - struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev); > + return __create_region_show(to_cxl_root_decoder(dev), buf); > +} > > - return sysfs_emit(buf, "region%u\n", atomic_read(&cxlrd->region_id)); > +static ssize_t create_ram_region_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + return __create_region_show(to_cxl_root_decoder(dev), buf); > +} > + > +static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd, > + enum cxl_decoder_mode mode, int id) > +{ > + int rc; > + > + rc = memregion_alloc(GFP_KERNEL); > + if (rc < 0) > + return ERR_PTR(rc); > + > + if (atomic_cmpxchg(&cxlrd->region_id, id, rc) != id) { > + memregion_free(rc); > + return ERR_PTR(-EBUSY); > + } > + > + return devm_cxl_add_region(cxlrd, id, mode, CXL_DECODER_EXPANDER); > } > > static ssize_t create_pmem_region_store(struct device *dev, > @@ -1727,29 +1762,37 @@ static ssize_t create_pmem_region_store(struct device *dev, > { > struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev); > struct cxl_region *cxlr; > - int id, rc; > + int rc, id; > > rc = sscanf(buf, "region%d\n", &id); > if (rc != 1) > return -EINVAL; > > - rc = memregion_alloc(GFP_KERNEL); > - if (rc < 0) > - return rc; > + cxlr = __create_region(cxlrd, CXL_DECODER_PMEM, id); > + if (IS_ERR(cxlr)) > + return PTR_ERR(cxlr); > + return len; > +} > +DEVICE_ATTR_RW(create_pmem_region); > > - if (atomic_cmpxchg(&cxlrd->region_id, id, rc) != id) { > - memregion_free(rc); > - return -EBUSY; > - } > +static ssize_t create_ram_region_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t len) > +{ > + struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev); > + struct cxl_region *cxlr; > + int rc, id; > > - cxlr = devm_cxl_add_region(cxlrd, id, CXL_DECODER_PMEM, > - CXL_DECODER_EXPANDER); > + rc = sscanf(buf, "region%d\n", &id); > + if (rc != 1) > + return -EINVAL; > + > + cxlr = __create_region(cxlrd, CXL_DECODER_RAM, id); > if (IS_ERR(cxlr)) > return PTR_ERR(cxlr); > - > return len; > } > -DEVICE_ATTR_RW(create_pmem_region); > +DEVICE_ATTR_RW(create_ram_region); > > static ssize_t region_show(struct device *dev, struct device_attribute *attr, > char *buf) > >
diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl index 4c4e1cbb1169..3acf2f17a73f 100644 --- a/Documentation/ABI/testing/sysfs-bus-cxl +++ b/Documentation/ABI/testing/sysfs-bus-cxl @@ -285,20 +285,20 @@ Description: interleave_granularity). -What: /sys/bus/cxl/devices/decoderX.Y/create_pmem_region -Date: May, 2022 -KernelVersion: v6.0 +What: /sys/bus/cxl/devices/decoderX.Y/create_{pmem,ram}_region +Date: May, 2022, January, 2023 +KernelVersion: v6.0 (pmem), v6.3 (ram) Contact: linux-cxl@vger.kernel.org Description: (RW) Write a string in the form 'regionZ' to start the process - of defining a new persistent memory region (interleave-set) - within the decode range bounded by root decoder 'decoderX.Y'. - The value written must match the current value returned from - reading this attribute. An atomic compare exchange operation is - done on write to assign the requested id to a region and - allocate the region-id for the next creation attempt. EBUSY is - returned if the region name written does not match the current - cached value. + of defining a new persistent, or volatile memory region + (interleave-set) within the decode range bounded by root decoder + 'decoderX.Y'. The value written must match the current value + returned from reading this attribute. An atomic compare exchange + operation is done on write to assign the requested id to a + region and allocate the region-id for the next creation attempt. + EBUSY is returned if the region name written does not match the + current cached value. What: /sys/bus/cxl/devices/decoderX.Y/delete_region diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h index 8c04672dca56..5eb873da5a30 100644 --- a/drivers/cxl/core/core.h +++ b/drivers/cxl/core/core.h @@ -11,6 +11,7 @@ extern struct attribute_group cxl_base_attribute_group; #ifdef CONFIG_CXL_REGION extern struct device_attribute dev_attr_create_pmem_region; +extern struct device_attribute dev_attr_create_ram_region; extern struct device_attribute dev_attr_delete_region; extern struct device_attribute dev_attr_region; extern const struct device_type cxl_pmem_region_type; diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c index 8566451cb22f..47e450c3a5a9 100644 --- a/drivers/cxl/core/port.c +++ b/drivers/cxl/core/port.c @@ -294,6 +294,7 @@ static struct attribute *cxl_decoder_root_attrs[] = { &dev_attr_cap_type3.attr, &dev_attr_target_list.attr, SET_CXL_REGION_ATTR(create_pmem_region) + SET_CXL_REGION_ATTR(create_ram_region) SET_CXL_REGION_ATTR(delete_region) NULL, }; @@ -305,6 +306,13 @@ static bool can_create_pmem(struct cxl_root_decoder *cxlrd) return (cxlrd->cxlsd.cxld.flags & flags) == flags; } +static bool can_create_ram(struct cxl_root_decoder *cxlrd) +{ + unsigned long flags = CXL_DECODER_F_TYPE3 | CXL_DECODER_F_RAM; + + return (cxlrd->cxlsd.cxld.flags & flags) == flags; +} + static umode_t cxl_root_decoder_visible(struct kobject *kobj, struct attribute *a, int n) { struct device *dev = kobj_to_dev(kobj); @@ -313,7 +321,11 @@ static umode_t cxl_root_decoder_visible(struct kobject *kobj, struct attribute * if (a == CXL_REGION_ATTR(create_pmem_region) && !can_create_pmem(cxlrd)) return 0; - if (a == CXL_REGION_ATTR(delete_region) && !can_create_pmem(cxlrd)) + if (a == CXL_REGION_ATTR(create_ram_region) && !can_create_ram(cxlrd)) + return 0; + + if (a == CXL_REGION_ATTR(delete_region) && + !(can_create_pmem(cxlrd) || can_create_ram(cxlrd))) return 0; return a->mode; diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index 53d6dbe4de6d..8dea49c021b8 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -1685,6 +1685,15 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd, struct device *dev; int rc; + switch (mode) { + case CXL_DECODER_RAM: + case CXL_DECODER_PMEM: + break; + default: + dev_err(&cxlrd->cxlsd.cxld.dev, "unsupported mode %d\n", mode); + return ERR_PTR(-EINVAL); + } + cxlr = cxl_region_alloc(cxlrd, id); if (IS_ERR(cxlr)) return cxlr; @@ -1713,12 +1722,38 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd, return ERR_PTR(rc); } +static ssize_t __create_region_show(struct cxl_root_decoder *cxlrd, char *buf) +{ + return sysfs_emit(buf, "region%u\n", atomic_read(&cxlrd->region_id)); +} + static ssize_t create_pmem_region_show(struct device *dev, struct device_attribute *attr, char *buf) { - struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev); + return __create_region_show(to_cxl_root_decoder(dev), buf); +} - return sysfs_emit(buf, "region%u\n", atomic_read(&cxlrd->region_id)); +static ssize_t create_ram_region_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + return __create_region_show(to_cxl_root_decoder(dev), buf); +} + +static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd, + enum cxl_decoder_mode mode, int id) +{ + int rc; + + rc = memregion_alloc(GFP_KERNEL); + if (rc < 0) + return ERR_PTR(rc); + + if (atomic_cmpxchg(&cxlrd->region_id, id, rc) != id) { + memregion_free(rc); + return ERR_PTR(-EBUSY); + } + + return devm_cxl_add_region(cxlrd, id, mode, CXL_DECODER_EXPANDER); } static ssize_t create_pmem_region_store(struct device *dev, @@ -1727,29 +1762,37 @@ static ssize_t create_pmem_region_store(struct device *dev, { struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev); struct cxl_region *cxlr; - int id, rc; + int rc, id; rc = sscanf(buf, "region%d\n", &id); if (rc != 1) return -EINVAL; - rc = memregion_alloc(GFP_KERNEL); - if (rc < 0) - return rc; + cxlr = __create_region(cxlrd, CXL_DECODER_PMEM, id); + if (IS_ERR(cxlr)) + return PTR_ERR(cxlr); + return len; +} +DEVICE_ATTR_RW(create_pmem_region); - if (atomic_cmpxchg(&cxlrd->region_id, id, rc) != id) { - memregion_free(rc); - return -EBUSY; - } +static ssize_t create_ram_region_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t len) +{ + struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev); + struct cxl_region *cxlr; + int rc, id; - cxlr = devm_cxl_add_region(cxlrd, id, CXL_DECODER_PMEM, - CXL_DECODER_EXPANDER); + rc = sscanf(buf, "region%d\n", &id); + if (rc != 1) + return -EINVAL; + + cxlr = __create_region(cxlrd, CXL_DECODER_RAM, id); if (IS_ERR(cxlr)) return PTR_ERR(cxlr); - return len; } -DEVICE_ATTR_RW(create_pmem_region); +DEVICE_ATTR_RW(create_ram_region); static ssize_t region_show(struct device *dev, struct device_attribute *attr, char *buf)
Expand the region creation infrastructure to enable 'ram' (volatile-memory) regions. The internals of create_pmem_region_store() and create_pmem_region_show() are factored out into helpers __create_region() and __create_region_show() for the 'ram' case to reuse. Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- Documentation/ABI/testing/sysfs-bus-cxl | 22 +++++----- drivers/cxl/core/core.h | 1 drivers/cxl/core/port.c | 14 ++++++ drivers/cxl/core/region.c | 71 +++++++++++++++++++++++++------ 4 files changed, 82 insertions(+), 26 deletions(-)