Re: [GIT PULL tip/x86/mm] xen/x86 fixes ===> fix sp5100_tco mmio checking.

On 03/16/2011 11:02 AM, Stefano Stabellini wrote:

On Wed, 16 Mar 2011, Yinghai Lu wrote:

quoted

On 03/16/2011 07:43 AM, Stefano Stabellini wrote:

quoted

actually attach the logs :)

On Wed, 16 Mar 2011, Stefano Stabellini wrote:

quoted

On Fri, 11 Mar 2011, Konrad Rzeszutek Wilk wrote:

quoted

On Fri, Mar 11, 2011 at 01:17:23PM +0000, Stefano Stabellini wrote:

quoted

Hello,
recently we had a couple of long discussions with Yinghai about boot
crashes on xen, related to pagetable initialization.
As a result we came up with three patches, two of them fix the first [1]
boot crash and provide a nice cleanup on native:

I don't know why this is happening now, but it could be very well
related to the build config. Smaller builds don't seem to encounter this, while
this is a distro type build. If I use:

quoted

Stefano Stabellini (1):
        xen: set max_pfn_mapped to the last pfn mapped

it hangs during bootup. The machine hangs during the box (no keyboard interaction)
and I can see this in the bootup.

Konrad sent me few other logs offline: log1 is the log of the hang and
log2 is a successful boot (reverting the problematic patch).
It looks like the SP5100 TCO WatchDog Timer Driver is using ioremap on
an address (0xb8fe00) that belongs to the memory range used for the
pagetable (0x9fc000-0xf43fff).

Mar 15 16:09:04 phenom kernel: [    0.000000] found SMP MP-table at [ffff8800000ff780] ff780

Mar 15 16:09:04 phenom kernel: [    0.000000]     memblock_x86_reserve_range: [0x000ff780-0x000ff78f]   * MP-table mpf

Mar 15 16:09:04 phenom kernel: [    0.000000]     memblock_x86_reserve_range: [0x000fd240-0x000fd423]   * MP-table mpc

Mar 15 16:09:04 phenom kernel: [    0.000000]     memblock_x86_reserve_range: [0x01cfd000-0x01d1c0e4]              BRK

Mar 15 16:09:04 phenom kernel: [    0.000000] MEMBLOCK configuration:

Mar 15 16:09:04 phenom kernel: [    0.000000]  memory size = 0x23fe39000

Mar 15 16:09:04 phenom kernel: [    0.000000]  memory.cnt  = 0x3

Mar 15 16:09:04 phenom kernel: [    0.000000]  memory[0x0]      [0x00000000010000-0x0000000009afff], 0x8b000 bytes

Mar 15 16:09:04 phenom kernel: [    0.000000]  memory[0x1]      [0x00000000100000-0x000000bffaffff], 0xbfeb0000 bytes

Mar 15 16:09:04 phenom kernel: [    0.000000]  memory[0x2]      [0x00000100000000-0x0000027fefdfff], 0x17fefe000 bytes

Mar 15 16:09:04 phenom kernel: [    0.000000]  reserved.cnt  = 0x5

Mar 15 16:09:04 phenom kernel: [    0.000000]  reserved[0x0]    [0x000000000fd240-0x000000000fd423], 0x1e4 bytes

Mar 15 16:09:04 phenom kernel: [    0.000000]  reserved[0x1]    [0x000000000ff780-0x000000000ff78f], 0x10 bytes

Mar 15 16:09:04 phenom kernel: [    0.000000]  reserved[0x2]    [0x00000001000000-0x00000001d1c0e4], 0xd1c0e5 bytes

Mar 15 16:09:04 phenom kernel: [    0.000000]  reserved[0x3]    [0x00000001e33000-0x00000016a36fff], 0x14c04000 bytes

Mar 15 16:09:04 phenom kernel: [    0.000000]  reserved[0x4]    [0x000001f0f7e000-0x0000027fefdfff], 0x8ef80000 bytes

Mar 15 16:09:04 phenom kernel: [    0.000000] Scanning 0 areas for low memory corruption

Mar 15 16:09:04 phenom kernel: [    0.000000]     memblock_x86_reserve_range: [0x00099000-0x0009afff]       TRAMPOLINE

Mar 15 16:09:04 phenom kernel: [    0.000000]     memblock_x86_reserve_range: [0x00095000-0x00098fff]      ACPI WAKEUP

Mar 15 16:09:04 phenom kernel: [    0.000000] init_memory_mapping: 0000000000000000-00000000bffb0000

Mar 15 16:09:04 phenom kernel: [    0.000000] DEBUG find_early_table_space: _text=1000000 _end=1e33000 pgtable_start=9fc000 pgtable_end=9fc000

Mar 15 16:09:04 phenom kernel: [    0.000000]     memblock_x86_reserve_range: [0x009fc000-0x00f43fff]          PGTABLE

e820 said that range is ram and usable. so it is right for memblock to use it.

why TCO watchdog try to use ioremap with RAM?  BIOS put wrong mmio in that BAR?

could do some sanitary check in that driver.

Yeah, I think the max_pfn_mapped patch might be exposing bugs in the
drivers.
Do you remember this patch:

https://lkml.org/lkml/2011/2/4/60

would you be happy with it as a safer alternative?

we should fix tco driver

Mar 15 16:09:04 phenom kernel: [    9.148536] SP5100 TCO timer: SP5100 TCO WatchDog Timer Driver v0.01

