Re: [Qemu-devel] [PATCH] target-i386: Don't cpu->migratable field when filtering features

[Eduardo Habkost]

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 <address@hidden>

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 <address@hidden>

Thanks!

-- 
Eduardo