Re: [PATCH v3 1/2] PCI: MCFG: Consolidate the separate PCI MCFG table entry list

From: Xuesong Chen
Date: Tue Oct 26 2021 - 23:14:45 EST

Next message: 王贇: "[PATCH v7 1/2] ftrace: disable preemption when recursion locked"
Previous message: Masami Hiramatsu: "Re: [PATCH v4 7/8] tracing/selftests: Add tests for hist trigger expression parsing"
In reply to: Lorenzo Pieralisi: "Re: [PATCH v3 1/2] PCI: MCFG: Consolidate the separate PCI MCFG table entry list"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 26/10/2021 22:08, Lorenzo Pieralisi wrote:
> On Tue, Oct 26, 2021 at 03:14:37PM +0200, Rafael J. Wysocki wrote:
>> On Tue, Oct 19, 2021 at 6:50 AM Xuesong Chen
>> <xuesong.chen@xxxxxxxxxxxxxxxxx> wrote:
>>>
>>> The PCI MCFG entry list is discrete on x86 and other arches like ARM64
>>> in current implementation, this list variable can be consolidated for
>>> unnecessary duplication and other purposes, for example, we can remove
>>> some of the arch-specific codes in the APEI/EINJ module and re-implement
>>> it in a more common arch-agnostic way.
>>>
>>> To reduce the redundancy, it:
>>> - Moves the "struct pci_mmcfg_region" definition from
>>> arch/x86/include/asm/pci_x86.h to include/linux/pci.h, where it
>>> can be shared across arches.
>>>
>>> - Moves pci_mmcfg_list (a list of pci_mmcfg_region structs) from
>>> arch/x86/pci/mmconfig-shared.c to drivers/pci/pci.c, where it can
>>> be shared across arches.
>>>
>>> - On x86 (which does not enable CONFIG_ACPI_MCFG), pci_mmcfg_list is
>>> built in arch/x86/pci/mmconfig-shared.c as before.
>>>
>>> - Removes the "struct mcfg_entry" from drivers/acpi/pci_mcfg.c.
>>>
>>> - Replaces pci_mcfg_list (previously a list of mcfg_entry structs)
>>> in drivers/acpi/pci_mcfg.c with the newly-shared pci_mmcfg_list (a
>>> list of pci_mmcfg_region structs).
>>>
>>> - On ARM64 (which does enable CONFIG_ACPI_MCFG), pci_mmcfg_list is
>>> built in drivers/acpi/pci_mcfg.c.
>>>
>>> Signed-off-by: Xuesong Chen <xuesong.chen@xxxxxxxxxxxxxxxxx>
>>> Reviewed-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
>>> Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
>>
>> I'm guessing that I'm expected to pick up this one?
>
> I did not provide a Reviewed-by: tag for this patch (and I don't
> think Bjorn provided his either).
>
> That's valid also for patch 2:
>
> https://lore.kernel.org/linux-pci/20211019150405.GA2338201@bhelgaas
>

Ah, those tags will be dropped in the next version together with the
'Reported-by: kernel test robot <lkp@xxxxxxxxx>' tag suggested by Bjorn.

Thanks for pointing this out and sorry for the possible confusion from
my courtesy position :-)

Thanks,
Xuesong

