LinuxLists.cc - [PATCH v2 0/3] kdump: Allow ELF header creation in new kernel

2013-05-22 17:58:43

Subject: [PATCH v2 0/3] kdump: Allow ELF header creation in new kernel

Hello Vivek,

Here the 2nd version of the patch set. We tried to implement the
function using the arch_get/free_crash_header() weak function as
you suggested. We kept the ELFCORE_ADDR_NEWMEM define so that
is_kdump_kernel() also works for our case.

ChangeLog
=========
v1 => v2)

- Rebase 3.10-rc2 + vmcore mmap patches
- Introduced arch_get/free_crash_header() and ELFCORE_ADDR_NEWMEM

Feature Description
===================
For s390 we want to use /proc/vmcore for our SCSI stand-alone
dump (zfcpdump). We have support where the first HSA_SIZE bytes are
saved into a hypervisor owned memory area (HSA) before the kdump
kernel is booted. When the kdump kernel starts, it is restricted
to use only HSA_SIZE bytes.

The advantages of this mechanism are:

* No crashkernel memory has to be defined in the old kernel.
* Early boot problems (before kexec_load has been done) can be dumped
* Non-Linux systems can be dumped.

We modify the s390 copy_oldmem_page() function to read from the HSA memory
if memory below HSA_SIZE bytes is requested.

Since we cannot use the kexec tool to load the kernel in this scenario,
we have to build the ELF header in the 2nd (kdump/new) kernel.

So with the following patch set we would like to introduce the new
function that the ELF header for /proc/vmcore can be created in the 2nd
kernel memory.

The following steps are done during zfcpdump execution:

1. Production system crashes
2. User boots a SCSI disk that has been prepared with the zfcpdump tool
3. Hypervisor saves CPU state of boot CPU and HSA_SIZE bytes of memory into HSA
4. Boot loader loads kernel into low memory area
5. Kernel boots and uses only HSA_SIZE bytes of memory
6. Kernel saves registers of non-boot CPUs
7. Kernel does memory detection for dump memory map
8. Kernel creates ELF header for /proc/vmcore
9. /proc/vmcore uses this header for initialization
10. The zfcpdump user space reads /proc/vmcore to write dump to SCSI disk
- copy_oldmem_page() copies from HSA for memory below HSA_SIZE
- copy_oldmem_page() copies from real memory for memory above HSA_SIZE

---
Jan Willeke (1):
s390/kdump: Use vmcore for zfcpdump

Michael Holzheu (2):
kdump: Introduce ELF header in new memory feature
s390/kdump: Use arch function call for kdump

arch/s390/Kconfig | 3 +-
arch/s390/include/asm/sclp.h | 1 +
arch/s390/kernel/crash_dump.c | 126 +++++++++++++++++++++++++++++++-----------
drivers/s390/char/zcore.c | 6 +-
fs/proc/vmcore.c | 80 +++++++++++++++++++++------
include/linux/crash_dump.h | 3 +
6 files changed, 164 insertions(+), 55 deletions(-)

--
1.8.1.6

2013-05-22 17:58:47

by Michael Holzheu

[permalink] [raw]

Subject: [PATCH v2 1/3] kdump: Introduce ELF header in new memory feature

Currently for s390 we create the ELF core header in the 2nd kernel
with a small trick. We relocate the addresses in the ELF header in
a way that for the /proc/vmcore code it seems to be in the 1st kernel
(old) memory and the read_from_oldmem() returns the correct data.
This allows the /proc/vmcore code to use the ELF header in the
2nd kernel.

This patch now exchanges the old mechanism with the new and much
cleaner function call override feature that now offcially allows to
create the ELF core header in the 2nd kernel.

To use the new feature the following has to be done by the architecture
backend code:

* Set elfcorehdr_addr to ELFCORE_ADDR_NEWMEM
-> is_kdump_kernel() will return true
* Override arch_get_crash_header() to return the address of the ELF
header in new memory.
* Override arch_free_crash_header() to free the memory of the ELF
header in new memory.

