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

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

[David Witbrodt]

> From: Yinghai Lu <yhlu.kernel@gmail.com>
[...]
> please hang a while, there is some merging problem with current tip/master.
> 
> one or two hours later ingo may fix it.
> 
> or please try attached two patches....

Yinghai,

I applied your patches to the tip/master from yesterday (IOW, I did not
run 'git remote update' before the following session.

The patches applied fine and the kernel built and ran OK (though with a
little weirdness... see further down), but it seems that the combined 
changes that these 2 patches make to e820.h result in no change to that
file at all!  See 'git status' output at the end of this session:

===== SHELL STUFF ============
$ git show |head
commit e3fc96d5aca609bcf6ab0327850a109df65c1dbb
Merge: 6b8c836... a36d241...
Author: Ingo Molnar <mingo@elte.hu>
Date:   Thu Aug 28 22:57:20 2008 +0200

    Merge branch 'x86/core'

$ git apply --verbose --check ../yh01-revert_wrong_split_e820_reserve.patch 
Checking patch arch/x86/kernel/e820.c...
Checking patch arch/x86/pci/i386.c...
Checking patch include/asm-x86/e820.h...

$ git apply --verbose ../yh01-revert_wrong_split_e820_reserve.patch 
Checking patch arch/x86/kernel/e820.c...
Checking patch arch/x86/pci/i386.c...
Checking patch include/asm-x86/e820.h...
Applied patch arch/x86/kernel/e820.c cleanly.
Applied patch arch/x86/pci/i386.c cleanly.
Applied patch include/asm-x86/e820.h cleanly.

$ git apply --verbose --check ../yh02-split_e820_reserve.patch 
Checking patch arch/x86/kernel/e820.c...
Checking patch arch/x86/pci/i386.c...
Checking patch include/asm-x86/e820.h...

$ git apply --verbose ../yh02-split_e820_reserve.patch 
Checking patch arch/x86/kernel/e820.c...
Checking patch arch/x86/pci/i386.c...
Checking patch include/asm-x86/e820.h...
Applied patch arch/x86/kernel/e820.c cleanly.
Applied patch arch/x86/pci/i386.c cleanly.
Applied patch include/asm-x86/e820.h cleanly.

$ git status
# Not currently on any branch.
# Changed but not updated:
#   (use "git add <file>..." to update what will be committed)
#
#    modified:   arch/x86/kernel/e820.c
#    modified:   arch/x86/pci/i386.c
#
no changes added to commit (use "git add" and/or "git commit -a")
==============================

I installed the kernel and it booted OK.  There was a delay when my
MTA (exim4) loaded, but that happens sometimes.  However, I got a
call trace I've never seen before.  This could just be a fluke, but
I'm passing it along in case it's meaningful.  I grabbed the trace on
TTY1 by logging in on TTY2 and using 'setterm -append 1':

===== SETTERM OUTPUT =====
Clocksource tsc unstable (delta = -98677795 ns)
NETDEV WATCHDOG: eth0 (r8169): transmit timed out
------------[ cut here ]------------
WARNING: at net/sched/sch_generic.c:221 dev_watchdog+0x21e/0x250()
Modules linked in: ipv6 cpufreq_userspace pcspkr 8250_pnp 8250 serial_core evdev
Pid: 0, comm: swapper Not tainted 2.6.27-rc4.080829.yh-patches-tip #1
Call Trace:
 <IRQ>  [<ffffffff8023b524>] warn_on_slowpath+0x64/0xb0
 [<ffffffff80584adc>] printk+0x4e/0x5a
 [<ffffffff80245ab4>] lock_timer_base+0x34/0x70
 [<ffffffff80245ca0>] __mod_timer+0xb0/0xd0
 [<ffffffff802565b8>] getnstimeofday+0x48/0xc0
 [<ffffffff8050f9be>] dev_watchdog+0x21e/0x250
 [<ffffffff802389bf>] scheduler_tick+0xcf/0x220
 [<ffffffff802451eb>] cascade+0x7b/0xa0
 [<ffffffff8050f7a0>] dev_watchdog+0x0/0x250
 [<ffffffff8024538c>] run_timer_softirq+0x13c/0x210
 [<ffffffff802548ac>] ktime_get+0xc/0x50
 [<ffffffff80240d13>] __do_softirq+0x73/0xf0
 [<ffffffff8020ca3c>] call_softirq+0x1c/0x30
 [<ffffffff8020eac5>] do_softirq+0x35/0x70
 [<ffffffff80240a0d>] irq_exit+0x8d/0x90
 [<ffffffff8021e9b6>] smp_apic_timer_interrupt+0x86/0xd0
 [<ffffffff8020c486>] apic_timer_interrupt+0x66/0x70
 <EOI>  [<ffffffff8021e130>] lapic_next_event+0x0/0x20
 [<ffffffff80212fca>] default_idle+0x3a/0x40
 [<ffffffff802130d4>] c1e_idle+0x34/0xe0
 [<ffffffff8020a8d6>] cpu_idle+0x56/0xa0
---[ end trace 28095e4c2b529c2a ]---
r8169: eth0: link up
===== END SETTERM OUTPUT =====

Other than this strangeness, everything seems OK with your patches.  This
looks like some random networking annoyance to me, not really related to
the regression patches, but I'll let better minds be the judge of that.

The HPET-related output in 'dmesg' looks just fine:

==============================
$ dmesg | grep -i -3 hpet
ACPI: DSDT 77FE3180, 4B0B (r1 RS690  AWRDACPI     1000 MSFT  3000000)
ACPI: FACS 77FE0000, 0040
ACPI: SSDT 77FE7DC0, 028A (r1 PTLTD  POWERNOW        1  LTP        1)
ACPI: HPET 77FE80C0, 0038 (r1 RS690  AWRDACPI 42302E31 AWRD       98)
ACPI: MCFG 77FE8140, 003C (r1 RS690  AWRDACPI 42302E31 AWRD        0)
ACPI: APIC 77FE7D00, 0068 (r1 RS690  AWRDACPI 42302E31 AWRD        0)
No NUMA configuration found
--
ACPI: IRQ0 used by override.
ACPI: IRQ2 used by override.
ACPI: IRQ9 used by override.
ACPI: HPET id: 0x10b9a201 base: 0xfed00000
Using ACPI (MADT) for SMP configuration information
Allocating PCI resources starting at 80000000 (gap: 78000000:68000000)
dyn_array 0xffffffff80840c68 size:0x10 nr:256 align:0x1000
--
Memory: 1929712k/1965952k available (3625k kernel code, 35852k reserved, 1446k data, 1116k init)
CPA: page pool initialized 1 of 1 pages preallocated
SLUB: Genslabs=13, HWalign=64, Order=0-3, MinObjects=0, CPUs=2, Nodes=1
hpet clockevent registered
Calibrating delay loop (skipped), value calculated using timer frequency.. 5001.73 BogoMIPS (lpj=8332380)
Dentry cache hash table entries: 262144 (order: 9, 2097152 bytes)
Inode-cache hash table entries: 131072 (order: 8, 1048576 bytes)
--
initcall pci_iommu_init+0x0/0x1e returned 0 after 0 msecs
calling  print_all_ICs+0x0/0x565
initcall print_all_ICs+0x0/0x565 returned 0 after 0 msecs
calling  hpet_late_init+0x0/0xed
hpet0: at MMIO 0xfed00000, IRQs 2, 8, 0, 0
hpet0: 4 comparators, 32-bit 14.318180 MHz counter
initcall hpet_late_init+0x0/0xed returned 0 after 0 msecs
calling  clocksource_done_booting+0x0/0xd
initcall clocksource_done_booting+0x0/0xd returned 0 after 0 msecs
calling  init_pipe_fs+0x0/0x44
--
initcall tty_init+0x0/0x1dc returned 0 after 21 msecs
calling  pty_init+0x0/0x2eb
initcall pty_init+0x0/0x2eb returned 0 after 0 msecs
calling  hpet_init+0x0/0x6b
hpet_resources: 0xfed00000 is busy
initcall hpet_init+0x0/0x6b returned 0 after 0 msecs
calling  agp_init+0x0/0x2d
Linux agpgart interface v0.103
initcall agp_init+0x0/0x2d returned 0 after 0 msecs
--
powernow-k8:    4 : fid 0xa (1800 MHz), vid 0x13
powernow-k8:    5 : fid 0x2 (1000 MHz), vid 0x16
initcall powernowk8_init+0x0/0x94 returned 0 after 14 msecs
calling  hpet_insert_resource+0x0/0x1e
initcall hpet_insert_resource+0x0/0x1e returned 0 after 0 msecs
calling  update_mp_table+0x0/0x68e
initcall update_mp_table+0x0/0x68e returned 0 after 0 msecs
calling  lapic_insert_resource+0x0/0x45
==============================


HTH,
Dave W.

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

[Francois Romieu]

David Witbrodt <dawitbro@sbcglobal.net> :
[...]
> ===== SETTERM OUTPUT =====
> Clocksource tsc unstable (delta = -98677795 ns)
> NETDEV WATCHDOG: eth0 (r8169): transmit timed out
> ------------[ cut here ]------------
> WARNING: at net/sched/sch_generic.c:221 dev_watchdog+0x21e/0x250()
> Modules linked in: ipv6 cpufreq_userspace pcspkr 8250_pnp 8250 serial_core evdev
> Pid: 0, comm: swapper Not tainted 2.6.27-rc4.080829.yh-patches-tip #1
> Call Trace:
[...]
>  [<ffffffff8020a8d6>] cpu_idle+0x56/0xa0
> ---[ end trace 28095e4c2b529c2a ]---
> r8169: eth0: link up
> ===== END SETTERM OUTPUT =====
> 
> Other than this strangeness, everything seems OK with your patches.  This
> looks like some random networking annoyance to me, not really related to
> the regression patches, but I'll let better minds be the judge of that.

Could you enable CONFIG_PRINTK_TIME ?

Even if the networking driver manages to negotiate the link, this is
the kind of message which upsets people (myself included :o) ).

