Page MenuHomeFreeBSD

Move the arm64 DMAP creation to C
ClosedPublic

Authored by andrew on Mar 15 2022, 5:24 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Mar 3, 2:02 AM
Unknown Object (File)
Sun, Mar 3, 2:02 AM
Unknown Object (File)
Sun, Mar 3, 2:02 AM
Unknown Object (File)
Sun, Mar 3, 2:02 AM
Unknown Object (File)
Sun, Mar 3, 2:02 AM
Unknown Object (File)
Sun, Mar 3, 2:02 AM
Unknown Object (File)
Jan 15 2024, 11:30 PM
Unknown Object (File)
Dec 23 2023, 7:19 PM

Details

Summary

To simplify the creation of the direct map (DMAP) region on arm64 move
it from the pre-C code into pmap. This simplifies the DMAP creation
as we can use the notmal index macros, and should reduce the number
of pages needed to hold the level 1 tables to just those needed.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

sys/arm64/arm64/pmap.c
1000–1005

Ln_ENTRIES seems like a more intuitive sentinel value, if only because these variables are unsigned.

1013

What if the physmap entry is larger than the region mappable by an L0 slot?

  • Check if we need a new L0 -> L1 map when va & pa change
  • Use Ln_ENTRIES as the invalid slot number

I can't see any bugs in the patch but now pmap_bootstrap_dmap() looks overly hairy to me. Can we at least make pmap_bootstrap_dmap_l2() responsible for creating L1 table entries, similar to how pmap_bootstrap_dmap_l1() is responsible for creating L0 table entries?

sys/arm64/arm64/pmap.c
811

va doesn't need to be a pointer.

811

pmap_bootstrap_dmap_l1() updates freemempos directly. It would be nice to be consistent.

sys/arm64/arm64/pmap.c
831

Rather than this bit-masking operation, I would argue for asserting that l1 (or *freemempos) is page aligned. If l1 isn't page aligned, then the problems will go beyond a bogus l0 table entry.

838

Can't this assignment be inside the above if statement?

952–953

Can't this assignment be inside the above if statement?

  • Use a state struct rather than pass individual pointers
  • Have pmap_bootstrap_dmap_l2 call pmap_bootstrap_dmap_l1

Remove physical address masks from the page table creation and add asserts for alignment

Remove a rename of freemempos used while testing

This revision was not accepted when it landed; it landed in state Needs Review.Mar 17 2022, 7:40 PM
This revision was automatically updated to reflect the committed changes.

Sigh, I'm sorry. Wrong review link.

The last revision looks ok to me.

This revision is now accepted and ready to land.Mar 17 2022, 7:52 PM
alc added inline comments.
sys/arm64/arm64/pmap.c
841

I would suggest doing the zeroing before hooking the page into the page table. Otherwise, when I see things done in this order, I pause and ponder if there might be a possible problem/race.

897–898

Please swap the order of these two lines for consistency with pmap_bootstrap_dmap_l1().

905–915

I would suggest doing the zeroing before hooking the page into the page table.

  • Reorder to zero memory before attaching it to the table
  • Rename functions to make them consistant with what they're creating
  • Reorder code to be consistant between similar functions
  • Create level 3 pages so we only create the DMAP mappings needed
This revision now requires review to proceed.Mar 18 2022, 6:50 PM
This revision is now accepted and ready to land.Mar 18 2022, 6:57 PM

This seems to break booting at least one of our Altra systems (never get back to a console after loader). I can throw a debugger on it if there's anything particularly of interest for me to probe at.

sys/arm64/arm64/pmap.c
959

We seem to be hitting this assertion:

Thread 1 hit Breakpoint 2, pmap_bootstrap_dmap_l2_block (state=state@entry=0xffff000000fa6740, i=i@entry=2) at /usr/home/kyle.evans/freebsd/sys/arm64/arm64/pmap.c:928
928             MPASS((state->va & L2_OFFSET) == 0);

(gdb) print *state
$1 = {va = 18446638546234179584, pa = 26714636288, l1 = 0xffff000005a24000, l2 = 0xffff000005a25000, l3 = 0xffff000005a26000, 
  l0_slot = 320, l1_slot = 23, l2_slot = 450, freemempos = 18446462598827372544}

(gdb) bt
#0  pmap_bootstrap_dmap_l2_block (state=state@entry=0xffff000000fa6740, i=i@entry=2)
    at /usr/home/kyle.evans/freebsd/sys/arm64/arm64/pmap.c:928
#1  0xffff000000776210 in pmap_bootstrap_dmap (freemempos=<optimized out>, kern_l1=<optimized out>, min_pa=<optimized out>)
    at /usr/home/kyle.evans/freebsd/sys/arm64/arm64/pmap.c:1010
#2  pmap_bootstrap (l0pt=<optimized out>, l1pt=<optimized out>, kernstart=<optimized out>, kernlen=<optimized out>)
    at /usr/home/kyle.evans/freebsd/sys/arm64/arm64/pmap.c:1158
#3  0xffff000000771c08 in initarm (abp=0xffff000000fa6a60) at /usr/home/kyle.evans/freebsd/sys/arm64/arm64/machdep.c:817
#4  0xffff0000000008b4 in _start () at /usr/home/kyle.evans/freebsd/sys/arm64/arm64/locore.S:152

(gdb) print physmap
$2 = {1073741824, 26714046464, 26714505216, 26714636288, 26715291648, 26716274688, 26716995584, 26772373504, 26774011904, 
  26843545600, 0 <repeats 116 times>}
sys/arm64/arm64/pmap.c
959

Ah, a bunch of intermediates were optimized out. Here's this for completeness:

(gdb) print *abp
$5 = {modulep = 18446462598827315200, kern_l1pt = 18446462598749208576, kern_delta = 18446462572182896640, 
  kern_stack = 18446462598749237248, kern_l0pt = 18446462598749212672, kern_ttbr0 = 26566332416, boot_el = 1, pad = 0}
sys/arm64/arm64/pmap.c
1016

Kyle, try changing this line as above.

sys/arm64/arm64/pmap.c
1016

Perfect- we're back to multi-user with this change. Thanks!

sys/arm64/arm64/pmap.c
1021

Previously, this line was required, but with the new pmap_bootstrap_dmap_l2_block() it isn't. Instead, I would suggest putting MPASS(state.pa <= physmap[i + 1]) between each of the cases, i.e., 4K, 2M, 1G, 2M, 4K.

1036–1041

Shouldn't this be >=?

1040

Shouldn't this be >=?

Attempt to fix when regions are smaller than a level 2 bloc ksize

  • Move the size checks to _l2_block and _l3_page functions
  • Add asserts for pa <= physmap[i + 1]
This revision now requires review to proceed.Mar 21 2022, 10:04 AM

I've booted the latest version on an Oracle VM with sub-2M regions in the physmap.

I've booted the latest version on an Oracle VM with sub-2M regions in the physmap.

Latest version boots on our Altra hardware, as well.

dch added a subscriber: dch.

LGTM, on arm64 eMAG + altra (vm in OCI). both boot & run happily.

This revision is now accepted and ready to land.Mar 24 2022, 11:04 PM

Andrew, I'll give this patch a final look over the weekend.

In D34568#785414, @alc wrote:

Andrew, I'll give this patch a final look over the weekend.

Did you get a chance for a final look?

This revision was automatically updated to reflect the committed changes.