Re: [PATCH v2 02/12] buildid: Add method to get running kernel's build ID
Quoting Rasmus Villemoes (2021-03-24 02:24:27)
> On 24/03/2021 03.04, Stephen Boyd wrote:
> > Add vmlinux_build_id() so that callers can print a hex format string
> > representation of the running kernel's build ID. This will be used in
> > the kdump and dump_stack code so that developers can easily locate the
> > vmlinux debug symbols for a crash/stacktrace.
> >
> > 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/buildid.h | 2 ++
> > lib/buildid.c | 19 +++
> > 2 files changed, 21 insertions(+)
> >
> > diff --git a/include/linux/buildid.h b/include/linux/buildid.h
> > index ebce93f26d06..2ff6b1b7cc9b 100644
> > --- a/include/linux/buildid.h
> > +++ b/include/linux/buildid.h
> > @@ -10,4 +10,6 @@ int build_id_parse(struct vm_area_struct *vma, unsigned
> > char *build_id,
> > __u32 *size);
> > int build_id_parse_buf(const void *buf, unsigned char *build_id, u32
> > buf_size);
> >
> > +const unsigned char *vmlinux_build_id(void);
> > +
> > #endif
> > diff --git a/lib/buildid.c b/lib/buildid.c
> > index 010ab0674cb9..fa1b6466b4b8 100644
> > --- a/lib/buildid.c
> > +++ b/lib/buildid.c
> > @@ -4,6 +4,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #define BUILD_ID 3
> >
> > @@ -171,3 +172,21 @@ int build_id_parse_buf(const void *buf, unsigned char
> > *build_id, u32 buf_size)
> > {
> > return parse_build_id_buf(build_id, NULL, buf, buf_size);
> > }
> > +
> > +/**
> > + * vmlinux_build_id - Get the running kernel's build ID
> > + *
> > + * Return: Running kernel's build ID
> > + */
> > +const unsigned char *vmlinux_build_id(void)
> > +{
> > + extern const void __start_notes __weak;
> > + extern const void __stop_notes __weak;
> > + unsigned int size = &__stop_notes - &__start_notes;
> > + static unsigned char vmlinux_build_id[BUILD_ID_SIZE_MAX];
> > +
> > + if (!memchr_inv(vmlinux_build_id, 0, BUILD_ID_SIZE_MAX))
> > + build_id_parse_buf(&__start_notes, vmlinux_build_id, size);
> > +
> > + return vmlinux_build_id;
> > +}
> >
>
> Hm, is there any reason to do that initialization lazily and thus need
> an accessor? If the system is coming down hard, there's a (very very
> small) risk that one thread starts finding the build id, is in the
> middle of the memcpy, another thread also ends up wanting the vmlinux
> build id, sees some non-nul byte, and proceeds to using the partially
> written vmlinux_build_id.
>
> Perhaps consider just exposing the vmlinux_build_id[] array itself,
> adding a init_vmlinux_build_id() call somewhere early in start_kernel().
>
> It could then also be made __ro_after_init.
>
> In any case, if you decide to keep the current way, please rename the
> local variable (just "build_id" is fine) so that it doesn't shadow the
> very function it resides in, that's very confusing.
>
No particular reason to do it this way. I'll take that approach to
initialize it early in start_kernel() and then expose the array instead.
Thanks!
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v2 00/12] Add build ID to stacktraces
HTML mail? Quoting Konstantin Khlebnikov (2021-03-24 01:23:55) > 24.03.2021, 05:04, "Stephen Boyd" : > > Looks too noisy for me. Maybe print id in the line "Modules linked in:"? > I suppose only out-of-tree modules need this? > Please see this note in patch 4: Originally, I put this on the %pS format, but that was quickly rejected given that %pS is used in other places such as ftrace where build IDs aren't meaningful. There was some discussions on the list to put every module build ID into the "Modules linked in:" section of the stacktrace message but that quickly becomes very hard to read once you have more than three or four modules linked in. It also provides too much information when we don't expect each module to be traversed in a stacktrace. Having the build ID for modules that aren't important just makes things messy. Splitting it to multiple lines for each module quickly explodes the number of lines printed in an oops too, possibly wrapping the warning off the console. And finally, trying to stash away each module used in a callstack to provide the ID of each symbol printed is cumbersome and would require changes to each architecture to stash away modules and return their build IDs once unwinding has completed. ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v1 1/3] kernel/resource: make walk_system_ram_res() find all busy IORESOURCE_SYSTEM_RAM resources
On 24.03.21 12:18, Oscar Salvador wrote: On Mon, Mar 22, 2021 at 05:01:58PM +0100, David Hildenbrand wrote: It used to be true that we can have busy system RAM only on the first level in the resourc tree. However, this is no longer holds for driver-managed system RAM (i.e., added via dax/kmem and virtio-mem), which gets added on lower levels. Let me ask some rookie questions: What does "busy" term stand for here? IORESOURCE_BUSY - here: actually added, not just some reserved range / container. Why resources coming from virtio-mem are added at a lower levels? Some information can be had from ebf71552bb0e690cad523ad175e8c4c89a33c333 commit ebf71552bb0e690cad523ad175e8c4c89a33c333 Author: David Hildenbrand Date: Thu May 7 16:01:35 2020 +0200 virtio-mem: Add parent resource for all added "System RAM" Let's add a parent resource, named after the virtio device (inspired by drivers/dax/kmem.c). This allows user space to identify which memory belongs to which virtio-mem device. With this change and two virtio-mem devices: :/# cat /proc/iomem -0fff : Reserved 1000-0009fbff : System RAM [...] 14000-333ff : virtio0 14000-147ff : System RAM 14800-14fff : System RAM 15000-157ff : System RAM [...] 33400-3033ff : virtio1 33800-33fff : System RAM 34000-347ff : System RAM 34800-34fff : System RAM [...] For dax/kmem it comes naturally due to the "Persistent Memory" and device parent resources like: 14000-33fff : Persistent Memory 14000-1481f : namespace0.0 15000-33fff : dax0.0 15000-33fff : System RAM (kmem) 328000-32 : PCI Bus :00 Thanks -- Thanks, David / dhildenb ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v1 1/3] kernel/resource: make walk_system_ram_res() find all busy IORESOURCE_SYSTEM_RAM resources
On Mon, Mar 22, 2021 at 05:01:58PM +0100, David Hildenbrand wrote:
> It used to be true that we can have busy system RAM only on the first level
> in the resourc tree. However, this is no longer holds for driver-managed
> system RAM (i.e., added via dax/kmem and virtio-mem), which gets added on
> lower levels.
Let me ask some rookie questions:
What does "busy" term stand for here?
Why resources coming from virtio-mem are added at a lower levels?
>
> We have two users of walk_system_ram_res(), which currently only
> consideres the first level:
> a) kernel/kexec_file.c:kexec_walk_resources() -- We properly skip
>IORESOURCE_SYSRAM_DRIVER_MANAGED resources via
>locate_mem_hole_callback(), so even after this change, we won't be
>placing kexec images onto dax/kmem and virtio-mem added memory. No
>change.
> b) arch/x86/kernel/crash.c:fill_up_crash_elf_data() -- we're currently
>not adding relevant ranges to the crash elf info, resulting in them
>not getting dumped via kdump.
>
> This change fixes loading a crashkernel via kexec_file_load() and including
> dax/kmem and virtio-mem added System RAM in the crashdump on x86-64. Note
> that e.g,, arm64 relies on memblock data and, therefore, always considers
> all added System RAM already.
>
> Let's find all busy IORESOURCE_SYSTEM_RAM resources, making the function
> behave like walk_system_ram_range().
>
> Cc: Andrew Morton
> Cc: Greg Kroah-Hartman
> Cc: Dan Williams
> Cc: Daniel Vetter
> Cc: Andy Shevchenko
> Cc: Mauro Carvalho Chehab
> Cc: Signed-off-by: David Hildenbrand
> Cc: Dave Young
> Cc: Baoquan He
> Cc: Vivek Goyal
> Cc: Dave Hansen
> Cc: Keith Busch
> Cc: Michal Hocko
> Cc: Qian Cai
> Cc: Oscar Salvador
> Cc: Eric Biederman
> Cc: Thomas Gleixner
> Cc: Ingo Molnar
> Cc: Borislav Petkov
> Cc: "H. Peter Anvin"
> Cc: Tom Lendacky
> Cc: Brijesh Singh
> Cc: x...@kernel.org
> Cc: kexec@lists.infradead.org
> Signed-off-by: David Hildenbrand
> ---
> kernel/resource.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 627e61b0c124..4efd6e912279 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -457,7 +457,7 @@ int walk_system_ram_res(u64 start, u64 end, void *arg,
> {
> unsigned long flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
>
> - return __walk_iomem_res_desc(start, end, flags, IORES_DESC_NONE, true,
> + return __walk_iomem_res_desc(start, end, flags, IORES_DESC_NONE, false,
>arg, func);
> }
>
> --
> 2.29.2
>
>
--
Oscar Salvador
SUSE L3
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v2 02/12] buildid: Add method to get running kernel's build ID
On 24/03/2021 03.04, Stephen Boyd wrote:
> Add vmlinux_build_id() so that callers can print a hex format string
> representation of the running kernel's build ID. This will be used in
> the kdump and dump_stack code so that developers can easily locate the
> vmlinux debug symbols for a crash/stacktrace.
>
> 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/buildid.h | 2 ++
> lib/buildid.c | 19 +++
> 2 files changed, 21 insertions(+)
>
> diff --git a/include/linux/buildid.h b/include/linux/buildid.h
> index ebce93f26d06..2ff6b1b7cc9b 100644
> --- a/include/linux/buildid.h
> +++ b/include/linux/buildid.h
> @@ -10,4 +10,6 @@ int build_id_parse(struct vm_area_struct *vma, unsigned
> char *build_id,
> __u32 *size);
> int build_id_parse_buf(const void *buf, unsigned char *build_id, u32
> buf_size);
>
> +const unsigned char *vmlinux_build_id(void);
> +
> #endif
> diff --git a/lib/buildid.c b/lib/buildid.c
> index 010ab0674cb9..fa1b6466b4b8 100644
> --- a/lib/buildid.c
> +++ b/lib/buildid.c
> @@ -4,6 +4,7 @@
> #include
> #include
> #include
> +#include
>
> #define BUILD_ID 3
>
> @@ -171,3 +172,21 @@ int build_id_parse_buf(const void *buf, unsigned char
> *build_id, u32 buf_size)
> {
> return parse_build_id_buf(build_id, NULL, buf, buf_size);
> }
> +
> +/**
> + * vmlinux_build_id - Get the running kernel's build ID
> + *
> + * Return: Running kernel's build ID
> + */
> +const unsigned char *vmlinux_build_id(void)
> +{
> + extern const void __start_notes __weak;
> + extern const void __stop_notes __weak;
> + unsigned int size = &__stop_notes - &__start_notes;
> + static unsigned char vmlinux_build_id[BUILD_ID_SIZE_MAX];
> +
> + if (!memchr_inv(vmlinux_build_id, 0, BUILD_ID_SIZE_MAX))
> + build_id_parse_buf(&__start_notes, vmlinux_build_id, size);
> +
> + return vmlinux_build_id;
> +}
>
Hm, is there any reason to do that initialization lazily and thus need
an accessor? If the system is coming down hard, there's a (very very
small) risk that one thread starts finding the build id, is in the
middle of the memcpy, another thread also ends up wanting the vmlinux
build id, sees some non-nul byte, and proceeds to using the partially
written vmlinux_build_id.
Perhaps consider just exposing the vmlinux_build_id[] array itself,
adding a init_vmlinux_build_id() call somewhere early in start_kernel().
It could then also be made __ro_after_init.
In any case, if you decide to keep the current way, please rename the
local variable (just "build_id" is fine) so that it doesn't shadow the
very function it resides in, that's very confusing.
Rasmus
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v2 00/12] Add build ID to stacktraces
On Tue, Mar 23, 2021 at 07:04:31PM -0700, Stephen Boyd wrote: > x5 : x4 : 0001 > x3 : 0008 x2 : ff93fef25a70 > x1 : ff93fef15788 x0 : ffe3622352e0 > Call trace: > lkdtm_WARNING+0x28/0x30 [lkdtm ed5019fdf5e53be37cb1ba7899292d7e143b259e] > direct_entry+0x16c/0x1b4 [lkdtm ed5019fdf5e53be37cb1ba7899292d7e143b259e] Yikes. No, please do not make the backtraces a complete mess for something that serves absolutely no need. ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
