On Tue, Oct 27, 2020 at 10:44:21PM +0000, Edgecombe, Rick P wrote:

On Tue, 2020-10-27 at 10:49 +0200, Mike Rapoport wrote:

quoted

On Mon, Oct 26, 2020 at 06:57:32PM +0000, Edgecombe, Rick P wrote:

quoted

On Mon, 2020-10-26 at 11:15 +0200, Mike Rapoport wrote:

quoted

On Mon, Oct 26, 2020 at 12:38:32AM +0000, Edgecombe, Rick P
wrote:

quoted

On Sun, 2020-10-25 at 12:15 +0200, Mike Rapoport wrote:

quoted

From: Mike Rapoport <redacted>

When DEBUG_PAGEALLOC or ARCH_HAS_SET_DIRECT_MAP is enabled a
page
may
be
not present in the direct map and has to be explicitly mapped
before
it
could be copied.

On arm64 it is possible that a page would be removed from the
direct
map
using set_direct_map_invalid_noflush() but
__kernel_map_pages()
will
refuse
to map this page back if DEBUG_PAGEALLOC is disabled.

It looks to me that arm64 __kernel_map_pages() will still
attempt
to
map it if rodata_full is true, how does this happen?

Unless I misread the code, arm64 requires both rodata_full and
debug_pagealloc_enabled() to be true for __kernel_map_pages() to
do
anything.
But rodata_full condition applies to set_direct_map_*_noflush()
as
well,
so with !rodata_full the linear map won't be ever changed.

Hmm, looks to me that __kernel_map_pages() will only skip it if
both
debug pagealloc and rodata_full are false.

But now I'm wondering if maybe we could simplify things by just
moving
the hibernate unmapped page logic off of the direct map. On x86,
text_poke() used to use this reserved fixmap pte thing that it
could
rely on to remap memory with. If hibernate had some separate pte
for
remapping like that, then we could not have any direct map
restrictions
caused by it/kernel_map_pages(), and it wouldn't have to worry
about
relying on anything else.

Well, there is map_kernel_range() that can be used by hibernation as
there is no requirement for particular virtual address, but that
would
be quite costly if done for every page.

Maybe we can do somthing like

	if (kernel_page_present(s_page)) {
		do_copy_page(dst, page_address(s_page));
	} else {
		map_kernel_range_noflush(page_address(page), PAGE_SIZE,
					 PROT_READ, &page);
		do_copy_page(dst, page_address(s_page));
		unmap_kernel_range_noflush(page_address(page),
PAGE_SIZE);
	}

But it seems that a prerequisite for changing the way a page is
mapped
in safe_copy_page() would be to teach hibernation that a mapping here
may fail.

Yea that is what I meant, the direct map could still be used for mapped
pages.

But for the unmapped case it could have a pre-setup 4k pte for some non
direct map address. Then just change the pte to point to any unmapped
direct map page that was encountered. The point would be to give
hibernate some 4k pte of its own to manipulate so that it can't fail.

Yet another option would be have hibernate_map_page() just map large
pages if it finds them.

So we could teach hibernate to handle mapping failures, OR we could
change it so it doesn't rely on direct map page sizes in order to
succeed. The latter seems better to me since there isn't a reason why
it should have to fail and the resulting logic might be simpler. Both
seem like improvements in robustness though.

That's correct, but as the purpose of this series is to prevent usage of
__kernel_map_pages() outside DEBUG_PAGALLOC, for now I'm going to update this
patch changelog and add a comment to hibernate_map_page().

-- 
Sincerely yours,
Mike.