Re: [PATCH 00/18] Add FADump support on PowerNV platform

[Hari Bathini <hbathini@linux.ibm.com>]

Hi Nick,


On 27/02/19 9:48 AM, Nicholas Piggin wrote:
> Hari Bathini's on February 22, 2019 3:35 am:
>> Firmware-Assisted Dump (FADump) is currently supported only on pseries
>> platform. This patch series adds support for powernv platform too.
>>
>> The first and third patches refactor the FADump code to make use of common
>> code across multiple platforms. The fifth patch adds basic FADump support
>> for powernv platform. Patches seven & eight honour reserved-ranges DT node
>> while reserving/releasing memory used by FADump. The next patch processes
>> CPU state data provided by firmware to create and append core notes to the
>> ELF core file. The tenth patch adds support for preserving crash data for
>> subsequent boots (useful in cases like petitboot). Patch twelve provides
>> support to export opalcore. This is to make debugging of failures in OPAL
>> code easier. The subsequent patch ensures vmcore processing is skipped
>> when only OPAL core is exported by f/w. The next patch provides option to
>> release the kernel memory used to export opalcore. Patch seventeen adds
>> backup area (an area populated before crash and used in the capture kernel
>> to setup vmcore file robustly) support on PowerNV platform. The remaining
>> patches update Firmware-Assisted Dump documentation appropriately.
>>
>> Note that the quantam of increase in robustness due to patch seventeen may
>> not be worth breaking backward compatibility for older kernel versions.
>> Would like to hear thoughts from others on it.
>>
>> The patch series is tested with the latest firmware plus the below skiboot
>> changes for MPIPL support:
>>
>>      https://patchwork.ozlabs.org/project/skiboot/list/?series=78497
>>      ("MPIPL support")
>>
>> ---
>>
>> Hari Bathini (18):
>>        powerpc/fadump: move internal fadump code to a new file
>>        powerpc/fadump: Improve fadump documentation
>>        pseries/fadump: move out platform specific support from generic code
>>        powerpc/fadump: use FADump instead of fadump for how it is pronounced
>>        powerpc/fadump: enable fadump support on OPAL based POWER platform
>>        powerpc/fadump: Update documentation about OPAL platform support
>>        powerpc/fadump: consider reserved ranges while reserving memory
>>        powerpc/fadump: consider reserved ranges while releasing memory
>>        powernv/fadump: process architected register state data provided by firmware
>>        powernv/fadump: add support to preserve crash data on FADUMP disabled kernel
>>        powerpc/fadump: update documentation about CONFIG_PRESERVE_FA_DUMP
>>        powerpc/powernv: export /proc/opalcore for analysing opal crashes
>>        powernv/fadump: Skip processing /proc/vmcore when only OPAL core exists
>>        powernv/opalcore: provide an option to invalidate /proc/opalcore file
>>        powernv/fadump: consider f/w load area
>>        powernv/fadump: update documentation about option to release opalcore
>>        powernv/fadump: use backup area to map PIR to logical CPUs
> The need to map firmware identifiers like PIR to Linux numbering comes
> up in a few places, OPAL msglog, pdbg debugger, etc. I wonder if we
> could have Linux register its logical CPU numbers with OPAL after it
> boots. Would that help with your usage?

The logical to PIR map of crashing kernel is needed in the capture kernel
(the kernel booted after crash to save the dump) that processes the register
data provided by f/w. Not sure if the logical to PIR map would be guaranteed
to be the same for both the crashing kernel and capture kernel.

Actually, I don't see any value-add in using the logical to PIR map in processing
the register data provided by f/w. pSeries isn't doing that and has been reliable.
Intention was to get inputs from others on whether it is worth it..

>>        powerpc/fadump: Update documentation about backup area support
>>
>>
>>   Documentation/powerpc/firmware-assisted-dump.txt |  208 ++--
>>   arch/powerpc/Kconfig                             |   23
>>   arch/powerpc/include/asm/fadump.h                |  190 ---
>>   arch/powerpc/include/asm/opal-api.h              |   58 +
>>   arch/powerpc/include/asm/opal.h                  |    1
>>   arch/powerpc/kernel/Makefile                     |    6
>>   arch/powerpc/kernel/fadump.c                     | 1199 ++++++++--------------
>>   arch/powerpc/kernel/fadump_internal.c            |  297 +++++
>>   arch/powerpc/kernel/fadump_internal.h            |  250 +++++
> I don't have much knowledge of fadump code, so I'll nitpick instead :P
>
> Why are you calling it fadump_internal, what's internal about it? You
> have the framework for the ops table etc here, which makes the platform
> code have to #include "../kernel/fadump_internal.h", and suggests it's
> not so internal. Seems like it would be fine just to go in
> include/asm/fadump.h and kernel fadump.c?

Intention was to use that file to put common code used by platform specific code
on both pSeries & PowerNV. How about fadump_common instead of fadump_internal
to put that in perspective?

>
>>   arch/powerpc/kernel/prom.c                       |    4
>>   arch/powerpc/platforms/powernv/Makefile          |    3
>>   arch/powerpc/platforms/powernv/opal-core.c       |  602 +++++++++++
>>   arch/powerpc/platforms/powernv/opal-fadump.c     |  670 ++++++++++++
>>   arch/powerpc/platforms/powernv/opal-fadump.h     |  116 ++
>>   arch/powerpc/platforms/powernv/opal-wrappers.S   |    1
>>   arch/powerpc/platforms/pseries/Makefile          |    1
>>   arch/powerpc/platforms/pseries/pseries_fadump.c  |  535 ++++++++++
>>   arch/powerpc/platforms/pseries/pseries_fadump.h  |   96 ++
> These hopefully could just be called pseries/fadump.[ch]? Or maybe
> rtas-fadump if you want to be like powernv.

I was trying to mimic the naming convention of other files in
dir arch/powerpc/platforms/pseries. Fine with rtas-fadump as well..

Thanks
Hari