> Date: Wed, 19 Dec 2018 22:36:49 -0500
> From: George Koehler <[email protected]>
>
> On Wed, 19 Dec 2018 00:13:13 -0500
> George Koehler <[email protected]> wrote:
>
> > By my guess, pmap_is_modified() is broken on powerpc. It seems to
> > fail to return true, so uvm wouldn't know that the page is modified,
> > and then uvm wouldn't write the modified page to swap.
>
> Turns out that pmap_page_protect(pg, PROT_NONE) forgot to check if
> the page was modified. The below diff seems to fix it.
>
> pmap_is_modified(pg) calls pmap_test_attrs(pg, PG_PMAP_MOD), which
> iterates pg->mdpage.pv_list to check if the hardware had set
> PTE_CHG_32 on any mapping of this physical page. The list is empty
> because uvm seems to remove all the mappings before it asks if the
> page is modified, then swaps out the page.
>
> A function that does LIST_REMOVE a mapping with PTE_CHG_32 must set
> PG_PMAP_MOD in pg->pg_flags, but pmap_page_protect(pg, PROT_NONE)
> fails to do so. This function does
> pted->pted_va &= ~PTED_VA_MANAGED_M;
> LIST_REMOVE(pted, pted_pv_list);
> before
> pmap_remove_pted(pm, pted);
> Then pmap_remove_pted() calls pte_zap(), which would check PTE_CHG_32
> if PTED_MANAGED(pted) is true. The check is skipped because we had
> unset PTED_VA_MANAGED_M.
>
> The following diff fixes my test program on my single G4 processor
> (MPC7447A), where ppc_proc_is_64b is false. I will use this diff
> until someone gives me a better diff.
That's a very good find. I think there still is a potential race in
your diff on MP systems since you save the bits before removing the
PTE from the has tables. I'll see if I can come up with a better diff.
> Index: pmap.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/powerpc/powerpc/pmap.c,v
> retrieving revision 1.168
> diff -u -p -r1.168 pmap.c
> --- pmap.c 22 Oct 2018 17:31:25 -0000 1.168
> +++ pmap.c 20 Dec 2018 03:16:13 -0000
> @@ -540,6 +540,28 @@ pmap_attr_save(paddr_t pa, u_int32_t bit
> atomic_setbits_int(&pg->pg_flags, pmap_pte2flags(bits));
> }
>
> +/* Fixes pg->pg_flags by checking REF and CHG. */
> +void
> +pmap_fix_page_attrs(struct vm_page *pg, struct pte_desc *pted)
> +{
> + void *pte;
> + u_int bits;
> + int s;
> +
> + PMAP_HASH_LOCK(s);
> + if ((pte = pmap_ptedinhash(pted)) != NULL) {
> + if (ppc_proc_is_64b) {
> + struct pte_64 *pte64 = pte;
> + bits = pte64->pte_lo & (PTE_REF_64|PTE_CHG_64);
> + } else {
> + struct pte_32 *pte32 = pte;
> + bits = pte32->pte_lo & (PTE_REF_32|PTE_CHG_32);
> + }
> + atomic_setbits_int(&pg->pg_flags, pmap_pte2flags(bits));
> + }
> + PMAP_HASH_UNLOCK(s);
> +}
> +
> int
> pmap_enter(pmap_t pm, vaddr_t va, paddr_t pa, vm_prot_t prot, int flags)
> {
> @@ -2101,6 +2123,13 @@ pmap_page_protect(struct vm_page *pg, vm
> pted->pted_va &= ~PTED_VA_MANAGED_M;
> LIST_REMOVE(pted, pted_pv_list);
> mtx_leave(&pg->mdpage.pv_mtx);
> +
> + /*
> + * Check REF and CHG now, because
> + * pmap_remove_pted() won't check them when
> + * not PTED_VA_MANAGED_M.
> + */
> + pmap_fix_page_attrs(pg, pted);
>
> pmap_remove_pted(pm, pted);
>
>
> --
> George Koehler <[email protected]>
>
>