-- 
Ueimor

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

[David Witbrodt]

> David Witbrodt :
> [...]
> > ===== SETTERM OUTPUT =====
> > Clocksource tsc unstable (delta = -98677795 ns)
> > NETDEV WATCHDOG: eth0 (r8169): transmit timed out
> > ------------[ cut here ]------------
> > WARNING: at net/sched/sch_generic.c:221 dev_watchdog+0x21e/0x250()
> > Modules linked in: ipv6 cpufreq_userspace pcspkr 8250_pnp 8250 serial_core 
> evdev
> > Pid: 0, comm: swapper Not tainted 2.6.27-rc4.080829.yh-patches-tip #1
> > Call Trace:
> [...]
> >  [] cpu_idle+0x56/0xa0
> > ---[ end trace 28095e4c2b529c2a ]---
> > r8169: eth0: link up
> > ===== END SETTERM OUTPUT =====
> > 
> > Other than this strangeness, everything seems OK with your patches.  This
> > looks like some random networking annoyance to me, not really related to
> > the regression patches, but I'll let better minds be the judge of that.
> 
> Could you enable CONFIG_PRINTK_TIME ?
> 
> Even if the networking driver manages to negotiate the link, this is
> the kind of message which upsets people (myself included :o) ).

Will do.

FYI, that kernel would not reproduce the call trace after several reboots.
I had never seen a trace like that before, and have not seen one since.


