neorv32 icon indicating copy to clipboard operation
neorv32 copied to clipboard

Possible enhancements to management of test bin/hex files

Open umarcor opened this issue 3 years ago • 10 comments

According to https://github.com/stnolting/neorv32/blob/master/riscv-arch-test/port-neorv32/framework_v2.0/riscv-target/neorv32/README.md, https://github.com/stnolting/neorv32/blob/master/rtl/core/neorv32_application_image.vhd and https://github.com/stnolting/neorv32/blob/master/riscv-arch-test/port-neorv32/framework_v2.0/riscv-target/neorv32/device/rv32i_m/M/Makefile.include, currently each test program is compiled from assembler, an elf file is generated, then a bin file, which is then converted and replaced in a VHDL source. I feel there needs to be some more idiomatic and cleaner way of doing so. Let me drop some ideas:

  • Use a VHDL function for initialising a ROM from a bin or hex file. So, the simulation/synthesis can use the same input file that you would program through a debugging cable.
  • Avoid compiling all the example bin/hex files in each CI run. The core is expected to change more frequently than the RISC-V test suite. Therefore, it might be sensible to precompile all the tests and use them as an asset. All those bin/hex files do only need to be updated when the bootloader or any relevant part of the core is modified. There are three solutions for handling bin/hex files:
    • Check them into the repo.
    • Use a dummy pre-release. In fact, the same 'nightly' pre-release would fit.
    • Use the CI cache (see actions/cache).

umarcor avatar May 21 '21 00:05 umarcor

I have to admit this might not be the most elegant way to initialize the ROM.

  • Use a VHDL function for initialising a ROM from a bin or hex file. So, the simulation/synthesis can use the same input file that you would program through a debugging cable.

