Details

Reviewers

alc
markj
dougm

Commits

rS348476: Simplify flow of pmap_demote_pde_locked() and add more comprehensive

Summary

In particular,

Move the code to handle failure to allocate page table page into helper
After the previous item is done, it is possible to distinguish !PG_A case and case of missed page, in the control flow.
Make the variable to indicate that in-kernel mapping is demoted.
Assert that missed page table page can only happen for in-kernel mapping when demoting direct map.
If DIAGNOSTIC is enabled, and the page table page should be already filled, check all ptes instead of only first one.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 24404

Event Timeline

kib created this revision.May 15 2019, 8:54 PM

Herald added a subscriber: imp. · View Herald TranscriptMay 15 2019, 8:54 PM

Harbormaster completed remote builds in B24250: Diff 57420.May 15 2019, 8:54 PM

kib added a subscriber: pho.May 15 2019, 8:58 PM

dougm added inline comments.May 16 2019, 8:45 AM

sys/amd64/amd64/pmap.c
1406	If you're using rounded-up firstaddr for this loop, you should just move the loop to after the point where you've rounded up firstaddr in line 1045.

kib added inline comments.May 16 2019, 9:00 PM

sys/amd64/amd64/pmap.c
1406	This operation is boot-time only, so micro-optimizing it is not too important. The current sequence of initialization actually makes some sense from the hierarchical view on the page tables, and I want to keep it as is.

What are the errors that Peter reported?

sys/amd64/amd64/pmap.c
1759	The kernel page table pages that are dynamically allocated by pmap_growkernel() will have a wire count of 1.

In D20266#437675, @alc wrote:

What are the errors that Peter reported?

It started with assert *firstpte & PG_FRAME != newpte & PG_FRAME at old line 4179 for pipe_map. Then I added the comparision and dump of the whole page table page and saw some other cases.

alc added inline comments.May 22 2019, 4:53 PM

sys/amd64/amd64/pmap.c
4607–4612	When I wrote this block of code, the PG_PROMOTED flag did not yet exist. If, today, I were writing this block for the first time, I would test (oldpte & PG_PROMOTED) == 0 instead of mpte->wire_count == 1.
4618–4619	I speculate that this block would no longer be needed if we test (oldpte & PG_PROMOTED) == 0 above. In particular, pmap_protect_pde() clears PG_PROMOTED if it removes access rights from the PDE.

Also, I never intended for promotion and demotion on the kernel pmap to create an inconsistency in how we maintain the wire count on kernel page table pages. Switching to testing (oldpte & PG_PROMOTED) == 0 would allow for eliminating that inconsistency.

Index: amd64/amd64/pmap.c
===================================================================
--- amd64/amd64/pmap.c  (revision 348078)
+++ amd64/amd64/pmap.c  (working copy)
@@ -4537,8 +4537,10 @@ pmap_demote_pde_locked(pmap_t pmap, pd_entry_t *pd
                            " in pmap %p", va, pmap);
                        return (FALSE);
                }
-               if (va < VM_MAXUSER_ADDRESS)
+               if (va < VM_MAXUSER_ADDRESS) {
+                       mpte->wire_count = NPTEPG;
                        pmap_resident_count_inc(pmap, 1);
+               }
        }
        mptepa = VM_PAGE_TO_PHYS(mpte);
        firstpte = (pt_entry_t *)PHYS_TO_DMAP(mptepa);
