In D23895#525337, @markj wrote:

In D23895#525319, @alc wrote:

The _NOFREE reminds me that we still need a way to segregate _NOFREE allocations in physical memory. Such segregation would most likely provide contiguity inherently.

The freepool approach requires some work in order to segregate KVA allocations, I believe, but I think that should be straightforward.

The _NOFREE reminds me that we still need a way to segregate _NOFREE allocations in physical memory. Such segregation would most likely provide contiguity inherently.

Is your overall strategy to incrementally replace PMAP_ASSERT_STAGE1(pmap)'s by correct handling of stage 2 PTE?

I also tested this patch with the following added changes:

diff --git a/sys/arm64/arm64/pmap.c b/sys/arm64/arm64/pmap.c
index 7f96431648d..ea394d12ee0 100644
--- a/sys/arm64/arm64/pmap.c
+++ b/sys/arm64/arm64/pmap.c
@@ -5983,8 +5983,8 @@ pmap_sync_icache(pmap_t pmap, vm_offset_t va, vm_size_t sz
)
        }
 }

Remove diagnostic printf() calls. This version is ready to commit.

Correct a mistakenly inverted condition.

Peter, can you please test this patch on i386?

Refactor the interface between pmap_alloc{_l2,pde}() and its callers to reduce duplicated code and handle kernel page table pages. The only downside to this update being that I had to abandon a micro-optimization in pmap_enter_l2() on arm64: When calling pmap_remove_l2(), I still have to repeat a pmap_l1() call that the previous version eliminated.

Handle a possible page table page leak in pmap_copy(). Suppose that we are determining whether to copy a superpage mapping. If we abort because there is already a mapping in the destination pmap at the current address, then simply decrementing the page table page's ref count is correct, because the page table page must have a ref count > 1. However, if we abort because we failed to allocate a PV entry, this might be a just allocated page table page that has a ref count = 1, so we should call pmap_abort_ptp().

Use Ln_ENTRIES on arm64.

On the whole, this is okay. Just ask Peter to test it.

In D22622#495862, @kib wrote:

In D22622#495703, @alc wrote:

In D22622#495323, @andrew wrote:

Which context synchronization event are you using after the dsb? ARMv8.5 adds the ARMv8.5-CSEH (Context synchronization and exception handling) extension that allows us to remove exceptions as a context synchronization event. It might pay to document where we know we are relying on exception entry/exit for these to help later if we decide to use the extension.

None really. Suppose that I am a thread that is preempted in the middle of a function F, which is one of the various "ic" or "tlbi" issuing functions. Further, suppose that I am then migrated to a new processor. If switching to a new thread on my old processor requires an address space change, then we are guaranteed that an "isb" instruction will be performed. In contrast, where the new thread and I share the same address space, we are not certain to perform an "isb" instruction as part of the context switch. However, I'm going to argue that this doesn't matter. Until I have completed F, and this new thread has synchronized with me in some way, it really isn't entitled to make any assumptions about the state in which I left the old processor. Simply having a context synchronization event as part of the context switch doesn't really achieve anything.

But isn't the problem that we modified page table entry on one processor, while issued isb on another ?

In D22622#495323, @andrew wrote:

Which context synchronization event are you using after the dsb? ARMv8.5 adds the ARMv8.5-CSEH (Context synchronization and exception handling) extension that allows us to remove exceptions as a context synchronization event. It might pay to document where we know we are relying on exception entry/exit for these to help later if we decide to use the extension.

In D22606#494706, @mmel wrote:

i just realized that this breaks EARLY_UART because SOCDEV_PA mapping is not passed to initarm().
So I think that we should postpone change of kernel ttbt0 until cninit() is processed.

@@ -1164,6 +1164,7 @@ initarm(struct arm64_bootparams *abp)
        valid = bus_probe();
        cninit();
+       set_ttbr0((uint64_t)pagetable_l0_ttbr0 - abp->kern_delta);
        if (!valid)
                panic("Invalid bus configuration: %s",

Also, summary is slightly misleading. This patch doesn't fully remove architectural undefined behavior (and I'm able to demonstrate this on RK3399) because it still exist - same physical memory is mapped in identity map as uncached and in kernel map as cached. This is direct violation of B2.9 in ARMv8 ARM, and it causes lost of coherency visible later in boot process.

Feel free to cross this task off from the list at https://wiki.freebsd.org/PmapReview :-)

In D22544#493871, @dougm wrote:

In D22544#493747, @alc wrote:

Can you explain why this change reduces the number of L1 data cache misses?

I haven't observed that. I didn't expect to impact L1 data cache misses.

Can you explain why this change reduces the number of L1 data cache misses?

In D22423#490985, @jeff wrote:

...

Maybe Alan has some thoughts. I feel like this set of patches is a welcome bandaid but I would've preferred to have done something more architectural to solve it better in the long run.

While I like the suggestion that we use a consistent variable naming schema, I felt like those style changes were overwhelming the functional changes. So, I asked Doug to separate the functional and stylistic changes.

In D22394#490621, @markj wrote:

In D22394#490201, @jeff wrote:

We run all of our paging threads constantly now. I would prefer not to disable creating cache buckets from pageproc unless we're in low memory situation. Even then it may be preferable to simply flush the buckets at the end of paging in that case.

The page daemon threads don't regularly allocate pages as far as I know. Maybe infrequently when unmapping a page will cause a superpage demotion and the leaf PTP is not cached in the pmap, and I believe that will only happen when the mapping was created with pmap_enter(psind==1).

Shouldn't this be closed?

While I think that this change is unquestionably correct, I would still ask Peter to test it.

The code is correct, but the summary is wrong.