Page MenuHomeFreeBSD
Differential D24912

Handle load from loader(8)
ClosedPublic
Actions

Authored by mhorne on May 19 2020, 1:18 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 20, 12:47 AM
Unknown Object (File)
Oct 1 2024, 7:22 AM
Unknown Object (File)
Sep 27 2024, 11:42 PM
Unknown Object (File)
Sep 23 2024, 11:31 PM
Unknown Object (File)
Sep 17 2024, 9:42 PM
Unknown Object (File)
Sep 11 2024, 5:46 AM
Unknown Object (File)
Sep 7 2024, 8:50 AM
Unknown Object (File)
Sep 4 2024, 6:42 AM
View All 74 Files
Subscribers
imp
nick

Details

Reviewers
br
markj
jrtc27
Group Reviewers
riscv
Commits
rS362583: Handle load from loader(8)
Summary

In locore, we must detect and handle different arguments passed by
loader(8) compared to what we have when booting directly via SBI
firmware. Currently we receive the hart ID in a0 and a pointer to the
device tree blob in a1. loader(8) provides only a pointer to its
metadata.

The only tricky part to this is that the hart ID of the boot hart is
expected to have been stored in the device tree by a previous booting
stage. This is performed by u-boot v2020.07 and onwards, but without
this we must fail to boot as there is no way for a hart to obtain its
own ID while executing in supervisor mode.

Fix-up initriscv() to parse the loader's metadata, continuing to use
fake_preload_metadata() in the direct boot case.

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 31173
Build 28836: arc lint + arc unit

Event Timeline

mhorne requested review of this revision.May 19 2020, 1:18 AM
mhorne created this revision.
Harbormaster completed remote builds in B31172: Diff 71976.May 19 2020, 1:18 AM
mhorne updated this revision to Diff 71977.May 19 2020, 1:23 AM

Remove a duplicated section of code from a mis-merge.

Harbormaster completed remote builds in B31173: Diff 71977.May 19 2020, 1:23 AM
jrtc27 added a comment.May 19 2020, 1:38 AM

There is no guarantee that mhartid does not collide with the kernel's VA space. Unlikely, yes (especially since the spec encourages reducing the magnitude of the largest mhartid), but completely permitted. The only restriction on mhartid is this:

Hart IDs might not necessarily be numbered contiguously in a multiprocessor system, but at least one hart must have a hart ID of zero.

Why can we not make loader conform to the standard RISC-V boot interface, putting mhartid in a0 and the dtb address in a1? It can shove the address of modulep somewhere in the mangled FDT. I really don't like the idea of guessing whether an integer is an address or not. Alternatively, we could provide a different entry point for loader, if it really needs the current interface.

mhorne added a comment.EditedMay 19 2020, 1:54 AM
In D24912#548332, @jrtc27 wrote:

There is no guarantee that mhartid does not collide with the kernel's VA space. Unlikely, yes (especially since the spec encourages reducing the magnitude of the largest mhartid), but completely permitted. The only restriction on mhartid is this:

Hart IDs might not necessarily be numbered contiguously in a multiprocessor system, but at least one hart must have a hart ID of zero.

Why can we not make loader conform to the standard RISC-V boot interface, putting mhartid in a0 and the dtb address in a1? It can shove the address of modulep somewhere in the mangled FDT. I really don't like the idea of guessing whether an integer is an address or not. Alternatively, we could provide a different entry point for loader, if it really needs the current interface.

Providing modulep as the first and only argument seems to be standardized across the various loader(8) implementations, although there may be an example that differs. However, u-boot does not provide the boot hart ID as an argument to EFI payloads (nor, as I discovered, simple payloads invoked with the "go" command). The solution of stuffing the ID in the device tree and reading it later is what Linux plans on using for its EFI implementation.

Admittedly I don't love the check against the address either. I can see about providing an alternate entry point, but a hart id > 0xffffffc000000000 is an edge-case I'm not all that worried about.

jrtc27 added a comment.May 19 2020, 1:59 AM

https://lists.denx.de/pipermail/u-boot/2020-February/401563.html suggests Linux is going the way of an alternate entry point, FWIW.

nick added a subscriber: nick.May 20 2020, 6:57 AM
mhorne mentioned this in D25034: riscv: honor the environment set via the kernel config file.May 28 2020, 7:32 PM
mhorne updated this revision to Diff 72493.May 31 2020, 11:48 PM

Okay, I've reworked this a bit and I am happier with it. Now, CPUs will enter the _alt_start routine from SBI firmware (placed first in .text), while loader(8) will continue to read the ELF entry point and send the boot CPU to _start. With this version there is no guesswork involved as to which way we booted, and both routines can perform their own fixups before entering the common page table setup.

mhorne added inline comments.May 31 2020, 11:52 PM
sys/riscv/riscv/locore.S
84

Note that I've dropped the #ifdef SMP requirement for storing to boot_hart as I believe it is better that the boot CPU always have access to its hart ID. The PLIC driver requires it, for example, and could cause issues if a !SMP system was running on a hart != 0.

mhorne updated this revision to Diff 72829.Jun 8 2020, 3:45 PM

Rebase.

Harbormaster completed remote builds in B31565: Diff 72829.Jun 8 2020, 3:45 PM
mhorne added a comment.Jun 18 2020, 8:43 PM

Ping. @jrtc27 do you have any thoughts on this version?

jrtc27 added a comment.Jun 18 2020, 8:50 PM

I don't know the details of loader, but this looks like a much better approach and doesn't perturb the SBI paths much so I'm happy.

markj added inline comments.Jun 24 2020, 2:01 PM
sys/riscv/riscv/machdep.c
886

In this line both boot_hart and hart are uint32_t, so the cast doesn't really make sense.

We seem to consistently use uint32_t for hart IDs (e.g., boot_hart, pc_hart), but per Jessica's initial point there is no apparent requirement that the IDs fit in 32 bits on RV64.

markj accepted this revision as: markj.Jun 24 2020, 2:07 PM

Looks ok to me modulo the one comment about the cast. I'm fine with committing this if the cast is dropped, since the issue around the type used for hart IDs is preexisting.

This revision is now accepted and ready to land.Jun 24 2020, 2:07 PM
mhorne added a comment.Jun 24 2020, 2:22 PM
In D24912#561272, @markj wrote:

Looks ok to me modulo the one comment about the cast. I'm fine with committing this if the cast is dropped, since the issue around the type used for hart IDs is preexisting.

Will do. We should probably convert hart ID types to u_long in the future, but there is no motivator for this at the moment.

sys/riscv/riscv/machdep.c
886

I think it was left from an earlier iteration where I converted boot_hart to a u_long. Thanks for catching this.

Closed by commit rS362583: Handle load from loader(8) (authored by mhorne). · Explain WhyJun 24 2020, 3:20 PM
This revision was automatically updated to reflect the committed changes.
mhorne added a commit: rS362583: Handle load from loader(8).
Herald added a subscriber: imp. · View Herald TranscriptJun 24 2020, 3:20 PM
jrtc27 mentioned this in D25544: riscv: look for bootargs in FDT.Jul 2 2020, 2:21 AM

Revision Contents
Changeset List

PathSize
sys/
riscv/
include/
2 lines
riscv/
1 line
38 lines
85 lines

Diff 71977

View Options

sys/riscv/include/machdep.h

Loading...
View Options

sys/riscv/riscv/genassym.c

Loading...
View Options

sys/riscv/riscv/locore.S

Loading...
View Options

sys/riscv/riscv/machdep.c

Loading...