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

[Gleb Natapov <gleb@redhat.com>]

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.