[PATCH V2 0/2] kexec: Make a pair of map/unmap reserved pages in error path

[PATCH V2 0/2] kexec: Make a pair of map/unmap reserved pages in error path

[Minfei Huang]

v1:
- Bisect the patch according to Andrew Morton's suggestion

Minfei Huang (2):
  kexec: Make a pair of map/unmap reserved pages in error path
  kexec: Do a cleanup for function kexec_load

 kernel/kexec.c | 112 ++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 63 insertions(+), 49 deletions(-)

-- 
1.9.1

[PATCH V2 1/2] kexec: Make a pair of map/unmap reserved pages in error path

[Minfei Huang]

For some arch, kexec shall map the reserved pages, then use them, when
we try to start the kdump service.

kexec may return directly, without unmaping the reserved pages, if it
fails during starting service. To fix it, we make a pair of map/unmap
reserved pages both in generic path and error path.

Signed-off-by: Minfei Huang <mnfhuang@gmail.com>
---
 kernel/kexec.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/kernel/kexec.c b/kernel/kexec.c
index ee70aef..5cd60c4 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -169,6 +169,7 @@ SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
 	dest_image = &kexec_image;
 	if (flags & KEXEC_ON_CRASH)
 		dest_image = &kexec_crash_image;
+
 	if (nr_segments > 0) {
 		unsigned long i;
 
@@ -190,22 +191,25 @@ SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
 						   segments, flags);
 		}
 		if (result)
-			goto out;
+			goto unmap_page;
 
 		if (flags & KEXEC_PRESERVE_CONTEXT)
 			image->preserve_context = 1;
 		result = machine_kexec_prepare(image);
 		if (result)
-			goto out;
+			goto unmap_page;
 
 		for (i = 0; i < nr_segments; i++) {
 			result = kimage_load_segment(image, &image->segment[i]);
 			if (result)
-				goto out;
+				goto unmap_page;
 		}
 		kimage_terminate(image);
+unmap_page:
 		if (flags & KEXEC_ON_CRASH)
 			crash_unmap_reserved_pages();
+		if (result)
+			goto out;
 	}
 	/* Install the new kernel, and  Uninstall the old */
 	image = xchg(dest_image, image);
-- 
1.9.1

Re: [PATCH V2 1/2] kexec: Make a pair of map/unmap reserved pages in error path

[Andrew Morton]

On Tue,  1 Mar 2016 16:02:28 +0800 Minfei Huang <mnfhuang@gmail.com> wrote:

> For some arch, kexec shall map the reserved pages, then use them, when
> we try to start the kdump service.

Which architectures are these, by the way?

> kexec may return directly, without unmaping the reserved pages, if it
> fails during starting service. To fix it, we make a pair of map/unmap
> reserved pages both in generic path and error path.

I'm having trouble understanding the urgency of this patch.  Do you
think it is needed in 4.5?  -stable?  If so, why?

Thanks.

Re: [PATCH V2 1/2] kexec: Make a pair of map/unmap reserved pages in error path

[Minfei Huang]

On 03/01/16 at 01:56pm, Andrew Morton wrote:
> On Tue,  1 Mar 2016 16:02:28 +0800 Minfei Huang <mnfhuang@gmail.com> wrote:
> 
> > For some arch, kexec shall map the reserved pages, then use them, when
> > we try to start the kdump service.
> 
> Which architectures are these, by the way?

Hi.

This patch only affects s390. The others doesn't implement the interface
of crash_unmap_reserved_pages and crash_map_reserved_pages.

> 
> > kexec may return directly, without unmaping the reserved pages, if it
> > fails during starting service. To fix it, we make a pair of map/unmap
> > reserved pages both in generic path and error path.
> 
> I'm having trouble understanding the urgency of this patch.  Do you
> think it is needed in 4.5?  -stable?  If so, why?

IMHO, it is fine in next release as it isn't a urgent patch. Kernel can
work well without any risk, although the reseverd pages are not unmaped
before returning in error path.

Thanks
Minfei

[PATCH V2 2/2] kexec: Do a cleanup for function kexec_load

[Minfei Huang]

There are a lof of work to be done in function kexec_load, not only for
allocating structs and loading initram, but also for some misc.

To make it more clear, wrap a new function do_kexec_load which is used
to allocate structs and load initram. And the pre-work will be done in
kexec_load.

Signed-off-by: Minfei Huang <mnfhuang@gmail.com>
---
 kernel/kexec.c | 116 +++++++++++++++++++++++++++++++--------------------------
 1 file changed, 63 insertions(+), 53 deletions(-)

diff --git a/kernel/kexec.c b/kernel/kexec.c
index 5cd60c4..48cf69c 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -103,6 +103,68 @@ out_free_image:
 	return ret;
 }
 
