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

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 11 Oct 2016 10:24:38 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Oct 11, 2016 at 03:21:05PM +0200, Igor Mammedov wrote:
> > On Tue, 11 Oct 2016 08:58:02 -0300
> > Eduardo Habkost <ehabkost@redhat.com> 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@redhat.com> 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@redhat.com> 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.  
> 
> See git URLs on cover letter. Series is based on my x86-next branch.
> 
> ] This series can be seen in the git branch at:
> ]   https://github.com/ehabkost/qemu-hacks.git work/query-cpu-definitions-runnable-info
> ]
> ] The series is based on my x86-next branch:
> ]   https://github.com/ehabkost/qemu.git x86-next
I've used this one from yesterday as base and it didn't have
 "qmp: Add runnability information to query-cpu-definitions"

I'll refetch and try again.