@@ -4553,10 +4555,8 @@ pmap_demote_pde_locked(pmap_t pmap, pd_entry_t *pd
        /*
         * If the page table page is new, initialize it.
         */
-       if (mpte->wire_count == 1) {
-               mpte->wire_count = NPTEPG;
+       if ((oldpde & PG_PROMOTED) == 0)
                pmap_fill_ptp(firstpte, newpte);
-       }
        KASSERT((*firstpte & PG_FRAME) == (newpte & PG_FRAME),
            ("pmap_demote_pde: firstpte and newpte map different physical"
            " addresses"));

Use PG_PROMOTED == 0 as a predicate for the pmap_pt_fill() call.

Harbormaster completed remote builds in B24400: Diff 57709.May 22 2019, 6:24 PM

markj added inline comments.May 22 2019, 6:56 PM

sys/amd64/amd64/pmap.c
4556	I guess it's supposed to be MAXUSER_ADDRESS. Technically this is a functional change since before we were only allocating with interrupt priority for demotions in the direct map. I think it's fine, just contradicts the review description.

alc added inline comments.May 22 2019, 7:07 PM

sys/amd64/amd64/pmap.c
1406	Unless you want to set PG_PROMOTED on the PDEs below, this loop is pointless (and has been for quite some time). If we actually want to support demotion on the virtual address range being mapped here, we should be inserting these page table pages into the pmap's radix tree.

alc added inline comments.May 22 2019, 7:09 PM

sys/amd64/amd64/pmap.c
1406	... and, in fact, pmap_init() is doing the insertion.

alc added inline comments.May 22 2019, 7:15 PM

sys/amd64/amd64/pmap.c
1760	I'm not convinced that KERNend is the correct value to use here. I think that it is too small.

Fix VM_MAXUSER_ADDRESS spelling.
Remember the last initialized page table page for insertion into the pmap radix.

Harbormaster completed remote builds in B24404: Diff 57713.May 22 2019, 7:38 PM

alc added inline comments.May 22 2019, 7:50 PM

sys/amd64/amd64/pmap.c
1334	Before introducing a new variable, please take a look at how KERNend is initialized and used. I don't think that we need both KERNend and kern_promoted_end.

alc added inline comments.May 22 2019, 8:01 PM

sys/amd64/amd64/pmap.c
4556	The only situation in which this code should ever allocate a kernel page table page is when it is called upon to demote a direct map mapping, so this doesn't really represent a functional change.

kib added inline comments.May 22 2019, 8:01 PM

sys/amd64/amd64/pmap.c
1334	We probably can remove KERNend if going ahead with my change. I think you mean that KERNend is de-facto unused in the patched pmap, and kern_promoted_end can be int, i.e. 4 bytes shorter.

Remove KERNend.

Harbormaster completed remote builds in B24416: Diff 57739.May 22 2019, 11:38 PM

Can you separate the pmap_demote_pde_locked() changes from the create_pagetables()/pmap_init() changes, i.e., create a separate patch.

Leave only refactoring of the demotion in this review.

Harbormaster completed remote builds in B24477: Diff 57849.May 24 2019, 5:30 PM

kib retitled this revision from Fixes for some corner cases of amd64 demotions. to Simplify flow of pmap_demote_pde_locked() and add more comprehensive debugging checks..May 24 2019, 5:34 PM

kib edited the summary of this revision. (Show Details)

i386 has similar code.

sys/amd64/amd64/pmap.c
4561	_fail is a bit misleading here. Maybe _abort?

This revision is now accepted and ready to land.May 24 2019, 6:09 PM

alc added inline comments.May 24 2019, 8:25 PM

sys/amd64/amd64/pmap.c
4548	You've eliminated the only use of PG_G in this function.

alc added inline comments.May 24 2019, 8:31 PM

sys/amd64/amd64/pmap.c
4573–4585	The first part of this comment no longer belongs here.

kib marked 2 inline comments as done.May 24 2019, 10:11 PM

kib added inline comments.

sys/amd64/amd64/pmap.c
4548	It is used by PG_PTE_PROMOTED macro. I tried that in initial version.

Split large comment.
_fault() -> _abort().

This revision now requires review to proceed.May 24 2019, 10:12 PM

Harbormaster completed remote builds in B24490: Diff 57868.May 24 2019, 10:12 PM

markj accepted this revision.May 24 2019, 11:10 PM

This revision is now accepted and ready to land.May 24 2019, 11:10 PM

kib mentioned this in D20380: Fixes for some corner cases of amd64 demotions..May 25 2019, 10:36 PM

I ran tests on D20266.57868.diff for 24 hours without seeing any problems.

In D20266#440649, @pho wrote:

I ran tests on D20266.57868.diff for 24 hours without seeing any problems.

Thank you. It would require a combined test with D20380 after both settle.

alc added inline comments.May 27 2019, 6:17 AM

sys/amd64/amd64/pmap.c
4499	You could simply add PAGE_SIZE to newpte at the end of each iteration.
4503	The "XX" should be replaced.
4531–4532	You should be able to unwrap the format string.
4584	"is for a kernel address, ..."
4602	"abort" could reasonably be interpreted as leave the mapping in place. Instead, I would explicitly say, "invalidate the 2MB page mapping".

Handle Alan' comments. Adjust the output of the diagnostic version of the checker even more.

This revision now requires review to proceed.May 27 2019, 3:43 PM

Harbormaster completed remote builds in B24538: Diff 57957.May 27 2019, 3:43 PM

markj accepted this revision.May 27 2019, 6:41 PM

markj added inline comments.

sys/amd64/amd64/pmap.c
4658	This format string may be unwrapped as well.

This revision is now accepted and ready to land.May 27 2019, 6:41 PM

Unwrap one more CTR format string.

This revision now requires review to proceed.May 27 2019, 7:13 PM

Harbormaster completed remote builds in B24542: Diff 57963.May 27 2019, 7:13 PM

alc accepted this revision.May 31 2019, 4:31 PM

alc added inline comments.

sys/amd64/amd64/pmap.c
4501	I suggest: "different pages: found %#lx, expected %#lx\n"

This revision is now accepted and ready to land.May 31 2019, 4:31 PM

Closed by commit rS348476: Simplify flow of pmap_demote_pde_locked() and add more comprehensive (authored by kib). · Explain WhyMay 31 2019, 6:53 PM

This revision was automatically updated to reflect the committed changes.

Diff 57713

View Options

sys/amd64/amd64/pmap.c

This file is larger than 256 KB, so syntax highlighting is disabled by default.

Show First 20 Lines • Show All 1,325 Lines • ▼ Show 20 Lines	bootaddr_rwx(vm_paddr_t pa)
* Note that fixups to the .text section will still work until we		* Note that fixups to the .text section will still work until we
* set CR0.WP.		* set CR0.WP.
*/		*/
if (pa < round_2mpage(etext - KERNBASE))		if (pa < round_2mpage(etext - KERNBASE))
return (0);		return (0);
return (pg_nx);		return (pg_nx);
}		}

		static int kern_promoted_end;
		alcUnsubmitted Not Done Inline Actions Before introducing a new variable, please take a look at how KERNend is initialized and used. I don't think that we need both KERNend and kern_promoted_end. alc: Before introducing a new variable, please take a look at how KERNend is initialized and used.
		kibAuthorUnsubmitted Done Inline Actions We probably can remove KERNend if going ahead with my change. I think you mean that KERNend is de-facto unused in the patched pmap, and kern_promoted_end can be int, i.e. 4 bytes shorter. kib: We probably can remove KERNend if going ahead with my change. I think you mean that KERNend is…

