| Message ID | 1476473294-11052-1-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 10/14/2016 02:28 PM, Eduardo Habkost wrote: Subject line is missing a word; perhaps s/don't/don't read/ > 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/ > 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(). > > 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/ > where <feature> is a non-migratable feature. The only existing > named migratable feature is "invtsc". s/migratable/non-migratable/ ? > > 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 <ehabkost@redhat.com> > --- > 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. Reviewed-by: Eric Blake <eblake@redhat.com>
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 <ehabkost@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 <eblake@redhat.com> Thanks!
On Fri, Oct 14, 2016 at 04:28:14PM -0300, Eduardo Habkost wrote: >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. > [...] > >Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >--- > target-i386/cpu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Tested-by: Ján Tomko <jtomko@redhat.com>
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 754e575..d95514c 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -2248,7 +2248,7 @@ static int x86_cpu_filter_features(X86CPU *cpu) for (w = 0; w < FEATURE_WORDS; w++) { uint32_t host_feat = - x86_cpu_get_supported_feature_word(w, cpu->migratable); + x86_cpu_get_supported_feature_word(w, false); uint32_t requested_features = env->features[w]; env->features[w] &= host_feat; cpu->filtered_features[w] = requested_features & ~env->features[w];
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 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(). 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>", where <feature> is a non-migratable feature. The only existing named migratable feature is "invtsc". 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 <ehabkost@redhat.com> --- target-i386/cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)