Re: [PATCH] x86/mm, hibernate: Fix misjudgment of register setup_data page to nosave region

From: joeyli
Date: Fri Jan 30 2015 - 11:28:40 EST

Next message: Bryan O'Donoghue: "[PATCH v8 0/2] x86: Add IMR support to Quark/Galileo"
Previous message: Chris Packham: "Re: [PATCH] dtc: Use quotes to include header files"
In reply to: joeyli: "Re: [PATCH] x86/mm, hibernate: Fix misjudgment of register setup_data page to nosave region"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Jan 30, 2015 at 10:46:48PM +0800, joeyli wrote:
> On Fri, Jan 30, 2015 at 12:30:00AM -0800, Yinghai Lu wrote:
> > On Thu, Jan 29, 2015 at 7:58 PM, Lee, Chun-Yi <joeyli.kernel@xxxxxxxxx> wrote:
> > > The reserve setup_data action break usable regions to not align to
> > > page size. As following case:
> > >
> > > BIOS-e820: [mem 0x0000000000088000-0x00000000000bffff] reserved
> > > BIOS-e820: [mem 0x0000000000100000-0x0000000094caffff] usable
> > > ...
> > > e820: update [mem 0x93c5f018-0x93c6f057] usable ==> usable /* reserve setup_data */
> > > e820: update [mem 0x93c4f018-0x93c5e057] usable ==> usable /* reserve setup_data */
> > > ...
> > > reserve setup_data: [mem0x0000000000088000-0x00000000000bffff] reserved
> > > reserve setup_data: [mem0x0000000000100000-0x0000000093c4f017] usable /* not align */
> > > reserve setup_data: [mem0x0000000093c4f018-0x0000000093c5e057] usable /* not align */
> > > reserve setup_data: [mem0x0000000093c5e058-0x0000000093c5f017] usable /* not align */
> > > reserve setup_data: [mem0x0000000093c5f018-0x0000000093c6f057] usable /* not align */
> > > reserve setup_data: [mem0x0000000093c6f058-0x0000000094caffff] usable
> > >
> > > The codes in e820_mark_nosave_regions() check pfn of each e820
> > > entry to find out the hole between two entries then register it to
> > > nosave region. This logic misjudges the non-align but continuous usable
> > > region, then register one page in usable region to nosave. As following:
> > >
> > > PM: Registered nosave memory: [mem 0x000c0000-0x000fffff]
> > > PM: Registered nosave memory: [mem 0x93c4f000-0x93c4ffff] /* misjudgment */
> > > PM: Registered nosave memory: [mem 0x93c5e000-0x93c5efff] /* misjudgment */
> > > PM: Registered nosave memory: [mem 0x93c5f000-0x93c5ffff] /* misjudgment */
> > > PM: Registered nosave memory: [mem 0x93c6f000-0x93c6ffff] /* misjudgment */
> > > PM: Registered nosave memory: [mem 0x94cb0000-0x960affff]
> > >
> > > The issue causes some usable pages didn't collect to hibernate snapshot image.
> > > And, it also misjudges a usable page in nosave regions then hibernate resuming
> > > process interrupted by the unsafe pages checking:
> > > https://bugzilla.opensuse.org/show_bug.cgi?id=913885
> > >
> > > This patch changed the code in e820_mark_nosave_regions() to check the
> > > address instead of pfn to avoid above issue.
> >
> > [+cc: Ying Huang]
> >
> > would like to suggest use attached instead:
> >
> > Subject: [RFC PATCH] x86: Kill E820_RESERVED_KERN
> >
> > Now we are using memblock to do early resource allocation instead of using
> > e820 map directly, and setup_data is reserved in memblock early.
> > Also kexec pass setup_data pointer to second kernel, so second kernel
> > will reserve setup_data by their own.
> >
> > So we can kill E820_RESERVED_KERN and not touch e820 map at all.
> >
> > That will fix bug in mark_nonsave_region that can not handle that
> > case: E820_RAM and E820_RESERVED_KERN continuous and boundary is
> > not page aligned.
> >
> > Not sure about why tboot need to use this...
>
> Yes, that's good to totally remove the E820_RESERVED_KERN because we already
> use memblock to reserve setup_data ranges.
>
>
> Thanks
> Joey Lee

