Re: [PATCH v3 12/12] kdump: Use vmlinux_build_id to simplify
Quoting Baoquan He (2021-04-08 03:17:43)
> On 04/07/21 at 07:03pm, Petr Mladek wrote:
> > On Tue 2021-03-30 20:05:20, Stephen Boyd wrote:
> > > We can use the vmlinux_build_id array here now instead of open coding
> > > it. This mostly consolidates code.
> > >
> > > Cc: Jiri Olsa
> > > Cc: Alexei Starovoitov
> > > Cc: Jessica Yu
> > > Cc: Evan Green
> > > Cc: Hsin-Yi Wang
> > > Cc: Dave Young
> > > Cc: Baoquan He
> > > Cc: Vivek Goyal
> > > Cc:
> > > Signed-off-by: Stephen Boyd
> > > ---
> > > include/linux/crash_core.h | 6 +-
> > > kernel/crash_core.c| 41 ++
> > > 2 files changed, 3 insertions(+), 44 deletions(-)
> > >
> > > diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
> > > index 206bde8308b2..fb8ab99bb2ee 100644
> > > --- a/include/linux/crash_core.h
> > > +++ b/include/linux/crash_core.h
> > > @@ -39,7 +39,7 @@ phys_addr_t paddr_vmcoreinfo_note(void);
> > > #define VMCOREINFO_OSRELEASE(value) \
> > > vmcoreinfo_append_str("OSRELEASE=%s\n", value)
> > > #define VMCOREINFO_BUILD_ID(value) \
> > > - vmcoreinfo_append_str("BUILD-ID=%s\n", value)
> > > + vmcoreinfo_append_str("BUILD-ID=%20phN\n", value)
>
> I may miss something, wondering why we need add '20' here.
The build ID is an array of 20 bytes and this format is to print 20
bytes in hex.
Re: [PATCH v3 12/12] kdump: Use vmlinux_build_id to simplify
On 04/07/21 at 07:03pm, Petr Mladek wrote:
> On Tue 2021-03-30 20:05:20, Stephen Boyd wrote:
> > We can use the vmlinux_build_id array here now instead of open coding
> > it. This mostly consolidates code.
> >
> > Cc: Jiri Olsa
> > Cc: Alexei Starovoitov
> > Cc: Jessica Yu
> > Cc: Evan Green
> > Cc: Hsin-Yi Wang
> > Cc: Dave Young
> > Cc: Baoquan He
> > Cc: Vivek Goyal
> > Cc:
> > Signed-off-by: Stephen Boyd
> > ---
> > include/linux/crash_core.h | 6 +-
> > kernel/crash_core.c| 41 ++
> > 2 files changed, 3 insertions(+), 44 deletions(-)
> >
> > diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
> > index 206bde8308b2..fb8ab99bb2ee 100644
> > --- a/include/linux/crash_core.h
> > +++ b/include/linux/crash_core.h
> > @@ -39,7 +39,7 @@ phys_addr_t paddr_vmcoreinfo_note(void);
> > #define VMCOREINFO_OSRELEASE(value) \
> > vmcoreinfo_append_str("OSRELEASE=%s\n", value)
> > #define VMCOREINFO_BUILD_ID(value) \
> > - vmcoreinfo_append_str("BUILD-ID=%s\n", value)
> > + vmcoreinfo_append_str("BUILD-ID=%20phN\n", value)
I may miss something, wondering why we need add '20' here.
>
> Please, add also build check that BUILD_ID_MAX == 20.
>
>
> > #define VMCOREINFO_PAGESIZE(value) \
> > vmcoreinfo_append_str("PAGESIZE=%ld\n", value)
> > #define VMCOREINFO_SYMBOL(name) \
> > @@ -69,10 +69,6 @@ extern unsigned char *vmcoreinfo_data;
> > extern size_t vmcoreinfo_size;
> > extern u32 *vmcoreinfo_note;
> >
> > -/* raw contents of kernel .notes section */
> > -extern const void __start_notes __weak;
> > -extern const void __stop_notes __weak;
> > -
> > Elf_Word *append_elf_note(Elf_Word *buf, char *name, unsigned int type,
> > void *data, size_t data_len);
> > void final_note(Elf_Word *buf);
> > diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> > index 825284baaf46..6b560cf9f374 100644
> > --- a/kernel/crash_core.c
> > +++ b/kernel/crash_core.c
> > @@ -4,6 +4,7 @@
> > * Copyright (C) 2002-2004 Eric Biederman
> > */
> >
> > +#include
> > #include
> > #include
> > #include
> > @@ -378,51 +379,13 @@ phys_addr_t __weak paddr_vmcoreinfo_note(void)
> > }
> > EXPORT_SYMBOL(paddr_vmcoreinfo_note);
> >
> > -#define NOTES_SIZE (&__stop_notes - &__start_notes)
> > -#define BUILD_ID_MAX SHA1_DIGEST_SIZE
> > -#define NT_GNU_BUILD_ID 3
> > -
> > -struct elf_note_section {
> > - struct elf_note n_hdr;
> > - u8 n_data[];
> > -};
> > -
> > /*
> > * Add build ID from .notes section as generated by the GNU ld(1)
> > * or LLVM lld(1) --build-id option.
> > */
> > static void add_build_id_vmcoreinfo(void)
> > {
> > - char build_id[BUILD_ID_MAX * 2 + 1];
> > - int n_remain = NOTES_SIZE;
> > -
> > - while (n_remain >= sizeof(struct elf_note)) {
> > - const struct elf_note_section *note_sec =
> > - &__start_notes + NOTES_SIZE - n_remain;
> > - const u32 n_namesz = note_sec->n_hdr.n_namesz;
> > -
> > - if (note_sec->n_hdr.n_type == NT_GNU_BUILD_ID &&
> > - n_namesz != 0 &&
> > - !strcmp((char *)¬e_sec->n_data[0], "GNU")) {
> > - if (note_sec->n_hdr.n_descsz <= BUILD_ID_MAX) {
> > - const u32 n_descsz = note_sec->n_hdr.n_descsz;
> > - const u8 *s = ¬e_sec->n_data[n_namesz];
> > -
> > - s = PTR_ALIGN(s, 4);
> > - bin2hex(build_id, s, n_descsz);
> > - build_id[2 * n_descsz] = '\0';
> > - VMCOREINFO_BUILD_ID(build_id);
> > - return;
> > - }
> > - pr_warn("Build ID is too large to include in
> > vmcoreinfo: %u > %u\n",
> > - note_sec->n_hdr.n_descsz,
> > - BUILD_ID_MAX);
> > - return;
> > - }
> > - n_remain -= sizeof(struct elf_note) +
> > - ALIGN(note_sec->n_hdr.n_namesz, 4) +
> > - ALIGN(note_sec->n_hdr.n_descsz, 4);
> > - }
> > + VMCOREINFO_BUILD_ID(vmlinux_build_id);
> > }
>
> The function add_build_id_vmcoreinfo() is used in
> crash_save_vmcoreinfo_init() in this context:
>
>
> VMCOREINFO_OSRELEASE(init_uts_ns.name.release);
> add_build_id_vmcoreinfo();
> VMCOREINFO_PAGESIZE(PAGE_SIZE);
>
> VMCOREINFO_SYMBOL(init_uts_ns);
> VMCOREINFO_OFFSET(uts_namespace, name);
> VMCOREINFO_SYMBOL(node_online_map);
>
> The function is not longer need. VMCOREINFO_BUILD_ID()
> can be used directly:
>
> VMCOREINFO_OSRELEASE(init_uts_ns.name.release);
> VMCOREINFO_BUILD_ID(vmlinux_build_id);
> VMCOREINFO_PAGESIZE(PAGE_SIZE);
>
> VMCOREINFO_SYMBOL(init_uts_ns);
> VMCOREINFO_OFFSET(uts_namespace, name);
> VMCOREINFO_SYMBOL(node_online_map);
>
>
> Best Regards,
> Petr
>
>
> >
> >
Re: [PATCH v3 12/12] kdump: Use vmlinux_build_id to simplify
Quoting Petr Mladek (2021-04-07 10:03:28)
> On Tue 2021-03-30 20:05:20, Stephen Boyd wrote:
> > We can use the vmlinux_build_id array here now instead of open coding
> > it. This mostly consolidates code.
> >
> > Cc: Jiri Olsa
> > Cc: Alexei Starovoitov
> > Cc: Jessica Yu
> > Cc: Evan Green
> > Cc: Hsin-Yi Wang
> > Cc: Dave Young
> > Cc: Baoquan He
> > Cc: Vivek Goyal
> > Cc:
> > Signed-off-by: Stephen Boyd
> > ---
> > include/linux/crash_core.h | 6 +-
> > kernel/crash_core.c| 41 ++
> > 2 files changed, 3 insertions(+), 44 deletions(-)
> >
> > diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
> > index 206bde8308b2..fb8ab99bb2ee 100644
> > --- a/include/linux/crash_core.h
> > +++ b/include/linux/crash_core.h
> > @@ -39,7 +39,7 @@ phys_addr_t paddr_vmcoreinfo_note(void);
> > #define VMCOREINFO_OSRELEASE(value) \
> > vmcoreinfo_append_str("OSRELEASE=%s\n", value)
> > #define VMCOREINFO_BUILD_ID(value) \
> > - vmcoreinfo_append_str("BUILD-ID=%s\n", value)
> > + vmcoreinfo_append_str("BUILD-ID=%20phN\n", value)
>
> Please, add also build check that BUILD_ID_MAX == 20.
>
I added a BUILD_BUG_ON() in kernel/crash_core.c. I tried static_assert()
here but got mixed ISO errors from gcc-10, although it feels like it
should work.
In file included from ./arch/arm64/include/asm/cmpxchg.h:10,
from ./arch/arm64/include/asm/atomic.h:16,
from ./include/linux/atomic.h:7,
from ./include/linux/mm_types_task.h:13,
from ./include/linux/mm_types.h:5,
from ./include/linux/buildid.h:5,
from kernel/crash_core.c:7:
kernel/crash_core.c: In function 'crash_save_vmcoreinfo_init':
./include/linux/build_bug.h:78:41: warning: ISO C90 forbids mixed declarations
and code [-Wdeclaration-after-statement]
78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
| ^~
./include/linux/build_bug.h:77:34: note: in expansion of macro '__static_assert'
77 | #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__,
#expr)
| ^~~
./include/linux/crash_core.h:42:2: note: in expansion of macro 'static_assert'
42 | static_assert(ARRAY_SIZE(value) == BUILD_ID_SIZE_MAX); \
| ^
kernel/crash_core.c:401:2: note: in expansion of macro 'VMCOREINFO_BUILD_ID'
401 | VMCOREINFO_BUILD_ID(vmlinux_build_id);
>
> The function add_build_id_vmcoreinfo() is used in
> crash_save_vmcoreinfo_init() in this context:
>
>
> VMCOREINFO_OSRELEASE(init_uts_ns.name.release);
> add_build_id_vmcoreinfo();
> VMCOREINFO_PAGESIZE(PAGE_SIZE);
>
> VMCOREINFO_SYMBOL(init_uts_ns);
> VMCOREINFO_OFFSET(uts_namespace, name);
> VMCOREINFO_SYMBOL(node_online_map);
>
> The function is not longer need. VMCOREINFO_BUILD_ID()
> can be used directly:
>
> VMCOREINFO_OSRELEASE(init_uts_ns.name.release);
> VMCOREINFO_BUILD_ID(vmlinux_build_id);
> VMCOREINFO_PAGESIZE(PAGE_SIZE);
>
> VMCOREINFO_SYMBOL(init_uts_ns);
> VMCOREINFO_OFFSET(uts_namespace, name);
> VMCOREINFO_SYMBOL(node_online_map);
>
>
Thanks. Makes sense. I've rolled that in.
Re: [PATCH v3 12/12] kdump: Use vmlinux_build_id to simplify
On Tue 2021-03-30 20:05:20, Stephen Boyd wrote:
> We can use the vmlinux_build_id array here now instead of open coding
> it. This mostly consolidates code.
>
> Cc: Jiri Olsa
> Cc: Alexei Starovoitov
> Cc: Jessica Yu
> Cc: Evan Green
> Cc: Hsin-Yi Wang
> Cc: Dave Young
> Cc: Baoquan He
> Cc: Vivek Goyal
> Cc:
> Signed-off-by: Stephen Boyd
> ---
> include/linux/crash_core.h | 6 +-
> kernel/crash_core.c| 41 ++
> 2 files changed, 3 insertions(+), 44 deletions(-)
>
> diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
> index 206bde8308b2..fb8ab99bb2ee 100644
> --- a/include/linux/crash_core.h
> +++ b/include/linux/crash_core.h
> @@ -39,7 +39,7 @@ phys_addr_t paddr_vmcoreinfo_note(void);
> #define VMCOREINFO_OSRELEASE(value) \
> vmcoreinfo_append_str("OSRELEASE=%s\n", value)
> #define VMCOREINFO_BUILD_ID(value) \
> - vmcoreinfo_append_str("BUILD-ID=%s\n", value)
> + vmcoreinfo_append_str("BUILD-ID=%20phN\n", value)
Please, add also build check that BUILD_ID_MAX == 20.
> #define VMCOREINFO_PAGESIZE(value) \
> vmcoreinfo_append_str("PAGESIZE=%ld\n", value)
> #define VMCOREINFO_SYMBOL(name) \
> @@ -69,10 +69,6 @@ extern unsigned char *vmcoreinfo_data;
> extern size_t vmcoreinfo_size;
> extern u32 *vmcoreinfo_note;
>
> -/* raw contents of kernel .notes section */
> -extern const void __start_notes __weak;
> -extern const void __stop_notes __weak;
> -
> Elf_Word *append_elf_note(Elf_Word *buf, char *name, unsigned int type,
> void *data, size_t data_len);
> void final_note(Elf_Word *buf);
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index 825284baaf46..6b560cf9f374 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -4,6 +4,7 @@
> * Copyright (C) 2002-2004 Eric Biederman
> */
>
> +#include
> #include
> #include
> #include
> @@ -378,51 +379,13 @@ phys_addr_t __weak paddr_vmcoreinfo_note(void)
> }
> EXPORT_SYMBOL(paddr_vmcoreinfo_note);
>
> -#define NOTES_SIZE (&__stop_notes - &__start_notes)
> -#define BUILD_ID_MAX SHA1_DIGEST_SIZE
> -#define NT_GNU_BUILD_ID 3
> -
> -struct elf_note_section {
> - struct elf_note n_hdr;
> - u8 n_data[];
> -};
> -
> /*
> * Add build ID from .notes section as generated by the GNU ld(1)
> * or LLVM lld(1) --build-id option.
> */
> static void add_build_id_vmcoreinfo(void)
> {
> - char build_id[BUILD_ID_MAX * 2 + 1];
> - int n_remain = NOTES_SIZE;
> -
> - while (n_remain >= sizeof(struct elf_note)) {
> - const struct elf_note_section *note_sec =
> - &__start_notes + NOTES_SIZE - n_remain;
> - const u32 n_namesz = note_sec->n_hdr.n_namesz;
> -
> - if (note_sec->n_hdr.n_type == NT_GNU_BUILD_ID &&
> - n_namesz != 0 &&
> - !strcmp((char *)¬e_sec->n_data[0], "GNU")) {
> - if (note_sec->n_hdr.n_descsz <= BUILD_ID_MAX) {
> - const u32 n_descsz = note_sec->n_hdr.n_descsz;
> - const u8 *s = ¬e_sec->n_data[n_namesz];
> -
> - s = PTR_ALIGN(s, 4);
> - bin2hex(build_id, s, n_descsz);
> - build_id[2 * n_descsz] = '\0';
> - VMCOREINFO_BUILD_ID(build_id);
> - return;
> - }
> - pr_warn("Build ID is too large to include in
> vmcoreinfo: %u > %u\n",
> - note_sec->n_hdr.n_descsz,
> - BUILD_ID_MAX);
> - return;
> - }
> - n_remain -= sizeof(struct elf_note) +
> - ALIGN(note_sec->n_hdr.n_namesz, 4) +
> - ALIGN(note_sec->n_hdr.n_descsz, 4);
> - }
> + VMCOREINFO_BUILD_ID(vmlinux_build_id);
> }
The function add_build_id_vmcoreinfo() is used in
crash_save_vmcoreinfo_init() in this context:
VMCOREINFO_OSRELEASE(init_uts_ns.name.release);
add_build_id_vmcoreinfo();
VMCOREINFO_PAGESIZE(PAGE_SIZE);
VMCOREINFO_SYMBOL(init_uts_ns);
VMCOREINFO_OFFSET(uts_namespace, name);
VMCOREINFO_SYMBOL(node_online_map);
The function is not longer need. VMCOREINFO_BUILD_ID()
can be used directly:
VMCOREINFO_OSRELEASE(init_uts_ns.name.release);
VMCOREINFO_BUILD_ID(vmlinux_build_id);
VMCOREINFO_PAGESIZE(PAGE_SIZE);
VMCOREINFO_SYMBOL(init_uts_ns);
VMCOREINFO_OFFSET(uts_namespace, name);
VMCOREINFO_SYMBOL(node_online_map);
Best Regards,
Petr
>
> static int __init crash_save_vmcoreinfo_init(void)
> --
> https://chromeos.dev
[PATCH v3 12/12] kdump: Use vmlinux_build_id to simplify
We can use the vmlinux_build_id array here now instead of open coding
it. This mostly consolidates code.
Cc: Jiri Olsa
Cc: Alexei Starovoitov
Cc: Jessica Yu
Cc: Evan Green
Cc: Hsin-Yi Wang
Cc: Dave Young
Cc: Baoquan He
Cc: Vivek Goyal
Cc:
Signed-off-by: Stephen Boyd
---
include/linux/crash_core.h | 6 +-
kernel/crash_core.c| 41 ++
2 files changed, 3 insertions(+), 44 deletions(-)
diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
index 206bde8308b2..fb8ab99bb2ee 100644
--- a/include/linux/crash_core.h
+++ b/include/linux/crash_core.h
@@ -39,7 +39,7 @@ phys_addr_t paddr_vmcoreinfo_note(void);
#define VMCOREINFO_OSRELEASE(value) \
vmcoreinfo_append_str("OSRELEASE=%s\n", value)
#define VMCOREINFO_BUILD_ID(value) \
- vmcoreinfo_append_str("BUILD-ID=%s\n", value)
+ vmcoreinfo_append_str("BUILD-ID=%20phN\n", value)
#define VMCOREINFO_PAGESIZE(value) \
vmcoreinfo_append_str("PAGESIZE=%ld\n", value)
#define VMCOREINFO_SYMBOL(name) \
@@ -69,10 +69,6 @@ extern unsigned char *vmcoreinfo_data;
extern size_t vmcoreinfo_size;
extern u32 *vmcoreinfo_note;
-/* raw contents of kernel .notes section */
-extern const void __start_notes __weak;
-extern const void __stop_notes __weak;
-
Elf_Word *append_elf_note(Elf_Word *buf, char *name, unsigned int type,
void *data, size_t data_len);
void final_note(Elf_Word *buf);
diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index 825284baaf46..6b560cf9f374 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -4,6 +4,7 @@
* Copyright (C) 2002-2004 Eric Biederman
*/
+#include
#include
#include
#include
@@ -378,51 +379,13 @@ phys_addr_t __weak paddr_vmcoreinfo_note(void)
}
EXPORT_SYMBOL(paddr_vmcoreinfo_note);
-#define NOTES_SIZE (&__stop_notes - &__start_notes)
-#define BUILD_ID_MAX SHA1_DIGEST_SIZE
-#define NT_GNU_BUILD_ID 3
-
-struct elf_note_section {
- struct elf_note n_hdr;
- u8 n_data[];
-};
-
/*
* Add build ID from .notes section as generated by the GNU ld(1)
* or LLVM lld(1) --build-id option.
*/
static void add_build_id_vmcoreinfo(void)
{
- char build_id[BUILD_ID_MAX * 2 + 1];
- int n_remain = NOTES_SIZE;
-
- while (n_remain >= sizeof(struct elf_note)) {
- const struct elf_note_section *note_sec =
- &__start_notes + NOTES_SIZE - n_remain;
- const u32 n_namesz = note_sec->n_hdr.n_namesz;
-
- if (note_sec->n_hdr.n_type == NT_GNU_BUILD_ID &&
- n_namesz != 0 &&
- !strcmp((char *)¬e_sec->n_data[0], "GNU")) {
- if (note_sec->n_hdr.n_descsz <= BUILD_ID_MAX) {
- const u32 n_descsz = note_sec->n_hdr.n_descsz;
- const u8 *s = ¬e_sec->n_data[n_namesz];
-
- s = PTR_ALIGN(s, 4);
- bin2hex(build_id, s, n_descsz);
- build_id[2 * n_descsz] = '\0';
- VMCOREINFO_BUILD_ID(build_id);
- return;
- }
- pr_warn("Build ID is too large to include in
vmcoreinfo: %u > %u\n",
- note_sec->n_hdr.n_descsz,
- BUILD_ID_MAX);
- return;
- }
- n_remain -= sizeof(struct elf_note) +
- ALIGN(note_sec->n_hdr.n_namesz, 4) +
- ALIGN(note_sec->n_hdr.n_descsz, 4);
- }
+ VMCOREINFO_BUILD_ID(vmlinux_build_id);
}
static int __init crash_save_vmcoreinfo_init(void)
--
https://chromeos.dev
