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

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