static void		static void
create_pagetables(vm_paddr_t *firstaddr)		create_pagetables(vm_paddr_t *firstaddr)
{		{
int i, j, ndm1g, nkpdpe, nkdmpde;		int i, j, ndm1g, nkpdpe, nkdmpde;
pt_entry_t *pt_p;		pt_entry_t *pt_p;
pd_entry_t *pd_p;		pd_entry_t *pd_p;
pdp_entry_t *pdp_p;		pdp_entry_t *pdp_p;
pml4_entry_t *p4_p;		pml4_entry_t *p4_p;
▲ Show 20 Lines • Show All 49 Lines • ▼ Show 20 Lines	create_pagetables(vm_paddr_t *firstaddr)
* pages. (pmap_enter requires a PD page to exist for each KPML4E.)		* pages. (pmap_enter requires a PD page to exist for each KPML4E.)
*/		*/
nkpt_init(*firstaddr);		nkpt_init(*firstaddr);
nkpdpe = NKPDPE(nkpt);		nkpdpe = NKPDPE(nkpt);

KPTphys = allocpages(firstaddr, nkpt);		KPTphys = allocpages(firstaddr, nkpt);
KPDphys = allocpages(firstaddr, nkpdpe);		KPDphys = allocpages(firstaddr, nkpdpe);

/* Fill in the underlying page table pages */		/*
/* XXX not fully used, underneath 2M pages */		* Fill in the underlying page table pages. Only used when
		* PTmap mappings are demoted.
		*/
pt_p = (pt_entry_t *)KPTphys;		pt_p = (pt_entry_t *)KPTphys;
for (i = 0; ptoa(i) < *firstaddr; i++)		for (i = 0; ptoa(i) < round_2mpage(*firstaddr); i++)
		dougmUnsubmitted Not Done Inline Actions If you're using rounded-up firstaddr for this loop, you should just move the loop to after the point where you've rounded up firstaddr in line 1045. dougm: If you're using rounded-up *firstaddr for this loop, you should just move the loop to after the…
		kibAuthorUnsubmitted Done Inline Actions This operation is boot-time only, so micro-optimizing it is not too important. The current sequence of initialization actually makes some sense from the hierarchical view on the page tables, and I want to keep it as is. kib: This operation is boot-time only, so micro-optimizing it is not too important. The current…
		alcUnsubmitted Not Done Inline Actions Unless you want to set PG_PROMOTED on the PDEs below, this loop is pointless (and has been for quite some time). If we actually want to support demotion on the virtual address range being mapped here, we should be inserting these page table pages into the pmap's radix tree. alc: Unless you want to set PG_PROMOTED on the PDEs below, this loop is pointless (and has been for…
		alcUnsubmitted Not Done Inline Actions ... and, in fact, pmap_init() is doing the insertion. alc: ... and, in fact, pmap_init() is doing the insertion.
pt_p[i] = ptoa(i) \| X86_PG_V \| pg_g \| bootaddr_rwx(ptoa(i));		pt_p[i] = ptoa(i) \| X86_PG_V \| pg_g \| bootaddr_rwx(ptoa(i));

/* Now map the page tables at their location within PTmap */		/* Now map the page tables at their location within PTmap */
pd_p = (pd_entry_t *)KPDphys;		pd_p = (pd_entry_t *)KPDphys;
for (i = 0; i < nkpt; i++)		for (i = 0; i < nkpt; i++)
pd_p[i] = (KPTphys + ptoa(i)) \| X86_PG_RW \| X86_PG_V;		pd_p[i] = (KPTphys + ptoa(i)) \| X86_PG_RW \| X86_PG_V;

/* Map from zero to end of allocations under 2M pages */		/*
/* This replaces some of the KPTphys entries above */		* Map from zero to end of allocations under 2M pages.
for (i = 0; (i << PDRSHIFT) < *firstaddr; i++)		* This replaces some of the KPTphys entries above.
		*/
		kern_promoted_end = *firstaddr;
		for (i = 0; (i << PDRSHIFT) < kern_promoted_end; i++)
/* Preset PG_M and PG_A because demotion expects it. */		/* Preset PG_M and PG_A because demotion expects it. */
pd_p[i] = (i << PDRSHIFT) \| X86_PG_V \| PG_PS \| pg_g \|		pd_p[i] = (i << PDRSHIFT) \| X86_PG_V \| PG_PS \| pg_g \|
X86_PG_M \| X86_PG_A \| bootaddr_rwx(i << PDRSHIFT);		X86_PG_M \| X86_PG_A \| bootaddr_rwx(i << PDRSHIFT);
		kern_promoted_end = i;
/*		/*
* Because we map the physical blocks in 2M pages, adjust firstaddr		* Because we map the physical blocks in 2M pages, adjust firstaddr
* to record the physical blocks we've actually mapped into kernel		* to record the physical blocks we've actually mapped into kernel
* virtual address space.		* virtual address space.
*/		*/
firstaddr = round_2mpage(firstaddr);		firstaddr = round_2mpage(firstaddr);

/* And connect up the PD to the PDP (leaving room for L4 pages) */		/* And connect up the PD to the PDP (leaving room for L4 pages) */
▲ Show 20 Lines • Show All 319 Lines • ▼ Show 20 Lines	pmap_init(void)
PMAP_LOCK(kernel_pmap);		PMAP_LOCK(kernel_pmap);
for (i = 0; i < nkpt; i++) {		for (i = 0; i < nkpt; i++) {
mpte = PHYS_TO_VM_PAGE(KPTphys + (i << PAGE_SHIFT));		mpte = PHYS_TO_VM_PAGE(KPTphys + (i << PAGE_SHIFT));
KASSERT(mpte >= vm_page_array &&		KASSERT(mpte >= vm_page_array &&
mpte < &vm_page_array[vm_page_array_size],		mpte < &vm_page_array[vm_page_array_size],
("pmap_init: page table page is out of range"));		("pmap_init: page table page is out of range"));
mpte->pindex = pmap_pde_pindex(KERNBASE) + i;		mpte->pindex = pmap_pde_pindex(KERNBASE) + i;
mpte->phys_addr = KPTphys + (i << PAGE_SHIFT);		mpte->phys_addr = KPTphys + (i << PAGE_SHIFT);
mpte->wire_count = 1;		mpte->wire_count = 1;
		alcUnsubmitted Not Done Inline Actions The kernel page table pages that are dynamically allocated by pmap_growkernel() will have a wire count of 1. alc: The kernel page table pages that are dynamically allocated by pmap_growkernel() will have a…
if (i << PDRSHIFT < KERNend &&		if (i < kern_promoted_end &&
		alcUnsubmitted Not Done Inline Actions I'm not convinced that KERNend is the correct value to use here. I think that it is too small. alc: I'm not convinced that KERNend is the correct value to use here. I think that it is too small.
pmap_insert_pt_page(kernel_pmap, mpte))		pmap_insert_pt_page(kernel_pmap, mpte))
panic("pmap_init: pmap_insert_pt_page failed");		panic("pmap_init: pmap_insert_pt_page failed");
}		}
PMAP_UNLOCK(kernel_pmap);		PMAP_UNLOCK(kernel_pmap);
vm_wire_add(nkpt);		vm_wire_add(nkpt);

/*		/*
* If the kernel is running on a virtual machine, then it must assume		* If the kernel is running on a virtual machine, then it must assume
▲ Show 20 Lines • Show All 2,713 Lines • ▼ Show 20 Lines	pmap_demote_pde(pmap_t pmap, pd_entry_t *pde, vm_offset_t va)

lock = NULL;		lock = NULL;
rv = pmap_demote_pde_locked(pmap, pde, va, &lock);		rv = pmap_demote_pde_locked(pmap, pde, va, &lock);
if (lock != NULL)		if (lock != NULL)
rw_wunlock(lock);		rw_wunlock(lock);
return (rv);		return (rv);
}		}

		static void
		pmap_demote_pde_check(pt_entry_t *firstpte __unused, pt_entry_t newpte __unused)
		{
		#ifdef INVARIANTS
		#ifdef DIAGNOSTIC
		pt_entry_t xpte, ypte;

		for (xpte = firstpte; xpte < firstpte + NPTEPG; xpte++) {
		if ((*xpte & PG_FRAME) != (newpte & PG_FRAME) +
		ptoa(xpte - firstpte)) {
		alcUnsubmitted Not Done Inline Actions You could simply add PAGE_SIZE to newpte at the end of each iteration. alc: You could simply add PAGE_SIZE to newpte at the end of each iteration.
		printf("pmap_demote_pde: xpte %zd and newpte map "
		"different pages %#lx %#lx\n",
		alcUnsubmitted Not Done Inline Actions I suggest: "different pages: found %#lx, expected %#lx\n" alc: I suggest: "different pages: found %#lx, expected %#lx\n"
		xpte - firstpte, *xpte, newpte);
		printf("XX %lx %lx\n", (*xpte & PG_FRAME),
		alcUnsubmitted Not Done Inline Actions The "XX" should be replaced. alc: The "XX" should be replaced.
		(newpte & PG_FRAME) + ptoa(xpte - firstpte));
		for (ypte = firstpte; ypte < firstpte + NPTEPG; ypte++)
		printf("%zd %#lx\n", ypte - firstpte, *ypte);
		panic("firstpte");
		}
		}
		#else
		KASSERT((*firstpte & PG_FRAME) == (newpte & PG_FRAME),
		("pmap_demote_pde: firstpte and newpte map different physical"
		" addresses"));
		#endif
		#endif
		}

		static void
		pmap_demote_pde_fail(pmap_t pmap, vm_offset_t va, pd_entry_t *pde,
		pd_entry_t oldpde, struct rwlock **lockp)
		{
		struct spglist free;
		vm_offset_t sva;

		SLIST_INIT(&free);
		sva = trunc_2mpage(va);
		pmap_remove_pde(pmap, pde, sva, &free, lockp);
		if ((oldpde & pmap_global_bit(pmap)) == 0)
		pmap_invalidate_pde_page(pmap, sva, oldpde);
		vm_page_free_pages_toq(&free, true);
		CTR2(KTR_PMAP, "pmap_demote_pde: failure for va %#lx"
		" in pmap %p", va, pmap);
		alcUnsubmitted Not Done Inline Actions You should be able to unwrap the format string. alc: You should be able to unwrap the format string.
		}

static boolean_t		static boolean_t
pmap_demote_pde_locked(pmap_t pmap, pd_entry_t *pde, vm_offset_t va,		pmap_demote_pde_locked(pmap_t pmap, pd_entry_t *pde, vm_offset_t va,
struct rwlock **lockp)		struct rwlock **lockp)
{		{
pd_entry_t newpde, oldpde;		pd_entry_t newpde, oldpde;
pt_entry_t *firstpte, newpte;		pt_entry_t *firstpte, newpte;
pt_entry_t PG_A, PG_G, PG_M, PG_PKU_MASK, PG_RW, PG_V;		pt_entry_t PG_A, PG_G, PG_M, PG_PKU_MASK, PG_RW, PG_V;
vm_paddr_t mptepa;		vm_paddr_t mptepa;
vm_page_t mpte;		vm_page_t mpte;
struct spglist free;
vm_offset_t sva;
int PG_PTE_CACHE;		int PG_PTE_CACHE;
		bool in_kernel;

PG_G = pmap_global_bit(pmap);
PG_A = pmap_accessed_bit(pmap);		PG_A = pmap_accessed_bit(pmap);
		PG_G = pmap_global_bit(pmap);
		alcUnsubmitted Not Done Inline Actions You've eliminated the only use of PG_G in this function. alc: You've eliminated the only use of PG_G in this function.
		kibAuthorUnsubmitted Done Inline Actions It is used by PG_PTE_PROMOTED macro. I tried that in initial version. kib: It is used by PG_PTE_PROMOTED macro. I tried that in initial version.
PG_M = pmap_modified_bit(pmap);		PG_M = pmap_modified_bit(pmap);
PG_RW = pmap_rw_bit(pmap);		PG_RW = pmap_rw_bit(pmap);
PG_V = pmap_valid_bit(pmap);		PG_V = pmap_valid_bit(pmap);
PG_PTE_CACHE = pmap_cache_mask(pmap, 0);		PG_PTE_CACHE = pmap_cache_mask(pmap, 0);
PG_PKU_MASK = pmap_pku_mask_bit(pmap);		PG_PKU_MASK = pmap_pku_mask_bit(pmap);

PMAP_LOCK_ASSERT(pmap, MA_OWNED);		PMAP_LOCK_ASSERT(pmap, MA_OWNED);
		in_kernel = va >= VM_MAXUSER_ADDRESS;
		markjUnsubmitted Not Done Inline Actions I guess it's supposed to be MAXUSER_ADDRESS. Technically this is a functional change since before we were only allocating with interrupt priority for demotions in the direct map. I think it's fine, just contradicts the review description. markj: I guess it's supposed to be MAXUSER_ADDRESS. Technically this is a functional change since…
		alcUnsubmitted Not Done Inline Actions The only situation in which this code should ever allocate a kernel page table page is when it is called upon to demote a direct map mapping, so this doesn't really represent a functional change. alc: The only situation in which this code should ever allocate a kernel page table page is when it…
oldpde = *pde;		oldpde = *pde;
KASSERT((oldpde & (PG_PS \| PG_V)) == (PG_PS \| PG_V),		KASSERT((oldpde & (PG_PS \| PG_V)) == (PG_PS \| PG_V),
("pmap_demote_pde: oldpde is missing PG_PS and/or PG_V"));		("pmap_demote_pde: oldpde is missing PG_PS and/or PG_V"));
if ((oldpde & PG_A) == 0 \|\| (mpte = pmap_remove_pt_page(pmap, va)) ==		if ((oldpde & PG_A) == 0) {
NULL) {		pmap_demote_pde_fail(pmap, va, pde, oldpde, lockp);
		markjUnsubmitted Done Inline Actions _fail is a bit misleading here. Maybe _abort? markj: _fail is a bit misleading here. Maybe _abort?
		return (FALSE);
		}
		mpte = pmap_remove_pt_page(pmap, va);
		if (mpte == NULL) {
KASSERT((oldpde & PG_W) == 0,		KASSERT((oldpde & PG_W) == 0,
("pmap_demote_pde: page table page for a wired mapping"		("pmap_demote_pde: page table page for a wired mapping"
" is missing"));		" is missing"));
		KASSERT(!in_kernel \|\| (va >= DMAP_MIN_ADDRESS &&
		va < DMAP_MAX_ADDRESS),
		("pmap_demote_pde: No saved mpte for va %#lx", va));

/*		/*
* Invalidate the 2MB page mapping and return "failure" if the		* Invalidate the 2MB page mapping and return "failure" if the
* mapping was never accessed or the allocation of the new		* mapping was never accessed or the allocation of the new
* page table page fails. If the 2MB page mapping belongs to		* page table page fails. If the 2MB page mapping belongs to
* the direct map region of the kernel's address space, then		* the direct map region of the kernel's address space, then
* the page allocation request specifies the highest possible		* the page allocation request specifies the highest possible
* priority (VM_ALLOC_INTERRUPT). Otherwise, the priority is		* priority (VM_ALLOC_INTERRUPT). Otherwise, the priority is
* normal. Page table pages are preallocated for every other		* normal. Page table pages are preallocated for every other
* part of the kernel address space, so the direct map region		* part of the kernel address space, so the direct map region
* is the only part of the kernel address space that must be		* is the only part of the kernel address space that must be
* handled here.		* handled here.
*/		*/
		alcUnsubmitted Not Done Inline Actions "is for a kernel address, ..." alc: "is for a kernel address, ..."
if ((oldpde & PG_A) == 0 \|\| (mpte = vm_page_alloc(NULL,		mpte = vm_page_alloc(NULL, pmap_pde_pindex(va),
		alcUnsubmitted Done Inline Actions The first part of this comment no longer belongs here. alc: The first part of this comment no longer belongs here.
pmap_pde_pindex(va), (va >= DMAP_MIN_ADDRESS && va <		(in_kernel ? VM_ALLOC_INTERRUPT : VM_ALLOC_NORMAL) \|
DMAP_MAX_ADDRESS ? VM_ALLOC_INTERRUPT : VM_ALLOC_NORMAL) \|		VM_ALLOC_NOOBJ \| VM_ALLOC_WIRED);
VM_ALLOC_NOOBJ \| VM_ALLOC_WIRED)) == NULL) {		if (mpte == NULL) {
SLIST_INIT(&free);		pmap_demote_pde_fail(pmap, va, pde, oldpde, lockp);
sva = trunc_2mpage(va);
pmap_remove_pde(pmap, pde, sva, &free, lockp);
if ((oldpde & PG_G) == 0)
pmap_invalidate_pde_page(pmap, sva, oldpde);
vm_page_free_pages_toq(&free, true);
CTR2(KTR_PMAP, "pmap_demote_pde: failure for va %#lx"
" in pmap %p", va, pmap);
return (FALSE);		return (FALSE);
}		}
if (va < VM_MAXUSER_ADDRESS)		if (!in_kernel) {
		mpte->wire_count = NPTEPG;
pmap_resident_count_inc(pmap, 1);		pmap_resident_count_inc(pmap, 1);
}		}
		}
