Re: [Qemu-devel] [PATCH] target-i386: Don't cpu->migratable field when filtering features
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--- target-i386/cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Tested-by: Ján Tomko signature.asc Description: Digital signature
Re: [Qemu-devel] [PATCH] target-i386: Don't cpu->migratable field when filtering features
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=" (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,+", > > s/cange/change/ Fixed. > > > where 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 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 Thanks! -- Eduardo
Re: [Qemu-devel] [PATCH] target-i386: Don't cpu->migratable field when filtering features
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=" (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,+", s/cange/change/ > where 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 > --- > 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 -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
[Qemu-devel] [PATCH] target-i386: Don't cpu->migratable field 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 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=" (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,+", where 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 --- target-i386/cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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]; -- 2.7.4