DW

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

[David Witbrodt]

> can you check tip/master now? Ingo fixed it arlready...

Yinghai,

Before updating my git tree, I copied some files (in their 
respective directories):

e820.c ==> e820.c.yh
i386.c ==> i386.c.yh
e820.h ==> e820.h.yh


Then I updated and checked out tip/master:

git remote update
git checkout -f tip/master


Running diffs on the updated files and the old files with your 
patches (i.e., 'diff e820.c{,.yh}') confirmed that the files
were the same.

However, for the sake of paranoia prevention, I went ahead and
built the kernel and rebooted:  it worked perfectly.


Thx,
Dave W.

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

[David Witbrodt]

> I installed the kernel and it booted OK.  There was a delay when my
> MTA (exim4) loaded, but that happens sometimes.  However, I got a
> call trace I've never seen before.  This could just be a fluke, but
> I'm passing it along in case it's meaningful.  I grabbed the trace on
> TTY1 by logging in on TTY2 and using 'setterm -append 1':
> 
> ===== SETTERM OUTPUT =====
> Clocksource tsc unstable (delta = -98677795 ns)
> NETDEV WATCHDOG: eth0 (r8169): transmit timed out
> ------------[ cut here ]------------
> WARNING: at net/sched/sch_generic.c:221 dev_watchdog+0x21e/0x250()
> Modules linked in: ipv6 cpufreq_userspace pcspkr 8250_pnp 8250 serial_core evdev
> Pid: 0, comm: swapper Not tainted 2.6.27-rc4.080829.yh-patches-tip #1
> Call Trace:
>   [] warn_on_slowpath+0x64/0xb0
> [] printk+0x4e/0x5a
> [] lock_timer_base+0x34/0x70
> [] __mod_timer+0xb0/0xd0
> [] getnstimeofday+0x48/0xc0
> [] dev_watchdog+0x21e/0x250
> [] scheduler_tick+0xcf/0x220
> [] cascade+0x7b/0xa0
> [] dev_watchdog+0x0/0x250
> [] run_timer_softirq+0x13c/0x210
> [] ktime_get+0xc/0x50
> [] __do_softirq+0x73/0xf0
> [] call_softirq+0x1c/0x30
> [] do_softirq+0x35/0x70
> [] irq_exit+0x8d/0x90
> [] smp_apic_timer_interrupt+0x86/0xd0
> [] apic_timer_interrupt+0x66/0x70
>   [] lapic_next_event+0x0/0x20
> [] default_idle+0x3a/0x40
> [] c1e_idle+0x34/0xe0
> [] cpu_idle+0x56/0xa0
> ---[ end trace 28095e4c2b529c2a ]---
> r8169: eth0: link up
> ===== END SETTERM OUTPUT =====
> 
> Other than this strangeness, everything seems OK with your patches.  This
> looks like some random networking annoyance to me, not really related to
> the regression patches, but I'll let better minds be the judge of that.


