Re: BIOS overwritten during resume (was: Re: Asus L5D resume on battery power)

Re: BIOS overwritten during resume (was: Re: Asus L5D resume on battery power)

[Pavel Machek]

Hi!

> > > It sounds to me like we run at 2GHz from batteries at resume time, and
> > > that causes bad things (tm),
> [-- snip --]
> 
> It seems that we write to the BIOS while moving the image, at least on my box,
> which is quite not correct, IMO.
...
> At the same time, from powernow-k8, I got this:
> 
> powernow-k8: Found 1 AMD Athlon 64 / Opteron processors (version 1.00.09e)
> powernow-k8: found PSB header at 0xffff8100000fbb10
> 
> where ffff8100000fbb10 is the (virtual) address containing the PSB header
> (ie a part of the BIOS).  Hence, the PSB gets overwritten during resume (as
> well as some other BIOS stuff, it seems).
> 
> IMO this may lead to unexpected results, like the mysterious reboots during
> resume.

Well, I always thought that ROM-BIOS is expected to
be... well... read-only? Can you really write to your BIOS? [I know
about Flash-BIOSen, but they are certainly not writable by "normal"
write.] Plus we should overwrite it with same values...

Anyway, IMO bios should be marked as reserved (and we should not be
touching reserved pages). Can you verify that your BIOS is properly
marked reserved? [Ccing l-k, this might be interesting.]
								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: BIOS overwritten during resume (was: Re: Asus L5D resume on battery power)

[Rafael J. Wysocki]

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

Hi,

On Wednesday, 2 of March 2005 23:05, Pavel Machek wrote:
> Hi!
> 
[-- snip --] 
> > It seems that we write to the BIOS while moving the image, at least on my box,
> > which is quite not correct, IMO.
[-- snip --]
> > 
> > IMO this may lead to unexpected results, like the mysterious reboots during
> > resume.
> 
> Well, I always thought that ROM-BIOS is expected to
> be... well... read-only? Can you really write to your BIOS? [I know
> about Flash-BIOSen, but they are certainly not writable by "normal"
> write.] Plus we should overwrite it with same values...
> 
> Anyway, IMO bios should be marked as reserved (and we should not be
> touching reserved pages). Can you verify that your BIOS is properly
> marked reserved? [Ccing l-k, this might be interesting.]

It is, but that's even more interesting.  Namely, the BIOS-e820 memory map
looks as follows:

BIOS-provided physical RAM map:
 BIOS-e820: 0000000000000000 - 000000000009fc00 (usable)
 BIOS-e820: 000000000009fc00 - 00000000000a0000 (reserved)
 BIOS-e820: 00000000000e0000 - 0000000000100000 (reserved)
 BIOS-e820: 0000000000100000 - 000000001ff40000 (usable)
 BIOS-e820: 000000001ff40000 - 000000001ff50000 (ACPI data)
 BIOS-e820: 000000001ff50000 - 0000000020000000 (ACPI NVS)
 BIOS-e820: 00000000fec00000 - 00000000fec01000 (reserved)
 BIOS-e820: 00000000fee00000 - 00000000fee01000 (reserved)

and the pages in the first two reserved areas are properly marked as reserved.
Moreover, there is an apparent hole between a0000 and e0000 which also
is marked as reserved.  However, we treat all of these pages as saveable
(a page is treated as non-saveable if it is marked as reserved _and_ satisfies
specific condition which is met by only one page on my system).  Consequently,
during resume we try to overwrite all of these pages and we not only try to
write to (theoretically) read-only areas, but also we try to write to where there's
nothing. :-)

Still, there are almost 3500 reserved pages on my system and I'm not sure that
we don't need to save at least some of them.  Anyway, I'm attaching the full
list of pages that are marked as reserved and treated as saveable, so they get
overwritten during resume.  Please have a look (these are virtual x86-64
addresses, but the mapping is straightforward).

Greets,
Rafael


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

[-- Attachment #2: reserved_pages.log.gz --]
[-- Type: application/x-gzip, Size: 10033 bytes --]

Re: BIOS overwritten during resume (was: Re: Asus L5D resume on battery power)

[Pavel Machek]

Hi!

> > > It seems that we write to the BIOS while moving the image, at least on my box,
> > > which is quite not correct, IMO.
> [-- snip --]
> > > 
> > > IMO this may lead to unexpected results, like the mysterious reboots during
> > > resume.
> > 
> > Well, I always thought that ROM-BIOS is expected to
> > be... well... read-only? Can you really write to your BIOS? [I know
> > about Flash-BIOSen, but they are certainly not writable by "normal"
> > write.] Plus we should overwrite it with same values...
> > 
> > Anyway, IMO bios should be marked as reserved (and we should not be
> > touching reserved pages). Can you verify that your BIOS is properly
> > marked reserved? [Ccing l-k, this might be interesting.]
> 
> It is, but that's even more interesting.  Namely, the BIOS-e820 memory map
> looks as follows:
> 
> BIOS-provided physical RAM map:
>  BIOS-e820: 0000000000000000 - 000000000009fc00 (usable)
>  BIOS-e820: 000000000009fc00 - 00000000000a0000 (reserved)
>  BIOS-e820: 00000000000e0000 - 0000000000100000 (reserved)
>  BIOS-e820: 0000000000100000 - 000000001ff40000 (usable)
>  BIOS-e820: 000000001ff40000 - 000000001ff50000 (ACPI data)
>  BIOS-e820: 000000001ff50000 - 0000000020000000 (ACPI NVS)
>  BIOS-e820: 00000000fec00000 - 00000000fec01000 (reserved)
>  BIOS-e820: 00000000fee00000 - 00000000fee01000 (reserved)
> 
> and the pages in the first two reserved areas are properly marked as reserved.
> Moreover, there is an apparent hole between a0000 and e0000 which also
> is marked as reserved.  However, we treat all of these pages as saveable
> (a page is treated as non-saveable if it is marked as reserved _and_ satisfies
> specific condition which is met by only one page on my system).  Consequently,
> during resume we try to overwrite all of these pages and we not only try to
> write to (theoretically) read-only areas, but also we try to write to where there's
> nothing. :-)
> 
> Still, there are almost 3500 reserved pages on my system and I'm not sure that
> we don't need to save at least some of them.  Anyway, I'm attaching the full
> list of pages that are marked as reserved and treated as saveable, so they get
> overwritten during resume.  Please have a look (these are virtual x86-64
> addresses, but the mapping is straightforward).

