Re: [PATCH V6 0/6] RISC-V fixups to work with crash tool
On Wed, Oct 12, 2022 at 05:42:37PM +0800, Xianting Tian wrote:
>
> 在 2022/8/12 上午12:17, Palmer Dabbelt 写道:
> > On Thu, 11 Aug 2022 00:41:44 PDT (-0700),
> > xianting.t...@linux.alibaba.com wrote:
> > > I ever sent the patch 1 in the link:
> > > https://patchwork.kernel.org/project/linux-riscv/patch/20220708073150.352830-3-xianting.t...@linux.alibaba.com/
> > >
> > > And patch 2,3 in the link:
> > > https://patchwork.kernel.org/project/linux-riscv/patch/20220714113300.367854-2-xianting.t...@linux.alibaba.com/
> > >
> > > https://patchwork.kernel.org/project/linux-riscv/patch/20220714113300.367854-3-xianting.t...@linux.alibaba.com/
> > >
> > >
> > > This patch set just put these patches together, and with three new
> > > patch 4, 5, 6.
> > > these six patches are the fixups for machine_kexec, kernel mode PC
> > > for vmcore
> > > and improvements for vmcoreinfo, memory layout dump and fixup
> > > schedule out issue
> > > in machine_crash_shutdown().
> > >
> > > The main changes in the six patchs as below,
> > > Patch 1: Fixup use of smp_processor_id() in preemptible context, to
> > > cleanup
> > > the console prints.
> > > Patch 2: Fixup to get correct kernel mode PC for kernel mode regs
> > > for vmcore.
> > > Patch 3: Fixup schedule out issue in machine_crash_shutdown()
> > > Patch 4: Add modules to virtual kernel memory layout dump.
> > > Patch 5: Add VM layout, va bits, ram base to vmcoreinfo, which can
> > > simplify
> > > the development of crash tool as ARM64 already did
> > > (arch/arm64/kernel/crash_core.c).
> > > Patch 6: Updates vmcoreinfo.rst for vmcoreinfo export for RISCV64.
> > >
> > > With these six patches(patch 2 is must), crash tool can work well to
> > > analyze
> > > a vmcore. The patches for crash tool for RISCV64 is in the link:
> > > https://lore.kernel.org/linux-riscv/20220801043040.2003264-1-xianting.t...@linux.alibaba.com/
> > >
> > >
> > > --
> > > Changes v1 -> v2:
> > > 1, remove the patch "Add a fast call path of crash_kexec()" from
> > > this series
> > > of patches, as it already applied to riscv git.
> > > https://git.kernel.org/pub/scm/linux/kernel/git/riscv/linux.git/commit/?h=for-next&id=3f1901110a89b0e2e13adb2ac8d1a7102879ea98
> > > 2, add 'Reviewed-by' based on the comments of v1.
> > > Changes v2 -> v3:
> > > use "riscv" instead of "riscv64" in patch 5 subject line.
> > > Changes v3 -> v4:
> > > use "riscv" instead of "riscv64" in the summary of patch 5 subject
> > > line.
> > > Changes v4 -> v5:
> > > add a new patch "RISC-V: Fixup schedule out issue in
> > > machine_crash_shutdown()"
> > > Changes v5 -> v6:
> > > 1, move "fixup" patches to the start of the patch set.
> > > 2, change patch 1, 2, 6's subject to make it tell more what it's
> > > about.
> > > 3, add Fixes for patch 3.
> > > 4, adjuest the changes format for patch 6.
> > >
> > >
> > > Xianting Tian (6):
> > > RISC-V: kexec: Fixup use of smp_processor_id() in preemptible context
> > > RISC-V: Fixup get incorrect user mode PC for kernel mode regs
> > > RISC-V: Fixup schedule out issue in machine_crash_shutdown()
> > > RISC-V: Add modules to virtual kernel memory layout dump
> > > RISC-V: Add arch_crash_save_vmcoreinfo support
> > > Documentation: kdump: describe VMCOREINFO export for RISCV64
> > >
> > > .../admin-guide/kdump/vmcoreinfo.rst | 31 +++
> > > arch/riscv/kernel/Makefile | 1 +
> > > arch/riscv/kernel/crash_core.c | 29 +
> > > arch/riscv/kernel/crash_save_regs.S | 2 +-
> > > arch/riscv/kernel/machine_kexec.c | 28 ++---
> > > arch/riscv/mm/init.c | 4 +++
> > > 6 files changed, 89 insertions(+), 6 deletions(-)
> > > create mode 100644 arch/riscv/kernel/crash_core.c
> >
> > Thank. I've taken the first 4 onto for-next, which is still targeted
> > for 5.20, as they're fixes. I'm not opposed to taking the documentation
> > patch for this cycle as well, it just needs some going-over as the
> > wording looks very odd (or at least it does to me right now, maybe I'm
> > just still half asleep). Patch 5 is a new feature, and given that it's
> > being spun during the merge window it's too late.
>
> Hi Palmer
>
> Do you plan to merge the two patch to Linux 6.1 to support crash tool work?
> thanks
>
> RISC-V: kexec: Fixup use of smp_processor_id() in preemptible context
357628e68f5c ("RISC-V: kexec: Fixup use of smp_processor_id() in preemptible
context"
> RISC-V: Fixup get incorrect user mode PC for kernel mode regs
59c026c359c3 ("RISC-V: Fixup get incorrect user mode PC for kernel mode regs")
Hey Xianting, those two commits already seem to have been applied, do
you perhaps instead mean the documentation patch and the addition of
support for arch_crash_save_vmcoreinfo? I recalled asking if you needed
to respin at the time, but do not see a response:
https://lore.kernel.org/li
Re: [PATCH V6 0/6] RISC-V fixups to work with crash tool
On 13 October 2022 03:28:09 IST, Xianting Tian wrote: > >在 2022/10/12 下午5:52, Conor Dooley 写道: >> On Wed, Oct 12, 2022 at 05:42:37PM +0800, Xianting Tian wrote: >>> 在 2022/8/12 上午12:17, Palmer Dabbelt 写道: >>>> On Thu, 11 Aug 2022 00:41:44 PDT (-0700), >>>> xianting.t...@linux.alibaba.com wrote: >>>>> I ever sent the patch 1 in the link: >>>>> https://patchwork.kernel.org/project/linux-riscv/patch/20220708073150.352830-3-xianting.t...@linux.alibaba.com/ >>>>> >>>>> And patch 2,3 in the link: >>>>> https://patchwork.kernel.org/project/linux-riscv/patch/20220714113300.367854-2-xianting.t...@linux.alibaba.com/ >>>>> >>>>> https://patchwork.kernel.org/project/linux-riscv/patch/20220714113300.367854-3-xianting.t...@linux.alibaba.com/ >>>>> >>>>> >>>>> This patch set just put these patches together, and with three new >>>>> patch 4, 5, 6. >>>>> these six patches are the fixups for machine_kexec, kernel mode PC >>>>> for vmcore >>>>> and improvements for vmcoreinfo, memory layout dump and fixup >>>>> schedule out issue >>>>> in machine_crash_shutdown(). >>>>> >>>>> The main changes in the six patchs as below, >>>>> Patch 1: Fixup use of smp_processor_id() in preemptible context, to >>>>> cleanup >>>>> the console prints. >>>>> Patch 2: Fixup to get correct kernel mode PC for kernel mode regs >>>>> for vmcore. >>>>> Patch 3: Fixup schedule out issue in machine_crash_shutdown() >>>>> Patch 4: Add modules to virtual kernel memory layout dump. >>>>> Patch 5: Add VM layout, va bits, ram base to vmcoreinfo, which can >>>>> simplify >>>>> the development of crash tool as ARM64 already did >>>>> (arch/arm64/kernel/crash_core.c). >>>>> Patch 6: Updates vmcoreinfo.rst for vmcoreinfo export for RISCV64. >>>>> >>>>> With these six patches(patch 2 is must), crash tool can work well to >>>>> analyze >>>>> a vmcore. The patches for crash tool for RISCV64 is in the link: >>>>> https://lore.kernel.org/linux-riscv/20220801043040.2003264-1-xianting.t...@linux.alibaba.com/ >>>>> >>>>> >>>>> -- >>>>> Changes v1 -> v2: >>>>> 1, remove the patch "Add a fast call path of crash_kexec()" from >>>>> this series >>>>> of patches, as it already applied to riscv git. >>>>> https://git.kernel.org/pub/scm/linux/kernel/git/riscv/linux.git/commit/?h=for-next&id=3f1901110a89b0e2e13adb2ac8d1a7102879ea98 >>>>> 2, add 'Reviewed-by' based on the comments of v1. >>>>> Changes v2 -> v3: >>>>> use "riscv" instead of "riscv64" in patch 5 subject line. >>>>> Changes v3 -> v4: >>>>> use "riscv" instead of "riscv64" in the summary of patch 5 subject >>>>> line. >>>>> Changes v4 -> v5: >>>>> add a new patch "RISC-V: Fixup schedule out issue in >>>>> machine_crash_shutdown()" >>>>> Changes v5 -> v6: >>>>> 1, move "fixup" patches to the start of the patch set. >>>>> 2, change patch 1, 2, 6's subject to make it tell more what it's >>>>> about. >>>>> 3, add Fixes for patch 3. >>>>> 4, adjuest the changes format for patch 6. >>>>> >>>>> >>>>> Xianting Tian (6): >>>>> RISC-V: kexec: Fixup use of smp_processor_id() in preemptible context >>>>> RISC-V: Fixup get incorrect user mode PC for kernel mode regs >>>>> RISC-V: Fixup schedule out issue in machine_crash_shutdown() >>>>> RISC-V: Add modules to virtual kernel memory layout dump >>>>> RISC-V: Add arch_crash_save_vmcoreinfo support >>>>> Documentation: kdump: describe VMCOREINFO export for RISCV64 >>>>> >>>>> .../admin-guide/kdump/vmcoreinfo.rst | 31 +++ >>>>> arch/riscv/kernel/Makefile | 1 + >>>>> arch/riscv/kernel/crash_core.c | 29 + >>>>> arch/riscv/kernel/crash_save_regs.S | 2 +- >>>>> arch
Re: [PATCH 2/2] Documentation: kdump: describe VMCOREINFO export for RISCV64
Hey Bagas, On Fri, Oct 14, 2022 at 08:01:32PM +0700, Bagas Sanjaya wrote: > On Fri, Oct 14, 2022 at 03:48:10PM +0800, Xianting Tian wrote: > > The following interrelated definitions and ranges are needed by the kdump > > crash tool, they are exported by "arch/riscv/kernel/crash_core.c": > > VA_BITS, > > PAGE_OFFSET, > > phys_ram_base, > > MODULES_VADDR ~ MODULES_END, > > VMALLOC_START ~ VMALLOC_END, > > VMEMMAP_START ~ VMEMMAP_END, > > KASAN_SHADOW_START ~ KASAN_SHADOW_END, > > KERNEL_LINK_ADDR ~ ADDRESS_SPACE_END > > > > Document these RISCV64 exports above. > > > > The patch description LGTM, thanks. > > > + > > +MODULES_VADDR|MODULES_END|VMALLOC_START|VMALLOC_END|VMEMMAP_START|VMEMMAP_END|KASAN_SHADOW_START|KASAN_SHADOW_END|KERNEL_LINK_ADDR|ADDRESS_SPACE_END > > + > > The overline above header text is unnecessary, so I have to strip it: > > >8 > > diff --git a/Documentation/admin-guide/kdump/vmcoreinfo.rst > b/Documentation/admin-guide/kdump/vmcoreinfo.rst > index 6c7a1728de220e..8e2e164cf3db49 100644 > --- a/Documentation/admin-guide/kdump/vmcoreinfo.rst > +++ b/Documentation/admin-guide/kdump/vmcoreinfo.rst > @@ -615,7 +615,6 @@ phys_ram_base > > Indicates the start physical RAM address. > > - > > MODULES_VADDR|MODULES_END|VMALLOC_START|VMALLOC_END|VMEMMAP_START|VMEMMAP_END|KASAN_SHADOW_START|KASAN_SHADOW_END|KERNEL_LINK_ADDR|ADDRESS_SPACE_END > > Without whitespace highlighting, your change threw me for a sec.. But yeah, having the overline is inconsistent with other headings in the doc. What I wanted to ask about was the linelength as I don't know anything about rst. Is it possible to avoid having the ~150 character line or is that a necessary evil? Thanks, Conor. > > Thanks. > > -- > An old man doll... just what I always wanted! - Clara ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH V2 2/2] Documentation: kdump: describe VMCOREINFO export for RISCV64
On Fri, Oct 14, 2022 at 09:41:39PM +0800, Xianting Tian wrote: > The following interrelated definitions and ranges are needed by the kdump > crash tool, they are exported by "arch/riscv/kernel/crash_core.c": > VA_BITS, > PAGE_OFFSET, > phys_ram_base, > MODULES_VADDR ~ MODULES_END, > VMALLOC_START ~ VMALLOC_END, > VMEMMAP_START ~ VMEMMAP_END, > KASAN_SHADOW_START ~ KASAN_SHADOW_END, > KERNEL_LINK_ADDR ~ ADDRESS_SPACE_END > > Document these RISCV64 exports above. > > Signed-off-by: Xianting Tian > --- > .../admin-guide/kdump/vmcoreinfo.rst | 30 +++ > 1 file changed, 30 insertions(+) > > diff --git a/Documentation/admin-guide/kdump/vmcoreinfo.rst > b/Documentation/admin-guide/kdump/vmcoreinfo.rst > index 6726f439958c..8e2e164cf3db 100644 > --- a/Documentation/admin-guide/kdump/vmcoreinfo.rst > +++ b/Documentation/admin-guide/kdump/vmcoreinfo.rst > @@ -595,3 +595,33 @@ X2TLB > - > > Indicates whether the crashed kernel enabled SH extended mode. > + > +RISCV64 > +=== > + > +VA_BITS > +--- > + > +The maximum number of bits for virtual addresses. Used to compute the > +virtual memory ranges. > + > +PAGE_OFFSET > +--- > + > +Indicates the virtual kernel start address of direct-mapped RAM region. Apologies for not seeing this sooner, but should there not be a "the" prior to "direct-mapped"? > + > +phys_ram_base > +- > + > +Indicates the start physical RAM address. > + > +MODULES_VADDR|MODULES_END|VMALLOC_START|VMALLOC_END|VMEMMAP_START|VMEMMAP_END|KASAN_SHADOW_START|KASAN_SHADOW_END|KERNEL_LINK_ADDR|ADDRESS_SPACE_END > + > + > +Used to get the correct ranges: > + > + * MODULES_VADDR ~ MODULES_END : Kernel module space. > + * VMALLOC_START ~ VMALLOC_END : vmalloc() / ioremap() space. > + * VMEMMAP_START ~ VMEMMAP_END : vmemmap region, used for struct page array. Since I'm in pedant mode, it does look a little odd that you're using region for vmemmap but space for the others but idc that much. Thanks, Conor. > + * KASAN_SHADOW_START ~ KASAN_SHADOW_END : kasan shadow space. > + * KERNEL_LINK_ADDR ~ ADDRESS_SPACE_END : Kernel link and BPF space. > -- > 2.17.1 > > ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH V2 1/2] RISC-V: Add arch_crash_save_vmcoreinfo support
On Fri, Oct 14, 2022 at 09:41:38PM +0800, Xianting Tian wrote:
> Add arch_crash_save_vmcoreinfo(), which exports VM layout(MODULES, VMALLOC,
> VMEMMAP and KERNEL_LINK_ADDR ranges), va bits and ram base for vmcore.
>
> Default pagetable levels and PAGE_OFFSET aren't same for different kernel
> version as below. For default pagetable levels, it sets sv57 on defaultly
> in latest kernel and do fallback to try to set sv48 on boot time if sv57
> is not supported in current hardware.
nit: This would read better as "it sets sv57 by default and falls back
to setting sv48 at boot time if sv57 is not supported by the hardware".
>
> For ram base, the default value is 0x8020 for qemu riscv64 env, 0x20
> for riscv64 SoC platform(eg, SoC platform of RISC-V XuanTie 910 CPU).
The second part of this sentence I'm not really sure that that is true,
I think you should reword it to something like "...for qemu riscv64 and,
for example, is 0x20 on the XuanTie 910 CPU." and avoid applying
that number blanketly for other SoCs.
>
> * Linux Kernel 5.18 ~
> * PGTABLE_LEVELS = 5
> * PAGE_OFFSET = 0xff60
> * Linux Kernel 5.17 ~
> * PGTABLE_LEVELS = 4
> * PAGE_OFFSET = 0xaf80
> * Linux Kernel 4.19 ~
> * PGTABLE_LEVELS = 3
> * PAGE_OFFSET = 0xffe0
>
> Since these configurations change from time to time and version to version,
> it is preferable to export them via vmcoreinfo than to change the crash's
> code frequently, it can simplify the development of crash tool.
>
> Signed-off-by: Xianting Tian
> ---
> arch/riscv/kernel/Makefile | 1 +
> arch/riscv/kernel/crash_core.c | 29 +
> 2 files changed, 30 insertions(+)
> create mode 100644 arch/riscv/kernel/crash_core.c
>
> diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
> index db6e4b1294ba..4cf303a779ab 100644
> --- a/arch/riscv/kernel/Makefile
> +++ b/arch/riscv/kernel/Makefile
> @@ -81,6 +81,7 @@ obj-$(CONFIG_KGDB) += kgdb.o
> obj-$(CONFIG_KEXEC_CORE) += kexec_relocate.o crash_save_regs.o
> machine_kexec.o
> obj-$(CONFIG_KEXEC_FILE) += elf_kexec.o machine_kexec_file.o
> obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
> +obj-$(CONFIG_CRASH_CORE) += crash_core.o
>
> obj-$(CONFIG_JUMP_LABEL) += jump_label.o
>
> diff --git a/arch/riscv/kernel/crash_core.c b/arch/riscv/kernel/crash_core.c
> new file mode 100644
> index ..8d7f5ff108da
> --- /dev/null
> +++ b/arch/riscv/kernel/crash_core.c
> @@ -0,0 +1,29 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include
> +#include
> +
> +void arch_crash_save_vmcoreinfo(void)
> +{
> + VMCOREINFO_NUMBER(VA_BITS);
> + VMCOREINFO_NUMBER(phys_ram_base);
> +
> + vmcoreinfo_append_str("NUMBER(PAGE_OFFSET)=0x%lx\n", PAGE_OFFSET);
> + vmcoreinfo_append_str("NUMBER(VMALLOC_START)=0x%lx\n", VMALLOC_START);
> + vmcoreinfo_append_str("NUMBER(VMALLOC_END)=0x%lx\n", VMALLOC_END);
> + vmcoreinfo_append_str("NUMBER(VMEMMAP_START)=0x%lx\n", VMEMMAP_START);
> + vmcoreinfo_append_str("NUMBER(VMEMMAP_END)=0x%lx\n", VMEMMAP_END);
> +#ifdef CONFIG_64BIT
> + vmcoreinfo_append_str("NUMBER(MODULES_VADDR)=0x%lx\n", MODULES_VADDR);
> + vmcoreinfo_append_str("NUMBER(MODULES_END)=0x%lx\n", MODULES_END);
> +#endif
> +
> + if (IS_ENABLED(CONFIG_64BIT)) {
You've already got a #ifdef CONFIG_64BIT above, is there a reason why
you'd use the IS_ENABLED here rather than merge this with the above
section? I'm a big fan of IS_ENABLED but I'm not sure what it adds here,
maybe you can show me the light :)
Thanks,
Conor.
> +#ifdef CONFIG_KASAN
> + vmcoreinfo_append_str("NUMBER(KASAN_SHADOW_START)=0x%lx\n",
> KASAN_SHADOW_START);
> + vmcoreinfo_append_str("NUMBER(KASAN_SHADOW_END)=0x%lx\n",
> KASAN_SHADOW_END);
> +#endif
> + vmcoreinfo_append_str("NUMBER(KERNEL_LINK_ADDR)=0x%lx\n",
> KERNEL_LINK_ADDR);
> + vmcoreinfo_append_str("NUMBER(ADDRESS_SPACE_END)=0x%lx\n",
> ADDRESS_SPACE_END);
> + }
> +}
> --
> 2.17.1
>
>
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH V2 2/2] Documentation: kdump: describe VMCOREINFO export for RISCV64
On Tue, Oct 18, 2022 at 10:41:50AM +0800, Xianting Tian wrote: > > 在 2022/10/18 上午3:40, Conor Dooley 写道: > > On Fri, Oct 14, 2022 at 09:41:39PM +0800, Xianting Tian wrote: > > > The following interrelated definitions and ranges are needed by the kdump > > > crash tool, they are exported by "arch/riscv/kernel/crash_core.c": > > > VA_BITS, > > > PAGE_OFFSET, > > > phys_ram_base, > > > MODULES_VADDR ~ MODULES_END, > > > VMALLOC_START ~ VMALLOC_END, > > > VMEMMAP_START ~ VMEMMAP_END, > > > KASAN_SHADOW_START ~ KASAN_SHADOW_END, > > > KERNEL_LINK_ADDR ~ ADDRESS_SPACE_END > > > > > > Document these RISCV64 exports above. > > > > > > Signed-off-by: Xianting Tian > > > --- > > > .../admin-guide/kdump/vmcoreinfo.rst | 30 +++ > > > 1 file changed, 30 insertions(+) > > > > > > diff --git a/Documentation/admin-guide/kdump/vmcoreinfo.rst > > > b/Documentation/admin-guide/kdump/vmcoreinfo.rst > > > index 6726f439958c..8e2e164cf3db 100644 > > > --- a/Documentation/admin-guide/kdump/vmcoreinfo.rst > > > +++ b/Documentation/admin-guide/kdump/vmcoreinfo.rst > > > @@ -595,3 +595,33 @@ X2TLB > > > - > > > Indicates whether the crashed kernel enabled SH extended mode. > > > + > > > +RISCV64 > > > +=== > > > + > > > +VA_BITS > > > +--- > > > + > > > +The maximum number of bits for virtual addresses. Used to compute the > > > +virtual memory ranges. > > > + > > > +PAGE_OFFSET > > > +--- > > > + > > > +Indicates the virtual kernel start address of direct-mapped RAM region. > > Apologies for not seeing this sooner, but should there not be a "the" > > prior to "direct-mapped"? > will fix in v3 > > > > > + > > > +phys_ram_base > > > +- > > > + > > > +Indicates the start physical RAM address. > > > + > > > +MODULES_VADDR|MODULES_END|VMALLOC_START|VMALLOC_END|VMEMMAP_START|VMEMMAP_END|KASAN_SHADOW_START|KASAN_SHADOW_END|KERNEL_LINK_ADDR|ADDRESS_SPACE_END > > > + > > > + > > > +Used to get the correct ranges: > > > + > > > + * MODULES_VADDR ~ MODULES_END : Kernel module space. > > > + * VMALLOC_START ~ VMALLOC_END : vmalloc() / ioremap() space. > > > + * VMEMMAP_START ~ VMEMMAP_END : vmemmap region, used for struct page > > > array. > > Since I'm in pedant mode, it does look a little odd that you're using > > region for vmemmap but space for the others but idc that much. > > Sorry, I didn't get your point :( > > it contains vmemmap area with reference wth arch/arm64/kernel/crash_core.c. The wording is different, vmemmap uses space and the others use region. Not a big deal. > > > > > Thanks, > > Conor. > > > > > + * KASAN_SHADOW_START ~ KASAN_SHADOW_END : kasan shadow space. > > > + * KERNEL_LINK_ADDR ~ ADDRESS_SPACE_END : Kernel link and BPF space. > > > -- > > > 2.17.1 > > > > > > > > ___ > linux-riscv mailing list > linux-ri...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH V4 1/2] RISC-V: Add arch_crash_save_vmcoreinfo support
On Wed, Oct 26, 2022 at 05:08:11PM +0800, Xianting Tian wrote:
> Hi Palmer, Conor
>
> Is this version OK for you?
The weird ifdef/IS_ENABLED thing was the only comment I had. It's a bit
odd & I notice Baoquan brought it up too. I didn't (and won't) give you
a reviewed by on these patches because I don't understand the area well
enough. The general nitpickery seems to be sorted though.
Thanks,
Conor.
>
> 在 2022/10/20 下午12:40, Xianting Tian 写道:
> >
> > 在 2022/10/20 上午11:05, Baoquan He 写道:
> > > On 10/20/22 at 10:17am, Xianting Tian wrote:
> > > > 在 2022/10/20 上午10:08, Baoquan He 写道:
> > > > > On 10/19/22 at 06:36pm, Xianting Tian wrote:
> > > > > > Add arch_crash_save_vmcoreinfo(), which exports VM
> > > > > > layout(MODULES, VMALLOC,
> > > > > > VMEMMAP ranges and KERNEL_LINK_ADDR), va bits and ram
> > > > > > base for vmcore.
> > > > > >
> > > > > > Default pagetable levels and PAGE_OFFSET aren't same for
> > > > > > different kernel
> > > > > > version as below. For pagetable levels, it sets sv57 by
> > > > > > default and falls
> > > > > > back to setting sv48 at boot time if sv57 is not
> > > > > > supported by the hardware.
> > > > > >
> > > > > > For ram base, the default value is 0x8020 for qemu
> > > > > > riscv64 env and,
> > > > > > for example, is 0x20 on the XuanTie 910 CPU.
> > > > > >
> > > > > > * Linux Kernel 5.18 ~
> > > > > > * PGTABLE_LEVELS = 5
> > > > > > * PAGE_OFFSET = 0xff60
> > > > > > * Linux Kernel 5.17 ~
> > > > > > * PGTABLE_LEVELS = 4
> > > > > > * PAGE_OFFSET = 0xaf80
> > > > > > * Linux Kernel 4.19 ~
> > > > > > * PGTABLE_LEVELS = 3
> > > > > > * PAGE_OFFSET = 0xffe0
> > > > > >
> > > > > > Since these configurations change from time to time and
> > > > > > version to version,
> > > > > > it is preferable to export them via vmcoreinfo than to
> > > > > > change the crash's
> > > > > > code frequently, it can simplify the development of crash tool.
> > > > > >
> > > > > > Signed-off-by: Xianting Tian
> > > > > > ---
> > > > > > arch/riscv/kernel/Makefile | 1 +
> > > > > > arch/riscv/kernel/crash_core.c | 23 +++
> > > > > > 2 files changed, 24 insertions(+)
> > > > > > create mode 100644 arch/riscv/kernel/crash_core.c
> > > > > >
> > > > > > diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
> > > > > > index db6e4b1294ba..4cf303a779ab 100644
> > > > > > --- a/arch/riscv/kernel/Makefile
> > > > > > +++ b/arch/riscv/kernel/Makefile
> > > > > > @@ -81,6 +81,7 @@ obj-$(CONFIG_KGDB) += kgdb.o
> > > > > > obj-$(CONFIG_KEXEC_CORE) += kexec_relocate.o
> > > > > > crash_save_regs.o machine_kexec.o
> > > > > > obj-$(CONFIG_KEXEC_FILE) += elf_kexec.o machine_kexec_file.o
> > > > > > obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
> > > > > > +obj-$(CONFIG_CRASH_CORE) += crash_core.o
> > > > > > obj-$(CONFIG_JUMP_LABEL) += jump_label.o
> > > > > > diff --git a/arch/riscv/kernel/crash_core.c
> > > > > > b/arch/riscv/kernel/crash_core.c
> > > > > > new file mode 100644
> > > > > > index ..3e889d0ed7bd
> > > > > > --- /dev/null
> > > > > > +++ b/arch/riscv/kernel/crash_core.c
> > > > > > @@ -0,0 +1,23 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > > > +
> > > > > > +#include
> > > > > > +#include
> > > > > > +
> > > > > > +void arch_crash_save_vmcoreinfo(void)
> > > > > > +{
> > > > > > + VMCOREINFO_NUMBER(VA_BITS);
> > > > > > + VMCOREINFO_NUMBER(phys_ram_base);
> > > > > > +
> > > > > > +
> > > > > > vmcoreinfo_append_str("NUMBER(PAGE_OFFSET)=0x%lx\n",
> > > > > > PAGE_OFFSET);
> > > > > > + vmcoreinfo_append_str("NUMBER(VMALLOC_START)=0x%lx\n",
> > > > > > VMALLOC_START);
> > > > > > +
> > > > > > vmcoreinfo_append_str("NUMBER(VMALLOC_END)=0x%lx\n",
> > > > > > VMALLOC_END);
> > > > > > + vmcoreinfo_append_str("NUMBER(VMEMMAP_START)=0x%lx\n",
> > > > > > VMEMMAP_START);
> > > > > > +
> > > > > > vmcoreinfo_append_str("NUMBER(VMEMMAP_END)=0x%lx\n",
> > > > > > VMEMMAP_END);
> > > > > > +#ifdef CONFIG_64BIT
> > > > > > + vmcoreinfo_append_str("NUMBER(MODULES_VADDR)=0x%lx\n",
> > > > > > MODULES_VADDR);
> > > > > > +
> > > > > > vmcoreinfo_append_str("NUMBER(MODULES_END)=0x%lx\n",
> > > > > > MODULES_END);
> > > > > > +#endif
> > > > > > +
> > > > > > + if (IS_ENABLED(CONFIG_64BIT))
> > > > > > +
> > > > > > vmcoreinfo_append_str("NUMBER(KERNEL_LINK_ADDR)=0x%lx\n",
> > > > > > KERNEL_LINK_ADDR);
> > > > > Wondering why you don't put KERNEL_LINK_ADDR exporting into the above
> > > > > ifdeffery scope, with that you can save one line of
> > > > > "IS_ENABLED(CONFIG_64BIT)".
> > > > I followed the rule in print_vm_layout() of
> > > > arch/riscv/mm/init.c, which used
> > > > IS_ENABLED when print the value of KERNEL_LINK_ADDR.
> > > >
> > > I see. There's PAGE_OFFSET in the middle. Thanks.
> > >
> > > print_ml("lowmem", (unsigned long)PAGE_
Re: [PATCH V4 1/2] RISC-V: Add arch_crash_save_vmcoreinfo support
On Wed, Oct 26, 2022 at 08:05:41PM +0800, Baoquan He wrote:
> Hi Xianting,
>
> On 10/26/22 at 05:44pm, Xianting Tian wrote:
> >
> > 在 2022/10/26 下午5:25, Conor Dooley 写道:
> > > On Wed, Oct 26, 2022 at 05:08:11PM +0800, Xianting Tian wrote:
> > > > Hi Palmer, Conor
> > > >
> > > > Is this version OK for you?
> > > The weird ifdef/IS_ENABLED thing was the only comment I had. It's a bit
> > > odd & I notice Baoquan brought it up too. I didn't (and won't) give you
> > > a reviewed by on these patches because I don't understand the area well
> > > enough. The general nitpickery seems to be sorted though.
> >
> > I checked the KERNEL_LINK_ADDR definition of riscv, it is valid for
> > CONFIG_64BIT and !CONFIG_64BIT.
>
> This series looks good to me. My only minor concern is if we can make
> the arch_crash_save_vmcoreinfo() as below. I don't understand why we
> have to have the CONFIG_64BIT ifdeffery and the IS_ENABLED(CONFIG_64BIT)
> between two adjacent code blocks. Not sure if we are saying the same
> thing.
I think we can just go and drop the IS_ENABLED(). From looking at it
last time, one bit is compileable (but not usable) for !64BIT and the
other isn't hence the IS_ENABLED(). I think it would make sense to drop
the IS_ENABLED() - I don't think we're too likely to hit some compile
testing edge cases that IS_ENABLED() would help with & only having one
makes the code look a lot less odd and a lot more intentional.
>
> +void arch_crash_save_vmcoreinfo(void)
> +{
> + VMCOREINFO_NUMBER(VA_BITS);
> + VMCOREINFO_NUMBER(phys_ram_base);
> +
> + vmcoreinfo_append_str("NUMBER(PAGE_OFFSET)=0x%lx\n", PAGE_OFFSET);
> + vmcoreinfo_append_str("NUMBER(VMALLOC_START)=0x%lx\n", VMALLOC_START);
> + vmcoreinfo_append_str("NUMBER(VMALLOC_END)=0x%lx\n", VMALLOC_END);
> + vmcoreinfo_append_str("NUMBER(VMEMMAP_START)=0x%lx\n", VMEMMAP_START);
> + vmcoreinfo_append_str("NUMBER(VMEMMAP_END)=0x%lx\n", VMEMMAP_END);
> +#ifdef CONFIG_64BIT
> + vmcoreinfo_append_str("NUMBER(MODULES_VADDR)=0x%lx\n", MODULES_VADDR);
> + vmcoreinfo_append_str("NUMBER(MODULES_END)=0x%lx\n", MODULES_END);
> + vmcoreinfo_append_str("NUMBER(KERNEL_LINK_ADDR)=0x%lx\n",
> KERNEL_LINK_ADDR);
> +#endif
> +}
>
> >
> > Maybe we can remove IS_ENABLED(CONFIG_64BIT)
> >
> > arch/riscv/include/asm/pgtable.h
> > #define ADDRESS_SPACE_END (UL(-1))
> > #ifdef CONFIG_64BIT
> > /* Leave 2GB for kernel and BPF at the end of the address space */
> > #define KERNEL_LINK_ADDR (ADDRESS_SPACE_END - SZ_2G + 1)
> > #else
> > #define KERNEL_LINK_ADDR PAGE_OFFSET
> > #endif
> >
> > arch/riscv/include/asm/page.h
> > #ifdef CONFIG_64BIT
> > #ifdef CONFIG_MMU
> > #define PAGE_OFFSET kernel_map.page_offset
> > #else
> > #define PAGE_OFFSET _AC(CONFIG_PAGE_OFFSET, UL)
> > #endif
> > /*
> > * By default, CONFIG_PAGE_OFFSET value corresponds to SV48 address space so
> > * define the PAGE_OFFSET value for SV39.
> > */
> > #define PAGE_OFFSET_L4 _AC(0xaf80, UL)
> > #define PAGE_OFFSET_L3 _AC(0xffd8, UL)
> > #else
> > #define PAGE_OFFSET _AC(CONFIG_PAGE_OFFSET, UL)
> > #endif /* CONFIG_64BIT */
> >
> > >
> > > Thanks,
> > > Conor.
> > >
> > > > 在 2022/10/20 下午12:40, Xianting Tian 写道:
> > > > > 在 2022/10/20 上午11:05, Baoquan He 写道:
> > > > > > On 10/20/22 at 10:17am, Xianting Tian wrote:
> > > > > > > 在 2022/10/20 上午10:08, Baoquan He 写道:
> > > > > > > > On 10/19/22 at 06:36pm, Xianting Tian wrote:
> > > > > > > > > Add arch_crash_save_vmcoreinfo(), which exports VM
> > > > > > > > > layout(MODULES, VMALLOC,
> > > > > > > > > VMEMMAP ranges and KERNEL_LINK_ADDR), va bits and ram
> > > > > > > > > base for vmcore.
> > > > > > > > >
> > > > > > > > > Default pagetable levels and PAGE_OFFSET aren't same for
> > > > > > > > > different kernel
> > > > > > > > > version as below. For pagetable levels, it sets sv57 by
> > > > > > > > > default and falls
> > > > > > > > > back to setting sv48 at boot time if sv57 is not
> > > > >
Re: [PATCH 1/2] RISC-V: kexec: Fix memory leak of fdt buffer
On Fri, Nov 04, 2022 at 05:56:57PM +0800, Li Huafei wrote:
> This is reported by kmemleak detector:
>
> unreferenced object 0xff6082864000 (size 9588):
> comm "kexec", pid 146, jiffies 4294900634 (age 64.788s)
> hex dump (first 32 bytes):
> d0 0d fe ed 00 00 12 ed 00 00 00 48 00 00 11 40 ...H...@
> 00 00 00 28 00 00 00 11 00 00 00 02 00 00 00 00 ...(
> backtrace:
> [<f95b17c4>] kmemleak_alloc+0x34/0x3e
> [<b9ec8e3e>] kmalloc_order+0x9c/0xc4
> [<a95cf02e>] kmalloc_order_trace+0x34/0xb6
> [<f01e68b4>] __kmalloc+0x5c2/0x62a
> [<2bd497b2>] kvmalloc_node+0x66/0xd6
> [<906542fa>] of_kexec_alloc_and_setup_fdt+0xa6/0x6ea
> [<e1166bde>] elf_kexec_load+0x206/0x4ec
> [<36548e09>] kexec_image_load_default+0x40/0x4c
> [<79fbe1b4>] sys_kexec_file_load+0x1c4/0x322
> [<40c62c03>] ret_from_syscall+0x0/0x2
>
> In elf_kexec_load(), a buffer is allocated via kvmalloc() to store fdt.
> While it's not freed back to system when kexec kernel is reloaded or
> unloaded. Then memory leak is caused. Fix it by introducing riscv
> specific function arch_kimage_file_post_load_cleanup(), and freeing the
> buffer there.
>
> Fixes: 6261586e0c91 ("RISC-V: Add kexec_file support")
> Signed-off-by: Li Huafei
Both of these bits of cleanup seem to echo what's the case on arm64.
Seems reasonable to me..
Reviewed-by: Conor Dooley
> ---
> arch/riscv/include/asm/kexec.h | 5 +
> arch/riscv/kernel/elf_kexec.c | 10 ++
> 2 files changed, 15 insertions(+)
>
> diff --git a/arch/riscv/include/asm/kexec.h b/arch/riscv/include/asm/kexec.h
> index eee260e8ab30..2b56769cb530 100644
> --- a/arch/riscv/include/asm/kexec.h
> +++ b/arch/riscv/include/asm/kexec.h
> @@ -39,6 +39,7 @@ crash_setup_regs(struct pt_regs *newregs,
> #define ARCH_HAS_KIMAGE_ARCH
>
> struct kimage_arch {
> + void *fdt; /* For CONFIG_KEXEC_FILE */
> unsigned long fdt_addr;
> };
>
> @@ -62,6 +63,10 @@ int arch_kexec_apply_relocations_add(struct purgatory_info
> *pi,
>const Elf_Shdr *relsec,
>const Elf_Shdr *symtab);
> #define arch_kexec_apply_relocations_add arch_kexec_apply_relocations_add
> +
> +struct kimage;
> +int arch_kimage_file_post_load_cleanup(struct kimage *image);
> +#define arch_kimage_file_post_load_cleanup arch_kimage_file_post_load_cleanup
> #endif
>
> #endif
> diff --git a/arch/riscv/kernel/elf_kexec.c b/arch/riscv/kernel/elf_kexec.c
> index 0cb94992c15b..ff30fcb43f47 100644
> --- a/arch/riscv/kernel/elf_kexec.c
> +++ b/arch/riscv/kernel/elf_kexec.c
> @@ -21,6 +21,14 @@
> #include
> #include
>
> +int arch_kimage_file_post_load_cleanup(struct kimage *image)
> +{
> + kvfree(image->arch.fdt);
> + image->arch.fdt = NULL;
> +
> + return kexec_image_post_load_cleanup_default(image);
> +}
> +
> static int riscv_kexec_elf_load(struct kimage *image, struct elfhdr *ehdr,
> struct kexec_elf_info *elf_info, unsigned long
> old_pbase,
> unsigned long new_pbase)
> @@ -298,6 +306,8 @@ static void *elf_kexec_load(struct kimage *image, char
> *kernel_buf,
> pr_err("Error add DTB kbuf ret=%d\n", ret);
> goto out_free_fdt;
> }
> + /* Cache the fdt buffer address for memory cleanup */
> + image->arch.fdt = fdt;
> pr_notice("Loaded device tree at 0x%lx\n", kbuf.mem);
> goto out;
>
> --
> 2.17.1
>
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 2/2] RISC-V: kexec: Fix memory leak of elf header buffer
Again, echos arm64's cleanup. Reviewed-by: Conor Dooley ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH V5 0/2] Support VMCOREINFO export for RISCV64
Hey Palmer, Xianting, On Fri, Dec 02, 2022 at 04:49:39PM -0800, Palmer Dabbelt wrote: > On Wed, 26 Oct 2022 22:42:06 +0800, Xianting Tian wrote: > > As disscussed in below patch set, the patch of 'describe VMCOREINFO export > > in Documentation' > > need to update according to Bagas's comments. > > https://lore.kernel.org/linux-riscv/22aaf52e-8cc8-4d11-99cb-88de4d113...@kernel.org/ > > > > As others patches in above patch set already applied, so this patch set > > only contains below two > > patches. > > > > [...] > > Applied, thanks! > > [1/2] RISC-V: Add arch_crash_save_vmcoreinfo support > https://git.kernel.org/palmer/c/649d6b1019a2 So this patch seems to be causing issues for the nommu build: https://lore.kernel.org/oe-kbuild-all/202212062250.tr0othcz-...@intel.com/ I had a bit of a poke at trying to see if there were some headers we could pull in before actually checking the .config only to see: # CONFIG_MMU is not set Do we have to wrap the whole thing in a `#ifdef CONFIG_MMU` to fix compilation here? Thanks, Conor. signature.asc Description: PGP signature ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH V5 0/2] Support VMCOREINFO export for RISCV64
On Wed, Dec 07, 2022 at 11:34:57AM +0800, Guo Ren wrote: > On Wed, Dec 7, 2022 at 7:05 AM Conor Dooley wrote: > > > > Hey Palmer, Xianting, > > > > On Fri, Dec 02, 2022 at 04:49:39PM -0800, Palmer Dabbelt wrote: > > > On Wed, 26 Oct 2022 22:42:06 +0800, Xianting Tian wrote: > > > > As disscussed in below patch set, the patch of 'describe VMCOREINFO > > > > export in Documentation' > > > > need to update according to Bagas's comments. > > > > https://lore.kernel.org/linux-riscv/22aaf52e-8cc8-4d11-99cb-88de4d113...@kernel.org/ > > > > > > > > As others patches in above patch set already applied, so this patch set > > > > only contains below two > > > > patches. > > > > > > > > [...] > > > > > > Applied, thanks! > > > > > > [1/2] RISC-V: Add arch_crash_save_vmcoreinfo support > > > https://git.kernel.org/palmer/c/649d6b1019a2 > > > > So this patch seems to be causing issues for the nommu build: > > https://lore.kernel.org/oe-kbuild-all/202212062250.tr0othcz-...@intel.com/ > > > > I had a bit of a poke at trying to see if there were some headers we > > could pull in before actually checking the .config only to see: > > # CONFIG_MMU is not set > > > > Do we have to wrap the whole thing in a `#ifdef CONFIG_MMU` to fix > > compilation here? > The problem does not belong to the patch. > > Could I send a fixup patch? like this? That seems like a sane dependancy, but did you build that config? This doesn't fix the problem for me :/ > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > index ef8d66de5f38..d8c07999426c 100644 > --- a/arch/riscv/Kconfig > +++ b/arch/riscv/Kconfig > @@ -512,6 +512,7 @@ config ARCH_HAS_KEXEC_PURGATORY > > config CRASH_DUMP > bool "Build kdump crash kernel" > + depends on KEXEC > help > Generate crash dump after being started by kexec. This should > be normally only set in special crash dump kernels which are > > > > > Thanks, > > Conor. > > > > > -- > Best Regards > Guo Ren signature.asc Description: PGP signature ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v6 4/4] risc/purgatory: Add linker script
On Mon, May 01, 2023 at 07:18:12PM +0200, Ricardo Ribalda wrote:
> On Mon, 1 May 2023 at 18:19, Nick Desaulniers wrote:
> >
> > On Mon, May 1, 2023 at 5:39 AM Ricardo Ribalda wrote:
> > >
> > > If PGO is enabled, the purgatory ends up with multiple .text sections.
> > > This is not supported by kexec and crashes the system.
> > >
> > > Cc: sta...@vger.kernel.org
> > > Fixes: 930457057abe ("kernel/kexec_file.c: split up
> > > __kexec_load_puragory")
> > > Signed-off-by: Ricardo Ribalda
> >
> > Hi Ricardo,
> > Thanks for the series. Does this patch 4/4 need a new online commit
> > description? It's not adding a linker script (maybe an earlier version
> > was).
> Thanks for catching this. It should have said
>
> risc/purgatory: Remove profile optimization flags
^^
Perhaps with the omitted v added too?
Also while playing the $subject nitpicking game, is it not called
"profile**-guided** optimisation" (and ditto in the comments)?
Cheers,
Conor.
> Will fix it on my local branch in case there is a next version of the
> series. Otherwise, please the maintainer fix the subject.
> > > ---
> > > arch/riscv/purgatory/Makefile | 5 +
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/arch/riscv/purgatory/Makefile b/arch/riscv/purgatory/Makefile
> > > index 5730797a6b40..cf3a44121a90 100644
> > > --- a/arch/riscv/purgatory/Makefile
> > > +++ b/arch/riscv/purgatory/Makefile
> > > @@ -35,6 +35,11 @@ CFLAGS_sha256.o := -D__DISABLE_EXPORTS
> > > CFLAGS_string.o := -D__DISABLE_EXPORTS
> > > CFLAGS_ctype.o := -D__DISABLE_EXPORTS
> > >
> > > +# When profile optimization is enabled, llvm emits two different
> > > overlapping
> > > +# text sections, which is not supported by kexec. Remove profile
> > > optimization
> > > +# flags.
> > > +KBUILD_CFLAGS := $(filter-out -fprofile-sample-use=%
> > > -fprofile-use=%,$(KBUILD_CFLAGS))
> > > +
> > > # When linking purgatory.ro with -r unresolved symbols are not checked,
> > > # also link a purgatory.chk binary without -r to check for unresolved
> > > symbols.
> > > PURGATORY_LDFLAGS := -e purgatory_start -z nodefaultlib
> > >
> > > --
> > > 2.40.1.495.gc816e09b53d-goog
> > >
> >
> >
> > --
> > Thanks,
> > ~Nick Desaulniers
>
>
>
> --
> Ricardo Ribalda
>
> ___
> linux-riscv mailing list
> linux-ri...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
signature.asc
Description: PGP signature
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v6 4/4] risc/purgatory: Add linker script
Hey Ricardo,
On Mon, May 01, 2023 at 02:38:22PM +0200, Ricardo Ribalda wrote:
> If PGO is enabled, the purgatory ends up with multiple .text sections.
> This is not supported by kexec and crashes the system.
>
> Cc: sta...@vger.kernel.org
> Fixes: 930457057abe ("kernel/kexec_file.c: split up __kexec_load_puragory")
> Signed-off-by: Ricardo Ribalda
> ---
> arch/riscv/purgatory/Makefile | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/arch/riscv/purgatory/Makefile b/arch/riscv/purgatory/Makefile
> index 5730797a6b40..cf3a44121a90 100644
> --- a/arch/riscv/purgatory/Makefile
> +++ b/arch/riscv/purgatory/Makefile
> @@ -35,6 +35,11 @@ CFLAGS_sha256.o := -D__DISABLE_EXPORTS
> CFLAGS_string.o := -D__DISABLE_EXPORTS
> CFLAGS_ctype.o := -D__DISABLE_EXPORTS
>
> +# When profile optimization is enabled, llvm emits two different overlapping
> +# text sections, which is not supported by kexec. Remove profile optimization
> +# flags.
> +KBUILD_CFLAGS := $(filter-out -fprofile-sample-use=%
> -fprofile-use=%,$(KBUILD_CFLAGS))
With the caveat of not being au fait with the workings of either PGO or
of purgatory, how come you modify KBUILD_CFLAGS here rather than the
purgatory specific PURGATORY_CFLAGS that are used later in the file?
Cheers,
Conor.
> +
> # When linking purgatory.ro with -r unresolved symbols are not checked,
> # also link a purgatory.chk binary without -r to check for unresolved
> symbols.
> PURGATORY_LDFLAGS := -e purgatory_start -z nodefaultlib
>
> --
> 2.40.1.495.gc816e09b53d-goog
>
>
> ___
> linux-riscv mailing list
> linux-ri...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
signature.asc
Description: PGP signature
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v6 4/4] risc/purgatory: Add linker script
On Mon, May 01, 2023 at 09:54:43PM +0200, Ricardo Ribalda wrote:
> On Mon, 1 May 2023 at 19:41, Conor Dooley wrote:
> > On Mon, May 01, 2023 at 02:38:22PM +0200, Ricardo Ribalda wrote:
> > > If PGO is enabled, the purgatory ends up with multiple .text sections.
> > > This is not supported by kexec and crashes the system.
> > >
> > > Cc: sta...@vger.kernel.org
> > > Fixes: 930457057abe ("kernel/kexec_file.c: split up
> > > __kexec_load_puragory")
> > > Signed-off-by: Ricardo Ribalda
> > > ---
> > > arch/riscv/purgatory/Makefile | 5 +
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/arch/riscv/purgatory/Makefile b/arch/riscv/purgatory/Makefile
> > > index 5730797a6b40..cf3a44121a90 100644
> > > --- a/arch/riscv/purgatory/Makefile
> > > +++ b/arch/riscv/purgatory/Makefile
> > > @@ -35,6 +35,11 @@ CFLAGS_sha256.o := -D__DISABLE_EXPORTS
> > > CFLAGS_string.o := -D__DISABLE_EXPORTS
> > > CFLAGS_ctype.o := -D__DISABLE_EXPORTS
> > >
> > > +# When profile optimization is enabled, llvm emits two different
> > > overlapping
> > > +# text sections, which is not supported by kexec. Remove profile
> > > optimization
> > > +# flags.
> > > +KBUILD_CFLAGS := $(filter-out -fprofile-sample-use=%
> > > -fprofile-use=%,$(KBUILD_CFLAGS))
> >
> > With the caveat of not being au fait with the workings of either PGO or
> > of purgatory, how come you modify KBUILD_CFLAGS here rather than the
> > purgatory specific PURGATORY_CFLAGS that are used later in the file?
>
> Definitely, not a Makefile expert here, but when I tried this:
>
> @@ -35,6 +40,7 @@ PURGATORY_CFLAGS_REMOVE := -mcmodel=kernel
> PURGATORY_CFLAGS := -mcmodel=large -ffreestanding
> -fno-zero-initialized-in-bss -g0
> PURGATORY_CFLAGS += $(DISABLE_STACKLEAK_PLUGIN) -DDISABLE_BRANCH_PROFILING
> PURGATORY_CFLAGS += -fno-stack-protector
> +PURGATORY_CFLAGS := $(filter-out -fprofile-sample-use=%
> -fprofile-use=%,$(KBUILD_CFLAGS))
>
> It did not work.
Unfortunately I am oh-so-far from an expert on this kind of thing, but I
had thought that PURGATORY_CFLAGS_REMOVE was intended for this sort of
purpose.
> Fixes: bde971a83bbf ("KVM: arm64: nvhe: Fix build with profile optimization")
>
> does this approach, so this is what I tried and worked.
That doesn't have a specific CFLAGS though afaict.
Perhaps Nick etc have a more informed opinion here than I do, sorry.
Thanks,
Conor.
> > > +
> > > # When linking purgatory.ro with -r unresolved symbols are not checked,
> > > # also link a purgatory.chk binary without -r to check for unresolved
> > > symbols.
> > > PURGATORY_LDFLAGS := -e purgatory_start -z nodefaultlib
> > >
> > > --
> > > 2.40.1.495.gc816e09b53d-goog
> > >
> > >
> > > ___
> > > linux-riscv mailing list
> > > linux-ri...@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv
>
>
>
> --
> Ricardo Ribalda
signature.asc
Description: PGP signature
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v2 3/3] riscv/kexec: load initrd high in available memory
On Wed, Jul 26, 2023 at 11:54:01AM +0200, Petr Tesarik wrote:
> From: Torsten Duwe
>
> When initrd is loaded low, the secondary kernel fails like this:
>
> INITRD: 0xdc581000+0x00eef000 overlaps in-use memory region
>
> This initrd load address corresponds to the _end symbol, but the
> reservation is aligned on PMD_SIZE, as explained by a comment in
> setup_bootmem().
>
> It is technically possible to align the initrd load address accordingly,
> leaving a hole between the end of kernel and the initrd, but it is much
> simpler to allocate the initrd top-down.
>
> Fixes: 838b3e28488f ("RISC-V: Load purgatory in kexec_file")
> Signed-off-by: Torsten Duwe
> Signed-off-by: Petr Tesarik
> Cc: sta...@vger.kernel.org
Trying to align it might be worthwhile, but the simple fix makes sense
for now & w.r.t backporting.
Reviewed-by: Conor Dooley
Thanks,
Conor.
signature.asc
Description: PGP signature
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v2 2/3] riscv/purgatory: do not link with string.o and its dependencies
On Wed, Jul 26, 2023 at 11:54:00AM +0200, Petr Tesarik wrote:
> From: Petr Tesarik
>
> Linking with this object file makes kexec_file_load(2) fail because of
> multiple unknown relocation types:
>
> - R_RISCV_ADD16, R_RISCV_SUB16: used by alternatives in strcmp()
> - R_RISCV_GOT_HI20: used to resolve _ctype in strcasecmp()
>
> All this hassle is needed for one single call to memcmp() from
> verify_sha256_digest() to compare 32 bytes of SHA256 checksum.
>
> Simply replace this memcmp() call with an explicit loop over those 32 bytes
> and remove the need to link with string.o and all the other object files
> that provide undefined symbols from that object file.
>
> Fixes: 838b3e28488f ("RISC-V: Load purgatory in kexec_file")
> Signed-off-by: Petr Tesarik
This version keeps the automation happy,
Acked-by: Conor Dooley
Thanks,
Conor.
signature.asc
Description: PGP signature
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [Crash-utility] RISCV64: Use va_kernel_pa_offset in VTOP()
Hey,
On Mon, Jul 24, 2023 at 12:06:49PM +0800, Song Shuai wrote:
> Since RISC-V Linux v6.4, the commit 3335068f8721 ("riscv: Use
> PUD/P4D/PGD pages for the linear mapping") changes the
> phys_ram_base from the kernel_map.phys_addr to the start of DRAM.
>
> The Crash's VTOP() still uses phys_ram_base and kernel_map.virt_addr
> to translate kernel virtual address, that made Crash boot failed with
> Linux v6.4 and later version.
>
> Let Linux export kernel_map.va_kernel_pa_offset in v6.5 and Crash can
> use "va_kernel_pa_offset" to translate the kernel virtual address in
> VTOP() correctly.
>
> Signed-off-by: Song Shuai
> ---
> You can check/test the Linux changes from this link:
> https://github.com/sugarfillet/linux/commits/6.5-rc3-crash
>
> And I'll send the Linux changes to riscv/for-next If you're ok with this
> patch.
If you want this to go into 6.5, you'll need to send it for riscv/fixes
instead. It sounds like a fix for this would need to go into 6.4 too,
no?
signature.asc
Description: PGP signature
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH -next v8 0/2] support allocating crashkernel above 4G explicitly on riscv
Hey, Your $subject says -next, but the patch failed to apply to riscv/for-next. What was the base for this patchset? Thanks, Conor. signature.asc Description: PGP signature ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH -next v8 0/2] support allocating crashkernel above 4G explicitly on riscv
On Wed, Jul 26, 2023 at 10:20:00AM +0800, chenjiahao (C) wrote: > > On 2023/7/26 5:48, Conor Dooley wrote: > > Hey, > > > > Your $subject says -next, but the patch failed to apply to > > riscv/for-next. What was the base for this patchset? > > > > Thanks, > > Conor. > > Hi, > > My patchset was tested on current linux-next HEAD > (commit ID: 1e25dd777248, tag: next-20230725) and > it seems all ok. > Could you try applying with the base above, or > is there any problem with that base? There's some difference between linux-next and riscv/for-next that prevents the patchwork automation from applying the patches. signature.asc Description: PGP signature ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v2 1/3] riscv/kexec: handle R_RISCV_CALL_PLT relocation type
On Wed, Jul 26, 2023 at 11:53:59AM +0200, Petr Tesarik wrote:
> From: Torsten Duwe
>
> R_RISCV_CALL has been deprecated and replaced by R_RISCV_CALL_PLT. See Enum
> 18-19 in Table 3. Relocation types here:
>
> https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-elf.adoc
>
> It was deprecated in ("Deprecated R_RISCV_CALL, prefer R_RISCV_CALL_PLT"):
>
> https://github.com/riscv-non-isa/riscv-elf-psabi-doc/commit/a0dced85018d7a0ec17023c9389cbd70b1dbc1b0
Ideally, these would be Link: tags, but that's certainly not worth a
respin. This looks fine to me,
Reviewed-by: Conor Dooley
Thanks,
Conor.
signature.asc
Description: PGP signature
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v2 2/3] riscv/purgatory: do not link with string.o and its dependencies
On Thu, Aug 03, 2023 at 08:13:48AM -0700, Palmer Dabbelt wrote:
> On Wed, 26 Jul 2023 09:33:49 PDT (-0700), Conor Dooley wrote:
> > On Wed, Jul 26, 2023 at 11:54:00AM +0200, Petr Tesarik wrote:
> > > From: Petr Tesarik
> > >
> > > Linking with this object file makes kexec_file_load(2) fail because of
> > > multiple unknown relocation types:
> > >
> > > - R_RISCV_ADD16, R_RISCV_SUB16: used by alternatives in strcmp()
> > > - R_RISCV_GOT_HI20: used to resolve _ctype in strcasecmp()
> > >
> > > All this hassle is needed for one single call to memcmp() from
> > > verify_sha256_digest() to compare 32 bytes of SHA256 checksum.
> > >
> > > Simply replace this memcmp() call with an explicit loop over those 32
> > > bytes
> > > and remove the need to link with string.o and all the other object files
> > > that provide undefined symbols from that object file.
> > >
> > > Fixes: 838b3e28488f ("RISC-V: Load purgatory in kexec_file")
> > > Signed-off-by: Petr Tesarik
> >
> > This version keeps the automation happy,
> > Acked-by: Conor Dooley
>
> Oddly enough, this breaks my builds. I tried fixing the first one with
> something like
You say "tried", does that mean it also didn't work?
What's the config?
Cheers,
Conor.
>
>From 41c5a952f77e53bf4201296abff0132725aa19e6 Mon Sep 17 00:00:00 2001
>From: Palmer Dabbelt
>Date: Wed, 2 Aug 2023 20:22:33 -0700
>Subject: [PATCH] RISC-V: Include io from timex
>Without this I get some implicit declarations.
> CC arch/riscv/kernel/asm-offsets.s
>In file included from linux/include/linux/timex.h:67,
> from linux/include/linux/time32.h:13,
> from linux/include/linux/time.h:60,
> from linux/include/linux/ktime.h:24,
> from linux/include/linux/timer.h:6,
> from linux/include/linux/workqueue.h:9,
> from linux/include/linux/mm_types.h:19,
> from linux/include/linux/mmzone.h:22,
> from linux/include/linux/gfp.h:7,
> from linux/include/linux/mm.h:7,
> from linux/arch/riscv/kernel/asm-offsets.c:10:
>linux/arch/riscv/include/asm/timex.h: In function 'get_cycles':
>linux/arch/riscv/include/asm/timex.h:25:16: error: implicit declaration of
> function 'readl_relaxed' [-Werror=implicit-function-declaration]
> 25 | return readl_relaxed(((u32 *)clint_time_val));
> |
>Signed-off-by: Palmer Dabbelt
>---
> arch/riscv/include/asm/timex.h | 1 +
> 1 file changed, 1 insertion(+)
>diff --git a/arch/riscv/include/asm/timex.h
> b/arch/riscv/include/asm/timex.h
>index a06697846e69..1a4d181193e0 100644
>--- a/arch/riscv/include/asm/timex.h
>+++ b/arch/riscv/include/asm/timex.h
>@@ -7,6 +7,7 @@
> #define _ASM_RISCV_TIMEX_H
> #include
>+#include
> typedef unsigned long cycles_t;
>
>--2.41.0
>
> The other two look fine and are somewhat independent, so I've picked those up
> for fixes.
>
> Thanks!
>
> >
> > Thanks,
> > Conor.
signature.asc
Description: PGP signature
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 1/2] RISC-V: Use linux,usable-memory-range for crash kernel
On Fri, Aug 11, 2023 at 06:41:16PM +0800, Song Shuai wrote:
> Now we use "memeory::linux,usable-memory" to indicate the available
> memory for the crash kernel.
>
> While booting with UEFI, the crash kernel would use efi.memmap to
> re-populate memblock and then first kernel's memory would be corrputed.
> Consequently, the /proc/vmcore file failed to create in my local test.
>
> And according to "chosen" dtschema [1], the available memory for the
> crash kernel should be held via "chosen::linux,usable-memory-range"
> property which will re-cap memblock even after UEFI's re-population.
>
> [1]:
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/chosen.yaml
nit: Please use Link: tags for links rather than free-form text.
This doesn't apply to fixes, for-next or master. What is your base?
>
> Signed-off-by: Song Shuai
> ---
> kexec/arch/riscv/kexec-riscv.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/kexec/arch/riscv/kexec-riscv.c b/kexec/arch/riscv/kexec-riscv.c
> index fe5dd2d..5aea035 100644
> --- a/kexec/arch/riscv/kexec-riscv.c
> +++ b/kexec/arch/riscv/kexec-riscv.c
> @@ -79,20 +79,20 @@ int load_extra_segments(struct kexec_info *info, uint64_t
> kernel_base,
> }
>
> ret = dtb_add_range_property(&fdt->buf, &fdt->size, start, end,
> - "memory", "linux,usable-memory");
> + "chosen",
> "linux,usable-memory-range");
> if (ret) {
> - fprintf(stderr, "Couldn't add usable-memory to fdt\n");
> + fprintf(stderr, "Couldn't add usable-memory-range to
> fdt\n");
> return ret;
> }
>
> max_usable = end;
> } else {
> /*
> - * Make sure we remove elfcorehdr and usable-memory
> + * Make sure we remove elfcorehdr and usable-memory-range
>* when switching from crash kernel to a normal one.
>*/
> dtb_delete_property(fdt->buf, "chosen", "linux,elfcorehdr");
> - dtb_delete_property(fdt->buf, "memory", "linux,usable-memory");
> + dtb_delete_property(fdt->buf, "chosen",
> "linux,usable-memory-range");
> }
>
> /* Do we need to include an initrd image ? */
> --
> 2.20.1
>
>
> ___
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
signature.asc
Description: PGP signature
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 1/2] RISC-V: Use linux,usable-memory-range for crash kernel
On Tue, Aug 15, 2023 at 10:49:12AM +0800, Song Shuai wrote: > > > 在 2023/8/14 19:10, Conor Dooley 写道: > > On Fri, Aug 11, 2023 at 06:41:16PM +0800, Song Shuai wrote: > > > Now we use "memeory::linux,usable-memory" to indicate the available > > > memory for the crash kernel. > > > > > > While booting with UEFI, the crash kernel would use efi.memmap to > > > re-populate memblock and then first kernel's memory would be corrputed. > > > Consequently, the /proc/vmcore file failed to create in my local test. > > > > > > And according to "chosen" dtschema [1], the available memory for the > > > crash kernel should be held via "chosen::linux,usable-memory-range" > > > property which will re-cap memblock even after UEFI's re-population. > > > > > > [1]: > > > https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/chosen.yaml > > > > nit: Please use Link: tags for links rather than free-form text. > > > > This doesn't apply to fixes, for-next or master. What is your base? > > > Actually, I sent these patches to kexec-tools/build-test-riscv-v2, > perhaps I shouldn't cc the linux-riscv mailing list. Oh right, sorry about that! signature.asc Description: PGP signature ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v3 5/7] kexec_file, ricv: print out debugging message if required
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
> ---
> 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
> #include
>
> -/*
> - * 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("\tsegment[%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
>
signature.asc
Description: PGP signature
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v3 5/7] kexec_file, ricv: print out debugging message if required
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
> > > ---
> > > 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.
signature.asc
Description: PGP signature
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v3 5/7] kexec_file, ricv: print out debugging message if required
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
> > > > > ---
> > > > > 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;
> &
Re: [PATCH v4 5/7] kexec_file, ricv: print out debugging message if required
On Wed, Dec 13, 2023 at 01:57:45PM +0800, Baoquan He wrote: > 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 loaded location of purgatory and device tree are only printed > out for debugging, it doesn't make sense to always print them out. > > And also remove kexec_image_info() because the content has been printed > out in generic code. > > Signed-off-by: Baoquan He I'm sorry - I meant to look at this several days ago but I forgot. Apart from the typo that crept back into $subject, this version explains the rationale behind what you're changing a lot better, thanks. Reviewed-by: Conor Dooley Cheers, Conor. signature.asc Description: PGP signature ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
