linux-kernel - Re: [PATCH 1/2] LoongArch: kdump: Add the same binary implementation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6d71e063-8e18-dd08-ac40-36b41ccfcfdb@xen0n.name>
Date:   Wed, 22 Feb 2023 17:05:31 +0800
From:   WANG Xuerui <kernel@...0n.name>
To:     Youling Tang <tangyouling@...ngson.cn>,
        Huacai Chen <chenhuacai@...nel.org>
Cc:     loongarch@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] LoongArch: kdump: Add the same binary implementation

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@...ngson.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/