From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id DCFA5C38A2D for ; Wed, 26 Oct 2022 09:45:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233158AbiJZJpJ (ORCPT ); Wed, 26 Oct 2022 05:45:09 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37098 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233427AbiJZJpH (ORCPT ); Wed, 26 Oct 2022 05:45:07 -0400 Received: from out199-12.us.a.mail.aliyun.com (out199-12.us.a.mail.aliyun.com [47.90.199.12]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id F00E210553; Wed, 26 Oct 2022 02:45:01 -0700 (PDT) X-Alimail-AntiSpam: AC=PASS;BC=-1|-1;BR=01201311R901e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=ay29a033018046050;MF=xianting.tian@linux.alibaba.com;NM=1;PH=DS;RN=24;SR=0;TI=SMTPD_---0VT6jFsX_1666777492; Received: from 30.221.97.58(mailfrom:xianting.tian@linux.alibaba.com fp:SMTPD_---0VT6jFsX_1666777492) by smtp.aliyun-inc.com; Wed, 26 Oct 2022 17:44:55 +0800 Message-ID: <3c8beab1-3ca7-c3d7-6f31-c28a0ae008a3@linux.alibaba.com> Date: Wed, 26 Oct 2022 17:44:52 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.12.0 Subject: Re: [PATCH V4 1/2] RISC-V: Add arch_crash_save_vmcoreinfo support To: Conor Dooley Cc: Baoquan He , Palmer Dabbelt , paul.walmsley@sifive.com, aou@eecs.berkeley.edu, anup@brainfault.org, heiko@sntech.de, guoren@kernel.org, mick@ics.forth.gr, alexandre.ghiti@canonical.com, vgoyal@redhat.com, dyoung@redhat.com, corbet@lwn.net, bagasdotme@gmail.com, k-hagio-ab@nec.com, lijiang@redhat.com, kexec@lists.infradead.org, linux-doc@vger.kernel.org, linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, crash-utility@redhat.com, heinrich.schuchardt@canonical.com, hschauhan@nulltrace.org, yixun.lan@gmail.com References: <20221019103623.7008-1-xianting.tian@linux.alibaba.com> <20221019103623.7008-2-xianting.tian@linux.alibaba.com> <30621b3b-47ba-d612-cfb0-583d779691a3@linux.alibaba.com> <6af05838-fa58-8197-f3ce-ca95457077a7@linux.alibaba.com> <5df30e57-88ae-0a3b-2c1a-b962363d8670@linux.alibaba.com> From: Xianting Tian In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-doc@vger.kernel.org 在 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. 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(0xffffaf8000000000, UL) #define PAGE_OFFSET_L3          _AC(0xffffffd800000000, 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 >>>>>>> supported by the hardware. >>>>>>> >>>>>>> For ram base, the default value is 0x80200000 for qemu >>>>>>> riscv64 env and, >>>>>>> for example, is 0x200000 on the XuanTie 910 CPU. >>>>>>> >>>>>>>    * Linux Kernel 5.18 ~ >>>>>>>    *      PGTABLE_LEVELS = 5 >>>>>>>    *      PAGE_OFFSET = 0xff60000000000000 >>>>>>>    * Linux Kernel 5.17 ~ >>>>>>>    *      PGTABLE_LEVELS = 4 >>>>>>>    *      PAGE_OFFSET = 0xffffaf8000000000 >>>>>>>    * Linux Kernel 4.19 ~ >>>>>>>    *      PGTABLE_LEVELS = 3 >>>>>>>    *      PAGE_OFFSET = 0xffffffe000000000 >>>>>>> >>>>>>> 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 000000000000..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_OFFSET, >>>>                  (unsigned long)high_memory) >>>> >>>> So now, do you think if it's necessary to have another >>>> IS_ENABLED(CONFIG_64BIT) in the current arch_crash_save_vmcoreinfo()? >>> For which MACRO?  I think current code for PAGE_OFFSET is OK. >>>