> On Sept. 27, 2016, 8:53 p.m., Andreas Hansson wrote: > > Thanks again for getting this in shape. There are a few minor questions and > > comment on the patch, the most important one is related to the copyrights. > > > > Also, I assume you have used hg copy for the files that are more or less > > 1:1 copies from other architectures? > > Alec Roelke wrote: > I didn't; I used cp and then hg add. Does hg copy do anything different?
Yeah that is a bit of an issue. If you didn't actually use hg copy then all history of the file is lost. Sorry to be a drag, but could you redo it using hg copy? > On Sept. 27, 2016, 8:53 p.m., Andreas Hansson wrote: > > src/arch/riscv/faults.hh, line 139 > > <http://reviews.gem5.org/r/3624/diff/5/?file=58091#file58091line139> > > > > Could you perhaps elaborate on why this is ok as is? > > Alec Roelke wrote: > Currently this only supports SE mode, which doesn't have elevated > privilege (which is also why the eret instruction is not implemented). That > is a reminder to me of how I intended for that fault to change when elevated > privilege is implemented (or a hint to the person who does it if I don't). > Is it not appropriate to include it? Fair enough. Feel free to leave it. I would imagine if you called out every location there would be tons of TODOs though :-) > On Sept. 27, 2016, 8:53 p.m., Andreas Hansson wrote: > > src/arch/riscv/isa/base.isa, line 3 > > <http://reviews.gem5.org/r/3624/diff/5/?file=58099#file58099line3> > > > > ? Is that a legal entity? > > Alec Roelke wrote: > Some of the files in this project were originally from a GitHub project > by Austin Harris. In those cases, I left whatever copyright was originally > there in addition to adding mine if I felt it was appropriate. I don't want to cause too much effort on your end, but I really think we need to get this right. Should Austin have used that entity for the copyright? Is it even a real legal entity? Could you reach out to him and check? We really need to get this right imho. > On Sept. 27, 2016, 8:53 p.m., Andreas Hansson wrote: > > src/arch/riscv/mmapped_ipr.hh, line 43 > > <http://reviews.gem5.org/r/3624/diff/5/?file=58118#file58118line43> > > > > RISCV has mmapped IPRs? > > Alec Roelke wrote: > This file was from the original GitHub project, and appears to be > required by the CPU models. I see. Someone more familiar with the CPU model could perhaps comment? I find it odd that we need it when only ALPHA has the construct. > On Sept. 27, 2016, 8:53 p.m., Andreas Hansson wrote: > > src/arch/riscv/tlb.cc, line 3 > > <http://reviews.gem5.org/r/3624/diff/5/?file=58133#file58133line3> > > > > I am really surprise that this and a few other files do not have a Uni > > Virginia copyright. Is that intentional? > > Alec Roelke wrote: > Files that I copied from other ISAs, and only made minor changes like > names, I didn't feel merited that addition, so I left them as they are. Makes sense. My rule is usually: If it's trivial to make the change using a sed script then don't bother. As mentioned above, please make sure you use hg copy so that the revision history is maintained. - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3624/#review8737 ----------------------------------------------------------- On Sept. 16, 2016, 4:49 p.m., Alec Roelke wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3624/ > ----------------------------------------------------------- > > (Updated Sept. 16, 2016, 4:49 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11620:3da42ffa5b4b > --------------------------- > 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. > > Patch 2 will implement the multiply extension, RV64M; patch 3 will implement > the floating point (single- and double-precision) extensions, RV64FD; > patch 4 will implement the atomic memory instructions, RV64A, and patch 5 > will add support for timing, minor, and detailed CPU models that is missing > from the first four patches (such as handling locked memory). > > [Removed several unused parameters and imports from RiscvInterrupts.py, > RiscvISA.py, and RiscvSystem.py.] > [Fixed copyright information in RISC-V files copied from elsewhere that had > ARM licenses attached.] > [Reorganized instruction definitions in decoder.isa so that they are sorted > by opcode in preparation for the addition of ISA extensions M, A, F, D.] > Signed-off by: Alec Roelke > > > Diffs > ----- > > src/arch/riscv/stacktrace.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 > 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/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 > 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/linux/process.cc PRE-CREATION > src/arch/riscv/locked_mem.hh PRE-CREATION > src/arch/riscv/microcode_rom.hh 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/registers.hh PRE-CREATION > src/arch/riscv/remote_gdb.hh PRE-CREATION > src/arch/riscv/remote_gdb.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/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 > src/arch/riscv/stacktrace.cc PRE-CREATION > src/arch/riscv/system.hh PRE-CREATION > src/arch/riscv/system.cc PRE-CREATION > > 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
