[PATCH V5] panic: Move panic_print before kmsg dumpers

2022-02-11 Thread Guilherme G. Piccoli
The panic_print setting allows users to collect more information in a
panic event, like memory stats, tasks, CPUs backtraces, etc.
This is an interesting debug mechanism, but currently the print event
happens *after* kmsg_dump(), meaning that pstore, for example, cannot
collect a dmesg with the panic_print extra information.

This patch changes that in 2 ways:

(a) The panic_print setting allows to replay the existing kernel log
buffer to the console (bit 5), besides the extra information dump.
This functionality makes sense only at the end of the panic() function.
So, we hereby allow to distinguish the two situations by a new boolean
parameter in the function panic_print_sys_info().

(b) With the above change, we can safely call panic_print_sys_info()
before kmsg_dump(), allowing to dump the extra information when using
pstore or other kmsg dumpers.

The additional messages from panic_print could overwrite the oldest
messages when the buffer is full. The only reasonable solution is to
use a large enough log buffer, hence we added an advice into the kernel
parameters documentation about that.

Cc: Baoquan He 
Cc: Feng Tang 
Cc: Petr Mladek 
Signed-off-by: Guilherme G. Piccoli 
---


V5:
* Rebased against next-20220211.
* Removed code dealing with kdump, based on Baoquan concerns.
  This was possible after asking Stephen to remove a patch from
  linux-next[0] to address Baoquan sugestions, so this version
  is more simple and doesn't ever panic_print before kdump, unless
  "crash_kexec_post_notifiers" is passed in the kernel cmdline.

[0] 
https://lore.kernel.org/lkml/c10fc4fc-58c9-0b3f-5f1e-6f44b0c19...@igalia.com/

V4: https://lore.kernel.org/lkml/20220124203101.216051-1-gpicc...@igalia.com/


 Documentation/admin-guide/kernel-parameters.txt |  4 
 kernel/panic.c  | 13 +
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt 
b/Documentation/admin-guide/kernel-parameters.txt
index 3c2b3e24e8f5..2cf7078eaa95 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3766,6 +3766,10 @@
bit 4: print ftrace buffer
bit 5: print all printk messages in buffer
bit 6: print all CPUs backtrace (if available in the 
arch)
+   *Be aware* that this option may print a _lot_ of lines,
+   so there are risks of losing older messages in the log.
+   Use this option carefully, maybe worth to setup a
+   bigger log buffer with "log_buf_len" along with this.
 
panic_on_taint= Bitmask for conditionally calling panic() in add_taint()
Format: [,nousertaint]
diff --git a/kernel/panic.c b/kernel/panic.c
index 3c3fb36d8d41..eb4dfb932c85 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -148,10 +148,13 @@ void nmi_panic(struct pt_regs *regs, const char *msg)
 }
 EXPORT_SYMBOL(nmi_panic);
 
