Further below we handle the case where a mapping already exists at the specified va. In that situation a leaf PTP may already be resident in the trie, for example if the existing mapping was created by a promotion. Then the new pmap_insert_pt_page() call will fail.

We should insert the leaf PTP further below, after any existing mapping has been dealt with.

I suspect that VM_ALLOC_INTERRUPT is inappropriate here, since we can gracefully handle allocation failures. _INTERRUPT should generally be used to minimize the risk of failure in paths where error handling isn't possible.

Local variables should be defined at the beginning of the function or block.

Just a note: this part shouldn't be committed until other pmaps which implement pmap_enter(psind = 1) (i386, arm64, riscv) have been similarly modified.

Of course, if this approach gets accepted I'll move it into a separate patch.

It is really strange to return an error in case of page table page allocation failure, but panic on trie insert.

thank you for pointing this out, I've reworked the condition and removed the panic.

If pmap_insert_pt_page() failed, wouldn't you leak the page table page?

Yes, that would cause a leak, I've reworked the code to deal with this.

pv entry must be removed if the ptpage below cannot be mounted. Perhaps PGA_WRITEABLE setting for the constituent pages should be postponed until we committed to return success.

Do you think it would be more appropriate to move this code block before PV entry creation? That way we only have worry about removing and freeing the newly allocated ptpage if things go awry.

There are two things you need to worry about allocation failure: pde pv entry and page table page. I do not see much difference if you change order of allocation, either way you need to handle both failures.

Actually, the error handling will be simpler if you move it.

Increase the scope of this variable, and initialize it to NULL.

This does not correctly handle the PAT bit, which is in a different bit position between PDEs and PTEs. That said, there is no reason to initialize the PTEs. Instead, pass false as the last argument to pmap_insert_pt_page(), and the page table page will be initialized on demotion, if demotion ever occurs. (The comment describing the last parameter to pmap_insert_pt_page() is specific to its current usage and can be updated later.)

This has three problems: It's removing from the 4KB PV list when it should be removing from the 2MB PV list. It should only attempt removal if the PDE has the managed bit set. It needs to conditionally perform pmap_abort_ptp().

On some occasions, the page table page that you are later going to allocate is being freed here. :-)

As a micro-optimization, because wired mappings are rare and user-space mapping are common, so test the condition to most likely to be false first.

We need to undo the pmap_insert_pt_page().

Please note that the surrounding code does not introduce blank lines except around blocks of code preceded by a multiline block comment. This is an idiosyncrasy of style(9) that admittedly isn't always adhered to.

The page is not guaranteed to be zero filled.

This should only be indented as far as the opening parentheses on the previous line.

This needs to be pmap_pde_pindex(va). pmap_pde_index() gives you the PDE index for the address, which despite the similar name is completely different. I'll fix this before pushing.