Page MenuHomeFreeBSD

powerpc64: Fix disappearing low memory on radix MMU systems (POWER9)
ClosedPublic

Authored by tpearson_raptorengineering.com on Wed, Feb 4, 5:21 AM.
Referenced Files
F145232298: D55095.diff
Tue, Feb 17, 9:08 AM
Unknown Object (File)
Mon, Feb 16, 12:43 AM
Unknown Object (File)
Tue, Feb 10, 12:03 AM

Details

Reviewers
jhibbits
adrian
Group Reviewers
PowerPC
Summary

The FreeBSD radix implementation piggybacks on the physical memory
allocation function from the HPT implementation, but did not share
the same state information for number of physical memory ranges.
This led to a situation where the HPT physical memory allocator
would attempt to shift the physical memory ranges in the shared
range table, but would use the wrong number of entries, thus
overwriting the large segment of low memory that should have been
available for e.g. 32-bit DMA.

Incorrect physical memory map:

real memory = 33997058048 (32422 MB)
available KVA = 34359619583 (32767 MB)
Physical memory chunk(s):
0x0000000000003000 - 0x0000000000002fff, 0 bytes (0 pages)
0x000000000000e000 - 0x000000000000ffff, 8192 bytes (2 pages)
0x0000000000094000 - 0x0000000000ffffff, 16171008 bytes (3948 pages)
0x0000000100000000 - 0x00000007a2042fff, 28487987200 bytes (6955075 pages)
0x00000007d0006000 - 0x00000007fc72dfff, 745701376 bytes (182056 pages)
0x00000007fdc00000 - 0x00000007ff79ffff, 28966912 bytes (7072 pages)
0x00000007ff7d1000 - 0x00000007ff7effff, 126976 bytes (31 pages)
avail memory = 29190103040 (27837 MB)
FreeBSD/SMP: Multiprocessor System Detected: 16 CPUs

Correct physical memory map:

real memory = 33997058048 (32422 MB)
available KVA = 34359619583 (32767 MB)
Physical memory chunk(s):
0x0000000000003000 - 0x0000000000002fff, 0 bytes (0 pages)
0x000000000000e000 - 0x000000000000ffff, 8192 bytes (2 pages)
0x0000000000094000 - 0x0000000000ffffff, 16171008 bytes (3948 pages)
0x0000000002000000 - 0x000000000284ffff, 8716288 bytes (2128 pages)
0x0000000004627000 - 0x000000002fffffff, 731746304 bytes (178649 pages)
0x0000000034040000 - 0x00000000efffffff, 3153854464 bytes (769984 pages)
0x0000000100000000 - 0x00000007982ecfff, 28323008512 bytes (6914797 pages)
0x00000007cc20a000 - 0x00000007fc72dfff, 810696704 bytes (197924 pages)
0x00000007fdc00000 - 0x00000007ff79ffff, 28966912 bytes (7072 pages)
0x00000007ff7d1000 - 0x00000007ff7effff, 126976 bytes (31 pages)
avail memory = 32984436736 (31456 MB)
FreeBSD/SMP: Multiprocessor System Detected: 16 CPUs

Tested on Raptor Computing Systems Blackbird with 4-core POWER9 CPU and 32GB RAM

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

oh that's a good catch!

What about regions, pregions, numa_pregions, etc? Are there any other shared variables that need to be unified here?

I think this is fine; let's see what justin thinks tomorrow morning!

This revision is now accepted and ready to land.Wed, Feb 4, 6:23 AM

Interesting, so mmu_radix depends on mmu_oea64 for bootstrap allocations, which in turn depends on its own phys_avail_count. Can you just put the extern declaration in mmu_oea64.h instead of having the extern in mmu_radix.c? mmu_radix.c already includes mmu_oea64.h, so that looks like the best place to put these shared variable declarations.

oh that's a good catch!

What about regions, pregions, numa_pregions, etc? Are there any other shared variables that need to be unified here?

I checked; the only function we use from the HPT side of things is moea64_bootstrap_alloc(), therefore this is the only variable that needs to stay in sync. While I can't say I particularly like this brittle architecture, I suspect it's going to be changed so infrequently that it's not worth rewriting everything at this point.

Interesting, so mmu_radix depends on mmu_oea64 for bootstrap allocations, which in turn depends on its own phys_avail_count. Can you just put the extern declaration in mmu_oea64.h instead of having the extern in mmu_radix.c? mmu_radix.c already includes mmu_oea64.h, so that looks like the best place to put these shared variable declarations.

Sure. I'll update when I push a new version of the other two patches as well.

This revision now requires review to proceed.Thu, Feb 5, 5:12 AM

looks good, land when you're ready!

This revision is now accepted and ready to land.Thu, Feb 5, 5:23 AM

LGTM. Good find! Approved.

oh that's a good catch!

What about regions, pregions, numa_pregions, etc? Are there any other shared variables that need to be unified here?

I checked; the only function we use from the HPT side of things is moea64_bootstrap_alloc(), therefore this is the only variable that needs to stay in sync. While I can't say I particularly like this brittle architecture, I suspect it's going to be changed so infrequently that it's not worth rewriting everything at this point.

(This is just a drive-by comment as I was skimming the review out of curiosity.)

FYI there is a common interface for physical memory enumeration and management of globals (e.g. phys_avail[]) in kern/subr_physmem.c. It is used by the arm, arm64, and riscv architectures.

I agree that it is best not to poke at a potential mess like this one without good reason. If you were going to rewrite some of this code, I suggest keeping the above interface in mind.

Bye!