[PATCH v3 0/7] kexec_file: print out debugging message if required

[PATCH v3 0/7] kexec_file: print out debugging message if required

[Baoquan He]

Currently, specifying '-d' on kexec command will print a lot of debugging
informationabout kexec/kdump loading with kexec_load interface.

However, kexec_file_load prints nothing even though '-d' is specified.
It's very inconvenient to debug or analyze the kexec/kdump loading when
something wrong happened with kexec/kdump itself or develper want to
check the kexec/kdump loading.

In this patchset, a kexec_file flag is KEXEC_FILE_DEBUG added and checked
in code. If it's passed in, debugging message of kexec_file code will be
printed out and can be seen from console and dmesg. Otherwise, the
debugging message is printed like beofre when pr_debug() is taken.

Note:
****
=====
1) The code in kexec-tools utility also need be changed to support
passing KEXEC_FILE_DEBUG to kernel when 'kexec -s -d' is specified.
The patch link is here:
=========
[PATCH] kexec_file: add kexec_file flag to support debug printing
http://lists.infradead.org/pipermail/kexec/2023-November/028505.html

2) s390 also has kexec_file code, while I am not sure what debugging
information is necessary. So leave it to s390 developer.

Test:
****
====
Testing was done in v1 on x86_64 and arm64. For v3, tested on x86_64
again. And on x86_64, the printed messages look like below:
--------------------------------------------------------------
kexec measurement buffer for the loaded kernel at 0x207fffe000.
Loaded purgatory at 0x207fff9000
Loaded boot_param, command line and misc at 0x207fff3000 bufsz=0x1180 memsz=0x1180
Loaded 64bit kernel at 0x207c000000 bufsz=0xc88200 memsz=0x3c4a000
Loaded initrd at 0x2079e79000 bufsz=0x2186280 memsz=0x2186280
Final command line is: root=/dev/mapper/fedora_intel--knightslanding--lb--02-root ro
rd.lvm.lv=fedora_intel-knightslanding-lb-02/root console=ttyS0,115200N81 crashkernel=256M
E820 memmap:
0000000000000000-000000000009a3ff (1)
000000000009a400-000000000009ffff (2)
00000000000e0000-00000000000fffff (2)
0000000000100000-000000006ff83fff (1)
000000006ff84000-000000007ac50fff (2)
......
000000207fff6150-000000207fff615f (128)
000000207fff6160-000000207fff714f (1)
000000207fff7150-000000207fff715f (128)
000000207fff7160-000000207fff814f (1)
000000207fff8150-000000207fff815f (128)
000000207fff8160-000000207fffffff (1)
nr_segments = 5
segment[0]: buf=0x000000004e5ece74 bufsz=0x211 mem=0x207fffe000 memsz=0x1000
segment[1]: buf=0x000000009e871498 bufsz=0x4000 mem=0x207fff9000 memsz=0x5000
segment[2]: buf=0x00000000d879f1fe bufsz=0x1180 mem=0x207fff3000 memsz=0x2000
segment[3]: buf=0x000000001101cd86 bufsz=0xc88200 mem=0x207c000000 memsz=0x3c4a000
segment[4]: buf=0x00000000c6e38ac7 bufsz=0x2186280 mem=0x2079e79000 memsz=0x2187000
kexec_file_load: type:0, start:0x207fff91a0 head:0x109e004002 flags:0x8
---------------------------------------------------------------------------

History:
********
=========
v2->v3:
- Adjust all the indentation of continuation line to the open parenthesis
  for all kexec_dprintk() call sites. Thank Joe to point this out.
- Fix the LKP report that macro kexec_dprintk() is invalid when
  CONFIG_KEXEC=Y, CONFIG_KEXEC_FILE=n, CONFIG_CRASH_DUMP=y.

v1->v2:
- Take the new format of kexec_dprintk() suggested by Joe which can
  reduce kernel text size.
- Fix building error of patch 2 in kernel/crash_core.c reported by LKP.
- Fix building warning on arm64 in patch 4 reported by LKP.

Baoquan He (7):
  kexec_file: add kexec_file flag to control debug printing
  kexec_file: print out debugging message if required
  kexec_file, x86: print out debugging message if required
  kexec_file, arm64: print out debugging message if required
  kexec_file, ricv: print out debugging message if required
  kexec_file, power: print out debugging message if required
  kexec_file, parisc: print out debugging message if required

 arch/arm64/kernel/kexec_image.c        |  6 +++---
 arch/arm64/kernel/machine_kexec.c      | 26 ++++++--------------------
 arch/arm64/kernel/machine_kexec_file.c | 12 ++++++------
 arch/parisc/kernel/kexec_file.c        |  8 ++++----
 arch/powerpc/kexec/elf_64.c            |  8 ++++----
 arch/powerpc/kexec/file_load_64.c      | 18 +++++++++---------
 arch/riscv/kernel/elf_kexec.c          | 11 ++++++-----
 arch/riscv/kernel/machine_kexec.c      | 26 --------------------------
 arch/x86/kernel/crash.c                |  4 ++--
 arch/x86/kernel/kexec-bzimage64.c      | 23 ++++++++++++++---------
 include/linux/kexec.h                  |  9 ++++++++-
 include/uapi/linux/kexec.h             |  1 +
 kernel/crash_core.c                    |  9 ++++++---
 kernel/kexec_core.c                    |  2 ++
 kernel/kexec_file.c                    | 14 +++++++++++---
 security/integrity/ima/ima_kexec.c     |  4 ++--
 16 files changed, 84 insertions(+), 97 deletions(-)

-- 
2.41.0

[PATCH v3 1/7] kexec_file: add kexec_file flag to control debug printing

[Baoquan He]

When specifying 'kexec -c -d', kexec_load interface will print loading
information, e.g the regions where kernel/initrd/purgatory/cmdline
are put, the memmap passed to 2nd kernel taken as system RAM ranges,
and printing all contents of struct kexec_segment, etc. These are
very helpful for analyzing or positioning what's happening when
kexec/kdump itself failed. The debugging printing for kexec_load
interface is made in user space utility kexec-tools.

Whereas, with kexec_file_load interface, 'kexec -s -d' print nothing.
Because kexec_file code is mostly implemented in kernel space, and the
debugging printing functionality is missed. It's not convenient when
debugging kexec/kdump loading and jumping with kexec_file_load
interface.

Now add KEXEC_FILE_DEBUG to kexec_file flag to control the debugging
message printing. And add global variable kexec_file_dbg_print and
macro kexec_dprintk() to facilitate the printing.

This is a preparation, later kexec_dprintk() will be used to replace the
existing pr_debug(). Once 'kexec -s -d' is specified, it will print out
kexec/kdump loading information. If '-d' is not specified, it regresses
to pr_debug().

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 include/linux/kexec.h      | 9 ++++++++-
 include/uapi/linux/kexec.h | 1 +
 kernel/kexec_core.c        | 2 ++
 kernel/kexec_file.c        | 3 +++
 4 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 8227455192b7..400cb6c02176 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -403,7 +403,7 @@ bool kexec_load_permitted(int kexec_image_type);
 
 /* List of defined/legal kexec file flags */
 #define KEXEC_FILE_FLAGS	(KEXEC_FILE_UNLOAD | KEXEC_FILE_ON_CRASH | \
-				 KEXEC_FILE_NO_INITRAMFS)
+				 KEXEC_FILE_NO_INITRAMFS | KEXEC_FILE_DEBUG)
 
 /* flag to track if kexec reboot is in progress */
 extern bool kexec_in_progress;
@@ -500,6 +500,13 @@ static inline int crash_hotplug_memory_support(void) { return 0; }
 static inline unsigned int crash_get_elfcorehdr_size(void) { return 0; }
 #endif
 