+static int do_kexec_load(unsigned long entry, unsigned long nr_segments,
+		struct kexec_segment __user *segments, unsigned long flags)
+{
+	struct kimage **dest_image, *image;
+	unsigned long i;
+	int ret;
+
+	if (flags & KEXEC_ON_CRASH)
+		dest_image = &kexec_crash_image;
+	else
+		dest_image = &kexec_image;
+
+	if (nr_segments == 0) {
+		/* Uninstall image */
+		kimage_free(xchg(dest_image, NULL));
+		return 0;
+	}
+	if (flags & KEXEC_ON_CRASH) {
+		/*
+		 * Loading another kernel to switch to if this one
+		 * crashes.  Free any current crash dump kernel before
+		 * we corrupt it.
+		 */
+		kimage_free(xchg(&kexec_crash_image, NULL));
+	}
+
+	ret = kimage_alloc_init(&image, entry, nr_segments, segments, flags);
+	if (ret)
+		return ret;
+
+	if (flags & KEXEC_ON_CRASH)
+		crash_map_reserved_pages();
+
+	if (flags & KEXEC_PRESERVE_CONTEXT)
+		image->preserve_context = 1;
+
+	ret = machine_kexec_prepare(image);
+	if (ret)
+		goto out;
+
+	for (i = 0; i < nr_segments; i++) {
+		ret = kimage_load_segment(image, &image->segment[i]);
+		if (ret)
+			goto out;
+	}
+
+	kimage_terminate(image);
+
+	/* Install the new kernel and uninstall the old */
+	image = xchg(dest_image, image);
+
+out:
+	/*
+	 * Once the reserved memory is mapped, we should unmap this memory
+	 * before returning
+	 */
+	if (flags & KEXEC_ON_CRASH)
+		crash_unmap_reserved_pages();
+	kimage_free(image);
+	return ret;
+}
+
 /*
  * Exec Kernel system call: for obvious reasons only root may call it.
  *
@@ -127,7 +189,6 @@ out_free_image:
 SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
 		struct kexec_segment __user *, segments, unsigned long, flags)
 {
-	struct kimage **dest_image, *image;
 	int result;
 
 	/* We only trust the superuser with rebooting the system. */
@@ -152,9 +213,6 @@ SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
 	if (nr_segments > KEXEC_SEGMENT_MAX)
 		return -EINVAL;
 
-	image = NULL;
-	result = 0;
-
 	/* Because we write directly to the reserved memory
 	 * region when loading crash kernels we need a mutex here to
 	 * prevent multiple crash  kernels from attempting to load
@@ -166,57 +224,9 @@ SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
 	if (!mutex_trylock(&kexec_mutex))
 		return -EBUSY;
 
-	dest_image = &kexec_image;
-	if (flags & KEXEC_ON_CRASH)
-		dest_image = &kexec_crash_image;
-
-	if (nr_segments > 0) {
-		unsigned long i;
-
-		if (flags & KEXEC_ON_CRASH) {
-			/*
-			 * Loading another kernel to switch to if this one
-			 * crashes.  Free any current crash dump kernel before
-			 * we corrupt it.
-			 */
-
-			kimage_free(xchg(&kexec_crash_image, NULL));
-			result = kimage_alloc_init(&image, entry, nr_segments,
-						   segments, flags);
-			crash_map_reserved_pages();
-		} else {
-			/* Loading another kernel to reboot into. */
-
-			result = kimage_alloc_init(&image, entry, nr_segments,
-						   segments, flags);
-		}
-		if (result)
-			goto unmap_page;
-
-		if (flags & KEXEC_PRESERVE_CONTEXT)
-			image->preserve_context = 1;
-		result = machine_kexec_prepare(image);
-		if (result)
-			goto unmap_page;
-
-		for (i = 0; i < nr_segments; i++) {
-			result = kimage_load_segment(image, &image->segment[i]);
-			if (result)
-				goto unmap_page;
-		}
-		kimage_terminate(image);
-unmap_page:
-		if (flags & KEXEC_ON_CRASH)
-			crash_unmap_reserved_pages();
-		if (result)
-			goto out;
-	}
-	/* Install the new kernel, and  Uninstall the old */
-	image = xchg(dest_image, image);
+	result = do_kexec_load(entry, nr_segments, segments, flags);
 
-out:
 	mutex_unlock(&kexec_mutex);
-	kimage_free(image);
 
 	return result;
 }
-- 
1.9.1

Re: [PATCH V2 2/2] kexec: Do a cleanup for function kexec_load

[Andrew Morton]

On Tue,  1 Mar 2016 16:02:29 +0800 Minfei Huang <mnfhuang@gmail.com> wrote:

> There are a lof of work to be done in function kexec_load, not only for
> allocating structs and loading initram, but also for some misc.
> 
> To make it more clear, wrap a new function do_kexec_load which is used
> to allocate structs and load initram. And the pre-work will be done in
> kexec_load.
> 

This patch needed quite a few changes to accommodate
http://ozlabs.org/~akpm/mmots/broken-out/kexec-introduce-a-protection-mechanism-for-the-crashkernel-reserved-memory.patch.
The resulting code and the resulting diff are below.  Please test and
check carefully.

static int do_kexec_load(unsigned long entry, unsigned long nr_segments,
		struct kexec_segment __user *segments, unsigned long flags)
{
	struct kimage **dest_image, *image;
	unsigned long i;
	int ret;

	if (flags & KEXEC_ON_CRASH) {
		dest_image = &kexec_crash_image;
		if (kexec_crash_image)
			arch_kexec_unprotect_crashkres();
	} else {
		dest_image = &kexec_image;
	}

	if (nr_segments == 0) {
		/* Uninstall image */
		kimage_free(xchg(dest_image, NULL));
		return 0;
	}
	if (flags & KEXEC_ON_CRASH) {
		/*
		 * Loading another kernel to switch to if this one
		 * crashes.  Free any current crash dump kernel before
		 * we corrupt it.
		 */
		kimage_free(xchg(&kexec_crash_image, NULL));
	}

	ret = kimage_alloc_init(&image, entry, nr_segments, segments, flags);
	if (ret)
		return ret;

