* [Qemu-devel] [PATCH] target-i386: Don't cpu->migratable field when filtering features
@ 2016-10-14 19:28 Eduardo Habkost
2016-10-14 19:36 ` Eric Blake
2016-10-17 10:20 ` Ján Tomko
0 siblings, 2 replies; 4+ messages in thread
From: Eduardo Habkost @ 2016-10-14 19:28 UTC (permalink / raw)
To: qemu-devel; +Cc: Marcelo Tosatti, Bandan Das, jtomko, Igor Mammedov
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(-)
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] target-i386: Don't cpu->migratable field when filtering features
2016-10-14 19:28 [Qemu-devel] [PATCH] target-i386: Don't cpu->migratable field when filtering features Eduardo Habkost
@ 2016-10-14 19:36 ` Eric Blake
2016-10-14 19:43 ` Eduardo Habkost
2016-10-17 10:20 ` Ján Tomko
1 sibling, 1 reply; 4+ messages in thread
From: Eric Blake @ 2016-10-14 19:36 UTC (permalink / raw)
To: Eduardo Habkost, qemu-devel
Cc: Igor Mammedov, Bandan Das, Marcelo Tosatti, jtomko
[-- Attachment #1: Type: text/plain, Size: 2293 bytes --]
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>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] target-i386: Don't cpu->migratable field when filtering features
2016-10-14 19:36 ` Eric Blake
@ 2016-10-14 19:43 ` Eduardo Habkost
0 siblings, 0 replies; 4+ messages in thread
From: Eduardo Habkost @ 2016-10-14 19:43 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Igor Mammedov, Bandan Das, Marcelo Tosatti, jtomko
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!
--
Eduardo
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] target-i386: Don't cpu->migratable field when filtering features
2016-10-14 19:28 [Qemu-devel] [PATCH] target-i386: Don't cpu->migratable field when filtering features Eduardo Habkost
2016-10-14 19:36 ` Eric Blake
@ 2016-10-17 10:20 ` Ján Tomko
1 sibling, 0 replies; 4+ messages in thread
From: Ján Tomko @ 2016-10-17 10:20 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: qemu-devel, Marcelo Tosatti, Bandan Das, Igor Mammedov
[-- Attachment #1: Type: text/plain, Size: 447 bytes --]
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>
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-10-17 10:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-14 19:28 [Qemu-devel] [PATCH] target-i386: Don't cpu->migratable field when filtering features Eduardo Habkost
2016-10-14 19:36 ` Eric Blake
2016-10-14 19:43 ` Eduardo Habkost
2016-10-17 10:20 ` Ján Tomko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.