mptepa = VM_PAGE_TO_PHYS(mpte);		mptepa = VM_PAGE_TO_PHYS(mpte);
firstpte = (pt_entry_t *)PHYS_TO_DMAP(mptepa);		firstpte = (pt_entry_t *)PHYS_TO_DMAP(mptepa);
newpde = mptepa \| PG_M \| PG_A \| (oldpde & PG_U) \| PG_RW \| PG_V;		newpde = mptepa \| PG_M \| PG_A \| (oldpde & PG_U) \| PG_RW \| PG_V;
KASSERT((oldpde & PG_A) != 0,		KASSERT((oldpde & PG_A) != 0,
("pmap_demote_pde: oldpde is missing PG_A"));		("pmap_demote_pde: oldpde is missing PG_A"));
KASSERT((oldpde & (PG_M \| PG_RW)) != PG_RW,		KASSERT((oldpde & (PG_M \| PG_RW)) != PG_RW,
		alcUnsubmitted Not Done Inline Actions "abort" could reasonably be interpreted as leave the mapping in place. Instead, I would explicitly say, "invalidate the 2MB page mapping". alc: "abort" could reasonably be interpreted as leave the mapping in place. Instead, I would…
("pmap_demote_pde: oldpde is missing PG_M"));		("pmap_demote_pde: oldpde is missing PG_M"));
newpte = oldpde & ~PG_PS;		newpte = oldpde & ~PG_PS;
newpte = pmap_swap_pat(pmap, newpte);		newpte = pmap_swap_pat(pmap, newpte);

