| 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
| ||
|
Message-ID: <10b22e23-7865-424d-95d2-eeab366e7cc3@linux.ibm.com>
Date: Thu, 23 Jan 2025 17:23:04 +0530
From: Sourabh Jain <sourabhjain@...ux.ibm.com>
To: Hari Bathini <hbathini@...ux.ibm.com>, linuxppc-dev@...ts.ozlabs.org
Cc: Andrew Morton <akpm@...ux-foundation.org>, Baoquan he <bhe@...hat.com>,
Madhavan Srinivasan <maddy@...ux.ibm.com>,
Michael Ellerman <mpe@...erman.id.au>, kexec@...ts.infradead.org,
linux-kernel@...r.kernel.org, Mahesh Salgaonkar <mahesh@...ux.ibm.com>
Subject: Re: [PATCH v2 5/6] powerpc/crash: use generic crashkernel reservation
Hello Hari,
On 23/01/25 16:15, Hari Bathini wrote:
> Hi Sourabh,
>
> On 21/01/25 5:24 pm, Sourabh Jain wrote:
>> Commit 0ab97169aa05 ("crash_core: add generic function to do
>> reservation") added a generic function to reserve crashkernel memory.
>> So let's use the same function on powerpc and remove the
>> architecture-specific code that essentially does the same thing.
>>
>> The generic crashkernel reservation also provides a way to split the
>> crashkernel reservation into high and low memory reservations, which can
>> be enabled for powerpc in the future.
>>
>> Along with moving to the generic crashkernel reservation, the code
>> related to finding the base address for the crashkernel has been
>> separated into its own function name get_crash_base() for better
>> readability and maintainability.
>>
>> To prevent crashkernel memory from being added to iomem_resource, the
>> function arch_add_crash_res_to_iomem() has been introduced. For further
>> details on why this should not be done for the PowerPC architecture,
>> please refer to the previous commit titled "crash: let arch decide crash
>> memory export to iomem_resource.
>>
>> Cc: Andrew Morton <akpm@...ux-foundation.org>
>> Cc: Baoquan he <bhe@...hat.com>
>> Cc: Hari Bathini <hbathini@...ux.ibm.com>
>> CC: Madhavan Srinivasan <maddy@...ux.ibm.com>
>> Cc: Michael Ellerman <mpe@...erman.id.au>
>> Cc: kexec@...ts.infradead.org
>> Cc: linux-kernel@...r.kernel.org
>> Reviewed-by: Mahesh Salgaonkar <mahesh@...ux.ibm.com>
>> Signed-off-by: Sourabh Jain <sourabhjain@...ux.ibm.com>
>> ---
>> arch/powerpc/Kconfig | 3 +
>> arch/powerpc/include/asm/crash_reserve.h | 18 +++++
>> arch/powerpc/include/asm/kexec.h | 4 +-
>> arch/powerpc/kernel/prom.c | 2 +-
>> arch/powerpc/kexec/core.c | 90 ++++++++++--------------
>> 5 files changed, 63 insertions(+), 54 deletions(-)
>> create mode 100644 arch/powerpc/include/asm/crash_reserve.h
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index db9f7b2d07bf..880d35fadf40 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -718,6 +718,9 @@ config ARCH_SUPPORTS_CRASH_HOTPLUG
>> def_bool y
>> depends on PPC64
>> +config ARCH_HAS_GENERIC_CRASHKERNEL_RESERVATION
>> + def_bool CRASH_RESERVE
>> +
>> config FA_DUMP
>> bool "Firmware-assisted dump"
>> depends on CRASH_DUMP && PPC64 && (PPC_RTAS || PPC_POWERNV)
>> diff --git a/arch/powerpc/include/asm/crash_reserve.h
>> b/arch/powerpc/include/asm/crash_reserve.h
>> new file mode 100644
>> index 000000000000..f5e60721de41
>> --- /dev/null
>> +++ b/arch/powerpc/include/asm/crash_reserve.h
>> @@ -0,0 +1,18 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _ASM_POWERPC_CRASH_RESERVE_H
>> +#define _ASM_POWERPC_CRASH_RESERVE_H
>> +
>> +/* crash kernel regions are Page size agliged */
>> +#define CRASH_ALIGN PAGE_SIZE
>> +
>> +#ifdef CONFIG_ARCH_HAS_GENERIC_CRASHKERNEL_RESERVATION
>> +
>
>> +static inline bool arch_add_crash_res_to_iomem(void)
>> +{
>> + return false;
>> +}
>> +#define arch_add_crash_res_to_iomem arch_add_crash_res_to_iomem
>
> This is probably not needed if commit c40dd2f766440 can be reverted.
> Othewise..
Agree. Since I am reverting c40dd2f766440 in next series so I will the
above change.
>
> Acked-by: Hari Bathini <hbathini@...ux.ibm.com>
Thanks for the Ack!
- Sourabh Jain
>
>> +#endif
>> +
>> +#endif /* _ASM_POWERPC_CRASH_RESERVE_H */
>> +
>> diff --git a/arch/powerpc/include/asm/kexec.h
>> b/arch/powerpc/include/asm/kexec.h
>> index 270ee93a0f7d..64741558071f 100644
>> --- a/arch/powerpc/include/asm/kexec.h
>> +++ b/arch/powerpc/include/asm/kexec.h
>> @@ -113,9 +113,9 @@ int setup_new_fdt_ppc64(const struct kimage
>> *image, void *fdt, struct crash_mem
>> #ifdef CONFIG_CRASH_RESERVE
>> int __init overlaps_crashkernel(unsigned long start, unsigned long
>> size);
>> -extern void reserve_crashkernel(void);
>> +extern void arch_reserve_crashkernel(void);
>> #else
>> -static inline void reserve_crashkernel(void) {}
>> +static inline void arch_reserve_crashkernel(void) {}
>> static inline int overlaps_crashkernel(unsigned long start,
>> unsigned long size) { return 0; }
>> #endif
>> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
>> index e0059842a1c6..9ed9dde7d231 100644
>> --- a/arch/powerpc/kernel/prom.c
>> +++ b/arch/powerpc/kernel/prom.c
>> @@ -860,7 +860,7 @@ void __init early_init_devtree(void *params)
>> */
>> if (fadump_reserve_mem() == 0)
>> #endif
>> - reserve_crashkernel();
>> + arch_reserve_crashkernel();
>> early_reserve_mem();
>> if (memory_limit > memblock_phys_mem_size())
>> diff --git a/arch/powerpc/kexec/core.c b/arch/powerpc/kexec/core.c
>> index 4945b33322ae..b21cfa814492 100644
>> --- a/arch/powerpc/kexec/core.c
>> +++ b/arch/powerpc/kexec/core.c
>> @@ -80,38 +80,20 @@ void machine_kexec(struct kimage *image)
>> }
>> #ifdef CONFIG_CRASH_RESERVE
>> -void __init reserve_crashkernel(void)
>> -{
>> - unsigned long long crash_size, crash_base, total_mem_sz;
>> - int ret;
>> - total_mem_sz = memory_limit ? memory_limit :
>> memblock_phys_mem_size();
>> - /* use common parsing */
>> - ret = parse_crashkernel(boot_command_line, total_mem_sz,
>> - &crash_size, &crash_base, NULL, NULL);
>> - if (ret == 0 && crash_size > 0) {
>> - crashk_res.start = crash_base;
>> - crashk_res.end = crash_base + crash_size - 1;
>> - }
>> -
>> - if (crashk_res.end == crashk_res.start) {
>> - crashk_res.start = crashk_res.end = 0;
>> - return;
>> - }
>> -
>> - /* We might have got these values via the command line or the
>> - * device tree, either way sanitise them now. */
>> -
>> - crash_size = resource_size(&crashk_res);
>> +static unsigned long long __init get_crash_base(unsigned long long
>> crash_base)
>> +{
>> #ifndef CONFIG_NONSTATIC_KERNEL
>> - if (crashk_res.start != KDUMP_KERNELBASE)
>> + if (crash_base != KDUMP_KERNELBASE)
>> printk("Crash kernel location must be 0x%x\n",
>> KDUMP_KERNELBASE);
>> - crashk_res.start = KDUMP_KERNELBASE;
>> + return KDUMP_KERNELBASE;
>> #else
>> - if (!crashk_res.start) {
>> + unsigned long long crash_base_align;
>> +
>> + if (!crash_base) {
>> #ifdef CONFIG_PPC64
>> /*
>> * On the LPAR platform place the crash kernel to mid of
>> @@ -123,45 +105,51 @@ void __init reserve_crashkernel(void)
>> * kernel starts at 128MB offset on other platforms.
>> */
>> if (firmware_has_feature(FW_FEATURE_LPAR))
>> - crashk_res.start = min_t(u64, ppc64_rma_size / 2, SZ_512M);
>> + crash_base = min_t(u64, ppc64_rma_size / 2, SZ_512M);
>> else
>> - crashk_res.start = min_t(u64, ppc64_rma_size / 2, SZ_128M);
>> + crash_base = min_t(u64, ppc64_rma_size / 2, SZ_128M);
>> #else
>> - crashk_res.start = KDUMP_KERNELBASE;
>> + crash_base = KDUMP_KERNELBASE;
>> #endif
>> }
>> - crash_base = PAGE_ALIGN(crashk_res.start);
>> - if (crash_base != crashk_res.start) {
>> - printk("Crash kernel base must be aligned to 0x%lx\n",
>> - PAGE_SIZE);
>> - crashk_res.start = crash_base;
>> - }
>> + crash_base_align = PAGE_ALIGN(crash_base);
>> + if (crash_base != crash_base_align)
>> + pr_warn("Crash kernel base must be aligned to 0x%lx\n",
>> PAGE_SIZE);
>> + return crash_base_align;
>> #endif
>> - crash_size = PAGE_ALIGN(crash_size);
>> - crashk_res.end = crashk_res.start + crash_size - 1;
>> +}
>> - /* The crash region must not overlap the current kernel */
>> - if (overlaps_crashkernel(__pa(_stext), _end - _stext)) {
>> - printk(KERN_WARNING
>> - "Crash kernel can not overlap current kernel\n");
>> - crashk_res.start = crashk_res.end = 0;
>> +void __init arch_reserve_crashkernel(void)
>> +{
>> + unsigned long long crash_size, crash_base, crash_end;
>> + unsigned long long kernel_start, kernel_size;
>> + unsigned long long total_mem_sz;
>> + int ret;
>> +
>> + total_mem_sz = memory_limit ? memory_limit :
>> memblock_phys_mem_size();
>> +
>> + /* use common parsing */
>> + ret = parse_crashkernel(boot_command_line, total_mem_sz,
>> &crash_size,
>> + &crash_base, NULL, NULL);
>> +
>> + if (ret)
>> return;
>> - }
>> - printk(KERN_INFO "Reserving %ldMB of memory at %ldMB "
>> - "for crashkernel (System RAM: %ldMB)\n",
>> - (unsigned long)(crash_size >> 20),
>> - (unsigned long)(crashk_res.start >> 20),
>> - (unsigned long)(total_mem_sz >> 20));
>> + crash_base = get_crash_base(crash_base);
>> + crash_end = crash_base + crash_size - 1;
>> - if (!memblock_is_region_memory(crashk_res.start, crash_size) ||
>> - memblock_reserve(crashk_res.start, crash_size)) {
>> - pr_err("Failed to reserve memory for crashkernel!\n");
>> - crashk_res.start = crashk_res.end = 0;
>> + kernel_start = __pa(_stext);
>> + kernel_size = _end - _stext;
>> +
>> + /* The crash region must not overlap the current kernel */
>> + if ((kernel_start + kernel_size > crash_base) && (kernel_start
>> <= crash_end)) {
>> + pr_warn("Crash kernel can not overlap current kernel\n");
>> return;
>> }
>> +
>> + reserve_crashkernel_generic(crash_size, crash_base, 0, false);
>> }
>> int __init overlaps_crashkernel(unsigned long start, unsigned
>> long size)
>
Powered by blists - more mailing lists