Signed-off-by: Michael Holzheu <[email protected]>
---
fs/proc/vmcore.c | 80 +++++++++++++++++++++++++++++++++++-----------
include/linux/crash_dump.h | 3 ++
2 files changed, 65 insertions(+), 18 deletions(-)

diff --git a/fs/proc/vmcore.c b/fs/proc/vmcore.c
index 6ba32f8..d97ed31 100644
--- a/fs/proc/vmcore.c
+++ b/fs/proc/vmcore.c
@@ -123,6 +123,47 @@ static ssize_t read_from_oldmem(char *buf, size_t count,
return read;
}

+/*
+ * Read from the current (new) kernel memory
+ */
+static ssize_t read_from_newmem(char *buf, size_t count, u64 *ppos, int userbuf)
+{
+ if (userbuf) {
+ if (copy_to_user(buf, (void *)*ppos, count))
+ return -EFAULT;
+ } else {
+ memcpy(buf, (void *)*ppos, count);
+ }
+ *ppos += count;
+ return count;
+}
+
+/*
+ * Provide access to the header
+ */
+static ssize_t read_from_crash_header(char *buf, size_t count, u64 *ppos,
+ int userbuf)
+{
+ if (elfcorehdr_addr == ELFCORE_ADDR_NEWMEM)
+ return read_from_newmem(buf, count, ppos, userbuf);
+ else
+ return read_from_oldmem(buf, count, ppos, userbuf);
+}
+
+/*
+ * Architetures may override this function to get header address
+ */
+unsigned long long __weak arch_get_crash_header(void)
+{
+ return elfcorehdr_addr;
+}
+
+/*
+ * Architetures may override this function to free header
+ */
+void __weak arch_free_crash_header(void)
+{}
+
/* Read from the ELF header and then the crash dump. On error, negative value is
* returned otherwise number of bytes read are returned.
*/
@@ -356,7 +397,7 @@ static int __init get_note_number_and_size_elf64(const Elf64_Ehdr *ehdr_ptr,
notes_section = kmalloc(max_sz, GFP_KERNEL);
if (!notes_section)
return -ENOMEM;
- rc = read_from_oldmem(notes_section, max_sz, &offset, 0);
+ rc = read_from_crash_header(notes_section, max_sz, &offset, 0);
if (rc < 0) {
kfree(notes_section);
return rc;
@@ -412,7 +453,7 @@ static int __init copy_notes_elf64(const Elf64_Ehdr *ehdr_ptr, char *notes_buf)
notes_section = kmalloc(max_sz, GFP_KERNEL);
if (!notes_section)
return -ENOMEM;
- rc = read_from_oldmem(notes_section, max_sz, &offset, 0);
+ rc = read_from_crash_header(notes_section, max_sz, &offset, 0);
if (rc < 0) {
kfree(notes_section);
return rc;
@@ -428,8 +469,8 @@ static int __init copy_notes_elf64(const Elf64_Ehdr *ehdr_ptr, char *notes_buf)
nhdr_ptr = (Elf64_Nhdr*)((char*)nhdr_ptr + sz);
}
offset = phdr_ptr->p_offset;
- rc = read_from_oldmem(notes_buf + phdr_sz, real_sz,
- &offset, 0);
+ rc = read_from_crash_header(notes_buf + phdr_sz, real_sz,
+ &offset, 0);
if (rc < 0) {
kfree(notes_section);
return rc;
@@ -533,7 +574,7 @@ static int __init get_note_number_and_size_elf32(const Elf32_Ehdr *ehdr_ptr,
notes_section = kmalloc(max_sz, GFP_KERNEL);
if (!notes_section)
return -ENOMEM;
- rc = read_from_oldmem(notes_section, max_sz, &offset, 0);
+ rc = read_from_crash_header(notes_section, max_sz, &offset, 0);
if (rc < 0) {
kfree(notes_section);
return rc;
@@ -589,7 +630,7 @@ static int __init copy_notes_elf32(const Elf32_Ehdr *ehdr_ptr, char *notes_buf)
notes_section = kmalloc(max_sz, GFP_KERNEL);
if (!notes_section)
return -ENOMEM;
- rc = read_from_oldmem(notes_section, max_sz, &offset, 0);
+ rc = read_from_crash_header(notes_section, max_sz, &offset, 0);
if (rc < 0) {
kfree(notes_section);
return rc;
@@ -605,8 +646,8 @@ static int __init copy_notes_elf32(const Elf32_Ehdr *ehdr_ptr, char *notes_buf)
nhdr_ptr = (Elf32_Nhdr*)((char*)nhdr_ptr + sz);
}
offset = phdr_ptr->p_offset;
- rc = read_from_oldmem(notes_buf + phdr_sz, real_sz,
- &offset, 0);
+ rc = read_from_crash_header(notes_buf + phdr_sz, real_sz,
+ &offset, 0);
if (rc < 0) {
kfree(notes_section);
return rc;
@@ -804,10 +845,11 @@ static int __init parse_crash_elf64_headers(void)
Elf64_Ehdr ehdr;
u64 addr;

- addr = elfcorehdr_addr;
+ addr = arch_get_crash_header();

/* Read Elf header */
- rc = read_from_oldmem((char*)&ehdr, sizeof(Elf64_Ehdr), &addr, 0);
+ rc = read_from_crash_header((char *)&ehdr, sizeof(Elf64_Ehdr),
+ &addr, 0);
if (rc < 0)
return rc;

@@ -832,8 +874,8 @@ static int __init parse_crash_elf64_headers(void)
get_order(elfcorebuf_sz_orig));
if (!elfcorebuf)
return -ENOMEM;
- addr = elfcorehdr_addr;
- rc = read_from_oldmem(elfcorebuf, elfcorebuf_sz_orig, &addr, 0);
+ addr = arch_get_crash_header();
+ rc = read_from_crash_header(elfcorebuf, elfcorebuf_sz_orig, &addr, 0);
if (rc < 0) {
free_pages((unsigned long)elfcorebuf,
get_order(elfcorebuf_sz_orig));
@@ -867,10 +909,11 @@ static int __init parse_crash_elf32_headers(void)
Elf32_Ehdr ehdr;
u64 addr;

- addr = elfcorehdr_addr;
+ addr = arch_get_crash_header();

/* Read Elf header */
- rc = read_from_oldmem((char*)&ehdr, sizeof(Elf32_Ehdr), &addr, 0);
+ rc = read_from_crash_header((char *)&ehdr, sizeof(Elf32_Ehdr),
+ &addr, 0);
if (rc < 0)
return rc;

@@ -895,8 +938,8 @@ static int __init parse_crash_elf32_headers(void)
get_order(elfcorebuf_sz_orig));
if (!elfcorebuf)
return -ENOMEM;
- addr = elfcorehdr_addr;
- rc = read_from_oldmem(elfcorebuf, elfcorebuf_sz_orig, &addr, 0);
+ addr = arch_get_crash_header();
+ rc = read_from_crash_header(elfcorebuf, elfcorebuf_sz_orig, &addr, 0);
if (rc < 0) {
free_pages((unsigned long)elfcorebuf,
get_order(elfcorebuf_sz_orig));
@@ -930,8 +973,8 @@ static int __init parse_crash_elf_headers(void)
u64 addr;
int rc=0;

- addr = elfcorehdr_addr;
- rc = read_from_oldmem(e_ident, EI_NIDENT, &addr, 0);
+ addr = arch_get_crash_header();
+ rc = read_from_crash_header(e_ident, EI_NIDENT, &addr, 0);
if (rc < 0)
return rc;
if (memcmp(e_ident, ELFMAG, SELFMAG) != 0) {
@@ -961,6 +1004,7 @@ static int __init parse_crash_elf_headers(void)
pr_warn("Warning: Core image elf header is not sane\n");
return -EINVAL;
}
+ arch_free_crash_header();
return 0;
}

diff --git a/include/linux/crash_dump.h b/include/linux/crash_dump.h
index 37e4f8d..a1ef7b5 100644
--- a/include/linux/crash_dump.h
+++ b/include/linux/crash_dump.h
@@ -8,12 +8,15 @@

#define ELFCORE_ADDR_MAX (-1ULL)
#define ELFCORE_ADDR_ERR (-2ULL)
+#define ELFCORE_ADDR_NEWMEM (-3ULL)

extern unsigned long long elfcorehdr_addr;
extern unsigned long long elfcorehdr_size;

extern ssize_t copy_oldmem_page(unsigned long, char *, size_t,
unsigned long, int);
+extern unsigned long long __weak arch_get_crash_header(void);
+extern void __weak arch_free_crash_header(void);

/* Architecture code defines this if there are other possible ELF
* machine types, e.g. on bi-arch capable hardware. */
--
1.8.1.6

2013-05-22 17:58:45

by Michael Holzheu

[permalink] [raw]

Subject: [PATCH v2 2/3] s390/kdump: Use arch function call for kdump

2013-05-22 17:59:26

by Michael Holzheu

[permalink] [raw]

Subject: [PATCH v2 3/3] s390/kdump: Use vmcore for zfcpdump

From: Jan Willeke <[email protected]>

This patch modifies the s390 copy_oldmem_page() function for zfcpdump
to read from the HSA memory if memory below HSA_SIZE bytes is requested.
Otherwise from real memory is read.

Signed-off-by: Jan Willeke <[email protected]>
Signed-off-by: Michael Holzheu <[email protected]>
---
arch/s390/Kconfig | 3 +-
arch/s390/include/asm/sclp.h | 1 +
arch/s390/kernel/crash_dump.c | 74 ++++++++++++++++++++++++++++++++++---------
drivers/s390/char/zcore.c | 6 ++--
4 files changed, 65 insertions(+), 19 deletions(-)

diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 2c9789d..a0d78f1 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -516,6 +516,7 @@ config CRASH_DUMP
bool "kernel crash dumps"
depends on 64BIT && SMP
select KEXEC
+ select ZFCPDUMP
help
Generate crash dump after being started by kexec.
Crash dump kernels are loaded in the main kernel with kexec-tools
@@ -526,7 +527,7 @@ config CRASH_DUMP
config ZFCPDUMP
def_bool n
prompt "zfcpdump support"
- select SMP
+ depends on SMP
help
Select this option if you want to build an zfcpdump enabled kernel.
Refer to <file:Documentation/s390/zfcpdump.txt> for more details on this.
diff --git a/arch/s390/include/asm/sclp.h b/arch/s390/include/asm/sclp.h
index 06a1361..7dc7f9c 100644
--- a/arch/s390/include/asm/sclp.h
+++ b/arch/s390/include/asm/sclp.h
@@ -56,5 +56,6 @@ bool sclp_has_linemode(void);
bool sclp_has_vt220(void);
int sclp_pci_configure(u32 fid);
int sclp_pci_deconfigure(u32 fid);
+int memcpy_hsa(void *dest, unsigned long src, size_t count, int mode);

#endif /* _ASM_S390_SCLP_H */
diff --git a/arch/s390/kernel/crash_dump.c b/arch/s390/kernel/crash_dump.c
index 4fe7fa7..40373b3 100644
--- a/arch/s390/kernel/crash_dump.c
+++ b/arch/s390/kernel/crash_dump.c
@@ -16,6 +16,7 @@
#include <asm/os_info.h>
#include <asm/elf.h>
#include <asm/ipl.h>
+#include <asm/sclp.h>

#define PTR_ADD(x, y) (((char *) (x)) + ((unsigned long) (y)))
#define PTR_SUB(x, y) (((char *) (x)) - ((unsigned long) (y)))
@@ -24,6 +25,25 @@
static size_t elfcorebuf_sz;
static char *elfcorebuf;

+/* For pages below ZFCPDUMP_HSA_SIZE memory from the HSA is copied. Otherwise
+ * real memory copy is used.
+ */
+static ssize_t copy_oldmem_page_zfcpdump(char *buf, size_t csize,
+ unsigned long src, int userbuf)
+{
+ if (src < ZFCPDUMP_HSA_SIZE) {
+ if (memcpy_hsa(buf, src, csize, userbuf) < 0)
+ return -EINVAL;
+ } else {
+ if (userbuf)
+ copy_to_user_real((void __force __user *) buf,
+ (void *) src, csize);
+ else
+ memcpy_real(buf, (void *) src, csize);
+ }
+ return csize;
+}
+
/*
* Copy one page from "oldmem"
*
@@ -31,15 +51,10 @@ static char *elfcorebuf;
* - [OLDMEM_BASE - OLDMEM_BASE + OLDMEM_SIZE] is mapped to [0 - OLDMEM_SIZE].
* - [0 - OLDMEM_SIZE] is mapped to [OLDMEM_BASE - OLDMEM_BASE + OLDMEM_SIZE]
*/
-ssize_t copy_oldmem_page(unsigned long pfn, char *buf,
- size_t csize, unsigned long offset, int userbuf)
-{
- unsigned long src;
+static ssize_t copy_oldmem_page_kdump(char *buf, size_t csize,
+ unsigned long src, int userbuf)

- if (!csize)
- return 0;
-
- src = (pfn << PAGE_SHIFT) + offset;
+{
if (src < OLDMEM_SIZE)
src += OLDMEM_BASE;
else if (src > OLDMEM_BASE &&
@@ -54,6 +69,23 @@ ssize_t copy_oldmem_page(unsigned long pfn, char *buf,
}

/*
+ * Copy one page from "oldmem"
+ */
+ssize_t copy_oldmem_page(unsigned long pfn, char *buf, size_t csize,
+ unsigned long offset, int userbuf)
+{
+ unsigned long src;
+ if (!csize)
+ return 0;
+ src = (pfn << PAGE_SHIFT) + offset;
+
+ if (OLDMEM_BASE)
+ return copy_oldmem_page_kdump(buf, csize, src, userbuf);
+ else
+ return copy_oldmem_page_zfcpdump(buf, csize, src, userbuf);
+}
+
+/*
* Copy memory from old kernel
*/
int copy_from_oldmem(void *dest, void *src, size_t count)
@@ -61,11 +93,21 @@ int copy_from_oldmem(void *dest, void *src, size_t count)
unsigned long copied = 0;
int rc;

- if ((unsigned long) src < OLDMEM_SIZE) {
- copied = min(count, OLDMEM_SIZE - (unsigned long) src);
- rc = memcpy_real(dest, src + OLDMEM_BASE, copied);
- if (rc)
- return rc;
+ if (OLDMEM_BASE) {
+ if ((unsigned long) src < OLDMEM_SIZE) {
+ copied = min(count, OLDMEM_SIZE - (unsigned long) src);
+ rc = memcpy_real(dest, src + OLDMEM_BASE, copied);
+ if (rc)
+ return rc;
+ }
+ } else {
+ if ((unsigned long) src < ZFCPDUMP_HSA_SIZE) {
+ copied = min(count,
+ ZFCPDUMP_HSA_SIZE - (unsigned long) src);
+ rc = memcpy_hsa(dest, (unsigned long) src, copied, 0);
+ if (rc)
+ return rc;
+ }
}
return memcpy_real(dest + copied, src + copied, count - copied);
}
@@ -448,7 +490,9 @@ void arch_free_crash_header(void)
*/
static int setup_kdump_elfcorehdr(void)
{
- if (!OLDMEM_BASE || is_kdump_kernel())
+ if (!OLDMEM_BASE || (ipl_info.type != IPL_TYPE_FCP_DUMP))
+ return -EINVAL;
+ if (is_kdump_kernel())
return -EINVAL;
arch_vmcore_get_elf_hdr(&elfcorebuf, &elfcorebuf_sz);
elfcorehdr_addr = ELFCORE_ADDR_NEWMEM;
@@ -456,4 +500,4 @@ static int setup_kdump_elfcorehdr(void)
return 0;
}

-subsys_initcall(setup_kdump_elfcorehdr);
+subsys_initcall_sync(setup_kdump_elfcorehdr);
diff --git a/drivers/s390/char/zcore.c b/drivers/s390/char/zcore.c
index 9e5e146..794820a 100644
--- a/drivers/s390/char/zcore.c
+++ b/drivers/s390/char/zcore.c
@@ -30,8 +30,8 @@

#define TRACE(x...) debug_sprintf_event(zcore_dbf, 1, x)

-#define TO_USER 0
-#define TO_KERNEL 1
+#define TO_USER 1
+#define TO_KERNEL 0
#define CHUNK_INFO_SIZE 34 /* 2 16-byte char, each followed by blank */

enum arch_id {
@@ -73,7 +73,7 @@ static struct ipl_parameter_block *ipl_block;
* @count: Size of buffer, which should be copied
* @mode: Either TO_KERNEL or TO_USER
*/
-static int memcpy_hsa(void *dest, unsigned long src, size_t count, int mode)
+int memcpy_hsa(void *dest, unsigned long src, size_t count, int mode)
{
int offs, blk_num;
static char buf[PAGE_SIZE] __attribute__((__aligned__(PAGE_SIZE)));
--
1.8.1.6

2013-05-23 17:42:54

by Vivek Goyal

[permalink] [raw]

Subject: Re: [PATCH v2 1/3] kdump: Introduce ELF header in new memory feature

On Wed, May 22, 2013 at 07:58:35PM +0200, Michael Holzheu wrote:
> or s390 we create the ELF core header in the 2nd kernel
> with a small trick. We relocate the addresses in the ELF header in
> a way that for the /proc/vmcore code it seems to be in the 1st kernel
> (old) memory and the read_from_oldmem() returns the correct data.
> This allows the /proc/vmcore code to use the ELF header in the
> 2nd kernel.
>
> This patch now exchanges the old mechanism with the new and much
> cleaner function call override feature that now offcially allows to
> create the ELF core header in the 2nd kernel.
>
> To use the new feature the following has to be done by the architecture
> backend code:
>
> * Set elfcorehdr_addr to ELFCORE_ADDR_NEWMEM
> -> is_kdump_kernel() will return true

setting elfcorehdr_addr to ELFCORE_ADDR_NEWMEM looks really odd to me.
There is no need for arch independent code to know whether crash headers
are in new memory or not. Our old assumption was that everything is in
old memory. Now that is broken because of s390. So arch should be
able to override read_from_crash_header() call and read headers from
new memory. If need be s390 can maintain another variable for this
state to figure out where headers are.

Also arch should be able to set elfcorehdr_addr to virtual address
of ELF header. is_kdump_kernel() does not care whether address stored
in elfcorehdr_addr is physical or virtual.

IOW, generic code would not care whether headers are in new memory
or old memory. All the operations on header will be abstracted with
the help of helper functions.

Can we please get rid of this NEWMEM stuff.

> * Override arch_get_crash_header() to return the address of the ELF
> header in new memory.
> * Override arch_free_crash_header() to free the memory of the ELF
> header in new memory.
>
> Signed-off-by: Michael Holzheu <[email protected]>
> ---
> fs/proc/vmcore.c | 80 +++++++++++++++++++++++++++++++++++-----------
> include/linux/crash_dump.h | 3 ++
> 2 files changed, 65 insertions(+), 18 deletions(-)
>
> diff --git a/fs/proc/vmcore.c b/fs/proc/vmcore.c
> index 6ba32f8..d97ed31 100644
> --- a/fs/proc/vmcore.c
> +++ b/fs/proc/vmcore.c
> @@ -123,6 +123,47 @@ static ssize_t read_from_oldmem(char *buf, size_t count,
> return read;
> }
>
> +/*
> + * Read from the current (new) kernel memory
> + */
> +static ssize_t read_from_newmem(char *buf, size_t count, u64 *ppos, int userbuf)
> +{
> + if (userbuf) {
> + if (copy_to_user(buf, (void *)*ppos, count))
> + return -EFAULT;
> + } else {
> + memcpy(buf, (void *)*ppos, count);
> + }
> + *ppos += count;
> + return count;
> +}
> +

Why do we need read_from_newmem? I thought arch code will override
read_from_crash_header() and write a variant of read_from_newmem()
internally. I think could still be an helper function in vmcore.c if
there are many arch keeping headers in new memory so that we don't
have same code across multiple arches. But s390 seems to be the only
expcetion at this point of time.

> +/*
> + * Provide access to the header
> + */
> +static ssize_t read_from_crash_header(char *buf, size_t count, u64 *ppos,
> + int userbuf)
> +{
> + if (elfcorehdr_addr == ELFCORE_ADDR_NEWMEM)
> + return read_from_newmem(buf, count, ppos, userbuf);
> + else
> + return read_from_oldmem(buf, count, ppos, userbuf);
> +}
> +

I thought read_from_crash_header() will simply be

read_from_crash_header() {
return read_from_oldmem()
}

And arch code will override it to read the headers from new memory. That
way generic code does not have to know whether headers are in old memory
or in new memory or somewhere else.

Also current usage of read_from_crash_header() seems to be that we are
not copying header data to user space buffer directly. Generic code will
process headers and copy them in kernel memory and it will be copied to
user space from there if need be.

So for time being I think it should be just fine to assume that
read_from_crash_header() is copying data to kernel memory only.

Thanks
Vivek

2013-05-23 18:52:50

by Michael Holzheu

[permalink] [raw]

Subject: Re: [PATCH v2 1/3] kdump: Introduce ELF header in new memory feature

Hello Vivek,

Thanks for your reply. I will integrate your feedback into the v3 patch
set and will make the following changes:

- Get rid of ELFCORE_ADDR_NEWMEM (sorry for that:)
- Make read_from_crash_header() only read from kernel
- Move read_from_crash_header() to weak function
arch_read_from_crash_header()
- Implement read_from_crash_header() strong function for s390
- Set elfcorehdr_addr to address of new ELF header

Best Regards,
Michael

On Thu, 23 May 2013 13:41:27 -0400
Vivek Goyal <[email protected]> wrote:

> On Wed, May 22, 2013 at 07:58:35PM +0200, Michael Holzheu wrote:
> > or s390 we create the ELF core header in the 2nd kernel
> > with a small trick. We relocate the addresses in the ELF header in
> > a way that for the /proc/vmcore code it seems to be in the 1st
> > kernel (old) memory and the read_from_oldmem() returns the correct
> > data. This allows the /proc/vmcore code to use the ELF header in the
> > 2nd kernel.
> >
> > This patch now exchanges the old mechanism with the new and much
> > cleaner function call override feature that now offcially allows to
> > create the ELF core header in the 2nd kernel.
> >
> > To use the new feature the following has to be done by the
> > architecture backend code:
> >
> > * Set elfcorehdr_addr to ELFCORE_ADDR_NEWMEM
> > -> is_kdump_kernel() will return true
>
> setting elfcorehdr_addr to ELFCORE_ADDR_NEWMEM looks really odd to me.
> There is no need for arch independent code to know whether crash
> headers are in new memory or not. Our old assumption was that
> everything is in old memory. Now that is broken because of s390. So
> arch should be able to override read_from_crash_header() call and
> read headers from new memory. If need be s390 can maintain another
> variable for this state to figure out where headers are.
>
> Also arch should be able to set elfcorehdr_addr to virtual address
> of ELF header. is_kdump_kernel() does not care whether address stored
> in elfcorehdr_addr is physical or virtual.
>
> IOW, generic code would not care whether headers are in new memory
> or old memory. All the operations on header will be abstracted with
> the help of helper functions.
>
> Can we please get rid of this NEWMEM stuff.
>
>
> > * Override arch_get_crash_header() to return the address of the ELF
> > header in new memory.
> > * Override arch_free_crash_header() to free the memory of the ELF
> > header in new memory.
> >
> > Signed-off-by: Michael Holzheu <[email protected]>
> > ---
> > fs/proc/vmcore.c | 80
> > +++++++++++++++++++++++++++++++++++-----------
> > include/linux/crash_dump.h | 3 ++ 2 files changed, 65
> > insertions(+), 18 deletions(-)
> >
> > diff --git a/fs/proc/vmcore.c b/fs/proc/vmcore.c
> > index 6ba32f8..d97ed31 100644
> > --- a/fs/proc/vmcore.c
> > +++ b/fs/proc/vmcore.c
> > @@ -123,6 +123,47 @@ static ssize_t read_from_oldmem(char *buf,
> > size_t count, return read;
> > }
> >
> > +/*
> > + * Read from the current (new) kernel memory
> > + */
> > +static ssize_t read_from_newmem(char *buf, size_t count, u64
> > *ppos, int userbuf) +{
> > + if (userbuf) {
> > + if (copy_to_user(buf, (void *)*ppos, count))
> > + return -EFAULT;
> > + } else {
> > + memcpy(buf, (void *)*ppos, count);
> > + }
> > + *ppos += count;
> > + return count;
> > +}
> > +
>
> Why do we need read_from_newmem? I thought arch code will override
> read_from_crash_header() and write a variant of read_from_newmem()
> internally. I think could still be an helper function in vmcore.c if
> there are many arch keeping headers in new memory so that we don't
> have same code across multiple arches. But s390 seems to be the only
> expcetion at this point of time.
>
> > +/*
> > + * Provide access to the header
> > + */
> > +static ssize_t read_from_crash_header(char *buf, size_t count, u64
> > *ppos,
> > + int userbuf)
> > +{
> > + if (elfcorehdr_addr == ELFCORE_ADDR_NEWMEM)
> > + return read_from_newmem(buf, count, ppos, userbuf);
> > + else
> > + return read_from_oldmem(buf, count, ppos, userbuf);
> > +}
> > +
>
> I thought read_from_crash_header() will simply be
>
> read_from_crash_header() {
> return read_from_oldmem()
> }
>
> And arch code will override it to read the headers from new memory.
> That way generic code does not have to know whether headers are in
> old memory or in new memory or somewhere else.
>
> Also current usage of read_from_crash_header() seems to be that we are
> not copying header data to user space buffer directly. Generic code
> will process headers and copy them in kernel memory and it will be
> copied to user space from there if need be.
>
> So for time being I think it should be just fine to assume that
> read_from_crash_header() is copying data to kernel memory only.
>
> Thanks
> Vivek
>

2013-05-23 18:58:30

by Vivek Goyal

[permalink] [raw]

Subject: Re: [PATCH v2 1/3] kdump: Introduce ELF header in new memory feature

On Thu, May 23, 2013 at 08:52:41PM +0200, Michael Holzheu wrote:
> Hello Vivek,
>
> Thanks for your reply. I will integrate your feedback into the v3 patch
> set and will make the following changes:
>
> - Get rid of ELFCORE_ADDR_NEWMEM (sorry for that:)
> - Make read_from_crash_header() only read from kernel
> - Move read_from_crash_header() to weak function
> arch_read_from_crash_header()
> - Implement read_from_crash_header() strong function for s390
> - Set elfcorehdr_addr to address of new ELF header

Thanks Michael. Also note, that hatayama has posted a new version (V8)
of his mmap() patches. So you can rebase your changes on top of V8.

Thanks
Vivek