Page MenuHomeFreeBSD

RISC-V: copy the DTB to early KVA
ClosedPublic

Authored by mhorne on Sun, Mar 22, 6:01 PM.

Details

Reviewers
br
markj
Group Reviewers
riscv
Commits
rS359673: RISC-V: copy the DTB to early KVA
Summary

The location of the device-tree blob is passed to the kernel by the
previous booting stage (i.e. BBL or OpenSBI). Currently, we leave it
untouched and mark the 1MB of memory holding it as unavailable.

Instead, do what is done by other fake_preload_metadata() routines and
copy to the DTB to KVA space. This is more in line with what loader(8)
will provide us in the future, and it allows us to reclaim the hole in
physical memory.

Test Plan

Boot with OpenSBI and BBL

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mhorne created this revision.Sun, Mar 22, 6:01 PM
kp added a subscriber: kp.Tue, Mar 24, 9:02 AM

I've not tested it yet, but this does look correct to me.

sys/riscv/riscv/machdep.c
852 ↗(On Diff #69770)

Not your code, but I think this wants a KASSERT(i < nitems(fake_preload), ("fake_preload overflow")); or similar.

nick added a subscriber: nick.Tue, Mar 24, 4:02 PM

I haven't tested this, yet, either. It looks good though.

sys/riscv/riscv/machdep.c
844 ↗(On Diff #69770)

Rather than writing this as:

*(vm_offset_t *)&fake_preload[i++] = (vm_offset_t)lastaddr;
i += 1;

this might be clearer:

*(vm_offset_t *)&fake_preload[i] = (vm_offset_t)lastaddr;
i += sizeof(vm_offset_t) / sizeof(uint32_t);
mhorne updated this revision to Diff 69833.Tue, Mar 24, 5:05 PM

Address comments.

mhorne marked 2 inline comments as done.Tue, Mar 24, 5:07 PM
mhorne added inline comments.
sys/riscv/riscv/machdep.c
844 ↗(On Diff #69770)

Done. Note that earlier entries in this function do a similar thing, but it's not worth fixing up here and now.

markj added a subscriber: markj.Tue, Mar 24, 7:26 PM

The mapping of the DTB at MAX_KERNEL_ADDRESS - 4MB remains valid, doesn't it?

mhorne marked an inline comment as done.EditedTue, Mar 24, 8:31 PM

The mapping of the DTB at MAX_KERNEL_ADDRESS - 4MB remains valid, doesn't it?

Yes, the mapping remains. Is there any reason we would need to invalidate it? pmap_bootstrap() sets virtual_end = VM_MAX_KERNEL_ADDRESS - L2_SIZE, so the virtual address is free to be re-mapped later.

EDIT: actually, thinking about it more, there's nothing currently stopping this virtual address from being re-mapped if KVA were to grow large enough... and that might cause problems because ofw_fdt.c holds onto the DTB pointer.

markj added a comment.Wed, Mar 25, 3:24 PM

The mapping of the DTB at MAX_KERNEL_ADDRESS - 4MB remains valid, doesn't it?

Yes, the mapping remains. Is there any reason we would need to invalidate it? pmap_bootstrap() sets virtual_end = VM_MAX_KERNEL_ADDRESS - L2_SIZE, so the virtual address is free to be re-mapped later.

Right, that looks like a preexisting bug. It is just unlikely that a running system will consume enough KVA to hit that problem, since many kernel allocations will go through the direct map on riscv.

EDIT: actually, thinking about it more, there's nothing currently stopping this virtual address from being re-mapped if KVA were to grow large enough... and that might cause problems because ofw_fdt.c holds onto the DTB pointer.

Hmm, I don't quite follow. Isn't OFW using the new mapping "created" by fake_preload_metadata()?

As for leaving the old DTB mapping around: it may not be a problem most of the time, but there are times when you want to guarantee that there are no aliases exist for a given mapping of a physical page. It also may break pmap invariants. For instance, if something tries to pmap_enter() a page at the old DTB address, the ref_count value of the L2 PTP will end up being wrong because pmap_enter() does not need to adjust ref_count when updating a valid mapping, but pmap_bootstrap() doesn't appear to initialize ref_count properly for that PTP (presumably since it expects bootstrapped mappings to be permanent).

mhorne updated this revision to Diff 69931.Fri, Mar 27, 6:56 PM

Invalidate the original DTB mapping in pmap_bootstrap().

The mapping of the DTB at MAX_KERNEL_ADDRESS - 4MB remains valid, doesn't it?

Yes, the mapping remains. Is there any reason we would need to invalidate it? pmap_bootstrap() sets virtual_end = VM_MAX_KERNEL_ADDRESS - L2_SIZE, so the virtual address is free to be re-mapped later.

Right, that looks like a preexisting bug. It is just unlikely that a running system will consume enough KVA to hit that problem, since many kernel allocations will go through the direct map on riscv.

EDIT: actually, thinking about it more, there's nothing currently stopping this virtual address from being re-mapped if KVA were to grow large enough... and that might cause problems because ofw_fdt.c holds onto the DTB pointer.

Hmm, I don't quite follow. Isn't OFW using the new mapping "created" by fake_preload_metadata()?

Sorry, my wording was unclear. By "currently" I meant what you said above: that there is a preexisting bug.

As for leaving the old DTB mapping around: it may not be a problem most of the time, but there are times when you want to guarantee that there are no aliases exist for a given mapping of a physical page. It also may break pmap invariants. For instance, if something tries to pmap_enter() a page at the old DTB address, the ref_count value of the L2 PTP will end up being wrong because pmap_enter() does not need to adjust ref_count when updating a valid mapping, but pmap_bootstrap() doesn't appear to initialize ref_count properly for that PTP (presumably since it expects bootstrapped mappings to be permanent).

Got you, it should be fixed now. I could be wrong, but it seems like pmap_enter() only updates ref_count for userspace mappings?

kp added inline comments.Sat, Mar 28, 9:42 AM
sys/riscv/riscv/machdep.c
84 ↗(On Diff #69931)

I don't seem to have a metadata.h header for riscv. Am I missing another patch?

markj added a comment.Sat, Mar 28, 4:42 PM

As for leaving the old DTB mapping around: it may not be a problem most of the time, but there are times when you want to guarantee that there are no aliases exist for a given mapping of a physical page. It also may break pmap invariants. For instance, if something tries to pmap_enter() a page at the old DTB address, the ref_count value of the L2 PTP will end up being wrong because pmap_enter() does not need to adjust ref_count when updating a valid mapping, but pmap_bootstrap() doesn't appear to initialize ref_count properly for that PTP (presumably since it expects bootstrapped mappings to be permanent).

Got you, it should be fixed now. I could be wrong, but it seems like pmap_enter() only updates ref_count for userspace mappings?

Sorry, you're right.

sys/riscv/riscv/pmap.c
620 ↗(On Diff #69931)

You might pass this address from initriscv(), or define some constant for use both here and in locore.

621 ↗(On Diff #69931)

I would assert that the mapping is valid.

622 ↗(On Diff #69931)

Doesn't pmap_l2() return the PTE? That is, I'd expect to see pmap_clear(l2).

mhorne updated this revision to Diff 70210.Sat, Apr 4, 9:58 PM

Address markj's comments

mhorne marked 4 inline comments as done.Sat, Apr 4, 10:02 PM
mhorne added inline comments.
sys/riscv/riscv/machdep.c
84 ↗(On Diff #69931)

I missed adding it to the original submission, but it is included now.

markj accepted this revision as: markj.Mon, Apr 6, 3:38 PM

Looks good to me.

This revision is now accepted and ready to land.Mon, Apr 6, 3:38 PM
This revision was automatically updated to reflect the committed changes.
mhorne marked an inline comment as done.