Re: [PATCH v3] x86/hibernate: Use fixmap for saving unmapped pages

["Rafael J. Wysocki" <rafael@kernel.org>]

On Fri, Jan 20, 2023 at 12:52 AM Rick Edgecombe
<rick.p.edgecombe@intel.com> wrote:
>
> Hibernate uses the direct map to read memory it saves to disk. Since
> sometimes pages are not accessible on the direct map ("not present" on
> x86), it has special case logic to temporarily make a page present. On x86
> these direct map addresses can be mapped at various page sizes, but the
> logic works ok as long as the not present pages are always mapped as
> PAGE_SIZE such that they don't require a split to map the region as
> present. If the address was mapped not present by a larger page size, the
> split may fail and hibernate would then try to read an address mapped not
> present.
>
> Today on x86 there are no known cases of this (huge not present pages on
> the direct map), but it has come up from time to time when developing
> things that operate on the direct map. It blocked making
> VM_FLUSH_RESET_PERMS support huge vmalloc when that came up, and also
> has been a complication for various direct map protection efforts.
>
> This dependency is also pretty hidden and easily missed by people poking at
> the direct map. For this reason, there are warnings in place to complain
> but not handle this scenario.
>
> One way to make this more robust would be to create some new CPA
> functionality that can know to map and reset the whole huge page in the
> case of trying to map a subpage. But for simplicity and smaller code, just
> make x86 hibernate have its own fixmap PTE that it can use to point
> to 4k pages when it encounters an unmapped direct map page.
>
> So create arch breakouts for hibernate_map_page() and
> hibernate_unmap_page() so that the mapping of unmapped pages can be
> off the direct map. Change hibernate_map_page() to return an address,
> and implement an arch breakout on x86 on that uses the fixmap.
>
> Since now hibernate_map_page() can return a value, have it return NULL
> when the page fails to map, and have safe_copy_page() skip copying the
> page if it fails to map. The existing behavior should crash in this case,
> so this way there is a chance the system would be recoverable.
>
> Use __weak for the arch breakout because there is not a suitable arch
> specific header to use the #define method.
>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> ---
>
> v3:
>  - Switch from hib_copy_page() breakout to hibernate_un/map_page()
>    breakouts.
>  - Since there is an error to use now, skip copying for unmappable pages

Much better than before.

In case the x86 folks want to take this

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

and otherwise please let me know that I need to take care of it.

>
> v2:
>  - Rebase
>
>  arch/x86/include/asm/fixmap.h |  3 +++
>  arch/x86/power/hibernate.c    | 12 ++++++++++++
>  include/linux/suspend.h       |  2 ++
>  kernel/power/snapshot.c       | 22 ++++++++++++++--------
>  4 files changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/include/asm/fixmap.h b/arch/x86/include/asm/fixmap.h
> index d0dcefb5cc59..0fceed9a4152 100644
> --- a/arch/x86/include/asm/fixmap.h
> +++ b/arch/x86/include/asm/fixmap.h
> @@ -108,6 +108,9 @@ enum fixed_addresses {
>  #ifdef CONFIG_PARAVIRT_XXL
>         FIX_PARAVIRT_BOOTMAP,
>  #endif
> +#ifdef CONFIG_HIBERNATION
> +       FIX_HIBERNATE,
> +#endif
>
>  #ifdef CONFIG_ACPI_APEI_GHES
>         /* Used for GHES mapping from assorted contexts */
> diff --git a/arch/x86/power/hibernate.c b/arch/x86/power/hibernate.c
> index 6f955eb1e163..e7cde7cd529d 100644
> --- a/arch/x86/power/hibernate.c
> +++ b/arch/x86/power/hibernate.c
> @@ -147,6 +147,18 @@ int arch_hibernation_header_restore(void *addr)
>         return 0;
>  }
>
> +long *hibernate_map_page(struct page *page)
> +{
> +       set_fixmap(FIX_HIBERNATE, page_to_phys(page));
> +       __flush_tlb_all();
> +       return (long *)fix_to_virt(FIX_HIBERNATE);
> +}
> +
> +void hibernate_unmap_page(struct page *page)
> +{
> +       clear_fixmap(FIX_HIBERNATE);
> +}
> +
>  int relocate_restore_code(void)
>  {
>         pgd_t *pgd;
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index cfe19a028918..a6c3f7dac9e1 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -447,6 +447,8 @@ extern bool hibernation_available(void);
>  asmlinkage int swsusp_save(void);
>  extern struct pbe *restore_pblist;
>  int pfn_is_nosave(unsigned long pfn);
> +long *hibernate_map_page(struct page *page);
> +void hibernate_unmap_page(struct page *page);
>
>  int hibernate_quiet_exec(int (*func)(void *data), void *data);
>  #else /* CONFIG_HIBERNATION */
> diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
> index cd8b7b35f1e8..8cc16114da75 100644
> --- a/kernel/power/snapshot.c
> +++ b/kernel/power/snapshot.c
> @@ -83,19 +83,18 @@ static inline void hibernate_restore_unprotect_page(void *page_address) {}
>   * It is still worth to have a warning here if something changes and this
>   * will no longer be the case.
>   */
> -static inline void hibernate_map_page(struct page *page)
> +long * __weak hibernate_map_page(struct page *page)
>  {
>         if (IS_ENABLED(CONFIG_ARCH_HAS_SET_DIRECT_MAP)) {
> -               int ret = set_direct_map_default_noflush(page);
> -
> -               if (ret)
> -                       pr_warn_once("Failed to remap page\n");
> +               if (set_direct_map_default_noflush(page))
> +                       return NULL;
>         } else {
>                 debug_pagealloc_map_pages(page, 1);
>         }
> +       return page_address(page);
>  }
>
> -static inline void hibernate_unmap_page(struct page *page)
> +void __weak hibernate_unmap_page(struct page *page)
>  {
>         if (IS_ENABLED(CONFIG_ARCH_HAS_SET_DIRECT_MAP)) {
>                 unsigned long addr = (unsigned long)page_address(page);
> @@ -1394,8 +1393,15 @@ static void safe_copy_page(void *dst, struct page *s_page)
>         if (kernel_page_present(s_page)) {
>                 do_copy_page(dst, page_address(s_page));
>         } else {
> -               hibernate_map_page(s_page);
> -               do_copy_page(dst, page_address(s_page));
> +               long *src = hibernate_map_page(s_page);
> +
> +               if (!src) {
> +                       pr_warn_once("Failed to remap page\n");
> +                       return;
> +               }
> +
> +               do_copy_page(dst, src);
> +
>                 hibernate_unmap_page(s_page);
>         }
>  }
> --