[PATCH V8 7/9] acpi: Add generic MCFG table handling

Bjorn Helgaas helgaas at kernel.org
Wed Jun 8 06:17:09 PDT 2016


On Wed, Jun 08, 2016 at 02:21:30PM +0200, Tomasz Nowicki wrote:
> 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.

> >>+	/* 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.

Lack of a bus range in _CRS is a firmware defect.  There's a comment
about this in acpi_pci_root_add().  On x86, we probably had to live
with firmware in the field that had this defect.  I think we should
expect all ARM64 systems to provide a bus number range in _CRS, and
fail the attach if it's not there.

I don't think we should warn about an MCFG entry that covers more than
the _CRS bus range.  On x86, it's common to have something like:

  ACPI: PCI Root Bridge [PCI0] (domain 0000 [bus 00-7f])
  ACPI: PCI Root Bridge [PCI1] (domain 0000 [bus 80-ff])

with a single MCFG entry that covers [bus 00-ff].  That seems
reasonable and I don't think it's worth warning about it.

If the MCFG entry doesn't cover all of a _CRS bus range, we should
just fail so we can find and fix broken firmware.

> >>+/* 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.

Hmmm...  I see your argument, but this is a problem waiting to happen.
We should not depend on the internal implementation of
early_acpi_os_unmap_memory().  The pattern of:

  y = x;
  unmap(x);
  z = *y;

is just broken and we shouldn't expect readers to recognize that "oh,
unmap() isn't really unmapping anything in this special case, so this
looks wrong but is really fine."

Bjorn



More information about the linux-arm-kernel mailing list