[PATCH v33 07/14] arm64: hibernate: preserve kdump image around hibernation

On Tue, Mar 21, 2017 at 06:25:46PM +0000, James Morse wrote:

Hi Akashi,

On 15/03/17 09:59, AKASHI Takahiro wrote:

quoted

Since arch_kexec_protect_crashkres() removes a mapping for crash dump
kernel image, the loaded data won't be preserved around hibernation.

In this patch, helper functions, kexec_prepare_suspend()/
kexec_post_resume(), are additionally called before/after hibernation so
that the relevant memory segments will be mapped again and preserved just
as the others are.

In addition, to minimize the size of hibernation image,
kexec_is_chraskres_nosave() is added to pfn_is_nosave() in order to

(crashkres)

Yes.

quoted

recoginize only the pages that hold loaded crash dump kernel image as

(recognize)

Ah, yes ...

quoted

saveable. Hibernation excludes any pages that are marked as Reserved and
yet "nosave."

Neat! I didn't think this would be possible without hacking kernel/power/snapshot.c.

I've given this a spin on Juno and Seattle, I even added debug_pagealloc, but
that doesn't trick it because your kexec_prepare_suspend() puts the mapping back.

Reviewed-by: James Morse <james.morse@arm.com>


Thanks,

James


Some nits about comments:

quoted

diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c
index 97a7384100f3..1e10fafa59bd 100644
--- a/arch/arm64/kernel/hibernate.c
+++ b/arch/arm64/kernel/hibernate.c

@@ -286,6 +288,9 @@ int swsusp_arch_suspend(void)
 	local_dbg_save(flags);
 
 	if (__cpu_suspend_enter(&state)) {
+		/* make the crash dump kernel image visible/saveable */
+		kexec_prepare_suspend();

Strictly this is kdump not kexec, but the comment makes that clear.

I hesitated to use "kdump_" here as there are no functions with
such a prefix (say, even arch_kexec_protect_crashkres()).
So probably "crash_" or "crash_kexec_" would sound much better.

quoted

+
 		sleep_cpu = smp_processor_id();
 		ret = swsusp_save();
 	} else {

quoted

diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c
index 02e4f929db3b..82f48db589cf 100644
--- a/arch/arm64/kernel/machine_kexec.c
+++ b/arch/arm64/kernel/machine_kexec.c

quoted

@@ -220,7 +221,6 @@ void arch_kexec_protect_crashkres(void)
 			kexec_crash_image->segment[i].memsz,
 			PAGE_KERNEL_INVALID, true);
 
-

Stray whitespace change from a previous patch?

Oops, fix it.

quoted

 	flush_tlb_all();
 }
 

@@ -233,3 +233,74 @@ void arch_kexec_unprotect_crashkres(void)

quoted

+/*
+ * kexec_is_crashres_nosave
+ *
+ * Return true only if a page is part of reserved memory for crash dump kernel,
+ * but does not hold any data of loaded kernel image.
+ *
+ * Note that all the pages in crash dump kernel memory have been initially
+ * marked as Reserved in kexec_reserve_crashkres_pages().
+ *
+ * In hibernation, the pages which are Reserved and yet "nosave"
+ * are excluded from the hibernation iamge. kexec_is_crashkres_nosave()
+ * does this check for crash dump kernel and will reduce the total size
+ * of hibernation image.
+ */
+
+bool kexec_is_crashkres_nosave(unsigned long pfn)
+{
+	int i;
+	phys_addr_t addr;
+
+	/* in reserved memory? */

Comment in the wrong place?

This is after my deep thinking :) but

quoted

+	if (!crashk_res.end)
+		return false;
+
+	addr = __pfn_to_phys(pfn);

(makes more sense here)

if you think so, I will follow you.

quoted

+	if ((addr < crashk_res.start) || (crashk_res.end < addr))
+		return false;
+

quoted

+	/* not part of loaded kernel image? */

Comment in the wrong place?

quoted

+	if (!kexec_crash_image)
+		return true;
+

(makes more sense here)

ditto

quoted

+	for (i = 0; i < kexec_crash_image->nr_segments; i++)
+		if (addr >= kexec_crash_image->segment[i].mem &&
+				addr < (kexec_crash_image->segment[i].mem +
+					kexec_crash_image->segment[i].memsz))
+			return false;
+
+	return true;
+}
+

Thanks,

James

Thank you for your review!
-Takahiro AKASHI