-
Notifications
You must be signed in to change notification settings - Fork 210
Move to new os-boot. #889
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Move to new os-boot. #889
Conversation
I don't think we want to add another 23 MB ELF to the repo. Ideally we would add a script/Makefile that allows people to build it. We could also host a pre built version somewhere outside the repo and link to it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be great to get these images updated! Here are some initial comments. Likely more to follow.
os-boot/sail.dts
Outdated
@@ -14,10 +17,13 @@ | |||
reg = <0>; | |||
status = "okay"; | |||
compatible = "riscv"; | |||
riscv,isa = "rv64imac"; | |||
riscv,isa = "rv64imafd"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're updating the device tree, we might as well use the full list of supported extensions in the default config. That will exercise the widest variety of instructions, and therefore be a better test for the model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd still like to see this fixed. Not sure why this was marked as resolved. Maybe something like this instead:
riscv,isa-base = "rv64i";
riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "v", "sstc", "svinval", "zba", "zbb", "zbc", "zbs", "zca", "zcb", "zcd", "zfa", "zfh", "zkn", "zkt", "zicbom", "zicboz", "zicntr", "zicond", "zicsr", "zifencei", "zihpm";
riscv,cboz-block-size = <64>;
riscv,cbom-block-size = <64>;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep riscv,isa though, FreeBSD does not currently recognise the newer form
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Arielfoever This needs to be switched to use the longer list of extensions in the older riscv,isa
format.
I will make a makefile and a CI to build it. How's this idea? |
Sounds good to me. I think we probably don't need to add it to PR CIs but I might be nice to have a manually triggered workflow. |
Makefile will be in https://github.com/Arielfoever/sail-riscv master branch because I need to check CI. I will rebase my work if everything's fine. |
Finished it with a Makefile and a manually triggered CI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. Makefile needs a bit of work (blame me!). How did you resolve the ET_DYN thing? Or does it just not happen for some reason...
os-boot/Makefile
Outdated
sail: sail.dtb | ||
../build/c_emulator/riscv_sim_rv64d --no-trace -p -l $(LIMIT_INSTRUCTIONS) --device-tree-blob sail.dtb -t /tmp/console.log build/opensbi-$(OPENSBI_VERSION)/build/platform/generic/firmware/fw_payload.elf | ||
|
||
# QEMU attempt. Not sure I ever got this working. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete this attempt! (Or this comment if it actually works.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this qemu is work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you wish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this qemu is work
Do you mean that command does work? If so that's great - let's leave it in! (And remove the comment about it not working.)
os-boot/Makefile
Outdated
# Number of instructions to run. | ||
LIMIT_INSTRUCTIONS ?= 250000 | ||
|
||
linux: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice if we made the dependency tree work. E.g. this should depend build/linux-$(LINUX_VERSION)/Makefile
and the download_linux
rule should be
build/linux-$(LINUX_VERSION)/Makefile:
mkdir -p build
...
I can do it if you like when the other comments are resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will do it after no one objects in other comments after I finish my roommate's birthday lunch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like it is not (easily) possible to download the generated artifact from the command line (actions/upload-artifact#89), so we probably need to figure out a different place for the final upload.
Also, should we rename all of this to just linux
instead of os-boot
since this is removing all of the other OSs? At least the workflow file.
renamed CI as you wish. |
72eb8ee
to
d66a750
Compare
Everything's up-to-date. See https://github.com/Arielfoever/sail-riscv/actions/runs/14734807045 |
os-boot/sail.dts
Outdated
@@ -14,10 +17,13 @@ | |||
reg = <0>; | |||
status = "okay"; | |||
compatible = "riscv"; | |||
riscv,isa = "rv64imac"; | |||
riscv,isa = "rv64imafd"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd still like to see this fixed. Not sure why this was marked as resolved. Maybe something like this instead:
riscv,isa-base = "rv64i";
riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "v", "sstc", "svinval", "zba", "zbb", "zbc", "zbs", "zca", "zcb", "zcd", "zfa", "zfh", "zkn", "zkt", "zicbom", "zicboz", "zicntr", "zicond", "zicsr", "zifencei", "zihpm";
riscv,cboz-block-size = <64>;
riscv,cbom-block-size = <64>;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth actually booting Linux on Sail in this CI job to make sure everything worked correctly before uploading it.
b74016d
to
21747c2
Compare
Test at last step of CI. |
Looks like we need to install dtc in the CI? |
Clarity is better than terseness in scripts.
Unfortunately the riscv-collab toolchains require a really really recent (2021) Glibc version which is quite annoying for everyone working with commercial RISC-V designs who are stuck on ancient RHEL versions.
It's really easy to forget .PHONY if they are all at the top.
The source actually ends up in build so this is a bit clearer.
?= is unnecessary since normal := can already be overridden.
It goes to stdout by default.
More consistent and clearer
Update paths and hash .url files
I think ultimately the goal should be that Sail supports PLIC so we can have a proper UART, and then the emulator can support interactive input (depending on how patient you are). klibc sounds like a good intermediate step if anyone wants to try it (I don't plan to). @Arielfoever I pushed a load of changes to this. Hope that's ok. It has a vaguely reasonable history and I tried to put justification in the commit messages. Seems pretty good to me now. I haven't looked into the CI flow in detail yet though; I'll do that next.
Oops that's embarassing. Thanks for figuring it out @arichardson |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Workflow looks reasonable to me. It's quite complicated so I think we can avoid caching the Sail model there at least.
It would also be great if we could run Linux at least for a few million instructions in normal PR CI, given how crap our current testing is.
That can be a future PR though.
|
||
- name: Check Sail Model ID | ||
id: sail-id | ||
run: echo "commitid=$(git log --pretty=format:"%H" -1 -- . ':(exclude)os-boot' ':(exclude).github')" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I would agree. I understand caching Linux since it takes a while to build and isn't likely to change often, but the Sail model is pretty quick to build and changes rapidly.
- name: Test ELF File | ||
working-directory: os-boot/linux | ||
timeout-minutes: 2 | ||
run: make sail 2>&1 | tee linux.log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess since we're saving it to a log we could hackily grep it for some pattern here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we could run it until it crashes and grep for the expected error when it fails to find an init program. Once we eventually add some form of user space that could be changed, but this would be a good test to start with.
Hi guys, how about make it part by part? we first boot a openSBI only then we can see something like this and merge. Then we deal with linux or something. [ariel@archlinux linux]$ ../../build/c_emulator/riscv_sim_rv64d --no-trace -p --device-tree-blob sail.dtb -t /dev/stdout build/fw_payload.elf
will show execution times on completion.
using sail.dtb as DTB file.
using /dev/stdout for terminal output.
Read 1391 bytes of DTB from sail.dtb.
Running file build/fw_payload.elf.
ELF Entry @ 0x80000000
tohost located at 0x80042c20
kips: 51
OpenSBI v1.6
____ _____ ____ _____
/ __ \ / ____| _ \_ _|
| | | |_ __ ___ _ __ | (___ | |_) || |
| | | | '_ \ / _ \ '_ \ \___ \| _ < | |
| |__| | |_) | __/ | | |____) | |_) || |_
\____/| .__/ \___|_| |_|_____/|____/_____|
| |
|_|
kips: 51
kips: 51
kips: 51
Platform Name : ucbbar,spike-bare
Platform Features : medeleg
Platform HART Count : 1
Platform IPI Device : aclint-mswi
Platform Timer Device : aclint-mtimer @ 10000000Hz
Platform Console Device : htif
Platform HSM Device : ---
Platform PMU Device : ---
Platform Reboot Device : htif
Platform Shutdown Device : htif
Platform Suspend Device : ---
Platform CPPC Device : ---
Platform Suspend Device : --- 23:44:47 [88/888]Platform CPPC Device : ---
Firmware Base : 0x80000000
Firmware Size : 325 KB
Firmware RW Offset : 0x40000
Firmware RW Size : 69 KB
Firmware Heap Offset : 0x48000
Firmware Heap Size : 37 KB (total), 2 KB (reserved), 12 KB (used), 22 KB (free)
Firmware Scratch Size : 4096 B (total), 432 B (used), 3664 B (free)
Runtime SBI Version : 2.0
Standard SBI Extensions : time,rfnc,ipi,base,hsm,srst,pmu,dbcn,legacy
Experimental SBI Extensions : fwft,sse
Domain0 Name : root
Domain0 Boot HART : 0
Domain0 HARTs : 0*
Domain0 Region00 : 0x0000000000000000-0x0000000000000kips: 51
fff M: (I,R,W) S/U: (R,W)
Domain0 Region01 : 0x0000000080040000-0x000000008005ffff M: (R,W) S/U: ()
Domain0 Region02 : 0x0000000002080000-0x00000000020bffff M: (I,R,W) S/U: ()
Domain0 Region03 : 0x0000000080000000-0x000000008003ffff M: (R,X) S/U: ()
Domain0 Region04 : 0x0000000002000000-0x000000000207ffff M: (I,R,W) S/U: ()
Domain0 Region05 : 0x0000000000000000-0xffffffffffffffff M: () S/U: (R,W,X)
Domain0 Next Address : 0x0000000080200000
Domain0 Next Arg1 : 0x0000000082200000
Domain0 Next Mode : S-mode
Domain0 SysReset : yes
Domain0 SysSuspend : yes
Boot HART ID : 0
Boot HART Domain : root
Boot HART Priv Version : v1.12
Boot HART Base ISA : rv64imafdcbv
Boot HART ISA Extensions : sscofpmf,sstc,zicntr,zihpm,smcntrpmf,zicboz,zicbom,sdtrig
Boot HART PMP Count : 16
Boot HART PMP Granularity : 2 bits
Boot HART PMP Address Bits : 54
Boot HART MHPM Info : 29 (0xfffffff8)
Boot HART Debug Triggers : 0 triggers
Boot HART MIDELEG : 0x0000000000002222
Boot HART MEDELEG : 0x000000000004b109
Test payload running |
I'd suggest that we put in agenda @Timmmm |
Hi all, I have edited some config and now linux kernel works as same as qemu.
|
Is this the log from QEMU or Sail? If it's Sail, why is it finding the Sdtrig extension? Sail doesn't support that extension yet. |
From sail of course. |
Fix #580