[looking at code]

IIRC kernel code/data is marked as PageReserved(), that's why we need
to save that :(. Not sure what to do with data e820 marked as
reserved...

								Pavel

-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: BIOS overwritten during resume (was: Re: Asus L5D resume on battery power)

[Rafael J. Wysocki]

Hi,

On Thursday, 3 of March 2005 00:54, Pavel Machek wrote:
> Hi!
> 
> > > > It seems that we write to the BIOS while moving the image, at least on my box,
> > > > which is quite not correct, IMO.
> > [-- snip --]
> > > > 
> > > > IMO this may lead to unexpected results, like the mysterious reboots during
> > > > resume.
> > > 
> > > Well, I always thought that ROM-BIOS is expected to
> > > be... well... read-only? Can you really write to your BIOS? [I know
> > > about Flash-BIOSen, but they are certainly not writable by "normal"
> > > write.] Plus we should overwrite it with same values...
> > > 
> > > Anyway, IMO bios should be marked as reserved (and we should not be
> > > touching reserved pages). Can you verify that your BIOS is properly
> > > marked reserved? [Ccing l-k, this might be interesting.]
> > 
> > It is, but that's even more interesting.  Namely, the BIOS-e820 memory map
> > looks as follows:
> > 
> > BIOS-provided physical RAM map:
> >  BIOS-e820: 0000000000000000 - 000000000009fc00 (usable)
> >  BIOS-e820: 000000000009fc00 - 00000000000a0000 (reserved)
> >  BIOS-e820: 00000000000e0000 - 0000000000100000 (reserved)
> >  BIOS-e820: 0000000000100000 - 000000001ff40000 (usable)
> >  BIOS-e820: 000000001ff40000 - 000000001ff50000 (ACPI data)
> >  BIOS-e820: 000000001ff50000 - 0000000020000000 (ACPI NVS)
> >  BIOS-e820: 00000000fec00000 - 00000000fec01000 (reserved)
> >  BIOS-e820: 00000000fee00000 - 00000000fee01000 (reserved)
> > 
> > and the pages in the first two reserved areas are properly marked as reserved.
> > Moreover, there is an apparent hole between a0000 and e0000 which also
> > is marked as reserved.  However, we treat all of these pages as saveable
> > (a page is treated as non-saveable if it is marked as reserved _and_ satisfies
> > specific condition which is met by only one page on my system).  Consequently,
> > during resume we try to overwrite all of these pages and we not only try to
> > write to (theoretically) read-only areas, but also we try to write to where there's
> > nothing. :-)
> > 
> > Still, there are almost 3500 reserved pages on my system and I'm not sure that
> > we don't need to save at least some of them.  Anyway, I'm attaching the full
> > list of pages that are marked as reserved and treated as saveable, so they get
> > overwritten during resume.  Please have a look (these are virtual x86-64
> > addresses, but the mapping is straightforward).
> 
> [looking at code]
> 
> IIRC kernel code/data is marked as PageReserved(), that's why we need
> to save that :(. Not sure what to do with data e820 marked as
> reserved...

Perhaps we need another page flag, like PG_readonly, and mark the pages
reserved by the e820 as PG_reserved | PG_readonly (the same for the areas
that are not returned by e820 at all).  Would that be acceptable?

Greets,
Rafael


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: BIOS overwritten during resume (was: Re: Asus L5D resume on battery power)

[Pavel Machek]

Hi!

> > IIRC kernel code/data is marked as PageReserved(), that's why we need
> > to save that :(. Not sure what to do with data e820 marked as
> > reserved...
> 
> Perhaps we need another page flag, like PG_readonly, and mark the pages
> reserved by the e820 as PG_reserved | PG_readonly (the same for the areas
> that are not returned by e820 at all).  Would that be acceptable?

This flags are little in the short supply, but being able to tell
kernel code from memory hole seems like "must have", so yes, that
looks ok.

You could get subtle and reuse some other pageflag. I do not think
PG_reserved can have PG_locked... So using for example PG_locked for
this purpose should be okay.
								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: BIOS overwritten during resume (was: Re: Asus L5D resume on battery power)

[Rafael J. Wysocki]

Hi,

On Friday, 4 of March 2005 12:04, Pavel Machek wrote:
> Hi!
> 
> > > IIRC kernel code/data is marked as PageReserved(), that's why we need
> > > to save that :(. Not sure what to do with data e820 marked as
> > > reserved...
> > 
> > Perhaps we need another page flag, like PG_readonly, and mark the pages
> > reserved by the e820 as PG_reserved | PG_readonly (the same for the areas
> > that are not returned by e820 at all).  Would that be acceptable?
> 
> This flags are little in the short supply, but being able to tell
> kernel code from memory hole seems like "must have", so yes, that
> looks ok.
> 
> You could get subtle and reuse some other pageflag. I do not think
> PG_reserved can have PG_locked... So using for example PG_locked for
> this purpose should be okay.

The following patch does this.  It is only for x86-64 without
CONFIG_DISCONTIGMEM, but it has no effect in other cases.


--- linux-2.6.11-rc5-mm1/arch/x86_64/mm/init.c	2005-03-04 12:19:29.000000000 +0100
+++ new/arch/x86_64/mm/init.c	2005-03-04 13:53:08.000000000 +0100
@@ -438,11 +438,14 @@
 	totalram_pages += free_all_bootmem();
 
 	for (tmp = 0; tmp < end_pfn; tmp++)
-		/*
-		 * Only count reserved RAM pages
-		 */
-		if (page_is_ram(tmp) && PageReserved(pfn_to_page(tmp)))
-			reservedpages++;
+		if (!page_is_ram(tmp))
+			SetPageLocked(pfn_to_page(tmp));
+		else
+			/*
+			 * Count reserved RAM pages
+			 */
+			if (PageReserved(pfn_to_page(tmp)))
+				reservedpages++;
 #endif
 
 	after_bootmem = 1;
--- linux-2.6.11-rc5-mm1/kernel/power/swsusp.c	2005-03-04 13:54:50.000000000 +0100
+++ new/kernel/power/swsusp.c	2005-03-04 13:57:37.000000000 +0100
@@ -531,9 +531,15 @@
 	BUG_ON(PageReserved(page) && PageNosave(page));
 	if (PageNosave(page))
 		return 0;
-	if (PageReserved(page) && pfn_is_nosave(pfn)) {
-		pr_debug("[nosave pfn 0x%lx]", pfn);
-		return 0;
+	if (PageReserved(page)) {
+		if (pfn_is_nosave(pfn)) {
+			pr_debug("[nosave pfn 0x%lx]\n", pfn);
+			return 0;
+		}
+		if (PageLocked(page)) {
+			pr_debug("[locked pfn 0x%lx]\n", pfn);
+			return 0;
+		}
 	}
 	if (PageNosaveFree(page))
 		return 0;


I thought it would be more complicated. :-)

Greets,
Rafael


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: BIOS overwritten during resume (was: Re: Asus L5D resume on battery power)

[Nigel Cunningham]

Hi.

On Fri, 2005-03-04 at 22:04, Pavel Machek wrote:
> Hi!
> 
> > > IIRC kernel code/data is marked as PageReserved(), that's why we need
> > > to save that :(. Not sure what to do with data e820 marked as
> > > reserved...
> > 
> > Perhaps we need another page flag, like PG_readonly, and mark the pages
> > reserved by the e820 as PG_reserved | PG_readonly (the same for the areas
> > that are not returned by e820 at all).  Would that be acceptable?
> 
> This flags are little in the short supply, but being able to tell
> kernel code from memory hole seems like "must have", so yes, that
> looks ok.
> 
> You could get subtle and reuse some other pageflag. I do not think
> PG_reserved can have PG_locked... So using for example PG_locked for
> this purpose should be okay.

Will something like this patch help?

Regards,

Nigel

diff -ruNp 208-e820-table-support-old/arch/i386/mm/init.c 208-e820-table-support-new/arch/i386/mm/init.c
--- 208-e820-table-support-old/arch/i386/mm/init.c	2005-01-12 17:06:58.481035848 +1100
+++ 208-e820-table-support-new/arch/i386/mm/init.c	2005-01-12 17:22:55.414559840 +1100
@@ -27,6 +27,7 @@
 #include <linux/slab.h>
 #include <linux/proc_fs.h>
 #include <linux/efi.h>
+#include <linux/suspend.h>
 
 #include <asm/processor.h>
 #include <asm/system.h>
@@ -272,12 +273,15 @@ void __init one_highpage_init(struct pag
 {
 	if (page_is_ram(pfn) && !(bad_ppro && page_kills_ppro(pfn))) {
 		ClearPageReserved(page);
+		ClearPageNosave(page);
 		set_bit(PG_highmem, &page->flags);
 		set_page_count(page, 1);
 		__free_page(page);
 		totalhigh_pages++;
-	} else
+	} else {
 		SetPageReserved(page);
+		SetPageNosave(page);
+	}
 }
 
 #ifndef CONFIG_DISCONTIGMEM
@@ -355,7 +359,7 @@ static void __init pagetable_init (void)
 #endif
 }
 
-#if defined(CONFIG_PM_DISK) || defined(CONFIG_SOFTWARE_SUSPEND)
+#ifdef CONFIG_PM
 /*
  * Swap suspend & friends need this for resume because things like the intel-agp
  * driver might have split up a kernel 4MB mapping.
@@ -571,6 +575,7 @@ void __init mem_init(void)
 	int codesize, reservedpages, datasize, initsize;
 	int tmp;
 	int bad_ppro;
+	void * addr;
 
 #ifndef CONFIG_DISCONTIGMEM
 	if (!mem_map)
@@ -601,12 +606,25 @@ void __init mem_init(void)
 	totalram_pages += __free_all_bootmem();
 
 	reservedpages = 0;
-	for (tmp = 0; tmp < max_low_pfn; tmp++)
-		/*
-		 * Only count reserved RAM pages
-		 */
-		if (page_is_ram(tmp) && PageReserved(pfn_to_page(tmp)))
-			reservedpages++;
+	addr = __va(0);
+	for (tmp = 0; tmp < max_low_pfn; tmp++, addr += PAGE_SIZE) {
+		if (page_is_ram(tmp)) {
+			/*
+			 * Only count reserved RAM pages
+			 */
+			if (PageReserved(mem_map+tmp))
+				reservedpages++;
+			/*
+			 * Mark nosave pages
+			 */
+			if (addr >= (void *)&__nosave_begin && addr < (void *)&__nosave_end)
+				SetPageNosave(mem_map+tmp);
+		} else
+			/*
+			 * Non-RAM pages are always nosave
+			 */
+			SetPageNosave(mem_map+tmp);
+	}
 
 	set_highmem_pages_init(bad_ppro);
 
@@ -705,6 +723,7 @@ void free_initmem(void)
 	addr = (unsigned long)(&__init_begin);
 	for (; addr < (unsigned long)(&__init_end); addr += PAGE_SIZE) {
 		ClearPageReserved(virt_to_page(addr));
+		ClearPageNosave(virt_to_page(addr));
 		set_page_count(virt_to_page(addr), 1);
 		memset((void *)addr, 0xcc, PAGE_SIZE);
 		free_page(addr);
@@ -720,6 +739,7 @@ void free_initrd_mem(unsigned long start
 		printk (KERN_INFO "Freeing initrd memory: %ldk freed\n", (end - start) >> 10);
 	for (; start < end; start += PAGE_SIZE) {
 		ClearPageReserved(virt_to_page(start));
+		ClearPageNosave(virt_to_page(start));
 		set_page_count(virt_to_page(start), 1);
 		free_page(start);
 		totalram_pages++;
diff -ruNp 208-e820-table-support-old/mm/bootmem.c 208-e820-table-support-new/mm/bootmem.c
--- 208-e820-table-support-old/mm/bootmem.c	2005-01-12 17:07:15.902387400 +1100
+++ 208-e820-table-support-new/mm/bootmem.c	2005-01-12 17:23:44.087160480 +1100
@@ -280,12 +280,14 @@ static unsigned long __init free_all_boo
 
 			count += BITS_PER_LONG;
 			__ClearPageReserved(page);
+			ClearPageNosave(page);
 			order = ffs(BITS_PER_LONG) - 1;
 			set_page_refs(page, order);
 			for (j = 1; j < BITS_PER_LONG; j++) {
 				if (j + 16 < BITS_PER_LONG)
 					prefetchw(page + j + 16);
 				__ClearPageReserved(page + j);
+				ClearPageNosave(page + j);
 			}
 			__free_pages(page, order);
 			i += BITS_PER_LONG;
@@ -296,6 +298,7 @@ static unsigned long __init free_all_boo
 				if (v & m) {
 					count++;
 					__ClearPageReserved(page);
+					ClearPageNosave(page);
 					set_page_refs(page, 0);
 					__free_page(page);
 				}
@@ -316,6 +319,7 @@ static unsigned long __init free_all_boo
 	for (i = 0; i < ((bdata->node_low_pfn-(bdata->node_boot_start >> PAGE_SHIFT))/8 + PAGE_SIZE-1)/PAGE_SIZE; i++,page++) {
 		count++;
 		__ClearPageReserved(page);
+		ClearPageNosave(page);
 		set_page_count(page, 1);
 		__free_page(page);
 	}

-- 
Nigel Cunningham
Software Engineer
Cyclades Corporation

http://cyclades.com

Re: BIOS overwritten during resume (was: Re: Asus L5D resume on battery power)

[Nigel Cunningham]

Hi.

On Sat, 2005-03-05 at 00:15, Rafael J. Wysocki wrote:
> Hi,
> 
> On Friday, 4 of March 2005 12:04, Pavel Machek wrote:
> > Hi!
> > 
> > > > IIRC kernel code/data is marked as PageReserved(), that's why we need
> > > > to save that :(. Not sure what to do with data e820 marked as
> > > > reserved...
> > > 
> > > Perhaps we need another page flag, like PG_readonly, and mark the pages
> > > reserved by the e820 as PG_reserved | PG_readonly (the same for the areas
> > > that are not returned by e820 at all).  Would that be acceptable?
> > 
> > This flags are little in the short supply, but being able to tell
> > kernel code from memory hole seems like "must have", so yes, that
> > looks ok.
> > 
> > You could get subtle and reuse some other pageflag. I do not think
> > PG_reserved can have PG_locked... So using for example PG_locked for
> > this purpose should be okay.
> 
> The following patch does this.  It is only for x86-64 without
> CONFIG_DISCONTIGMEM, but it has no effect in other cases.

Oops! That's what you get for replying to earlier messages before you
read later ones! The patch I posted has been in use for quite a while,
so it might be helpful to compare anyway.

Regards,

Nigel 

-- 
Nigel Cunningham
Software Engineer, Canberra, Australia
http://www.cyclades.com
Bus: +61 (2) 6291 9554; Hme: +61 (2) 6292 8028;  Mob: +61 (417) 100 574

Maintainer of Suspend2 Kernel Patches http://softwaresuspend.berlios.de

Re: BIOS overwritten during resume (was: Re: Asus L5D resume on battery power)

[Pavel Machek]

Hi!

> > > > IIRC kernel code/data is marked as PageReserved(), that's why we need
> > > > to save that :(. Not sure what to do with data e820 marked as
> > > > reserved...
> > > 
> > > Perhaps we need another page flag, like PG_readonly, and mark the pages
> > > reserved by the e820 as PG_reserved | PG_readonly (the same for the areas
> > > that are not returned by e820 at all).  Would that be acceptable?
> > 
> > This flags are little in the short supply, but being able to tell
> > kernel code from memory hole seems like "must have", so yes, that
> > looks ok.
> > 
> > You could get subtle and reuse some other pageflag. I do not think
> > PG_reserved can have PG_locked... So using for example PG_locked for
> > this purpose should be okay.
> 
> The following patch does this.  It is only for x86-64 without
> CONFIG_DISCONTIGMEM, but it has no effect in other cases.

Actually, take a look at Nigel's patch. He simply uses PageNosave
instead of PageLocked -- that is cleaner. He also found a few places
where reserved page becomes un-reserved, and you probably need to fix
those, too.
								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: BIOS overwritten during resume (was: Re: Asus L5D resume on battery power)

[Rafael J. Wysocki]

Hi,

On Friday, 4 of March 2005 21:11, Pavel Machek wrote:
> Hi!
> 
> > > > > IIRC kernel code/data is marked as PageReserved(), that's why we need
> > > > > to save that :(. Not sure what to do with data e820 marked as
> > > > > reserved...
> > > > 
> > > > Perhaps we need another page flag, like PG_readonly, and mark the pages
> > > > reserved by the e820 as PG_reserved | PG_readonly (the same for the areas
> > > > that are not returned by e820 at all).  Would that be acceptable?
> > > 
> > > This flags are little in the short supply, but being able to tell
> > > kernel code from memory hole seems like "must have", so yes, that
> > > looks ok.
> > > 
> > > You could get subtle and reuse some other pageflag. I do not think
> > > PG_reserved can have PG_locked... So using for example PG_locked for
> > > this purpose should be okay.
> > 
> > The following patch does this.  It is only for x86-64 without
> > CONFIG_DISCONTIGMEM, but it has no effect in other cases.
> 
> Actually, take a look at Nigel's patch. He simply uses PageNosave
> instead of PageLocked -- that is cleaner.

Yes.  I thought about using PG_nosave in the begining, but there's a

BUG_ON(PageReserved(page) && PageNosave(page));

in swsusp.c:saveable() that I just didn't want to trigger.  It seems to me,
though, that we don't need it any more, do we?

> He also found a few places where reserved page becomes un-reserved,
> and you probably need to fix those, too.

Yes, I think I'll just port the Nigel's patch to x86-64.  BTW, it's striking
that we found similar solutions independently (I didn't know the Nigel's
patch before :-)).

Unfortunately, it turns out that the patch does not fix my problem with random
reboots during resume on battery power, but I really think that we need to mark
non-RAM areas with PG_nosave, at least for sanity reasons (eg to be sure that
we do not break things by dumping stuff to where we should not write to).

Greets,
Rafael


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: BIOS overwritten during resume (was: Re: Asus L5D resume on battery power)

[Nigel Cunningham]

Hi.

On Sat, 2005-03-05 at 10:26, Rafael J. Wysocki wrote:
> Yes.  I thought about using PG_nosave in the begining, but there's a
> 
> BUG_ON(PageReserved(page) && PageNosave(page));
> 
> in swsusp.c:saveable() that I just didn't want to trigger.  It seems to me,
> though, that we don't need it any more, do we?
> 
> > He also found a few places where reserved page becomes un-reserved,
> > and you probably need to fix those, too.
> 
> Yes, I think I'll just port the Nigel's patch to x86-64.  BTW, it's striking
> that we found similar solutions independently (I didn't know the Nigel's
> patch before :-)).

I should clarify credit. I didn't work on that code. I don't recall now
whether it was Michael Frank or Bernard Blackham that came up with this
version. (This was about a year ago IIRC).

> Unfortunately, it turns out that the patch does not fix my problem with random
> reboots during resume on battery power, but I really think that we need to mark
> non-RAM areas with PG_nosave, at least for sanity reasons (eg to be sure that
> we do not break things by dumping stuff to where we should not write to).

Yes. I believed that that's what this patch does. Since I didn't write
it myself, I'm a little fuzzy on that issue. I'll bring Bernard in,
perhaps he can clarify?..

Nigel
-- 
Nigel Cunningham
Software Engineer, Canberra, Australia
http://www.cyclades.com
Bus: +61 (2) 6291 9554; Hme: +61 (2) 6292 8028;  Mob: +61 (417) 100 574

Maintainer of Suspend2 Kernel Patches http://softwaresuspend.berlios.de

Re: BIOS overwritten during resume (was: Re: Asus L5D resume on battery power)

[Pavel Machek]

Hi!

> > Actually, take a look at Nigel's patch. He simply uses PageNosave
> > instead of PageLocked -- that is cleaner.
> 
> Yes.  I thought about using PG_nosave in the begining, but there's a
> 
> BUG_ON(PageReserved(page) && PageNosave(page));
> 
> in swsusp.c:saveable() that I just didn't want to trigger.  It seems to me,
> though, that we don't need it any more, do we?

No, we can just kill it. It was "if something unexpected happens, bail
out soon".

> > He also found a few places where reserved page becomes un-reserved,
> > and you probably need to fix those, too.
> 
> Yes, I think I'll just port the Nigel's patch to x86-64.  BTW, it's striking
> that we found similar solutions independently (I didn't know the Nigel's
> patch before :-)).
> 
> Unfortunately, it turns out that the patch does not fix my problem with random
> reboots during resume on battery power, but I really think that we
> need to mark

:-( too bad.

> non-RAM areas with PG_nosave, at least for sanity reasons (eg to be sure that
> we do not break things by dumping stuff to where we should not write to).

I'm not sure if it is not better to save & restore non-RAM areas, but
it probably just does not matter.
								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: BIOS overwritten during resume (was: Re: Asus L5D resume on battery power)

[Bernard Blackham]

On Sat, Mar 05, 2005 at 10:37:29AM +1100, Nigel Cunningham wrote:
> On Sat, 2005-03-05 at 10:26, Rafael J. Wysocki wrote:
> > Yes, I think I'll just port the Nigel's patch to x86-64.  BTW, it's striking
> > that we found similar solutions independently (I didn't know the Nigel's
> > patch before :-)).
> 
> I should clarify credit. I didn't work on that code. I don't recall now
> whether it was Michael Frank or Bernard Blackham that came up with this
> version. (This was about a year ago IIRC).

Definitely not me, sorry :)

Bernard.

-- 
 Bernard Blackham <bernard at blackham dot com dot au>

Re: BIOS overwritten during resume (was: Re: Asus L5D resume on battery power)

[Nigel Cunningham]

Hi.

On Sat, 2005-03-05 at 10:41, Pavel Machek wrote:
> > non-RAM areas with PG_nosave, at least for sanity reasons (eg to be sure that
> > we do not break things by dumping stuff to where we should not write to).
> 
> I'm not sure if it is not better to save & restore non-RAM areas, but
> it probably just does not matter.

IIRC, it does matter. I think there were situations where you got
something nasty (MCE/oops/freeze) if you tried reading memory that
doesn't exist. If you push me I'll put the effort into looking up
suspend2 archives to find the discussion :>

Regards,

Nigel
-- 
Nigel Cunningham
Software Engineer, Canberra, Australia
http://www.cyclades.com
Bus: +61 (2) 6291 9554; Hme: +61 (2) 6292 8028;  Mob: +61 (417) 100 574

Maintainer of Suspend2 Kernel Patches http://softwaresuspend.berlios.de

Re: BIOS overwritten during resume (was: Re: Asus L5D resume on battery power)

[Rafael J. Wysocki]

Hi,

On Saturday, 5 of March 2005 02:10, Nigel Cunningham wrote:
> Hi.
> 
> On Sat, 2005-03-05 at 10:41, Pavel Machek wrote:
> > > non-RAM areas with PG_nosave, at least for sanity reasons (eg to be sure that
> > > we do not break things by dumping stuff to where we should not write to).
> > 
> > I'm not sure if it is not better to save & restore non-RAM areas, but
> > it probably just does not matter.

For the address ranges that are reported by the BIOS as reserved, it
probably doesn't matter indeed, but for the address ranges that are
not reported at all, it's potentially dangerous.  Unfortunately
they all are generally treated in the same way, so why should we do it
differently?
 
> IIRC, it does matter. I think there were situations where you got
> something nasty (MCE/oops/freeze) if you tried reading memory that
> doesn't exist. If you push me I'll put the effort into looking up
> suspend2 archives to find the discussion :>

If you're so kind, that would be great!

Greets,
Rafael


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: BIOS overwritten during resume (was: Re: Asus L5D resume on battery power)

[Rafael J. Wysocki]

Hi,

On Friday, 4 of March 2005 15:21, Nigel Cunningham wrote:
[-- snip --]
> 
> Will something like this patch help?
> 
[-- snip --]

I think that the changes below are unnecessary.  free_all_bootmem() is
actually called _before_ the loop in mem_init() in which PG_nosave is set for
the first time, so there's no need to clear it earlier.

> diff -ruNp 208-e820-table-support-old/mm/bootmem.c 208-e820-table-support-new/mm/bootmem.c
> --- 208-e820-table-support-old/mm/bootmem.c	2005-01-12 17:07:15.902387400 +1100
> +++ 208-e820-table-support-new/mm/bootmem.c	2005-01-12 17:23:44.087160480 +1100
> @@ -280,12 +280,14 @@ static unsigned long __init free_all_boo
>  
>  			count += BITS_PER_LONG;
>  			__ClearPageReserved(page);
> +			ClearPageNosave(page);
>  			order = ffs(BITS_PER_LONG) - 1;
>  			set_page_refs(page, order);
>  			for (j = 1; j < BITS_PER_LONG; j++) {
>  				if (j + 16 < BITS_PER_LONG)
>  					prefetchw(page + j + 16);
>  				__ClearPageReserved(page + j);
> +				ClearPageNosave(page + j);
>  			}
>  			__free_pages(page, order);
>  			i += BITS_PER_LONG;
> @@ -296,6 +298,7 @@ static unsigned long __init free_all_boo
>  				if (v & m) {
>  					count++;
>  					__ClearPageReserved(page);
> +					ClearPageNosave(page);
>  					set_page_refs(page, 0);
>  					__free_page(page);
>  				}
> @@ -316,6 +319,7 @@ static unsigned long __init free_all_boo
>  	for (i = 0; i < ((bdata->node_low_pfn-(bdata->node_boot_start >> PAGE_SHIFT))/8 + PAGE_SIZE-1)/PAGE_SIZE; i++,page++) {
>  		count++;
>  		__ClearPageReserved(page);
> +		ClearPageNosave(page);
>  		set_page_count(page, 1);
>  		__free_page(page);
>  	}

Greets,
Rafael
 

-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: BIOS overwritten during resume (was: Re: Asus L5D resume on battery power)

[Rafael J. Wysocki]

On Saturday, 5 of March 2005 00:41, Pavel Machek wrote:
> Hi!
> 
> > > Actually, take a look at Nigel's patch. He simply uses PageNosave
> > > instead of PageLocked -- that is cleaner.
> > 
> > Yes.  I thought about using PG_nosave in the begining, but there's a
> > 
> > BUG_ON(PageReserved(page) && PageNosave(page));
> > 
> > in swsusp.c:saveable() that I just didn't want to trigger.  It seems to me,
> > though, that we don't need it any more, do we?
> 
> No, we can just kill it. It was "if something unexpected happens, bail
> out soon".

OK

The following is what I'm comfortable with.  I didn't took the Nigel's patch
literally, because we do one thing differently (ie nosave pfns) and it contained
some changes that I thought were unnecessary.  The i386 part is untested.

Greets,
Rafael


Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

diff -Nrup linux-2.6.11/arch/i386/mm/init.c linux-2.6.11-a/arch/i386/mm/init.c
--- linux-2.6.11/arch/i386/mm/init.c	2005-03-02 08:38:17.000000000 +0100
+++ linux-2.6.11-a/arch/i386/mm/init.c	2005-03-06 18:16:34.000000000 +0100
@@ -272,12 +272,15 @@ void __init one_highpage_init(struct pag
 {
 	if (page_is_ram(pfn) && !(bad_ppro && page_kills_ppro(pfn))) {
 		ClearPageReserved(page);
+		ClearPageNosave(page);
 		set_bit(PG_highmem, &page->flags);
 		set_page_count(page, 1);
 		__free_page(page);
 		totalhigh_pages++;
-	} else
+	} else {
 		SetPageReserved(page);
+		SetPageNosave(page);
+	}
 }
 
 #ifndef CONFIG_DISCONTIGMEM
@@ -602,11 +605,17 @@ void __init mem_init(void)
 
 	reservedpages = 0;
 	for (tmp = 0; tmp < max_low_pfn; tmp++)
-		/*
-		 * Only count reserved RAM pages
-		 */
-		if (page_is_ram(tmp) && PageReserved(pfn_to_page(tmp)))
-			reservedpages++;
+		if (!page_is_ram(tmp))
+			/*
+			 * Non-RAM pages are always nosave
+			 */
+			SetPageNosave(pfn_to_page(tmp));
+		else
+			/*
+			 * Count reserved RAM pages
+			 */
+			if (PageReserved(pfn_to_page(tmp)))
+				reservedpages++;
 
 	set_highmem_pages_init(bad_ppro);
 
@@ -705,6 +714,7 @@ void free_initmem(void)
 	addr = (unsigned long)(&__init_begin);
 	for (; addr < (unsigned long)(&__init_end); addr += PAGE_SIZE) {
 		ClearPageReserved(virt_to_page(addr));
+		ClearPageNosave(virt_to_page(addr));
 		set_page_count(virt_to_page(addr), 1);
 		memset((void *)addr, 0xcc, PAGE_SIZE);
 		free_page(addr);
@@ -720,6 +730,7 @@ void free_initrd_mem(unsigned long start
 		printk (KERN_INFO "Freeing initrd memory: %ldk freed\n", (end - start) >> 10);
 	for (; start < end; start += PAGE_SIZE) {
 		ClearPageReserved(virt_to_page(start));
+		ClearPageNosave(virt_to_page(start));
 		set_page_count(virt_to_page(start), 1);
 		free_page(start);
 		totalram_pages++;
diff -Nrup linux-2.6.11/arch/x86_64/mm/init.c linux-2.6.11-a/arch/x86_64/mm/init.c
--- linux-2.6.11/arch/x86_64/mm/init.c	2005-03-02 08:37:49.000000000 +0100
+++ linux-2.6.11-a/arch/x86_64/mm/init.c	2005-03-06 18:16:34.000000000 +0100
@@ -438,11 +438,17 @@ void __init mem_init(void)
 	totalram_pages += free_all_bootmem();
 
 	for (tmp = 0; tmp < end_pfn; tmp++)
-		/*
-		 * Only count reserved RAM pages
-		 */
-		if (page_is_ram(tmp) && PageReserved(pfn_to_page(tmp)))
-			reservedpages++;
+		if (!page_is_ram(tmp))
+			/*
+			 * Non-RAM pages are always nosave
+			 */
+			SetPageNosave(pfn_to_page(tmp));
+		else
+			/*
+			 * Count reserved RAM pages
+			 */
+			if (PageReserved(pfn_to_page(tmp)))
+				reservedpages++;
 #endif
 
 	after_bootmem = 1;
@@ -488,6 +494,7 @@ void free_initmem(void)
 	addr = (unsigned long)(&__init_begin);
 	for (; addr < (unsigned long)(&__init_end); addr += PAGE_SIZE) {
 		ClearPageReserved(virt_to_page(addr));
+		ClearPageNosave(virt_to_page(addr));
 		set_page_count(virt_to_page(addr), 1);
 		memset((void *)(addr & ~(PAGE_SIZE-1)), 0xcc, PAGE_SIZE); 
 		free_page(addr);
@@ -505,6 +512,7 @@ void free_initrd_mem(unsigned long start
 	printk ("Freeing initrd memory: %ldk freed\n", (end - start) >> 10);
 	for (; start < end; start += PAGE_SIZE) {
 		ClearPageReserved(virt_to_page(start));
+		ClearPageNosave(virt_to_page(start));
 		set_page_count(virt_to_page(start), 1);
 		free_page(start);
 		totalram_pages++;
diff -Nrup linux-2.6.11/kernel/power/swsusp.c linux-2.6.11-a/kernel/power/swsusp.c
--- linux-2.6.11/kernel/power/swsusp.c	2005-03-02 08:37:50.000000000 +0100
+++ linux-2.6.11-a/kernel/power/swsusp.c	2005-03-06 18:16:34.000000000 +0100
@@ -532,9 +532,9 @@ static int saveable(struct zone * zone, 
 		return 0;
 
 	page = pfn_to_page(pfn);
-	BUG_ON(PageReserved(page) && PageNosave(page));
 	if (PageNosave(page))
 		return 0;
+
 	if (PageReserved(page) && pfn_is_nosave(pfn)) {
 		pr_debug("[nosave pfn 0x%lx]", pfn);
 		return 0;

-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: BIOS overwritten during resume (was: Re: Asus L5D resume on battery power)

[Pavel Machek]

Hi!
> > > Yes.  I thought about using PG_nosave in the begining, but there's a
> > > 
> > > BUG_ON(PageReserved(page) && PageNosave(page));
> > > 
> > > in swsusp.c:saveable() that I just didn't want to trigger.  It seems to me,
> > > though, that we don't need it any more, do we?
> > 
> > No, we can just kill it. It was "if something unexpected happens, bail
> > out soon".
> 
> OK
> 
> The following is what I'm comfortable with.  I didn't took the Nigel's patch
> literally, because we do one thing differently (ie nosave pfns) and it contained
> some changes that I thought were unnecessary.  The i386 part is
> untested.

I'd add

>  	page = pfn_to_page(pfn);
> -	BUG_ON(PageReserved(page) && PageNosave(page));

a comment here explaining what PageReserved && PageNosave means. 

>  	if (PageNosave(page))
>  		return 0;
> +
>  	if (PageReserved(page) && pfn_is_nosave(pfn)) {
>  		pr_debug("[nosave pfn 0x%lx]", pfn);
>  		return 0;

AFAICT it only fixes "potential" bug, so it can probably wait. Once
non-contiguous and initramfs patches are in, this can go...
								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: BIOS overwritten during resume (was: Re: Asus L5D resume on battery power)

[Rafael J. Wysocki]

Hi,

On Sunday, 6 of March 2005 20:41, Pavel Machek wrote:
> Hi!
> > > > Yes.  I thought about using PG_nosave in the begining, but there's a
> > > > 
> > > > BUG_ON(PageReserved(page) && PageNosave(page));
> > > > 
> > > > in swsusp.c:saveable() that I just didn't want to trigger.  It seems to me,
> > > > though, that we don't need it any more, do we?
> > > 
> > > No, we can just kill it. It was "if something unexpected happens, bail
> > > out soon".
> > 
> > OK
> > 
> > The following is what I'm comfortable with.  I didn't took the Nigel's patch
> > literally, because we do one thing differently (ie nosave pfns) and it contained
> > some changes that I thought were unnecessary.  The i386 part is
> > untested.
> 
> I'd add
> 
> >  	page = pfn_to_page(pfn);
> > -	BUG_ON(PageReserved(page) && PageNosave(page));
> 
> a comment here explaining what PageReserved && PageNosave means. 

OK, I will add the comment.

> >  	if (PageNosave(page))
> >  		return 0;
> > +
> >  	if (PageReserved(page) && pfn_is_nosave(pfn)) {
> >  		pr_debug("[nosave pfn 0x%lx]", pfn);
> >  		return 0;
> 
> AFAICT it only fixes "potential" bug, so it can probably wait. Once
> non-contiguous and initramfs patches are in, this can go...

OK

Greets,
Rafael


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"