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

[Gleb Natapov <gleb@redhat.com>]

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@redhat.com>
> > > ---
> > > Cc: Gleb Natapov <gleb@redhat.com>
> > > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> > > Cc: kvm@vger.kernel.org
> > > Cc: libvir-list@redhat.com
> > > Cc: Jiri Denemark <jdenemar@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? :)

--
			Gleb.