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

[Nicholas Piggin <npiggin@gmail.com>]

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?

>       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?

>  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.

Thanks,
Nick