Skip to contentSkip to conversation

kexec: Make a pair of map/unmap reserved pages in error path

PATCH In review Minfei Huang 6 participants 14 messages Mar 1, 2016 – Mar 26, 2016

Subsystems (3)
Live update kexec handover (KHO) kexec
lore
Share & tools
lore

Minfei Huang

author
#0 lore raw

v2 cover kexec: Make a pair of map/unmap reserved pages in error path

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(-)
Signature · 1 line
1.9.1
author
#1 lore raw

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

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 <[email protected]>
---
 kernel/kexec.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
kernel/kexec.c +7 -3
index ee70aef..5cd60c4 100644
@@ -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);
Signature · 1 line
1.9.1
maintainer
#2 lore raw
Quoted 2 lines · On Tue, 1 Mar 2016 16:02:28 +0800 Minfei Huang <[email protected]> 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?

Quoted 3 lines

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.

#3 lore raw
Quoted 6 lines · Andrew Morton
Quoted 2 lines · On Tue, 1 Mar 2016 16:02:28 +0800 Minfei Huang <[email protected]> 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.

Quoted 7 lines
Quoted 4 lines

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

author
#4 lore raw

PATCH v2 2/2 kexec: Do a cleanup for function kexec_load

In reply to 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 <[email protected]>
---
 kernel/kexec.c | 116 +++++++++++++++++++++++++++++++--------------------------
 1 file changed, 63 insertions(+), 53 deletions(-)
kernel/kexec.c +63 -53
index 5cd60c4..48cf69c 100644
@@ -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;
 }
Signature · 1 line
1.9.1
maintainer
#5 lore raw

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

Quoted 7 lines · On Tue, 1 Mar 2016 16:02:29 +0800 Minfei Huang <[email protected]> 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 <[email protected]>
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 <[email protected]>
Cc Vivek Goyal <[email protected]>
Cc "Eric W. Biederman" <[email protected]>
Signed-off-by Andrew Morton <[email protected]>

---

 kernel/kexec.c |  125 +++++++++++++++++++++++++----------------------
 1 file changed, 69 insertions(+), 56 deletions(-)
kernel/kexec.c renamed from kernel/kexec.c~kexec-do-a-cleanup-for-function-kexec_load +69 -56
diff -puN kernel/kexec.c~kexec-do-a-cleanup-for-function-kexec_load 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;
 }

_

#6 lore raw
In reply to Minfei Huang

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

Quoted 10 lines · 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(-)
#7 lore raw
Quoted 4 lines · 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().

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

Quoted 21 lines

Regards,
Xunlei

Quoted 10 lines · 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(-)

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

#8 lore raw
Quoted 7 lines · Baoquan He
Quoted 4 lines · 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().

Hi Xunlei, Minfei,

Quoted 8 lines

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

Quoted 31 lines

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

Quoted 19 lines

Regards,
Xunlei

Quoted 10 lines · 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(-)

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

#9 lore raw
Quoted 17 lines · Xunlei Pang
Quoted 7 lines · Baoquan He
Quoted 4 lines · 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().

Hi Xunlei, Minfei,

Quoted 8 lines

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,

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 41 lines

Regards,
Xunlei

Quoted 9 lines

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?

Quoted 22 lines

Thanks
Baoquan

Quoted 19 lines

Regards,
Xunlei

Quoted 10 lines · 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(-)

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

#10 lore raw
Quoted 20 lines · Baoquan He
Quoted 19 lines · Xunlei Pang
Quoted 7 lines · Baoquan He
Quoted 4 lines · 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().

Hi Xunlei, Minfei,

Quoted 8 lines

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 :-)

Quoted 6 lines
Quoted 2 lines

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

Quoted 44 lines
Quoted 3 lines

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.

Quoted 38 lines

Regards,
Xunlei

Quoted 9 lines

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?

Quoted 21 lines

Thanks
Baoquan

Quoted 18 lines

Regards,
Xunlei

Quoted 10 lines · 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(-)

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

#11 lore raw
Quoted 28 lines · Xunlei Pang
Quoted 20 lines · Baoquan He
Quoted 19 lines · Xunlei Pang
Quoted 7 lines · Baoquan He
Quoted 4 lines · 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().

Hi Xunlei, Minfei,

Quoted 8 lines

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 :-)

Quoted 6 lines
Quoted 2 lines

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.

Quoted 8 lines

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.

Quoted 49 lines

Regards,
Xunlei

Quoted 44 lines
Quoted 3 lines

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.

Quoted 38 lines

Regards,
Xunlei

Quoted 9 lines

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?

Quoted 21 lines

Thanks
Baoquan

Quoted 18 lines

Regards,
Xunlei

Quoted 10 lines · 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(-)

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

#12 lore raw
Quoted 36 lines · Baoquan He
Quoted 27 lines · Xunlei Pang
Quoted 20 lines · Baoquan He
Quoted 19 lines · Xunlei Pang
Quoted 7 lines · Baoquan He
Quoted 4 lines · 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().

Hi Xunlei, Minfei,

Quoted 8 lines

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 :-)

Quoted 6 lines
Quoted 2 lines

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.

Quoted 8 lines

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.

Quoted 5 lines

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

Quoted 46 lines
Quoted 46 lines

Regards,
Xunlei

Quoted 42 lines
Quoted 2 lines

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.

Quoted 37 lines

Regards,
Xunlei

Quoted 9 lines

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?

Quoted 21 lines

Thanks
Baoquan

Quoted 18 lines

Regards,
Xunlei

Quoted 10 lines · 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(-)

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

#13 lore raw
In reply to Baoquan He
Quoted 18 lines · Baoquan He
Quoted 4 lines · 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().

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.

Quoted 6 lines

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

Quoted 25 lines

Thanks
Baoquan

Quoted 21 lines

Regards,
Xunlei

Quoted 10 lines · 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(-)

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec