Page MenuHomeFreeBSD

RISC-V: copy the DTB to early KVA
ClosedPublic

Authored by mhorne on Mar 22 2020, 6:01 PM.
Tags
None
Referenced Files
F80196611: D24152.diff
Fri, Mar 29, 3:09 AM
Unknown Object (File)
Jan 24 2024, 7:02 AM
Unknown Object (File)
Jan 16 2024, 4:09 PM
Unknown Object (File)
Jan 15 2024, 12:05 PM
Unknown Object (File)
Dec 20 2023, 2:11 AM
Unknown Object (File)
Dec 15 2023, 7:41 PM
Unknown Object (File)
Dec 14 2023, 7:53 AM
Unknown Object (File)
Dec 1 2023, 10:23 AM
Subscribers

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

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 30085
Build 27894: arc lint + arc unit

Event Timeline

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

sys/riscv/riscv/machdep.c
852–854

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

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

sys/riscv/riscv/machdep.c
844

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 added inline comments.
sys/riscv/riscv/machdep.c
844

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

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

mhorne marked an inline comment as done.EditedMar 24 2020, 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.

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).

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?

sys/riscv/riscv/machdep.c
84

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

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 added inline comments.
sys/riscv/riscv/machdep.c
84

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

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