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

Gleb Natapov
2013-01-06T14:35:51Z

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.