Mar 15 16:09:04 phenom kernel: [    9.148628] DEBUG __ioremap_caller WARNING address=b8fe00 size=8 valid=1 reserved=1

so BIOS program wrong MMIO info.

need some checking in that driver like

diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index 8083728..2fac643 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c

@@ -42,6 +42,7 @@
 #define PFX TCO_MODULE_NAME ": "
 
 /* internal variables */
+static u32 tcobase_phys;
 static void __iomem *tcobase;
 static unsigned int pm_iobase;
 static DEFINE_SPINLOCK(tco_lock);	/* Guards the hardware */

@@ -305,6 +306,12 @@ static unsigned char __devinit sp5100_tco_setupdevice(void)
 	/* Low three bits of BASE0 are reserved. */
 	val = val << 8 | (inb(SP5100_IO_PM_DATA_REG) & 0xf8);
 
+	if (request_mem_region_exclusive(val, SP5100_WDT_MEM_MAP_SIZE, "SP5100 TCO")) {
+		printk(KERN_ERR PFX "mmio address 0x%04x already in use\n", val);
+		goto unreg_region;
+	}
+	tcobase_phys = val;
+
 	tcobase = ioremap(val, SP5100_WDT_MEM_MAP_SIZE);
 	if (tcobase == 0) {
 		printk(KERN_ERR PFX "failed to get tcobase address\n");

@@ -414,6 +421,7 @@ static void __devexit sp5100_tco_cleanup(void)
 	/* Deregister */
 	misc_deregister(&sp5100_tco_miscdev);
 	iounmap(tcobase);
+	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
 	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
 }
 

On Wed, Mar 16, 2011 at 1:45 PM, Yinghai Lu [off-list ref] wrote:

Needs a release_mem_region() in this path.  Otherwise this looks fine.

Stefano found SP5100 TCO watchdog driver using wrong address.

[    9.148536] SP5100 TCO timer: SP5100 TCO WatchDog Timer Driver v0.01
[    9.148628] DEBUG __ioremap_caller WARNING address=b8fe00 size=8 valid=1 reserved=1

and e820 said that range is RAM.

We should check if we can use that reading out. BIOS could just program wrong address there.

-v2: Mike pointed out one path need one release.

Reported-by: Stefano Stabellini <redacted>
Signed-off-by:Yinghai Lu [off-list ref]
Acked-by: Mike Waychison <redacted>

---
 drivers/watchdog/sp5100_tco.c |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Index: linux-2.6/drivers/watchdog/sp5100_tco.c
===================================================================

--- linux-2.6.orig/drivers/watchdog/sp5100_tco.c
+++ linux-2.6/drivers/watchdog/sp5100_tco.c

@@ -42,6 +42,7 @@
 #define PFX TCO_MODULE_NAME ": "
 
 /* internal variables */
+static u32 tcobase_phys;
 static void __iomem *tcobase;
 static unsigned int pm_iobase;
 static DEFINE_SPINLOCK(tco_lock);	/* Guards the hardware */

@@ -305,10 +306,16 @@ static unsigned char __devinit sp5100_tc
 	/* Low three bits of BASE0 are reserved. */
 	val = val << 8 | (inb(SP5100_IO_PM_DATA_REG) & 0xf8);
 
+	if (request_mem_region_exclusive(val, SP5100_WDT_MEM_MAP_SIZE, "SP5100 TCO")) {
+		printk(KERN_ERR PFX "mmio address 0x%04x already in use\n", val);
+		goto unreg_region;
+	}
+	tcobase_phys = val;
+
 	tcobase = ioremap(val, SP5100_WDT_MEM_MAP_SIZE);
 	if (tcobase == 0) {
 		printk(KERN_ERR PFX "failed to get tcobase address\n");
-		goto unreg_region;
+		goto unreg_mem_region;
 	}
 
 	/* Enable watchdog decode bit */

@@ -346,7 +353,8 @@ static unsigned char __devinit sp5100_tc
 	/* Done */
 	return 1;
 
-	iounmap(tcobase);
+unreg_mem_region:
+	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
 unreg_region:
 	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
 exit:

@@ -401,6 +409,7 @@ static int __devinit sp5100_tco_init(str
 
 exit:
 	iounmap(tcobase);
+	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
 	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
 	return ret;
 }

@@ -414,6 +423,7 @@ static void __devexit sp5100_tco_cleanup
 	/* Deregister */
 	misc_deregister(&sp5100_tco_miscdev);
 	iounmap(tcobase);
+	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
 	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
 }
 

On Wed, Mar 16, 2011 at 02:18:17PM -0700, Yinghai Lu wrote:

Stefano found SP5100 TCO watchdog driver using wrong address.

[    9.148536] SP5100 TCO timer: SP5100 TCO WatchDog Timer Driver v0.01
[    9.148628] DEBUG __ioremap_caller WARNING address=b8fe00 size=8 valid=1 reserved=1

and e820 said that range is RAM.

We should check if we can use that reading out. BIOS could just program wrong address there.

-v2: Mike pointed out one path need one release.

Reported-by: Stefano Stabellini <redacted>
Signed-off-by:Yinghai Lu [off-list ref]
Acked-by: Mike Waychison <redacted>

Tested-by: Konrad Rzeszutek Wilk <redacted>

Stefano,

this fixes my bootup issues with your:
xen: set max_pfn_mapped to the last pfn mapped
patch. Will try the full patchset tomorrow.