Move the arm64 DMAP creation to C

[andrew]

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.

[markj]

sys/arm64/arm64/pmap.c

873

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

881

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

[andrew]

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

[markj]

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

810

va doesn't need to be a pointer.

811

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

[alc]

sys/arm64/arm64/pmap.c

830

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.

837

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

852

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

[andrew]

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

[andrew]

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

[andrew]

Remove a rename of freemempos used while testing

[markj]

Sigh, I'm sorry. Wrong review link.

[markj]

The last revision looks ok to me.

[alc]

sys/arm64/arm64/pmap.c

830

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

831–832

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

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.

[andrew]

• 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

[kevans]

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.

[kevans]

sys/arm64/arm64/pmap.c

877

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>}

[kevans]

sys/arm64/arm64/pmap.c

877

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}

[alc]

sys/arm64/arm64/pmap.c

913

Kyle, try changing this line as above.

[kevans]

sys/arm64/arm64/pmap.c

913

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

[alc]

sys/arm64/arm64/pmap.c

918

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.

919

Shouldn't this be >=?

923

Shouldn't this be >=?

[andrew]

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]

[andrew]

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

[kevans]

In D34568#784479, @andrew wrote:

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]

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

[alc]

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

[andrew]

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?