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 (Diff revision 1)

You need to make more checks here as this patch only adds SE support(?) for X86.

Show all issues

1. Alexandru Dutu July 29, 2014, 3:53 p.m. (July 29, 2014, 3:53 p.m.)

   This only checks whether KvmCPU is in use, I will add more checks to line 200.

configs/example/se.py (Diff revision 1)

This is x86-specific, make sure to guard it properly.

Show all issues

src/arch/x86/cpuid.cc (Diff revision 1)

All changes to this file should be a separate patch. It has nothing to do with the KVM-based cpu.

Show all issues

src/arch/x86/process.cc (Diff revision 1)

Large portions of this code is identical to X86System::initState. Please refactor (as a separate patch) and reuse that code here.

Show all issues

src/arch/x86/process.cc (Diff revision 1)

Style

Show all issues

src/arch/x86/process.cc (Diff revision 1)

Why is this still here?

Show all issues

src/arch/x86/system.hh (Diff revision 1)

Shouldn't this be in process.hh since it is SE specific?

Show all issues

1. Alexandru Dutu July 29, 2014, 3:53 p.m. (July 29, 2014, 3:53 p.m.)

   These get used in places where it does not make sense to include process.hh and it makes sense to include system.hh (i.e. mem/ and cpu/).

src/arch/x86/system.hh (Diff revision 1)

SE-specific?

Show all issues

1. Alexandru Dutu July 29, 2014, 3:53 p.m. (July 29, 2014, 3:53 p.m.)

   Same as above.

src/arch/x86/system.hh (Diff revision 1)

This shouldn't be a part of the patch.

Show all issues

src/arch/x86/system.cc (Diff revision 1)

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;

Show all issues

src/cpu/kvm/base.hh (Diff revision 1)

This doesn't seem to be needed or used. Please remove it.

Show all issues

src/cpu/kvm/base.cc (Diff revision 1)

Why is this line changed?

Show all issues

src/cpu/kvm/x86_cpu.cc (Diff revision 1)

Is this really needed here?

Show all issues

1. Alexandru Dutu July 29, 2014, 3:53 p.m. (July 29, 2014, 3:53 p.m.)

   Actually, this is not needed anymore. I went through 3 modes of exit out of KVM for syscall emulation and entering to continue execution. This got left here from the previous 2 modes.

I'm really not happy about the use of kvm-specific ports magic here. It would have been nicer having a m5ops based interface that just passes the fault/syscall to the arch-specific code from any CPU model that uses the m5ops interface. Specifically, I'm a bit concerned about what would happen if you switch in the syscall or fault handler code before you enter into gem5. In this case the fault/syscall will be lost since the simulated CPUs don't know how to handle the port magic. If you have a good reason to believe that this won't be an issue, I'm happy to have this committed. I have fixed quite a few switching bugs in the past and I know that things like these are likely to come back and bite you and are a pain to diagnose.

src/arch/x86/tlb.cc (Diff revision 3)

Could you get rid of this code duplication before committing?

I'd suggest that you rewrite it something like this:

if (m5opRange) {
 ...
} else if (FullSystem) {
 ...
}

Show all issues

src/cpu/kvm/base.hh (Diff revision 3)

Is this supposed to be here?

Show all issues

src/sim/pseudo_inst.cc (Diff revision 3)

I think these are reserved for temporary user extensions, so you should probably use another range.

I'd suggest that you just number the functions as 0x60 and 0x61 and make a note that the 0x60-0x6f range is intended for SE mode calls.

Show all issues

Looks good overall, but there are some minor issues I'd like you to look into. As far as I'm concerned, you can go ahead and commit this patch when you have fixed them.

src/sim/pseudo_inst.cc (Diff revision 4)

You use 0x56 and 0x57 in m5ops.h and 0x60 and 0x61 here. Is it ok that these do not match?

Show all issues

1. Alexandru Dutu Oct. 13, 2014, 8:58 p.m. (Oct. 13, 2014, 8:58 p.m.)

   Thanks for detecting this, the values in m5ops.h should be updated to 0x60 and 0x61 as well.

src/arch/x86/pseudo_inst.hh (Diff revision 4)

I think this can probably be in the main X86ISA namespace. That would remove the need for a new magic macro up in src/SConscript.

Show all issues

src/arch/x86/pseudo_inst.hh (Diff revision 4)

These should be camel case, not all lower case. ie. m5Syscall and m5PageFault, depending on what you consider the boundary between words. These should also be called gem5*.

Show all issues

1. Andreas Sandberg Nov. 19, 2014, 6:17 a.m. (Nov. 19, 2014, 6:17 a.m.)

   I'd argue that we should stay with m5* in this case since the rest of the functions that are gem5-specific in the pseudo-inst interface is using that prefix. Having said that, we should probably rename them all at some point.

src/arch/x86/pseudo_inst.cc (Diff revision 4)

Don't put spaces inside the parenthesis.

Show all issues

src/arch/x86/pseudo_inst.cc (Diff revision 4)

Put spaces around the operator.

Show all issues

src/arch/x86/pseudo_inst.cc (Diff revision 4)

Remove extra space.

Show all issues

util/m5/m5ops.h (Diff revision 4)

Why do we need psuedo ops for syscalls when there are actual syscall instructions? The same goes for page faults. I'm not saying I know that we don't, I'm just surprised that that would be necessary.

It seems unlikely that a program is going to realize it's about to cause a pagefault and obligingly call a special instruction first.

Show all issues

1. Andreas Sandberg Nov. 19, 2014, 6:17 a.m. (Nov. 19, 2014, 6:17 a.m.)

   The problem is that KVM doesn't normally intercept syscalls. This means that they need a small syscall handler and fault handler stub to support SE mode in KVM (see RB #2322). There are other ways of communicating with the host (e.g., IO ports, hypervisor calls). The beauty of this solution is that it is supported seamlessly by the simulated CPUs, which makes it possible to do CPU switching reliably (we need to handle situation where the switch takes place while the stub code is executing). It should be fairly straight forward to extend this to simulated CPUs, making mixed FS/SE mode simulation possible.

2. Alexandru Dutu Nov. 19, 2014, 4 p.m. (Nov. 19, 2014, 4 p.m.)

   Initially I have implemented syscall and pagefault intercepts using IO ports. This implementation imposes all CPU models to be aware of the magic ports KvmCPU is using to trap syscalls and page faults. Andreas suggested the MMIO interface as it is already commun to all CPU models.

src/arch/arm/pseudo_inst.cc (Diff revision 4)

I'd suggest that you simplify this and get rid of all the arch-specific panic code and write it once in arch/generic/pseudo_inst.(cc|hh) and then import it in arch/XX/pseudo_inst.hh.

See arch/mips/mmapped_ipr.hh and arch/generic/mmapped_ipr.hh for an example.

Show all issues

Except for the issues pointed out by Gabe and Nilay, I think this looks good.