| 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: <20210406183232.00003189@Huawei.com>
Date: Tue, 6 Apr 2021 18:32:32 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Dan Williams <dan.j.williams@...el.com>
CC: <linux-cxl@...r.kernel.org>, <linux-pci@...r.kernel.org>,
<linux-acpi@...r.kernel.org>, <ira.weiny@...el.com>,
<vishal.l.verma@...el.com>, <alison.schofield@...el.com>,
<ben.widawsky@...el.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 5/8] cxl/acpi: Introduce ACPI0017 driver and cxl_root
On Thu, 1 Apr 2021 07:31:09 -0700
Dan Williams <dan.j.williams@...el.com> wrote:
> While CXL builds upon the PCI software model for dynamic enumeration and
> control, a static platform component is required to bootstrap the CXL
> memory layout. In addition to identifying the host bridges ACPI is
> responsible for enumerating the CXL memory space that can be addressed
> by decoders. This is similar to the requirement for ACPI to publish
> resources reported by _CRS for PCI host bridges.
>
> Introduce the cxl_root object as an abstract "port" into the CXL.mem
> address space described by HDM decoders identified by the ACPI
> CEDT.CHBS.
>
> For now just establish the initial boilerplate and sysfs attributes, to
> be followed by enumeration of the ports within the host bridge.
>
> Signed-off-by: Dan Williams <dan.j.williams@...el.com>
A few minor comments inline.
> ---
> drivers/cxl/Kconfig | 14 ++
> drivers/cxl/Makefile | 2
> drivers/cxl/acpi.c | 39 ++++++
> drivers/cxl/core.c | 349 ++++++++++++++++++++++++++++++++++++++++++++++++++
> drivers/cxl/cxl.h | 64 +++++++++
> 5 files changed, 468 insertions(+)
> create mode 100644 drivers/cxl/acpi.c
>
> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> index 97dc4d751651..fb282af84afd 100644
> --- a/drivers/cxl/Kconfig
> +++ b/drivers/cxl/Kconfig
> @@ -50,4 +50,18 @@ config CXL_MEM_RAW_COMMANDS
> potential impact to memory currently in use by the kernel.
>
> If developing CXL hardware or the driver say Y, otherwise say N.
> +
> +config CXL_ACPI
> + tristate "CXL ACPI: Platform Support"
> + depends on ACPI
> + help
> + Enable support for host managed device memory (HDM) resources
> + published by a platform's ACPI CXL memory layout description.
> + See Chapter 9.14.1 CXL Early Discovery Table (CEDT) in the CXL
> + 2.0 specification. The CXL core consumes these resource to
> + publish port and address_space objects used to map regions
> + that represent System RAM, or Persistent Memory regions to be
> + managed by LIBNVDIMM.
> +
> + If unsure say 'm'.
> endif
> diff --git a/drivers/cxl/Makefile b/drivers/cxl/Makefile
> index 3808e39dd31f..f429ca6b59d9 100644
> --- a/drivers/cxl/Makefile
> +++ b/drivers/cxl/Makefile
> @@ -1,7 +1,9 @@
> # SPDX-License-Identifier: GPL-2.0
> obj-$(CONFIG_CXL_BUS) += cxl_core.o
> obj-$(CONFIG_CXL_MEM) += cxl_mem.o
> +obj-$(CONFIG_CXL_ACPI) += cxl_acpi.o
>
> ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE=CXL
> cxl_core-y := core.o
> cxl_mem-y := mem.o
> +cxl_acpi-y := acpi.o
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> new file mode 100644
> index 000000000000..d54c2d5de730
> --- /dev/null
> +++ b/drivers/cxl/acpi.c
> @@ -0,0 +1,39 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2021 Intel Corporation. All rights reserved. */
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/acpi.h>
swap acpi.h that for mod_devicetable.h unless this is going to
need acpi.h later for something else.
> +#include "cxl.h"
> +
> +static int cxl_acpi_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct cxl_root *cxl_root;
> +
> + cxl_root = devm_cxl_add_root(dev, NULL, 0);
> + if (IS_ERR(cxl_root))
> + return PTR_ERR(cxl_root);
> + dev_dbg(dev, "register: %s\n", dev_name(&cxl_root->port.dev));
> +
> + return 0;
> +}
> +
> +static const struct acpi_device_id cxl_acpi_ids[] = {
> + { "ACPI0017", 0 },
> + { "", 0 },
> +};
> +MODULE_DEVICE_TABLE(acpi, cxl_acpi_ids);
> +
> +static struct platform_driver cxl_acpi_driver = {
> + .probe = cxl_acpi_probe,
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .acpi_match_table = cxl_acpi_ids,
> + },
> +};
> +
> +module_platform_driver(cxl_acpi_driver);
> +MODULE_LICENSE("GPL v2");
> +MODULE_IMPORT_NS(CXL);
> diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> index 2ab467ef9909..46c3b2588d2f 100644
> --- a/drivers/cxl/core.c
> +++ b/drivers/cxl/core.c
> @@ -2,6 +2,8 @@
> /* Copyright(c) 2020-2021 Intel Corporation. All rights reserved. */
> #include <linux/device.h>
> #include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/idr.h>
> #include "cxl.h"
>
> /**
> @@ -11,6 +13,353 @@
> * point for cross-device interleave coordination through cxl ports.
> */
>
> +static DEFINE_IDA(cxl_port_ida);
> +
> +static ssize_t devtype_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + return sysfs_emit(buf, "%s\n", dev->type->name);
> +}
> +static DEVICE_ATTR_RO(devtype);
> +
> +static struct attribute *cxl_base_attributes[] = {
> + &dev_attr_devtype.attr,
> + NULL,
> +};
> +
> +static struct attribute_group cxl_base_attribute_group = {
> + .attrs = cxl_base_attributes,
> +};
> +
> +static struct cxl_address_space *dev_to_address_space(struct device *dev)
> +{
> + struct cxl_address_space_dev *cxl_asd = to_cxl_address_space(dev);
> +
> + return cxl_asd->address_space;
> +}
> +
> +static ssize_t start_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct cxl_address_space *space = dev_to_address_space(dev);
> +
> + return sysfs_emit(buf, "%#llx\n", space->range.start);
> +}
> +static DEVICE_ATTR_RO(start);
> +
> +static ssize_t end_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct cxl_address_space *space = dev_to_address_space(dev);
> +
> + return sysfs_emit(buf, "%#llx\n", space->range.end);
> +}
> +static DEVICE_ATTR_RO(end);
> +
> +#define CXL_ATTR_SUPPORTS(name, flag) \
> +static ssize_t supports_##name##_show( \
> + struct device *dev, struct device_attribute *attr, char *buf) \
> +{ \
> + struct cxl_address_space *space = dev_to_address_space(dev); \
> + \
> + return sysfs_emit(buf, "%s\n", \
> + (space->flags & (flag)) ? "1" : "0"); \
> +} \
> +static DEVICE_ATTR_RO(supports_##name)
> +
> +CXL_ATTR_SUPPORTS(pmem, CXL_ADDRSPACE_PMEM);
> +CXL_ATTR_SUPPORTS(ram, CXL_ADDRSPACE_RAM);
> +CXL_ATTR_SUPPORTS(type2, CXL_ADDRSPACE_TYPE2);
> +CXL_ATTR_SUPPORTS(type3, CXL_ADDRSPACE_TYPE3);
> +
> +static struct attribute *cxl_address_space_attributes[] = {
> + &dev_attr_start.attr,
> + &dev_attr_end.attr,
> + &dev_attr_supports_pmem.attr,
> + &dev_attr_supports_ram.attr,
> + &dev_attr_supports_type2.attr,
> + &dev_attr_supports_type3.attr,
> + NULL,
> +};
> +
> +static umode_t cxl_address_space_visible(struct kobject *kobj,
> + struct attribute *a, int n)
> +{
> + struct device *dev = container_of(kobj, struct device, kobj);
> + struct cxl_address_space *space = dev_to_address_space(dev);
> +
> + if (a == &dev_attr_supports_pmem.attr &&
> + !(space->flags & CXL_ADDRSPACE_PMEM))
> + return 0;
> +
> + if (a == &dev_attr_supports_ram.attr &&
> + !(space->flags & CXL_ADDRSPACE_RAM))
> + return 0;
> +
> + if (a == &dev_attr_supports_type2.attr &&
> + !(space->flags & CXL_ADDRSPACE_TYPE2))
> + return 0;
> +
> + if (a == &dev_attr_supports_type3.attr &&
> + !(space->flags & CXL_ADDRSPACE_TYPE3))
> + return 0;
> +
> + return a->mode;
> +}
> +
> +static struct attribute_group cxl_address_space_attribute_group = {
> + .attrs = cxl_address_space_attributes,
> + .is_visible = cxl_address_space_visible,
> +};
> +
> +static const struct attribute_group *cxl_address_space_attribute_groups[] = {
> + &cxl_address_space_attribute_group,
> + &cxl_base_attribute_group,
> + NULL,
> +};
> +
> +static void cxl_address_space_release(struct device *dev)
> +{
> + struct cxl_address_space_dev *cxl_asd = to_cxl_address_space(dev);
> +
> + remove_resource(&cxl_asd->res);
> + kfree(cxl_asd);
> +}
> +
> +static const struct device_type cxl_address_space_type = {
> + .name = "cxl_address_space",
> + .release = cxl_address_space_release,
> + .groups = cxl_address_space_attribute_groups,
> +};
> +
> +struct cxl_address_space_dev *to_cxl_address_space(struct device *dev)
> +{
> + if (dev_WARN_ONCE(dev, dev->type != &cxl_address_space_type,
> + "not a cxl_address_space device\n"))
> + return NULL;
> + return container_of(dev, struct cxl_address_space_dev, dev);
> +}
> +
> +static void cxl_root_release(struct device *dev)
> +{
> + struct cxl_root *cxl_root = to_cxl_root(dev);
> +
> + ida_free(&cxl_port_ida, cxl_root->port.id);
> + kfree(cxl_root);
> +}
> +
> +static ssize_t target_id_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct cxl_port *cxl_port = to_cxl_port(dev);
> +
> + return sysfs_emit(buf, "%d\n", cxl_port->target_id);
> +}
> +static DEVICE_ATTR_RO(target_id);
> +
> +static struct attribute *cxl_port_attributes[] = {
> + &dev_attr_target_id.attr,
> + NULL,
> +};
> +
> +static struct attribute_group cxl_port_attribute_group = {
> + .attrs = cxl_port_attributes,
> +};
> +
> +static const struct attribute_group *cxl_port_attribute_groups[] = {
> + &cxl_port_attribute_group,
> + &cxl_base_attribute_group,
> + NULL,
> +};
> +
> +static const struct device_type cxl_root_type = {
> + .name = "cxl_root",
> + .release = cxl_root_release,
> + .groups = cxl_port_attribute_groups,
> +};
> +
> +struct cxl_root *to_cxl_root(struct device *dev)
> +{
> + if (dev_WARN_ONCE(dev, dev->type != &cxl_root_type,
> + "not a cxl_root device\n"))
> + return NULL;
> + return container_of(dev, struct cxl_root, port.dev);
> +}
> +
> +struct cxl_port *to_cxl_port(struct device *dev)
> +{
> + if (dev_WARN_ONCE(dev, dev->type != &cxl_root_type,
> + "not a cxl_port device\n"))
> + return NULL;
> + return container_of(dev, struct cxl_port, dev);
> +}
> +
> +static void unregister_dev(void *dev)
> +{
> + device_unregister(dev);
> +}
> +
> +static struct cxl_root *cxl_root_alloc(struct device *parent,
> + struct cxl_address_space *cxl_space,
> + int nr_spaces)
> +{
> + struct cxl_root *cxl_root;
> + struct cxl_port *port;
> + struct device *dev;
> + int rc;
> +
> + cxl_root = kzalloc(struct_size(cxl_root, address_space, nr_spaces),
> + GFP_KERNEL);
> + if (!cxl_root)
> + return ERR_PTR(-ENOMEM);
> +
> + memcpy(cxl_root->address_space, cxl_space,
> + flex_array_size(cxl_root, address_space, nr_spaces));
> + cxl_root->nr_spaces = nr_spaces;
> +
> + rc = ida_alloc(&cxl_port_ida, GFP_KERNEL);
> + if (rc < 0)
> + goto err;
> + port = &cxl_root->port;
> + port->id = rc;
> +
> + /*
> + * Root does not have a cxl_port as its parent and it does not
> + * have any corresponding component registers it is only a
have any corresponding component registers; it is only a
.. or you could use two sentences
> + * logical anchor to the first level of actual ports that decode
> + * the root address spaces.
> + */
> + port->port_host = parent;
> + port->target_id = -1;
> + port->component_regs_phys = -1;
> +
> + dev = &port->dev;
> + device_initialize(dev);
> + device_set_pm_not_required(dev);
> + dev->parent = parent;
> + dev->bus = &cxl_bus_type;
> + dev->type = &cxl_root_type;
> +
> + return cxl_root;
> +
> +err:
> + kfree(cxl_root);
> + return ERR_PTR(rc);
> +}
> +
> +static struct cxl_address_space_dev *
> +cxl_address_space_dev_alloc(struct device *parent,
> + struct cxl_address_space *space)
> +{
> + struct cxl_address_space_dev *cxl_asd;
> + struct resource *res;
> + struct device *dev;
> + int rc;
> +
> + cxl_asd = kzalloc(sizeof(*cxl_asd), GFP_KERNEL);
> + if (!cxl_asd)
> + return ERR_PTR(-ENOMEM);
> +
> + res = &cxl_asd->res;
> + res->name = "CXL Address Space";
> + res->start = space->range.start;
> + res->end = space->range.end;
> + res->flags = IORESOURCE_MEM;
> +
> + rc = insert_resource(&iomem_resource, res);
> + if (rc)
> + goto err;
> +
> + cxl_asd->address_space = space;
> + dev = &cxl_asd->dev;
> + device_initialize(dev);
> + device_set_pm_not_required(dev);
> + dev->parent = parent;
> + dev->type = &cxl_address_space_type;
> +
> + return cxl_asd;
> +
> +err:
> + kfree(cxl_asd);
> + return ERR_PTR(rc);
> +}
> +
> +static int cxl_address_space_dev_add(struct device *host,
> + struct cxl_address_space_dev *cxl_asd,
> + int id)
> +{
> + struct device *dev = &cxl_asd->dev;
> + int rc;
> +
> + rc = dev_set_name(dev, "address_space%d", id);
> + if (rc)
> + goto err;
> +
> + rc = device_add(dev);
> + if (rc)
> + goto err;
> +
> + dev_dbg(host, "%s: register %s\n", dev_name(dev->parent),
> + dev_name(dev));
> +
> + return devm_add_action_or_reset(host, unregister_dev, dev);
> +
> +err:
> + put_device(dev);
This is unusual. The error handling is undoing something this function
wasn't responsible for. See below for suggested resolution.
> + return rc;
> +}
> +
> +struct cxl_root *devm_cxl_add_root(struct device *host,
> + struct cxl_address_space *cxl_space,
> + int nr_spaces)
> +{
> + struct cxl_root *cxl_root;
> + struct cxl_port *port;
> + struct device *dev;
> + int i, rc;
> +
> + cxl_root = cxl_root_alloc(host, cxl_space, nr_spaces);
> + if (IS_ERR(cxl_root))
> + return cxl_root;
> +
> + port = &cxl_root->port;
> + dev = &port->dev;
> + rc = dev_set_name(dev, "root%d", port->id);
> + if (rc)
> + goto err;
> +
> + rc = device_add(dev);
> + if (rc)
> + goto err;
> +
> + rc = devm_add_action_or_reset(host, unregister_dev, dev);
> + if (rc)
> + return ERR_PTR(rc);
> +
> + for (i = 0; i < nr_spaces; i++) {
> + struct cxl_address_space *space = &cxl_root->address_space[i];
> + struct cxl_address_space_dev *cxl_asd;
> +
> + if (!range_len(&space->range))
> + continue;
> +
> + cxl_asd = cxl_address_space_dev_alloc(dev, space);
> + if (IS_ERR(cxl_asd))
> + return ERR_CAST(cxl_asd);
> +
Nothing is done between the dev_alloc() and the dev_add()
and this is currently in the odd position of doing put_device() in the
error path of *dev_add() when it wasn't responsible for getting the
reference it is putting, dev_alloc() did that.
That suggests to me that we can clean up the oddity by just combining
cxl_address_space_dev_alloc() and cxl_adress_space_dev_add() into one
alloc_and_add() function (with a better name)
> + rc = cxl_address_space_dev_add(host, cxl_asd, i);
Lifetime management here seems overly complex. Why not use host for both
the alloc and add() devm calls? I guess there is a good reason
though so good to have a comment here saying what it is.
> + if (rc)
> + return ERR_PTR(rc);
> + }
> +
> + return cxl_root;
> +
> +err:
> + put_device(dev);
> + return ERR_PTR(rc);
> +}
> +EXPORT_SYMBOL_GPL(devm_cxl_add_root);
> +
> /*
> * cxl_setup_device_regs() - Detect CXL Device register blocks
> * @dev: Host device of the @base mapping
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index cbd29650c4e2..559f8343fee4 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -70,5 +70,69 @@ struct cxl_regs {
> void cxl_setup_device_regs(struct device *dev, void __iomem *base,
> struct cxl_device_regs *regs);
>
> +/*
> + * Address space properties derived from:
> + * CXL 2.0 8.2.5.12.7 CXL HDM Decoder 0 Control Register
> + */
> +#define CXL_ADDRSPACE_RAM BIT(0)
> +#define CXL_ADDRSPACE_PMEM BIT(1)
> +#define CXL_ADDRSPACE_TYPE2 BIT(2)
> +#define CXL_ADDRSPACE_TYPE3 BIT(3)
> +#define CXL_ADDRSPACE_MASK GENMASK(3, 0)
> +
> +struct cxl_address_space {
> + struct range range;
> + int interleave_size;
> + unsigned long flags;
> + unsigned long targets;
> +};
> +
> +struct cxl_address_space_dev {
> + struct device dev;
> + struct resource res;
> + struct cxl_address_space *address_space;
> +};
> +
> +/**
> + * struct cxl_port - object representing a root, upstream, or downstream port
> + * @dev: this port's device
> + * @port_host: PCI or platform device host of the CXL capability
> + * @id: id for port device-name
> + * @target_id: this port's HDM decoder id in the parent port
> + * @component_regs_phys: component register capability array base address
> + */
> +struct cxl_port {
> + struct device dev;
> + struct device *port_host;
> + int id;
> + int target_id;
> + resource_size_t component_regs_phys;
> +};
> +
> +/*
> + * struct cxl_root - platform object parent of CXL host bridges
> + *
> + * A cxl_root object represents a set of address spaces that are
> + * interleaved across a set of child host bridges, but never interleaved
> + * to another cxl_root object. It contains a cxl_port that is a special
> + * case in that it does not have a parent port and related HDMs, instead
> + * its decode is derived from the root (platform firmware defined)
> + * address space description. Not to be confused with CXL Root Ports
> + * that are the PCIE Root Ports within PCIE Host Bridges that are
> + * flagged by platform firmware (ACPI0016 on ACPI platforms) as having
> + * CXL capabilities.
> + */
> +struct cxl_root {
> + struct cxl_port port;
> + int nr_spaces;
> + struct cxl_address_space address_space[];
> +};
> +
> +struct cxl_root *to_cxl_root(struct device *dev);
> +struct cxl_port *to_cxl_port(struct device *dev);
> +struct cxl_address_space_dev *to_cxl_address_space(struct device *dev);
> +struct cxl_root *devm_cxl_add_root(struct device *parent,
> + struct cxl_address_space *cxl_space,
> + int nr_spaces);
> extern struct bus_type cxl_bus_type;
> #endif /* __CXL_H__ */
>
Powered by blists - more mailing lists