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. |
jordancarlin
left a comment
There was a problem hiding this comment.
Will be great to get these images updated! Here are some initial comments. Likely more to follow.
os-boot/sail.dts
Outdated
| status = "okay"; | ||
| compatible = "riscv"; | ||
| riscv,isa = "rv64imac"; | ||
| riscv,isa = "rv64imafd"; |
There was a problem hiding this comment.
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.
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.
Please keep riscv,isa though, FreeBSD does not currently recognise the newer form
There was a problem hiding this comment.
@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. |
Timmmm
left a comment
There was a problem hiding this comment.
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.
Delete this attempt! (Or this comment if it actually works.)
There was a problem hiding this comment.
this qemu is work.
There was a problem hiding this comment.
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.
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.
I will do it after no one objects in other comments after I finish my roommate's birthday lunch.
jordancarlin
left a comment
There was a problem hiding this comment.
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
| status = "okay"; | ||
| compatible = "riscv"; | ||
| riscv,isa = "rv64imac"; | ||
| riscv,isa = "rv64imafd"; |
There was a problem hiding this comment.
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>;
jordancarlin
left a comment
There was a problem hiding this comment.
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? |
Agreed that it makes sense to merge the Linux boot without adding it to CI as a first step. I do wonder if it’s worth converting to CMake though to be consistent with the rest of our build system. |
|
I removed the CI flow from this PR. I think lets just merge this first and then we can rebase your other PR (should be easy). |
Yeah you're probably right... I think we should just get this in though and do that later if anyone can be bothered. |
|
A few minor comments, but I agree it would be great to land this makefile infrastructure to make it easier to test. |
|
Updated & rebased. I squashed the commits too since they were getting a bit numerous... |
|
Is it getting to Linux now? Last I checked, it booted OpenSBI and then appeared to get stuck. |
|
Ah yeah it was booting but the console output wasn't showing because the DTS was missing the |
|
I think everything's work. |
Add missing "chosen" section to device tree Signed-off-by: Ariel Xiong <ArielHeleneto@outlook.com> Co-authored-by: Tim Hutt <tdhutt@gmail.com>
jordancarlin
left a comment
There was a problem hiding this comment.
One minor comment, but otherwise LGTM and will be great to finally get this in.
Co-authored-by: Jordan Carlin <jordanmcarlin@gmail.com> Signed-off-by: Tim Hutt <tdhutt@gmail.com>
Remove old pre-built Linux images and add a Makefile to build one from scratch, including downloading all the tools.
This Makefile also includes a target for profiling using gperftools.
Fixes #580