	if (flags & KEXEC_ON_CRASH)
		crash_map_reserved_pages();

	if (flags & KEXEC_PRESERVE_CONTEXT)
		image->preserve_context = 1;

	ret = machine_kexec_prepare(image);
	if (ret)
		goto out;

	for (i = 0; i < nr_segments; i++) {
		ret = kimage_load_segment(image, &image->segment[i]);
		if (ret)
			goto out;
	}

	kimage_terminate(image);

	/* Install the new kernel and uninstall the old */
	image = xchg(dest_image, image);

out:
	if ((flags & KEXEC_ON_CRASH) && kexec_crash_image)
		arch_kexec_protect_crashkres();

	/*
	 * Once the reserved memory is mapped, we should unmap this memory
	 * before returning
	 */
	if (flags & KEXEC_ON_CRASH)
		crash_unmap_reserved_pages();
	kimage_free(image);
	return ret;
}

/*
 * Exec Kernel system call: for obvious reasons only root may call it.
 *
 * This call breaks up into three pieces.
 * - A generic part which loads the new kernel from the current
 *   address space, and very carefully places the data in the
 *   allocated pages.
 *
 * - A generic part that interacts with the kernel and tells all of
 *   the devices to shut down.  Preventing on-going dmas, and placing
 *   the devices in a consistent state so a later kernel can
 *   reinitialize them.
 *
 * - A machine specific part that includes the syscall number
 *   and then copies the image to it's final destination.  And
 *   jumps into the image at entry.
 *
 * kexec does not sync, or unmount filesystems so if you need
 * that to happen you need to do that yourself.
 */

SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
		struct kexec_segment __user *, segments, unsigned long, flags)
{
	int result;

	/* We only trust the superuser with rebooting the system. */
	if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
		return -EPERM;

	/*
	 * Verify we have a legal set of flags
	 * This leaves us room for future extensions.
	 */
	if ((flags & KEXEC_FLAGS) != (flags & ~KEXEC_ARCH_MASK))
		return -EINVAL;

	/* Verify we are on the appropriate architecture */
	if (((flags & KEXEC_ARCH_MASK) != KEXEC_ARCH) &&
		((flags & KEXEC_ARCH_MASK) != KEXEC_ARCH_DEFAULT))
		return -EINVAL;

	/* Put an artificial cap on the number
	 * of segments passed to kexec_load.
	 */
	if (nr_segments > KEXEC_SEGMENT_MAX)
		return -EINVAL;

	/* Because we write directly to the reserved memory
	 * region when loading crash kernels we need a mutex here to
	 * prevent multiple crash  kernels from attempting to load
	 * simultaneously, and to prevent a crash kernel from loading
	 * over the top of a in use crash kernel.
	 *
	 * KISS: always take the mutex.
	 */
	if (!mutex_trylock(&kexec_mutex))
		return -EBUSY;

	result = do_kexec_load(entry, nr_segments, segments, flags);

	if ((flags & KEXEC_ON_CRASH) && kexec_crash_image)
		arch_kexec_protect_crashkres();

	mutex_unlock(&kexec_mutex);

	return result;
}


From: Minfei Huang <mnfhuang@gmail.com>
Subject: kexec: do a cleanup for function kexec_load

There are a lof of work to be done in function kexec_load, not only for
allocating structs and loading initram, but also for some misc.

To make it more clear, wrap a new function do_kexec_load which is used to
allocate structs and load initram.  And the pre-work will be done in
kexec_load.

Signed-off-by: Minfei Huang <mnfhuang@gmail.com>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 kernel/kexec.c |  125 +++++++++++++++++++++++++----------------------
 1 file changed, 69 insertions(+), 56 deletions(-)

diff -puN kernel/kexec.c~kexec-do-a-cleanup-for-function-kexec_load kernel/kexec.c
--- a/kernel/kexec.c~kexec-do-a-cleanup-for-function-kexec_load
+++ a/kernel/kexec.c
@@ -103,6 +103,74 @@ out_free_image:
 	return ret;
 }
 
+static int do_kexec_load(unsigned long entry, unsigned long nr_segments,
+		struct kexec_segment __user *segments, unsigned long flags)
+{
+	struct kimage **dest_image, *image;
+	unsigned long i;
+	int ret;
+
+	if (flags & KEXEC_ON_CRASH) {
+		dest_image = &kexec_crash_image;
+		if (kexec_crash_image)
+			arch_kexec_unprotect_crashkres();
+	} else {
+		dest_image = &kexec_image;
+	}
+
+	if (nr_segments == 0) {
+		/* Uninstall image */
+		kimage_free(xchg(dest_image, NULL));
+		return 0;
+	}
+	if (flags & KEXEC_ON_CRASH) {
+		/*
+		 * Loading another kernel to switch to if this one
+		 * crashes.  Free any current crash dump kernel before
+		 * we corrupt it.
+		 */
+		kimage_free(xchg(&kexec_crash_image, NULL));
+	}
+
+	ret = kimage_alloc_init(&image, entry, nr_segments, segments, flags);
+	if (ret)
+		return ret;
+
+	if (flags & KEXEC_ON_CRASH)
+		crash_map_reserved_pages();
+
+	if (flags & KEXEC_PRESERVE_CONTEXT)
+		image->preserve_context = 1;
+
+	ret = machine_kexec_prepare(image);
+	if (ret)
+		goto out;
+
+	for (i = 0; i < nr_segments; i++) {
+		ret = kimage_load_segment(image, &image->segment[i]);
+		if (ret)
+			goto out;
+	}
+
+	kimage_terminate(image);
+
+	/* Install the new kernel and uninstall the old */
+	image = xchg(dest_image, image);
+
+out:
+	if ((flags & KEXEC_ON_CRASH) && kexec_crash_image)
+		arch_kexec_protect_crashkres();
+
+	/*
+	 * Once the reserved memory is mapped, we should unmap this memory
+	 * before returning
+	 */
+	if (flags & KEXEC_ON_CRASH)
+		crash_unmap_reserved_pages();
+	kimage_free(image);
+	return ret;
+}
+
 /*
  * Exec Kernel system call: for obvious reasons only root may call it.
  *
@@ -127,7 +195,6 @@ out_free_image:
 SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
 		struct kexec_segment __user *, segments, unsigned long, flags)
 {
-	struct kimage **dest_image, *image;
 	int result;
 
 	/* We only trust the superuser with rebooting the system. */
