Re: [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:19:15AM -0200, Eduardo Habkost wrote:
> 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@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? :)
> 
> If anybody find reliable documentation about the 0xC0000001 CPUID bits,
> I would happily do it.  :-)
> 
That's easy :) Just check the kernel:

        /* cpuid 0xC0000001.edx */
        const u32 kvm_supported_word5_x86_features =
                F(XSTORE) | F(XSTORE_EN) | F(XCRYPT) | F(XCRYPT_EN) |
                F(ACE2) | F(ACE2_EN) | F(PHE) | F(PHE_EN) |
                F(PMM) | F(PMM_EN);

> 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

--
			Gleb.