Re: [libvirt] [PATCH v6 3/3] target-i386: Return runnability information on query-cpu-definitions

On Tue, 11 Oct 2016 08:58:02 -0300 Eduardo Habkost <ehabkost(a)redhat.com&gt; wrote: > On Tue, Oct 11, 2016 at 01:45:21PM +0200, Igor Mammedov wrote: > > On Mon, 10 Oct 2016 14:01:10 -0300 > > Eduardo Habkost <ehabkost(a)redhat.com&gt; wrote: > > > On Mon, Oct 10, 2016 at 02:27:49PM +0200, Igor Mammedov wrote: > > > > On Fri, 7 Oct 2016 17:29:02 -0300 > > > > Eduardo Habkost <ehabkost(a)redhat.com&gt; wrote: > [...] > > > > > +static void x86_cpu_class_check_missing_features(X86CPUClass *xcc, > > > > > + strList **missing_feats) > > > > > +{ > > > > > + X86CPU *xc; > > > > > + FeatureWord w; > > > > > + Error *err = NULL; > > > > > + strList **next = missing_feats; > > > > > + > > > > > + if (xcc->kvm_required && !kvm_enabled()) { > > > > > + strList *new = g_new0(strList, 1); > > > > > + new->value = g_strdup("kvm");; > > > > > + *missing_feats = new; > > > > > + return; > > > > > + } > > > > > + > > > > > + xc = X86_CPU(object_new(object_class_get_name(OBJECT_CLASS(xcc)))); > > > > > + > > > > > + x86_cpu_load_features(xc, &err); > > > > > + if (err) { > > > > > + /* Errors at x86_cpu_load_features should never happen, > > > > > + * but in case it does, just report the model as not > > > > > + * runnable at all using the "type" property. > > > > > + */ > > > > > + strList *new = g_new0(strList, 1); > > > > > + new->value = g_strdup("type"); > > > > > + *next = new; > > > > > + next = &new->next; > > > > > + } > > > > > + > > > > > + x86_cpu_filter_features(xc); > > > > > + > > > > > + for (w = 0; w < FEATURE_WORDS; w++) { > > > > > + uint32_t filtered = xc->filtered_features[w]; > > > > > + int i; > > > > > + for (i = 0; i < 32; i++) { > > > > > + if (filtered & (1UL << i)) { > > > > > + strList *new = g_new0(strList, 1); > > > > > + new->value = g_strdup(x86_cpu_feature_name(w, i)); > > > > > + *next = new; > > > > > + next = &new->next; > > > > > + } > > > > > + } > > > > > + } > > > > Shouldn't you add > > > > if (IS_AMD_CPU(env)) { > > > > fixup here, that realize does right after calling x86_cpu_filter_features() > > > > > > What would it be useful for? The IS_AMD_CPU fixup runs after > > > x86_cpu_filter_features() (so it doesn't affect filtered_features > > > at all), and filtered_features is the only field used as input to > > > build missing_feats. > > For completeness of features returned by query-cpu-definitions, I'd guess. > > So that returned cpu definitions would match actually created cpus. > > For completeness of which query-cpu-definitions field, exactly? > There's no field in the return value of query-cpu-definitions > that would be affected by the AMD aliases. The AMD aliases don't > affect runnability of the CPU model because they aren't included > in the GET_SUPPORTED_CPUID check[1]. > > You would be right if we did return a copy of the low-level CPUID > data that's seen by the guest, or if the AMD aliases were set up > before x86_cpu_filter_features() (so they could affect > filtered_features/unavailable-features), but that's not the case. > > [1] They aren't included in the GET_SUPPORTED_CPUID check because > the existence of the AMD aliases depend only on the > configured guest vendor ID, not on the host CPU. > Got it. I've tried to build with this patch but build fails with make -j32 CHK version_gen.h CC i386-linux-user/target-i386/cpu.o target-i386/cpu.c: In function ‘x86_cpu_definition_entry’: target-i386/cpu.c:2199:51: error: ‘CpuDefinitionInfo’ has no member named ‘unavailable_features’ x86_cpu_class_check_missing_features(cc, &info->unavailable_features); ^ target-i386/cpu.c:2200:9: error: ‘CpuDefinitionInfo’ has no member named ‘has_unavailable_features’ info->has_unavailable_features = true; Probably series misses a patch that adds it.