Message-ID: <6d71e063-8e18-dd08-ac40-36b41ccfcfdb@xen0n.name>
Date:   Wed, 22 Feb 2023 17:05:31 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.8.0
Subject: Re: [PATCH 1/2] LoongArch: kdump: Add the same binary implementation
To:     Youling Tang <tangyouling@loongson.cn>,
        Huacai Chen <chenhuacai@kernel.org>
Cc:     loongarch@lists.linux.dev, linux-kernel@vger.kernel.org
References: <1677048791-26951-1-git-send-email-tangyouling@loongson.cn>
 <1677048791-26951-2-git-send-email-tangyouling@loongson.cn>
Content-Language: en-US
From:   WANG Xuerui <kernel@xen0n.name>
In-Reply-To: <1677048791-26951-2-git-send-email-tangyouling@loongson.cn>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2023/2/22 14:53, Youling Tang wrote:
> This feature depends on the relocation function, so the relocation configuration
> CONFIG_RELOCATABLE will be enabled.

In general try to describe things briefly: "This depends on the kernel 
being relocatable" would be enough in this case.

> 
> Add the same set of binary implementations for kdump, and then no longer need to
> compile two kernels (the production kernel and the capture kernel share the same
> binary).

Sorry but what do you mean by "same set of binary implementation", 
where's the "first set of binary implementation"?

Judging from the patch content, I guess it's kinda wrong translation, 
and what you actually mean is something like "enable using the same 
image for crashkernel"?

> 
> CONFIG_CRASH_DUMP is enabled by default (CONFIG_RELOCATABLE is also enabled).

No it's not: you didn't add "default y" anywhere. What you actually did 
is to enable it *in the defconfig*. And the latter part about 
CONFIG_RELOCATABLE shouldn't be necessary, it's implementation detail 
after all, and the users likely don't have to be aware of it.

Better reword a little bit, like "Also enable CONFIG_CRASH_DUMP in 
loongson3_defconfig".

> 
> Signed-off-by: Youling Tang <tangyouling@loongson.cn>
> ---
>   arch/loongarch/Kconfig                     | 12 +-----------
>   arch/loongarch/Makefile                    |  4 ----
>   arch/loongarch/configs/loongson3_defconfig |  1 +
>   arch/loongarch/include/asm/addrspace.h     |  2 ++
>   arch/loongarch/kernel/head.S               |  2 +-
>   5 files changed, 5 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
> index ab4c2ab146ab..84f220273296 100644
> --- a/arch/loongarch/Kconfig
> +++ b/arch/loongarch/Kconfig
> @@ -502,6 +502,7 @@ config KEXEC
>   
>   config CRASH_DUMP
>   	bool "Build kdump crash kernel"
> +	select RELOCATABLE
>   	help
>   	  Generate crash dump after being started by kexec. This should
>   	  be normally only set in special crash dump kernels which are
> @@ -511,17 +512,6 @@ config CRASH_DUMP
>   
>   	  For more details see Documentation/admin-guide/kdump/kdump.rst
>   
> -config PHYSICAL_START
> -	hex "Physical address where the kernel is loaded"
> -	default "0x90000000a0000000"
> -	depends on CRASH_DUMP
> -	help
> -	  This gives the XKPRANGE address where the kernel is loaded.
> -	  If you plan to use kernel for capturing the crash dump change
> -	  this value to start of the reserved region (the "X" value as
> -	  specified in the "crashkernel=YM@XM" command line boot parameter
> -	  passed to the panic-ed kernel).
> -
>   config RELOCATABLE
>   	bool "Relocatable kernel"
>   	help
> diff --git a/arch/loongarch/Makefile b/arch/loongarch/Makefile
> index 2aba6ff30436..8304fed7aa42 100644
> --- a/arch/loongarch/Makefile
> +++ b/arch/loongarch/Makefile
> @@ -79,11 +79,7 @@ endif
>   cflags-y += -ffreestanding
>   cflags-y += $(call cc-option, -mno-check-zero-division)
>   
> -ifndef CONFIG_PHYSICAL_START
>   load-y		= 0x9000000000200000
> -else
> -load-y		= $(CONFIG_PHYSICAL_START)
> -endif
>   bootvars-y	= VMLINUX_LOAD_ADDRESS=$(load-y)

Both load-y and VMLINUX_LOAD_ADDRESS are kinda LoongArch-ism (stemming 
from similar usage in arch/mips apparently), so why not just drop load-y 
and fold the constant into the bootvars-y definition? So we have one 
piece of "special" definition instead of two.

>   
>   drivers-$(CONFIG_PCI)		+= arch/loongarch/pci/
> diff --git a/arch/loongarch/configs/loongson3_defconfig b/arch/loongarch/configs/loongson3_defconfig
> index cb52774c80e8..7885f6e5de93 100644
> --- a/arch/loongarch/configs/loongson3_defconfig
> +++ b/arch/loongarch/configs/loongson3_defconfig
> @@ -48,6 +48,7 @@ CONFIG_HOTPLUG_CPU=y
>   CONFIG_NR_CPUS=64
>   CONFIG_NUMA=y
>   CONFIG_KEXEC=y
> +CONFIG_CRASH_DUMP=y
>   CONFIG_SUSPEND=y
>   CONFIG_HIBERNATION=y
>   CONFIG_ACPI=y
> diff --git a/arch/loongarch/include/asm/addrspace.h b/arch/loongarch/include/asm/addrspace.h
> index d342935e5a72..4edcb3c21cf5 100644
> --- a/arch/loongarch/include/asm/addrspace.h
> +++ b/arch/loongarch/include/asm/addrspace.h
> @@ -125,4 +125,6 @@ extern unsigned long vm_map_base;
>   #define ISA_IOSIZE	SZ_16K
>   #define IO_SPACE_LIMIT	(PCI_IOSIZE - 1)
>   
> +#define PHYS_LINK_ADDR	PHYSADDR(VMLINUX_LOAD_ADDRESS)
> +
>   #endif /* _ASM_ADDRSPACE_H */
> diff --git a/arch/loongarch/kernel/head.S b/arch/loongarch/kernel/head.S
> index b12f459ad73a..57962ff08f6d 100644
> --- a/arch/loongarch/kernel/head.S
> +++ b/arch/loongarch/kernel/head.S
> @@ -24,7 +24,7 @@ _head:
>   	.org	0x8
>   	.dword	kernel_entry		/* Kernel entry point */
>   	.dword	_end - _text		/* Kernel image effective size */
> -	.quad	0			/* Kernel image load offset from start of RAM */
> +	.quad	PHYS_LINK_ADDR		/* Kernel image load offset from start of RAM */
>   	.org	0x38			/* 0x20 ~ 0x37 reserved */
>   	.long	LINUX_PE_MAGIC
>   	.long	pe_header - _head	/* Offset to the PE header */

-- 
WANG "xen0n" Xuerui

Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/