-static void panic_print_sys_info(void)
+static void panic_print_sys_info(bool console_flush)
 {
-   if (panic_print & PANIC_PRINT_ALL_PRINTK_MSG)
-   console_flush_on_panic(CONSOLE_REPLAY_ALL);
+   if (console_flush) {
+   if (panic_print & PANIC_PRINT_ALL_PRINTK_MSG)
+   console_flush_on_panic(CONSOLE_REPLAY_ALL);
+   return;
+   }
 
if (panic_print & PANIC_PRINT_ALL_CPU_BT)
trigger_all_cpu_backtrace();
@@ -286,6 +289,8 @@ void panic(const char *fmt, ...)
 */
atomic_notifier_call_chain(_notifier_list, 0, buf);
 
+   panic_print_sys_info(false);
+
kmsg_dump(KMSG_DUMP_PANIC);
 
/*
@@ -316,7 +321,7 @@ void panic(const char *fmt, ...)
debug_locks_off();
console_flush_on_panic(CONSOLE_FLUSH_PENDING);
 
-   panic_print_sys_info();
+   panic_print_sys_info(true);
 
if (!panic_blink)
panic_blink = no_blink;
-- 
2.35.0


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec


Re: [PATCH v5 2/6] powerpc/kexec_file: Add KEXEC_SIG support.

2022-02-11 Thread Paul Menzel

Dear Michal,


Am 09.02.22 um 13:01 schrieb Michal Suchánek:


On Wed, Feb 09, 2022 at 07:44:15AM +0100, Paul Menzel wrote:



Am 11.01.22 um 12:37 schrieb Michal Suchanek:


[…]


How can this be tested?


Apparently KEXEC_SIG_FORCE is x86 only although the use of the option is
arch neutral:

arch/x86/Kconfig:config KEXEC_SIG_FORCE
kernel/kexec_file.c:if (IS_ENABLED(CONFIG_KEXEC_SIG_FORCE))
{

Maybe it should be moved?


Sounds good.


I used a patched kernel that enables lockdown in secure boot, and then
verified that signed kernel can be loaded by kexec and unsigned not,
with KEXEC_SIG enabled and IMA_KEXEC disabled.

The lockdown support can be enabled on any platform, and although I
can't find it documented anywhere there appears to be code in kexec_file
to take it into account:
kernel/kexec.c: result = security_locked_down(LOCKDOWN_KEXEC);
kernel/kexec_file.c:security_locked_down(LOCKDOWN_KEXEC))
kernel/module.c:return security_locked_down(LOCKDOWN_MODULE_SIGNATURE);
kernel/params.c:security_locked_down(LOCKDOWN_MODULE_PARAMETERS))
and lockdown can be enabled with a buildtime option, a kernel parameter, or a
debugfs file.

Still for testing lifting KEXEC_SIG_FORCE to some arch-neutral Kconfig file is
probably the simplest option.

kexec -s option should be used to select kexec_file rather than the old
style kexec which would either fail always or succeed always regardelss
of signature.


Thank you.


Signed-off-by: Michal Suchanek 
---
v3: - Philipp Rudo : Update the comit message with
explanation why the s390 code is usable on powerpc.
  - Include correct header for mod_check_sig
  - Nayna : Mention additional IMA features
in kconfig text
---
   arch/powerpc/Kconfig| 16 
   arch/powerpc/kexec/elf_64.c | 36 
   2 files changed, 52 insertions(+)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index dea74d7717c0..1cde9b6c5987 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -560,6 +560,22 @@ config KEXEC_FILE
   config ARCH_HAS_KEXEC_PURGATORY
def_bool KEXEC_FILE
+config KEXEC_SIG
+   bool "Verify kernel signature during kexec_file_load() syscall"
+   depends on KEXEC_FILE && MODULE_SIG_FORMAT
+   help
+ This option makes kernel signature verification mandatory for
+ the kexec_file_load() syscall.
+
+ In addition to that option, you need to enable signature
+ verification for the corresponding kernel image type being
+ loaded in order for this to work.
+
+ Note: on powerpc IMA_ARCH_POLICY also implements kexec'ed kernel
+ verification. In addition IMA adds kernel hashes to the measurement
+ list, extends IMA PCR in the TPM, and implements kernel image
+ blacklist by hash.


So, what is the takeaway for the user? IMA_ARCH_POLICY is preferred? What is
the disadvantage, and two implementations(?) needed then? More overhead?


IMA_KEXEC does more than KEXEC_SIG. The overhead is probably not big
unless you are trying to really minimize the kernel code size.

Arguably the simpler implementation has less potential for bugs, too.
Both in code and in user configuration required to enable the feature.

Interestingly IMA_ARCH_POLICY depends on KEXEC_SIG rather than
IMA_KEXEC. Just mind-boggling.


I have not looked into that.


The main problem with IMA_KEXEC from my point of view is it is not portable.
To record the measurements TPM support is requireed which is not available on
all platforms. It does not support PE so it cannot be used on platforms
that use PE kernel signature format.


Could you add that to the comment please?


+
   config RELOCATABLE
bool "Build a relocatable kernel"
depends on PPC64 || (FLATMEM && (44x || FSL_BOOKE))
diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
index eeb258002d1e..98d1cb5135b4 100644
--- a/arch/powerpc/kexec/elf_64.c
+++ b/arch/powerpc/kexec/elf_64.c
@@ -23,6 +23,7 @@
   #include 
   #include 
   #include 
+#include 
   static void *elf64_load(struct kimage *image, char *kernel_buf,
unsigned long kernel_len, char *initrd,
@@ -151,7 +152,42 @@ static void *elf64_load(struct kimage *image, char 
*kernel_buf,
return ret ? ERR_PTR(ret) : NULL;
   }
+#ifdef CONFIG_KEXEC_SIG
+int elf64_verify_sig(const char *kernel, unsigned long kernel_len)
+{
+   const unsigned long marker_len = sizeof(MODULE_SIG_STRING) - 1;
+   struct module_signature *ms;
+   unsigned long sig_len;


Use size_t to match the signature of `verify_pkcs7_signature()`?


Nope. struct module_signature uses unsigned long, and this needs to be
matched to avoid type errors on 32bit.


I meant for `sig_len`.


Technically using size_t for in-memory buffers is misguided because
AFAICT no memory buffer can be bigger than ULONG_MAX, and size_t is
non-native type on 32bit.

Sure, the 

Re: [PATCH v20 3/5] arm64: kdump: reimplement crashkernel=X

2022-02-11 Thread Baoquan He
On 02/11/22 at 06:41pm, Leizhen (ThunderTown) wrote:
> 
> 
> On 2022/2/11 18:30, Baoquan He wrote:
> > On 01/24/22 at 04:47pm, Zhen Lei wrote:
> >> From: Chen Zhou 
> > ..
> >> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> >> index 6c653a2c7cff052..a5d43feac0d7d96 100644
> >> --- a/arch/arm64/mm/init.c
> >> +++ b/arch/arm64/mm/init.c
> >> @@ -71,6 +71,30 @@ phys_addr_t arm64_dma_phys_limit __ro_after_init;
> >>  #define CRASH_ADDR_LOW_MAXarm64_dma_phys_limit
> >>  #define CRASH_ADDR_HIGH_MAX   MEMBLOCK_ALLOC_ACCESSIBLE
> >>  
> >> +static int __init reserve_crashkernel_low(unsigned long long low_size)
> >> +{
> >> +  unsigned long long low_base;
> >> +
> >> +  /* passed with crashkernel=0,low ? */
> >> +  if (!low_size)
> >> +  return 0;
> >> +
> >> +  low_base = memblock_phys_alloc_range(low_size, CRASH_ALIGN, 0, 
> >> CRASH_ADDR_LOW_MAX);
> >> +  if (!low_base) {
> >> +  pr_err("cannot allocate crashkernel low memory 
> >> (size:0x%llx).\n", low_size);
> >> +  return -ENOMEM;
> >> +  }
> >> +
> >> +  pr_info("crashkernel low memory reserved: 0x%llx - 0x%llx (%lld MB)\n",
> >> +  low_base, low_base + low_size, low_size >> 20);
> >> +
> >> +  crashk_low_res.start = low_base;
> >> +  crashk_low_res.end   = low_base + low_size - 1;
> >> +  insert_resource(_resource, _low_res);
> >> +
> >> +  return 0;
> >> +}
> >> +
> >>  /*
> >>   * reserve_crashkernel() - reserves memory for crash kernel
> >>   *
> >> @@ -81,29 +105,62 @@ phys_addr_t arm64_dma_phys_limit __ro_after_init;
> >>  static void __init reserve_crashkernel(void)
> >>  {
> >>unsigned long long crash_base, crash_size;
> >> +  unsigned long long crash_low_size = SZ_256M;
> >>unsigned long long crash_max = CRASH_ADDR_LOW_MAX;
> >>int ret;
> >> +  bool fixed_base;
> >> +  char *cmdline = boot_command_line;
> >>  
> >> -  ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(),
> >> +  /* crashkernel=X[@offset] */
> >> +  ret = parse_crashkernel(cmdline, memblock_phys_mem_size(),
> >>_size, _base);
> >> -  /* no crashkernel= or invalid value specified */
> >> -  if (ret || !crash_size)
> >> -  return;
> >> +  if (ret || !crash_size) {
> >> +  unsigned long long low_size;
> >>  
> >> +  /* crashkernel=X,high */
> >> +  ret = parse_crashkernel_high(cmdline, 0, _size, 
> >> _base);
> >> +  if (ret || !crash_size)
> >> +  return;
> >> +
> >> +  /* crashkernel=X,low */
> >> +  ret = parse_crashkernel_low(cmdline, 0, _size, _base);
> >> +  if (!ret)
> >> +  crash_low_size = low_size;
> > 
> > Here, the error case is not checked and handled. But it still gets
> > expeced result which is the default SZ_256M. Is this designed on
> > purpose?
> 
> Yes, we can specify only "crashkernel=X,high".
> 
> This is mentioned in Documentation/admin-guide/kernel-parameters.txt
> 
> crashkernel=size[KMG],low
> [KNL, X86-64] range under 4G. When crashkernel=X,high
> is passed, kernel could allocate physical memory 
> region
> above 4G, that cause second kernel crash on system
> that require some amount of low memory, e.g. swiotlb
> requires at least 64M+32K low memory, also enough 
> extra
> low memory is needed to make sure DMA buffers for 
> 32-bit
> devices won't run out. Kernel would try to allocate 
> at <-
> least 256M below 4G automatically.
>  <-

Yeah, that is expected becasue no crahskernel=,low is a right usage. The
'ret' is 0 in the case. If I gave below string, it works too.
"crashkernel=256M,high crashkernel=aaabbadfadfd,low"

> 
> > 
> >> +
> >> +  crash_max = CRASH_ADDR_HIGH_MAX;
> >> +  }
> >> +
> >> +  fixed_base = !!crash_base;
> >>crash_size = PAGE_ALIGN(crash_size);
> >>  
> >>/* User specifies base address explicitly. */
> >>if (crash_base)
> >>crash_max = crash_base + crash_size;
> >>  
> >> +retry:
> >>crash_base = memblock_phys_alloc_range(crash_size, CRASH_ALIGN,
> >>   crash_base, crash_max);
> >>if (!crash_base) {
> >> +  /*
> >> +   * Attempt to fully allocate low memory failed, fall back
> >> +   * to high memory, the minimum required low memory will be
> >> +   * reserved later.
> >> +   */
> >> +  if (!fixed_base && (crash_max == CRASH_ADDR_LOW_MAX)) {
> >> +  crash_max = CRASH_ADDR_HIGH_MAX;
> >> +  goto retry;
> >> +  }
> >> +
> >>pr_warn("cannot allocate crashkernel (size:0x%llx)\n",
> >>crash_size);
> >>return;
> >>}
> >>  
> >> +  if (crash_base >= SZ_4G && 

Re: [PATCH v20 2/5] arm64: kdump: introduce some macros for crash kernel reservation

2022-02-11 Thread Baoquan He
On 01/24/22 at 04:47pm, Zhen Lei wrote:
> From: Chen Zhou 
> 
> Introduce macro CRASH_ALIGN for alignment, macro CRASH_ADDR_LOW_MAX
> for upper bound of low crash memory, macro CRASH_ADDR_HIGH_MAX for
> upper bound of high crash memory, use macros instead.
> 
> Signed-off-by: Chen Zhou 
> Signed-off-by: Zhen Lei 
> Tested-by: John Donnelly 
> Tested-by: Dave Kleikamp 
> ---
>  arch/arm64/mm/init.c | 11 ---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index 90f276d46b93bc6..6c653a2c7cff052 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -65,6 +65,12 @@ EXPORT_SYMBOL(memstart_addr);
>  phys_addr_t arm64_dma_phys_limit __ro_after_init;
>  
>  #ifdef CONFIG_KEXEC_CORE
> +/* Current arm64 boot protocol requires 2MB alignment */
> +#define CRASH_ALIGN  SZ_2M
> +
> +#define CRASH_ADDR_LOW_MAX   arm64_dma_phys_limit
> +#define CRASH_ADDR_HIGH_MAX  MEMBLOCK_ALLOC_ACCESSIBLE

MEMBLOCK_ALLOC_ACCESSIBLE is obvoiously a alloc flag for memblock
allocator, I don't think it's appropriate to make HIGH_MAX get its value.
You can make it as memblock.current_limit, or do not define it, but using
MEMBLOCK_ALLOC_ACCESSIBLE direclty in memblock_phys_alloc_range() with
a code comment. 


> +
>  /*
>   * reserve_crashkernel() - reserves memory for crash kernel
>   *
> @@ -75,7 +81,7 @@ phys_addr_t arm64_dma_phys_limit __ro_after_init;
>  static void __init reserve_crashkernel(void)
>  {
>   unsigned long long crash_base, crash_size;
> - unsigned long long crash_max = arm64_dma_phys_limit;
> + unsigned long long crash_max = CRASH_ADDR_LOW_MAX;
>   int ret;
>  
>   ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(),
> @@ -90,8 +96,7 @@ static void __init reserve_crashkernel(void)
>   if (crash_base)
>   crash_max = crash_base + crash_size;
>  
> - /* Current arm64 boot protocol requires 2MB alignment */
> - crash_base = memblock_phys_alloc_range(crash_size, SZ_2M,
> + crash_base = memblock_phys_alloc_range(crash_size, CRASH_ALIGN,
>  crash_base, crash_max);
>   if (!crash_base) {
>   pr_warn("cannot allocate crashkernel (size:0x%llx)\n",
> -- 
> 2.25.1
> 


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec


Re: [PATCH v20 3/5] arm64: kdump: reimplement crashkernel=X

2022-02-11 Thread Leizhen (ThunderTown)



On 2022/2/11 18:30, Baoquan He wrote:
> On 01/24/22 at 04:47pm, Zhen Lei wrote:
>> From: Chen Zhou 
> ..
>> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
>> index 6c653a2c7cff052..a5d43feac0d7d96 100644
>> --- a/arch/arm64/mm/init.c
>> +++ b/arch/arm64/mm/init.c
>> @@ -71,6 +71,30 @@ phys_addr_t arm64_dma_phys_limit __ro_after_init;
>>  #define CRASH_ADDR_LOW_MAX  arm64_dma_phys_limit
>>  #define CRASH_ADDR_HIGH_MAX MEMBLOCK_ALLOC_ACCESSIBLE
>>  
>> +static int __init reserve_crashkernel_low(unsigned long long low_size)
>> +{
>> +unsigned long long low_base;
>> +
>> +/* passed with crashkernel=0,low ? */
>> +if (!low_size)
>> +return 0;
>> +
>> +low_base = memblock_phys_alloc_range(low_size, CRASH_ALIGN, 0, 
>> CRASH_ADDR_LOW_MAX);
>> +if (!low_base) {
>> +pr_err("cannot allocate crashkernel low memory 
>> (size:0x%llx).\n", low_size);
>> +return -ENOMEM;
>> +}
>> +
>> +pr_info("crashkernel low memory reserved: 0x%llx - 0x%llx (%lld MB)\n",
>> +low_base, low_base + low_size, low_size >> 20);
>> +
>> +crashk_low_res.start = low_base;
>> +crashk_low_res.end   = low_base + low_size - 1;
>> +insert_resource(_resource, _low_res);
>> +
>> +return 0;
>> +}
>> +
>>  /*
>>   * reserve_crashkernel() - reserves memory for crash kernel
>>   *
>> @@ -81,29 +105,62 @@ phys_addr_t arm64_dma_phys_limit __ro_after_init;
>>  static void __init reserve_crashkernel(void)
>>  {
>>  unsigned long long crash_base, crash_size;
>> +unsigned long long crash_low_size = SZ_256M;
>>  unsigned long long crash_max = CRASH_ADDR_LOW_MAX;
>>  int ret;
>> +bool fixed_base;
>> +char *cmdline = boot_command_line;
>>  
>> -ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(),
>> +/* crashkernel=X[@offset] */
>> +ret = parse_crashkernel(cmdline, memblock_phys_mem_size(),
>>  _size, _base);
>> -/* no crashkernel= or invalid value specified */
>> -if (ret || !crash_size)
>> -return;
>> +if (ret || !crash_size) {
>> +unsigned long long low_size;
>>  
>> +/* crashkernel=X,high */
>> +ret = parse_crashkernel_high(cmdline, 0, _size, 
>> _base);
>> +if (ret || !crash_size)
>> +return;
>> +
>> +/* crashkernel=X,low */
>> +ret = parse_crashkernel_low(cmdline, 0, _size, _base);
>> +if (!ret)
>> +crash_low_size = low_size;
> 
> Here, the error case is not checked and handled. But it still gets
> expeced result which is the default SZ_256M. Is this designed on
> purpose?

Yes, we can specify only "crashkernel=X,high".

This is mentioned in Documentation/admin-guide/kernel-parameters.txt

crashkernel=size[KMG],low
[KNL, X86-64] range under 4G. When crashkernel=X,high
is passed, kernel could allocate physical memory region
above 4G, that cause second kernel crash on system
that require some amount of low memory, e.g. swiotlb
requires at least 64M+32K low memory, also enough extra
low memory is needed to make sure DMA buffers for 32-bit
devices won't run out. Kernel would try to allocate at  
   <-
least 256M below 4G automatically.  
   <-

> 
>> +
>> +crash_max = CRASH_ADDR_HIGH_MAX;
>> +}
>> +
>> +fixed_base = !!crash_base;
>>  crash_size = PAGE_ALIGN(crash_size);
>>  
>>  /* User specifies base address explicitly. */
>>  if (crash_base)
>>  crash_max = crash_base + crash_size;
>>  
>> +retry:
>>  crash_base = memblock_phys_alloc_range(crash_size, CRASH_ALIGN,
>> crash_base, crash_max);
>>  if (!crash_base) {
>> +/*
>> + * Attempt to fully allocate low memory failed, fall back
>> + * to high memory, the minimum required low memory will be
>> + * reserved later.
>> + */
>> +if (!fixed_base && (crash_max == CRASH_ADDR_LOW_MAX)) {
>> +crash_max = CRASH_ADDR_HIGH_MAX;
>> +goto retry;
>> +}
>> +
>>  pr_warn("cannot allocate crashkernel (size:0x%llx)\n",
>>  crash_size);
>>  return;
>>  }
>>  
>> +if (crash_base >= SZ_4G && reserve_crashkernel_low(crash_low_size)) {
>> +memblock_phys_free(crash_base, crash_size);
>> +return;
>> +}
>> +
>>  pr_info("crashkernel reserved: 0x%016llx - 0x%016llx (%lld MB)\n",
>>  crash_base, crash_base + crash_size, crash_size >> 20);
>>  
>> @@ -112,6 +169,9 @@ static void __init reserve_crashkernel(void)
>>   * map. Inform kmemleak so that it won't try 

Re: [PATCH v20 3/5] arm64: kdump: reimplement crashkernel=X

2022-02-11 Thread Baoquan He
On 01/24/22 at 04:47pm, Zhen Lei wrote:
> From: Chen Zhou 
..
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index 6c653a2c7cff052..a5d43feac0d7d96 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -71,6 +71,30 @@ phys_addr_t arm64_dma_phys_limit __ro_after_init;
>  #define CRASH_ADDR_LOW_MAX   arm64_dma_phys_limit
>  #define CRASH_ADDR_HIGH_MAX  MEMBLOCK_ALLOC_ACCESSIBLE
>  
> +static int __init reserve_crashkernel_low(unsigned long long low_size)
> +{
> + unsigned long long low_base;
> +
> + /* passed with crashkernel=0,low ? */
> + if (!low_size)
> + return 0;
> +
> + low_base = memblock_phys_alloc_range(low_size, CRASH_ALIGN, 0, 
> CRASH_ADDR_LOW_MAX);
> + if (!low_base) {
> + pr_err("cannot allocate crashkernel low memory 
> (size:0x%llx).\n", low_size);
> + return -ENOMEM;
> + }
> +
> + pr_info("crashkernel low memory reserved: 0x%llx - 0x%llx (%lld MB)\n",
> + low_base, low_base + low_size, low_size >> 20);
> +
> + crashk_low_res.start = low_base;
> + crashk_low_res.end   = low_base + low_size - 1;
> + insert_resource(_resource, _low_res);
> +
> + return 0;
> +}
> +
>  /*
>   * reserve_crashkernel() - reserves memory for crash kernel
>   *
> @@ -81,29 +105,62 @@ phys_addr_t arm64_dma_phys_limit __ro_after_init;
>  static void __init reserve_crashkernel(void)
>  {
>   unsigned long long crash_base, crash_size;
> + unsigned long long crash_low_size = SZ_256M;
>   unsigned long long crash_max = CRASH_ADDR_LOW_MAX;
>   int ret;
> + bool fixed_base;
> + char *cmdline = boot_command_line;
>  
> - ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(),
> + /* crashkernel=X[@offset] */
> + ret = parse_crashkernel(cmdline, memblock_phys_mem_size(),
>   _size, _base);
> - /* no crashkernel= or invalid value specified */
> - if (ret || !crash_size)
> - return;
> + if (ret || !crash_size) {
> + unsigned long long low_size;
>  
> + /* crashkernel=X,high */
> + ret = parse_crashkernel_high(cmdline, 0, _size, 
> _base);
> + if (ret || !crash_size)
> + return;
> +
> + /* crashkernel=X,low */
> + ret = parse_crashkernel_low(cmdline, 0, _size, _base);
> + if (!ret)
> + crash_low_size = low_size;

Here, the error case is not checked and handled. But it still gets
expeced result which is the default SZ_256M. Is this designed on
purpose?

> +
> + crash_max = CRASH_ADDR_HIGH_MAX;
> + }
> +
> + fixed_base = !!crash_base;
>   crash_size = PAGE_ALIGN(crash_size);
>  
>   /* User specifies base address explicitly. */
>   if (crash_base)
>   crash_max = crash_base + crash_size;
>  
> +retry:
>   crash_base = memblock_phys_alloc_range(crash_size, CRASH_ALIGN,
>  crash_base, crash_max);
>   if (!crash_base) {
> + /*
> +  * Attempt to fully allocate low memory failed, fall back
> +  * to high memory, the minimum required low memory will be
> +  * reserved later.
> +  */
> + if (!fixed_base && (crash_max == CRASH_ADDR_LOW_MAX)) {
> + crash_max = CRASH_ADDR_HIGH_MAX;
> + goto retry;
> + }
> +
>   pr_warn("cannot allocate crashkernel (size:0x%llx)\n",
>   crash_size);
>   return;
>   }
>  
> + if (crash_base >= SZ_4G && reserve_crashkernel_low(crash_low_size)) {
> + memblock_phys_free(crash_base, crash_size);
> + return;
> + }
> +
>   pr_info("crashkernel reserved: 0x%016llx - 0x%016llx (%lld MB)\n",
>   crash_base, crash_base + crash_size, crash_size >> 20);
>  
> @@ -112,6 +169,9 @@ static void __init reserve_crashkernel(void)
>* map. Inform kmemleak so that it won't try to access it.
>*/
>   kmemleak_ignore_phys(crash_base);
> + if (crashk_low_res.end)
> + kmemleak_ignore_phys(crashk_low_res.start);
> +
>   crashk_res.start = crash_base;
>   crashk_res.end = crash_base + crash_size - 1;
>   insert_resource(_resource, _res);
> -- 
> 2.25.1
> 


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec