Details

Reviewers

andrew
markj
manu

Commits

rG5ee5c40402c9: arm64 pmap: Defer bti lookup

Summary

Defer the bti lookup until after page table page allocation is complete.
We sometimes release the pmap lock and sleep during page table page
allocation. Consequently, the result of a bti lookup from before
page table page allocation could be stale when we finally create the
mapping based on it.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

alc created this revision.Wed, Jun 5, 6:56 PM

Herald added a reviewer: manu. · View Herald TranscriptWed, Jun 5, 6:56 PM

Herald added a subscriber: emaste. · View Herald Transcript

alc requested review of this revision.Wed, Jun 5, 6:56 PM

markj added inline comments.Wed, Jun 5, 9:55 PM

sys/arm64/arm64/pmap.c
5749	I note that we don't do anything in the `ADDR_IS_KERNEL(va)` case. That's ok because the sole caller has already ORed the BTI value into the PTE and we don't sleep in that case. In the `!ADDR_IS_KERNEL(va)` case, while sleeping the BTI entry covering this VA may have been removed, in which case `pmap_pte_bti()` will return 0. But if the caller had already set the GP bit in the PTE, we don't clear it here.

alc marked an inline comment as done.Thu, Jun 6, 6:44 PM

alc added inline comments.

sys/arm64/arm64/pmap.c
5749	Regarding your first comment, no, that is an error. This version is failing to set GP on kernel mappings. Regarding your second comment, when we get here, no one has yet set GP in this PTE.

Correct a couple errors in the previous version.

pmap_bti_same() can easily update the PTE at the same time as checking the address range.

markj added inline comments.Fri, Jun 7, 12:45 AM

sys/arm64/arm64/pmap.c
5749	Sorry, I'm missing something. pmap_enter_l3c()'s sole caller is pmap_enter_l3c_rx(), which sets `l3e \|= pmap_pte_bti(pmap, va);` and passes the PTE as a parameter.

alc marked an inline comment as done.Fri, Jun 7, 6:48 AM

alc added inline comments.

sys/arm64/arm64/pmap.c
5749	You're absolutely right.

Remove two early calls to pmap_pte_bti() that are now redundant.

markj accepted this revision.Fri, Jun 7, 1:18 PM

markj added inline comments.

sys/arm64/arm64/pmap.c
9291	Do we want to assert that GP is not already set in the PTE?

This revision is now accepted and ready to land.Fri, Jun 7, 1:18 PM

Add KASSERT.

This revision now requires review to proceed.Fri, Jun 7, 6:14 PM

By the way, you mentioned pmap_enter_l3c() only being called from one place. The next big chunk of @ehs3_rice.edu 's work will include a call to pmap_enter_l3c() from pmap_enter() with pagesizes[] updated to include the L3C page size as psind == 1.

alc added inline comments.Fri, Jun 7, 6:34 PM

sys/arm64/arm64/pmap.c
5548	While working on this function, I realized that we missed something when reviewing this code: We are not releasing the PTP reference when failing here and below. No?

markj added a subscriber: bnovkov.Fri, Jun 7, 7:01 PM

markj added inline comments.

sys/arm64/arm64/pmap.c
5548	Yes, I believe you're right. We should be calling pmap_abort_ptp(l2pg) in these paths. The other pmaps have the same bug. @bnovkov do you have some time to write a patch for this? I can work on it soon if not.