@@ -152,9 +219,6 @@ SYSCALL_DEFINE4(kexec_load, unsigned lon
 	if (nr_segments > KEXEC_SEGMENT_MAX)
 		return -EINVAL;
 
-	image = NULL;
-	result = 0;
-
 	/* Because we write directly to the reserved memory
 	 * region when loading crash kernels we need a mutex here to
 	 * prevent multiple crash  kernels from attempting to load
@@ -166,63 +230,12 @@ SYSCALL_DEFINE4(kexec_load, unsigned lon
 	if (!mutex_trylock(&kexec_mutex))
 		return -EBUSY;
 
-	dest_image = &kexec_image;
-	if (flags & KEXEC_ON_CRASH) {
-		dest_image = &kexec_crash_image;
-		if (kexec_crash_image)
-			arch_kexec_unprotect_crashkres();
-	}
-
-	if (nr_segments > 0) {
-		unsigned long i;
-
-		if (flags & KEXEC_ON_CRASH) {
-			/*
-			 * Loading another kernel to switch to if this one
-			 * crashes.  Free any current crash dump kernel before
-			 * we corrupt it.
-			 */
-
-			kimage_free(xchg(&kexec_crash_image, NULL));
-			result = kimage_alloc_init(&image, entry, nr_segments,
-						   segments, flags);
-			crash_map_reserved_pages();
-		} else {
-			/* Loading another kernel to reboot into. */
-
-			result = kimage_alloc_init(&image, entry, nr_segments,
-						   segments, flags);
-		}
-		if (result)
-			goto unmap_page;
-
-		if (flags & KEXEC_PRESERVE_CONTEXT)
-			image->preserve_context = 1;
-		result = machine_kexec_prepare(image);
-		if (result)
-			goto unmap_page;
-
-		for (i = 0; i < nr_segments; i++) {
-			result = kimage_load_segment(image, &image->segment[i]);
-			if (result)
-				goto unmap_page;
-		}
-		kimage_terminate(image);
-unmap_page:
-		if (flags & KEXEC_ON_CRASH)
-			crash_unmap_reserved_pages();
-		if (result)
-			goto out;
-	}
-	/* Install the new kernel, and  Uninstall the old */
-	image = xchg(dest_image, image);
+	result = do_kexec_load(entry, nr_segments, segments, flags);
 
-out:
 	if ((flags & KEXEC_ON_CRASH) && kexec_crash_image)
 		arch_kexec_protect_crashkres();
 
 	mutex_unlock(&kexec_mutex);
-	kimage_free(image);
 
 	return result;
 }
_

Re: [PATCH V2 0/2] kexec: Make a pair of map/unmap reserved pages in error path

[Xunlei Pang]

This is a bug fix.

After this, I will try to do a cleanup for crash_unmap/map_reserved_pages()
(only used by S390) to consolidate it with arch_kexec_unprotect/protect_crashkres().

Regards,
Xunlei

On 03/01/2016 at 04:02 PM, Minfei Huang wrote:
> v1:
> - Bisect the patch according to Andrew Morton's suggestion
>
> Minfei Huang (2):
>   kexec: Make a pair of map/unmap reserved pages in error path
>   kexec: Do a cleanup for function kexec_load
>
>  kernel/kexec.c | 112 ++++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 63 insertions(+), 49 deletions(-)
>

Re: [PATCH V2 0/2] kexec: Make a pair of map/unmap reserved pages in error path

[Baoquan He]

On 03/01/16 at 05:53pm, Xunlei Pang wrote:
> This is a bug fix.
> 
> After this, I will try to do a cleanup for crash_unmap/map_reserved_pages()
> (only used by S390) to consolidate it with arch_kexec_unprotect/protect_crashkres().

Hi Xunlei, Minfei,

I think you need discuss together about how to do clean up codes in this
place. From my point of view, arch_map/unmap_reserved_pages and
arch_kexec_protect/unprotect_crashkres() are for the same goal but by
different ways on different arch. So for Xunlei's patchset, you might
need to rethink your implementation, the name of function. I personally
think you just implement a x86 specific arch_map/unmap_reserved_pages.
It may need a more generic name, and then add your x86 arch specific
implementation. Sorry I can't see your patches on my mail client,
Xunlei. Since Andrew asked, I just checked these.

