From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752716AbaHDMmh (ORCPT ); Mon, 4 Aug 2014 08:42:37 -0400 Received: from cantor2.suse.de ([195.135.220.15]:58765 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751478AbaHDMmf (ORCPT ); Mon, 4 Aug 2014 08:42:35 -0400 Date: Mon, 04 Aug 2014 14:42:32 +0200 Message-ID: From: Takashi Iwai To: "Lee, Chun-Yi" Cc: "Rafael J. Wysocki" , Len Brown , Pavel Machek , linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, "Lee, Chun-Yi" Subject: Re: [PATCH] Hibernate: check unsafe page should not in e820 reserved region In-Reply-To: <1407148396-21405-1-git-send-email-jlee@suse.com> References: <1407148396-21405-1-git-send-email-jlee@suse.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/24.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org At Mon, 4 Aug 2014 18:33:16 +0800, Lee, Chun-Yi wrote: > > When the machine doesn't well handle the e820 persistent when hibernate > resuming, then it may causes page fault when writing image to snapshot > buffer: > > [ 17.929495] BUG: unable to handle kernel paging request at ffff880069d4f000 > [ 17.933469] IP: [] load_image_lzo+0x810/0xe40 > [ 17.933469] PGD 2194067 PUD 77ffff067 PMD 2197067 PTE 0 > [ 17.933469] Oops: 0002 [#1] SMP > ... > > The ffff880069d4f000 page is in e820 reserved region of resume boot > kernel: > > [ 0.000000] BIOS-e820: [mem 0x0000000069d4f000-0x0000000069e12fff] reserved > ... > [ 0.000000] PM: Registered nosave memory: [mem 0x69d4f000-0x69e12fff] > > So snapshot.c mark the pfn to forbidden pages map. But, this > page is also in the memory bitmap in snapshot image because it's an > original page used by image kernel, so it will also mark as an > unsafe(free) page in prepare_image(). > > That means the page in e820 when resuming mark as "forbidden" and > "free", it causes get_buffer() treat it as an allocated unsafe page. > Then snapshot_write_next() return this page to load_image, load_image > writing content to this address, but this page didn't really allocated > . So, we got page fault. > > Although the root cause is from BIOS, I think aggressive check and > significant message in kernel will better then a page fault for > issue tracking, especially when serial console unavailable. > > This patch adds code in mark_unsafe_pages() for check does free pages in > nosave region. If so, then it print message and return fault to stop whole > S4 resume process: > > [ 8.166004] PM: Image loading progress: 0% > [ 8.658717] PM: 0x6796c000 in e820 nosave regsion: [mem 0x6796c000-0x6796cfff] > [ 8.918737] PM: Read 2511940 kbytes in 1.04 seconds (2415.32 MB/s) > [ 8.926633] PM: Error -14 resuming > [ 8.933534] PM: Failed to load hibernation image, recovering. > > Cc: "Rafael J. Wysocki" > Cc: Len Brown > Cc: Pavel Machek > Signed-off-by: Lee, Chun-Yi > --- > kernel/power/snapshot.c | 24 +++++++++++++++++++++++- > 1 file changed, 23 insertions(+), 1 deletion(-) > > diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c > index 98c3b34..e6db5a8 100644 > --- a/kernel/power/snapshot.c > +++ b/kernel/power/snapshot.c > @@ -730,6 +730,28 @@ static void mark_nosave_pages(struct memory_bitmap *bm) > } > } > > +static bool is_nosave_page(unsigned long pfn) > +{ > + struct nosave_region *region; > + > + if (list_empty(&nosave_regions)) > + return 0; Just a nitpicking: this should be "false" for consistency. But, looking further at the code: > + > + list_for_each_entry(region, &nosave_regions, list) { > + if (pfn >= region->start_pfn && pfn < region->end_pfn) { > + pr_err("PM: %#010llx in e820 nosave regsion: " > + "[mem %#010llx-%#010llx]\n", > + (unsigned long long) pfn << PAGE_SHIFT, > + (unsigned long long) region->start_pfn << PAGE_SHIFT, > + ((unsigned long long) region->end_pfn << PAGE_SHIFT) > + - 1); > + return true; > + } > + } > + > + return false; I think the above empty check can be removed completely. Takashi