Re: [PATCH] s390/kexec: Consolidate crash_map/unmap_reserved_pages() and arch_kexec_protect(unprotect)_crashkres()

[Michael Holzheu <holzheu@linux.vnet.ibm.com>]

Hello Xunlei,

On Sat, 2 Apr 2016 09:23:50 +0800
Xunlei Pang <xpang@redhat.com> wrote:

> On 2016/04/02 at 01:41, Michael Holzheu wrote:
> > Hello Xunlei again,
> >
> > Some initial comments below...
> >
> > On Wed, 30 Mar 2016 19:47:21 +0800
> > Xunlei Pang <xlpang@redhat.com> wrote:
> >

[snip]

> >> +			os_info_crashkernel_add(0, 0);
> >> +	}
> >> +}
> > Please do not move these functions in the file. If you leave it at their
> > old location, the patch will be *much* smaller.
> 
> In fact, I did this wanting avoiding adding extra declaration.

IMHO no extra declaration is necessary (see patch below).

[snip]

> >> +/*
> >>   * PM notifier callback for kdump
> >>   */
> >>  static int machine_kdump_pm_cb(struct notifier_block *nb, unsigned long action,
> >> @@ -43,12 +89,12 @@ static int machine_kdump_pm_cb(struct notifier_block *nb, unsigned long action,
> >>  	switch (action) {
> >>  	case PM_SUSPEND_PREPARE:
> >>  	case PM_HIBERNATION_PREPARE:
> >> -		if (crashk_res.start)
> >> +		if (kexec_crash_image)
> > Why this change?
> 
> arch_kexec_protect_crashkres() will do the unmapping once kdump kernel is loaded
> (i.e. kexec_crash_image is non-NULL), so we should check "kexec_crash_image" here
> and do the corresponding re-mapping. 
> 
> NULL crashk_res_image means that kdump kernel is not loaded, in this case mapping is
> already setup either initially in reserve_crashkernel() or by arch_kexec_unprotect_crashkres().

Sorry, I missed this obvious part. So your change seems to be ok here.

There is another problem with your patch: The vmem_remove_mapping() function
can only remove mappings which have been added by vmem_add_mapping() before.
Therefore currently the vmem_remove_mapping() call is a nop and we still have
a RW mapping after the kexec system call.

If you check "/sys/kernel/debug/kernel_page_tables" you will see that the
crashkernel memory is still mapped RW after loading kdump.

To fix this we could keep the memblock_remove() and call vmem_add_mapping()
from a init function (see patch below).

[snip]

> >> --- a/arch/s390/kernel/setup.c
> >> +++ b/arch/s390/kernel/setup.c
> >> @@ -603,7 +603,7 @@ static void __init reserve_crashkernel(void)
> >>  	crashk_res.start = crash_base;
> >>  	crashk_res.end = crash_base + crash_size - 1;
> >>  	insert_resource(&iomem_resource, &crashk_res);
> >> -	memblock_remove(crash_base, crash_size);
> >> +	memblock_reserve(crash_base, crash_size);
> > I will discuss this next week in our team.
> 
> This can address the bad page warning when shrinking crashk_res.

Yes, shrinking crashkernel memory is currently broken on s390. Heiko Carstens
(on cc) plans to do some general rework that probably will automatically fix
this issue.

So I would suggest that you merge the following with your initial patch and
then resend the merged patch.

What do you think?

Michael
---------------8<----
s390/kdump: Consolidate crash_map/unmap_reserved_pages() - update

 - Move functions back to keep the patch small
 - Consolidate crash_map_reserved_pages/arch_kexec_unprotect_crashkres()
 - Re-introduce memblock_remove()
 - Call arch_kexec_unprotect_crashkres() in machine_kdump_pm_init()

Signed-off-by: Michael Holzheu <holzheu@linux.vnet.ibm.com>
---
 arch/s390/kernel/machine_kexec.c |   88 +++++++++++++++++----------------------
 1 file changed, 40 insertions(+), 48 deletions(-)

--- a/arch/s390/kernel/machine_kexec.c
+++ b/arch/s390/kernel/machine_kexec.c
@@ -35,52 +35,6 @@ extern const unsigned long long relocate
 #ifdef CONFIG_CRASH_DUMP
 
 /*
- * Map or unmap crashkernel memory
- */
-static void crash_map_pages(int enable)
-{
-	unsigned long size = resource_size(&crashk_res);
-
-	BUG_ON(crashk_res.start % KEXEC_CRASH_MEM_ALIGN ||
-	       size % KEXEC_CRASH_MEM_ALIGN);
-	if (enable)
-		vmem_add_mapping(crashk_res.start, size);
-	else {
-		vmem_remove_mapping(crashk_res.start, size);
-		if (size)
-			os_info_crashkernel_add(crashk_res.start, size);
-		else
-			os_info_crashkernel_add(0, 0);
-	}
-}
-
-/*
- * Map crashkernel memory
- */
-static void crash_map_reserved_pages(void)
-{
-	crash_map_pages(1);
-}
-
-/*
- * Unmap crashkernel memory
- */
-static void crash_unmap_reserved_pages(void)
-{
-	crash_map_pages(0);
-}
-
-void arch_kexec_protect_crashkres(void)
-{
-	crash_unmap_reserved_pages();
-}
-
-void arch_kexec_unprotect_crashkres(void)
-{
-	crash_map_reserved_pages();
-}
-
-/*
  * PM notifier callback for kdump
  */
 static int machine_kdump_pm_cb(struct notifier_block *nb, unsigned long action,
@@ -90,12 +44,12 @@ static int machine_kdump_pm_cb(struct no
 	case PM_SUSPEND_PREPARE:
 	case PM_HIBERNATION_PREPARE:
 		if (kexec_crash_image)
-			crash_map_reserved_pages();
+			arch_kexec_unprotect_crashkres();
 		break;
 	case PM_POST_SUSPEND:
 	case PM_POST_HIBERNATION:
 		if (kexec_crash_image)
-			crash_unmap_reserved_pages();
+			arch_kexec_protect_crashkres();
 		break;
 	default:
 		return NOTIFY_DONE;
@@ -106,6 +60,8 @@ static int machine_kdump_pm_cb(struct no
 static int __init machine_kdump_pm_init(void)
 {
 	pm_notifier(machine_kdump_pm_cb, 0);
+	/* Create initial mapping for crashkernel memory */
+	arch_kexec_unprotect_crashkres();
 	return 0;
 }
 arch_initcall(machine_kdump_pm_init);
@@ -193,6 +149,42 @@ static int kdump_csum_valid(struct kimag
 }
 
 /*
+ * Map or unmap crashkernel memory
+ */
+static void crash_map_pages(int enable)
+{
+	unsigned long size = resource_size(&crashk_res);
+
+	BUG_ON(crashk_res.start % KEXEC_CRASH_MEM_ALIGN ||
+	       size % KEXEC_CRASH_MEM_ALIGN);
+	if (enable)
+		vmem_add_mapping(crashk_res.start, size);
+	else {
+		vmem_remove_mapping(crashk_res.start, size);
+		if (size)
+			os_info_crashkernel_add(crashk_res.start, size);
+		else
+			os_info_crashkernel_add(0, 0);
+	}
+}
+
+/*
+ * Unmap crashkernel memory
+ */
+void arch_kexec_protect_crashkres(void)
+{
+	crash_map_pages(0);
+}
+
+/*
+ * Map crashkernel memory
+ */
+void arch_kexec_unprotect_crashkres(void)
+{
+	crash_map_pages(1);
+}
+
+/*
  * Give back memory to hypervisor before new kdump is loaded
  */
 static int machine_kexec_prepare_kdump(void)


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec