Changeset View
Standalone View
sys/riscv/riscv/pmap.c
| Show First 20 Lines • Show All 536 Lines • ▼ Show 20 Lines | pmap_distribute_l1(struct pmap *pmap, vm_pindex_t l1index, | ||||||||
| * simply be copied at pmap initialization time. | * simply be copied at pmap initialization time. | ||||||||
| */ | */ | ||||||||
| if (pmap != kernel_pmap || pmap_mode != PMAP_MODE_SV39) | if (pmap != kernel_pmap || pmap_mode != PMAP_MODE_SV39) | ||||||||
| return; | return; | ||||||||
| mtx_lock(&allpmaps_lock); | mtx_lock(&allpmaps_lock); | ||||||||
| LIST_FOREACH(user_pmap, &allpmaps, pm_list) { | LIST_FOREACH(user_pmap, &allpmaps, pm_list) { | ||||||||
| l1 = &user_pmap->pm_top[l1index]; | l1 = &user_pmap->pm_top[l1index]; | ||||||||
| pmap_store(l1, entry); | pmap_store(l1, entry); | ||||||||
markj: Don't you need a fence here too? | |||||||||
| } | } | ||||||||
| mtx_unlock(&allpmaps_lock); | mtx_unlock(&allpmaps_lock); | ||||||||
| } | } | ||||||||
| /* | /* | ||||||||
| * Holds the PTE mode bits (defined in pte.h) for defining e.g. cacheability. | * Holds the PTE mode bits (defined in pte.h) for defining e.g. cacheability. | ||||||||
| * | * | ||||||||
| * The indices correspond to the VM_MEMATTR_* defines in riscv/include/vm.h. | * The indices correspond to the VM_MEMATTR_* defines in riscv/include/vm.h. | ||||||||
| ▲ Show 20 Lines • Show All 2,353 Lines • ▼ Show 20 Lines | retryl3: | ||||||||
| if (anychanged) | if (anychanged) | ||||||||
| pmap_invalidate_all(pmap); | pmap_invalidate_all(pmap); | ||||||||
| if (pv_lists_locked) | if (pv_lists_locked) | ||||||||
| rw_runlock(&pvh_global_lock); | rw_runlock(&pvh_global_lock); | ||||||||
| PMAP_UNLOCK(pmap); | PMAP_UNLOCK(pmap); | ||||||||
| } | } | ||||||||
| int | int | ||||||||
| pmap_fault(pmap_t pmap, vm_offset_t va, vm_prot_t ftype) | pmap_fault(pmap_t pmap, vm_offset_t va, vm_prot_t ftype) | ||||||||
Done Inline ActionsI would suggest making this look more like pmap_pte() on amd64 and arm64: return the PTE directly, or NULL if there is no valid PTE for the VA. Let the caller load the value. markj: I would suggest making this look more like `pmap_pte()` on amd64 and arm64: return the PTE… | |||||||||
Done Inline ActionsHaving _unlocked in the name is kind of strange, since the pmap will only be unlocked if it's the kernel pmap. I think pmap_fault_lookup() is fine. markj: Having _unlocked in the name is kind of strange, since the pmap will only be unlocked if it's… | |||||||||
| { | { | ||||||||
| pd_entry_t *l2, l2e; | pd_entry_t *l2, l2e; | ||||||||
| pt_entry_t bits, *pte, oldpte; | pt_entry_t bits, *pte, oldpte; | ||||||||
| int rv; | int rv; | ||||||||
| KASSERT(VIRT_IS_VALID(va), ("pmap_fault: invalid va %#lx", va)); | KASSERT(VIRT_IS_VALID(va), ("pmap_fault: invalid va %#lx", va)); | ||||||||
| rv = 0; | rv = 0; | ||||||||
| PMAP_LOCK(pmap); | PMAP_LOCK(pmap); | ||||||||
| l2 = pmap_l2(pmap, va); | l2 = pmap_l2(pmap, va); | ||||||||
| if (l2 == NULL || ((l2e = pmap_load(l2)) & PTE_V) == 0) | if (l2 == NULL || ((l2e = pmap_load(l2)) & PTE_V) == 0) | ||||||||
Done Inline ActionsThis comment could use more detail: we don't perform an sfence.vma after storing a PTE, so spurious faults are possible, and locking the kernel pmap is unsafe in this context; we might be in a critical section, or we might already hold the pmap lock. The pmap lock is a non-sleepable lock (it's just a mtx(9)), so the bit about a non-sleepable context doesn't quite make sense. (I realize that the witness message in the review description refers to the lock as a "sleep mutex", which is kind of confusing. That just means it isn't a spin mutex.) It's safe to look up the PTE without the pmap lock because kernel PTPs are always preallocated and never freed. markj: This comment could use more detail: we don't perform an sfence.vma after storing a PTE, so… | |||||||||
| goto done; | goto done; | ||||||||
| if ((l2e & PTE_RWX) == 0) { | if ((l2e & PTE_RWX) == 0) { | ||||||||
| pte = pmap_l2_to_l3(l2, va); | pte = pmap_l2_to_l3(l2, va); | ||||||||
| if (((oldpte = pmap_load(pte)) & PTE_V) == 0) | if (((oldpte = pmap_load(pte)) & PTE_V) == 0) | ||||||||
| goto done; | goto done; | ||||||||
| } else { | } else { | ||||||||
| pte = l2; | pte = l2; | ||||||||
Not Done Inline Actions
Maybe? mhorne: Maybe? | |||||||||
| oldpte = l2e; | oldpte = l2e; | ||||||||
| } | } | ||||||||
Done Inline ActionsIt's possible for PTE_D to be unset too: if an exec args buffer is paged out, the mappings will be marked clean. markj: It's possible for PTE_D to be unset too: if an exec args buffer is paged out, the mappings will… | |||||||||
| if ((pmap != kernel_pmap && (oldpte & PTE_U) == 0) || | if ((pmap != kernel_pmap && (oldpte & PTE_U) == 0) || | ||||||||
| (ftype == VM_PROT_WRITE && (oldpte & PTE_W) == 0) || | (ftype == VM_PROT_WRITE && (oldpte & PTE_W) == 0) || | ||||||||
| (ftype == VM_PROT_EXECUTE && (oldpte & PTE_X) == 0) || | (ftype == VM_PROT_EXECUTE && (oldpte & PTE_X) == 0) || | ||||||||
Done Inline ActionsThe comment is just restating the assertion. markj: The comment is just restating the assertion. | |||||||||
| (ftype == VM_PROT_READ && (oldpte & PTE_R) == 0)) | (ftype == VM_PROT_READ && (oldpte & PTE_R) == 0)) | ||||||||
| goto done; | goto done; | ||||||||
| bits = PTE_A; | bits = PTE_A; | ||||||||
Done Inline ActionsNow this assertion is effectively stating that the PTE has both the R and W bits set. Why should we assert that? What if the mapping is read-only? I don't think it's useful to assert anything in particular. I think the control flow can be simpler:
markj: Now this assertion is effectively stating that the PTE has both the R and W bits set. Why… | |||||||||
| if (ftype == VM_PROT_WRITE) | if (ftype == VM_PROT_WRITE) | ||||||||
| bits |= PTE_D; | bits |= PTE_D; | ||||||||
| /* | /* | ||||||||
| * Spurious faults can occur if the implementation caches invalid | * Spurious faults can occur if the implementation caches invalid | ||||||||
| * entries in the TLB, or if simultaneous accesses on multiple CPUs | * entries in the TLB, or if simultaneous accesses on multiple CPUs | ||||||||
| * race with each other. | * race with each other. | ||||||||
| */ | */ | ||||||||
| ▲ Show 20 Lines • Show All 514 Lines • ▼ Show 20 Lines | if (orig_l3 != 0) { | ||||||||
| pmap_invalidate_page(pmap, va); | pmap_invalidate_page(pmap, va); | ||||||||
| KASSERT(PTE_TO_PHYS(orig_l3) == pa, | KASSERT(PTE_TO_PHYS(orig_l3) == pa, | ||||||||
| ("pmap_enter: invalid update")); | ("pmap_enter: invalid update")); | ||||||||
| if ((orig_l3 & (PTE_D | PTE_SW_MANAGED)) == | if ((orig_l3 & (PTE_D | PTE_SW_MANAGED)) == | ||||||||
| (PTE_D | PTE_SW_MANAGED)) | (PTE_D | PTE_SW_MANAGED)) | ||||||||
| vm_page_dirty(m); | vm_page_dirty(m); | ||||||||
| } else { | } else { | ||||||||
| pmap_store(l3, new_l3); | pmap_store(l3, new_l3); | ||||||||
| if (pmap == kernel_pmap) { | |||||||||
| /* | |||||||||
| * Issue a TLB shootdown for 'va' to guard | |||||||||
| * against spurious kernel page faults on | |||||||||
| * implementations that cache invalid PTEs. | |||||||||
| */ | |||||||||
| pmap_invalidate_page(pmap, va); | |||||||||
Done Inline ActionsIf you're going with this approach, don't you need to do this on all harts? markj: If you're going with this approach, don't you need to do this on all harts? | |||||||||
Done Inline Actionshm, issuing a TLB shootdown is what I did initially until some premature optimization thoughts kicked in because that seemed a bit excessive. bnovkov: hm, issuing a TLB shootdown is what I did initially until some premature optimization thoughts… | |||||||||
Not Done Inline ActionsUnfortunately there are other places that need to be updated: pmap_enter_l2(), pmap_enter_quick(), ??? pmap_qenter() already handles this. Thinking about this some more, I suspect that the problems with dereferencing curthread on arm64 aren't going to arise even if you solve this problem with a lockless lookup for the kernel pmap in the fault handler: pmap_activate() issues sfence_vma(), so when a new thread first runs, it'll have already issued a fence, which should be enough. markj: Unfortunately there are other places that need to be updated: pmap_enter_l2(), pmap_enter_quick… | |||||||||
Not Done Inline ActionsI meant that doing a lockless lookup from pmap_fault() is probably better IMO than annotating all of the places where we install a kernel PTE. The latter seems to be rather tricky, see my other comments. markj: I meant that doing a lockless lookup from pmap_fault() is probably better IMO than annotating… | |||||||||
Done Inline Actions
Ah, sorry, I didn't get that from your initial comment, I'd also prefer a pmap_klookup() equivalent. bnovkov: > I meant that doing a lockless lookup from pmap_fault() is probably better IMO than annotating… | |||||||||
markjUnsubmitted Not Done Inline ActionsAs I understand it, pmap_invalidate_page() will just issue sfence.vma <va> on all harts, and this only invalidates cached translation structures for leaf page table entries, not higher level entries which may have been installed as part of this operation. markj: As I understand it, pmap_invalidate_page() will just issue `sfence.vma <va>` on all harts, and… | |||||||||
| } | } | ||||||||
| } | |||||||||
| #if VM_NRESERVLEVEL > 0 | #if VM_NRESERVLEVEL > 0 | ||||||||
| if (mpte != NULL && mpte->ref_count == Ln_ENTRIES && | if (mpte != NULL && mpte->ref_count == Ln_ENTRIES && | ||||||||
| (m->flags & PG_FICTITIOUS) == 0 && | (m->flags & PG_FICTITIOUS) == 0 && | ||||||||
| vm_reserv_level_iffullpop(m) == 0) | vm_reserv_level_iffullpop(m) == 0) | ||||||||
| (void)pmap_promote_l2(pmap, l2, va, mpte, &lock); | (void)pmap_promote_l2(pmap, l2, va, mpte, &lock); | ||||||||
| #endif | #endif | ||||||||
| ▲ Show 20 Lines • Show All 206 Lines • ▼ Show 20 Lines | pmap_enter_l2(pmap_t pmap, vm_offset_t va, pd_entry_t new_l2, u_int flags, | ||||||||
| if ((new_l2 & PTE_SW_WIRED) != 0) | if ((new_l2 & PTE_SW_WIRED) != 0) | ||||||||
| pmap->pm_stats.wired_count += L2_SIZE / PAGE_SIZE; | pmap->pm_stats.wired_count += L2_SIZE / PAGE_SIZE; | ||||||||
| pmap->pm_stats.resident_count += L2_SIZE / PAGE_SIZE; | pmap->pm_stats.resident_count += L2_SIZE / PAGE_SIZE; | ||||||||
| /* | /* | ||||||||
| * Map the superpage. | * Map the superpage. | ||||||||
| */ | */ | ||||||||
| pmap_store(l2, new_l2); | pmap_store(l2, new_l2); | ||||||||
| if (pmap == kernel_pmap && oldl2 == 0) | |||||||||
| pmap_invalidate_range(pmap, va, va + L2_SIZE); | |||||||||
| atomic_add_long(&pmap_l2_mappings, 1); | atomic_add_long(&pmap_l2_mappings, 1); | ||||||||
| CTR2(KTR_PMAP, "pmap_enter_l2: success for va %#lx in pmap %p", | CTR2(KTR_PMAP, "pmap_enter_l2: success for va %#lx in pmap %p", | ||||||||
| va, pmap); | va, pmap); | ||||||||
| return (KERN_SUCCESS); | return (KERN_SUCCESS); | ||||||||
| } | } | ||||||||
| ▲ Show 20 Lines • Show All 72 Lines • ▼ Show 20 Lines | |||||||||
| } | } | ||||||||
| static vm_page_t | static vm_page_t | ||||||||
| pmap_enter_quick_locked(pmap_t pmap, vm_offset_t va, vm_page_t m, | pmap_enter_quick_locked(pmap_t pmap, vm_offset_t va, vm_page_t m, | ||||||||
| vm_prot_t prot, vm_page_t mpte, struct rwlock **lockp) | vm_prot_t prot, vm_page_t mpte, struct rwlock **lockp) | ||||||||
| { | { | ||||||||
| struct spglist free; | struct spglist free; | ||||||||
| pd_entry_t *l2; | pd_entry_t *l2; | ||||||||
| pt_entry_t *l3, newl3; | pt_entry_t *l3, newl3, oldl3; | ||||||||
| KASSERT(!VA_IS_CLEANMAP(va) || | KASSERT(!VA_IS_CLEANMAP(va) || | ||||||||
| (m->oflags & VPO_UNMANAGED) != 0, | (m->oflags & VPO_UNMANAGED) != 0, | ||||||||
| ("pmap_enter_quick_locked: managed mapping within the clean submap")); | ("pmap_enter_quick_locked: managed mapping within the clean submap")); | ||||||||
| rw_assert(&pvh_global_lock, RA_LOCKED); | rw_assert(&pvh_global_lock, RA_LOCKED); | ||||||||
| PMAP_LOCK_ASSERT(pmap, MA_OWNED); | PMAP_LOCK_ASSERT(pmap, MA_OWNED); | ||||||||
| l2 = NULL; | l2 = NULL; | ||||||||
| ▲ Show 20 Lines • Show All 41 Lines • ▼ Show 20 Lines | if (va < VM_MAXUSER_ADDRESS) { | ||||||||
| l3 = VM_PAGE_TO_DMAP(mpte); | l3 = VM_PAGE_TO_DMAP(mpte); | ||||||||
| l3 = &l3[pmap_l3_index(va)]; | l3 = &l3[pmap_l3_index(va)]; | ||||||||
| } else { | } else { | ||||||||
| mpte = NULL; | mpte = NULL; | ||||||||
| l3 = pmap_l3(kernel_pmap, va); | l3 = pmap_l3(kernel_pmap, va); | ||||||||
| } | } | ||||||||
| if (l3 == NULL) | if (l3 == NULL) | ||||||||
| panic("pmap_enter_quick_locked: No l3"); | panic("pmap_enter_quick_locked: No l3"); | ||||||||
| if (pmap_load(l3) != 0) { | |||||||||
| oldl3 = pmap_load(l3); | |||||||||
| if (oldl3 != 0) { | |||||||||
| if (mpte != NULL) | if (mpte != NULL) | ||||||||
| mpte->ref_count--; | mpte->ref_count--; | ||||||||
| return (NULL); | return (NULL); | ||||||||
| } | } | ||||||||
| /* | /* | ||||||||
| * Enter on the PV list if part of our managed memory. | * Enter on the PV list if part of our managed memory. | ||||||||
| */ | */ | ||||||||
| ▲ Show 20 Lines • Show All 45 Lines • ▼ Show 20 Lines | if ((prot & VM_PROT_NO_PROMOTE) == 0 && | ||||||||
| /* | /* | ||||||||
| * If promotion succeeds, then the next call to this function | * If promotion succeeds, then the next call to this function | ||||||||
| * should not be given the unmapped PTP as a hint. | * should not be given the unmapped PTP as a hint. | ||||||||
| */ | */ | ||||||||
| if (pmap_promote_l2(pmap, l2, va, mpte, lockp)) | if (pmap_promote_l2(pmap, l2, va, mpte, lockp)) | ||||||||
| mpte = NULL; | mpte = NULL; | ||||||||
| } | } | ||||||||
| #endif | #endif | ||||||||
| if (pmap == kernel_pmap && oldl3 == 0) | |||||||||
markjUnsubmitted Not Done Inline ActionsHow can oldl3 be 0 here? markj: How can oldl3 be 0 here? | |||||||||
| pmap_invalidate_page(pmap, va); | |||||||||
| return (mpte); | return (mpte); | ||||||||
| } | } | ||||||||
| /* | /* | ||||||||
| * This code maps large physical mmap regions into the | * This code maps large physical mmap regions into the | ||||||||
| * processor address space. Note that some shortcuts | * processor address space. Note that some shortcuts | ||||||||
| * are taken, but the code works. | * are taken, but the code works. | ||||||||
| ▲ Show 20 Lines • Show All 1,747 Lines • Show Last 20 Lines | |||||||||
Don't you need a fence here too?