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

[Rafael J. Wysocki]

On Wed, Jan 11, 2023 at 9:46 PM Edgecombe, Rick P
<rick.p.edgecombe@xxxxxxxxx> wrote:
>
> On Wed, 2023-01-11 at 21:35 +0100, Rafael J. Wysocki wrote:
> > On Tue, Jan 10, 2023 at 10:19 PM Rick Edgecombe
> > <rick.p.edgecombe@xxxxxxxxx> 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.
> > >
> > > Move do_copy_page() to a header such that it can be used in an arch
> > > breakout. Rename it hib_copy_page() to be more hibernate specific
> > > since
> > > it could appear in other files.
> > >
> > > 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@xxxxxxxxx>
> > > ---
> > >
> > > Rebase to v6.2-rc3 (original still applied) and resending per:
> > >
> https://lore.kernel.org/lkml/CAJZ5v0i6cxGD+V6G+q-Y_Lp-ov51_zmkZr8ZGpCtqWV-e=BsLg@xxxxxxxxxxxxxx/
> > >
> > >  arch/x86/include/asm/fixmap.h |  3 +++
> > >  arch/x86/power/hibernate.c    | 10 ++++++++++
> > >  include/linux/suspend.h       | 13 +++++++++++++
> > >  kernel/power/snapshot.c       | 21 +++++++--------------
> > >  4 files changed, 33 insertions(+), 14 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..473b6b5f6b7e 100644
> > > --- a/arch/x86/power/hibernate.c
> > > +++ b/arch/x86/power/hibernate.c
> > > @@ -147,6 +147,16 @@ int arch_hibernation_header_restore(void
> > > *addr)
> > >         return 0;
> > >  }
> > >
> > > +void copy_unmapped_page(void *dst, struct page *page)
> > > +{
> > > +       WARN_ON(!preempt_count());
> >
> > I don't think the above is needed.  The code using this function
> > cannot be preempted anyway AFAICS.
>
> The reason I thought it was useful was because this function is now
> defined in a header. Someone else might decide to use it. Does it seem
> more useful?

Well, it is exposed now, but only in order to allow the __weak
function to be overridden.  I don't think it is logically valid to use
it anywhere beyond its original call site.

To make that clear, I would call it something hibernation-specific,
like hibernate_copy_unmapped_page() and I would add a kerneldoc
comment to it to describe its intended use.

Furthermore, I'm not sure about the new code layout.

Personally, I would prefer hibernate_map_page() and
hibernate_unmap_page() to be turned into __weak functions and possibly
overridden by the arch code, which would allow the amount of changes
to be reduced and do_copy_page() wouldn't need to be moved into the
header any more.

> >
> > > +
> > > +       set_fixmap(FIX_HIBERNATE, page_to_phys(page));
> > > +       __flush_tlb_all();
> >
> > So do TLBs need to be flushed before copying every single page?
> > Basically, they are all copied in one loop.
>
> It is only one fixmap entry so it needs to be flushed after changing
> the PTE to point to a different page. But this is only for the case of
> unmapped pages, the more common mapped pages are copied from the direct
> map like usual.

OK

> >
> > > +       hib_copy_page(dst, (void *)fix_to_virt(FIX_HIBERNATE));
> > > +       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..0b19b910526e 100644
> > > --- a/include/linux/suspend.h
> > > +++ b/include/linux/suspend.h
> > > @@ -447,6 +447,19 @@ extern bool hibernation_available(void);
> > >  asmlinkage int swsusp_save(void);
> > >  extern struct pbe *restore_pblist;
> > >  int pfn_is_nosave(unsigned long pfn);
> > > +void copy_unmapped_page(void *dst, struct page *page);
> > > +
> > > +/*
> > > + * This is needed, because copy_page and memcpy are not usable for
> > > copying
> > > + * task structs.
> > > + */
> > > +static inline void hib_copy_page(long *dst, long *src)
> > > +{
> > > +       int n;
> > > +
> > > +       for (n = PAGE_SIZE / sizeof(long); n; n--)
> > > +               *dst++ = *src++;
> > > +}
> > >
> > >  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..344c071f29d3 100644
> > > --- a/kernel/power/snapshot.c
> > > +++ b/kernel/power/snapshot.c
> > > @@ -1369,16 +1369,11 @@ static unsigned int count_data_pages(void)
> > >         return n;
> > >  }
> > >
> > > -/*
> > > - * This is needed, because copy_page and memcpy are not usable for
> > > copying
> > > - * task structs.
> > > - */
> > > -static inline void do_copy_page(long *dst, long *src)
> > > +void __weak copy_unmapped_page(void *dst, struct page *page)
> > >  {
> > > -       int n;
> > > -
> > > -       for (n = PAGE_SIZE / sizeof(long); n; n--)
> > > -               *dst++ = *src++;
> > > +       hibernate_map_page(page);
> > > +       hib_copy_page(dst, page_address(page));
> > > +       hibernate_unmap_page(page);
> > >  }
> > >
> > >  /**
> > > @@ -1392,11 +1387,9 @@ static inline void do_copy_page(long *dst,
> > > long *src)
> > >  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));
> > > +               hib_copy_page(dst, page_address(s_page));
> > >         } else {
> > > -               hibernate_map_page(s_page);
> > > -               do_copy_page(dst, page_address(s_page));
> > > -               hibernate_unmap_page(s_page);
> > > +               copy_unmapped_page(dst, s_page);
> > >         }
> > >  }
> > >
> > > @@ -1417,7 +1410,7 @@ static void copy_data_page(unsigned long
> > > dst_pfn, unsigned long src_pfn)
> > >         if (PageHighMem(s_page)) {
> > >                 src = kmap_atomic(s_page);
> > >                 dst = kmap_atomic(d_page);
> > > -               do_copy_page(dst, src);
> > > +               hib_copy_page(dst, src);
> > >                 kunmap_atomic(dst);
> > >                 kunmap_atomic(src);
> > >         } else {
> > > --
> > > 2.17.1
> > >