Re: [PATCH] x86: split e820 reserved entries record to late v2

Yinghai Lu
2008-08-28T20:52:35Z

[-- Attachment #1: Type: text/plain, Size: 455 bytes --]

On Thu, Aug 28, 2008 at 1:40 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
>
> On Thu, 28 Aug 2008, Yinghai Lu wrote:
>>
>> v2: insert e820 reserve resources before pnp_system_init
>
> Looks ok by me. Now it just needs testing ;)
>
> Does it actually fix the HPET regression on that odd machine (without the
> special hacks to recognize HPET explicitly)?
>

David,

can you test attached patch?
also you may try to revert the old patch.

YH

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: split_e820_reserve.patch --]
[-- Type: text/x-patch; name=split_e820_reserve.patch, Size: 2978 bytes --]

From: Yinghai Lu <yhlu.kernel@gmail.com>
Subject: [PATCH] x86: split e820 reserved entries record to late v2

so could let BAR res register at first, or even pnp?

v2: insert e820 reserve resources before pnp_system_init

Signed-off-by: Yinghai Lu <yhlu.kernel@gmail.com>

---
arch/x86/kernel/e820.c | 20 ++++++++++++++++++--
arch/x86/pci/i386.c | 3 +++
include/asm-x86/e820.h | 1 +
3 files changed, 22 insertions(+), 2 deletions(-)

Index: linux-2.6/arch/x86/kernel/e820.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/e820.c
+++ linux-2.6/arch/x86/kernel/e820.c
@@ -1271,13 +1271,15 @@ static inline const char *e820_type_to_s
/*
* Mark e820 reserved areas as busy for the resource manager.
*/
+struct resource __initdata *e820_res;
void __init e820_reserve_resources(void)
{
int i;
- struct resource *res;
u64 end;
+ struct resource *res;

res = alloc_bootmem_low(sizeof(struct resource) * e820.nr_map);
+ e820_res = res;
for (i = 0; i < e820.nr_map; i++) {
end = e820.map[i].addr + e820.map[i].size - 1;
#ifndef CONFIG_RESOURCES_64BIT
@@ -1291,7 +1293,8 @@ void __init e820_reserve_resources(void)
res->end = end;

res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
- insert_resource(&iomem_resource, res);
+ if (e820.map[i].type != E820_RESERVED || res->start < (1ULL<<20))
+ insert_resource(&iomem_resource, res);
res++;
}

@@ -1303,6 +1306,19 @@ void __init e820_reserve_resources(void)
}
}

