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

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(a)redhat.com&gt; > > > > --- > > > > Cc: Gleb Natapov <gleb(a)redhat.com&gt; > > > > Cc: Marcelo Tosatti <mtosatti(a)redhat.com&gt; > > > > Cc: kvm(a)vger.kernel.org > > > > Cc: libvir-list(a)redhat.com > > > > Cc: Jiri Denemark <jdenemar(a)redhat.com&gt; > > > > > > > > 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. -- Eduardo