On Fri, Oct 14, 2016 at 02:36:52PM -0500, Eric Blake wrote: > On 10/14/2016 02:28 PM, Eduardo Habkost wrote: > > Subject line is missing a word; perhaps s/don't/don't read/
Changed to: "target-i386: Don't use cpu->migratable when filtering features". > > > When explicitly enabling unmigratable flags using "-cpu host" > > (e.g. "-cpu host,+invtsc"), the requested feature won't be > > enabled because cpu->migratable is true by default. > > > > This is inconsistent with all other CPU models, which don't have > > the "migratable" option, making "+invtsc" work without the need > > for extra options. > > > > This happens because x86_cpu_filter_features() uses > > cpu->migratable as argument for > > s/as/as an/ Fixed. > > > x86_cpu_get_supported_feature_word(). This is not useful > > because: > > 2) on "-cpu host" it only makes QEMU disable features that were > > explicitly enabled in the command-line; > > 1) on all the other CPU models, cpu->migratable is already false. > > > > The fix is to just use 'false' as argument to > > x86_cpu_get_supported_feature_word() in > > x86_cpu_filter_features(). s/as/as an/ here, too. > > > > Note that: > > > > * This won't change anything for people using using > > "-cpu host" or "-cpu host,migratable=<on|off>" (with no extra > > features) because the x86_cpu_get_supported_feature_word() call > > on the cpu->host_features check uses cpu->migratable as > > argument. > > * This won't change anything for any CPU model except "host" > > because they all have cpu->migratable == false (and only "host" > > has the "migratable" property that allows it to be changed). > > * This will only cange things for people using "-cpu host,+<feature>", > > s/cange/change/ Fixed. > > > where <feature> is a non-migratable feature. The only existing > > named migratable feature is "invtsc". > > s/migratable/non-migratable/ ? Fixed. > > > > > In other words, this change will only affect people using > > "-cpu host,+invtsc" (that will now get what they asked for: the > > invtsc flag will be enabled). All other use cases are unaffected. > > > > Signed-off-by: Eduardo Habkost <ehabk...@redhat.com> Ouch, the hurry to write the commit the message in time submit this patch before the end of the day is noticeable. Thanks for the review! > > --- > > target-i386/cpu.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > Love the commit:patch signal-to-noise ratio :) But the lengthy > explanation is vital, so keep it that way. Yes, the rules and use cases behind the command-line options are not trivial, so I wanted to be very explicit about the case the patch affects, and the cases it doesn't affect. > > Reviewed-by: Eric Blake <ebl...@redhat.com> Thanks! -- Eduardo
