Re: [PATCH] arm64: kdump: retain reserved memory regions

[AKASHI Takahiro]

Ard, Bhupesh,

Thank you for the comments.
I will re-post a revised patch soon after running some tests.

But I'm still wondering whether my original approach[1] may be
useful in other (non-ACPI/efi) cases given that the current
memblock_cap_memory_range() has kinda flaw that any memory
reserved by firmware can be ignored at crash dump kernel.

[1] 
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-January/553098.html

Thanks,
-Takahiro AKASHI

On Wed, Jan 31, 2018 at 11:20:20AM +0530, Bhupesh Sharma wrote:
> Hi Ard, Akashi,
> 
> On Mon, Jan 29, 2018 at 5:41 PM, Ard Biesheuvel
>  wrote:
> > On 29 January 2018 at 08:12, AKASHI Takahiro  
> > wrote:
> >> James,
> >>
> >> On Fri, Jan 19, 2018 at 11:39:58AM +, James Morse wrote:
> >>> Hi Akashi,
> >>>
> >>> On 11/01/18 11:38, AKASHI Takahiro wrote:
> >>> > On Wed, Jan 10, 2018 at 11:26:55AM +, James Morse wrote:
> >>> >> On 10/01/18 10:09, AKASHI Takahiro wrote:
> >>> >>> This is a fix against the issue that crash dump kernel may hang up
> >>> >>> during booting, which can happen on any ACPI-based system with "ACPI
> >>> >>> Reclaim Memory."
> >>>
> >>> >>> (diagnosis)
> >>> >>> * This fault is a data abort, alignment fault (ESR=0x9621)
> >>> >>>   during reading out ACPI table.
> >>> >>> * Initial ACPI tables are normally stored in system ram and marked as
> >>> >>>   "ACPI Reclaim memory" by the firmware.
> >>> >>> * After the commit f56ab9a5b73c ("efi/arm: Don't mark ACPI reclaim
> >>> >>>   memory as MEMBLOCK_NOMAP"), those regions' attribute were changed
> >>> >>>   removing NOMAP bit and they are instead "memblock-reserved".
> >>> >>> * When crash dump kernel boots up, it tries to accesses ACPI tables by
> >>> >>>   ioremap'ing them (through acpi_os_ioremap()).
> >>> >>> * Since those regions are not included in device tree's
> >>> >>>   "usable-memory-range" and so not recognized as part of crash dump
> >>> >>>   kernel's system ram, ioremap() will create a non-cacheable mapping 
> >>> >>> here.
> >>> >>
> >>> >> Ugh, because acpi_os_ioremap() looks at the efi memory map through the 
> >>> >> prism of
> >>> >> what we pulled into memblock, which is different during kdump.
> >>> >>
> >>> >> Is an alternative to teach acpi_os_ioremap() to ask
> >>> >> efi_mem_attributes() directly for the attributes to use?
> >>> >> (e.g. arch_apei_get_mem_attribute())
> >>> >
> >>> > I didn't think of this approach.
> >>> > Do you mean a change like the patch below?
> >>>
> >>> Yes. Aha, you can pretty much re-use the helper directly.
> >>>
> >>> It was just a suggestion, removing the extra abstraction that is causing 
> >>> the bug
> >>> could be cleaner ...
> >>>
> >>> > (I'm still debugging this code since the kernel fails to boot.)
> >>>
> >>> ... but might be too fragile.
> >>>
> >>> There are points during boot when the EFI memory map isn't mapped.
> >>
> >> Right, this was a problem for my patch.
> >> Attached is the revised and workable one.
> >> Efi_memmap_init_late() may alternatively be called in acpi_early_init() or
> >> even in acpi_os_ioremap(), but either way it looks a bit odd.
> >>
> >
> > Akashi-san,
> >
> > efi_memmap_init_late() is currently being called from
> > arm_enable_runtime_services(), which is an early initcall. If that is
> > too late for acpi_early_init(), we could perhaps move the call
> > forward, i.e., sth like
> >
> > -8<
> > diff --git a/drivers/firmware/efi/arm-runtime.c
> > b/drivers/firmware/efi/arm-runtime.c
> > index 6f60d659b323..e835d3b20af6 100644
> > --- a/drivers/firmware/efi/arm-runtime.c
> > +++ b/drivers/firmware/efi/arm-runtime.c
> > @@ -117,7 +117,7 @@ static bool __init efi_virtmap_init(void)
> >   * non-early mapping of the UEFI system table and virtual mappings for all
> >   * EFI_MEMORY_RUNTIME regions.
> >   */
> > -static int __init arm_enable_runtime_services(void)
> > +void __init efi_enter_virtual_mode(void)
> >  {
> > u64 mapsize;
> >
> > @@ -156,7 +156,6 @@ static int __init arm_enable_runtime_services(void)
> >
> > return 0;
> >  }
> > -early_initcall(arm_enable_runtime_services);
> >
> >  void efi_virtmap_load(void)
> >  {
> > diff --git a/init/main.c b/init/main.c
> > index a8100b954839..2d0927768e2d 100644
> > --- a/init/main.c
> > +++ b/init/main.c
> > @@ -674,6 +674,9 @@ asmlinkage __visible void __init start_kernel(void)
> > debug_objects_mem_init();
> > setup_per_cpu_pageset();
> > numa_policy_init();
> > +   if (IS_ENABLED(CONFIG_EFI) &&
> > +   (IS_ENABLED(CONFIG_ARM64) || IS_ENABLED(CONFIG_ARM)))
> > +   efi_enter_virtual_mode();
> > acpi_early_init();
> > if (late_time_init)
> > late_time_init();
> > -8<
> >
> > would be reasonable imo. Also, I think it is justifiable to make ACPI
> > depend on UEFI on arm64, which is notably different from x86.
> >
> > (I know 'efi_enter_virtual_mode' is 

Re: [PATCH] arm64: kdump: retain reserved memory regions

[Bhupesh Sharma]

Hi Ard, Akashi,

On Mon, Jan 29, 2018 at 5:41 PM, Ard Biesheuvel
 wrote:
> On 29 January 2018 at 08:12, AKASHI Takahiro  
> wrote:
>> James,
>>
>> On Fri, Jan 19, 2018 at 11:39:58AM +, James Morse wrote:
>>> Hi Akashi,
>>>
>>> On 11/01/18 11:38, AKASHI Takahiro wrote:
>>> > On Wed, Jan 10, 2018 at 11:26:55AM +, James Morse wrote:
>>> >> On 10/01/18 10:09, AKASHI Takahiro wrote:
>>> >>> This is a fix against the issue that crash dump kernel may hang up
>>> >>> during booting, which can happen on any ACPI-based system with "ACPI
>>> >>> Reclaim Memory."
>>>
>>> >>> (diagnosis)
>>> >>> * This fault is a data abort, alignment fault (ESR=0x9621)
>>> >>>   during reading out ACPI table.
>>> >>> * Initial ACPI tables are normally stored in system ram and marked as
>>> >>>   "ACPI Reclaim memory" by the firmware.
>>> >>> * After the commit f56ab9a5b73c ("efi/arm: Don't mark ACPI reclaim
>>> >>>   memory as MEMBLOCK_NOMAP"), those regions' attribute were changed
>>> >>>   removing NOMAP bit and they are instead "memblock-reserved".
>>> >>> * When crash dump kernel boots up, it tries to accesses ACPI tables by
>>> >>>   ioremap'ing them (through acpi_os_ioremap()).
>>> >>> * Since those regions are not included in device tree's
>>> >>>   "usable-memory-range" and so not recognized as part of crash dump
>>> >>>   kernel's system ram, ioremap() will create a non-cacheable mapping 
>>> >>> here.
>>> >>
>>> >> Ugh, because acpi_os_ioremap() looks at the efi memory map through the 
>>> >> prism of
>>> >> what we pulled into memblock, which is different during kdump.
>>> >>
>>> >> Is an alternative to teach acpi_os_ioremap() to ask
>>> >> efi_mem_attributes() directly for the attributes to use?
>>> >> (e.g. arch_apei_get_mem_attribute())
>>> >
>>> > I didn't think of this approach.
>>> > Do you mean a change like the patch below?
>>>
>>> Yes. Aha, you can pretty much re-use the helper directly.
>>>
>>> It was just a suggestion, removing the extra abstraction that is causing 
>>> the bug
>>> could be cleaner ...
>>>
>>> > (I'm still debugging this code since the kernel fails to boot.)
>>>
>>> ... but might be too fragile.
>>>
>>> There are points during boot when the EFI memory map isn't mapped.
>>
>> Right, this was a problem for my patch.
>> Attached is the revised and workable one.
>> Efi_memmap_init_late() may alternatively be called in acpi_early_init() or
>> even in acpi_os_ioremap(), but either way it looks a bit odd.
>>
>
> Akashi-san,
>
> efi_memmap_init_late() is currently being called from
> arm_enable_runtime_services(), which is an early initcall. If that is
> too late for acpi_early_init(), we could perhaps move the call
> forward, i.e., sth like
>
> -8<
> diff --git a/drivers/firmware/efi/arm-runtime.c
> b/drivers/firmware/efi/arm-runtime.c
> index 6f60d659b323..e835d3b20af6 100644
> --- a/drivers/firmware/efi/arm-runtime.c
> +++ b/drivers/firmware/efi/arm-runtime.c
> @@ -117,7 +117,7 @@ static bool __init efi_virtmap_init(void)
>   * non-early mapping of the UEFI system table and virtual mappings for all
>   * EFI_MEMORY_RUNTIME regions.
>   */
> -static int __init arm_enable_runtime_services(void)
> +void __init efi_enter_virtual_mode(void)
>  {
> u64 mapsize;
>
> @@ -156,7 +156,6 @@ static int __init arm_enable_runtime_services(void)
>
> return 0;
>  }
> -early_initcall(arm_enable_runtime_services);
>
>  void efi_virtmap_load(void)
>  {
> diff --git a/init/main.c b/init/main.c
> index a8100b954839..2d0927768e2d 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -674,6 +674,9 @@ asmlinkage __visible void __init start_kernel(void)
> debug_objects_mem_init();
> setup_per_cpu_pageset();
> numa_policy_init();
> +   if (IS_ENABLED(CONFIG_EFI) &&
> +   (IS_ENABLED(CONFIG_ARM64) || IS_ENABLED(CONFIG_ARM)))
> +   efi_enter_virtual_mode();
> acpi_early_init();
> if (late_time_init)
> late_time_init();
> -8<
>
> would be reasonable imo. Also, I think it is justifiable to make ACPI
> depend on UEFI on arm64, which is notably different from x86.
>
> (I know 'efi_enter_virtual_mode' is not entirely accurate here, given
> that we call SetVirtualAddressMap from the UEFI stub on ARM, but it is
> still close enough, given that one could argue that EFI is not in
> 'virtual mode' until the mappings are in place)
>
>
>
>> ===8<===
>> From c88f4c8106ba7a918c835b1cdf538b1d21019863 Mon Sep 17 00:00:00 2001
>> From: AKASHI Takahiro 
>> Date: Mon, 29 Jan 2018 15:07:43 +0900
>> Subject: [PATCH] arm64: kdump: make acpi_os_ioremap() more generic
>>
>> ---
>>  arch/arm64/include/asm/acpi.h | 23 ---
>>  arch/arm64/kernel/acpi.c  |  7 ++-
>>  init/main.c   |  4 
>>  3 files changed, 22 insertions(+), 12 deletions(-)
>>
>> diff --git 

Re: [PATCH V2] print kdump kernel loaded status in stack dump

[Petr Mladek]

On Tue 2018-01-30 17:07:29, Dave Young wrote:
> On 01/30/18 at 05:50pm, Sergey Senozhatsky wrote:
> > On (01/27/18 12:11), Dave Young wrote:
> > > It is useful to print kdump kernel loaded status in dump_stack() 
> > > especially when panic happens so that we can differenciate 
> > > kdump kernel early hang and a normal panic in a bug report.
> > > 
> > > Signed-off-by: Dave Young 
> > 
> > Looks OK to me.
> > 
> > Reviewed-by: Sergey Senozhatsky 

Same here:

Reviewed-by: Petr Mladek 

> > I agree with Steven, would be better to move the whole thing
> > to lib/dump_stack.c
> 
> If nobody has plan I can put it in my todo list, probably do it next
> week.

Sounds good.

JFYI, I would target these changes for 4.17. There was some discussion
about where and how to show the new information. IMHO, it would deserve
some gurgling in linux-next.

Best Regards,
Petr

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

Re: [PATCH V2] print kdump kernel loaded status in stack dump

[Dave Young]

On 01/30/18 at 05:50pm, Sergey Senozhatsky wrote:
> On (01/27/18 12:11), Dave Young wrote:
> > It is useful to print kdump kernel loaded status in dump_stack() 
> > especially when panic happens so that we can differenciate 
> > kdump kernel early hang and a normal panic in a bug report.
> > 
> > Signed-off-by: Dave Young 
> 
> Looks OK to me.
> 
> Reviewed-by: Sergey Senozhatsky 
> 

Thank you Sergey

> 
> I agree with Steven, would be better to move the whole thing
> to lib/dump_stack.c

If nobody has plan I can put it in my todo list, probably do it next
week.

> 
>   -ss

Thanks
Dave

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

Re: [PATCH V2] print kdump kernel loaded status in stack dump

[Sergey Senozhatsky]

On (01/27/18 12:11), Dave Young wrote:
> It is useful to print kdump kernel loaded status in dump_stack() 
> especially when panic happens so that we can differenciate 
> kdump kernel early hang and a normal panic in a bug report.
> 
> Signed-off-by: Dave Young 

Looks OK to me.

Reviewed-by: Sergey Senozhatsky 


I agree with Steven, would be better to move the whole thing
to lib/dump_stack.c

-ss

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