I am fine with Minfei's patch 1/2. But for patch 2/2, it's a little
comfortable to me. Is it really necessary to abstract code block from
kexec_load, then wrap them into a newly added function do_kexec_load()?
Without this wrapping is there a way to do your bug fix? Is there
possibility that do_kexec_load will be called in other places? What's
the benefit to wrap it into do_kexec_load against not wrapping?

Thanks
Baoquan

> 
> Regards,
> Xunlei
> 
> On 03/01/2016 at 04:02 PM, Minfei Huang wrote:
> > v1:
> > - Bisect the patch according to Andrew Morton's suggestion
> >
> > Minfei Huang (2):
> >   kexec: Make a pair of map/unmap reserved pages in error path
> >   kexec: Do a cleanup for function kexec_load
> >
> >  kernel/kexec.c | 112 ++++++++++++++++++++++++++++++++-------------------------
> >  1 file changed, 63 insertions(+), 49 deletions(-)
> >
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V2 0/2] kexec: Make a pair of map/unmap reserved pages in error path

[Xunlei Pang]

On 2016/03/23 at 10:48, Baoquan He wrote:
> On 03/01/16 at 05:53pm, Xunlei Pang wrote:
>> This is a bug fix.
>>
>> After this, I will try to do a cleanup for crash_unmap/map_reserved_pages()
>> (only used by S390) to consolidate it with arch_kexec_unprotect/protect_crashkres().
> Hi Xunlei, Minfei,
>
> I think you need discuss together about how to do clean up codes in this
> place. From my point of view, arch_map/unmap_reserved_pages and
> arch_kexec_protect/unprotect_crashkres() are for the same goal but by
> different ways on different arch. So for Xunlei's patchset, you might
> need to rethink your implementation, the name of function. I personally
> think you just implement a x86 specific arch_map/unmap_reserved_pages.
> It may need a more generic name, and then add your x86 arch specific
> implementation. Sorry I can't see your patches on my mail client,

Like what you said, I think arch_kexec_unprotect/protect_crashkres() are
generic enough, but any other better name is welcome :-)

It also covered the newly-added kexec file path, and we can easily transfer
arch_map/unmap_reserved_pages into this new interface.

I was planning doing that, but sick recently, I will try to send a patch
doing that later.

Regards,
Xunlei

> Xunlei. Since Andrew asked, I just checked these.
>
> I am fine with Minfei's patch 1/2. But for patch 2/2, it's a little
> comfortable to me. Is it really necessary to abstract code block from
> kexec_load, then wrap them into a newly added function do_kexec_load()?
> Without this wrapping is there a way to do your bug fix? Is there
> possibility that do_kexec_load will be called in other places? What's
> the benefit to wrap it into do_kexec_load against not wrapping?
>
> Thanks
> Baoquan
>
>> Regards,
>> Xunlei
>>
>> On 03/01/2016 at 04:02 PM, Minfei Huang wrote:
>>> v1:
>>> - Bisect the patch according to Andrew Morton's suggestion
>>>
>>> Minfei Huang (2):
>>>   kexec: Make a pair of map/unmap reserved pages in error path
>>>   kexec: Do a cleanup for function kexec_load
>>>
>>>  kernel/kexec.c | 112 ++++++++++++++++++++++++++++++++-------------------------
>>>  1 file changed, 63 insertions(+), 49 deletions(-)
>>>
>>
>> _______________________________________________
>> kexec mailing list
>> kexec@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V2 0/2] kexec: Make a pair of map/unmap reserved pages in error path

[Baoquan He]

On 03/23/16 at 11:32am, Xunlei Pang wrote:
> On 2016/03/23 at 10:48, Baoquan He wrote:
> > On 03/01/16 at 05:53pm, Xunlei Pang wrote:
> >> This is a bug fix.
> >>
> >> After this, I will try to do a cleanup for crash_unmap/map_reserved_pages()
> >> (only used by S390) to consolidate it with arch_kexec_unprotect/protect_crashkres().
> > Hi Xunlei, Minfei,
> >
> > I think you need discuss together about how to do clean up codes in this
> > place. From my point of view, arch_map/unmap_reserved_pages and
> > arch_kexec_protect/unprotect_crashkres() are for the same goal but by
> > different ways on different arch. So for Xunlei's patchset, you might
> > need to rethink your implementation, the name of function. I personally
> > think you just implement a x86 specific arch_map/unmap_reserved_pages.
> > It may need a more generic name, and then add your x86 arch specific
> > implementation. Sorry I can't see your patches on my mail client,
> 
> Like what you said, I think arch_kexec_unprotect/protect_crashkres() are
> generic enough, but any other better name is welcome :-)
> 
> It also covered the newly-added kexec file path, and we can easily transfer
> arch_map/unmap_reserved_pages into this new interface.

I don't know the status of your patchset. If possible I think the 1st
patch in your patchset shoule rename arch_map/unmap_reserved_pages to
arch_kexec_protect/unprotect_crashkres, 2nd patch is to add your x86
specific patch.

> 
> I was planning doing that, but sick recently, I will try to send a patch
> doing that later.

Yeah, totally understand. This is not urgent, please take care of
yourself.

