Details

Reviewers

kib
markj
manu

Commits

rS355883: When pmap_enter_{l2,pde}() are called to create a kernel mapping, they are

Summary

When pmap_enter_{l2,pde} are called to create a kernel mapping, they are incrementing (and decrementing) the ref_count on kernel page table pages. They should not do this. Kernel page table pages are expected to have a fixed ref_count. Address this problem by refactoring the interface between pmap_alloc{_l2,pde}() and its callers to reduce duplicated code and handle kernel page table pages.

Correctly implement PMAP_ENTER_NOREPLACE in pmap_enter_{l2,pde} on kernel mappings.

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().

Reduce code duplication by defining a function, pmap_abort_ptp(), for handling a common error case.

Simplify error handling in pmap_enter_quick_locked().

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

alc created this revision.Dec 11 2019, 4:44 PM

Herald added a reviewer: manu. · View Herald TranscriptDec 11 2019, 4:44 PM

Herald added subscribers: emaste, andrew. · View Herald Transcript

markj accepted this revision.Dec 12 2019, 3:48 PM

markj added inline comments.

sys/amd64/amd64/pmap.c
6380	Perhaps assert `SLIST_EMPTY(&free)`?
sys/arm64/arm64/pmap.c
3588	PAGE_SIZE / sizeof(*pte) is spelled Ln_ENTRIES in most places.

This revision is now accepted and ready to land.Dec 12 2019, 3:48 PM

kib added inline comments.Dec 12 2019, 10:55 PM

sys/amd64/amd64/pmap.c
6316	So you are inlining pmap_allocpde() there. Would it make less code duplication to check for kernel_pmap in pmap_allocpde() instead ?

Use Ln_ENTRIES on arm64.

Assert that the "free" list of page table pages is empty when destroying kernel mappings.

This revision now requires review to proceed.Dec 13 2019, 7:21 PM

alc marked 2 inline comments as done.Dec 13 2019, 9:30 PM

alc added inline comments.

sys/amd64/amd64/pmap.c
6316	This will require other changes as well. Specifically, pmap_allocpde() will need to calculate and return the PDE pointer as well as the page table page pointer, because the page table page pointer will be NULL in the case of kernel mappings. On arm64, this approach will eliminate a micro-optimization that I was able to introduce, specifically, avoiding a repeated page table walk when calling pmap_remove_l2(). The same optimization could be applied on amd64, if we moved the pmap_pdpe() call out of pmap_remove_pde(). That said, it would be even better to pass the pdpg to pmap_remove_pde(). I'll see what I can do.

kib accepted this revision.Dec 14 2019, 8:00 PM

This revision is now accepted and ready to land.Dec 14 2019, 8:00 PM

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.

This revision now requires review to proceed.Dec 14 2019, 10:45 PM

alc edited the summary of this revision. (Show Details)Dec 14 2019, 10:46 PM

alc edited the summary of this revision. (Show Details)Dec 14 2019, 10:49 PM

Thank you.

This revision is now accepted and ready to land.Dec 17 2019, 8:08 PM

Closed by commit rS355883: When pmap_enter_{l2,pde}() are called to create a kernel mapping, they are (authored by alc). · Explain WhyDec 18 2019, 6:21 PM

This revision was automatically updated to reflect the committed changes.

alc added a commit: rS355883: When pmap_enter_{l2,pde}() are called to create a kernel mapping, they are.

Herald added a subscriber: imp. · View Herald TranscriptDec 18 2019, 6:21 PM