This patch works on my Gateway EG50_HW notebook to fix issue in nosave regions.

Tested-by: Lee, Chun-Yi <jlee@xxxxxxxx>

Thanks a lot!
Joey Lee

>
> > Subject: [RFC PATCH] x86: Kill E820_RESERVED_KERN
> >
> > Now we are using memblock to do early resource allocation instead of using
> > e820 map directly, and setup_data is reserved in memblock early.
> > Also kexec pass setup_data pointer to second kernel, so second kernel
> > will reserve setup_data by their own.
> >
> > So we can kill E820_RESERVED_KERN and not touch e820 map at all.
> >
> > That will fix bug in mark_nonsave_region that can not handle that
> > case: E820_RAM and E820_RESERVED_KERN continuous and boundary is
> > not page aligned.
> >
> > Not sure about why tboot need to use this...
> >
> > Signed-off-by: Yinghai Lu <yinghai@xxxxxxxxxx>
> >
> > ---
> > arch/x86/include/uapi/asm/e820.h | 9 ---------
> > arch/x86/kernel/e820.c | 6 ++----
> > arch/x86/kernel/setup.c | 26 --------------------------
> > arch/x86/kernel/tboot.c | 3 +--
> > arch/x86/mm/init_64.c | 11 ++++-------
> > 5 files changed, 7 insertions(+), 48 deletions(-)
> >
> > Index: linux-2.6/arch/x86/include/uapi/asm/e820.h
> > ===================================================================
> > --- linux-2.6.orig/arch/x86/include/uapi/asm/e820.h
> > +++ linux-2.6/arch/x86/include/uapi/asm/e820.h
> > @@ -33,15 +33,6 @@
> > #define E820_NVS 4
> > #define E820_UNUSABLE 5
> >
> > -
> > -/*
> > - * reserved RAM used by kernel itself
> > - * if CONFIG_INTEL_TXT is enabled, memory of this type will be
> > - * included in the S3 integrity calculation and so should not include
> > - * any memory that BIOS might alter over the S3 transition
> > - */
> > -#define E820_RESERVED_KERN 128
> > -
> > #ifndef __ASSEMBLY__
> > #include <linux/types.h>
> > struct e820entry {
> > 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
> > @@ -134,7 +134,6 @@ static void __init e820_print_type(u32 t
> > {
> > switch (type) {
> > case E820_RAM:
> > - case E820_RESERVED_KERN:
> > printk(KERN_CONT "usable");
> > break;
> > case E820_RESERVED:
> > @@ -688,7 +687,7 @@ void __init e820_mark_nosave_regions(uns
> > register_nosave_region(pfn, PFN_UP(ei->addr));
> >
> > pfn = PFN_DOWN(ei->addr + ei->size);
> > - if (ei->type != E820_RAM && ei->type != E820_RESERVED_KERN)
> > + if (ei->type != E820_RAM)
> > register_nosave_region(PFN_UP(ei->addr), pfn);
> >
> > if (pfn >= limit_pfn)
> > @@ -902,7 +901,6 @@ void __init finish_e820_parsing(void)
> > static inline const char *e820_type_to_string(int e820_type)
> > {
> > switch (e820_type) {
> > - case E820_RESERVED_KERN:
> > case E820_RAM: return "System RAM";
> > case E820_ACPI: return "ACPI Tables";
> > case E820_NVS: return "ACPI Non-volatile Storage";
> > @@ -1077,7 +1075,7 @@ void __init memblock_x86_fill(void)
> > if (end != (resource_size_t)end)
> > continue;
> >
> > - if (ei->type != E820_RAM && ei->type != E820_RESERVED_KERN)
> > + if (ei->type != E820_RAM)
> > continue;
> >
> > memblock_add(ei->addr, ei->size);
> > Index: linux-2.6/arch/x86/kernel/setup.c
> > ===================================================================
> > --- linux-2.6.orig/arch/x86/kernel/setup.c
> > +++ linux-2.6/arch/x86/kernel/setup.c
> > @@ -456,30 +456,6 @@ static void __init parse_setup_data(void
> > }
> > }
> >
> > -static void __init e820_reserve_setup_data(void)
> > -{
> > - struct setup_data *data;
> > - u64 pa_data;
> > - int found = 0;
> > -
> > - pa_data = boot_params.hdr.setup_data;
> > - while (pa_data) {
> > - data = early_memremap(pa_data, sizeof(*data));
> > - e820_update_range(pa_data, sizeof(*data)+data->len,
> > - E820_RAM, E820_RESERVED_KERN);
> > - found = 1;
> > - pa_data = data->next;
> > - early_iounmap(data, sizeof(*data));
> > - }
> > - if (!found)
> > - return;
> > -
> > - sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &e820.nr_map);
> > - memcpy(&e820_saved, &e820, sizeof(struct e820map));
> > - printk(KERN_INFO "extended physical RAM map:\n");
> > - e820_print_map("reserve setup_data");
> > -}
> > -
> > static void __init memblock_x86_reserve_range_setup_data(void)
> > {
> > struct setup_data *data;
> > @@ -1011,8 +987,6 @@ void __init setup_arch(char **cmdline_p)
> > early_dump_pci_devices();
> > #endif
> >
> > - /* update the e820_saved too */
> > - e820_reserve_setup_data();
> > finish_e820_parsing();
> >
> > if (efi_enabled(EFI_BOOT))
> > Index: linux-2.6/arch/x86/kernel/tboot.c
> > ===================================================================
> > --- linux-2.6.orig/arch/x86/kernel/tboot.c
> > +++ linux-2.6/arch/x86/kernel/tboot.c
> > @@ -195,8 +195,7 @@ static int tboot_setup_sleep(void)
> > tboot->num_mac_regions = 0;
> >
> > for (i = 0; i < e820.nr_map; i++) {
> > - if ((e820.map[i].type != E820_RAM)
> > - && (e820.map[i].type != E820_RESERVED_KERN))
> > + if (e820.map[i].type != E820_RAM)
> > continue;
> >
> > add_mac_region(e820.map[i].addr, e820.map[i].size);
> > Index: linux-2.6/arch/x86/mm/init_64.c
> > ===================================================================
> > --- linux-2.6.orig/arch/x86/mm/init_64.c
> > +++ linux-2.6/arch/x86/mm/init_64.c
> > @@ -426,8 +426,7 @@ phys_pte_init(pte_t *pte_page, unsigned
> > next = (addr & PAGE_MASK) + PAGE_SIZE;
> > if (addr >= end) {
> > if (!after_bootmem &&
> > - !e820_any_mapped(addr & PAGE_MASK, next, E820_RAM) &&
> > - !e820_any_mapped(addr & PAGE_MASK, next, E820_RESERVED_KERN))
> > + !e820_any_mapped(addr & PAGE_MASK, next, E820_RAM))
> > set_pte(pte, __pte(0));
> > continue;
> > }
> > @@ -473,9 +472,8 @@ phys_pmd_init(pmd_t *pmd_page, unsigned
> >
> > next = (address & PMD_MASK) + PMD_SIZE;
> > if (address >= end) {
> > - if (!after_bootmem &&
> > - !e820_any_mapped(address & PMD_MASK, next, E820_RAM) &&
> > - !e820_any_mapped(address & PMD_MASK, next, E820_RESERVED_KERN))
> > + if (!after_bootmem && !e820_any_mapped(
> > + address & PMD_MASK, next, E820_RAM))
> > set_pmd(pmd, __pmd(0));
> > continue;
> > }
> > @@ -548,8 +546,7 @@ phys_pud_init(pud_t *pud_page, unsigned
> > next = (addr & PUD_MASK) + PUD_SIZE;
> > if (addr >= end) {
> > if (!after_bootmem &&
> > - !e820_any_mapped(addr & PUD_MASK, next, E820_RAM) &&
> > - !e820_any_mapped(addr & PUD_MASK, next, E820_RESERVED_KERN))
> > + !e820_any_mapped(addr & PUD_MASK, next, E820_RAM))
> > set_pud(pud, __pud(0));
> > continue;
> > }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Bryan O'Donoghue: "[PATCH v8 0/2] x86: Add IMR support to Quark/Galileo"
Previous message: Chris Packham: "Re: [PATCH] dtc: Use quotes to include header files"
In reply to: joeyli: "Re: [PATCH] x86/mm, hibernate: Fix misjudgment of register setup_data page to nosave region"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]