> 
> Regards,
> Xunlei
> 
> > Xunlei. Since Andrew asked, I just checked these.
> >
> > I am fine with Minfei's patch 1/2. But for patch 2/2, it's a little
> > comfortable to me. Is it really necessary to abstract code block from
> > kexec_load, then wrap them into a newly added function do_kexec_load()?
> > Without this wrapping is there a way to do your bug fix? Is there
> > possibility that do_kexec_load will be called in other places? What's
> > the benefit to wrap it into do_kexec_load against not wrapping?
> >
> > Thanks
> > Baoquan
> >
> >> Regards,
> >> Xunlei
> >>
> >> On 03/01/2016 at 04:02 PM, Minfei Huang wrote:
> >>> v1:
> >>> - Bisect the patch according to Andrew Morton's suggestion
> >>>
> >>> Minfei Huang (2):
> >>>   kexec: Make a pair of map/unmap reserved pages in error path
> >>>   kexec: Do a cleanup for function kexec_load
> >>>
> >>>  kernel/kexec.c | 112 ++++++++++++++++++++++++++++++++-------------------------
> >>>  1 file changed, 63 insertions(+), 49 deletions(-)
> >>>
> >>
> >> _______________________________________________
> >> kexec mailing list
> >> kexec@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/kexec
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V2 0/2] kexec: Make a pair of map/unmap reserved pages in error path

[Xunlei Pang]

On 2016/03/23 at 16:23, Baoquan He wrote:
> On 03/23/16 at 11:32am, Xunlei Pang wrote:
>> On 2016/03/23 at 10:48, Baoquan He wrote:
>>> On 03/01/16 at 05:53pm, Xunlei Pang wrote:
>>>> This is a bug fix.
>>>>
>>>> After this, I will try to do a cleanup for crash_unmap/map_reserved_pages()
>>>> (only used by S390) to consolidate it with arch_kexec_unprotect/protect_crashkres().
>>> Hi Xunlei, Minfei,
>>>
>>> I think you need discuss together about how to do clean up codes in this
>>> place. From my point of view, arch_map/unmap_reserved_pages and
>>> arch_kexec_protect/unprotect_crashkres() are for the same goal but by
>>> different ways on different arch. So for Xunlei's patchset, you might
>>> need to rethink your implementation, the name of function. I personally
>>> think you just implement a x86 specific arch_map/unmap_reserved_pages.
>>> It may need a more generic name, and then add your x86 arch specific
>>> implementation. Sorry I can't see your patches on my mail client,
>> Like what you said, I think arch_kexec_unprotect/protect_crashkres() are
>> generic enough, but any other better name is welcome :-)
>>
>> It also covered the newly-added kexec file path, and we can easily transfer
>> arch_map/unmap_reserved_pages into this new interface.
> I don't know the status of your patchset. If possible I think the 1st
> patch in your patchset shoule rename arch_map/unmap_reserved_pages to
> arch_kexec_protect/unprotect_crashkres, 2nd patch is to add your x86
> specific patch.

Yes, actually when I filed my patchset, I didn't notice arch_map/unmap_reserved_pages,
too much back then, s390 is its only user, and hard to get the purpose from its name.

But from other point of view, they are a bit different, crash_map_reserved_pages()
is also called by crash_shrink_memory(), it is a bit more complex(and needs some
s390 arch code modification) than just simply renaming/consolidating them, so I think
it's  ok to provide a new generic mechanism first and then put renaming/consolidating
arch work back a little as a separate patch.

Regards,
Xunlei

>
>> I was planning doing that, but sick recently, I will try to send a patch
>> doing that later.
> Yeah, totally understand. This is not urgent, please take care of
> yourself.
>
>> Regards,
>> Xunlei
>>
>>> Xunlei. Since Andrew asked, I just checked these.
>>>
>>> I am fine with Minfei's patch 1/2. But for patch 2/2, it's a little
>>> comfortable to me. Is it really necessary to abstract code block from
>>> kexec_load, then wrap them into a newly added function do_kexec_load()?
>>> Without this wrapping is there a way to do your bug fix? Is there
>>> possibility that do_kexec_load will be called in other places? What's
>>> the benefit to wrap it into do_kexec_load against not wrapping?
>>>
>>> Thanks
>>> Baoquan
>>>
>>>> Regards,
>>>> Xunlei
>>>>
>>>> On 03/01/2016 at 04:02 PM, Minfei Huang wrote:
>>>>> v1:
>>>>> - Bisect the patch according to Andrew Morton's suggestion
>>>>>
>>>>> Minfei Huang (2):
>>>>>   kexec: Make a pair of map/unmap reserved pages in error path
>>>>>   kexec: Do a cleanup for function kexec_load
>>>>>
>>>>>  kernel/kexec.c | 112 ++++++++++++++++++++++++++++++++-------------------------
>>>>>  1 file changed, 63 insertions(+), 49 deletions(-)
>>>>>
>>>> _______________________________________________
>>>> kexec mailing list
>>>> kexec@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/kexec
>>
>> _______________________________________________
>> kexec mailing list
>> kexec@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V2 0/2] kexec: Make a pair of map/unmap reserved pages in error path

[Baoquan He]

