> On Jan. 6, 2015, 7:21 p.m., Steve Reinhardt wrote: > > Can you be more specific about what doesn't work? Do we really need to > > back out all of the enabled features? > > > > Also, it would be nice to replace the comments with different comments, > > rather than just getting rid of them. If you want to keep this as a pure > > backout of the previous commit and make the other changes separately, that > > would be fine.
I can't point to exactly what doesn't work, but software that used to run no longer runs with this CL. Another user reported similar issues, although I don't know what they were running. Essentially we're telling the simulated software it can use features that aren't actually implemented (or don't make sense, like SVM on an intel system). If the host CPU doesn't get upset, the simulated software might try actually using those features, and then something bad will likely happen. Having comments are a good thing, but those comments weren't great. Those CPUID results hold more than just feature bits, first of all. Second, it's not well defined to say, effectively, that everything is enabled except such and such. The set those exceptions are from will change over time, making the comment invalid. It's also more relevant what features enabled than which are disabled. My intention is to revert the original change here, and then to put up a new version which does what the original was trying to do. I have that CL posted over here http://reviews.gem5.org/r/2591/ which you reviewed. The features that needed to be enabled to get things working for KVM in SE mode (why those I have no idea) were a small subset of those enabled by this change. That was a while ago, but I think I also turned on one or two other things which we support but weren't telling software about. - Gabe ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2590/#review5739 ----------------------------------------------------------- On Dec. 23, 2014, 12:37 a.m., Gabe Black wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2590/ > ----------------------------------------------------------- > > (Updated Dec. 23, 2014, 12:37 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10607:334a1f4026ca > --------------------------- > cpuid, x86: Revert "Enabling more features in CPUid" > > That change enables CPUID bits for features that aren't implemented in gem5. > If a simulated system tries to use those features because it was told it > could, bad things can happen. > > > Diffs > ----- > > src/arch/x86/cpuid.cc a0cb57e1c072965dcdd51465beff37b264b41424 > > Diff: http://reviews.gem5.org/r/2590/diff/ > > > Testing > ------- > > > Thanks, > > Gabe Black > > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