>>> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
>>> Cc: James Morse <james.morse@xxxxxxx>
>>> Cc: Will Deacon <will@xxxxxxxxxx>
>>> Cc: Rafael. J. Wysocki <rafael@xxxxxxxxxx>
>>> Cc: Tony Luck <tony.luck@xxxxxxxxx>
>>> Cc: Tomasz Nowicki <tn@xxxxxxxxxxxx>
>>> ---
>>> arch/x86/include/asm/pci_x86.h | 17 +----------------
>>> arch/x86/pci/mmconfig-shared.c | 2 --
>>> drivers/acpi/pci_mcfg.c | 34 +++++++++++++---------------------
>>> drivers/pci/pci.c | 2 ++
>>> include/linux/pci.h | 17 +++++++++++++++++
>>> 5 files changed, 33 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/arch/x86/include/asm/pci_x86.h b/arch/x86/include/asm/pci_x86.h
>>> index 490411d..1f4257c 100644
>>> --- a/arch/x86/include/asm/pci_x86.h
>>> +++ b/arch/x86/include/asm/pci_x86.h
>>> @@ -146,20 +146,7 @@ static inline int __init pci_acpi_init(void)
>>> extern void pcibios_fixup_irqs(void);
>>>
>>> /* pci-mmconfig.c */
>>> -
>>> -/* "PCI MMCONFIG %04x [bus %02x-%02x]" */
>>> -#define PCI_MMCFG_RESOURCE_NAME_LEN (22 + 4 + 2 + 2)
>>> -
>>> -struct pci_mmcfg_region {
>>> - struct list_head list;
>>> - struct resource res;
>>> - u64 address;
>>> - char __iomem *virt;
>>> - u16 segment;
>>> - u8 start_bus;
>>> - u8 end_bus;
>>> - char name[PCI_MMCFG_RESOURCE_NAME_LEN];
>>> -};
>>> +struct pci_mmcfg_region;
>>>
>>> extern int __init pci_mmcfg_arch_init(void);
>>> extern void __init pci_mmcfg_arch_free(void);
>>> @@ -174,8 +161,6 @@ extern struct pci_mmcfg_region *__init pci_mmconfig_add(int segment, int start,
>>>
>>> extern struct list_head pci_mmcfg_list;
>>>
>>> -#define PCI_MMCFG_BUS_OFFSET(bus) ((bus) << 20)
>>> -
>>> /*
>>> * On AMD Fam10h CPUs, all PCI MMIO configuration space accesses must use
>>> * %eax. No other source or target registers may be used. The following
>>> diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
>>> index 758cbfe..0b961fe6 100644
>>> --- a/arch/x86/pci/mmconfig-shared.c
>>> +++ b/arch/x86/pci/mmconfig-shared.c
>>> @@ -31,8 +31,6 @@
>>> static DEFINE_MUTEX(pci_mmcfg_lock);
>>> #define pci_mmcfg_lock_held() lock_is_held(&(pci_mmcfg_lock).dep_map)
>>>
>>> -LIST_HEAD(pci_mmcfg_list);
>>> -
>>> static void __init pci_mmconfig_remove(struct pci_mmcfg_region *cfg)
>>> {
>>> if (cfg->res.parent)
>>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
>>> index 53cab97..d9506b0 100644
>>> --- a/drivers/acpi/pci_mcfg.c
>>> +++ b/drivers/acpi/pci_mcfg.c
>>> @@ -13,14 +13,7 @@
>>> #include <linux/pci-acpi.h>
>>> #include <linux/pci-ecam.h>
>>>
>>> -/* Structure to hold entries from the MCFG table */
>>> -struct mcfg_entry {
>>> - struct list_head list;
>>> - phys_addr_t addr;
>>> - u16 segment;
>>> - u8 bus_start;
>>> - u8 bus_end;
>>> -};
>>> +extern struct list_head pci_mmcfg_list;
>>>
>>> #ifdef CONFIG_PCI_QUIRKS
>>> struct mcfg_fixup {
>>> @@ -214,16 +207,13 @@ static void pci_mcfg_apply_quirks(struct acpi_pci_root *root,
>>> #endif
>>> }
>>>
>>> -/* List to save MCFG entries */
>>> -static LIST_HEAD(pci_mcfg_list);
>>> -
>>> int pci_mcfg_lookup(struct acpi_pci_root *root, struct resource *cfgres,
>>> const struct pci_ecam_ops **ecam_ops)
>>> {
>>> const struct pci_ecam_ops *ops = &pci_generic_ecam_ops;
>>> struct resource *bus_res = &root->secondary;
>>> u16 seg = root->segment;
>>> - struct mcfg_entry *e;
>>> + struct pci_mmcfg_region *e;
>>> struct resource res;
>>>
>>> /* Use address from _CBA if present, otherwise lookup MCFG */
>>> @@ -233,10 +223,10 @@ int pci_mcfg_lookup(struct acpi_pci_root *root, struct resource *cfgres,
>>> /*
>>> * We expect the range in bus_res in the coverage of MCFG bus range.
>>> */
>>> - list_for_each_entry(e, &pci_mcfg_list, list) {
>>> - if (e->segment == seg && e->bus_start <= bus_res->start &&
>>> - e->bus_end >= bus_res->end) {
>>> - root->mcfg_addr = e->addr;
>>> + list_for_each_entry(e, &pci_mmcfg_list, list) {
>>> + if (e->segment == seg && e->start_bus <= bus_res->start &&
>>> + e->end_bus >= bus_res->end) {
>>> + root->mcfg_addr = e->address;
>>> }
>>>
>>> }
>>> @@ -268,7 +258,7 @@ static __init int pci_mcfg_parse(struct acpi_table_header *header)
>>> {
>>> struct acpi_table_mcfg *mcfg;
>>> struct acpi_mcfg_allocation *mptr;
>>> - struct mcfg_entry *e, *arr;
>>> + struct pci_mmcfg_region *e, *arr;
>>> int i, n;
>>>
>>> if (header->length < sizeof(struct acpi_table_mcfg))
>>> @@ -285,10 +275,12 @@ static __init int pci_mcfg_parse(struct acpi_table_header *header)
>>>
>>> for (i = 0, e = arr; i < n; i++, mptr++, e++) {
>>> e->segment = mptr->pci_segment;
>>> - e->addr = mptr->address;
>>> - e->bus_start = mptr->start_bus_number;
>>> - e->bus_end = mptr->end_bus_number;
>>> - list_add(&e->list, &pci_mcfg_list);
>>> + e->address = mptr->address;
>>> + e->start_bus = mptr->start_bus_number;
>>> + e->end_bus = mptr->end_bus_number;
>>> + e->res.start = e->address + PCI_MMCFG_BUS_OFFSET(e->start_bus);
>>> + e->res.end = e->address + PCI_MMCFG_BUS_OFFSET(e->end_bus + 1) - 1;
>>> + list_add(&e->list, &pci_mmcfg_list);
>>> }
>>>
>>> #ifdef CONFIG_PCI_QUIRKS
>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>> index ce2ab62..899004e 100644
>>> --- a/drivers/pci/pci.c
>>> +++ b/drivers/pci/pci.c
>>> @@ -47,6 +47,8 @@
>>> int pci_pci_problems;
>>> EXPORT_SYMBOL(pci_pci_problems);
>>>
>>> +LIST_HEAD(pci_mmcfg_list);
>>> +
>>> unsigned int pci_pm_d3hot_delay;
>>>
>>> static void pci_pme_list_scan(struct work_struct *work);
>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>> index cd8aa6f..71e4c06 100644
>>> --- a/include/linux/pci.h
>>> +++ b/include/linux/pci.h
>>> @@ -55,6 +55,23 @@
>>> #define PCI_RESET_PROBE true
>>> #define PCI_RESET_DO_RESET false
>>>
>>> +#define PCI_MMCFG_BUS_OFFSET(bus) ((bus) << 20)
>>> +
>>> +/* "PCI MMCONFIG %04x [bus %02x-%02x]" */
>>> +#define PCI_MMCFG_RESOURCE_NAME_LEN (22 + 4 + 2 + 2)
>>> +
>>> +/* pci mcfg region */
>>> +struct pci_mmcfg_region {
>>> + struct list_head list;
>>> + struct resource res;
>>> + u64 address;
>>> + char __iomem *virt;
>>> + u16 segment;
>>> + u8 start_bus;
>>> + u8 end_bus;
>>> + char name[PCI_MMCFG_RESOURCE_NAME_LEN];
>>> +};
>>> +
>>> /*
>>> * The PCI interface treats multi-function devices as independent
>>> * devices. The slot/function address of each device is encoded
>>> --
>>> 1.8.3.1
>>>

Next message: 王贇: "[PATCH v7 1/2] ftrace: disable preemption when recursion locked"
Previous message: Masami Hiramatsu: "Re: [PATCH v4 7/8] tracing/selftests: Add tests for hist trigger expression parsing"
In reply to: Lorenzo Pieralisi: "Re: [PATCH v3 1/2] PCI: MCFG: Consolidate the separate PCI MCFG table entry list"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]