Bug 193873
| Summary: | [PATCH] Unify dumpsys() under generic kern_dump.c. | ||
|---|---|---|---|
| Product: | Base System | Reporter: | Conrad Meyer <cse.cem> |
| Component: | kern | Assignee: | Mark Johnston <markj> |
| Status: | Closed FIXED | ||
| Severity: | Affects Only Me | CC: | Andrew, bdrewery, benno, emaste, jhibbits, markj |
| Priority: | --- | ||
| Version: | CURRENT | ||
| Hardware: | Any | ||
| OS: | Any | ||
| Attachments: | |||
Instead of wrapping with DUMPSYS_HAS_*, why not add wrapper functions in the md headers, most of which simply call a common routine, with the oddballs doing their own thing? (The other issue brought up on IRC is that there is no reason to initialize both minidumps and full dump headers on PPC (moea{,64}_scan_init), and that allows us to drop the MD static vdump_map.)
I will fix both of these things, the change should be small. I don't think I can get to it today, but sometime this week.
(In reply to Justin Hibbits from comment #1) > Instead of wrapping with DUMPSYS_HAS_*, why not add wrapper functions in the > md headers, most of which simply call a common routine, with the oddballs > doing their own thing? Is this something like what you had in mind? http://dpaste.com/0QRFFAS It's a rough sketch and I haven't tried to compile it. Just want to make sure it seems reasonable to you before going down that path. Thanks! (In reply to Conrad Meyer from comment #3) > (In reply to Justin Hibbits from comment #1) > > Instead of wrapping with DUMPSYS_HAS_*, why not add wrapper functions in the > > md headers, most of which simply call a common routine, with the oddballs > > doing their own thing? > > Is this something like what you had in mind? http://dpaste.com/0QRFFAS > > It's a rough sketch and I haven't tried to compile it. Just want to make > sure it seems reasonable to you before going down that path. Thanks! Looks reasonable to me. Thanks for doing this work! Created attachment 147868 [details]
v2 of the patch w/ feedback incorporated
Updated to incorporate feedback. Buildtested against the same architectures. Diff is against r272336.
Created attachment 147869 [details]
(Just the changes between v1 and v2.)
This isn't the whole patch, just the changes made after v1 per feedback.
Created attachment 148257 [details]
v2 of the patch rebased on top of r273035 (past conflicting r272766)
Re: Mark's comments from phab: > Could you not just these define these to do nothing on powerpc and sparc? Yep. That removes the header #ifdef. > Is there any reason this stuff couldn't go into sys/kerneldump.h? I just followed the existing pattern, where doadump() etc already lived in conf.h. I'm not attached to the particular header. kerneldump.h is fine with me. You will need to take into account r273284. On ARM there is now a new section to allow libkvm to translate from va to pa that needs to be added to the dump. (In reply to Andrew Turner from comment #10) > You will need to take into account r273284. On ARM there is now a new > section to allow libkvm to translate from va to pa that needs to be added to > the dump. Updating for this now. I think the generic facility here is that some architectures may provide a routine to write out auxiliary elf headers. Created attachment 149303 [details]
v3 of patch. Addresses Andrew's ARM change and feedback from Mark in Differential.
Andrew's ARM dump change (r273284) adds an extra ELF phdr; I've added a generic facility for archs to dump a static number of extra headers, with the ARM stuff as an example.
Several clean-ups and changes brought up in CR feedback from Mark. In particular:
* There are several fewer #ifdefs in the generic routines and headers, and a few more stubs in the MD headers.
* This includes 'do_minidump' sysctl moving back to MD code.
* 'md_pa' has been renamed to 'dump_pa' because it is no longer MD.
* Returns in some MD stubs have been fixed to follow style(9).
* sparc64's dumpsys' kerenldumpheader is back in bss to try and avoid overflowing the kernel stack during dump.
* Several dump-internal routines have been relocated from sys/conf.h to sys/kerneldump.h.
A commit references this bug: Author: markj Date: Wed Jan 7 01:01:42 UTC 2015 New revision: 276772 URL: https://svnweb.freebsd.org/changeset/base/276772 Log: Factor out duplicated code from dumpsys() on each architecture into generic code in sys/kern/kern_dump.c. Most dumpsys() implementations are nearly identical and simply redefine a number of constants and helper subroutines; a generic implementation will make it easier to implement features around kernel core dumps. This change does not alter any minidump code and should have no functional impact. PR: 193873 Differential Revision: https://reviews.freebsd.org/D904 Submitted by: Conrad Meyer <conrad.meyer@isilon.com> Reviewed by: jhibbits (earlier version) Sponsored by: EMC / Isilon Storage Division Changes: head/sys/amd64/include/dump.h head/sys/arm/arm/dump_machdep.c head/sys/arm/include/dump.h head/sys/conf/files head/sys/i386/include/dump.h head/sys/kern/kern_dump.c head/sys/kern/kern_shutdown.c head/sys/mips/include/dump.h head/sys/mips/include/md_var.h head/sys/mips/mips/dump_machdep.c head/sys/pc98/include/dump.h head/sys/powerpc/aim/mmu_oea.c head/sys/powerpc/aim/mmu_oea64.c head/sys/powerpc/booke/pmap.c head/sys/powerpc/include/dump.h head/sys/powerpc/include/pmap.h head/sys/powerpc/powerpc/dump_machdep.c head/sys/powerpc/powerpc/mmu_if.m head/sys/powerpc/powerpc/pmap_dispatch.c head/sys/sparc64/include/dump.h head/sys/sparc64/sparc64/dump_machdep.c head/sys/sys/conf.h head/sys/sys/kerneldump.h head/sys/x86/include/dump.h head/sys/x86/x86/dump_machdep.c |
Created attachment 147599 [details] (Applies with patch -p1 to CURRENT as of r272027) (To varying degrees by arch.) x86, ARM, and MIPS are all relatively similar and straightforward. Some MD-specific methods are left in dump_machdep.c in each arch to provide mach-dependent implementations. (Map a region temporarily for dumping, unmap a region, iterate physical memory segments, flush WB caches.) Sparc and PowerPC are weirder. PowerPC had a merged dump/minidump path that used a different md_pa structure, pmap_md, plumbed through its MMU interface. So, that was ripped out and replaced with the standard path. Sparc uses its own non-ELF dumping header and that makes its dumpsys different enough that unification wasn't an improvement. However, some logic shared with other archs (blk_dump == cb_dumpdata) was refactored away. Patch build-tested against: - ARMv6 / CHROMEBOOK - AMD64 / GENERIC - i386 / GENERIC - MIPS / WZR-300HP - MIPS64 / SWARM64_SMP - PPC / MPC85XX (cpu=booke) - PPC / GENERIC (cpu=aim) - PPC64 / GENERIC64 (cpu=aim64) - Sparc64 / GENERIC Sponsored by: EMC / Isilon storage division Notes: * This patch is the first step towards some other dump/minidump improvements. I didn't want to apply them independently to 7-8 different architectures' forks of dumpsys. * Patch applies cleanly against a git tree; some hunks include SVN $FreeBSD$ macros which may require some coddling to apply to an SVN tree. * powerpc/dump_machdep.c is entirely deleted (something patch(1) doesn't really do well) * Net 875 lines removed: 21 files changed, 820 insertions(+), 1697 deletions(-) sys/amd64/include/md_var.h | 5 + sys/arm/arm/dump_machdep.c | 325 +---------------------------- sys/arm/include/md_var.h | 7 + sys/conf/files | 1 + sys/conf/files.powerpc | 1 - sys/i386/include/md_var.h | 5 + sys/kern/kern_dump.c | 402 ++++++++++++++++++++++++++++++++++++ sys/mips/include/md_var.h | 8 + sys/mips/mips/dump_machdep.c | 318 +--------------------------- sys/powerpc/aim/mmu_oea.c | 167 +++++++-------- sys/powerpc/aim/mmu_oea64.c | 164 +++++++-------- sys/powerpc/booke/pmap.c | 228 ++++++++++---------- sys/powerpc/include/md_var.h | 10 + sys/powerpc/include/pmap.h | 13 +- sys/powerpc/powerpc/dump_machdep.c | 315 ---------------------------- sys/powerpc/powerpc/mmu_if.m | 41 ++-- sys/powerpc/powerpc/pmap_dispatch.c | 28 ++- sys/sparc64/include/md_var.h | 5 + sys/sparc64/sparc64/dump_machdep.c | 115 +++-------- sys/sys/conf.h | 12 ++ sys/x86/x86/dump_machdep.c | 347 +------------------------------