Details

Reviewers

manu
markj
kib

Group Reviewers

arm64

Commits

rGf64329bcdc4f: Extract the logic from pmap_kextract

Summary

This allows us to use it when we only need to check if the virtual address
is valid. For example when checking if an address in the DMAP region is
mapped.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 36219
Build 33108: arc lint + arc unit

Event Timeline

andrew created this revision.Dec 15 2020, 2:57 PM

Herald added a reviewer: manu. · View Herald TranscriptDec 15 2020, 2:57 PM

Herald added subscribers: emaste, imp. · View Herald Transcript

andrew requested review of this revision.Dec 15 2020, 2:57 PM

Harbormaster completed remote builds in B35417: Diff 80722.Dec 15 2020, 2:57 PM

andrew mentioned this in D27528: Fix /dev/mem access lock up.Dec 15 2020, 2:58 PM

This is untested, I just moved the code to a new function as a proof of concept.

andrew added reviewers: arm64, markj, kib.Jan 15 2021, 10:12 AM

Fix the build and boot tested on N1SDP

Harbormaster completed remote builds in B36219: Diff 82308.Jan 15 2021, 10:13 AM

I was always envy for aarch64 AT instructions. AT S1E0R (or W) seems to do exactly what you want, and avoid manual parsing of the page tables. Why not use the instruction?

Use an AT instruction with a fallback on walking the page table.

Harbormaster completed remote builds in B36221: Diff 82310.Jan 15 2021, 11:22 AM

kib added inline comments.Jan 15 2021, 11:36 AM

sys/arm64/arm64/pmap.c
1381	Why do we need to disable interrupts around AT ?

andrew added inline comments.Jan 15 2021, 11:42 AM

sys/arm64/arm64/pmap.c
1381	It's two instructions. The at instruction stores the result in par_el1 so we then need the following to get the result in `x1`: at s1e1r, x0 mrs x1, par_el1 If a interrupt happens after the `at`, but before the `mrs` we may have an invalid value in `par_el`, e.g. if the thread is moved to a new CPU. We could handle this by restarting the instruction, however disabling interrupts on arm64 is cheap so there is no need.

kib accepted this revision.Jan 15 2021, 12:42 PM

kib added inline comments.

sys/arm64/arm64/pmap.c
1381	May be add a comment there noting that operation is not atomic?

This revision is now accepted and ready to land.Jan 15 2021, 12:42 PM

BTW would it make sense to use AT in pmap_extract()?

Add a comment explaining why interrupts are disabled

This revision now requires review to proceed.Jan 15 2021, 3:10 PM

Harbormaster completed remote builds in B36222: Diff 82314.Jan 15 2021, 3:10 PM

markj added inline comments.Jan 15 2021, 3:16 PM

sys/arm64/arm64/pmap.c
1368–1375	Extra newline.
1381	Or perhaps disable interrupts in the implementation rather than the caller.
6861	I think pmap_klookup() might be a better name. kva_to_pa is a bit confusing since 1) translating KVAs to PAs is ostensibly pmap_kextract()'s job, and 2) the function is used here only to see if the address is valid.

I expect we could do something similar in pmap_extract. We would need to limit it to stage 1 pmaps (i.e. non-hypervisor pmaps), and need to check if it's a userspace or kernel pmap.

It looks like we would only need the page table walking code for stage 2 pmaps as we only modify them with the pmap lock that pmap_extract holds.

Update based on feedback from markj

Harbormaster completed remote builds in B36225: Diff 82317.Jan 15 2021, 4:13 PM

markj accepted this revision.Jan 15 2021, 4:25 PM

This revision is now accepted and ready to land.Jan 15 2021, 4:25 PM

Closed by commit rGf64329bcdc4f: Extract the logic from pmap_kextract (authored by andrew). · Explain WhyJan 15 2021, 7:14 PM

This revision was automatically updated to reflect the committed changes.

andrew added a commit: rGf64329bcdc4f: Extract the logic from pmap_kextract.