It's nice to see this getting close to done... Did you verify there were no stats changes at all? One of my biggest issues with it is the extra indenting that is used a lot of places to do something like:

func() 
{
     if (FullSystem) {
          ....
          ....
          ....
          ....
          ....
          ....
          ....
          ....
     }
}

and I'd much rather see:

func()
{
    if (!FullSystem)
          return;
    ....
    ....
}

configs/common/Simulation.py (Diff revision 1)

Is there any issue with setting workload on a CPU that is running in full system mode?

1. Gabe Black Jan. 9, 2012, 5 p.m. (Jan. 9, 2012, 5 p.m.)

   There shouldn't be any ill effect because that should be ignored. In practice I haven't tried it, so I wouldn't rule anything out. Once this big first cut is in place, I'll start going through these sorts of things and clean them up. Unifying things in a more seamless way will take more invasive and significant reworking and would make this already very large set of changes even bigger.

src/arch/alpha/AlphaSystem.py (Diff revision 1)

Why is there a blank line here?

1. Gabe Black Jan. 9, 2012, 5 p.m. (Jan. 9, 2012, 5 p.m.)

   To separate the parameter stuff from the non-parameter stuff.

src/arch/alpha/faults.cc (Diff revision 1)

I'm not crazy about replacing #if with if () and all the indenting that goes with it. Could we maybe structure these things like if (FullSystem)
return ....

....?

1. Gabe Black Jan. 9, 2012, 5:02 p.m. (Jan. 9, 2012, 5:02 p.m.)

   Not really since it isn't just skipping the function it's a choice between two options.

src/arch/alpha/isa/fp.isa (Diff revision 1)

Doesn't seem like FullSystem si needed here either as long as we initialize the ICSR register to have FP enabled for non-fullsystem mode

1. Gabe Black Jan. 9, 2012, 5:04 p.m. (Jan. 9, 2012, 5:04 p.m.)

   It's not really needed, but it saves a call to xc->readMiscReg. I can remove it, but that might affect performance.

src/arch/alpha/tlb.cc (Diff revision 1)

I imagine this would be one of those high-performance areas. It's probably not necessary to have the check here either. As long as PcPAL exists (i think it just checks if the PC is odd). 

1. Gabe Black Jan. 9, 2012, 5:04 p.m. (Jan. 9, 2012, 5:04 p.m.)

   This is probably the same thing, aka saving a call to req->getPC().

src/arch/alpha/utility.cc (Diff revision 1)

Here too can we structure there like:
if (!FullSystem)
   panic("getArgument()...")
...

the dummy return can go away.

1. Gabe Black Jan. 9, 2012, 5:05 p.m. (Jan. 9, 2012, 5:05 p.m.)

   Ok.

src/arch/arm/tlb.cc (Diff revision 1)

How is this code ever called this this is translateSe? It's already handled with the indirection between translateSe and translateFs

1. Gabe Black Jan. 9, 2012, 5:06 p.m. (Jan. 9, 2012, 5:06 p.m.)

   That's true. That's a transliteration of the #ifdef stuff. It can be removed.

src/arch/arm/tlb.cc (Diff revision 1)

This is definitely in the critical path. as long as bootUncachability is initialized to  true in SE mode this extra check isn't needed. Also this should already be handled by the indirection between translateSe and translateFs below

1. Gabe Black Jan. 9, 2012, 5:06 p.m. (Jan. 9, 2012, 5:06 p.m.)

   Yep.

src/arch/arm/tlb.cc (Diff revision 1)

very much in the critical path. 

1. Gabe Black Jan. 9, 2012, 5:06 p.m. (Jan. 9, 2012, 5:06 p.m.)

   Yep.

src/arch/power/tlb.cc (Diff revision 1)

less indent please

1. Gabe Black Jan. 9, 2012, 5:09 p.m. (Jan. 9, 2012, 5:09 p.m.)

   Ok.

src/arch/power/vtophys.cc (Diff revision 1)

is it posible to have this many authors and copyright holders on what is pretty much a null file?

1. Gabe Black Jan. 9, 2012, 5:10 p.m. (Jan. 9, 2012, 5:10 p.m.)

   Yes, but they don't belong in this one. Copyright is annoying.

src/arch/sparc/faults.cc (Diff revision 1)

return SparcFaultBase::invoke(tc, inst);

1. Gabe Black Jan. 9, 2012, 5:12 p.m. (Jan. 9, 2012, 5:12 p.m.)

   No, because invoke is a void function.

src/arch/sparc/isa/base.isa (Diff revision 1)

Same?

1. Gabe Black Jan. 9, 2012, 5:13 p.m. (Jan. 9, 2012, 5:13 p.m.)

   Ok.

src/arch/sparc/tlb.cc (Diff revision 1)

Seems like the se version should just get the data from the tag access register instead of having a different fault defined

1. Gabe Black Jan. 9, 2012, 5:15 p.m. (Jan. 9, 2012, 5:15 p.m.)

   They're not different faults, just different constructors. I don't follow what you're saying about the tag access register.

src/arch/sparc/tlb.cc (Diff revision 1)

same

I think now is the only time I imagine this would get cleaned up, so that is why I'm asking

src/arch/sparc/utility.cc (Diff revision 1)

Move the panic to the top and then have the rest of the code unidented?

1. Gabe Black Jan. 9, 2012, 5:16 p.m. (Jan. 9, 2012, 5:16 p.m.)

   Ok.

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

Same

1. Gabe Black Jan. 9, 2012, 5:17 p.m. (Jan. 9, 2012, 5:17 p.m.)

   This is choosing between alternatives, not skipping the function.

src/dev/alpha/backdoor.cc (Diff revision 1)

seems like a random change?

1. Gabe Black Jan. 9, 2012, 5:19 p.m. (Jan. 9, 2012, 5:19 p.m.)

   Yeah, I don't know why that was changed.

src/dev/alpha/backdoor.cc (Diff revision 1)

probably should be a fatal as it's not performance critical and would cause a segfault if it didn't cast

1. Gabe Black Jan. 9, 2012, 5:19 p.m. (Jan. 9, 2012, 5:19 p.m.)

   Ok.

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

if (!FullSystem)
   panic()

1. Gabe Black Jan. 9, 2012, 5:22 p.m. (Jan. 9, 2012, 5:22 p.m.)

   I don't see this line in the new patch. I'll assume this magically went away somehow.

It certainly is a great big diff. It looks fine to me, thanks for addressing my comments. I might have something minor, but I'd rather get it committed now and fix that later.