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

login
register
mail settings
Submitter Eduardo Habkost
Date Jan. 4, 2013, 10:01 p.m.
Message ID <1357336872-7200-12-git-send-email-ehabkost@redhat.com>
Download mbox | patch
Permalink /patch/209562/
State New
Headers show

Comments

Eduardo Habkost - Jan. 4, 2013, 10:01 p.m.
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(-)
Gleb Natapov - Jan. 6, 2013, 2:35 p.m.
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?

> +        {&guest_def->cpuid_7_0_ebx_features, &host_def.cpuid_7_0_ebx_features,
> +            cpuid_7_0_ebx_feature_name, 7, R_EBX},
> +        {&guest_def->svm_features, &host_def.svm_features,
> +            svm_feature_name, 0x8000000A, R_EDX},
> +        {&guest_def->kvm_features, &host_def.kvm_features,
> +            kvm_feature_name, KVM_CPUID_FEATURES, R_EAX},
>      };
>  
>      assert(kvm_enabled());
> -- 
> 1.7.11.7

--
			Gleb.
Eduardo Habkost - Jan. 7, 2013, 12:06 p.m.
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.

> 
> > +        {&guest_def->cpuid_7_0_ebx_features, &host_def.cpuid_7_0_ebx_features,
> > +            cpuid_7_0_ebx_feature_name, 7, R_EBX},
> > +        {&guest_def->svm_features, &host_def.svm_features,
> > +            svm_feature_name, 0x8000000A, R_EDX},
> > +        {&guest_def->kvm_features, &host_def.kvm_features,
> > +            kvm_feature_name, KVM_CPUID_FEATURES, R_EAX},
> >      };
> >  
> >      assert(kvm_enabled());
> > -- 
> > 1.7.11.7
> 
> --
> 			Gleb.
Gleb Natapov - Jan. 7, 2013, 12:06 p.m.
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.
Eduardo Habkost - Jan. 7, 2013, 12:19 p.m.
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.  :-)

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.
Gleb Natapov - Jan. 7, 2013, 12:23 p.m.
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.

Patch

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},
+        {&guest_def->cpuid_7_0_ebx_features, &host_def.cpuid_7_0_ebx_features,
+            cpuid_7_0_ebx_feature_name, 7, R_EBX},
+        {&guest_def->svm_features, &host_def.svm_features,
+            svm_feature_name, 0x8000000A, R_EDX},
+        {&guest_def->kvm_features, &host_def.kvm_features,
+            kvm_feature_name, KVM_CPUID_FEATURES, R_EAX},
     };
 
     assert(kvm_enabled());