----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2313/#review5187 -----------------------------------------------------------
I have two high-level comments that need to be fixed: * This is should be four different commits (configuration, CPUID updates, segment initialization, kvm, process). * Write a proper commit message (see http://www.m5sim.org/Commit_Access). Specifically, include a short one-line summary and a longer description of what the patch does and why. I'm also a bit unsure about the design of this patch. From RB2312, it /seems/ like the useArchPT flag is supposed to enable page tables in SE emulation mode, however in this patch you seem to overload it to mean "use magic syscall redirection". Could you clean that up a bit? Could you redesign the syscall & fault handler to use the memory mapped m5ops interface instead? That way, the code would in the kvm CPU would be much cleaner and you'd get rid of the magic ports. As a side effect, we could potentially scrap the SE-specific bits of the ISA (this is definitely something I'd like to see long term). configs/example/se.py <http://reviews.gem5.org/r/2313/#comment4724> You need to make more checks here as this patch only adds SE support(?) for X86. configs/example/se.py <http://reviews.gem5.org/r/2313/#comment4725> This is x86-specific, make sure to guard it properly. src/arch/x86/cpuid.cc <http://reviews.gem5.org/r/2313/#comment4726> All changes to this file should be a separate patch. It has nothing to do with the KVM-based cpu. src/arch/x86/process.cc <http://reviews.gem5.org/r/2313/#comment4730> Large portions of this code is identical to X86System::initState. Please refactor (as a separate patch) and reuse that code here. src/arch/x86/process.cc <http://reviews.gem5.org/r/2313/#comment4731> Style src/arch/x86/process.cc <http://reviews.gem5.org/r/2313/#comment4732> Why is this still here? src/arch/x86/system.hh <http://reviews.gem5.org/r/2313/#comment4733> Shouldn't this be in process.hh since it is SE specific? src/arch/x86/system.hh <http://reviews.gem5.org/r/2313/#comment4734> SE-specific? src/arch/x86/system.hh <http://reviews.gem5.org/r/2313/#comment4735> This shouldn't be a part of the patch. src/arch/x86/system.cc <http://reviews.gem5.org/r/2313/#comment4729> This actually makes the whole initialization much less readable and error prone. Can't you just create a base segment descriptor with the default values that you assign to the segment descriptors at initialization time and only change the relevant values. For example: SegDescriptor csDesc = baseDesc; csDesc.type.codeOrData = 1; csDesc.privilege = 0; src/cpu/kvm/base.hh <http://reviews.gem5.org/r/2313/#comment4728> This doesn't seem to be needed or used. Please remove it. src/cpu/kvm/base.cc <http://reviews.gem5.org/r/2313/#comment4736> Why is this line changed? src/cpu/kvm/x86_cpu.cc <http://reviews.gem5.org/r/2313/#comment4727> Is this really needed here? - Andreas Sandberg On July 11, 2014, 6:01 p.m., Alexandru Dutu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2313/ > ----------------------------------------------------------- > > (Updated July 11, 2014, 6:01 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10254:661c482f7d58 > --------------------------- > Making KvmCPU model usable in SE mode. > > > Diffs > ----- > > configs/example/se.py c625a3c51bac050879457e666dd83299a36d761b > src/arch/x86/cpuid.cc c625a3c51bac050879457e666dd83299a36d761b > src/arch/x86/process.cc c625a3c51bac050879457e666dd83299a36d761b > src/arch/x86/regs/misc.hh c625a3c51bac050879457e666dd83299a36d761b > src/arch/x86/system.hh c625a3c51bac050879457e666dd83299a36d761b > src/arch/x86/system.cc c625a3c51bac050879457e666dd83299a36d761b > src/cpu/kvm/base.hh c625a3c51bac050879457e666dd83299a36d761b > src/cpu/kvm/base.cc c625a3c51bac050879457e666dd83299a36d761b > src/cpu/kvm/x86_cpu.hh c625a3c51bac050879457e666dd83299a36d761b > src/cpu/kvm/x86_cpu.cc c625a3c51bac050879457e666dd83299a36d761b > > Diff: http://reviews.gem5.org/r/2313/diff/ > > > Testing > ------- > > Quick regressions passed. > > > Thanks, > > Alexandru Dutu > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
