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

Connect Dromajo Simulator #415

Merged
merged 12 commits into from
Dec 10, 2019
Merged

Connect Dromajo Simulator #415

merged 12 commits into from
Dec 10, 2019

Conversation

abejgonzalez
Copy link
Contributor

@abejgonzalez abejgonzalez commented Nov 19, 2019

Related issue:

Type of change: new feature

Impact: new rtl

Development Phase: implementation

Release Notes
This adds the https://github.com/chipsalliance/dromajo simulator to BOOM for functional verification. Additionally, it has the following core changes:

Dromajo Func Sim Notes
This uses a fork of Dromajo (https://github.com/abejgonzalez/dromajo) to do a couple of things:

  1. Override the MMIO start and end addresses
  2. Override the Dromajo hard-coded bootrom with another
  3. Override the Dromajo DTB with another
  4. Change the reset vector
  5. Change the PLIC/CLINT parameters
  6. Have the bootrom be compact (have the dtb be appended directly after the bootrom end)
  7. Override the main memory size

To get the simulation running here is an example command that I used to get bare-metal tests working (BIN == baremetal-test):

make run-binary CONFIG=SmallBoomConfig BINARY=<BIN>

For Linux, I just ran the br-base-bin that comes with FireMarshal and it matches. Note: The Dromajo uart output will probably have alot of duplicated characters (centossssssssss) because Dromajo doesn't have UART tx/rx fifos (on Linux a write is continually done until the txfifo is not full). So when co-simulating the UART output from the DUT will differ from Dromajo. Similar command (BIN == br-base-bin):

make run-binary CONFIG=SmallBoomConfig BINARY=<BIN> SIM_FLAGS="+drj_dtb=\"<DTB_FILE>\""

Also, you need to change the VCS Makefile to include the following two lines in the compile:

<PATH-TO>/dromajo/src/libdromajo_cosim.a
-CC "-I<PATH-TO>/dromajo/src"

To get the proper Dromajo inputs (MMIO, PLIC, etc), I had to modify the incoming TileParams and add the overall system BootROMParams, MemoryPortParams, CLINTParams, and PLICParams. Additionally, the binary is taken from the VPI arguments (assuming HTIF is the bringup procedure).

@abejgonzalez
Copy link
Contributor Author

I'll leave this open for people to leave comments, recommendations.

src/main/resources/csrc/dromajo_cosim.cc Outdated Show resolved Hide resolved
src/main/resources/csrc/dromajo_cosim.cc Outdated Show resolved Hide resolved
src/main/resources/csrc/dromajo_cosim.cc Outdated Show resolved Hide resolved
src/main/scala/common/consts.scala Outdated Show resolved Hide resolved
@jerryz123
Copy link
Contributor

Great job, this looks very promising.
We should think about how to integrate this into Chipyard cleanly. It would be great if we could just do make CONFIG=DromajoSmallBoomConfig run-asm-tests and have everything work.

@ccelio
Copy link
Contributor

ccelio commented Nov 19, 2019

Looks awesome! Please do PR some of your Dromajo changes so we can share improvements!

@abejgonzalez
Copy link
Contributor Author

@ccelio I will definitely PR the changes, I just want to get things cleaned up first!

@ccelio
Copy link
Contributor

ccelio commented Nov 21, 2019

Does this turn off the DTM to prevent external agent/device polling?

@abejgonzalez
Copy link
Contributor Author

In Chipyard we use HTIF/TSI to bringup the core. I don't turn off the debug module.

@ccelio
Copy link
Contributor

ccelio commented Nov 21, 2019

How do you co-simulate if HTIF/TSI can perturb the state of the system? I would think you would also have to simulate the side-effects of the HTIF/TSI actions.

@jerryz123
Copy link
Contributor

How do you co-simulate if HTIF/TSI can perturb the state of the system? I would think you would also have to simulate the side-effects of the HTIF/TSI actions.

TSI only reads from the TOHOST addr in our systems. As long as it does no writes, we are good.

@abejgonzalez
Copy link
Contributor Author

@jerryz123 I went ahead and updated the release notes. Currently, you have to specify the binary and the dtb file with plusargs. Unfortunately, I don't think there is a way to make the binary plusarg go away. VCS doesn't have a default variable of some sort that I can check and get the binary file. As for the dtb, I need to modify Chipyard so Ill leave that for a later time since that is a bit ugly. Until that is fixed you can do dtc -O dtb *.dts -o *.dtb to get the dtb to point to.

@abejgonzalez
Copy link
Contributor Author

Also, I will remove the default COSIM = true constant once CI finishes, I just want to make sure Verilator compiles with this.

@jerryz123
Copy link
Contributor

@jerryz123 I went ahead and updated the release notes. Currently, you have to specify the binary and the dtb file with plusargs. Unfortunately, I don't think there is a way to make the binary plusarg go away. VCS doesn't have a default variable of some sort that I can check and get the binary file. As for the dtb, I need to modify Chipyard so Ill leave that for a later time since that is a bit ugly. Until that is fixed you can do dtc -O dtb *.dts -o *.dtb to get the dtb to point to.

The DTB thing is fine for now. Can we really not get the args with vpi_get_vlog_info? https://github.com/ucb-bar/testchipip/blob/master/src/main/resources/testchipip/csrc/SimSerial.cc does this.

Also can you merge some intermediate commits, and prepend them with [cosim].

@abejgonzalez
Copy link
Contributor Author

Done @jerryz123. This should be ready for review now... assuming CI passes.

src/main/scala/exu/core.scala Show resolved Hide resolved
@abejgonzalez
Copy link
Contributor Author

I am running all assembly/bmark tests with Dromajo and I am encountering divergences (I will look into it after Thanksgiving break).

@abejgonzalez
Copy link
Contributor Author

abejgonzalez commented Dec 3, 2019

Ok. I resolved/figured out most of the divergences... here they are:

  • rv64mi-p-mcsr - mimpid, marchid, mvendorid are all set to specific values by Dromajo by default. This has no large impact but I have a quick patch on my Dromajo branch (abejgonzalez/dromajo)
  • rv64mi-p-breakpoint - this test fails unless you add one breakpoint, additionally if you set nBreakpoints to true then it will error since Rocket sets its maskmax reset value to 4 by default while Dromajo has it unset.
  • rv64ua-v-lrsc - bug with BOOM. see AMO Exceptions scause Error #417
  • all benchmark tests - all fail due to tohost/fromhost issues (additionally, this only works with non-randomized memory init)

With that being said, I think we won't be running with Dromajo by default so not having these features is fine for now. I'll just wait until a patch for the scause issue is added to this, then Ill test Linux/Fedora.

@ccelio
Copy link
Contributor

ccelio commented Dec 3, 2019

(additionally, this only works with non-randomized memory init)

TL,DR: I think you have to disable randomized memory.

The problem is that some of the riscv-tests will access memory that has not been initialized first (for example, a load-reserve that doesn't care about what it is actually loading). Unless you randomize dramajo with the exact same memory pattern from boom, there's no hope of this really working (aside from expensively tracking whether each byte has been accessed yet, and if not, letting the DUT override dramajo, which means you have the same coverage hole anyways).

This gets extra complicated because elf binaries have a bss section which is generally assumed to be zeroed out by software. To save on elf space, the bss simply says "I am X bytes in size". This requires the elf loader to do the zeroing. I don't remember if elf2hex does this bss zeroing, because the bss section can be large.

For this reason, I recommend zeroing out all DUT memory. It's a small loss in coverage, but a huge removal of headaches.

@jerryz123
Copy link
Contributor

For this reason, I recommend zeroing out all DUT memory. It's a small loss in coverage, but a huge removal of headaches.

Yeah adding +define+RANDOM=0 to the VCS CCFLAGS is sufficient for this.

@ccelio
Copy link
Contributor

ccelio commented Dec 3, 2019

Yeah adding +define+RANDOM=0 to the VCS CCFLAGS is sufficient for this.

Is that too heavy-handed, or does that only affect the bootROM/bootRAM and simRAM? I believe in the past I used a modified simRAM/bootRAM model that zeroed out the memory, but let other SeqMems and Regs be randomized (which is nice for X-prop/reset bug hunting).

Doing so is not unrealistic, since in the "real thing", you can freely zero out the bootRAM anyways.

@jerryz123
Copy link
Contributor

Unfortunately it does cause all regs to be zero initialized, which is annoying. I think we would like to continue using the SimAXIMem for the simRAM, which does not let us specifically zero out that memory.

@ccelio
Copy link
Contributor

ccelio commented Dec 3, 2019

I’d consider pushing upstream a “zeroing” option to SimAXIMem (perhaps as a verilog define flag?).

@abejgonzalez
Copy link
Contributor Author

Yes. I ended up using the +define+RANDOM=0 hack to get around this. We will just end up using that hack for Linux boot for now.

[lsu] AMOs should generate pf/ae_st. Do this by putting LR in the LDQ
@abejgonzalez
Copy link
Contributor Author

This should be good to merge now.

@jerryz123
Copy link
Contributor

This looks good to merge, but there's lots of opportunity for more features.

  • We should do a bit more testing. How far does BOOM get in linux boot now?
  • For the tohost/fromhost thing, it might be straightforward to modify Dromajo to consider those addresses to be "MMIO". Dromajo should have its own elfreader, so it should be able to determine the tohost/fromhost addresses when reading the binary.
  • Can Dromajo be modified to simulate interactions with the block device? The key is that we don't need to simulate the entire device, we only need to simulate the sequence of stores the block device writes into RAM.
  • Can the DromajoCosimBlackBox be modified to work as a Firesim bridge?

@abejgonzalez
Copy link
Contributor Author

This looks good to merge, but there's lots of opportunity for more features.

Ill go ahead and make a GH issue to track improvements. But I think this is a good stepping stone to merge in for basic support.

  • We should do a bit more testing. How far does BOOM get in linux boot now?

I tested Linux boot (br-base + fedora + coremark) on FireSim. I haven't retried booting with Dromajo cosim. Though at the time, the failure was that uninitialized memory was getting read in. So I don't expect an issue.

  • For the tohost/fromhost thing, it might be straightforward to modify Dromajo to consider those addresses to be "MMIO". Dromajo should have its own elfreader, so it should be able to determine the tohost/fromhost addresses when reading the binary.

I think we can just ignore any errors related to to/fromhost instead of hacking Dromajo to make them MMIO.

  • Can Dromajo be modified to simulate interactions with the block device? The key is that we don't need to simulate the entire device, we only need to simulate the sequence of stores the block device writes into RAM.

Future work!

  • Can the DromajoCosimBlackBox be modified to work as a Firesim bridge?

Future work!

@jerryz123
Copy link
Contributor

I tested Linux boot (br-base + fedora + coremark) on FireSim. I haven't retried booting with Dromajo cosim. Though at the time, the failure was that uninitialized memory was getting read in. So I don't expect an issue.

Sorry, I meant how far does it get when run with cosim? Can you start up a SW sim run with cosim, and the boot-binary?

@abejgonzalez
Copy link
Contributor Author

Sorry, I meant how far does it get when run with cosim? Can you start up a SW sim run with cosim, and the boot-binary?

Running as we speak!

@abejgonzalez
Copy link
Contributor Author

As an update... we think this is good to merge. Linux tests are finding small bugs here and there which means this is working! So we will go ahead and merge after CI.

@abejgonzalez abejgonzalez merged commit 6d14cfe into master Dec 10, 2019
@abejgonzalez abejgonzalez deleted the dromajo-integration branch December 10, 2019 05:02
jerryz123 pushed a commit that referenced this pull request Mar 11, 2021
* Move firemarshal into chipyard (was in firesim)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AMO Exceptions scause Error TracePort Interrupt Cause Missing
3 participants