| 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: Fri, 7 Nov 2014 14:55:25 +0000
From: Liviu Dudau <Liviu.Dudau@....com>
To: Tomasz Nowicki <tomasz.nowicki@...aro.org>
Cc: Catalin Marinas <Catalin.Marinas@....com>,
Will Deacon <Will.Deacon@....com>,
"bhelgaas@...gle.com" <bhelgaas@...gle.com>,
"tglx@...utronix.de" <tglx@...utronix.de>,
"mingo@...hat.com" <mingo@...hat.com>,
"hpa@...or.com" <hpa@...or.com>,
"rjw@...ysocki.net" <rjw@...ysocki.net>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"x86@...nel.org" <x86@...nel.org>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
"linaro-acpi@...ts.linaro.org" <linaro-acpi@...ts.linaro.org>
Subject: Re: [RFC PATCH 4/4] arm64, acpi, pci: Provide arch-specific calls
for PCI host bridge dirver (PNP0A03).
On Fri, Nov 07, 2014 at 01:27:56PM +0000, Tomasz Nowicki wrote:
> Code inspired by arch/x86/pci/acpi.c and arch/ia64/pci/pci.c files.
> The main reasons why we need this:
> - parse and manage resources (BUS, IO and MEM)
> - create pci root bus and enumerate its children
> - provide PCI config space accessors (via MMCONFIG)
Hi Tomasz,
I have some comments to your code:
>
> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@...aro.org>
> ---
> arch/arm64/include/asm/pci.h | 8 +
> arch/arm64/kernel/pci.c | 401 +++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 391 insertions(+), 18 deletions(-)
>
> diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
> index fded096..ecd940f 100644
> --- a/arch/arm64/include/asm/pci.h
> +++ b/arch/arm64/include/asm/pci.h
> @@ -33,6 +33,14 @@ static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel)
> extern int isa_dma_bridge_buggy;
>
> #ifdef CONFIG_PCI
> +struct pci_controller {
> + struct acpi_device *companion;
> + int segment;
> + int node; /* nearest node with memory or NUMA_NO_NODE for global allocation */
> +};
> +
> +#define PCI_CONTROLLER(busdev) ((struct pci_controller *) busdev->sysdata)
I am trying to move away from the model where the architecture dictates the shape
of the pci_controller structure. Can I suggest that you put all this code and the
one under #ifdef CONFIG_ACPI in a separate file, say arch/arm64/kernel/pci-acpi.c ?
Or that you add an #ifdef CONFIG_ACPI around this structure definition?
I can't see anyone using this definition outside ACPI (due to struct acpi_device
dependency) and I would like to avoid it conflicting with other host bridge
drivers trying to define the same name structure.
> +
> static inline int pci_proc_domain(struct pci_bus *bus)
> {
> return 1;
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index 42fb195..cc34ded 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -1,6 +1,10 @@
> /*
> - * Code borrowed from powerpc/kernel/pci-common.c
> + * Code borrowed from powerpc/kernel/pci-common.c and arch/ia64/pci/pci.c
> *
> + * Copyright (c) 2002, 2005 Hewlett-Packard Development Company, L.P.
> + * David Mosberger-Tang <davidm@....hp.com>
> + * Bjorn Helgaas <bjorn.helgaas@...com>
> + * Copyright (C) 2004 Silicon Graphics, Inc.
> * Copyright (C) 2003 Anton Blanchard <anton@...ibm.com>, IBM
> * Copyright (C) 2014 ARM Ltd.
> *
> @@ -17,10 +21,16 @@
> #include <linux/mm.h>
> #include <linux/of_pci.h>
> #include <linux/of_platform.h>
> +#include <linux/of_address.h>
> #include <linux/slab.h>
> +#include <linux/pci.h>
> +#include <linux/acpi.h>
> +#include <linux/mmconfig.h>
>
> #include <asm/pci-bridge.h>
>
> +#define PREFIX "PCI: "
Where is this used?
> +
> /*
> * Called after each bus is probed, but before its children are examined
> */
> @@ -43,7 +53,8 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
> */
> int pcibios_add_device(struct pci_dev *dev)
> {
> - dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
> + if (acpi_disabled)
> + dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
>
> return 0;
> }
> @@ -54,45 +65,399 @@ static bool dt_domain_found = false;
>
> void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> {
> - int domain = of_get_pci_domain_nr(parent->of_node);
> + int domain = -1;
>
> - if (domain >= 0) {
> - dt_domain_found = true;
> - } else if (dt_domain_found == true) {
> - dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
> - parent->of_node->full_name);
> - return;
> + if (acpi_disabled) {
> + domain = of_get_pci_domain_nr(parent->of_node);
> +
> + if (domain >= 0) {
> + dt_domain_found = true;
> + } else if (dt_domain_found == true) {
> + dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
> + parent->of_node->full_name);
> + return;
> + } else {
> + domain = pci_get_new_domain_nr();
> + }
> } else {
> - domain = pci_get_new_domain_nr();
> + /*
> + * Take the domain nr from associated private data.
> + * Case where bus has parent is covered in pci_alloc_bus()
> + */
> + if (!parent)
> + domain = PCI_CONTROLLER(bus)->segment;
I would also like to wrap this into an ACPI specific function. Reason for it
is that when I get to split the pci_host_bridge creation out of pci_create_root_bus()
this will become a pci_controller property.
> }
>
> bus->domain_nr = domain;
> }
> #endif
>
> +static char __iomem *pci_dev_base(unsigned int seg, unsigned int bus,
> + unsigned int devfn)
> +{
> + struct pci_mmcfg_region *cfg = pci_mmconfig_lookup(seg, bus);
> +
> + if (cfg && cfg->virt)
> + return cfg->virt + (PCI_MMCFG_BUS_OFFSET(bus) | (devfn << 12));
> + return NULL;
> +}
> +
> /*
> * raw_pci_read/write - Platform-specific PCI config space access.
> - *
> - * Default empty implementation. Replace with an architecture-specific setup
> - * routine, if necessary.
> */
> int raw_pci_read(unsigned int domain, unsigned int bus,
> unsigned int devfn, int reg, int len, u32 *val)
> {
> - return -EINVAL;
> + char __iomem *addr;
> +
> + if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095))) {
> +err: *val = -1;
I believe the general usage is to have labels on their own line.
> + return -EINVAL;
> + }
> +
> + rcu_read_lock();
> + addr = pci_dev_base(domain, bus, devfn);
> + if (!addr) {
> + rcu_read_unlock();
> + goto err;
> + }
> +
> + switch (len) {
> + case 1:
> + *val = readb(addr + reg);
> + break;
> + case 2:
> + *val = readw(addr + reg);
> + break;
> + case 4:
> + *val = readl(addr + reg);
> + break;
> + }
> + rcu_read_unlock();
> +
> + return 0;
> }
>
> int raw_pci_write(unsigned int domain, unsigned int bus,
> unsigned int devfn, int reg, int len, u32 val)
> {
> - return -EINVAL;
> + char __iomem *addr;
> +
> + if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095)))
> + return -EINVAL;
> +
> + rcu_read_lock();
> + addr = pci_dev_base(domain, bus, devfn);
> + if (!addr) {
> + rcu_read_unlock();
> + return -EINVAL;
> + }
> +
> + switch (len) {
> + case 1:
> + writeb(val, addr + reg);
> + break;
> + case 2:
> + writew(val, addr + reg);
> + break;
> + case 4:
> + writel(val, addr + reg);
> + break;
> + }
> + rcu_read_unlock();
> +
> + return 0;
> }
>
These raw_pci_{read,write} functions are all ACPI specific so they can move
into the new file as well.
> #ifdef CONFIG_ACPI
> +static int pci_read(struct pci_bus *bus, unsigned int devfn, int where,
> + int size, u32 *value)
> +{
> + return raw_pci_read(pci_domain_nr(bus), bus->number,
> + devfn, where, size, value);
> +}
> +
> +static int pci_write(struct pci_bus *bus, unsigned int devfn, int where,
> + int size, u32 value)
> +{
> + return raw_pci_write(pci_domain_nr(bus), bus->number,
> + devfn, where, size, value);
> +}
> +
> +struct pci_ops pci_root_ops = {
> + .read = pci_read,
> + .write = pci_write,
> +};
> +
> +static struct pci_controller *alloc_pci_controller(int seg)
> +{
> + struct pci_controller *controller;
> +
> + controller = kzalloc(sizeof(*controller), GFP_KERNEL);
> + if (!controller)
> + return NULL;
> +
> + controller->segment = seg;
> + return controller;
> +}
> +
> +struct pci_root_info {
> + struct acpi_device *bridge;
> + struct pci_controller *controller;
> + struct list_head resources;
> + struct resource *res;
> + resource_size_t *res_offset;
Why do you need to keep an array of res_offsets here? You only seem
to be using the values once when you add the resource to the host
bridge windows.
> + unsigned int res_num;
> + char *name;
> +};
> +
> +static acpi_status resource_to_window(struct acpi_resource *resource,
> + struct acpi_resource_address64 *addr)
> +{
> + acpi_status status;
> +
> + /*
> + * We're only interested in _CRS descriptors that are
> + * - address space descriptors for memory
> + * - non-zero size
> + * - producers, i.e., the address space is routed downstream,
> + * not consumed by the bridge itself
> + */
> + status = acpi_resource_to_address64(resource, addr);
> + if (ACPI_SUCCESS(status) &&
> + (addr->resource_type == ACPI_MEMORY_RANGE ||
> + addr->resource_type == ACPI_IO_RANGE) &&
> + addr->address_length &&
> + addr->producer_consumer == ACPI_PRODUCER)
> + return AE_OK;
> +
> + return AE_ERROR;
> +}
> +
> +static acpi_status count_window(struct acpi_resource *resource, void *data)
> +{
> + unsigned int *windows = (unsigned int *) data;
> + struct acpi_resource_address64 addr;
> + acpi_status status;
> +
> + status = resource_to_window(resource, &addr);
> + if (ACPI_SUCCESS(status))
> + (*windows)++;
> +
> + return AE_OK;
> +}
> +
> +static acpi_status add_window(struct acpi_resource *res, void *data)
> +{
> + struct pci_root_info *info = data;
> + struct resource *resource;
> + struct acpi_resource_address64 addr;
> + acpi_status status;
> + unsigned long flags;
> + struct resource *root;
> + u64 start;
> +
> + /* Return AE_OK for non-window resources to keep scanning for more */
> + status = resource_to_window(res, &addr);
> + if (!ACPI_SUCCESS(status))
> + return AE_OK;
> +
> + if (addr.resource_type == ACPI_MEMORY_RANGE) {
> + flags = IORESOURCE_MEM;
> + root = &iomem_resource;
> + } else if (addr.resource_type == ACPI_IO_RANGE) {
> + flags = IORESOURCE_IO;
> + root = &ioport_resource;
> + } else
> + return AE_OK;
> +
> + start = addr.minimum + addr.translation_offset;
> +
> + resource = &info->res[info->res_num];
> + resource->name = info->name;
> + resource->flags = flags;
> + resource->start = start;
> + resource->end = resource->start + addr.address_length - 1;
> +
> + if (flags & IORESOURCE_IO) {
> + unsigned long port;
> + int err;
> +
> + err = pci_register_io_range(start, addr.address_length);
> + if (err)
> + return AE_OK;
> +
> + port = pci_address_to_pio(start);
> + if (port == (unsigned long)-1)
> + return AE_OK;
> +
> + resource->start = port;
> + resource->end = port + addr.address_length - 1;
> +
> + if (pci_remap_iospace(resource, start) < 0)
> + return AE_OK;
You seem to leave around invalid resources every time you return in this code
path due to your choice of ignoring error conditions.
I think there are a few things that can be streamlined in this patch, but
overall I think it looks promising.
Best regards,
Liviu
> +
> + info->res_offset[info->res_num] = 0;
> + } else
> + info->res_offset[info->res_num] = addr.translation_offset;
> +
> + if (insert_resource(root, resource)) {
> + dev_err(&info->bridge->dev,
> + "can't allocate host bridge window %pR\n",
> + resource);
> + } else {
> + if (addr.translation_offset)
> + dev_info(&info->bridge->dev, "host bridge window %pR "
> + "(PCI address [%#llx-%#llx])\n",
> + resource,
> + resource->start - addr.translation_offset,
> + resource->end - addr.translation_offset);
> + else
> + dev_info(&info->bridge->dev,
> + "host bridge window %pR\n", resource);
> + }
> +
> + pci_add_resource_offset(&info->resources, resource,
> + info->res_offset[info->res_num]);
> + info->res_num++;
> + return AE_OK;
> +}
> +
> +static void free_pci_root_info_res(struct pci_root_info *info)
> +{
> + kfree(info->name);
> + kfree(info->res);
> + info->res = NULL;
> + kfree(info->res_offset);
> + info->res_offset = NULL;
> + info->res_num = 0;
> + kfree(info->controller);
> + info->controller = NULL;
> +}
> +
> +static void __release_pci_root_info(struct pci_root_info *info)
> +{
> + int i;
> + struct resource *res;
> +
> + for (i = 0; i < info->res_num; i++) {
> + res = &info->res[i];
> +
> + if (!res->parent)
> + continue;
> +
> + if (!(res->flags & (IORESOURCE_MEM | IORESOURCE_IO)))
> + continue;
> +
> + release_resource(res);
> + }
> +
> + free_pci_root_info_res(info);
> + kfree(info);
> +}
> +
> +static void release_pci_root_info(struct pci_host_bridge *bridge)
> +{
> + struct pci_root_info *info = bridge->release_data;
> +
> + __release_pci_root_info(info);
> +}
> +
> +static int
> +probe_pci_root_info(struct pci_root_info *info, struct acpi_device *device,
> + int busnum, int domain)
> +{
> + char *name;
> +
> + name = kmalloc(16, GFP_KERNEL);
> + if (!name)
> + return -ENOMEM;
> +
> + sprintf(name, "PCI Bus %04x:%02x", domain, busnum);
> + info->bridge = device;
> + info->name = name;
> +
> + acpi_walk_resources(device->handle, METHOD_NAME__CRS, count_window,
> + &info->res_num);
> + if (info->res_num) {
> + info->res =
> + kzalloc_node(sizeof(*info->res) * info->res_num,
> + GFP_KERNEL, info->controller->node);
> + if (!info->res) {
> + kfree(name);
> + return -ENOMEM;
> + }
> +
> + info->res_offset =
> + kzalloc_node(sizeof(*info->res_offset) * info->res_num,
> + GFP_KERNEL, info->controller->node);
> + if (!info->res_offset) {
> + kfree(name);
> + kfree(info->res);
> + info->res = NULL;
> + return -ENOMEM;
> + }
> +
> + info->res_num = 0;
> + acpi_walk_resources(device->handle, METHOD_NAME__CRS,
> + add_window, info);
> + } else
> + kfree(name);
> +
> + return 0;
> +}
> +
> /* Root bridge scanning */
> struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
> {
> - /* TODO: Should be revisited when implementing PCI on ACPI */
> - return NULL;
> + struct acpi_device *device = root->device;
> + int domain = root->segment;
> + int bus = root->secondary.start;
> + struct pci_controller *controller;
> + struct pci_root_info *info = NULL;
> + int busnum = root->secondary.start;
> + struct pci_bus *pbus;
> + int ret;
> +
> + controller = alloc_pci_controller(domain);
> + if (!controller)
> + return NULL;
> +
> + controller->companion = device;
> + controller->node = acpi_get_node(device->handle);
> +
> + info = kzalloc(sizeof(*info), GFP_KERNEL);
> + if (!info) {
> + dev_err(&device->dev,
> + "pci_bus %04x:%02x: ignored (out of memory)\n",
> + domain, busnum);
> + kfree(controller);
> + return NULL;
> + }
> +
> + info->controller = controller;
> + INIT_LIST_HEAD(&info->resources);
> +
> + ret = probe_pci_root_info(info, device, busnum, domain);
> + if (ret) {
> + kfree(info->controller);
> + kfree(info);
> + return NULL;
> + }
> + /* insert busn resource at first */
> + pci_add_resource(&info->resources, &root->secondary);
> +
> + pbus = pci_create_root_bus(NULL, bus, &pci_root_ops, controller,
> + &info->resources);
> + if (!pbus) {
> + pci_free_resource_list(&info->resources);
> + __release_pci_root_info(info);
> + return NULL;
> + }
> +
> + pci_set_host_bridge_release(to_pci_host_bridge(pbus->bridge),
> + release_pci_root_info, info);
> + pci_scan_child_bus(pbus);
> + return pbus;
> }
> -#endif
> +#endif /* CONFIG_ACPI */
> --
> 1.9.1
>
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists