Redefines PTE present vs mapped
authorBarret Rhoden <brho@cs.berkeley.edu>
Mon, 6 Apr 2015 16:23:43 +0000 (12:23 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Tue, 7 Apr 2015 19:06:59 +0000 (15:06 -0400)
Mapped now means it points to a physical page in all cases.  Present is
mapped and with bits set for some valid PTE walk.  For instance, a user
read works on x86 with PTE_U and PTE_P.

PTE_P shouldn't be used from the arch-indep code now.

I might have missed something with this commit, like leaking memory or
dirty bits.  Feel free to check out the usages of is_present, is_mapped,
and is_unmapped.

kern/arch/riscv/ros/mmu.h
kern/arch/x86/pmap64.c
kern/arch/x86/pmap_ops.h
kern/arch/x86/ros/mmu64.h
kern/src/env.c
kern/src/mm.c
kern/src/pagemap.c
kern/src/pmap.c
kern/src/umem.c

index c1de48e..a393b91 100644 (file)
 #define PTE_NOCACHE    0 // PTE bits to turn off caching, if possible
 
 // commly used access modes
+
+#warning "Review RISCV PTEs.  Maybe want PTE_E/PTE_R?"
+       /* arch-indep code doesn't set PTE_P, it just sets a perm */
+
+
 #define PTE_KERN_RW    (PTE_SR | PTE_SW | PTE_SX)
 #define PTE_KERN_RO    (PTE_SR | PTE_SX)
 #define PTE_USER_RW    (PTE_SR | PTE_SW | PTE_UR | PTE_UW | PTE_UX)
 #define PTE_USER_RO    (PTE_SR | PTE_UR | PTE_UX)
+#define PTE_NONE       0
 
+#warning "probably remove this"
 // x86 equivalencies
 #define PTE_P      PTE_E
 
index 658df50..901a9af 100644 (file)
@@ -163,7 +163,7 @@ static void map_my_pages(kpte_t *pgdir, uintptr_t va, size_t size,
             pa += pgsize) {
                kpte = get_next_pte(kpte, pgdir, va, PG_WALK_CREATE | pml_shift);
                assert(kpte);
-               *kpte = PTE_ADDR(pa) | PTE_P | perm |
+               *kpte = PTE_ADDR(pa) | perm |
                        (pml_shift != PML1_SHIFT ? PTE_PS : 0);
                printd("Wrote *kpte %p, for va %p to pa %p tried to cover %p\n",
                       *kpte, va, pa, amt_mapped);
@@ -230,8 +230,8 @@ static int max_possible_shift(uintptr_t va, uintptr_t pa)
 
 /* Map [va, va+size) of virtual (linear) address space to physical [pa, pa+size)
  * in the page table rooted at pgdir.  Size is a multiple of PGSIZE.  Use
- * permission bits perm|PTE_P for the entries.  Set pml_shift to the shift of
- * the largest page size you're willing to use.
+ * permission bits perm for the entries.  Set pml_shift to the shift of the
+ * largest page size you're willing to use.
  *
  * Doesn't handle having pages currently mapped yet, and while supporting that
  * is relatively easy, doing an insertion of small pages into an existing jumbo
@@ -383,7 +383,7 @@ static int pml_perm_walk(kpte_t *pml, const void *va, int pml_shift)
        kpte = &pml[PMLx(va, pml_shift)];
        if (!(*kpte & PTE_P))
                return 0;
-       perms_here = *kpte & (PTE_PERM | PTE_P);
+       perms_here = *kpte & PTE_PERM;
        if (walk_is_complete(kpte, pml_shift, PML1_SHIFT))
                return perms_here;
        return pml_perm_walk(kpte2pml(*kpte), va, pml_shift - BITS_PER_PML) &
@@ -450,9 +450,9 @@ void vm_init(void)
        /* For the LAPIC and IOAPIC, we use PAT (but not *the* PAT flag) to make
         * these type UC */
        map_segment(boot_pgdir, LAPIC_BASE, APIC_SIZE, LAPIC_PBASE,
-                   PTE_PCD | PTE_PWT | PTE_W | PTE_G, max_jumbo_shift);
+                   PTE_PCD | PTE_PWT | PTE_KERN_RW | PTE_G, max_jumbo_shift);
        map_segment(boot_pgdir, IOAPIC_BASE, APIC_SIZE, IOAPIC_PBASE,
-                   PTE_PCD | PTE_PWT | PTE_W | PTE_G, max_jumbo_shift);
+                   PTE_PCD | PTE_PWT | PTE_KERN_RW | PTE_G, max_jumbo_shift);
        /* VPT mapping: recursive PTE inserted at the VPT spot */
        boot_kpt[PML4(VPT)] = PADDR(boot_kpt) | PTE_W | PTE_P;
        /* same for UVPT, accessible by userspace (RO). */
@@ -594,8 +594,8 @@ int arch_pgdir_setup(pgdir_t boot_copy, pgdir_t *new_pd)
        memset(ept, 0, PGSIZE);
 
        /* VPT and UVPT map the proc's page table, with different permissions. */
-       kpt[PML4(VPT)]  = PTE(LA2PPN(PADDR(kpt)), PTE_P | PTE_KERN_RW);
-       kpt[PML4(UVPT)] = PTE(LA2PPN(PADDR(kpt)), PTE_P | PTE_USER_RO);
+       kpt[PML4(VPT)]  = PTE(LA2PPN(PADDR(kpt)), PTE_KERN_RW);
+       kpt[PML4(UVPT)] = PTE(LA2PPN(PADDR(kpt)), PTE_USER_RO);
 
        new_pd->kpte = kpt;
        /* Processes do not have EPTs by default, only once they are VMMs */
@@ -627,7 +627,7 @@ int arch_max_jumbo_page_shift(void)
 static int print_pte(kpte_t *kpte, uintptr_t kva, int shift, bool visited_subs,
                      void *data)
 {
-       if (!(*kpte & PTE_P))
+       if (kpte_is_unmapped(kpte))
                return 0;
        switch (shift) {
                case (PML1_SHIFT):
index 7a2293e..78c05fb 100644 (file)
@@ -24,13 +24,21 @@ static inline bool pte_walk_okay(pte_t pte)
 }
 
 /* PTE states:
- *  - present: the PTE is involved in a valid page table walk, with the physaddr
- *  part pointing to a physical page.
+ *  - present: the PTE is involved in a valid page table walk, can be used
+ *  for some form of hardware access (read, write, user, etc), and with the
+ *  physaddr part pointing to a physical page.
  *
  *     - mapped: the PTE is involved in some sort of mapping, e.g. a VMR.  We're
- *     storing something in the PTE, but it is isn't necessarily present and
- *     pointing to an actual physical page.  All present are mapped, but not vice
- *     versa.  Mapped could also include paged-out, if we support that later.
+ *     storing something in the PTE, but it is isn't necessarily present.
+ *     Currently, all mapped pages should point to an actual physical page.
+ *     All present are mapped, but not vice versa.  Mapped pages can point to a
+ *     real page, but with no access permissions, which is the main distinction
+ *     between present and mapped.
+ *
+ *     - paged_out: we don't actually use this yet.  Since mapped vs present is
+ *     based on the PTE present bits, we'd need to use reserved bits in the PTE to
+ *     differentiate between other states.  Right now, paged_out == mapped, as far
+ *     as the code is concerned.
  *
  *     - unmapped: completely unused. (0 value) */
 static inline bool pte_is_present(pte_t pte)
index ab97a02..38b8361 100644 (file)
@@ -286,11 +286,12 @@ typedef struct x86_pgdir {
 
 /* Permissions fields and common access modes.  These should be read as 'just
  * kernel or user too' and 'RO or RW'.  USER_RO means read-only for everyone. */
-#define PTE_PERM               (PTE_W | PTE_U)
-#define PTE_KERN_RW            PTE_W           // Kernel Read/Write
-#define PTE_KERN_RO            0               // Kernel Read-Only
-#define PTE_USER_RW            (PTE_W | PTE_U) // Kernel/User Read/Write
-#define PTE_USER_RO            PTE_U           // Kernel/User Read-Only
+#define PTE_PERM               (PTE_W | PTE_U | PTE_P)
+#define PTE_KERN_RW            (PTE_W | PTE_P)
+#define PTE_KERN_RO            PTE_P
+#define PTE_USER_RW            (PTE_W | PTE_U | PTE_P)
+#define PTE_USER_RO            (PTE_U | PTE_P)
+#define PTE_NONE               0
 
 /* The PTE/translation part of a PTE/virtual(linear) address.  It's used
  * frequently to be the page address of a virtual address.  Note this doesn't
@@ -303,7 +304,7 @@ typedef struct x86_pgdir {
 /* we must guarantee that for any PTE, exactly one of the following is true */
 #define PAGE_PRESENT(pte) ((pte) & PTE_P)
 #define PAGE_UNMAPPED(pte) ((pte) == 0)
-#define PAGE_PAGED_OUT(pte) (!PAGE_PRESENT(pte) && !PAGE_UNMAPPED(pte))
+#define PAGE_PAGED_OUT(pte) (0)        /* Need to use an OS reserved PTE bit */
 
 
 /* **************************************** */
index d7a58c7..f2c49c1 100644 (file)
@@ -103,7 +103,7 @@ void env_user_mem_free(env_t* e, void* start, size_t len)
        assert((uintptr_t)start + len <= UVPT); //since this keeps fucking happening
        int user_page_free(env_t* e, pte_t pte, void* va, void* arg)
        {
-               if (!pte_is_present(pte))
+               if (!pte_is_mapped(pte))
                        return 0;
                page_t *page = pa2page(pte_get_paddr(pte));
                pte_clear(pte);
index 94daa68..2b10536 100644 (file)
@@ -288,7 +288,7 @@ static int copy_pages(struct proc *p, struct proc *new_p, uintptr_t va_start,
                        return 0;
                /* pages could be !P, but right now that's only for file backed VMRs
                 * undergoing page removal, which isn't the caller of copy_pages. */
-               if (pte_is_present(pte)) {
+               if (pte_is_mapped(pte)) {
                        /* TODO: check for jumbos */
                        if (upage_alloc(new_p, &pp, 0))
                                return -ENOMEM;
@@ -459,7 +459,7 @@ out_error:  /* for debugging */
        return FALSE;
 }
 
-/* Helper, maps in page at addr, but only if nothing is present there.  Returns
+/* Helper, maps in page at addr, but only if nothing is mapped there.  Returns
  * 0 on success.  If this is called by non-PM code, we'll store your ref in the
  * PTE. */
 static int map_page_at_addr(struct proc *p, struct page *page, uintptr_t addr,
@@ -483,10 +483,19 @@ static int map_page_at_addr(struct proc *p, struct page *page, uintptr_t addr,
                page_decref(page);
                return 0;
        }
+       if (pte_is_mapped(pte)) {
+               /* we're clobbering an old entry.  if we're just updating the prot, then
+                * it's no big deal.  o/w, there might be an issue. */
+               if (page2pa(page) != pte_get_paddr(pte)) {
+                       warn_once("Clobbered a PTE mapping (%p -> %p)\n", pte_print(pte),
+                                 page2pa(page) | prot);
+               }
+               page_decref(pa2page(pte_get_paddr(pte)));
+       }
        /* preserve the dirty bit - pm removal could be looking concurrently */
        prot |= (pte_is_dirty(pte) ? PTE_D : 0);
        /* We have a ref to page, which we are storing in the PTE */
-       pte_write(pte, page2pa(page), PTE_P | prot);
+       pte_write(pte, page2pa(page), prot);
        spin_unlock(&p->pte_lock);
        return 0;
 }
@@ -705,7 +714,7 @@ int __do_mprotect(struct proc *p, uintptr_t addr, size_t len, int prot)
        pte_t pte;
        bool shootdown_needed = FALSE;
        int pte_prot = (prot & PROT_WRITE) ? PTE_USER_RW :
-                      (prot & (PROT_READ|PROT_EXEC)) ? PTE_USER_RO : 0;
+                      (prot & (PROT_READ|PROT_EXEC)) ? PTE_USER_RO : PTE_NONE;
        /* TODO: this is aggressively splitting, when we might not need to if the
         * prots are the same as the previous.  Plus, there are three excessive
         * scans.  Finally, we might be able to merge when we are done. */
@@ -720,10 +729,14 @@ int __do_mprotect(struct proc *p, uintptr_t addr, size_t len, int prot)
                }
                vmr->vm_prot = prot;
                spin_lock(&p->pte_lock);        /* walking and changing PTEs */
-               /* TODO: use a memwalk */
+               /* TODO: use a memwalk.  At a minimum, we need to change every existing
+                * PTE that won't trigger a PF (meaning, present PTEs) to have the new
+                * prot.  The others will fault on access, and we'll change the PTE
+                * then.  In the off chance we have a mapped but not present PTE, we
+                * might as well change it too, since we're already here. */
                for (uintptr_t va = vmr->vm_base; va < vmr->vm_end; va += PGSIZE) { 
                        pte = pgdir_walk(p->env_pgdir, (void*)va, 0);
-                       if (pte_walk_okay(pte) && pte_is_present(pte)) {
+                       if (pte_walk_okay(pte) && pte_is_mapped(pte)) {
                                pte_replace_perm(pte, pte_prot);
                                shootdown_needed = TRUE;
                        }
@@ -765,11 +778,9 @@ static int __munmap_mark_not_present(struct proc *p, pte_t pte, void *va,
                                      void *arg)
 {
        bool *shootdown_needed = (bool*)arg;
-       struct page *page;
        /* could put in some checks here for !P and also !0 */
        if (!pte_is_present(pte))       /* unmapped (== 0) *ptes are also not PTE_P */
                return 0;
-       page = pa2page(pte_get_paddr(pte));
        pte_clear_present(pte);
        *shootdown_needed = TRUE;
        return 0;
@@ -1164,7 +1175,7 @@ static uintptr_t vmap_pmem_flags(uintptr_t paddr, size_t nr_bytes, int flags)
        /* it's not strictly necessary to drop paddr's pgoff, but it might save some
         * vmap heartache in the future. */
        if (map_vmap_segment(vaddr, PG_ADDR(paddr), nr_pages,
-                            PTE_P | PTE_KERN_RW | flags)) {
+                            PTE_KERN_RW | flags)) {
                warn("Unable to map a vmap segment");   /* probably a bug */
                return 0;
        }
index 8c7e5a2..6cf169c 100644 (file)
@@ -306,7 +306,11 @@ static void vmr_for_each(struct vm_region *vmr, unsigned long pg_idx,
 static int __pm_mark_not_present(struct proc *p, pte_t pte, void *va, void *arg)
 {
        struct page *page;
-       if (!pte_is_present(pte))
+       /* mapped includes present.  Any PTE pointing to a page (mapped) will get
+        * flagged for removal and have its access prots revoked.  We need to deal
+        * with mapped-but-maybe-not-present in case of a dirtied file that was
+        * mprotected to PROT_NONE (which is not present) */
+       if (pte_is_unmapped(pte))
                return 0;
        page = pa2page(pte_get_paddr(pte));
        if (atomic_read(&page->pg_flags) & PG_REMOVAL)
index cb8351f..b16020f 100644 (file)
@@ -223,7 +223,7 @@ int page_insert(pgdir_t pgdir, struct page *page, void *va, int perm)
         * any other state. (TODO: review all other states, maybe rm only for P) */
        if (pte_is_mapped(pte))
                page_remove(pgdir, va);
-       pte_write(pte, page2pa(page), PTE_P | perm);
+       pte_write(pte, page2pa(page), perm);
        return 0;
 }
 
@@ -247,7 +247,7 @@ int page_insert(pgdir_t pgdir, struct page *page, void *va, int perm)
 page_t *page_lookup(pgdir_t pgdir, void *va, pte_t *pte_store)
 {
        pte_t pte = pgdir_walk(pgdir, va, 0);
-       if (!pte_walk_okay(pte) || !pte_is_present(pte))
+       if (!pte_walk_okay(pte) || !pte_is_mapped(pte))
                return 0;
        if (pte_store)
                *pte_store = pte;
@@ -285,7 +285,7 @@ void page_remove(pgdir_t pgdir, void *va)
        if (!pte_walk_okay(pte) || pte_is_unmapped(pte))
                return;
 
-       if (pte_is_present(pte)) {
+       if (pte_is_mapped(pte)) {
                /* TODO: (TLB) need to do a shootdown, inval sucks.  And might want to
                 * manage the TLB / free pages differently. (like by the caller).
                 * Careful about the proc/memory lock here. */
index 38b4d92..87f4e3a 100644 (file)
@@ -34,7 +34,7 @@ int memcpy_from_user(struct proc *p, void *dest, const void *va,
        const void *start, *end;
        size_t num_pages, i;
        pte_t pte;
-       uintptr_t perm = PTE_P | PTE_USER_RO;
+       uintptr_t perm = PTE_USER_RO;
        size_t bytes_copied = 0;
 
        static_assert(ULIM % PGSIZE == 0 && ULIM != 0); // prevent wrap-around
@@ -100,7 +100,7 @@ int memcpy_to_user(struct proc *p, void *va, const void *src, size_t len)
        const void *start, *end;
        size_t num_pages, i;
        pte_t pte;
-       uintptr_t perm = PTE_P | PTE_USER_RW;
+       uintptr_t perm = PTE_USER_RW;
        size_t bytes_copied = 0;
 
        static_assert(ULIM % PGSIZE == 0 && ULIM != 0); // prevent wrap-around