+extern bool kexec_file_dbg_print;
+
+#define kexec_dprintk(fmt, ...)					\
+	printk("%s" fmt,					\
+	       kexec_file_dbg_print ? KERN_INFO : KERN_DEBUG,	\
+	       ##__VA_ARGS__)
+
 #else /* !CONFIG_KEXEC_CORE */
 struct pt_regs;
 struct task_struct;
diff --git a/include/uapi/linux/kexec.h b/include/uapi/linux/kexec.h
index 01766dd839b0..c17bb096ea68 100644
--- a/include/uapi/linux/kexec.h
+++ b/include/uapi/linux/kexec.h
@@ -25,6 +25,7 @@
 #define KEXEC_FILE_UNLOAD	0x00000001
 #define KEXEC_FILE_ON_CRASH	0x00000002
 #define KEXEC_FILE_NO_INITRAMFS	0x00000004
+#define KEXEC_FILE_DEBUG	0x00000008
 
 /* These values match the ELF architecture values.
  * Unless there is a good reason that should continue to be the case.
diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index be5642a4ec49..f70bf3a7885c 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -52,6 +52,8 @@ atomic_t __kexec_lock = ATOMIC_INIT(0);
 /* Flag to indicate we are going to kexec a new kernel */
 bool kexec_in_progress = false;
 
+bool kexec_file_dbg_print;
+
 int kexec_should_crash(struct task_struct *p)
 {
 	/*
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index f9a419cd22d4..aca5dac74044 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -123,6 +123,8 @@ void kimage_file_post_load_cleanup(struct kimage *image)
 	 */
 	kfree(image->image_loader_data);
 	image->image_loader_data = NULL;
+
+	kexec_file_dbg_print = false;
 }
 
 #ifdef CONFIG_KEXEC_SIG
@@ -278,6 +280,7 @@ kimage_file_alloc_init(struct kimage **rimage, int kernel_fd,
 	if (!image)
 		return -ENOMEM;
 
+	kexec_file_dbg_print = !!(flags & KEXEC_FILE_DEBUG);
 	image->file_mode = 1;
 
 	if (kexec_on_panic) {
-- 
2.41.0

[PATCH v3 2/7] kexec_file: print out debugging message if required

[Baoquan He]

Replace pr_debug() with the newly added kexec_dprintk() in kexec_file
loading related codes.

And also print out type/start/head of kimage and flags to help debug.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 kernel/crash_core.c                |  9 ++++++---
 kernel/kexec_file.c                | 11 ++++++++---
 security/integrity/ima/ima_kexec.c |  4 ++--
 3 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index efe87d501c8c..b2531eaacd1e 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -551,9 +551,12 @@ int crash_prepare_elf64_headers(struct crash_mem *mem, int need_kernel_map,
 		phdr->p_filesz = phdr->p_memsz = mend - mstart + 1;
 		phdr->p_align = 0;
 		ehdr->e_phnum++;
-		pr_debug("Crash PT_LOAD ELF header. phdr=%p vaddr=0x%llx, paddr=0x%llx, sz=0x%llx e_phnum=%d p_offset=0x%llx\n",
-			phdr, phdr->p_vaddr, phdr->p_paddr, phdr->p_filesz,
-			ehdr->e_phnum, phdr->p_offset);
+#ifdef CONFIG_KEXEC_FILE
+		kexec_dprintk("Crash PT_LOAD ELF header. phdr=%p vaddr=0x%llx, paddr=0x%llx, "
+			      "sz=0x%llx e_phnum=%d p_offset=0x%llx\n",
+			      phdr, phdr->p_vaddr, phdr->p_paddr, phdr->p_filesz,
+			      ehdr->e_phnum, phdr->p_offset);
+#endif
 		phdr++;
 	}
 
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index aca5dac74044..76de1ac7c424 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -204,6 +204,8 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
 	if (ret < 0)
 		return ret;
 	image->kernel_buf_len = ret;
+	kexec_dprintk("kernel: %p kernel_size: %#lx\n",
+		      image->kernel_buf, image->kernel_buf_len);
 
 	/* Call arch image probe handlers */
 	ret = arch_kexec_kernel_image_probe(image, image->kernel_buf,
@@ -387,13 +389,14 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
 	if (ret)
 		goto out;
 
+	kexec_dprintk("nr_segments = %lu\n", image->nr_segments);
 	for (i = 0; i < image->nr_segments; i++) {
 		struct kexec_segment *ksegment;
 
 		ksegment = &image->segment[i];
-		pr_debug("Loading segment %d: buf=0x%p bufsz=0x%zx mem=0x%lx memsz=0x%zx\n",
-			 i, ksegment->buf, ksegment->bufsz, ksegment->mem,
-			 ksegment->memsz);
+		kexec_dprintk("segment[%d]: buf=0x%p bufsz=0x%zx mem=0x%lx memsz=0x%zx\n",
+			      i, ksegment->buf, ksegment->bufsz, ksegment->mem,
+			      ksegment->memsz);
 
 		ret = kimage_load_segment(image, &image->segment[i]);
 		if (ret)
@@ -406,6 +409,8 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
 	if (ret)
 		goto out;
 
+	kexec_dprintk("kexec_file_load: type:%u, start:0x%lx head:0x%lx flags:0x%lx\n",
+		      image->type, image->start, image->head, flags);
 	/*
 	 * Free up any temporary buffers allocated which are not needed
 	 * after image has been loaded
diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
index ad133fe120db..dadc1d138118 100644
--- a/security/integrity/ima/ima_kexec.c
+++ b/security/integrity/ima/ima_kexec.c
@@ -129,8 +129,8 @@ void ima_add_kexec_buffer(struct kimage *image)
 	image->ima_buffer_size = kexec_segment_size;
 	image->ima_buffer = kexec_buffer;
 
-	pr_debug("kexec measurement buffer for the loaded kernel at 0x%lx.\n",
-		 kbuf.mem);
+	kexec_dprintk("kexec measurement buffer for the loaded kernel at 0x%lx.\n",
+		      kbuf.mem);
 }
 #endif /* IMA_KEXEC */
 
-- 
2.41.0

[PATCH v3 3/7] kexec_file, x86: print out debugging message if required

[Baoquan He]

Replace pr_debug() with the newly added kexec_dprintk() in kexec_file
loading related codes.

And also print out e820 memmap passed to 2nd kernel just as kexec_load
interface has been doing.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 arch/x86/kernel/crash.c           |  4 ++--
 arch/x86/kernel/kexec-bzimage64.c | 23 ++++++++++++++---------
 2 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index c92d88680dbf..1715e5f06a59 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -386,8 +386,8 @@ int crash_load_segments(struct kimage *image)
 	if (ret)
 		return ret;
 	image->elf_load_addr = kbuf.mem;
-	pr_debug("Loaded ELF headers at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
-		 image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
+	kexec_dprintk("Loaded ELF headers at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
+		      image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
 
 	return ret;
 }
diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
index a61c12c01270..e9ae0eac6bf9 100644
--- a/arch/x86/kernel/kexec-bzimage64.c
+++ b/arch/x86/kernel/kexec-bzimage64.c
@@ -82,7 +82,7 @@ static int setup_cmdline(struct kimage *image, struct boot_params *params,
 
 	cmdline_ptr[cmdline_len - 1] = '\0';
 
-	pr_debug("Final command line is: %s\n", cmdline_ptr);
+	kexec_dprintk("Final command line is: %s\n", cmdline_ptr);
 	cmdline_ptr_phys = bootparams_load_addr + cmdline_offset;
 	cmdline_low_32 = cmdline_ptr_phys & 0xffffffffUL;
 	cmdline_ext_32 = cmdline_ptr_phys >> 32;
@@ -272,7 +272,12 @@ setup_boot_parameters(struct kimage *image, struct boot_params *params,
 
 	nr_e820_entries = params->e820_entries;
 
+	kexec_dprintk("E820 memmap:\n");
 	for (i = 0; i < nr_e820_entries; i++) {
+		kexec_dprintk("%016llx-%016llx (%d)\n",
+			      params->e820_table[i].addr,
+			      params->e820_table[i].addr + params->e820_table[i].size - 1,
+			      params->e820_table[i].type);
 		if (params->e820_table[i].type != E820_TYPE_RAM)
 			continue;
 		start = params->e820_table[i].addr;
@@ -424,7 +429,7 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
 	 * command line. Make sure it does not overflow
 	 */
 	if (cmdline_len + MAX_ELFCOREHDR_STR_LEN > header->cmdline_size) {
-		pr_debug("Appending elfcorehdr=<addr> to command line exceeds maximum allowed length\n");
+		kexec_dprintk("Appending elfcorehdr=<addr> to command line exceeds maximum allowed length\n");
 		return ERR_PTR(-EINVAL);
 	}
 
@@ -445,7 +450,7 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
 		return ERR_PTR(ret);
 	}
 
-	pr_debug("Loaded purgatory at 0x%lx\n", pbuf.mem);
+	kexec_dprintk("Loaded purgatory at 0x%lx\n", pbuf.mem);
 
 
 	/*
@@ -490,8 +495,8 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
 	if (ret)
 		goto out_free_params;
 	bootparam_load_addr = kbuf.mem;
-	pr_debug("Loaded boot_param, command line and misc at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
-		 bootparam_load_addr, kbuf.bufsz, kbuf.bufsz);
+	kexec_dprintk("Loaded boot_param, command line and misc at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
+		      bootparam_load_addr, kbuf.bufsz, kbuf.bufsz);
 
 	/* Load kernel */
 	kbuf.buffer = kernel + kern16_size;
@@ -505,8 +510,8 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
 		goto out_free_params;
 	kernel_load_addr = kbuf.mem;
 
-	pr_debug("Loaded 64bit kernel at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
-		 kernel_load_addr, kbuf.bufsz, kbuf.memsz);
+	kexec_dprintk("Loaded 64bit kernel at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
+		      kernel_load_addr, kbuf.bufsz, kbuf.memsz);
 
 	/* Load initrd high */
 	if (initrd) {
@@ -520,8 +525,8 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
 			goto out_free_params;
 		initrd_load_addr = kbuf.mem;
 
-		pr_debug("Loaded initrd at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
-				initrd_load_addr, initrd_len, initrd_len);
+		kexec_dprintk("Loaded initrd at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
+			      initrd_load_addr, initrd_len, initrd_len);
 
 		setup_initrd(params, initrd_load_addr, initrd_len);
 	}
-- 
2.41.0

[PATCH v3 4/7] kexec_file, arm64: print out debugging message if required

[Baoquan He]

Replace pr_debug() with the newly added kexec_dprintk() in kexec_file
loading related codes.

And also remove the kimage->segment[] printing because the generic code
has done the printing.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 arch/arm64/kernel/kexec_image.c        |  6 +++---
 arch/arm64/kernel/machine_kexec.c      | 26 ++++++--------------------
 arch/arm64/kernel/machine_kexec_file.c | 12 ++++++------
 3 files changed, 15 insertions(+), 29 deletions(-)

diff --git a/arch/arm64/kernel/kexec_image.c b/arch/arm64/kernel/kexec_image.c
index 636be6715155..532d72ea42ee 100644
--- a/arch/arm64/kernel/kexec_image.c
+++ b/arch/arm64/kernel/kexec_image.c
@@ -122,9 +122,9 @@ static void *image_load(struct kimage *image,
 	kernel_segment->memsz -= text_offset;
 	image->start = kernel_segment->mem;
 
-	pr_debug("Loaded kernel at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
-				kernel_segment->mem, kbuf.bufsz,
-				kernel_segment->memsz);
+	kexec_dprintk("Loaded kernel at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
+		      kernel_segment->mem, kbuf.bufsz,
+		      kernel_segment->memsz);
 
 	return NULL;
 }
diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c
index 078910db77a4..b38aae5b488d 100644
--- a/arch/arm64/kernel/machine_kexec.c
+++ b/arch/arm64/kernel/machine_kexec.c
@@ -32,26 +32,12 @@
 static void _kexec_image_info(const char *func, int line,
 	const struct kimage *kimage)
 {
-	unsigned long i;
-
-	pr_debug("%s:%d:\n", func, line);
-	pr_debug("  kexec kimage info:\n");
-	pr_debug("    type:        %d\n", kimage->type);
-	pr_debug("    start:       %lx\n", kimage->start);
-	pr_debug("    head:        %lx\n", kimage->head);
-	pr_debug("    nr_segments: %lu\n", kimage->nr_segments);
-	pr_debug("    dtb_mem: %pa\n", &kimage->arch.dtb_mem);
-	pr_debug("    kern_reloc: %pa\n", &kimage->arch.kern_reloc);
-	pr_debug("    el2_vectors: %pa\n", &kimage->arch.el2_vectors);
-
-	for (i = 0; i < kimage->nr_segments; i++) {
-		pr_debug("      segment[%lu]: %016lx - %016lx, 0x%lx bytes, %lu pages\n",
-			i,
-			kimage->segment[i].mem,
-			kimage->segment[i].mem + kimage->segment[i].memsz,
-			kimage->segment[i].memsz,
-			kimage->segment[i].memsz /  PAGE_SIZE);
-	}
+	kexec_dprintk("%s:%d:\n", func, line);
+	kexec_dprintk("  kexec kimage info:\n");
+	kexec_dprintk("    type:        %d\n", kimage->type);
+	kexec_dprintk("    head:        %lx\n", kimage->head);
+	kexec_dprintk("    kern_reloc: %pa\n", &kimage->arch.kern_reloc);
+	kexec_dprintk("    el2_vectors: %pa\n", &kimage->arch.el2_vectors);
 }
 
 void machine_kexec_cleanup(struct kimage *kimage)
diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
index a11a6e14ba89..0e017358f4ba 100644
--- a/arch/arm64/kernel/machine_kexec_file.c
+++ b/arch/arm64/kernel/machine_kexec_file.c
@@ -127,8 +127,8 @@ int load_other_segments(struct kimage *image,
 		image->elf_load_addr = kbuf.mem;
 		image->elf_headers_sz = headers_sz;
 
-		pr_debug("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
-			 image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
+		kexec_dprintk("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
+			      image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
 	}
 
 	/* load initrd */
@@ -148,8 +148,8 @@ int load_other_segments(struct kimage *image,
 			goto out_err;
 		initrd_load_addr = kbuf.mem;
 
-		pr_debug("Loaded initrd at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
-				initrd_load_addr, kbuf.bufsz, kbuf.memsz);
+		kexec_dprintk("Loaded initrd at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
+			      initrd_load_addr, kbuf.bufsz, kbuf.memsz);
 	}
 
 	/* load dtb */
@@ -179,8 +179,8 @@ int load_other_segments(struct kimage *image,
 	image->arch.dtb = dtb;
 	image->arch.dtb_mem = kbuf.mem;
 
-	pr_debug("Loaded dtb at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
-			kbuf.mem, kbuf.bufsz, kbuf.memsz);
+	kexec_dprintk("Loaded dtb at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
+		      kbuf.mem, kbuf.bufsz, kbuf.memsz);
 
 	return 0;
 
-- 
2.41.0

[PATCH v3 5/7] kexec_file, ricv: print out debugging message if required

[Baoquan He]

Replace pr_debug() with the newly added kexec_dprintk() in kexec_file
loading related codes.

And also remove kexec_image_info() because the content has been printed
out in generic code.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 arch/riscv/kernel/elf_kexec.c     | 11 ++++++-----
 arch/riscv/kernel/machine_kexec.c | 26 --------------------------
 2 files changed, 6 insertions(+), 31 deletions(-)

diff --git a/arch/riscv/kernel/elf_kexec.c b/arch/riscv/kernel/elf_kexec.c
index e60fbd8660c4..5bd1ec3341fe 100644
--- a/arch/riscv/kernel/elf_kexec.c
+++ b/arch/riscv/kernel/elf_kexec.c
@@ -216,7 +216,6 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
 	if (ret)
 		goto out;
 	kernel_start = image->start;
-	pr_notice("The entry point of kernel at 0x%lx\n", image->start);
 
 	/* Add the kernel binary to the image */
 	ret = riscv_kexec_elf_load(image, &ehdr, &elf_info,
@@ -252,8 +251,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
 		image->elf_load_addr = kbuf.mem;
 		image->elf_headers_sz = headers_sz;
 
-		pr_debug("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
-			 image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
+		kexec_dprintk("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
+			      image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
 
 		/* Setup cmdline for kdump kernel case */
 		modified_cmdline = setup_kdump_cmdline(image, cmdline,
@@ -275,6 +274,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
 		pr_err("Error loading purgatory ret=%d\n", ret);
 		goto out;
 	}
+	kexec_dprintk("Loaded purgatory at 0x%lx\n", kbuf.mem);
+
 	ret = kexec_purgatory_get_set_symbol(image, "riscv_kernel_entry",
 					     &kernel_start,
 					     sizeof(kernel_start), 0);
@@ -293,7 +294,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
 		if (ret)
 			goto out;
 		initrd_pbase = kbuf.mem;
-		pr_notice("Loaded initrd at 0x%lx\n", initrd_pbase);
+		kexec_dprintk("Loaded initrd at 0x%lx\n", initrd_pbase);
 	}
 
 	/* Add the DTB to the image */
@@ -318,7 +319,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
 	}
 	/* Cache the fdt buffer address for memory cleanup */
 	image->arch.fdt = fdt;
-	pr_notice("Loaded device tree at 0x%lx\n", kbuf.mem);
+	kexec_dprintk("Loaded device tree at 0x%lx\n", kbuf.mem);
 	goto out;
 
 out_free_fdt:
diff --git a/arch/riscv/kernel/machine_kexec.c b/arch/riscv/kernel/machine_kexec.c
index 2d139b724bc8..ed9cad20c039 100644
--- a/arch/riscv/kernel/machine_kexec.c
+++ b/arch/riscv/kernel/machine_kexec.c
@@ -18,30 +18,6 @@
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 
-/*
- * kexec_image_info - Print received image details
- */
-static void
-kexec_image_info(const struct kimage *image)
-{
-	unsigned long i;
-
-	pr_debug("Kexec image info:\n");
-	pr_debug("\ttype:        %d\n", image->type);
-	pr_debug("\tstart:       %lx\n", image->start);
-	pr_debug("\thead:        %lx\n", image->head);
-	pr_debug("\tnr_segments: %lu\n", image->nr_segments);
-
-	for (i = 0; i < image->nr_segments; i++) {
-		pr_debug("\t    segment[%lu]: %016lx - %016lx", i,
-			image->segment[i].mem,
-			image->segment[i].mem + image->segment[i].memsz);
-		pr_debug("\t\t0x%lx bytes, %lu pages\n",
-			(unsigned long) image->segment[i].memsz,
-			(unsigned long) image->segment[i].memsz /  PAGE_SIZE);
-	}
-}
-
 /*
  * machine_kexec_prepare - Initialize kexec
  *
@@ -60,8 +36,6 @@ machine_kexec_prepare(struct kimage *image)
 	unsigned int control_code_buffer_sz = 0;
 	int i = 0;
 
-	kexec_image_info(image);
-
 	/* Find the Flattened Device Tree and save its physical address */
 	for (i = 0; i < image->nr_segments; i++) {
 		if (image->segment[i].memsz <= sizeof(fdt))
-- 
2.41.0

[PATCH v3 6/7] kexec_file, power: print out debugging message if required

[Baoquan He]

Replace pr_debug() with the newly added kexec_dprintk() in kexec_file
loading related codes.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 arch/powerpc/kexec/elf_64.c       |  8 ++++----
 arch/powerpc/kexec/file_load_64.c | 18 +++++++++---------
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
index eeb258002d1e..904016cf89ea 100644
--- a/arch/powerpc/kexec/elf_64.c
+++ b/arch/powerpc/kexec/elf_64.c
@@ -59,7 +59,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
 	if (ret)
 		goto out;
 
-	pr_debug("Loaded the kernel at 0x%lx\n", kernel_load_addr);
+	kexec_dprintk("Loaded the kernel at 0x%lx\n", kernel_load_addr);
 
 	ret = kexec_load_purgatory(image, &pbuf);
 	if (ret) {
@@ -67,7 +67,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
 		goto out;
 	}
 
-	pr_debug("Loaded purgatory at 0x%lx\n", pbuf.mem);
+	kexec_dprintk("Loaded purgatory at 0x%lx\n", pbuf.mem);
 
 	/* Load additional segments needed for panic kernel */
 	if (image->type == KEXEC_TYPE_CRASH) {
@@ -99,7 +99,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
 			goto out;
 		initrd_load_addr = kbuf.mem;
 
-		pr_debug("Loaded initrd at 0x%lx\n", initrd_load_addr);
+		kexec_dprintk("Loaded initrd at 0x%lx\n", initrd_load_addr);
 	}
 
 	fdt = of_kexec_alloc_and_setup_fdt(image, initrd_load_addr,
@@ -132,7 +132,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
 
 	fdt_load_addr = kbuf.mem;
 
-	pr_debug("Loaded device tree at 0x%lx\n", fdt_load_addr);
+	kexec_dprintk("Loaded device tree at 0x%lx\n", fdt_load_addr);
 
 	slave_code = elf_info.buffer + elf_info.proghdrs[0].p_offset;
 	ret = setup_purgatory_ppc64(image, slave_code, fdt, kernel_load_addr,
diff --git a/arch/powerpc/kexec/file_load_64.c b/arch/powerpc/kexec/file_load_64.c
index 961a6dd67365..5b4c5cb23354 100644
--- a/arch/powerpc/kexec/file_load_64.c
+++ b/arch/powerpc/kexec/file_load_64.c
@@ -577,7 +577,7 @@ static int add_usable_mem_property(void *fdt, struct device_node *dn,
 		       NODE_PATH_LEN, dn);
 		return -EOVERFLOW;
 	}
-	pr_debug("Memory node path: %s\n", path);
+	kexec_dprintk("Memory node path: %s\n", path);
 
 	/* Now that we know the path, find its offset in kdump kernel's fdt */
 	node = fdt_path_offset(fdt, path);
@@ -590,8 +590,8 @@ static int add_usable_mem_property(void *fdt, struct device_node *dn,
 	/* Get the address & size cells */
 	n_mem_addr_cells = of_n_addr_cells(dn);
 	n_mem_size_cells = of_n_size_cells(dn);
-	pr_debug("address cells: %d, size cells: %d\n", n_mem_addr_cells,
-		 n_mem_size_cells);
+	kexec_dprintk("address cells: %d, size cells: %d\n", n_mem_addr_cells,
+		      n_mem_size_cells);
 
 	um_info->idx  = 0;
 	if (!check_realloc_usable_mem(um_info, 2)) {
@@ -664,7 +664,7 @@ static int update_usable_mem_fdt(void *fdt, struct crash_mem *usable_mem)
 
 	node = fdt_path_offset(fdt, "/ibm,dynamic-reconfiguration-memory");
 	if (node == -FDT_ERR_NOTFOUND)
-		pr_debug("No dynamic reconfiguration memory found\n");
+		kexec_dprintk("No dynamic reconfiguration memory found\n");
 	else if (node < 0) {
 		pr_err("Malformed device tree: error reading /ibm,dynamic-reconfiguration-memory.\n");
 		return -EINVAL;
@@ -776,8 +776,8 @@ static void update_backup_region_phdr(struct kimage *image, Elf64_Ehdr *ehdr)
 	for (i = 0; i < ehdr->e_phnum; i++) {
 		if (phdr->p_paddr == BACKUP_SRC_START) {
 			phdr->p_offset = image->arch.backup_start;
-			pr_debug("Backup region offset updated to 0x%lx\n",
-				 image->arch.backup_start);
+			kexec_dprintk("Backup region offset updated to 0x%lx\n",
+				      image->arch.backup_start);
 			return;
 		}
 	}
@@ -850,7 +850,7 @@ int load_crashdump_segments_ppc64(struct kimage *image,
 		pr_err("Failed to load backup segment\n");
 		return ret;
 	}
-	pr_debug("Loaded the backup region at 0x%lx\n", kbuf->mem);
+	kexec_dprintk("Loaded the backup region at 0x%lx\n", kbuf->mem);
 
 	/* Load elfcorehdr segment - to export crashing kernel's vmcore */
 	ret = load_elfcorehdr_segment(image, kbuf);
@@ -858,8 +858,8 @@ int load_crashdump_segments_ppc64(struct kimage *image,
 		pr_err("Failed to load elfcorehdr segment\n");
 		return ret;
 	}
-	pr_debug("Loaded elf core header at 0x%lx, bufsz=0x%lx memsz=0x%lx\n",
-		 image->elf_load_addr, kbuf->bufsz, kbuf->memsz);
+	kexec_dprintk("Loaded elf core header at 0x%lx, bufsz=0x%lx memsz=0x%lx\n",
+		      image->elf_load_addr, kbuf->bufsz, kbuf->memsz);
 
 	return 0;
 }
-- 
2.41.0

[PATCH v3 7/7] kexec_file, parisc: print out debugging message if required

[Baoquan He]

Replace pr_debug() with the newly added kexec_dprintk() in kexec_file
loading related codes.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 arch/parisc/kernel/kexec_file.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/parisc/kernel/kexec_file.c b/arch/parisc/kernel/kexec_file.c
index 8c534204f0fd..3fc82130b6c3 100644
--- a/arch/parisc/kernel/kexec_file.c
+++ b/arch/parisc/kernel/kexec_file.c
@@ -38,8 +38,8 @@ static void *elf_load(struct kimage *image, char *kernel_buf,
 	for (i = 0; i < image->nr_segments; i++)
 		image->segment[i].mem = __pa(image->segment[i].mem);
 
-	pr_debug("Loaded the kernel at 0x%lx, entry at 0x%lx\n",
-		 kernel_load_addr, image->start);
+	kexec_dprintk("Loaded the kernel at 0x%lx, entry at 0x%lx\n",
+		      kernel_load_addr, image->start);
 
 	if (initrd != NULL) {
 		kbuf.buffer = initrd;
@@ -51,7 +51,7 @@ static void *elf_load(struct kimage *image, char *kernel_buf,
 		if (ret)
 			goto out;
 
-		pr_debug("Loaded initrd at 0x%lx\n", kbuf.mem);
+		kexec_dprintk("Loaded initrd at 0x%lx\n", kbuf.mem);
 		image->arch.initrd_start = kbuf.mem;
 		image->arch.initrd_end = kbuf.mem + initrd_len;
 	}
@@ -68,7 +68,7 @@ static void *elf_load(struct kimage *image, char *kernel_buf,
 		if (ret)
 			goto out;
 
-		pr_debug("Loaded cmdline at 0x%lx\n", kbuf.mem);
+		kexec_dprintk("Loaded cmdline at 0x%lx\n", kbuf.mem);
 		image->arch.cmdline = kbuf.mem;
 	}
 out:
-- 
2.41.0

Re: [PATCH v3 2/7] kexec_file: print out debugging message if required

[Joe Perches]

On Thu, 2023-11-30 at 10:39 +0800, Baoquan He wrote:
> Replace pr_debug() with the newly added kexec_dprintk() in kexec_file
> loading related codes.

trivia:

> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
[]
> @@ -551,9 +551,12 @@ int crash_prepare_elf64_headers(struct crash_mem *mem, int need_kernel_map,
>  		phdr->p_filesz = phdr->p_memsz = mend - mstart + 1;
>  		phdr->p_align = 0;
>  		ehdr->e_phnum++;
> -		pr_debug("Crash PT_LOAD ELF header. phdr=%p vaddr=0x%llx, paddr=0x%llx, sz=0x%llx e_phnum=%d p_offset=0x%llx\n",
> -			phdr, phdr->p_vaddr, phdr->p_paddr, phdr->p_filesz,
> -			ehdr->e_phnum, phdr->p_offset);
> +#ifdef CONFIG_KEXEC_FILE
> +		kexec_dprintk("Crash PT_LOAD ELF header. phdr=%p vaddr=0x%llx, paddr=0x%llx, "
> +			      "sz=0x%llx e_phnum=%d p_offset=0x%llx\n",
> +			      phdr, phdr->p_vaddr, phdr->p_paddr, phdr->p_filesz,
> +			      ehdr->e_phnum, phdr->p_offset);
> +#endif

Perhaps run checkpatch and coalesce the format string.

Re: [PATCH v3 5/7] kexec_file, ricv: print out debugging message if required

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4475 bytes --]

On Thu, Nov 30, 2023 at 10:39:53AM +0800, Baoquan He wrote:

$subject has a typo in the arch bit :)

> Replace pr_debug() with the newly added kexec_dprintk() in kexec_file
> loading related codes.

Commit messages should be understandable in isolation, but this only
explains (part of) what is obvious in the diff. Why is this change
being made?

> 
> And also remove kexec_image_info() because the content has been printed
> out in generic code.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  arch/riscv/kernel/elf_kexec.c     | 11 ++++++-----
>  arch/riscv/kernel/machine_kexec.c | 26 --------------------------
>  2 files changed, 6 insertions(+), 31 deletions(-)
> 
> diff --git a/arch/riscv/kernel/elf_kexec.c b/arch/riscv/kernel/elf_kexec.c
> index e60fbd8660c4..5bd1ec3341fe 100644
> --- a/arch/riscv/kernel/elf_kexec.c
> +++ b/arch/riscv/kernel/elf_kexec.c
> @@ -216,7 +216,6 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
>  	if (ret)
>  		goto out;
>  	kernel_start = image->start;
> -	pr_notice("The entry point of kernel at 0x%lx\n", image->start);
>  
>  	/* Add the kernel binary to the image */
>  	ret = riscv_kexec_elf_load(image, &ehdr, &elf_info,
> @@ -252,8 +251,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
>  		image->elf_load_addr = kbuf.mem;
>  		image->elf_headers_sz = headers_sz;
>  
> -		pr_debug("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
> -			 image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
> +		kexec_dprintk("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
> +			      image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
>  
>  		/* Setup cmdline for kdump kernel case */
>  		modified_cmdline = setup_kdump_cmdline(image, cmdline,
> @@ -275,6 +274,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
>  		pr_err("Error loading purgatory ret=%d\n", ret);
>  		goto out;
>  	}
> +	kexec_dprintk("Loaded purgatory at 0x%lx\n", kbuf.mem);
> +
>  	ret = kexec_purgatory_get_set_symbol(image, "riscv_kernel_entry",
>  					     &kernel_start,
>  					     sizeof(kernel_start), 0);
> @@ -293,7 +294,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
>  		if (ret)
>  			goto out;
>  		initrd_pbase = kbuf.mem;

> -		pr_notice("Loaded initrd at 0x%lx\n", initrd_pbase);
> +		kexec_dprintk("Loaded initrd at 0x%lx\n", initrd_pbase);

This is not a pr_debug().

>  	}
>  
>  	/* Add the DTB to the image */
> @@ -318,7 +319,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
>  	}
>  	/* Cache the fdt buffer address for memory cleanup */
>  	image->arch.fdt = fdt;

> -	pr_notice("Loaded device tree at 0x%lx\n", kbuf.mem);
> +	kexec_dprintk("Loaded device tree at 0x%lx\n", kbuf.mem);

Neither is this. Why are they being moved from pr_notice()?

Thanks,
Conor.

>  	goto out;
>  
>  out_free_fdt:
> diff --git a/arch/riscv/kernel/machine_kexec.c b/arch/riscv/kernel/machine_kexec.c
> index 2d139b724bc8..ed9cad20c039 100644
> --- a/arch/riscv/kernel/machine_kexec.c
> +++ b/arch/riscv/kernel/machine_kexec.c
> @@ -18,30 +18,6 @@
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
>  
> -/*
> - * kexec_image_info - Print received image details
> - */
> -static void
> -kexec_image_info(const struct kimage *image)
> -{
> -	unsigned long i;
> -
> -	pr_debug("Kexec image info:\n");
> -	pr_debug("\ttype:        %d\n", image->type);
> -	pr_debug("\tstart:       %lx\n", image->start);
> -	pr_debug("\thead:        %lx\n", image->head);
> -	pr_debug("\tnr_segments: %lu\n", image->nr_segments);
> -
> -	for (i = 0; i < image->nr_segments; i++) {
> -		pr_debug("\t    segment[%lu]: %016lx - %016lx", i,
> -			image->segment[i].mem,
> -			image->segment[i].mem + image->segment[i].memsz);
> -		pr_debug("\t\t0x%lx bytes, %lu pages\n",
> -			(unsigned long) image->segment[i].memsz,
> -			(unsigned long) image->segment[i].memsz /  PAGE_SIZE);
> -	}
> -}
> -
>  /*
>   * machine_kexec_prepare - Initialize kexec
>   *
> @@ -60,8 +36,6 @@ machine_kexec_prepare(struct kimage *image)
>  	unsigned int control_code_buffer_sz = 0;
>  	int i = 0;
>  
> -	kexec_image_info(image);
> -
>  	/* Find the Flattened Device Tree and save its physical address */
>  	for (i = 0; i < image->nr_segments; i++) {
>  		if (image->segment[i].memsz <= sizeof(fdt))
> -- 
> 2.41.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 2/7] kexec_file: print out debugging message if required

[Baoquan He]

On 11/29/23 at 06:52pm, Joe Perches wrote:
> On Thu, 2023-11-30 at 10:39 +0800, Baoquan He wrote:
> > Replace pr_debug() with the newly added kexec_dprintk() in kexec_file
> > loading related codes.
> 
> trivia:
> 
> > diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> []
> > @@ -551,9 +551,12 @@ int crash_prepare_elf64_headers(struct crash_mem *mem, int need_kernel_map,
> >  		phdr->p_filesz = phdr->p_memsz = mend - mstart + 1;
> >  		phdr->p_align = 0;
> >  		ehdr->e_phnum++;
> > -		pr_debug("Crash PT_LOAD ELF header. phdr=%p vaddr=0x%llx, paddr=0x%llx, sz=0x%llx e_phnum=%d p_offset=0x%llx\n",
> > -			phdr, phdr->p_vaddr, phdr->p_paddr, phdr->p_filesz,
> > -			ehdr->e_phnum, phdr->p_offset);
> > +#ifdef CONFIG_KEXEC_FILE
> > +		kexec_dprintk("Crash PT_LOAD ELF header. phdr=%p vaddr=0x%llx, paddr=0x%llx, "
> > +			      "sz=0x%llx e_phnum=%d p_offset=0x%llx\n",
> > +			      phdr, phdr->p_vaddr, phdr->p_paddr, phdr->p_filesz,
> > +			      ehdr->e_phnum, phdr->p_offset);
> > +#endif
> 
> Perhaps run checkpatch and coalesce the format string.

Sorry for being late to reply, this comment was missed.

I broke it into two because it's a too long line and not friendly for
reading. I did notice there's such line breaking code. I can change it
if it's not suggested. Thanks for careful checking.

Re: [PATCH v3 5/7] kexec_file, ricv: print out debugging message if required

[Baoquan He]

On 12/01/23 at 10:38am, Conor Dooley wrote:
> On Thu, Nov 30, 2023 at 10:39:53AM +0800, Baoquan He wrote:
> 
> $subject has a typo in the arch bit :)

Indeed, will fix if need report. Thanks for careful checking.

> 
> > Replace pr_debug() with the newly added kexec_dprintk() in kexec_file
> > loading related codes.
> 
> Commit messages should be understandable in isolation, but this only
> explains (part of) what is obvious in the diff. Why is this change
> being made?

The purpose has been detailedly described in cover letter and patch 1
log. Andrew has picked these patches into his tree and grabbed the cover
letter log into the relevant commit for people's later checking. All
these seven patches will be present in mainline together. This is common
way when posting patch series? Please let me know if I misunderstand
anything.
> 
> > 
> > And also remove kexec_image_info() because the content has been printed
> > out in generic code.
> > 
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > ---
> >  arch/riscv/kernel/elf_kexec.c     | 11 ++++++-----
> >  arch/riscv/kernel/machine_kexec.c | 26 --------------------------
> >  2 files changed, 6 insertions(+), 31 deletions(-)
> > 
> > diff --git a/arch/riscv/kernel/elf_kexec.c b/arch/riscv/kernel/elf_kexec.c
> > index e60fbd8660c4..5bd1ec3341fe 100644
> > --- a/arch/riscv/kernel/elf_kexec.c
> > +++ b/arch/riscv/kernel/elf_kexec.c
> > @@ -216,7 +216,6 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> >  	if (ret)
> >  		goto out;
> >  	kernel_start = image->start;
> > -	pr_notice("The entry point of kernel at 0x%lx\n", image->start);
> >  
> >  	/* Add the kernel binary to the image */
> >  	ret = riscv_kexec_elf_load(image, &ehdr, &elf_info,
> > @@ -252,8 +251,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> >  		image->elf_load_addr = kbuf.mem;
> >  		image->elf_headers_sz = headers_sz;
> >  
> > -		pr_debug("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
> > -			 image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
> > +		kexec_dprintk("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
> > +			      image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
> >  
> >  		/* Setup cmdline for kdump kernel case */
> >  		modified_cmdline = setup_kdump_cmdline(image, cmdline,
> > @@ -275,6 +274,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> >  		pr_err("Error loading purgatory ret=%d\n", ret);
> >  		goto out;
> >  	}
> > +	kexec_dprintk("Loaded purgatory at 0x%lx\n", kbuf.mem);
> > +
> >  	ret = kexec_purgatory_get_set_symbol(image, "riscv_kernel_entry",
> >  					     &kernel_start,
> >  					     sizeof(kernel_start), 0);
> > @@ -293,7 +294,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> >  		if (ret)
> >  			goto out;
> >  		initrd_pbase = kbuf.mem;
> 
> > -		pr_notice("Loaded initrd at 0x%lx\n", initrd_pbase);
> > +		kexec_dprintk("Loaded initrd at 0x%lx\n", initrd_pbase);
> 
> This is not a pr_debug().
> 
> >  	}
> >  
> >  	/* Add the DTB to the image */
> > @@ -318,7 +319,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> >  	}
> >  	/* Cache the fdt buffer address for memory cleanup */
> >  	image->arch.fdt = fdt;
> 
> > -	pr_notice("Loaded device tree at 0x%lx\n", kbuf.mem);
> > +	kexec_dprintk("Loaded device tree at 0x%lx\n", kbuf.mem);
> 
> Neither is this. Why are they being moved from pr_notice()?

You are right. 

While always printing out the loaded location of purgatory and
device tree doesn't make sense. It will be confusing when users
see these even when they do normal kexec/kdump loading. It should be
changed to pr_debug().

Which way do you suggest?
1) change it back to pr_debug(), fix it in another patch;
2) keep it as is in the patch;

Thanks
Baoquan

Re: [PATCH v3 5/7] kexec_file, ricv: print out debugging message if required

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4599 bytes --]

On Mon, Dec 04, 2023 at 11:38:05PM +0800, Baoquan He wrote:
> On 12/01/23 at 10:38am, Conor Dooley wrote:
> > On Thu, Nov 30, 2023 at 10:39:53AM +0800, Baoquan He wrote:
> > 
> > $subject has a typo in the arch bit :)
> 
> Indeed, will fix if need report. Thanks for careful checking.
> 
> > 
> > > Replace pr_debug() with the newly added kexec_dprintk() in kexec_file
> > > loading related codes.
> > 
> > Commit messages should be understandable in isolation, but this only
> > explains (part of) what is obvious in the diff. Why is this change
> > being made?
> 
> The purpose has been detailedly described in cover letter and patch 1
> log. Andrew has picked these patches into his tree and grabbed the cover
> letter log into the relevant commit for people's later checking. All
> these seven patches will be present in mainline together. This is common
> way when posting patch series? Please let me know if I misunderstand
> anything.

Each patch having a commit message that explains why a change is being
made is the expectation. It is especially useful to explain the why
here, since it is not just a mechanical conversion of pr_debug()s as the
commit message suggests.

> > 
> > > 
> > > And also remove kexec_image_info() because the content has been printed
> > > out in generic code.
> > > 
> > > Signed-off-by: Baoquan He <bhe@redhat.com>
> > > ---
> > >  arch/riscv/kernel/elf_kexec.c     | 11 ++++++-----
> > >  arch/riscv/kernel/machine_kexec.c | 26 --------------------------
> > >  2 files changed, 6 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/arch/riscv/kernel/elf_kexec.c b/arch/riscv/kernel/elf_kexec.c
> > > index e60fbd8660c4..5bd1ec3341fe 100644
> > > --- a/arch/riscv/kernel/elf_kexec.c
> > > +++ b/arch/riscv/kernel/elf_kexec.c
> > > @@ -216,7 +216,6 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> > >  	if (ret)
> > >  		goto out;
> > >  	kernel_start = image->start;
> > > -	pr_notice("The entry point of kernel at 0x%lx\n", image->start);
> > >  
> > >  	/* Add the kernel binary to the image */
> > >  	ret = riscv_kexec_elf_load(image, &ehdr, &elf_info,
> > > @@ -252,8 +251,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> > >  		image->elf_load_addr = kbuf.mem;
> > >  		image->elf_headers_sz = headers_sz;
> > >  
> > > -		pr_debug("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
> > > -			 image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
> > > +		kexec_dprintk("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
> > > +			      image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
> > >  
> > >  		/* Setup cmdline for kdump kernel case */
> > >  		modified_cmdline = setup_kdump_cmdline(image, cmdline,
> > > @@ -275,6 +274,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> > >  		pr_err("Error loading purgatory ret=%d\n", ret);
> > >  		goto out;
> > >  	}
> > > +	kexec_dprintk("Loaded purgatory at 0x%lx\n", kbuf.mem);
> > > +
> > >  	ret = kexec_purgatory_get_set_symbol(image, "riscv_kernel_entry",
> > >  					     &kernel_start,
> > >  					     sizeof(kernel_start), 0);
> > > @@ -293,7 +294,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> > >  		if (ret)
> > >  			goto out;
> > >  		initrd_pbase = kbuf.mem;
> > 
> > > -		pr_notice("Loaded initrd at 0x%lx\n", initrd_pbase);
> > > +		kexec_dprintk("Loaded initrd at 0x%lx\n", initrd_pbase);
> > 
> > This is not a pr_debug().
> > 
> > >  	}
> > >  
> > >  	/* Add the DTB to the image */
> > > @@ -318,7 +319,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> > >  	}
> > >  	/* Cache the fdt buffer address for memory cleanup */
> > >  	image->arch.fdt = fdt;
> > 
> > > -	pr_notice("Loaded device tree at 0x%lx\n", kbuf.mem);
> > > +	kexec_dprintk("Loaded device tree at 0x%lx\n", kbuf.mem);
> > 
> > Neither is this. Why are they being moved from pr_notice()?
> 
> You are right. 
> 
> While always printing out the loaded location of purgatory and
> device tree doesn't make sense. It will be confusing when users
> see these even when they do normal kexec/kdump loading. It should be
> changed to pr_debug().
> 
> Which way do you suggest?
> 1) change it back to pr_debug(), fix it in another patch;
> 2) keep it as is in the patch;

Personally I think it is fine to change them all in one patch, but the
rationale for converting pr_notice() to your new debug infrastructure
needs to be in the commit message.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 5/7] kexec_file, ricv: print out debugging message if required

[Baoquan He]

On 12/04/23 at 04:14pm, Conor Dooley wrote:
> On Mon, Dec 04, 2023 at 11:38:05PM +0800, Baoquan He wrote:
> > On 12/01/23 at 10:38am, Conor Dooley wrote:
> > > On Thu, Nov 30, 2023 at 10:39:53AM +0800, Baoquan He wrote:
> > > 
> > > $subject has a typo in the arch bit :)
> > 
> > Indeed, will fix if need report. Thanks for careful checking.
> > 
> > > 
> > > > Replace pr_debug() with the newly added kexec_dprintk() in kexec_file
> > > > loading related codes.
> > > 
> > > Commit messages should be understandable in isolation, but this only
> > > explains (part of) what is obvious in the diff. Why is this change
> > > being made?
> > 
> > The purpose has been detailedly described in cover letter and patch 1
> > log. Andrew has picked these patches into his tree and grabbed the cover
> > letter log into the relevant commit for people's later checking. All
> > these seven patches will be present in mainline together. This is common
> > way when posting patch series? Please let me know if I misunderstand
> > anything.
> 
> Each patch having a commit message that explains why a change is being
> made is the expectation. It is especially useful to explain the why
> here, since it is not just a mechanical conversion of pr_debug()s as the
> commit message suggests.

Sounds reasonable. I rephrase the patch 3 log as below, do you think
it's OK to you?

I will also adjust patch logs on other ARCH once this one is done.
Thanks.

=============================
Subject: [PATCH v3 5/7] kexec_file, ricv: print out debugging message if required

Then when specifying '-d' for kexec_file_load interface, loaded
locations of kernel/initrd/cmdline etc can be printed out to help debug.

Here replace pr_debug() with the newly added kexec_dprintk() in kexec_file
loading related codes.

And also replace pr_notice() with kexec_dprintk() in elf_kexec_load()
because it's make sense to always print out loaded location of purgatory
and device tree even though users don't expect the message.

And also remove kexec_image_info() because the content has been printed
out in generic code.

============================

> 
> > > 
> > > > 
> > > > And also remove kexec_image_info() because the content has been printed
> > > > out in generic code.
> > > > 
> > > > Signed-off-by: Baoquan He <bhe@redhat.com>
> > > > ---
> > > >  arch/riscv/kernel/elf_kexec.c     | 11 ++++++-----
> > > >  arch/riscv/kernel/machine_kexec.c | 26 --------------------------
> > > >  2 files changed, 6 insertions(+), 31 deletions(-)
> > > > 
> > > > diff --git a/arch/riscv/kernel/elf_kexec.c b/arch/riscv/kernel/elf_kexec.c
> > > > index e60fbd8660c4..5bd1ec3341fe 100644
> > > > --- a/arch/riscv/kernel/elf_kexec.c
> > > > +++ b/arch/riscv/kernel/elf_kexec.c
> > > > @@ -216,7 +216,6 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> > > >  	if (ret)
> > > >  		goto out;
> > > >  	kernel_start = image->start;
> > > > -	pr_notice("The entry point of kernel at 0x%lx\n", image->start);
> > > >  
> > > >  	/* Add the kernel binary to the image */
> > > >  	ret = riscv_kexec_elf_load(image, &ehdr, &elf_info,
> > > > @@ -252,8 +251,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> > > >  		image->elf_load_addr = kbuf.mem;
> > > >  		image->elf_headers_sz = headers_sz;
> > > >  
> > > > -		pr_debug("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
> > > > -			 image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
> > > > +		kexec_dprintk("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
> > > > +			      image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
> > > >  
> > > >  		/* Setup cmdline for kdump kernel case */
> > > >  		modified_cmdline = setup_kdump_cmdline(image, cmdline,
> > > > @@ -275,6 +274,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> > > >  		pr_err("Error loading purgatory ret=%d\n", ret);
> > > >  		goto out;
> > > >  	}
> > > > +	kexec_dprintk("Loaded purgatory at 0x%lx\n", kbuf.mem);
> > > > +
> > > >  	ret = kexec_purgatory_get_set_symbol(image, "riscv_kernel_entry",
> > > >  					     &kernel_start,
> > > >  					     sizeof(kernel_start), 0);
> > > > @@ -293,7 +294,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> > > >  		if (ret)
> > > >  			goto out;
> > > >  		initrd_pbase = kbuf.mem;
> > > 
> > > > -		pr_notice("Loaded initrd at 0x%lx\n", initrd_pbase);
> > > > +		kexec_dprintk("Loaded initrd at 0x%lx\n", initrd_pbase);
> > > 
> > > This is not a pr_debug().
> > > 
> > > >  	}
> > > >  
> > > >  	/* Add the DTB to the image */
> > > > @@ -318,7 +319,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> > > >  	}
> > > >  	/* Cache the fdt buffer address for memory cleanup */
> > > >  	image->arch.fdt = fdt;
> > > 
> > > > -	pr_notice("Loaded device tree at 0x%lx\n", kbuf.mem);
> > > > +	kexec_dprintk("Loaded device tree at 0x%lx\n", kbuf.mem);
> > > 
> > > Neither is this. Why are they being moved from pr_notice()?
> > 
> > You are right. 
> > 
> > While always printing out the loaded location of purgatory and
> > device tree doesn't make sense. It will be confusing when users
> > see these even when they do normal kexec/kdump loading. It should be
> > changed to pr_debug().
> > 
> > Which way do you suggest?
> > 1) change it back to pr_debug(), fix it in another patch;
> > 2) keep it as is in the patch;
> 
> Personally I think it is fine to change them all in one patch, but the
> rationale for converting pr_notice() to your new debug infrastructure
> needs to be in the commit message.

Sure, sounds good to me. I have changed the patch log to reflect this as
you suggested, please help check. Thanks again.

Re: [PATCH v3 5/7] kexec_file, ricv: print out debugging message if required

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 6665 bytes --]

On Wed, Dec 06, 2023 at 11:37:52PM +0800, Baoquan He wrote:
> On 12/04/23 at 04:14pm, Conor Dooley wrote:
> > On Mon, Dec 04, 2023 at 11:38:05PM +0800, Baoquan He wrote:
> > > On 12/01/23 at 10:38am, Conor Dooley wrote:
> > > > On Thu, Nov 30, 2023 at 10:39:53AM +0800, Baoquan He wrote:
> > > > 
> > > > $subject has a typo in the arch bit :)
> > > 
> > > Indeed, will fix if need report. Thanks for careful checking.
> > > 
> > > > 
> > > > > Replace pr_debug() with the newly added kexec_dprintk() in kexec_file
> > > > > loading related codes.
> > > > 
> > > > Commit messages should be understandable in isolation, but this only
> > > > explains (part of) what is obvious in the diff. Why is this change
> > > > being made?
> > > 
> > > The purpose has been detailedly described in cover letter and patch 1
> > > log. Andrew has picked these patches into his tree and grabbed the cover
> > > letter log into the relevant commit for people's later checking. All
> > > these seven patches will be present in mainline together. This is common
> > > way when posting patch series? Please let me know if I misunderstand
> > > anything.
> > 
> > Each patch having a commit message that explains why a change is being
> > made is the expectation. It is especially useful to explain the why
> > here, since it is not just a mechanical conversion of pr_debug()s as the
> > commit message suggests.
> 
> Sounds reasonable. I rephrase the patch 3 log as below, do you think
> it's OK to you?

Yes, but with one comment.

> 
> I will also adjust patch logs on other ARCH once this one is done.
> Thanks.
> 
> =============================
> Subject: [PATCH v3 5/7] kexec_file, ricv: print out debugging message if required
> 
> Then when specifying '-d' for kexec_file_load interface, loaded
> locations of kernel/initrd/cmdline etc can be printed out to help debug.
> 
> Here replace pr_debug() with the newly added kexec_dprintk() in kexec_file
> loading related codes.
> 

> And also replace pr_notice() with kexec_dprintk() in elf_kexec_load()
> because it's make sense to always print out loaded location of purgatory
> and device tree even though users don't expect the message.

This seems to contradict what you said in your earlier mail, about
moving these from notice to debug. I think you missed a negation in your
new version of the commit message. What you said in response to me seems
like a more complete explanation anyway:
	always printing out the loaded location of purgatory and
	device tree doesn't make sense. It will be confusing when users
	see these even when they do normal kexec/kdump loading.

Thanks,
Conor.

> And also remove kexec_image_info() because the content has been printed
> out in generic code.
> 
> ============================
> 
> > 
> > > > 
> > > > > 
> > > > > And also remove kexec_image_info() because the content has been printed
> > > > > out in generic code.
> > > > > 
> > > > > Signed-off-by: Baoquan He <bhe@redhat.com>
> > > > > ---
> > > > >  arch/riscv/kernel/elf_kexec.c     | 11 ++++++-----
> > > > >  arch/riscv/kernel/machine_kexec.c | 26 --------------------------
> > > > >  2 files changed, 6 insertions(+), 31 deletions(-)
> > > > > 
> > > > > diff --git a/arch/riscv/kernel/elf_kexec.c b/arch/riscv/kernel/elf_kexec.c
> > > > > index e60fbd8660c4..5bd1ec3341fe 100644
> > > > > --- a/arch/riscv/kernel/elf_kexec.c
> > > > > +++ b/arch/riscv/kernel/elf_kexec.c
> > > > > @@ -216,7 +216,6 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> > > > >  	if (ret)
> > > > >  		goto out;
> > > > >  	kernel_start = image->start;
> > > > > -	pr_notice("The entry point of kernel at 0x%lx\n", image->start);
> > > > >  
> > > > >  	/* Add the kernel binary to the image */
> > > > >  	ret = riscv_kexec_elf_load(image, &ehdr, &elf_info,
> > > > > @@ -252,8 +251,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> > > > >  		image->elf_load_addr = kbuf.mem;
> > > > >  		image->elf_headers_sz = headers_sz;
> > > > >  
> > > > > -		pr_debug("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
> > > > > -			 image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
> > > > > +		kexec_dprintk("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
> > > > > +			      image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
> > > > >  
> > > > >  		/* Setup cmdline for kdump kernel case */
> > > > >  		modified_cmdline = setup_kdump_cmdline(image, cmdline,
> > > > > @@ -275,6 +274,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> > > > >  		pr_err("Error loading purgatory ret=%d\n", ret);
> > > > >  		goto out;
> > > > >  	}
> > > > > +	kexec_dprintk("Loaded purgatory at 0x%lx\n", kbuf.mem);
> > > > > +
> > > > >  	ret = kexec_purgatory_get_set_symbol(image, "riscv_kernel_entry",
> > > > >  					     &kernel_start,
> > > > >  					     sizeof(kernel_start), 0);
> > > > > @@ -293,7 +294,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> > > > >  		if (ret)
> > > > >  			goto out;
> > > > >  		initrd_pbase = kbuf.mem;
> > > > 
> > > > > -		pr_notice("Loaded initrd at 0x%lx\n", initrd_pbase);
> > > > > +		kexec_dprintk("Loaded initrd at 0x%lx\n", initrd_pbase);
> > > > 
> > > > This is not a pr_debug().
> > > > 
> > > > >  	}
> > > > >  
> > > > >  	/* Add the DTB to the image */
> > > > > @@ -318,7 +319,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> > > > >  	}
> > > > >  	/* Cache the fdt buffer address for memory cleanup */
> > > > >  	image->arch.fdt = fdt;
> > > > 
> > > > > -	pr_notice("Loaded device tree at 0x%lx\n", kbuf.mem);
> > > > > +	kexec_dprintk("Loaded device tree at 0x%lx\n", kbuf.mem);
> > > > 
> > > > Neither is this. Why are they being moved from pr_notice()?
> > > 
> > > You are right. 
> > > 
> > > While always printing out the loaded location of purgatory and
> > > device tree doesn't make sense. It will be confusing when users
> > > see these even when they do normal kexec/kdump loading. It should be
> > > changed to pr_debug().
> > > 
> > > Which way do you suggest?
> > > 1) change it back to pr_debug(), fix it in another patch;
> > > 2) keep it as is in the patch;
> > 
> > Personally I think it is fine to change them all in one patch, but the
> > rationale for converting pr_notice() to your new debug infrastructure
> > needs to be in the commit message.
> 
> Sure, sounds good to me. I have changed the patch log to reflect this as
> you suggested, please help check. Thanks again.
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 5/7] kexec_file, ricv: print out debugging message if required

[Baoquan He]

On 12/06/23 at 04:54pm, Conor Dooley wrote:
> On Wed, Dec 06, 2023 at 11:37:52PM +0800, Baoquan He wrote:
> > On 12/04/23 at 04:14pm, Conor Dooley wrote:
> > > On Mon, Dec 04, 2023 at 11:38:05PM +0800, Baoquan He wrote:
> > > > On 12/01/23 at 10:38am, Conor Dooley wrote:
> > > > > On Thu, Nov 30, 2023 at 10:39:53AM +0800, Baoquan He wrote:
> > > > > 
> > > > > $subject has a typo in the arch bit :)
> > > > 
> > > > Indeed, will fix if need report. Thanks for careful checking.
> > > > 
> > > > > 
> > > > > > Replace pr_debug() with the newly added kexec_dprintk() in kexec_file
> > > > > > loading related codes.
> > > > > 
> > > > > Commit messages should be understandable in isolation, but this only
> > > > > explains (part of) what is obvious in the diff. Why is this change
> > > > > being made?
> > > > 
> > > > The purpose has been detailedly described in cover letter and patch 1
> > > > log. Andrew has picked these patches into his tree and grabbed the cover
> > > > letter log into the relevant commit for people's later checking. All
> > > > these seven patches will be present in mainline together. This is common
> > > > way when posting patch series? Please let me know if I misunderstand
> > > > anything.
> > > 
> > > Each patch having a commit message that explains why a change is being
> > > made is the expectation. It is especially useful to explain the why
> > > here, since it is not just a mechanical conversion of pr_debug()s as the
> > > commit message suggests.
> > 
> > Sounds reasonable. I rephrase the patch 3 log as below, do you think
> > it's OK to you?
> 
> Yes, but with one comment.
> 
> > 
> > I will also adjust patch logs on other ARCH once this one is done.
> > Thanks.
> > 
> > =============================
> > Subject: [PATCH v3 5/7] kexec_file, ricv: print out debugging message if required
> > 
> > Then when specifying '-d' for kexec_file_load interface, loaded
> > locations of kernel/initrd/cmdline etc can be printed out to help debug.
> > 
> > Here replace pr_debug() with the newly added kexec_dprintk() in kexec_file
> > loading related codes.
> > 
> 
> > And also replace pr_notice() with kexec_dprintk() in elf_kexec_load()
> > because it's make sense to always print out loaded location of purgatory
              ~
> > and device tree even though users don't expect the message.

Fixed typo:
==========

And also replace pr_notice() with kexec_dprintk() in elf_kexec_load()
because it doesn't make sense to always print out loaded location of
purgatory and device tree even though users don't expect the message.

> 
> This seems to contradict what you said in your earlier mail, about
> moving these from notice to debug. I think you missed a negation in your
> new version of the commit message. What you said in response to me seems
> like a more complete explanation anyway:

Ah, I made mistake when typing, these printing is only for debugging,
so always printing out them is not suggested.

> 	always printing out the loaded location of purgatory and
> 	device tree doesn't make sense. It will be confusing when users
> 	see these even when they do normal kexec/kdump loading.
> 
> Thanks,
> Conor.
> 
> > And also remove kexec_image_info() because the content has been printed
> > out in generic code.
> > 
> > ============================
> > 
> > > 
> > > > > 
> > > > > > 
> > > > > > And also remove kexec_image_info() because the content has been printed
> > > > > > out in generic code.
> > > > > > 
> > > > > > Signed-off-by: Baoquan He <bhe@redhat.com>
> > > > > > ---
> > > > > >  arch/riscv/kernel/elf_kexec.c     | 11 ++++++-----
> > > > > >  arch/riscv/kernel/machine_kexec.c | 26 --------------------------
> > > > > >  2 files changed, 6 insertions(+), 31 deletions(-)
> > > > > > 
> > > > > > diff --git a/arch/riscv/kernel/elf_kexec.c b/arch/riscv/kernel/elf_kexec.c
> > > > > > index e60fbd8660c4..5bd1ec3341fe 100644
> > > > > > --- a/arch/riscv/kernel/elf_kexec.c
> > > > > > +++ b/arch/riscv/kernel/elf_kexec.c
> > > > > > @@ -216,7 +216,6 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> > > > > >  	if (ret)
> > > > > >  		goto out;
> > > > > >  	kernel_start = image->start;
> > > > > > -	pr_notice("The entry point of kernel at 0x%lx\n", image->start);
> > > > > >  
> > > > > >  	/* Add the kernel binary to the image */
> > > > > >  	ret = riscv_kexec_elf_load(image, &ehdr, &elf_info,
> > > > > > @@ -252,8 +251,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> > > > > >  		image->elf_load_addr = kbuf.mem;
> > > > > >  		image->elf_headers_sz = headers_sz;
> > > > > >  
> > > > > > -		pr_debug("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
> > > > > > -			 image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
> > > > > > +		kexec_dprintk("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
> > > > > > +			      image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
> > > > > >  
> > > > > >  		/* Setup cmdline for kdump kernel case */
> > > > > >  		modified_cmdline = setup_kdump_cmdline(image, cmdline,
> > > > > > @@ -275,6 +274,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> > > > > >  		pr_err("Error loading purgatory ret=%d\n", ret);
> > > > > >  		goto out;
> > > > > >  	}
> > > > > > +	kexec_dprintk("Loaded purgatory at 0x%lx\n", kbuf.mem);
> > > > > > +
> > > > > >  	ret = kexec_purgatory_get_set_symbol(image, "riscv_kernel_entry",
> > > > > >  					     &kernel_start,
> > > > > >  					     sizeof(kernel_start), 0);
> > > > > > @@ -293,7 +294,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> > > > > >  		if (ret)
> > > > > >  			goto out;
> > > > > >  		initrd_pbase = kbuf.mem;
> > > > > 
> > > > > > -		pr_notice("Loaded initrd at 0x%lx\n", initrd_pbase);
> > > > > > +		kexec_dprintk("Loaded initrd at 0x%lx\n", initrd_pbase);
> > > > > 
> > > > > This is not a pr_debug().
> > > > > 
> > > > > >  	}
> > > > > >  
> > > > > >  	/* Add the DTB to the image */
> > > > > > @@ -318,7 +319,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> > > > > >  	}
> > > > > >  	/* Cache the fdt buffer address for memory cleanup */
> > > > > >  	image->arch.fdt = fdt;
> > > > > 
> > > > > > -	pr_notice("Loaded device tree at 0x%lx\n", kbuf.mem);
> > > > > > +	kexec_dprintk("Loaded device tree at 0x%lx\n", kbuf.mem);
> > > > > 
> > > > > Neither is this. Why are they being moved from pr_notice()?
> > > > 
> > > > You are right. 
> > > > 
> > > > While always printing out the loaded location of purgatory and
> > > > device tree doesn't make sense. It will be confusing when users
> > > > see these even when they do normal kexec/kdump loading. It should be
> > > > changed to pr_debug().
> > > > 
> > > > Which way do you suggest?
> > > > 1) change it back to pr_debug(), fix it in another patch;
> > > > 2) keep it as is in the patch;
> > > 
> > > Personally I think it is fine to change them all in one patch, but the
> > > rationale for converting pr_notice() to your new debug infrastructure
> > > needs to be in the commit message.
> > 
> > Sure, sounds good to me. I have changed the patch log to reflect this as
> > you suggested, please help check. Thanks again.
> > 

Re: [PATCH v3 5/7] kexec_file, ricv: print out debugging message if required

[Baoquan He]

On 12/07/23 at 07:22am, Baoquan He wrote:
> On 12/06/23 at 04:54pm, Conor Dooley wrote:
> > On Wed, Dec 06, 2023 at 11:37:52PM +0800, Baoquan He wrote:
> > > On 12/04/23 at 04:14pm, Conor Dooley wrote:
> > > > On Mon, Dec 04, 2023 at 11:38:05PM +0800, Baoquan He wrote:
> > > > > On 12/01/23 at 10:38am, Conor Dooley wrote:
> > > > > > On Thu, Nov 30, 2023 at 10:39:53AM +0800, Baoquan He wrote:
> > > > > > 
> > > > > > $subject has a typo in the arch bit :)
> > > > > 
> > > > > Indeed, will fix if need report. Thanks for careful checking.
> > > > > 
> > > > > > 
> > > > > > > Replace pr_debug() with the newly added kexec_dprintk() in kexec_file
> > > > > > > loading related codes.
> > > > > > 
> > > > > > Commit messages should be understandable in isolation, but this only
> > > > > > explains (part of) what is obvious in the diff. Why is this change
> > > > > > being made?
> > > > > 
> > > > > The purpose has been detailedly described in cover letter and patch 1
> > > > > log. Andrew has picked these patches into his tree and grabbed the cover
> > > > > letter log into the relevant commit for people's later checking. All
> > > > > these seven patches will be present in mainline together. This is common
> > > > > way when posting patch series? Please let me know if I misunderstand
> > > > > anything.
> > > > 
> > > > Each patch having a commit message that explains why a change is being
> > > > made is the expectation. It is especially useful to explain the why
> > > > here, since it is not just a mechanical conversion of pr_debug()s as the
> > > > commit message suggests.
> > > 
> > > Sounds reasonable. I rephrase the patch 3 log as below, do you think
> > > it's OK to you?
> > 
> > Yes, but with one comment.
> > 
> > > 
> > > I will also adjust patch logs on other ARCH once this one is done.
> > > Thanks.
> > > 
> > > =============================
> > > Subject: [PATCH v3 5/7] kexec_file, ricv: print out debugging message if required
> > > 
> > > Then when specifying '-d' for kexec_file_load interface, loaded
> > > locations of kernel/initrd/cmdline etc can be printed out to help debug.
> > > 
> > > Here replace pr_debug() with the newly added kexec_dprintk() in kexec_file
> > > loading related codes.
> > > 
> > 
> > > And also replace pr_notice() with kexec_dprintk() in elf_kexec_load()
> > > because it's make sense to always print out loaded location of purgatory
>               ~
> > > and device tree even though users don't expect the message.
> 
> Fixed typo:
> ==========
> 
> And also replace pr_notice() with kexec_dprintk() in elf_kexec_load()
> because it doesn't make sense to always print out loaded location of
> purgatory and device tree even though users don't expect the message.

I will post v4 to include these suggested changes, please add comments
if there's any concern. Thanks for reviewing.

> 
> > 
> > This seems to contradict what you said in your earlier mail, about
> > moving these from notice to debug. I think you missed a negation in your
> > new version of the commit message. What you said in response to me seems
> > like a more complete explanation anyway:
> 
> Ah, I made mistake when typing, these printing is only for debugging,
> so always printing out them is not suggested.
> 
> > 	always printing out the loaded location of purgatory and
> > 	device tree doesn't make sense. It will be confusing when users
> > 	see these even when they do normal kexec/kdump loading.
> > 
> > Thanks,
> > Conor.
> > 
> > > And also remove kexec_image_info() because the content has been printed
> > > out in generic code.
> > > 
> > > ============================
> > > 
> > > > 
> > > > > > 
> > > > > > > 
> > > > > > > And also remove kexec_image_info() because the content has been printed
> > > > > > > out in generic code.
> > > > > > > 
> > > > > > > Signed-off-by: Baoquan He <bhe@redhat.com>
> > > > > > > ---
> > > > > > >  arch/riscv/kernel/elf_kexec.c     | 11 ++++++-----
> > > > > > >  arch/riscv/kernel/machine_kexec.c | 26 --------------------------
> > > > > > >  2 files changed, 6 insertions(+), 31 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/arch/riscv/kernel/elf_kexec.c b/arch/riscv/kernel/elf_kexec.c
> > > > > > > index e60fbd8660c4..5bd1ec3341fe 100644
> > > > > > > --- a/arch/riscv/kernel/elf_kexec.c
> > > > > > > +++ b/arch/riscv/kernel/elf_kexec.c
> > > > > > > @@ -216,7 +216,6 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> > > > > > >  	if (ret)
> > > > > > >  		goto out;
> > > > > > >  	kernel_start = image->start;
> > > > > > > -	pr_notice("The entry point of kernel at 0x%lx\n", image->start);
> > > > > > >  
> > > > > > >  	/* Add the kernel binary to the image */
> > > > > > >  	ret = riscv_kexec_elf_load(image, &ehdr, &elf_info,
> > > > > > > @@ -252,8 +251,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> > > > > > >  		image->elf_load_addr = kbuf.mem;
> > > > > > >  		image->elf_headers_sz = headers_sz;
> > > > > > >  
> > > > > > > -		pr_debug("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
> > > > > > > -			 image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
> > > > > > > +		kexec_dprintk("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
> > > > > > > +			      image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
> > > > > > >  
> > > > > > >  		/* Setup cmdline for kdump kernel case */
> > > > > > >  		modified_cmdline = setup_kdump_cmdline(image, cmdline,
> > > > > > > @@ -275,6 +274,8 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> > > > > > >  		pr_err("Error loading purgatory ret=%d\n", ret);
> > > > > > >  		goto out;
> > > > > > >  	}
> > > > > > > +	kexec_dprintk("Loaded purgatory at 0x%lx\n", kbuf.mem);
> > > > > > > +
> > > > > > >  	ret = kexec_purgatory_get_set_symbol(image, "riscv_kernel_entry",
> > > > > > >  					     &kernel_start,
> > > > > > >  					     sizeof(kernel_start), 0);
> > > > > > > @@ -293,7 +294,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> > > > > > >  		if (ret)
> > > > > > >  			goto out;
> > > > > > >  		initrd_pbase = kbuf.mem;
> > > > > > 
> > > > > > > -		pr_notice("Loaded initrd at 0x%lx\n", initrd_pbase);
> > > > > > > +		kexec_dprintk("Loaded initrd at 0x%lx\n", initrd_pbase);
> > > > > > 
> > > > > > This is not a pr_debug().
> > > > > > 
> > > > > > >  	}
> > > > > > >  
> > > > > > >  	/* Add the DTB to the image */
> > > > > > > @@ -318,7 +319,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf,
> > > > > > >  	}
> > > > > > >  	/* Cache the fdt buffer address for memory cleanup */
> > > > > > >  	image->arch.fdt = fdt;
> > > > > > 
> > > > > > > -	pr_notice("Loaded device tree at 0x%lx\n", kbuf.mem);
> > > > > > > +	kexec_dprintk("Loaded device tree at 0x%lx\n", kbuf.mem);
> > > > > > 
> > > > > > Neither is this. Why are they being moved from pr_notice()?
> > > > > 
> > > > > You are right. 
> > > > > 
> > > > > While always printing out the loaded location of purgatory and
> > > > > device tree doesn't make sense. It will be confusing when users
> > > > > see these even when they do normal kexec/kdump loading. It should be
> > > > > changed to pr_debug().
> > > > > 
> > > > > Which way do you suggest?
> > > > > 1) change it back to pr_debug(), fix it in another patch;
> > > > > 2) keep it as is in the patch;
> > > > 
> > > > Personally I think it is fine to change them all in one patch, but the
> > > > rationale for converting pr_notice() to your new debug infrastructure
> > > > needs to be in the commit message.
> > > 
> > > Sure, sounds good to me. I have changed the patch log to reflect this as
> > > you suggested, please help check. Thanks again.
> > > 
> 
>