Move vm_page_dump bitset array definition to MI code

[scottph]

Summary

These definitions were repeated by all architectures, with small
variations. Consolidate the common definitons in machine
independent code and use bitset(9) macros for manipulation. Many
opportunities for deduplication remain in the machine dependent
minidump logic. The only intended functional change is increasing
the bit index type to vm_pindex_t, allowing the indexing of pages
with address of 8 TiB and greater.

MFC after: 1 week
Sponsored by: Ampere Computing, Inc.

[scottph]

sys/vm/vm_page.h

608 ↗

(On Diff #76017)

BIT_FFS_AT introduced in D26128

[kib]

sys/amd64/amd64/minidump_machdep.c

223 ↗

(On Diff #76017)

Might be it is time to change bit type to long ?

sys/vm/vm_page.c

593 ↗

(On Diff #76017)

So this was not done on 32 bit power before. Would it ever link ?

sys/vm/vm_page.h

607 ↗

(On Diff #76017)

Assuming bit is int, would't this calculation overflow for physical addresses > 4G/PAGE_SIZE ?

[markj]

sys/vm/vm_page.c

593 ↗

(On Diff #76017)

IIRC 32-bit powerpc does not implement minidumps, so we should not drop the ifdef. To get rid of the ugly ifdefs I would add a vmparam.h constant for each platform to define whether the vm_page_dump array should be allocated.

[markj]

Overall this looks good to me. I agree that we should try to move towards a generic minidump implementation, as we have for full ELF dumps in kern_dump.c. One snag I pointed out is that we have exactly one platform that doesn't implement minidumps, so we should take care to make vm_page_dump[] allocation conditional on a vmparam.h variable. It could make new platform bringup a bit simpler as well.

sys/arm/arm/minidump_machdep.c

292 ↗

(On Diff #76017)

The indentation of the continuing line was correct before.

sys/vm/vm_page.h

74 ↗

(On Diff #76017)

Hmm, it's a bug (or at least surprising) that sys/bitset.h doesn't include _bitset.h.

sys/* includes should come before vm/* includes.

605 ↗

(On Diff #76017)

This could be added to .clang-format in the root of the repo.

606 ↗

(On Diff #76017)

Is there any reason bit can't be defined in the loop header? None of the loop bodies seem to use it.

[scottph]

sys/amd64/amd64/minidump_machdep.c

223 ↗

(On Diff #76017)

INT_MAX bits for pages can cover almost 8 TiB of memory, so I think int is still working with existing hardware. I could do it now if you'd like or we can wait a year or two probably.

sys/vm/vm_page.c

622 ↗

(On Diff #76050)

The comment below make it seem like this next conditional could be replaced with: if (PMAP_HAS_DMAP), but that would change the behavior on 32 bit mips. Maybe it would still be correct?

593 ↗

(On Diff #76017)

ah right, it was totally busted. I'm now introducing VM_PAGE_DUMP_ALLOC in vmparam.h to control this.

sys/vm/vm_page.h

606 ↗

(On Diff #76017)

I didn't see declarations in the first clause of for in other vm code, but I'm happy to add it in.

607 ↗

(On Diff #76017)

Yes you're right, I had this corrected in another patch but had left it wrong here. I double checked and also noticed that this patch was removing the round_page() when setting mdhdr.bitmapsize which wasn't intended and now is corrected.

[kib]

sys/amd64/amd64/minidump_machdep.c

223 ↗

(On Diff #76017)

Indeed INT_MAX covers 8T, and this is the current limit for DMAP on amd64.

I see a value in fixing places as we see them.

sys/vm/vm_page.c

622 ↗

(On Diff #76050)

32bit mips has something that can be considered as DMAP AFAIR. More, that mapping does not even require TLB fills. I have no idea why it is not provided as DMAP, but I do not know mips pmap to claim anything.

[markj]

sys/amd64/include/vmparam.h

253 ↗

(On Diff #76050)

Nit-picking a bit, but I'd prefer a more descriptive name like MINIDUMP_PAGE_TRACKING.

sys/vm/vm_page.h

607 ↗

(On Diff #76017)

Isn't there still a a problem in that BIT_FFS will truncate the result for PAs larger above 8TB?

[scottph]

sys/amd64/amd64/minidump_machdep.c

223 ↗

(On Diff #76017)

Sounds good, I've updated the FOREACH macro to use vm_pindex_t for the bit type.

sys/vm/vm_page.c

622 ↗

(On Diff #76050)

I suppose I'll just leave it alone for the time being.

sys/vm/vm_page.h

607 ↗

(On Diff #76017)

Ah, quite right.

[scottph]

sys/sys/bitset.h

215 ↗

(On Diff #76183)

Notice new change to bit size in BIT_FFS macros.

[scottph]

Adding bitset.h to vm_page.h breaks the vhci driver. D26177 fixes that.

[markj]

This looks reasonable to me. I would suggest committing the bitset.h changes separately.

sys/sys/bitset.h

215 ↗

(On Diff #76183)

Hmm, does this not trigger any warnings for existing users? Maybe the compiler can determine that no integer truncation is happening for some existing users where the set size is known at compile-time.

I think BIT_COUNT() should probably also return a size_t.