Update:  this strangeness cannot be reproduced with a couple of reboots, so
I'm thinking it was just bad luck that it happened while testing your patches.


Thanks Yinghai,
DW

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

[Yinghai Lu]

On Fri, Aug 29, 2008 at 7:48 AM, David Witbrodt <dawitbro@sbcglobal.net> wrote:
>
>
>
>
> Update:  this strangeness cannot be reproduced with a couple of reboots, so
> I'm thinking it was just bad luck that it happened while testing your patches.
>

can you check tip/master now? Ingo fixed it arlready...

YH

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

[David Witbrodt]

> From: Ingo Molnar <mingo@elte.hu>
> To: Yinghai Lu <yhlu.kernel@gmail.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>; David Witbrodt <dawitbro@sbcglobal.net>; Thomas Gleixner <tglx@linutronix.de>; H. Peter Anvin <hpa@zytor.com>; Andrew Morton <akpm@linux-foundation.org>; Jesse Barnes <jbarnes@virtuousgeek.org>; linux-kernel@vger.kernel.org
> Sent: Thursday, August 28, 2008 4:58:28 PM
> Subject: Re: [PATCH] x86: split e820 reserved entries record to late v2
> 
> 
> * Yinghai Lu wrote:
> 
> > On Thu, Aug 28, 2008 at 1:40 PM, Linus Torvalds
> > 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.
> 
> great - i've done the revert of a2bd7274b471 and have applied your patch 
> and pushed it out into -tip. David, could you please test whether 
> tip/master works for you out of box?


===== SHELL OUTPUT (for sanity) =====
commit e3fc96d5aca609bcf6ab0327850a109df65c1dbb
Merge: 6b8c836... a36d241...
Author: Ingo Molnar <mingo@elte.hu>
Date:   Thu Aug 28 22:57:20 2008 +0200

    Merge branch 'x86/core'
=====================================

Results:  both ECS AMD690GM-M2 machines boot fine...

- no need for "hpet=disable"
- no error messages in 'dmesg' (except that annoying TSC b.s.)


More good news:  the user who first reported the regression in May
(he uses Intel Q6600 CPU + Abit F-190HD mboard, while I use AMD 
Athlon 64 X2 3600+ CPU + ECS mboard) posted a followup on his blog:

===== http://ciaranm.wordpress.com/2008/05/18/yay-for-git/#comments =====
Oh good. Looks like git head works just fine now for me, so whatever
it was appears to be fixed.

Comment by Ciaran McCreesh — August 26, 2008 @ 6:25 pm 
=========================================================================

I was afraid that Yinghai's original patch (many days ago) which
worked for me was too AMD-specific, and would not work for this other
user experiencing the regression against commit 3def3d6d...

When you (Ingo) and Yinghai made it more generic afterwards, I
feared that it was too HPET-specific.

