Skip to content
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

riscv_sim: Add command line arguments to enable or disable use of boot ROM. #199

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rmn30
Copy link
Collaborator

@rmn30 rmn30 commented Jan 10, 2023

riscv_sim: if NO_BOOT_ROM is defined then don't use a boot ROM at the reset vector, but set initial PC to elf_entry.

This provides a compilation option to avoid using the small hardcoded boot ROM at the reset vector and instead start execution directly from the the entry point specified by the ELF file.

@abukharmeh
Copy link
Contributor

I think this could be very useful for normal usage, why is it not a run time option ?

@rmn30
Copy link
Collaborator Author

rmn30 commented Jan 10, 2023

I could have made it a run time option but decided that whether you want it or not might depend on the emulator you are building more than on the particular invocation. This way means you don't have to specify a command line argument every time you run the emulator if you never want the boot ROM.

I could make it a command line option but have a compilation option to change what the default is? I thought I'd better keep the default as using the boot ROM to maintain backwards compatibiltiy.

@jrtc27
Copy link
Collaborator

jrtc27 commented Jan 10, 2023

What’s the purpose? In effect the boot ROM is just a software implementation of setting a specific reset state?

@rmn30
Copy link
Collaborator Author

rmn30 commented Jan 10, 2023

I'm not sure but I suspect it was for compatibility with Spike.

@github-actions

This comment has been minimized.

@github-actions
Copy link

github-actions bot commented Jan 10, 2023

Unit Test Results

712 tests  ±0   712 ✔️ ±0   0s ⏱️ ±0s
    6 suites ±0       0 💤 ±0 
    1 files   ±0       0 ±0 

Results for commit 871218e. ± Comparison against base commit 4d05aa1.

♻️ This comment has been updated with latest results.

…t ROM.

This adds command line arguments --boot-rom and --no-boot-rom to
enable or disable the use of the boot ROM at the reset vector.  The
current behaviour (--boot-rom) initialises a small bit of memory at
the reset vector to set up argument registers and then jump to the
address specified by entry point of the ELF file.  With --no-boot-rom
the simulator just sets the initial PC to the entry point.  For
backwards compatibility the default is to use the boot-rom unless
NO_BOOT_ROM is defined during compilation.
@rmn30
Copy link
Collaborator Author

rmn30 commented Jan 11, 2023

Any thoughts on this revised version with command line arguments?

@jrtc27 jrtc27 changed the title riscv_sim: if NO_BOOT_ROM is defined then don't use a boot ROM riscv_sim: Add command line arguments to enable or disable use of boot ROM. Jan 11, 2023
@jrtc27
Copy link
Collaborator

jrtc27 commented Jan 11, 2023

I still don't understand why it's needed, given the only side-effect is what state a handful of GPRs reset in and that's entirely undefined by the ISA. And I guess also instret. I also really don't like having a configurable default, that's how you end up with confusion and things working on one machine but not another.

@billmcspadden-riscv
Copy link
Collaborator

billmcspadden-riscv commented Jan 11, 2023 via email

@jrtc27
Copy link
Collaborator

jrtc27 commented Jan 11, 2023

If you stop providing a baked-in boot ROM like you suggest and force the user to explicitly provide one then you're going to make it a total pain to use the model for running real software, which expects the de-facto entry state of a0 == hartid, a1 == dtb_addr.

@allenjbaum
Copy link
Collaborator

allenjbaum commented Jan 11, 2023 via email

@jrtc27
Copy link
Collaborator

jrtc27 commented Jan 11, 2023

Because that's what Spike/Rocket/etc did in the early days and now BBL and OpenSBI assume that for their generic firmware builds. The harder you make it for users to run that kind of software the less likely it is people will take the Sail model seriously and will instead continue to use Spike.

@allenjbaum
Copy link
Collaborator

allenjbaum commented Jan 11, 2023 via email

@jrtc27
Copy link
Collaborator

jrtc27 commented Jan 11, 2023

How then do you propose the right FDT be provided to firmware? Because it's the Sail model that knows what its configuration is, not firmware.

@jrtc27
Copy link
Collaborator

jrtc27 commented Jan 11, 2023

There also may not be "real silicon"; for your use, you want Sail to be configured to match a specific DUT, but from a software development perspective you don't care about DUTs or boot ROMs, you just want to run the model, possibly with some extensions explicitly enabled or disabled, and test your software.

@allenjbaum
Copy link
Collaborator

allenjbaum commented Jan 11, 2023 via email

@jrtc27
Copy link
Collaborator

jrtc27 commented Jan 11, 2023

b. if we build it into Sail, then you have the opposite problem when you
try to run it on real HW that has the FDT somewhere else.
(and if that's not a problem because everybody puts the FDT in the same
place, then it should be speced by RV)

I don't understand this point. Software doesn't care where the FDT is in memory, it gets told that by the boot ROM.

c. If it's important to firmware, then there will be a boot image that
initializes those registers and runs right out of reset.
I fail to see why that is unworkable or onerous - that's what a real
bootROM would do, and there is no need to build that into SAIL.

The boot image that initialises those registers and runs right out of reset is what's there in the Sail model today.

A software development project will eventually need to run on real silicon,
and likely will have a boot ROM
(almost certainly much larger than the half dozen instructions we are
talking about here; the boot flow for high end processors is not a trivial
process).

But that boot ROM isn't appropriate for the Sail model, because, as you say, it's doing other microarchitecture-specific initialisation.

If it requires that we come up with some default bootROM that gets run if
no other bootROM is specified, then we could provide default bootROM -
but not as part of, or built into Sail. It should be no different than one
provided by a manufacturer.
The mechanism of loading that image into Sail should be built in, though.

I don't see how you can provide something and have built-in loading of it yet not have it be part of the model. Where's it living if not here when its contents needs to be generated based on the user-provided arguments to the model?

@allenjbaum
Copy link
Collaborator

allenjbaum commented Jan 14, 2023 via email

@ptomsich ptomsich added tgmm-agenda Tagged for the next Golden Model meeting agenda. and removed tgmm-agenda Tagged for the next Golden Model meeting agenda. labels May 29, 2023
@ptomsich
Copy link
Collaborator

ptomsich commented Jun 1, 2023

Proposed next steps from the discussion in the TGMM on May 30th:

  • file a feature ticket and take the design discussion there (e.g., document/discuss the design in it)
  • close this PR until we have a clear decision on what we want

From the discussion in the meeting, the consensus on what we might want (of course, this is just a sketch and not a detailed design and rationale as it may emerge after a thorough discussion) was:

  • don't inject ROM code from within the simulator's logic
  • support loading multiple ELF files and have the ROM code loaded up that way
  • include the current ROM code as a small, precompiled ELF file (and sources) in the repository

@jrtc27 Let me know if this is a sensible approach from your perspective?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants