Re: [PATCH V8 7/9] acpi: Add generic MCFG table handling
From: Tomasz Nowicki
Date: Wed Jun 08 2016 - 08:21:39 EST
On 08.06.2016 03:56, Bjorn Helgaas wrote:
On Mon, May 30, 2016 at 05:14:20PM +0200, Tomasz Nowicki wrote:
In order to handle PCI config space regions properly in ACPI, new MCFG
interface is defined which does sanity checks on MCFG table and keeps its
root pointer. The user is able to lookup MCFG regions based on
host bridge root structure and domain:bus_start:bus_end touple.
Use pci_mmcfg_late_init old prototype to avoid another function name.
Signed-off-by: Tomasz Nowicki <tn@xxxxxxxxxxxx>
Signed-off-by: Jayachandran C <jchandra@xxxxxxxxxxxx>
---
drivers/acpi/Kconfig | 3 ++
drivers/acpi/Makefile | 1 +
drivers/acpi/pci_mcfg.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/pci-acpi.h | 2 ++
include/linux/pci.h | 2 +-
5 files changed, 101 insertions(+), 1 deletion(-)
create mode 100644 drivers/acpi/pci_mcfg.c
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index b7e2e77..f98c328 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -217,6 +217,9 @@ config ACPI_PROCESSOR_IDLE
bool
select CPU_IDLE
+config ACPI_MCFG
+ bool
+
config ACPI_CPPC_LIB
bool
depends on ACPI_PROCESSOR
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 251ce85..632e81f 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -40,6 +40,7 @@ acpi-$(CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC) += processor_pdc.o
acpi-y += ec.o
acpi-$(CONFIG_ACPI_DOCK) += dock.o
acpi-y += pci_root.o pci_link.o pci_irq.o
+obj-$(CONFIG_ACPI_MCFG) += pci_mcfg.o
acpi-y += acpi_lpss.o acpi_apd.o
acpi-y += acpi_platform.o
acpi-y += acpi_pnp.o
diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
new file mode 100644
index 0000000..1847f74
--- /dev/null
+++ b/drivers/acpi/pci_mcfg.c
@@ -0,0 +1,94 @@
+/*
+ * Copyright (C) 2016 Broadcom
+ * Author: Jayachandran C <jchandra@xxxxxxxxxxxx>
+ * Copyright (C) 2016 Semihalf
+ * Author: Tomasz Nowicki <tn@xxxxxxxxxxxx>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2, as
+ * published by the Free Software Foundation (the "GPL").
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License version 2 (GPLv2) for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * version 2 (GPLv2) along with this source code.
+ */
+
+#define pr_fmt(fmt) "ACPI: " fmt
+
+#include <linux/kernel.h>
+#include <linux/pci.h>
+#include <linux/pci-acpi.h>
+
+/* Root pointer to the mapped MCFG table */
+static struct acpi_table_mcfg *mcfg_table;
+static int mcfg_entries;
+
+int pci_mcfg_lookup(struct acpi_pci_root *root)
I think this would be better if we passed in the domain and a pointer
to the bus range resource and returned the ECAM base address. I don't
think we need to be connected to struct acpi_pci_root.
I will use domain and bus range resource as you suggested.
+{
+ struct acpi_mcfg_allocation *mptr, *entry = NULL;
+ struct resource *bus_res = &root->secondary;
+ int i;
+
+ if (mcfg_table) {
+ mptr = (struct acpi_mcfg_allocation *) &mcfg_table[1];
+ for (i = 0; i < mcfg_entries && !entry; i++, mptr++)
+ if (mptr->pci_segment == root->segment &&
+ mptr->start_bus_number == bus_res->start)
+ entry = mptr;
+ }
+
+ /* not found, use _CBA if available, else error */
+ if (!entry) {
+ if (root->mcfg_addr)
+ return root->mcfg_addr;
+ pr_err("%04x:%pR MCFG lookup failed\n", root->segment, bus_res);
+ return -ENOENT;
+ } else if (root->mcfg_addr && entry->address != root->mcfg_addr) {
+ pr_warn("%04x:%pR CBA %pa != MCFG %lx, using CBA\n",
+ root->segment, bus_res, &root->mcfg_addr,
+ (unsigned long)entry->address);
+ return 0;
+ }
I keep looking at this code, trying to find where we "use _CBA", but I
can't find it. Oh, I see, acpi_pci_root_add() calls
acpi_pci_root_get_mcfg_addr() (which evaluates _CBA), and sets
root->mcfg_addr to the result.
"root->mcfg_addr" is sort of an unfortunate name because "MCFG" is the
ACPI table name, we've used "ECAM" for the memory-mapped config space,
and this address can come from either the MCFG table or the _CBA
method.
In the case where we have both _CBA and an MCFG entry, I think we
should prefer _CBA, and I'm not sure it's even worth warning about it.
So I think you could simplify this function if you made
pci_acpi_setup_ecam_mapping() look like this:
... pci_acpi_setup_ecam_mapping(...)
{
...
if (!root->mcfg_addr)
root->mcfg_addr = pci_mcfg_lookup(root);
if (!root->mcfg_addr) {
dev_err(..., "no ECAM region found\n");
return -EINVAL;
}
OK
+
+ /* found matching entry, bus range check */
+ if (entry->end_bus_number != bus_res->end) {
+ resource_size_t bus_end = min_t(resource_size_t,
+ entry->end_bus_number, bus_res->end);
+ pr_warn("%04x:%pR bus end mismatch, using %02lx\n",
+ root->segment, bus_res, (unsigned long)bus_end);
+ bus_res->end = bus_end;
+ }
What about bus end mismatch case? Should we trim the host bridge bus
range or expect MCFG entry covers that range? Sometimes we get _BBN-0xFF
bus range, not from _CRS.
+
+ if (!root->mcfg_addr)
+ root->mcfg_addr = entry->address;
Please move the assignment to the caller (I think Lorenzo pointed this
out already).
+ return 0;
+}
+
+static __init int pci_mcfg_parse(struct acpi_table_header *header)
+{
+ if (header->length < sizeof(struct acpi_table_mcfg))
+ return -EINVAL;
+
+ mcfg_entries = (header->length - sizeof(struct acpi_table_mcfg)) /
+ sizeof(struct acpi_mcfg_allocation);
+ if (mcfg_entries == 0) {
+ pr_err("MCFG has no entries\n");
Include an address here? I'm not really sure either of the messages
here is necessary. Users (callers of pci_mcfg_lookup()) will notice
if we can't find any ECAM space and will probably complain there,
where the message can include more information, e.g., the affected
device.
I would keep message about how many entries we found here. It would be
valuable information IMO.
+ return -EINVAL;
+ }
+
+ mcfg_table = (struct acpi_table_mcfg *)header;
+ pr_info("MCFG table detected, %d entries\n", mcfg_entries);
+ return 0;
+}
+
+/* Interface called by ACPI - parse and save MCFG table */
I think we save a *pointer* to the MCFG table, not the table itself.
Right, the comment is broken.
And acpi_table_parse() calls early_acpi_os_unmap_memory() immediately
after it calls pci_mcfg_parse(), so I'm doubtful that the pointer
remains valid.
At this stage early_acpi_os_unmap_memory() is doing nothing since
acpi_early_init() set acpi_gbl_permanent_mmap to 1 way before. The
pointer is fine then.
Thanks,
Tomasz