> On Jan. 6, 2015, 11:21 a.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. > > Gabe Black wrote: > 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.
Ah, I see... I had the order of these two patches reversed in my head, hence my confusion. Makes a lot more sense now. I'll just say that adding some comments to 2591 similar to the ones you're getting rid of here would be nice. - Steve ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2590/#review5739 ----------------------------------------------------------- On Dec. 22, 2014, 4:37 p.m., Gabe Black wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2590/ > ----------------------------------------------------------- > > (Updated Dec. 22, 2014, 4:37 p.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 [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
