In D33947#786095, @markj wrote:

vm_phys_alloc_seg_contig() will ignore runs of pages that aren't contained in the range [seg->start, seg->end], so doesn't this change potentially cause us to skip over eligible runs?

Peter, can you please test this?

I do, however, think that this deserves explanation in the form of a comment.

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

Hmm. At first this didn't make sense to me. The "assertions" were written this way so that systm.h wouldn't have to be included, but the prototype for panic() is provided by systm.h. So, this code might as well have used KASSERT all along.

I'm quite surprised to see this feature supported by the Graviton 2. I would only have expected to see this feature on micro-architectures that implement SMT/hyperthreading. In fact, the last paragraph of https://developer.arm.com/documentation/101811/0101/Address-spaces-in-AArch64 essentially says so. I wonder if it's simply a NOP on Graviton 2.

I'll take a look at this tomorrow.

Here is the start of pmap_qenter(). Note the right shift followed by left shift:

As an aside, I just verified that arm64 is already setting the equivalent bits.

@andrew didn't you have a patch for implementing CnP support? If so, could you please post it?

Add comments at the head of pmap_invalidate_{all,page,range}(). In particular, document pmap_invalidate_all() as always invalidating all intermediate-level entries.

Implement Mark's suggested optimization plus a related optimization to handling a singleton valid range as the last mapping in the PTP. Please check the logic carefully.

In D33705#761849, @kib wrote:

Does arm64 use recursive mapping?

In D33597#759863, @kib wrote:

In D33597#759857, @alc wrote:

Should we introduce analogous functions on amd64 and riscv?

For amd64, which function would it replace? We typically use the step-walk style functions, like pmap_pdpde_to_pde(), pmap_pde_to_pte(), in amd64 pmap, which both makes explicit level argument not useful (and IMO even quite inconvenient to use), and minimizes full-walk functions use (pmap_pte() as is is used twice) . Also we access resulting paging element right away, which means that we panic on NULL accesses without explicit asserts.

Should we introduce analogous functions on amd64 and riscv?

Update comment. Add level KASSERT.

As an aside, while reviewing all of the callers to pmap_pte(), I noticed that pmap_is_prefaultable() is incorrectly implemented. It is going to return FALSE when it should return TRUE, and occasionally the opposite.

See https://reviews.freebsd.org/D33597 for the pmap_pte_exists() prototype.

In D33509#757401, @kib wrote:

May be, it makes sense to add something like pmap_pte_exists() function that asserts that L3 page exists, and use it instead of repeated KASSERT().

I'm confused. I don't see how this change makes any functional difference. The original code set *level == 0 and returned NULL when the L0 entry was not L0_TABLE, and only under those circumstances.

In the commit message, I would somehow explain that the consequence of the bug was only that the wrong reservations might be broken, not that unaligned or inappropriately boundary crossing memory would be returned by contigmalloc(), et al.

I think that's all I have to say.

Individually, the updated comments make perfect sense from a purely localized standpoint, that is, they explain what is happening in that place. However, if I put myself in the place of a first-time reader of this code, the question that I would ask that isn't explained by either the old or updated comments is why any data is paged in at shutdown time.