+void __init e820_reserve_resources_late(void)
+{
+ int i;
+ struct resource *res;
+
+ res = e820_res;
+ for (i = 0; i < e820.nr_map; i++) {
+ if (e820.map[i].type == E820_RESERVED && res->start >= (1ULL<<20))
+ insert_resource(&iomem_resource, res);
+ res++;
+ }
+}
+
char *__init default_machine_specific_memory_setup(void)
{
char *who = "BIOS-e820";
Index: linux-2.6/arch/x86/pci/i386.c
===================================================================
--- linux-2.6.orig/arch/x86/pci/i386.c
+++ linux-2.6/arch/x86/pci/i386.c
@@ -36,6 +36,7 @@
#include <asm/pat.h>
#include <asm/hpet.h>
#include <asm/io_apic.h>
+#include <asm/e820.h>

#include "pci.h"

@@ -308,6 +309,8 @@ void __init pcibios_resource_survey(void
pcibios_allocate_bus_resources(&pci_root_buses);
pcibios_allocate_resources(0);
pcibios_allocate_resources(1);
+
+ e820_reserve_resources_late();
}

/**
Index: linux-2.6/include/asm-x86/e820.h
===================================================================
--- linux-2.6.orig/include/asm-x86/e820.h
+++ linux-2.6/include/asm-x86/e820.h
@@ -122,6 +122,7 @@ extern void e820_register_active_regions
extern u64 e820_hole_size(u64 start, u64 end);
extern void finish_e820_parsing(void);
extern void e820_reserve_resources(void);
+extern void e820_reserve_resources_late(void);
extern void setup_memory_map(void);
extern char *default_machine_specific_memory_setup(void);
extern char *machine_specific_memory_setup(void);

[-- Attachment #3: commit-a2bd727 --]
[-- Type: application/octet-stream, Size: 4273 bytes --]

commit a2bd7274b47124d2fc4dfdb8c0591f545ba749dd
Author: Yinghai Lu <yhlu.kernel@gmail.com>
Date: Mon Aug 25 00:56:08 2008 -0700

x86: fix HPET regression in 2.6.26 versus 2.6.25, check hpet against BAR, v3

David Witbrodt tracked down (and bisected) a hpet bootup hang on his
system to the following problem: a BIOS bug made the hpet device
visible as a generic PCI device. If e820 reserved entries happen to
be registered first in the resource tree [which v2.6.26 started doing],
then the PCI code will reallocate that device's BAR to some other
address - breaking timer IRQs and hanging the system.

( Normally hpet devices are hidden by the BIOS from the OS's PCI
discovery via chipset magic. Sometimes the hpet is not a PCI device
at all. )

Solve this fundamental fragility by making non-PCI platform drivers
insert resources into the resource tree even if it overlaps the e820
reserved entry, to keep the resource manager from updating the BAR.

Also do these checks for the ioapic and mmconfig addresses, and emit
a warning if this happens.

Bisected-by: David Witbrodt <dawitbro@sbcglobal.net>
Signed-off-by: Yinghai Lu <yhlu.kernel@gmail.com>
Tested-by: David Witbrodt <dawitbro@sbcglobal.net>
Signed-off-by: Ingo Molnar <mingo@elte.hu>

diff --git a/arch/x86/pci/i386.c b/arch/x86/pci/i386.c
index 5807d1b..d765da9 100644
--- a/arch/x86/pci/i386.c
+++ b/arch/x86/pci/i386.c
@@ -31,8 +31,11 @@
#include <linux/ioport.h>
#include <linux/errno.h>
#include <linux/bootmem.h>
+#include <linux/acpi.h>

#include <asm/pat.h>
+#include <asm/hpet.h>
+#include <asm/io_apic.h>

#include "pci.h"

@@ -77,6 +80,77 @@ pcibios_align_resource(void *data, struct resource *res,
}
EXPORT_SYMBOL(pcibios_align_resource);

+static int check_res_with_valid(struct pci_dev *dev, struct resource *res)
+{
+ unsigned long base;
+ unsigned long size;
+ int i;
+
+ base = res->start;
+ size = (res->start == 0 && res->end == res->start) ? 0 :
+ (res->end - res->start + 1);
+
+ if (!base || !size)
+ return 0;
+
+#ifdef CONFIG_HPET_TIMER
+ /* for hpet */
+ if (base == hpet_address && (res->flags & IORESOURCE_MEM)) {
+ dev_info(&dev->dev, "BAR has HPET at %08lx-%08lx\n",
+ base, base + size - 1);
+ return 1;
+ }
+#endif
+
+#ifdef CONFIG_X86_IO_APIC
+ for (i = 0; i < nr_ioapics; i++) {
+ unsigned long ioapic_phys = mp_ioapics[i].mp_apicaddr;
+
+ if (base == ioapic_phys && (res->flags & IORESOURCE_MEM)) {
+ dev_info(&dev->dev, "BAR has ioapic at %08lx-%08lx\n",
+ base, base + size - 1);
+ return 1;
+ }
+ }
+#endif
+
+#ifdef CONFIG_PCI_MMCONFIG
+ for (i = 0; i < pci_mmcfg_config_num; i++) {
+ unsigned long addr;
+
+ addr = pci_mmcfg_config[i].address;
+ if (base == addr && (res->flags & IORESOURCE_MEM)) {
+ dev_info(&dev->dev, "BAR has MMCONFIG at %08lx-%08lx\n",
+ base, base + size - 1);
+ return 1;
+ }
+ }
+#endif
+
+ return 0;
+}
+
+static int check_platform(struct pci_dev *dev, struct resource *res)
+{
+ struct resource *root = NULL;
+
+ /*
+ * forcibly insert it into the
+ * resource tree
+ */
+ if (res->flags & IORESOURCE_MEM)
+ root = &iomem_resource;
+ else if (res->flags & IORESOURCE_IO)
+ root = &ioport_resource;
+
+ if (root && check_res_with_valid(dev, res)) {
+ insert_resource(root, res);
+
+ return 1;
+ }
+
+ return 0;
+}
/*
* Handle resources of PCI devices. If the world were perfect, we could
* just allocate all the resource regions and do nothing more. It isn't.
@@ -128,6 +202,8 @@ static void __init pcibios_allocate_bus_resources(struct list_head *bus_list)
pr = pci_find_parent_resource(dev, r);
if (!r->start || !pr ||
request_resource(pr, r) < 0) {
+ if (check_platform(dev, r))
+ continue;
dev_err(&dev->dev, "BAR %d: can't "
"allocate resource\n", idx);
/*
@@ -171,6 +247,8 @@ static void __init pcibios_allocate_resources(int pass)
r->flags, disabled, pass);
pr = pci_find_parent_resource(dev, r);
if (!pr || request_resource(pr, r) < 0) {
+ if (check_platform(dev, r))
+ continue;
dev_err(&dev->dev, "BAR %d: can't "
"allocate resource\n", idx);
/* We'll assign a new address later */