On 03/23/16 at 05:59pm, Xunlei Pang wrote:
> On 2016/03/23 at 16:23, Baoquan He wrote:
> > On 03/23/16 at 11:32am, Xunlei Pang wrote:
> >> On 2016/03/23 at 10:48, Baoquan He wrote:
> >>> On 03/01/16 at 05:53pm, Xunlei Pang wrote:
> >>>> This is a bug fix.
> >>>>
> >>>> After this, I will try to do a cleanup for crash_unmap/map_reserved_pages()
> >>>> (only used by S390) to consolidate it with arch_kexec_unprotect/protect_crashkres().
> >>> Hi Xunlei, Minfei,
> >>>
> >>> I think you need discuss together about how to do clean up codes in this
> >>> place. From my point of view, arch_map/unmap_reserved_pages and
> >>> arch_kexec_protect/unprotect_crashkres() are for the same goal but by
> >>> different ways on different arch. So for Xunlei's patchset, you might
> >>> need to rethink your implementation, the name of function. I personally
> >>> think you just implement a x86 specific arch_map/unmap_reserved_pages.
> >>> It may need a more generic name, and then add your x86 arch specific
> >>> implementation. Sorry I can't see your patches on my mail client,
> >> Like what you said, I think arch_kexec_unprotect/protect_crashkres() are
> >> generic enough, but any other better name is welcome :-)
> >>
> >> It also covered the newly-added kexec file path, and we can easily transfer
> >> arch_map/unmap_reserved_pages into this new interface.
> > I don't know the status of your patchset. If possible I think the 1st
> > patch in your patchset shoule rename arch_map/unmap_reserved_pages to
> > arch_kexec_protect/unprotect_crashkres, 2nd patch is to add your x86
> > specific patch.
> 
> Yes, actually when I filed my patchset, I didn't notice arch_map/unmap_reserved_pages,
> too much back then, s390 is its only user, and hard to get the purpose from its name.
> 
> But from other point of view, they are a bit different, crash_map_reserved_pages()
> is also called by crash_shrink_memory(), it is a bit more complex(and needs some
> s390 arch code modification) than just simply renaming/consolidating them, so I think
> it's  ok to provide a new generic mechanism first and then put renaming/consolidating
> arch work back a little as a separate patch.

OK, sounds good, I am fine with this.

How do you think about Minfei's patch? You pick it up into your patchset
in next post with his author, or just wait for him to repost? Apparently
his patch has conflict with yours.

> 
> Regards,
> Xunlei
> 
> >
> >> I was planning doing that, but sick recently, I will try to send a patch
> >> doing that later.
> > Yeah, totally understand. This is not urgent, please take care of
> > yourself.
> >
> >> Regards,
> >> Xunlei
> >>
> >>> Xunlei. Since Andrew asked, I just checked these.
> >>>
> >>> I am fine with Minfei's patch 1/2. But for patch 2/2, it's a little
> >>> comfortable to me. Is it really necessary to abstract code block from
> >>> kexec_load, then wrap them into a newly added function do_kexec_load()?
> >>> Without this wrapping is there a way to do your bug fix? Is there
> >>> possibility that do_kexec_load will be called in other places? What's
> >>> the benefit to wrap it into do_kexec_load against not wrapping?
> >>>
> >>> Thanks
> >>> Baoquan
> >>>
> >>>> Regards,
> >>>> Xunlei
> >>>>
> >>>> On 03/01/2016 at 04:02 PM, Minfei Huang wrote:
> >>>>> v1:
> >>>>> - Bisect the patch according to Andrew Morton's suggestion
> >>>>>
> >>>>> Minfei Huang (2):
> >>>>>   kexec: Make a pair of map/unmap reserved pages in error path
> >>>>>   kexec: Do a cleanup for function kexec_load
> >>>>>
> >>>>>  kernel/kexec.c | 112 ++++++++++++++++++++++++++++++++-------------------------
> >>>>>  1 file changed, 63 insertions(+), 49 deletions(-)
> >>>>>
> >>>> _______________________________________________
> >>>> kexec mailing list
> >>>> kexec@lists.infradead.org
> >>>> http://lists.infradead.org/mailman/listinfo/kexec
> >>
> >> _______________________________________________
> >> kexec mailing list
> >> kexec@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/kexec
> 

Re: [PATCH V2 0/2] kexec: Make a pair of map/unmap reserved pages in error path

[Xunlei Pang]

On 2016/03/23 at 20:32, Baoquan He wrote:
> On 03/23/16 at 05:59pm, Xunlei Pang wrote:
>> On 2016/03/23 at 16:23, Baoquan He wrote:
>>> On 03/23/16 at 11:32am, Xunlei Pang wrote:
>>>> On 2016/03/23 at 10:48, Baoquan He wrote:
>>>>> On 03/01/16 at 05:53pm, Xunlei Pang wrote:
>>>>>> This is a bug fix.
>>>>>>
>>>>>> After this, I will try to do a cleanup for crash_unmap/map_reserved_pages()
>>>>>> (only used by S390) to consolidate it with arch_kexec_unprotect/protect_crashkres().
>>>>> Hi Xunlei, Minfei,
>>>>>
>>>>> I think you need discuss together about how to do clean up codes in this
>>>>> place. From my point of view, arch_map/unmap_reserved_pages and
>>>>> arch_kexec_protect/unprotect_crashkres() are for the same goal but by
>>>>> different ways on different arch. So for Xunlei's patchset, you might
>>>>> need to rethink your implementation, the name of function. I personally
>>>>> think you just implement a x86 specific arch_map/unmap_reserved_pages.
>>>>> It may need a more generic name, and then add your x86 arch specific
>>>>> implementation. Sorry I can't see your patches on my mail client,
>>>> Like what you said, I think arch_kexec_unprotect/protect_crashkres() are
>>>> generic enough, but any other better name is welcome :-)
>>>>
>>>> It also covered the newly-added kexec file path, and we can easily transfer
>>>> arch_map/unmap_reserved_pages into this new interface.
>>> I don't know the status of your patchset. If possible I think the 1st
>>> patch in your patchset shoule rename arch_map/unmap_reserved_pages to
>>> arch_kexec_protect/unprotect_crashkres, 2nd patch is to add your x86
>>> specific patch.
>> Yes, actually when I filed my patchset, I didn't notice arch_map/unmap_reserved_pages,
>> too much back then, s390 is its only user, and hard to get the purpose from its name.
>>
>> But from other point of view, they are a bit different, crash_map_reserved_pages()
>> is also called by crash_shrink_memory(), it is a bit more complex(and needs some
>> s390 arch code modification) than just simply renaming/consolidating them, so I think
>> it's  ok to provide a new generic mechanism first and then put renaming/consolidating
>> arch work back a little as a separate patch.
> OK, sounds good, I am fine with this.
>
> How do you think about Minfei's patch? You pick it up into your patchset
> in next post with his author, or just wait for him to repost? Apparently
> his patch has conflict with yours.

