Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1416825 > unrolled thread

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

Started byBjorn Helgaas <helgaas@kernel.org>
First post2016-06-08 04:00 +0200
Last post2016-06-08 15:50 +0200
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V8 7/9] acpi: Add generic MCFG table handling Bjorn Helgaas <helgaas@kernel.org> - 2016-06-08 04:00 +0200
    Re: [PATCH V8 7/9] acpi: Add generic MCFG table handling Tomasz Nowicki <tn@semihalf.com> - 2016-06-08 14:30 +0200
      Re: [PATCH V8 7/9] acpi: Add generic MCFG table handling Bjorn Helgaas <helgaas@kernel.org> - 2016-06-08 15:20 +0200
        Re: [PATCH V8 7/9] acpi: Add generic MCFG table handling Tomasz Nowicki <tn@semihalf.com> - 2016-06-08 15:50 +0200

#1416825 — Re: [PATCH V8 7/9] acpi: Add generic MCFG table handling

FromBjorn Helgaas <helgaas@kernel.org>
Date2016-06-08 04:00 +0200
SubjectRe: [PATCH V8 7/9] acpi: Add generic MCFG table handling
Message-ID<rHAWJ-21L-3@gated-at.bofh.it>
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@semihalf.com>
> Signed-off-by: Jayachandran C <jchandra@broadcom.com>
> ---
>  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@broadcom.com>
> + * Copyright (C) 2016 Semihalf
> + * 	Author: Tomasz Nowicki <tn@semihalf.com>
> + *
> + * 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.

> +{
> +	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;
    }

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

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

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.

> +void __init pci_mmcfg_late_init(void)
> +{
> +	int err = acpi_table_parse(ACPI_SIG_MCFG, pci_mcfg_parse);
> +	if (err)
> +		pr_err("Failed to parse MCFG (%d)\n", err);
> +}
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 89ab057..e0e6dfc 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -24,6 +24,8 @@ static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev)
>  }
>  extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle);
>  
> +extern int pci_mcfg_lookup(struct acpi_pci_root *root);
> +
>  static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev)
>  {
>  	struct pci_bus *pbus = pdev->bus;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index bba4053..9661c85 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1724,7 +1724,7 @@ void pcibios_free_irq(struct pci_dev *dev);
>  extern struct dev_pm_ops pcibios_pm_ops;
>  #endif
>  
> -#ifdef CONFIG_PCI_MMCONFIG
> +#if defined(CONFIG_PCI_MMCONFIG) || defined(CONFIG_ACPI_MCFG)
>  void __init pci_mmcfg_early_init(void);
>  void __init pci_mmcfg_late_init(void);
>  #else
> -- 
> 1.9.1
>

[toc] | [next] | [standalone]

#1417356

FromTomasz Nowicki <tn@semihalf.com>
Date2016-06-08 14:30 +0200
Message-ID<rHKMq-8w1-17@gated-at.bofh.it>
In reply to#1416825
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@semihalf.com>
>> Signed-off-by: Jayachandran C <jchandra@broadcom.com>
>> ---
>>   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@broadcom.com>
>> + * Copyright (C) 2016 Semihalf
>> + * 	Author: Tomasz Nowicki <tn@semihalf.com>
>> + *
>> + * 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

[toc] | [prev] | [next] | [standalone]

#1417413

FromBjorn Helgaas <helgaas@kernel.org>
Date2016-06-08 15:20 +0200
Message-ID<rHLyN-E7-21@gated-at.bofh.it>
In reply to#1417356
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

[toc] | [prev] | [next] | [standalone]

#1417446

FromTomasz Nowicki <tn@semihalf.com>
Date2016-06-08 15:50 +0200
Message-ID<rHM1Q-Pq-31@gated-at.bofh.it>
In reply to#1417413
On 08.06.2016 15:17, Bjorn Helgaas wrote:
> 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.

Make sense to me.

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

Right, so we are back to MCFG cache.

Thanks,
Tomasz

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web