----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2148/#review4869 -----------------------------------------------------------
Looking at this briefly, I'd say I'm with Andreas in spirit but with Nilay in practice. The implicit split is ugly, but that's just the way the PC platform works, for purely historical reasons, and it's not ever going to change. I don't see any benefit in exposing these warts to users if we don't have to. Aesthetically, it might be nice to make a better separation between the "generic" x86 architecture and the "PC platform", as this wart is clearly located in the latter. However, I doubt anyone would ever build a non-PC x86 platform, so I doubt the effort would have any non-aesthetic value. That said, the code really does need some comments explaining what's going on. configs/common/FSConfig.py <http://reviews.gem5.org/r/2148/#comment4547> It's more direct to do: mem_size = convert.toMemorySize(mdesc.mem()) configs/common/FSConfig.py <http://reviews.gem5.org/r/2148/#comment4548> I also think it would be clearer to pu the 3GB value in a variable and reuse it rather than calculating it twice. You could even generate it with: convert.toMemorySize('3GB') which would be clearer and more consistent configs/common/FSConfig.py <http://reviews.gem5.org/r/2148/#comment4549> Another comment would be nice; perhaps also an assertion that len(self.mem_ranges) <= 2 - Steve Reinhardt On Jan. 17, 2014, 6:38 p.m., Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2148/ > ----------------------------------------------------------- > > (Updated Jan. 17, 2014, 6:38 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10015:ad6b02503cc7 > --------------------------- > config: allow more than 3GB of memory for x86 simulations > This patch edits the configuration files so that x86 simulations can have > more than 3GB of memory. It also corrects a bug in the MemConfig.py script. > > > Diffs > ----- > > configs/common/FSConfig.py a362694dda2d > configs/common/MemConfig.py a362694dda2d > configs/example/fs.py a362694dda2d > > Diff: http://reviews.gem5.org/r/2148/diff/ > > > Testing > ------- > > > Thanks, > > Nilay Vaish > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