Essentially, Minfei's patchset has nothing to do with mine, his is a bug fix,
I think bugs have priority. Conflicts are commonplace in upstream patches,
should not be a problem. Just my two cents.

Regards,
Xunlei

>
>> Regards,
>> Xunlei
>>
>>>> I was planning doing that, but sick recently, I will try to send a patch
>>>> doing that later.
>>> Yeah, totally understand. This is not urgent, please take care of
>>> yourself.
>>>
>>>> Regards,
>>>> Xunlei
>>>>
>>>>> Xunlei. Since Andrew asked, I just checked these.
>>>>>
>>>>> I am fine with Minfei's patch 1/2. But for patch 2/2, it's a little
>>>>> comfortable to me. Is it really necessary to abstract code block from
>>>>> kexec_load, then wrap them into a newly added function do_kexec_load()?
>>>>> Without this wrapping is there a way to do your bug fix? Is there
>>>>> possibility that do_kexec_load will be called in other places? What's
>>>>> the benefit to wrap it into do_kexec_load against not wrapping?
>>>>>
>>>>> Thanks
>>>>> Baoquan
>>>>>
>>>>>> Regards,
>>>>>> Xunlei
>>>>>>
>>>>>> On 03/01/2016 at 04:02 PM, Minfei Huang wrote:
>>>>>>> v1:
>>>>>>> - Bisect the patch according to Andrew Morton's suggestion
>>>>>>>
>>>>>>> Minfei Huang (2):
>>>>>>>   kexec: Make a pair of map/unmap reserved pages in error path
>>>>>>>   kexec: Do a cleanup for function kexec_load
>>>>>>>
>>>>>>>  kernel/kexec.c | 112 ++++++++++++++++++++++++++++++++-------------------------
>>>>>>>  1 file changed, 63 insertions(+), 49 deletions(-)
>>>>>>>
>>>>>> _______________________________________________
>>>>>> kexec mailing list
>>>>>> kexec@lists.infradead.org
>>>>>> http://lists.infradead.org/mailman/listinfo/kexec
>>>> _______________________________________________
>>>> kexec mailing list
>>>> kexec@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V2 0/2] kexec: Make a pair of map/unmap reserved pages in error path

[Minfei Huang]

On 03/23/16 at 10:48am, Baoquan He wrote:
> On 03/01/16 at 05:53pm, Xunlei Pang wrote:
> > This is a bug fix.
> > 
> > After this, I will try to do a cleanup for crash_unmap/map_reserved_pages()
> > (only used by S390) to consolidate it with arch_kexec_unprotect/protect_crashkres().
> 
> Hi Xunlei, Minfei,
> 
> I think you need discuss together about how to do clean up codes in this
> place. From my point of view, arch_map/unmap_reserved_pages and
> arch_kexec_protect/unprotect_crashkres() are for the same goal but by
> different ways on different arch. So for Xunlei's patchset, you might
> need to rethink your implementation, the name of function. I personally
> think you just implement a x86 specific arch_map/unmap_reserved_pages.
> It may need a more generic name, and then add your x86 arch specific
> implementation. Sorry I can't see your patches on my mail client,
> Xunlei. Since Andrew asked, I just checked these.
> 
> I am fine with Minfei's patch 1/2. But for patch 2/2, it's a little
> comfortable to me. Is it really necessary to abstract code block from
> kexec_load, then wrap them into a newly added function do_kexec_load()?
> Without this wrapping is there a way to do your bug fix? Is there
> possibility that do_kexec_load will be called in other places? What's
> the benefit to wrap it into do_kexec_load against not wrapping?

Hi, Bao.

There is a suggestion from Vivek to wrap a new function do_kexec_load to
fix this issue, since there are a lot of logic handling in function
kexec_load. And this issue doesn't conflict with xlpang@'s patchset,
except for code confliction.

Thanks
Minfei

> 
> Thanks
> Baoquan
> 
> > 
> > Regards,
> > Xunlei
> > 
> > On 03/01/2016 at 04:02 PM, Minfei Huang wrote:
> > > v1:
> > > - Bisect the patch according to Andrew Morton's suggestion
> > >
> > > Minfei Huang (2):
> > >   kexec: Make a pair of map/unmap reserved pages in error path
> > >   kexec: Do a cleanup for function kexec_load
> > >
> > >  kernel/kexec.c | 112 ++++++++++++++++++++++++++++++++-------------------------
> > >  1 file changed, 63 insertions(+), 49 deletions(-)
> > >
> > 
> > 
> > _______________________________________________
> > kexec mailing list
> > kexec@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/kexec