Skip to content

Conversation

davideschiavone
Copy link
Contributor

This is still a draft, this PR adds the Verilator TestBench to run applications

@davideschiavone davideschiavone changed the title add verilator testbench [do not merge yet] add verilator testbench Jun 1, 2022
Copy link
Contributor

@jeremybennett jeremybennett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks my Verilator harness for GDB, because you have changed the name of the model.!

First I notice the build of make model-lib takes much longer.

When I try to build my harness, it quickly fails:

$ make clean all
rm -f Args.o Dmi.o DtmJtag.o Tap.o Utils.o VSim.o testbench.o TestJtag.o testbench.exe verilated.o verilated_vcd_c.o
g++ -I/opt/verilator-HEAD/share/verilator/include -I/opt/verilator-HEAD/share/verilator/include/vltstd -I/home/jeremy/gittrees/openhw/core-v-mcu/build/openhwgroup.org_systems_core-v-mcu_0/model-lib-verilator/obj_dir -I../vendor/cxxopts-3.0.0rc/include -DVL_TIME_CONTEXT -O0 -g3 -c -o Args.o Args.cpp
g++ -I/opt/verilator-HEAD/share/verilator/include -I/opt/verilator-HEAD/share/verilator/include/vltstd -I/home/jeremy/gittrees/openhw/core-v-mcu/build/openhwgroup.org_systems_core-v-mcu_0/model-lib-verilator/obj_dir -I../vendor/cxxopts-3.0.0rc/include -DVL_TIME_CONTEXT -O0 -g3 -c -o Dmi.o Dmi.cpp
g++ -I/opt/verilator-HEAD/share/verilator/include -I/opt/verilator-HEAD/share/verilator/include/vltstd -I/home/jeremy/gittrees/openhw/core-v-mcu/build/openhwgroup.org_systems_core-v-mcu_0/model-lib-verilator/obj_dir -I../vendor/cxxopts-3.0.0rc/include -DVL_TIME_CONTEXT -O0 -g3 -c -o DtmJtag.o DtmJtag.cpp
In file included from Tap.h:13:0,
                 from DtmJtag.h:12,
                 from DtmJtag.cpp:11:
VSim.h:14:10: fatal error: Vcore_v_mcu.h: No such file or directory
 #include "Vcore_v_mcu.h"
          ^~~~~~~~~~~~~~~
compilation terminated.
Makefile:74: recipe for target 'DtmJtag.o' failed
make: *** [DtmJtag.o] Error 1

Changing the name of the top level interface (or for that matter its top level pins) is going to break any other software that uses this model. It also seems to me that core_v_mcu_testharness is not the correct name, since it relates specifically to the use of these models for hardware testing, and in case of the CORE-V model project, it is not for testing, but for software development

I suggest that for make model-lib if core_v_mcu is not appropriate, then it should perhaps be core_v_mcu_lib. Let's get this fixed and stable ASAP..

@davideschiavone
Copy link
Contributor Author

hi @jeremybennett , thanks for the feedback.

Actually, I wanted to provide a testbench for SW development and why not, HW testing.

The reason I changed the name is that I built a top-level wrapper that includes the real top-level (core_v_mcu) and I included inside the UART modules from lowRISC that read the TX signals from the core_v_mcu UARTs, create the terminal and screen the output. Indeed I can see the BootROM's output as for the FPGA case.

If that is not ok, I propose to create another target in the core_v_mcu.core (say make model_verilator_test) or something like this and leave untouched the previous one - this also solves the speed at which you compile the model lib.

I am thinking long-term as well, maybe one may want to do a model of the Flash that is compatible with Verilator (the current one is not), or describe other HW peripherals - which are easier developed in SystemVerilog - Then I can include them in the testharness module.

How does it sound for you?

@jeremybennett
Copy link
Contributor

I think the ideal solution is to generate two libraries, one with the UART and one without. For some applications (specifically tool chain testing) you want the model as fast as possible, so minimize peripherals and for others (e.g. OS bring up) you want the UART.

You might therefore want make model-lib and make model-lib-uart. I suggest two separate targets, just so you don't have to build a model you don't want.

What do you think? Ultimately what matters is we freeze the top level of the Verilator model library, since any change here affects anyone who uses the library in software.

@davideschiavone
Copy link
Contributor Author

hi @jeremybennett , this should fix your model - can you please comment?

Copy link
Contributor

@jeremybennett jeremybennett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @davide,

I looked at both Verilator targets. Both need some fixes.

model-lib-sim

I first tried building make model-lib-sim, which blew up. Here are some observations. Up to you which of these you wish to adopt

The compilation of the build blew up.

cd ./build/openhwgroup.org_systems_core-v-mcu_0/model-lib-sim-verilator/ && make -f  ../../../verilatorsim.mk
make[1]: Entering directory '/home/jeremy/gittrees/openhw/core-v-mcu/build/openhwgroup.org_systems_core-v-mcu_0/model-lib-sim-verilator'
g++ -I/opt/verilator/bin/../share/verilator/include -I/opt/verilator/bin/../share/verilator/include/vltstd -I./obj_dir -I../../../tb/uartdpi/ -DVL_TIME_CONTEXT -DVM_COVERAGE=0 -DVM_SC=0 -DVM_TRACE=1 -DVM_TRACE_FST=1 -faligned-new -fcf-protection=none -Wno-bool-operation -Wno-sign-compare -Wno-uninitialized -Wno-unused-but-set-variable -Wno-unused-parameter -Wno-unused-variable -Wno-shadow -std=c++11 -Wall -g -fpermissive  -std=gnu++14 -O0 -g3 -c -o core_v_mcu_tb.o ../../../tb/core_v_mcu_tb.cpp
g++: error: unrecognized command line option ‘-fcf-protection=none’; did you mean ‘-flto-partition=none’?
../../../verilatorsim.mk:80: recipe for target 'core_v_mcu_tb.o' failed

I think the -fcf-protection=none flag is quite new - it targets recent x86 processors, and it isn't supported by the standard GCC on Ubuntu 20.04 LTS.

-faligned-new seems pointless, since it is a C++17 feature and we are specifying C++14 (and I really wouldn't be tempted to specify anything newer).

However, when I ditched these, linking blew up:

/opt/verilator/bin
g++ core_v_mcu_tb.o verilated.o verilated_dpi.o verilated_fst_c.o uartdpi.o obj_dir/Vcore_v_mcu_testharness__ALL.a -lz -pthread -lutil -lelf -o core_v_mcu_tb.exe
/usr/bin/ld: cannot find -lelf
collect2: error: ld returned 1 exit status
../../../verilatorsim.mk:60: recipe for target 'core_v_mcu_tb.exe' failed
make[1]: *** [core_v_mcu_tb.exe] Error 1
make[1]: Leaving directory '/home/jeremy/gittrees/openhw/core-v-mcu/build/openhwgroup.org_systems_core-v-mcu_0/model-lib-sim-verilator'
Makefile:86: recipe for target 'model-lib-sim' failed
make: *** [model-lib-sim] Error 2

So I ditched -lelf and the model built just fine.

I took a log at verilatorsim.mk. I noticed that you have some mix up over your flags, since you had put compiler options in CPPFLAGS, which is for pre-processor options. They belong properly in CXXFLAGS. Broadly CPPFLAGS should only have -I and -D options, and possibly warning flags specific to the pre-processor (don't know if there are any of these).

I suggest you include an optional USER_CPPFLAGS, USER_LDFLAGS, USER_CXXFLAGS, which default to empty, but which the user can use to set their own options. Suggest you put them at the end, so user choices override the makefile choices. Then you can add -fcf-protection when you build on your machine.

Overall, you need to go through the README.md from top to bottom. It is missing things and inconsistent in places for the two verilator model targets.

So my recommendations are:

  1. If you need libelf, then make it an explicit pre-requisite for the project.
  2. Avoid any flags newer than C++14 or that are specific to very recent Intel hardware
  3. Fix verilatorsim.mk to use the flags consistently and add user flag variables
  4. Clean up README.md

model-lib

Good news: make model-lib just built cleanly and then my code also built cleanly. However, when I ran the program, I got:

$ ./testbench.exe --test-status --test-gprs --test-fprs --test-csrs --test-mem
Timescale 1ns / 1ns
TOP.core_v_mcu.i_soc_domain.l2_ram_i.bank_sram_pri0_i.u0
%Error: mem_init/TOP.core_v_mcu.i_soc_domain.l2_ram_i.bank_sram_pri0_i.u0.mem:0: $readmem file not found
Aborting...
Aborted (core dumped)

This is the third time this bug has been reintroduced. Can you fix it, so that the GitHub checks include a check that the memory images are in the right place! This is a serious failure of the engineering process.

@davideschiavone
Copy link
Contributor Author

davideschiavone commented Jul 18, 2022

hi @jeremybennett - thanks for the comments, I asked for further questions on MM especially to reproduce that bug, which I think I solved but AFAIK there was no way to test verilator so far, but linting

@davideschiavone
Copy link
Contributor Author

@gmartin102 , do you know why the document generation is failing?

@gmartin102
Copy link
Contributor

gmartin102 commented Jul 20, 2022 via email

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.

4 participants