/*		/*
* If the page table page is new, initialize it.		* If the page table page is new, initialize it.
*/		*/
if (mpte->wire_count == 1) {		if ((oldpde & PG_PROMOTED) == 0)
mpte->wire_count = NPTEPG;
pmap_fill_ptp(firstpte, newpte);		pmap_fill_ptp(firstpte, newpte);
}		pmap_demote_pde_check(firstpte, newpte);
		alcUnsubmitted Not Done Inline Actions When I wrote this block of code, the PG_PROMOTED flag did not yet exist. If, today, I were writing this block for the first time, I would test (oldpte & PG_PROMOTED) == 0 instead of mpte->wire_count == 1. alc: When I wrote this block of code, the PG_PROMOTED flag did not yet exist. If, today, I were…
KASSERT((*firstpte & PG_FRAME) == (newpte & PG_FRAME),
("pmap_demote_pde: firstpte and newpte map different physical"
" addresses"));

/*		/*
* If the mapping has changed attributes, update the page table		* If the mapping has changed attributes, update the page table
* entries.		* entries.
*/		*/
if ((*firstpte & PG_PTE_PROMOTE) != (newpte & PG_PTE_PROMOTE))		if ((*firstpte & PG_PTE_PROMOTE) != (newpte & PG_PTE_PROMOTE))
pmap_fill_ptp(firstpte, newpte);		pmap_fill_ptp(firstpte, newpte);
		alcUnsubmitted Not Done Inline Actions I speculate that this block would no longer be needed if we test (oldpte & PG_PROMOTED) == 0 above. In particular, pmap_protect_pde() clears PG_PROMOTED if it removes access rights from the PDE. alc: I speculate that this block would no longer be needed if we test (oldpte & PG_PROMOTED) == 0…

/*		/*
* The spare PV entries must be reserved prior to demoting the		* The spare PV entries must be reserved prior to demoting the
* mapping, that is, prior to changing the PDE. Otherwise, the state		* mapping, that is, prior to changing the PDE. Otherwise, the state
* of the PDE and the PV lists will be inconsistent, which can result		* of the PDE and the PV lists will be inconsistent, which can result
* in reclaim_pv_chunk() attempting to remove a PV entry from the		* in reclaim_pv_chunk() attempting to remove a PV entry from the
* wrong PV list and pmap_pv_demote_pde() failing to find the expected		* wrong PV list and pmap_pv_demote_pde() failing to find the expected
* PV entry for the 2MB page mapping that is being demoted.		* PV entry for the 2MB page mapping that is being demoted.
Show All 11 Lines	pmap_demote_pde_locked(pmap_t pmap, pd_entry_t *pde, vm_offset_t va,
if (workaround_erratum383)		if (workaround_erratum383)
pmap_update_pde(pmap, va, pde, newpde);		pmap_update_pde(pmap, va, pde, newpde);
else		else
pde_store(pde, newpde);		pde_store(pde, newpde);

/*		/*
* Invalidate a stale recursive mapping of the page table page.		* Invalidate a stale recursive mapping of the page table page.
*/		*/
if (va >= VM_MAXUSER_ADDRESS)		if (in_kernel)
pmap_invalidate_page(pmap, (vm_offset_t)vtopte(va));		pmap_invalidate_page(pmap, (vm_offset_t)vtopte(va));

