Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 10 May 2021 16:21:21 +0100
From:   Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To:     Dan Williams <dan.j.williams@intel.com>
CC:     <linux-cxl@vger.kernel.org>, <linux-pci@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-acpi@vger.kernel.org>
Subject: Re: [PATCH 7/8] cxl/port: Introduce cxl_port objects
Message-ID: <20210510162121.000042be@Huawei.com>
In-Reply-To: <162042791852.1202325.8197739881935753009.stgit@dwillia2-desk3.amr.corp.intel.com>
References: <162042787450.1202325.5718541949681409566.stgit@dwillia2-desk3.amr.corp.intel.com>
        <162042791852.1202325.8197739881935753009.stgit@dwillia2-desk3.amr.corp.intel.com>
Organization: Huawei Technologies Research and Development (UK) Ltd.
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Fri, 7 May 2021 15:51:58 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Once the cxl_root is established then other ports in the hierarchy can
> be attached. The cxl_port object, unlike cxl_root that is associated
> with host bridges, is associated with PCIe Root Ports or PCIe Switch
> Ports. Add cxl_port instances for all PCIe Root Ports in an ACPI0016
> host bridge. The cxl_port instances for PCIe Switch Ports are not
> included here as those are to be modeled as another service device
> registered on the pcie_port_bus_type.
> 
> A sample sysfs topology for a single-host-bridge with
> single-PCIe/CXL-port follows:
> 
> /sys/bus/cxl/devices/root0
> ├── address_space0
> │   ├── devtype
> │   ├── end
> │   ├── start
> │   ├── supports_ram
> │   ├── supports_type2
> │   ├── supports_type3
> │   └── uevent
> ├── address_space1
> │   ├── devtype
> │   ├── end
> │   ├── start
> │   ├── supports_pmem
> │   ├── supports_type2
> │   ├── supports_type3
> │   └── uevent
> ├── devtype
> ├── port1
> │   ├── devtype
> │   ├── host -> ../../../../LNXSYSTM:00/LNXSYBUS:00/ACPI0016:00
> │   ├── port2
> │   │   ├── devtype
> │   │   ├── host -> ../../../../../pci0000:34/0000:34:00.0
> │   │   ├── subsystem -> ../../../../../../bus/cxl
> │   │   ├── target_id
> │   │   └── uevent
> │   ├── subsystem -> ../../../../../bus/cxl
> │   ├── target_id
> │   └── uevent
> ├── subsystem -> ../../../../bus/cxl
> ├── target_id
> └── uevent
> 
> In this listing the system-wide-singleton root0 has 2 address spaces, 1
> PMEM and 1 RAM. Those address spaces are accessed through port1 which
> represents the upstream port of an ACPI0016 host-bridge. A
> multi-host-bridge system would have other ports as peers to port1 to
> additionally decode root level address spaces. Port2 in this diagram
> represents the single downstream port of the host-bridge. Were it to be
> a multi-ported-host-bridge there would be peers / siblings of port2 with
> port1 as their common ancestor.

I guess it would be a pain to emulate a system that actually had
multiple ports at the last level. Pity as would have made your
explanation here a little easier to follow.

> 
> The rationale for this port hierarchy is to be able to walk the HDM
> decoder register sets that each port implements. Additionally it
> provides a representation of host-bridge interleave which will be
> necessary for follow-on work that adds CXL region devices.
> 
> The details in the /sys/bus/cxl hierarchy that are not suitable to be
> represented in the /sys/bus/pci hierarchy are:
> - memory address spaces that are interleaved across host bridges
> - common sub-device functionality represented by CXL component + device
>   registers (enumerated via DVSEC or platform firmware (ACPI CEDT)).

I'm sold :)

> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/acpi.c |   99 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/core.c |  121 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/cxl.h  |    5 ++
>  3 files changed, 224 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index d54c2d5de730..bc2a35ae880b 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -5,18 +5,117 @@
>  #include <linux/device.h>
>  #include <linux/kernel.h>
>  #include <linux/acpi.h>
> +#include <linux/pci.h>
>  #include "cxl.h"
>  
> +static int match_ACPI0016(struct device *dev, const void *host)
> +{
> +	struct acpi_device *adev = to_acpi_device(dev);
> +	const char *hid = acpi_device_hid(adev);
> +
> +	return strcmp(hid, "ACPI0016") == 0;
> +}
> +
> +struct cxl_walk_context {
> +	struct device *dev;
> +	struct pci_bus *root;
> +	struct cxl_port *port;
> +	int error;
> +	int count;
> +};
> +
> +static int match_add_root_ports(struct pci_dev *pdev, void *data)
> +{
> +	struct cxl_walk_context *ctx = data;
> +	struct pci_bus *root_bus = ctx->root;
> +	struct cxl_port *port = ctx->port;
> +	int type = pci_pcie_type(pdev);
> +	struct device *dev = ctx->dev;
> +	resource_size_t cxl_regs_phys;
> +	int target_id = ctx->count;
> +
> +	if (pdev->bus != root_bus)
> +		return 0;
> +	if (!pci_is_pcie(pdev))
> +		return 0;
> +	if (type != PCI_EXP_TYPE_ROOT_PORT)
> +		return 0;
> +
> +	ctx->count++;
> +
> +	/* TODO walk DVSEC to find component register base */
> +	cxl_regs_phys = -1;
> +
> +	port = devm_cxl_add_port(dev, port, &pdev->dev, target_id,
> +				 cxl_regs_phys);
> +	if (IS_ERR(port)) {
> +		ctx->error = PTR_ERR(port);
> +		return ctx->error;
> +	}
> +
> +	dev_dbg(dev, "%s: register: %s\n", dev_name(&pdev->dev),
> +		dev_name(&port->dev));
> +
> +	return 0;
> +}
> +
> +/*
> + * A host bridge may contain one or more root ports.  Register each port
> + * as a child of the cxl_root.
> + */
> +static int cxl_acpi_register_ports(struct device *dev, struct acpi_device *root,
> +				   struct cxl_port *port, int idx)
> +{
> +	struct acpi_pci_root *pci_root = acpi_pci_find_root(root->handle);
> +	struct cxl_walk_context ctx;
> +
> +	if (!pci_root)
> +		return -ENXIO;
> +
> +	/* TODO: fold in CEDT.CHBS retrieval */
> +	port = devm_cxl_add_port(dev, port, &root->dev, idx, ~0ULL);
> +	if (IS_ERR(port))
> +		return PTR_ERR(port);
> +	dev_dbg(dev, "%s: register: %s\n", dev_name(&root->dev),
> +		dev_name(&port->dev));
> +
> +	ctx = (struct cxl_walk_context) {
> +		.dev = dev,
> +		.root = pci_root->bus,
> +		.port = port,
> +	};
> +	pci_walk_bus(pci_root->bus, match_add_root_ports, &ctx);
> +
> +	if (ctx.count == 0)
> +		return -ENODEV;
> +	return ctx.error;
> +}
> +
>  static int cxl_acpi_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> +	struct acpi_device *adev = ACPI_COMPANION(dev);
> +	struct device *bridge = NULL;
>  	struct cxl_root *cxl_root;
> +	int rc, i = 0;
>  
>  	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));
>  
> +	while (true) {
> +		bridge = bus_find_device(adev->dev.bus, bridge, dev,
> +					 match_ACPI0016);
> +		if (!bridge)
> +			break;
> +
> +		rc = cxl_acpi_register_ports(dev, to_acpi_device(bridge),
> +					     &cxl_root->port, i++);
> +		if (rc)
> +			return rc;
> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> index 347824a62a66..cc9af9033292 100644
> --- a/drivers/cxl/core.c
> +++ b/drivers/cxl/core.c
> @@ -148,6 +148,15 @@ static void cxl_root_release(struct device *dev)
>  	kfree(cxl_root);
>  }
>  
> +static void cxl_port_release(struct device *dev)
> +{
> +	struct cxl_port *port = to_cxl_port(dev);
> +
> +	ida_free(&cxl_port_ida, port->id);
> +	put_device(port->port_host);
> +	kfree(port);
> +}
> +
>  static ssize_t target_id_show(struct device *dev, struct device_attribute *attr,
>  			      char *buf)
>  {
> @@ -178,6 +187,12 @@ static const struct device_type cxl_root_type = {
>  	.groups = cxl_port_attribute_groups,
>  };
>  
> +static const struct device_type cxl_port_type = {
> +	.name = "cxl_port",
> +	.release = cxl_port_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,
> @@ -188,7 +203,9 @@ struct cxl_root *to_cxl_root(struct device *dev)
>  
>  struct cxl_port *to_cxl_port(struct device *dev)
>  {
> -	if (dev_WARN_ONCE(dev, dev->type != &cxl_root_type,
> +	if (dev_WARN_ONCE(dev,
> +			  dev->type != &cxl_root_type &&
> +			  dev->type != &cxl_port_type,
>  			  "not a cxl_port device\n"))
>  		return NULL;
>  	return container_of(dev, struct cxl_port, dev);
> @@ -367,6 +384,108 @@ struct cxl_root *devm_cxl_add_root(struct device *host,
>  }
>  EXPORT_SYMBOL_GPL(devm_cxl_add_root);
>  
> +static void cxl_unlink_port(void *_port)
> +{
> +	struct cxl_port *port = _port;
> +
> +	sysfs_remove_link(&port->dev.kobj, "host");
> +}
> +
> +static int devm_cxl_link_port(struct device *dev, struct cxl_port *port)
> +{
> +	int rc;
> +
> +	rc = sysfs_create_link(&port->dev.kobj, &port->port_host->kobj, "host");
> +	if (rc)
> +		return rc;
> +	return devm_add_action_or_reset(dev, cxl_unlink_port, port);
> +}
> +
> +static struct cxl_port *cxl_port_alloc(struct cxl_port *parent_port,
> +				       struct device *port_dev, int target_id,
> +				       resource_size_t component_regs_phys)
> +{
> +	struct cxl_port *port;
> +	struct device *dev;
> +	int rc;
> +
> +	if (!port_dev)
> +		return ERR_PTR(-EINVAL);
> +
> +	port = kzalloc(sizeof(*port), GFP_KERNEL);
> +	if (!port)
> +		return ERR_PTR(-ENOMEM);
> +
> +	rc = ida_alloc(&cxl_port_ida, GFP_KERNEL);
> +	if (rc < 0)
> +		goto err;
> +
> +	port->id = rc;
> +	port->target_id = target_id;
> +	port->port_host = get_device(port_dev);
> +	port->component_regs_phys = component_regs_phys;
> +
> +	dev = &port->dev;
> +	device_initialize(dev);
> +	device_set_pm_not_required(dev);
> +	dev->parent = &parent_port->dev;
> +	dev->bus = &cxl_bus_type;
> +	dev->type = &cxl_port_type;
> +
> +	return port;
> +
> +err:
> +	kfree(port);
> +	return ERR_PTR(rc);
> +}
> +
> +/**
> + * devm_cxl_add_port() - add a cxl_port to the topology
> + * @host: devm context / discovery agent
> + * @parent_port: immediate ancestor towards cxl_root
> + * @port_host: PCI or platform-firmware device hosting this port
> + * @target_id: ordinal id relative to other siblings under @parent_port
> + * @component_regs_phys: CXL component register base address
> + */
> +struct cxl_port *devm_cxl_add_port(struct device *host,
> +				   struct cxl_port *parent_port,
> +				   struct device *port_host, int target_id,
> +				   resource_size_t component_regs_phys)
> +{
> +	struct cxl_port *port;
> +	struct device *dev;
> +	int rc;
> +
> +	port = cxl_port_alloc(parent_port, port_host, target_id,
> +			      component_regs_phys);
> +	if (IS_ERR(port))
> +		return port;
> +
> +	dev = &port->dev;
> +	rc = dev_set_name(dev, "port%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);
> +
> +	rc = devm_cxl_link_port(host, port);
> +	if (rc)
> +		return ERR_PTR(rc);
> +
> +	return port;
> +
> +err:
> +	put_device(dev);
> +	return ERR_PTR(rc);
> +}
> +EXPORT_SYMBOL_GPL(devm_cxl_add_port);
> +
>  /**
>   * 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 5cd1173151e5..71a991bdacb7 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -134,5 +134,10 @@ 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);
> +struct cxl_port *devm_cxl_add_port(struct device *host,
> +				   struct cxl_port *parent_port,
> +				   struct device *port_host, int target_id,
> +				   resource_size_t component_regs_phys);
> +
>  extern struct bus_type cxl_bus_type;
>  #endif /* __CXL_H__ */
>