Refactor dumpsys() to remove excessive code duplication

[markj]

Summary

PR 193873 / https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=193873

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(-)

[jhibbits]

Code looks fine to me, for PowerPC and generic (didn't look at the others). I'm a little hesitant at the #ifdef (arch) additions, since one reason for having different dump codes is to avoid those special cases. Since you said this is stage 1 of the overhaul, I'll let it go.

[markj]

sys/sys/conf.h

343

Is there any reason this stuff couldn't go into sys/kerneldump.h?

346

Could you not just these define these to do nothing on powerpc and sparc?

[emaste]

New version from PR

[cse_cem_gmail_com]

sys/sys/conf.h

343

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. How much of the other dump-related stuff should move? All of it? I'm going to skip this change for now. If we need to do a v4, ok.

346

Will fix.

[markj]

sys/amd64/include/dump.h

50

Parens around the return value.

sys/kern/kern_dump.c

54

I'd prefer to leave this in the MD code and add do_minidump to the MD headers. On sparc it can be hard-wired to 0.

242

It's preferable to use some indirection so that the code path is a bit more consistent across different arches. I'd like to have an MD DUMPSYS macro that calls dumpsys_generic() or so when that makes sense, and the MD dumpsys otherwise.

sys/mips/mips/dump_machdep.c

33–34

You should be able to get rid of most of these #includes.

sys/sparc64/sparc64/dump_machdep.c

91

This is a big struct (512 bytes, by design). Putting it on the stack increases the risk of stack overflow, which is not very nice when you're trying to dump core.

sys/sys/conf.h

339

md_pa doesn't doesn't really make sense as a name here - it's not MD.

[cse_cem_gmail_com]

sys/kern/kern_dump.c

54

Ok.

242

Sure. I've removed the macro and added inline wrappers to non-sparc64 MD headers, similar to the other generic / MD- routines. The main obstacle here was that the routine wouldn't compiled on sparc without a minidumpsys() symbol. So, I've added a dumb wrapper that just returns -ENOSYS to sparc64/include/dump.h.

sys/mips/mips/dump_machdep.c

33–34

Ok.

sys/sparc64/sparc64/dump_machdep.c

91

Ok. We can throw 'static' in front of it and keep it in the function.

sys/sys/conf.h

339

I left the existing name as-is. 'pa' by itself sucks. What would you suggest instead? It looks a lot like 'iovec' although that doesn't match intent here.

[cse_cem_gmail_com]

Let's try commandeering it? Does that take authorship?

[cse_cem_gmail_com]

(Copied from Bugzilla:)

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.

[emaste]

sys/arm/arm/dump_machdep.c

81–84

The name implies this is general functionality (arbitrary aux data) but the implementation is only the specific case of va/pa delta. If the intent is we'd keep the same function but extend it for possible future additional aux data we should reference that in the comment.

[cse_cem_gmail_com]

sys/arm/arm/dump_machdep.c

81–84

That is the intent. I can change this in a future version of the patch, if needed. I'm picturing something like: "Add additional ELF program headers for ARM; for now, this is just a header to be used by libkvm ..."

[emaste]

sys/amd64/include/dump.h

3–4

Having both EMC & a personal copyright seems odd - is there a reason for it?

sys/mips/include/md_var.h

83

you can remove this blank line change

sys/mips/mips/dump_machdep.c

55–56

?

[cse_cem_gmail_com]

sys/amd64/include/dump.h

3–4

Nope.

(It might be buried somewhere in irc history if you have logs going back weeks, but I don't.)

sys/mips/include/md_var.h

83

Sure.

sys/mips/mips/dump_machdep.c

55–56

As-is from the MIPS dump code. See line 192 in the old version of this file.

[markj]

One last round of nitpicks, sorry. I've re-pinged the person who was going to test on sparc too.

Most of the changes are mechanical, so I can do them myself if you like.

sys/amd64/include/dump.h

3–4

To be safe, perhaps it should just be copyright EMC? You can still include yourself as the author...

sys/i386/include/dump.h

35

It's seems a bit much to have an entirely separate header file just because a couple of constants are different. Could this be moved to x86, with #ifdef i386__/amd64__ as appropriate?

sys/kern/kern_dump.c

141

parens

287

This should be static or global. (I think you fixed this elsewhere already.)

sys/sys/kerneldump.h

104

md_ doesn't make sense as a name prefix here. I'd just change it to pa_.

120

Since the struct was renamed, these functions should be too. Either dump_pa or just pa.

[cse_cem_gmail_com]

Most of the changes are mechanical, so I can do them myself if you like.

Go ahead, thanks!

sys/amd64/include/dump.h

3–4

I'm not picky on the details. I think Benno suggested this. Remove me, change it to whatever you think it ought to be.

sys/i386/include/dump.h

35

Sure, it could be.

I've gotten negative feedback previously from having any— even small— MD #ifdefs, in the past. I am not particularly tied to a specific approach, though.

sys/kern/kern_dump.c

141

Good catch.

287

Yep, just make it static. (I moved the sparc64 one from a file-level static to a function static, so we should probably be consistent.)

sys/sys/kerneldump.h

104

+1

120

They're pretty wordy already, pa sounds fine to me.

[markj]

sys/amd64/include/dump.h

3–4

Ok.

sys/i386/include/dump.h

35

I think I complained about them appearing in sys/kern/* specifically. Moreover, the code currently in x86/dump_machdep.c already contains the exact #ifdef I have in mind.

[emaste]

What's the next step here? @markj, are you going to upload a new version with the minor changes you mentioned in an earlier comment?

[markj]

In D904#27, @emaste wrote:

What's the next step here? @markj, are you going to upload a new version with the minor changes you mentioned in an earlier comment?

Yup, I'm going to upload in the next day or two and commit the change if there aren't any further comments.

[markj]

Minor tweaks to address the last set of comments. No functional changes
intended.

[emaste]

I haven't had a chance to read through in as much depth as I'd like, but have no objection from a brief review.