/*		/*
* Demote the PV entry.		* Demote the PV entry.
*/		*/
if ((oldpde & PG_MANAGED) != 0)		if ((oldpde & PG_MANAGED) != 0)
pmap_pv_demote_pde(pmap, va, oldpde & PG_PS_FRAME, lockp);		pmap_pv_demote_pde(pmap, va, oldpde & PG_PS_FRAME, lockp);

atomic_add_long(&pmap_pde_demotions, 1);		atomic_add_long(&pmap_pde_demotions, 1);
CTR2(KTR_PMAP, "pmap_demote_pde: success for va %#lx"		CTR2(KTR_PMAP, "pmap_demote_pde: success for va %#lx"
" in pmap %p", va, pmap);		" in pmap %p", va, pmap);
		markjUnsubmitted Not Done Inline Actions This format string may be unwrapped as well. markj: This format string may be unwrapped as well.
return (TRUE);		return (TRUE);
}		}

/*		/*
* pmap_remove_kernel_pde: Remove a kernel superpage mapping.		* pmap_remove_kernel_pde: Remove a kernel superpage mapping.
*/		*/
static void		static void
pmap_remove_kernel_pde(pmap_t pmap, pd_entry_t *pde, vm_offset_t va)		pmap_remove_kernel_pde(pmap_t pmap, pd_entry_t *pde, vm_offset_t va)
▲ Show 20 Lines • Show All 5,149 Lines • Show Last 20 Lines

Simplify flow of pmap_demote_pde_locked() and add more comprehensive debugging checks.
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 57713

sys/amd64/amd64/pmap.c

Simplify flow of pmap_demote_pde_locked() and add more comprehensive debugging checks.ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 57713

sys/amd64/amd64/pmap.c

Simplify flow of pmap_demote_pde_locked() and add more comprehensive debugging checks.
ClosedPublic
Actions

Revision Contents
Changeset List