[libvirt] [PATCH qom-cpu 11/11] target-i386: check/enforce: Check all feature words

[Eduardo Habkost]

On Mon, Jan 07, 2013 at 02:06:38PM +0200, Gleb Natapov wrote:
> On Mon, Jan 07, 2013 at 10:06:21AM -0200, Eduardo Habkost wrote:
> > On Sun, Jan 06, 2013 at 04:35:51PM +0200, Gleb Natapov wrote:
> > > On Fri, Jan 04, 2013 at 08:01:12PM -0200, Eduardo Habkost wrote:
> > > > This adds the following feature words to the list of flags to be checked
> > > > by kvm_check_features_against_host():
> > > > 
> > > >  - cpuid_7_0_ebx_features
> > > >  - ext4_features
> > > >  - kvm_features
> > > >  - svm_features
> > > > 
> > > > This will ensure the "enforce" flag works as it should: it won't allow
> > > > QEMU to be started unless every flag that was requested by the user or
> > > > defined in the CPU model is supported by the host.
> > > > 
> > > > This patch may cause existing configurations where "enforce" wasn't
> > > > preventing QEMU from being started to abort QEMU. But that's exactly the
> > > > point of this patch: if a flag was not supported by the host and QEMU
> > > > wasn't aborting, it was a bug in the "enforce" code.
> > > > 
> > > > Signed-off-by: Eduardo Habkost <ehabkost at redhat.com>
> > > > ---
> > > > Cc: Gleb Natapov <gleb at redhat.com>
> > > > Cc: Marcelo Tosatti <mtosatti at redhat.com>
> > > > Cc: kvm at vger.kernel.org
> > > > Cc: libvir-list at redhat.com
> > > > Cc: Jiri Denemark <jdenemar at redhat.com>
> > > > 
> > > > CCing libvirt people, as this is directly related to the planned usage
> > > > of the "enforce" flag by libvirt.
> > > > 
> > > > The libvirt team probably has a problem in their hands: libvirt should
> > > > use "enforce" to make sure all requested flags are making their way into
> > > > the guest (so the resulting CPU is always the same, on any host), but
> > > > users may have existing working configurations where a flag is not
> > > > supported by the guest and the user really doesn't care about it. Those
> > > > configurations will necessarily break when libvirt starts using
> > > > "enforce".
> > > > 
> > > > One example where it may cause trouble for common setups: pc-1.3 wants
> > > > the kvm_pv_eoi flag enabled by default (so "enforce" will make sure it
> > > > is enabled), but the user may have an existing VM running on a host
> > > > without pv_eoi support. That setup is unsafe today because
> > > > live-migration between different host kernel versions may enable/disable
> > > > pv_eoi silently (that's why we need the "enforce" flag to be used by
> > > > libvirt), but the user probably would like to be able to live-migrate
> > > > that VM anyway (and have libvirt to "just do the right thing").
> > > > 
> > > > One possible solution to libvirt is to use "enforce" only on newer
> > > > machine-types, so existing machines with older machine-types will keep
> > > > the unsafe host-dependent-ABI behavior, but at least would keep
> > > > live-migration working in case the user is careful.
> > > > 
> > > > I really don't know what the libvirt team prefers, but that's the
> > > > situation today. The longer we take to make "enforce" strict as it
> > > > should and make libvirt finally use it, more users will have VMs with
> > > > migration-unsafe unpredictable guest ABIs.
> > > > 
> > > > Changes v2:
> > > >  - Coding style fix
> > > > ---
> > > >  target-i386/cpu.c | 15 ++++++++++++---
> > > >  1 file changed, 12 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > index 876b0f6..52727ad 100644
> > > > --- a/target-i386/cpu.c
> > > > +++ b/target-i386/cpu.c
> > > > @@ -955,8 +955,9 @@ static int unavailable_host_feature(struct model_features_t *f, uint32_t mask)
> > > >      return 0;
> > > >  }
> > > >  
> > > > -/* best effort attempt to inform user requested cpu flags aren't making
> > > > - * their way to the guest.
> > > > +/* Check if all requested cpu flags are making their way to the guest
> > > > + *
> > > > + * Returns 0 if all flags are supported by the host, non-zero otherwise.
> > > >   *
> > > >   * This function may be called only if KVM is enabled.
> > > >   */
> > > > @@ -973,7 +974,15 @@ static int kvm_check_features_against_host(x86_def_t *guest_def)
> > > >          {&guest_def->ext2_features, &host_def.ext2_features,
> > > >              ext2_feature_name, 0x80000001, R_EDX},
> > > >          {&guest_def->ext3_features, &host_def.ext3_features,
> > > > -            ext3_feature_name, 0x80000001, R_ECX}
> > > > +            ext3_feature_name, 0x80000001, R_ECX},
> > > > +        {&guest_def->ext4_features, &host_def.ext4_features,
> > > > +            NULL, 0xC0000001, R_EDX},
> > > Since there is not name array for ext4_features they cannot be added or
> > > removed on the command line hence no need to check them, no?
> > 
> > In theory, yes. But it won't hurt to check it, and it will be useful to
> > unify the list of feature words in a single place, so we can be sure the
> > checking/filtering/setting code at
> > kvm_check_features_against_host()/kvm_filter_features_for_host()/kvm_cpu_fill_host(),
> > will all check/filter/set exactly the same feature words.
> > 
> May be add a name array for the leaf? :)

If anybody find reliable documentation about the 0xC0000001 CPUID bits,
I would happily do it.  :-)

While we don't have the docs and feature names, I still believe that
having the complete list of feature words in the
kvm_check_features_against_host() code will save us trouble later, for
the same reason we already filter the 0xC0000001 leaf in
filter_features_for_kvm() today.

-- 
Eduardo