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 30138
Build 27944: arc lint + arc unit

Event Timeline

mhorne created this revision.Mar 22 2020, 6:01 PM

Harbormaster completed remote builds in B30043: Diff 69770.Mar 22 2020, 6:01 PM

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

Address comments.

Harbormaster completed remote builds in B30085: Diff 69833.Mar 24 2020, 5:05 PM

mhorne marked 2 inline comments as done.Mar 24 2020, 5:07 PM

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?

In D24152#531702, @markj wrote:

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.

In D24152#531715, @mhorne wrote:

In D24152#531702, @markj wrote:

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 mentioned this in D24154: RISC-V: use physmem to manage physical memory.Mar 27 2020, 5:42 PM

mhorne added a child revision: D24154: RISC-V: use physmem to manage physical memory.

Invalidate the original DTB mapping in pmap_bootstrap().

Harbormaster completed remote builds in B30138: Diff 69931.Mar 27 2020, 6:56 PM

In D24152#531814, @markj wrote:

In D24152#531715, @mhorne wrote:

In D24152#531702, @markj wrote:

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.Mar 28 2020, 9:42 AM

sys/riscv/riscv/machdep.c
84	I don't seem to have a metadata.h header for riscv. Am I missing another patch?

In D24152#532437, @mhorne wrote:

In D24152#531814, @markj wrote:

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	You might pass this address from initriscv(), or define some constant for use both here and in locore.
621	I would assert that the mapping is valid.
622	Doesn't pmap_l2() return the PTE? That is, I'd expect to see pmap_clear(l2).

Address markj's comments

Harbormaster completed remote builds in B30292: Diff 70210.Apr 4 2020, 9:58 PM

mhorne marked 4 inline comments as done.Apr 4 2020, 10:02 PM

mhorne added inline comments.

sys/riscv/riscv/machdep.c
84	I missed adding it to the original submission, but it is included now.

Looks good to me.

This revision is now accepted and ready to land.Apr 6 2020, 3:38 PM

Closed by commit rS359673: RISC-V: copy the DTB to early KVA (authored by mhorne). · Explain WhyApr 6 2020, 10:48 PM

This revision was automatically updated to reflect the committed changes.

mhorne marked an inline comment as done.

mhorne added a commit: rS359673: RISC-V: copy the DTB to early KVA.

Herald added a subscriber: imp. · View Herald TranscriptApr 6 2020, 10:48 PM

RISC-V: copy the DTB to early KVA
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 69931

sys/riscv/include/machdep.h

sys/riscv/riscv/genassym.c

sys/riscv/riscv/locore.S

sys/riscv/riscv/machdep.c

sys/riscv/riscv/pmap.c

RISC-V: copy the DTB to early KVAClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 69931

sys/riscv/include/machdep.h

sys/riscv/riscv/genassym.c

sys/riscv/riscv/locore.S

sys/riscv/riscv/machdep.c

sys/riscv/riscv/pmap.c

RISC-V: copy the DTB to early KVA
ClosedPublic
Actions

Revision Contents
Changeset List