minidumps: Always use 64-bit physical addresses for dump_avail[]
ClosedPublic
Actions

Authored by markj on Nov 3 2020, 8:57 PM.

Details

Reviewers

kib
scottph
alc

Commits

rS368307: Always use 64-bit physical addresses for dump_avail[] in minidumps

Summary

We recently started including dump_avail[] in minidumps. This is an
array of vm_paddr_t ranges. libkvm walks the array assuming that
sizeof(vm_paddr_t) is equal to the platform "word size", but that's not
correct on a couple of platforms, namely i386 and mips (sometimes).

Fix the problem by always dumping 64-bit addresses. On platforms where
vm_paddr_t is 32 bits wide, namely arm and mips (sometimes), translate
dump_avail[] to an array of uint64_t ranges. With this change, "word
size" is unused, so we can simplify libkvm ever so slightly.

This is a no-op on 64-bit platforms. I noticed it when trying to open
an i386 minidump.

Test Plan

This lets me open i386 minidumps. Before kgdb would crash with an assertion failure.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markj created this revision.Nov 3 2020, 8:57 PM

Herald added subscribers: emaste, andrew. · View Herald TranscriptNov 3 2020, 8:57 PM

Harbormaster completed remote builds in B34611: Diff 79148.Nov 3 2020, 8:57 PM

markj requested review of this revision.Nov 3 2020, 8:57 PM

markj edited the test plan for this revision. (Show Details)Nov 3 2020, 8:58 PM

markj added reviewers: kib, scottph, alc.

kib added inline comments.Nov 3 2020, 9:12 PM

sys/mips/mips/minidump_machdep.c
260 ↗	(On Diff #79148)	But then why not use uint64_t for dump_avail[] itself, on all arches ? For i386 it is required due to PAE, and for ppc I suspect we might have grow support for similar feature recently. So this leaves out arm and 32bit mips but it should be fine for them.

markj added inline comments.Nov 3 2020, 9:35 PM

sys/mips/mips/minidump_machdep.c
260 ↗	(On Diff #79148)	dump_avail[] is initialized from phys_avail[] so it feels a bit strange to use different types for them. I don't think there's any real problem with that though. OTOH, this hack is fairly localized and only required on somewhat "legacy" platforms. Note that the problem could be fixed entirely in libkvm, it just seemed preferable to avoid the extra complexity.

markj added inline comments.Nov 30 2020, 4:20 PM

sys/mips/mips/minidump_machdep.c
260 ↗	(On Diff #79148)	Do you still think that dump_avail[] should use uint64_t, rather than converting the array on the fly? If so I'll rework the patch.