I tried to get Ciaran here, so that the problems he and I were
having could both be solved.  It's not clear what change solved his
regression -- clearly it's not today's patch, since his blog post
is dated 2 days ago! -- but I'm glad to see that all is well in
the world again.


Ingo, could you do me a favor?

Mr. Anvin's inbox got badly messed up because of me:

> From: H. Peter Anvin <hpa@zytor.com>
[...]
> Sent: Thursday, August 28, 2008 8:21:09 PM
[...]
> This may be a rehash of things previously discussed in this thread; my 
> email seems to be a bit flakey to the point that I don't know if I have 
> gotten all the messages.

Could you explain this to him for me?  I don't want to post any more than 
this one message, just making a bad situation worse.  (Sorry about the 
threading, Peter!)

Since Monday I have gone back to work on my LAN.  RAID was quickly set up
on the fileserver, but the backup of the P4 isn't finished so I _still_
haven't been able to pull the hard disk from it for the webserver.  Only
after that will I be able to get the MTA arrangement I want, and banish
this webmail client FOREVER!  (I should have done all of this while I was
helping you troubleshoot the regression, since my 2.6.25 kernels work fine,
but I was too excited about being able to help to think that clearly.)


Thanks all!
Dave Witbrodt

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

[Yinghai Lu]

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

On Thu, Aug 28, 2008 at 8:59 PM, David Witbrodt <dawitbro@sbcglobal.net> wrote:

>
> ===== SHELL OUTPUT (for sanity) =====
> commit e3fc96d5aca609bcf6ab0327850a109df65c1dbb
> Merge: 6b8c836... a36d241...
> Author: Ingo Molnar <mingo@elte.hu>
> Date:   Thu Aug 28 22:57:20 2008 +0200
>
>    Merge branch 'x86/core'
> =====================================
>
> Results:  both ECS AMD690GM-M2 machines boot fine...
>
> - no need for "hpet=disable"
> - no error messages in 'dmesg' (except that annoying TSC b.s.)
>

please hang a while, there is some merging problem with current tip/master.

one or two hours later ingo may fix it.

or please try attached two patches....

YH

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

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 523d6c5..291e6cd 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -1271,15 +1271,13 @@ static inline const char *e820_type_to_string(int e820_type)
 /*
  * Mark e820 reserved areas as busy for the resource manager.
  */
-struct resource __initdata *e820_res;
 void __init e820_reserve_resources(void)
 {
 	int i;
-	u64 end;
 	struct resource *res;
+	u64 end;
 
 	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
@@ -1293,8 +1291,7 @@ void __init e820_reserve_resources(void)
 		res->end = end;
 
 		res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
-		if (e820.map[i].type != E820_RESERVED || res->start < (1ULL<<20))
-			insert_resource(&iomem_resource, res);
+		insert_resource(&iomem_resource, res);
 		res++;
 	}
 
@@ -1306,19 +1303,6 @@ 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";
diff --git a/arch/x86/pci/i386.c b/arch/x86/pci/i386.c
index 718fd77..5807d1b 100644
--- a/arch/x86/pci/i386.c
+++ b/arch/x86/pci/i386.c
@@ -31,12 +31,8 @@
 #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 <asm/e820.h>
 
 #include "pci.h"
 
@@ -81,77 +77,6 @@ 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.
@@ -203,8 +128,6 @@ 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);
 					/*
@@ -248,8 +171,6 @@ 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 */
diff --git a/include/asm-x86/e820.h b/include/asm-x86/e820.h
index 5abbdec..ca433c3 100644
--- a/include/asm-x86/e820.h
+++ b/include/asm-x86/e820.h
@@ -122,7 +122,6 @@ extern void e820_register_active_regions(int nid, unsigned long start_pfn,
 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);

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

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

Linus said we should register some entries in e820 later,
so could let BAR res register at first, or even pnp?

this one replace
| 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

v2: insert e820 reserve resources before pnp_system_init
v3: fix merging problem in tip/x86/core
    please drop the one in tip/x86/core use this one instead
v4: address Linus's review about comments and condition in _late()

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

---
 arch/x86/kernel/e820.c |   24 +++++++++++++++++++++++-
 arch/x86/pci/i386.c    |    3 +++
 include/asm-x86/e820.h |    1 +
 3 files changed, 27 insertions(+), 1 deletion(-)

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,6 +1271,7 @@ static inline const char *e820_type_to_s
 /*
  * Mark e820 reserved areas as busy for the resource manager.
  */