I also thought about this. At the time the whole "generate application image" thing was implemented, I had some troubles dealing with reading files with VHDL's textio (mainly "path issues")... Actually, I do not really like the way it is implemented right now:

  • having additional VHDL files in the core/rtl folder just for providing memory images does not feel right. Providing VHDL memory init functions to read the results of a compilation seems to be the right way to go.
  • the image generator is a relic from the NEORV32`s predecessor. I think the functionality could (should!) be easily moved to a simple script (bash, not python! 😉).

Avoid compiling all the example bin/hex files in each CI run. The core is expected to change more frequently than the RISC-V test suite. Therefore, it might be sensible to precompile all the tests and use them as an asset. All those bin/hex files do only need to be updated when the bootloader or any relevant part of the core is modified. There are three solutions for handling bin/hex files:

I already get in trouble (rightly!) when uploading PDFs to the repository (:wink:). Adding pre-compiled tests does not feel right - and maybe this is not required at all, since the riscv-arch-tests workflow only provides a manual trigger (yet), that is triggered every now and then. Anyway, the CI cache seems to be an interesting option!

By the way, running the riscv-arch-tests does not actually require any software (except for the image generator) from the rest of the project at all. The bootloader is not used since the test programs are "loaded" into the ROM during compilation.

stnolting avatar May 21 '21 14:05 stnolting

There was the idea to implement a new top generic to pass the path to memory init files to the design. Basically, I am ok with this as this could also be used to pass version info.

However, my main concerns are:

  • this seems to be platform-independent solution. But "who" defines this path? And where/when?
  • the solution should be robust. A FPGA beginner should not have to worry about it. Even not when doing a new setup by hand (without scripts).

stnolting avatar Jun 04 '21 21:06 stnolting

But "who" defines this path? And where/when?

Each tool can allow defining the top-level generics in a different way, because that is not specified in the LRM as far as I am aware. (maybe it changed with VHDL 2019?)

Anyway, in practice, most simulators allow specifying a top-level generic in the CLI through -gNAME=VALUE. In fact, it gets problematic if the generic is not of type string, but the type string is the once which we found to be most consistent. That is not surprising since CLI arguments are always strings, per se. For this specific reason, in VUnit we support using JSON-for-VHDL and base16 encoding the string. That converts any complex data structure into a "plain" string, which any simulator can handle.

Therefore, defining the path through a generic string is the most portable solution. That's not only with regard to multiple simulators but also with regard to different OS, since the path separator symbol is different!

Moreover, VUnit provides tb_path as an optional top-level argument. Therefore, you don't need to tell the whole path explicitly. You can just care about the name (provided that you always use the same location).

As a result, I think the only challenge is which VHDL subprogram to use for reading the hex/file and initialising the memory. @Xiretza, do you remember where we discussed this recently? I vaguely recall that you provided an example which you use for this purpose.

the solution should be robust. A FPGA beginner should not have to worry about it. Even not when doing a new setup by hand (without scripts).

The solution would be:

  • Build the app with GCC.
  • Convert the app to some hex/bin (if necessary).
  • Provide the path to the file through a top-level generic (VUnit allows handling it in the run.py script).

So, it'd be arguably easier, because users would have a list of hex/bin files and they would synthesize multiple designs without modifying any VHDL at all. Moreover, there are mechanisms for modifying the content in the bitstream, so it would be potentially possible to not require syntehsizing again. However, I believe that's out of scope.

umarcor avatar Jun 05 '21 14:06 umarcor

As a result, I think the only challenge is which VHDL subprogram to use for reading the hex/file and initialising the memory. @Xiretza, do you remember where we discussed this recently? I vaguely recall that you provided an example which you use for this purpose.

That must've been on gitter on 2021-05-14 (and also earlier on 2021-03-04). If you don't have matrix, here's a direct link to the implementation I pointed to: https://gitlab.com/YARM-project/soc/-/blob/72737f9910468c737b6c59a415000a6ca89829f6/vhdl/core/util_pkg.vhd#L57.

It reads a single memory word per line in plain hex formatting (i.e. xxd -p -c4 for 32-bit words). It's certainly not a very pretty implementation, but should be enough to draw some inspiration from.

Xiretza avatar Jun 05 '21 15:06 Xiretza

From /ghdl/ghdl#1004:

    type ram_t is array(0 to (MEMORY_SIZE / 8) - 1) of std_logic_vector(63 downto 0);

    impure function init_ram(name : STRING) return ram_t is
        file ram_file : text open read_mode is name;
        variable ram_line : line;
        variable temp_word : std_logic_vector(63 downto 0);
        variable temp_ram : ram_t := (others => (others => '0'));
    begin
        for i in 0 to (MEMORY_SIZE/8)-1 loop
            exit when endfile(ram_file);
            readline(ram_file, ram_line);
            hread(ram_line, temp_word);
            temp_ram(i) := temp_word;
        end loop;

        return temp_ram;
    end function;

    signal memory : ram_t := init_ram(RAM_INIT_FILE);

You can see that used in microwatt: https://github.com/antonblanchard/microwatt/search?q=initialize_ram. There, they use constants instead of generics.

umarcor avatar Jun 05 '21 15:06 umarcor

@umarcor

Therefore, defining the path through a generic string is the most portable solution. That's not only with regard to multiple simulators but also with regard to different OS, since the path separator symbol is different!

I agree :+1: Btw, I think Windows can also resolve paths with Linux-style separators. Need to check that...

Anyway, in practice, most simulators allow specifying a top-level generic in the CLI through -gNAME=VALUE.

What about synthesis tools? :thinking: I think they might provide a similar feature when using scripts for the setup but I am not sure about that.

Anyway. I think the idea with a path generic is great. FPGA beginners already have to setup linker files and memory generics to make the NEORV32 run - and I think they are doing quite fine :wink: So there should be not problem to expect them to configure another generic.

So, it'd be arguably easier, because users would have a list of hex/bin files and they would synthesize multiple designs without modifying any VHDL at all.

:+1:

Just another thing: I think it would be better to keep the bootloader image as dedicated VHDL package. The bootloader does (should) not change very often. Most of the time I update the bootloader code + image only when the CPU's rtl files have changed anyways.

So I would recommend using the path generic to configure the actual application code in the instruction memory for setups that do not use the bootloader + for passing version information.

@Xiretza

If you don't have matrix, here's a direct link to the implementation I pointed to: https://gitlab.com/YARM-project/soc/-/blob/72737f9910468c737b6c59a415000a6ca89829f6/vhdl/core/util_pkg.vhd#L57.

That looks good (and quite powerful)!

From /ghdl/ghdl#1004:

That is also fine!

One more note: The 32-bit wide instruction memory is build from 4 8-bit component so we would need a conversion there - but that is really no big deal. There already is a memory init function in the IMEM that copies from the according image package file. So modification should be very simple:

https://github.com/stnolting/neorv32/blob/652a022669ffd2272276dea2c05b2783de8fe3bc/rtl/core/neorv32_imem.vhd#L73-L86

plus

https://github.com/stnolting/neorv32/blob/652a022669ffd2272276dea2c05b2783de8fe3bc/rtl/core/neorv32_imem.vhd#L94-L101

stnolting avatar Jun 05 '21 17:06 stnolting

Maybe I'm missing the full context here but for the VUnit testbench the instruction memory could be a VUnit memory model connected to a Wishbone slave VC and we can use VUnit functionality to initialize that memory from file.

LarsAsplund avatar Jul 05 '21 22:07 LarsAsplund

@LarsAsplund, see https://github.com/stnolting/neorv32/blob/master/rtl/core/neorv32_application_image.vhd. Currently, the software is compiled to an hex file and then a program in C (https://github.com/stnolting/neorv32/blob/master/sw/image_gen/image_gen.cpp) is used for autogenerating the VHDL source, with the values of the program hardcoded.

Instead, I think it would be desirable to provide the path to the hex file as a top-level generic, which allows to have multiple hex files and run simulations by just changing the file passed to the generic. Currently, it is difficult to run multiple simulations with different software, because all of them expect a single (the same) file to contain the source. Moreover, the size of that memory/constant depends on the size of the software. As a result, each time you generate it, it's expected to have a different size.

I don't think the VUnit memory model is useful in this case, because the purpose is to test the ROM initialisation method which is used for synthesis too. However, using VUnit's model might be interesting in more intensive software tests, where the performance might be better. I vaguelly recall that @stnolting had some alternative model of the memories for simulation purposes...

umarcor avatar Jul 05 '21 22:07 umarcor

@umarcor Ok, I see that I was thinking about the external IMEM (https://github.com/stnolting/neorv32/blob/87ef9eff369e9458f9ef5d94abac0fa4c6b25232/sim/neorv32_tb.vhd#L514-517). Is that used?

LarsAsplund avatar Jul 06 '21 08:07 LarsAsplund

I have modified the makefiles and the image generator. A make hex will create a plain ASCII hex-char text file neorv32_exe.hex that can be used to initialize custom (instruction-)memories. The generated files look like this (for the blink_led demo): https://gist.github.com/stnolting/11854fd0f802907b0fc4d709309ad37a

With the internal IMEM disabled (MEM_INT_IMEM_EN => false,) we could use some external memory component to execute code from that, just like @LarsAsplund said. This would also allow to run multiple simulations in parallel since no HDL files are touched at all.

Instead, I think it would be desirable to provide the path to the hex file as a top-level generic, which allows to have multiple hex files and run simulations by just changing the file passed to the generic. Currently, it is difficult to run multiple simulations with different software, because all of them expect a single (the same) file to contain the source. Moreover, the size of that memory/constant depends on the size of the software. As a result, each time you generate it, it's expected to have a different size.

I know the current setup using the image packages is not the best. But maybe the idea with executing code fro external (VUnit) memories is a good trade-off here (so, for simulation setups).

I am thinking about "splitting" all the tests:

  • use the plain old simple testbench for all processor-internal test cases - including executing code from the pre-initialized internal IMEM
  • use the VUnit testbench for all external test cases like verifying the bus interfaces (AXI, Wishbone, Stream) and maybe also the serial interface (UART, SPI, I²C) 🤔

I don't think the VUnit memory model is useful in this case, because the purpose is to test the ROM initialisation method which is used for synthesis too. However, using VUnit's model might be interesting in more intensive software tests, where the performance might be better. I vaguelly recall that @stnolting had some alternative model of the memories for simulation purposes...

VUnit would be very handy here! The current RISC-V arch tests take ages to complete. A parallel job having the executable in external (and simulation-optimized VUnit) memories could speed things up a lot I think.

@umarcor Ok, I see that I was thinking about the external IMEM (https://github.com/stnolting/neorv32/blob/87ef9eff369e9458f9ef5d94abac0fa4c6b25232/sim/neorv32_tb.vhd#L514-517). Is that used?

This was just a test to check if this concept works (executing code from external memory) - which, by the way, works fine :smile:

stnolting avatar Jul 06 '21 14:07 stnolting