----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3624/#review8722 -----------------------------------------------------------
Thanks for the contribution. It looks pretty good! Could you give us a preview as to the other 4 patches? What is missing in this one? I know nothing of how the ISA stuff works in gem5, but I reviewed most of the other parts of this patch. Some high-level notes: a.out probably shouldn't be in the patch. Make sure you've updated all of the copyright information. Have you run any of the RISC-V test suite on your code? (https://riscv.org/software-tools/riscv-tests/) In fact, we could probably incorporate some (all?) of that into the regression tests. How much we include depends on how long the tests take. ext/libelf/elf_common.h (line 176) <http://reviews.gem5.org/r/3624/#comment7539> It would be nice if this were aligned with the above. src/arch/riscv/RiscvISA.py (line 51) <http://reviews.gem5.org/r/3624/#comment7542> I didn't see num_threads or num_vpes ever used. If these aren't used they should be removed. Also, is `system` ever used? src/arch/riscv/RiscvSystem.py (line 40) <http://reviews.gem5.org/r/3624/#comment7545> For all of these parameters: are they used by the RiscvSystem code? Will they be used in the future? It looks like you copied a lot of this code from MIPS. If the code isn't applicable to RISC-V, it should be removed. src/arch/riscv/RiscvSystem.py (line 46) <http://reviews.gem5.org/r/3624/#comment7544> Does this mean RISC-V only supports 32 bit? (I noticed the x86 mask is 64-bit.) src/arch/riscv/RiscvSystem.py (line 54) <http://reviews.gem5.org/r/3624/#comment7543> Is this paraemter ever used? I didn't see it. - Jason Lowe-Power On Sept. 14, 2016, 10:45 p.m., Alec Roelke wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3624/ > ----------------------------------------------------------- > > (Updated Sept. 14, 2016, 10:45 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11620:80e562ddf946 > --------------------------- > arch: [Patch 1/5] Added RISC-V base instruction set RV64I > > First of five patches adding RISC-V to GEM5. This patch introduces the > base 64-bit ISA (RV64I) in src/arch/riscv for use with syscall emulation. > The multiply, floating point, and atomic memory instructions will be added > in additional patches, as well as support for more detailed CPU models. > The loader is also modified to be able to parse RISC-V ELF files, and a > "Hello world!" example for RISC-V is added to test-progs. > > > Diffs > ----- > > src/arch/riscv/stacktrace.cc PRE-CREATION > src/arch/riscv/system.hh PRE-CREATION > src/arch/riscv/system.cc PRE-CREATION > src/arch/riscv/tlb.hh PRE-CREATION > src/arch/riscv/tlb.cc PRE-CREATION > src/arch/riscv/types.hh PRE-CREATION > src/arch/riscv/utility.hh PRE-CREATION > src/arch/riscv/vtophys.hh PRE-CREATION > src/base/loader/elf_object.cc 8bc53d5565ba > src/arch/riscv/registers.hh PRE-CREATION > src/arch/riscv/remote_gdb.hh PRE-CREATION > src/arch/riscv/remote_gdb.cc PRE-CREATION > src/arch/riscv/stacktrace.hh PRE-CREATION > src/arch/riscv/linux/process.cc PRE-CREATION > src/arch/riscv/linux/system.hh PRE-CREATION > src/arch/riscv/linux/system.cc PRE-CREATION > src/arch/riscv/locked_mem.hh PRE-CREATION > src/arch/riscv/microcode_rom.hh PRE-CREATION > src/arch/riscv/mmapped_ipr.hh PRE-CREATION > src/arch/riscv/pagetable.hh PRE-CREATION > src/arch/riscv/pagetable.cc PRE-CREATION > src/arch/riscv/pra_constants.hh PRE-CREATION > src/arch/riscv/process.hh PRE-CREATION > src/arch/riscv/process.cc PRE-CREATION > src/arch/riscv/pseudo_inst.hh PRE-CREATION > src/arch/riscv/isa/formats/basic.isa PRE-CREATION > src/arch/riscv/isa/formats/formats.isa PRE-CREATION > src/arch/riscv/isa/formats/mem.isa PRE-CREATION > src/arch/riscv/isa/formats/type.isa PRE-CREATION > src/arch/riscv/isa/formats/unknown.isa PRE-CREATION > src/arch/riscv/isa/includes.isa PRE-CREATION > src/arch/riscv/isa/main.isa PRE-CREATION > src/arch/riscv/isa/operands.isa PRE-CREATION > src/arch/riscv/isa_traits.hh PRE-CREATION > src/arch/riscv/kernel_stats.hh PRE-CREATION > src/arch/riscv/linux/linux.hh PRE-CREATION > src/arch/riscv/linux/linux.cc PRE-CREATION > src/arch/riscv/linux/process.hh PRE-CREATION > src/arch/riscv/idle_event.hh PRE-CREATION > src/arch/riscv/idle_event.cc PRE-CREATION > src/arch/riscv/interrupts.hh PRE-CREATION > src/arch/riscv/interrupts.cc PRE-CREATION > src/arch/riscv/isa.hh PRE-CREATION > src/arch/riscv/isa.cc PRE-CREATION > src/arch/riscv/isa/base.isa PRE-CREATION > src/arch/riscv/isa/bitfields.isa PRE-CREATION > src/arch/riscv/isa/decoder.isa PRE-CREATION > a.out 8bc53d5565ba > build_opts/RISCV PRE-CREATION > ext/libelf/elf_common.h 8bc53d5565ba > src/arch/riscv/RiscvISA.py PRE-CREATION > src/arch/riscv/RiscvInterrupts.py PRE-CREATION > src/arch/riscv/RiscvSystem.py PRE-CREATION > src/arch/riscv/RiscvTLB.py PRE-CREATION > src/arch/riscv/SConscript PRE-CREATION > src/arch/riscv/SConsopts PRE-CREATION > src/arch/riscv/decoder.hh PRE-CREATION > src/arch/riscv/decoder.cc PRE-CREATION > src/arch/riscv/faults.hh PRE-CREATION > src/arch/riscv/faults.cc PRE-CREATION > src/base/loader/object_file.hh 8bc53d5565ba > src/cpu/BaseCPU.py 8bc53d5565ba > src/sim/process.cc 8bc53d5565ba > tests/test-progs/hello/bin/riscv/linux/hello 8bc53d5565ba > > Diff: http://reviews.gem5.org/r/3624/diff/ > > > Testing > ------- > > > Thanks, > > Alec Roelke > > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