+static struct resource __initdata *e820_res;
 void __init e820_reserve_resources(void)
 {
 	int i;
@@ -1278,6 +1279,7 @@ void __init e820_reserve_resources(void)
 	u64 end;
 
 	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,14 @@ void __init e820_reserve_resources(void)
 		res->end = end;
 
 		res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
-		insert_resource(&iomem_resource, res);
+
+		/*
+		 * don't register the region that could be conflicted with
+		 * pci device BAR resource and insert them later in
+		 * pcibios_resource_survey()
+		 */
+		if (e820.map[i].type != E820_RESERVED || res->start < (1ULL<<20))
+			insert_resource(&iomem_resource, res);
 		res++;
 	}
 
@@ -1303,6 +1312,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 (!res->parent && res->end)
+			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
@@ -33,6 +33,7 @@
 #include <linux/bootmem.h>
 
 #include <asm/pat.h>
+#include <asm/e820.h>
 
 #include "pci.h"
 
@@ -230,6 +231,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);

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

[Yinghai Lu]

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);

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

[Linus Torvalds]

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)?

		Linus

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

[Yinghai Lu]

[-- 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 */

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

[Ingo Molnar]

* Yinghai Lu <yhlu.kernel@gmail.com> wrote:

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

great - i've done the revert of a2bd7274b471 and have applied your patch 
and pushed it out into -tip. David, could you please test whether 
tip/master works for you out of box?

	Ingo

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

[Ingo Molnar]

* Yinghai Lu <yhlu.kernel@gmail.com> wrote:

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

Here we have the problem of overlap i outlined earlier: if there's a 
partial overlap at this stage (as i think it can happen in the hpet case 
on David's box), we wont insert the E820_RESERVED resource.

The hpet hang will be solved, because we dont reprogram the BAR, but we 
now keep the formerly e820-reserved area as 'free' - which the PCI code 
could allocate new resources into - which could cause other problems 
(hangs, non-working devices, etc.) down the line.

Which most likely wont happen currently in practice (there's enough free 
space elsewhere), but it's still a not truly 'free' area and it would be 
nice to have a complete and correct picture, based on all sources of 
information we have.

	Ingo

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

[H. Peter Anvin]

Ingo Molnar wrote:
> 
> Here we have the problem of overlap i outlined earlier: if there's a 
> partial overlap at this stage (as i think it can happen in the hpet case 
> on David's box), we wont insert the E820_RESERVED resource.
> 
> The hpet hang will be solved, because we dont reprogram the BAR, but we 
> now keep the formerly e820-reserved area as 'free' - which the PCI code 
> could allocate new resources into - which could cause other problems 
> (hangs, non-working devices, etc.) down the line.
> 
> Which most likely wont happen currently in practice (there's enough free 
> space elsewhere), but it's still a not truly 'free' area and it would be 
> nice to have a complete and correct picture, based on all sources of 
> information we have.
> 

This may be a rehash of things previously discussed in this thread; my 
email seems to be a bit flakey to the point that I don't know if I have 
gotten all the messages.

Either way, Ingo mentioned in a private messages four steps, basically 
summarizing the above email:

1 - first we allocate the absolute essentials (e820 RAM and a few low
     RAM specials)
2 - then we register all existing PCI resources - but do not reallocate
     any PCI resources that conflict with existing step #1 resources
3 - then we allocate e820 reserved entries (and whatever special non-PCI
     resources we might know about in general) - these are less trusted
     than any of the existing PCI resources but still it can hurt us
     badly if the PCI code allocates new resources on them.
4 - then the PCI code can run and allocate free resources to all the
     zero, not yet allocated BARs, and can reallocate any resources that
     might conflict with existing [step #1 or step #3] registered
     resources.

I agree that this is almost certainly what we should be doing; there is 
a difference between claiming resources already allocated and allocating 
resources to new address space, in which case we want to be as 
conservative as possible.

The key, of course, is that nothing goes in #1 unless we are bloody 
damned sure that if a BAR points there, that BAR is unconditionally 
broken and pointing into hyperspace.  Something claiming RAM or, say, 
the legacy KBC might fall in this area.

	-hpa