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

From: joeyli
Date: Fri Jan 30 2015 - 09:47:12 EST

Next message: Kirill A. Shutemov: "[PATCH 10/19] parisc: expose number of page table levels on Kconfig level"
Previous message: Jiri Olsa: "[PATCHv3] tools lib traceevent: Add priv field to struct format_field"
In reply to: Yinghai Lu: "Re: [PATCH] x86/mm, hibernate: Fix misjudgment of register setup_data page to nosave region"
Next in thread: 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 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

> 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: Kirill A. Shutemov: "[PATCH 10/19] parisc: expose number of page table levels on Kconfig level"
Previous message: Jiri Olsa: "[PATCHv3] tools lib traceevent: Add priv field to struct format_field"
In reply to: Yinghai Lu: "Re: [PATCH] x86/mm, hibernate: Fix misjudgment of register setup_data page to nosave region"
Next in thread: joeyli: "Re: [PATCH] x86/mm, hibernate: Fix misjudgment of register setup_data page to nosave region"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]