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

Commit Message

Eduardo Habkost Oct. 7, 2016, 8:29 p.m. UTC

Fill the "unavailable-features" field on the x86 implementation
of query-cpu-definitions.

Cc: Jiri Denemark <[e-mail]>
Cc: [e-mail]
Signed-off-by: Eduardo Habkost <[e-mail]>
---
Changes v5 -> v6:
* Call x86_cpu_filter_features(), now that x86_cpu_load_features()
  won't run it automatically

Changes v4 -> v5:
* (none)

Changes v3 -> v4:
* Handle missing XSAVE components cleanly, but looking up
  the original feature that required it
* Use x86_cpu_load_features() function

Changes v2 -> v3:
* Create a x86_cpu_feature_name() function, to
  isolate the code that returns the property name

Changes v1 -> v2:
* Updated to the new schema: no @runnable field, and
  always report @unavailable-features as present
---
 target-i386/cpu.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

Comments

Igor Mammedov Oct. 10, 2016, 12:27 p.m. UTC | #1

On Fri,  7 Oct 2016 17:29:02 -0300
Eduardo Habkost <[e-mail]> wrote:

> Fill the "unavailable-features" field on the x86 implementation
> of query-cpu-definitions.
> 
> Cc: Jiri Denemark <[e-mail]>
> Cc: [e-mail]
> Signed-off-by: Eduardo Habkost <[e-mail]>
> ---
> Changes v5 -> v6:
> * Call x86_cpu_filter_features(), now that x86_cpu_load_features()
>   won't run it automatically
> 
> Changes v4 -> v5:
> * (none)
> 
> Changes v3 -> v4:
> * Handle missing XSAVE components cleanly, but looking up
>   the original feature that required it
> * Use x86_cpu_load_features() function
> 
> Changes v2 -> v3:
> * Create a x86_cpu_feature_name() function, to
>   isolate the code that returns the property name
> 
> Changes v1 -> v2:
> * Updated to the new schema: no @runnable field, and
>   always report @unavailable-features as present
> ---
>  target-i386/cpu.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 23cc19b..63330ce 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1945,6 +1945,27 @@ static inline void feat2prop(char *s)
>      }
>  }
>  
> +/* Return the feature property name for a feature flag bit */
> +static const char *x86_cpu_feature_name(FeatureWord w, int bitnr)
> +{
> +    /* XSAVE components are automatically enabled by other features,
> +     * so return the original feature name instead
> +     */
> +    if (w == FEAT_XSAVE_COMP_LO || w == FEAT_XSAVE_COMP_HI) {
> +        int comp = (w == FEAT_XSAVE_COMP_HI) ? bitnr + 32 : bitnr;
> +
> +        if (comp < ARRAY_SIZE(x86_ext_save_areas) &&
> +            x86_ext_save_areas[comp].bits) {
> +            w = x86_ext_save_areas[comp].feature;
> +            bitnr = ctz32(x86_ext_save_areas[comp].bits);
> +        }
> +    }
> +
> +    assert(bitnr < 32);
> +    assert(w < FEATURE_WORDS);
> +    return feature_word_info[w].feat_names[bitnr];
> +}
> +
>  /* Compatibily hack to maintain legacy +-feat semantic,
>   * where +-feat overwrites any feature set by
>   * feat=on|feat even if the later is parsed after +-feat
> @@ -2030,6 +2051,59 @@ static void x86_cpu_parse_featurestr(const char *typename, char *features,
>      }
>  }
>  
> +static void x86_cpu_load_features(X86CPU *cpu, Error **errp);
> +static int x86_cpu_filter_features(X86CPU *cpu);
> +
> +/* Check for missing features that may prevent the CPU class from
> + * running using the current machine and accelerator.
> + */
> +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()


> +    object_unref(OBJECT(xc));
> +}
> +
>  /* Print all cpuid feature names in featureset
>   */
>  static void listflags(FILE *f, fprintf_function print, const char **featureset)
> @@ -2122,6 +2196,8 @@ static void x86_cpu_definition_entry(gpointer data, gpointer user_data)
>  
>      info = g_malloc0(sizeof(*info));
>      info->name = x86_cpu_class_get_model_name(cc);
> +    x86_cpu_class_check_missing_features(cc, &info->unavailable_features);
> +    info->has_unavailable_features = true;
>  
>      entry = g_malloc0(sizeof(*entry));
>      entry->value = info;

Eduardo Habkost Oct. 10, 2016, 5:01 p.m. UTC | #2

On Mon, Oct 10, 2016 at 02:27:49PM +0200, Igor Mammedov wrote:
> On Fri,  7 Oct 2016 17:29:02 -0300
> Eduardo Habkost <[e-mail]> wrote:
> 
> > Fill the "unavailable-features" field on the x86 implementation
> > of query-cpu-definitions.
> > 
> > Cc: Jiri Denemark <[e-mail]>
> > Cc: [e-mail]
> > Signed-off-by: Eduardo Habkost <[e-mail]>
> > ---
> > Changes v5 -> v6:
> > * Call x86_cpu_filter_features(), now that x86_cpu_load_features()
> >   won't run it automatically
> > 
> > Changes v4 -> v5:
> > * (none)
> > 
> > Changes v3 -> v4:
> > * Handle missing XSAVE components cleanly, but looking up
> >   the original feature that required it
> > * Use x86_cpu_load_features() function
> > 
> > Changes v2 -> v3:
> > * Create a x86_cpu_feature_name() function, to
> >   isolate the code that returns the property name
> > 
> > Changes v1 -> v2:
> > * Updated to the new schema: no @runnable field, and
> >   always report @unavailable-features as present
> > ---
> >  target-i386/cpu.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 76 insertions(+)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 23cc19b..63330ce 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1945,6 +1945,27 @@ static inline void feat2prop(char *s)
> >      }
> >  }
> >  
> > +/* Return the feature property name for a feature flag bit */
> > +static const char *x86_cpu_feature_name(FeatureWord w, int bitnr)
> > +{
> > +    /* XSAVE components are automatically enabled by other features,
> > +     * so return the original feature name instead
> > +     */
> > +    if (w == FEAT_XSAVE_COMP_LO || w == FEAT_XSAVE_COMP_HI) {
> > +        int comp = (w == FEAT_XSAVE_COMP_HI) ? bitnr + 32 : bitnr;
> > +
> > +        if (comp < ARRAY_SIZE(x86_ext_save_areas) &&
> > +            x86_ext_save_areas[comp].bits) {
> > +            w = x86_ext_save_areas[comp].feature;
> > +            bitnr = ctz32(x86_ext_save_areas[comp].bits);
> > +        }
> > +    }
> > +
> > +    assert(bitnr < 32);
> > +    assert(w < FEATURE_WORDS);
> > +    return feature_word_info[w].feat_names[bitnr];
> > +}
> > +
> >  /* Compatibily hack to maintain legacy +-feat semantic,
> >   * where +-feat overwrites any feature set by
> >   * feat=on|feat even if the later is parsed after +-feat
> > @@ -2030,6 +2051,59 @@ static void x86_cpu_parse_featurestr(const char *typename, char *features,
> >      }
> >  }
> >  
> > +static void x86_cpu_load_features(X86CPU *cpu, Error **errp);
> > +static int x86_cpu_filter_features(X86CPU *cpu);
> > +
> > +/* Check for missing features that may prevent the CPU class from
> > + * running using the current machine and accelerator.
> > + */
> > +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.

Igor Mammedov Oct. 11, 2016, 11:45 a.m. UTC | #3

On Mon, 10 Oct 2016 14:01:10 -0300
Eduardo Habkost <[e-mail]> 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 <[e-mail]> wrote:
> >   
> > > Fill the "unavailable-features" field on the x86 implementation
> > > of query-cpu-definitions.
> > > 
> > > Cc: Jiri Denemark <[e-mail]>
> > > Cc: [e-mail]
> > > Signed-off-by: Eduardo Habkost <[e-mail]>
> > > ---
> > > Changes v5 -> v6:
> > > * Call x86_cpu_filter_features(), now that x86_cpu_load_features()
> > >   won't run it automatically
> > > 
> > > Changes v4 -> v5:
> > > * (none)
> > > 
> > > Changes v3 -> v4:
> > > * Handle missing XSAVE components cleanly, but looking up
> > >   the original feature that required it
> > > * Use x86_cpu_load_features() function
> > > 
> > > Changes v2 -> v3:
> > > * Create a x86_cpu_feature_name() function, to
> > >   isolate the code that returns the property name
> > > 
> > > Changes v1 -> v2:
> > > * Updated to the new schema: no @runnable field, and
> > >   always report @unavailable-features as present
> > > ---
> > >  target-i386/cpu.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 76 insertions(+)
> > > 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index 23cc19b..63330ce 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -1945,6 +1945,27 @@ static inline void feat2prop(char *s)
> > >      }
> > >  }
> > >  
> > > +/* Return the feature property name for a feature flag bit */
> > > +static const char *x86_cpu_feature_name(FeatureWord w, int bitnr)
> > > +{
> > > +    /* XSAVE components are automatically enabled by other features,
> > > +     * so return the original feature name instead
> > > +     */
> > > +    if (w == FEAT_XSAVE_COMP_LO || w == FEAT_XSAVE_COMP_HI) {
> > > +        int comp = (w == FEAT_XSAVE_COMP_HI) ? bitnr + 32 : bitnr;
> > > +
> > > +        if (comp < ARRAY_SIZE(x86_ext_save_areas) &&
> > > +            x86_ext_save_areas[comp].bits) {
> > > +            w = x86_ext_save_areas[comp].feature;
> > > +            bitnr = ctz32(x86_ext_save_areas[comp].bits);
> > > +        }
> > > +    }
> > > +
> > > +    assert(bitnr < 32);
> > > +    assert(w < FEATURE_WORDS);
> > > +    return feature_word_info[w].feat_names[bitnr];
> > > +}
> > > +
> > >  /* Compatibily hack to maintain legacy +-feat semantic,
> > >   * where +-feat overwrites any feature set by
> > >   * feat=on|feat even if the later is parsed after +-feat
> > > @@ -2030,6 +2051,59 @@ static void x86_cpu_parse_featurestr(const char *typename, char *features,
> > >      }
> > >  }
> > >  
> > > +static void x86_cpu_load_features(X86CPU *cpu, Error **errp);
> > > +static int x86_cpu_filter_features(X86CPU *cpu);
> > > +
> > > +/* Check for missing features that may prevent the CPU class from
> > > + * running using the current machine and accelerator.
> > > + */
> > > +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.

Eduardo Habkost Oct. 11, 2016, 11:58 a.m. UTC | #4

On Tue, Oct 11, 2016 at 01:45:21PM +0200, Igor Mammedov wrote:
> On Mon, 10 Oct 2016 14:01:10 -0300
> Eduardo Habkost <[e-mail]> 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 <[e-mail]> 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.

Igor Mammedov Oct. 11, 2016, 1:21 p.m. UTC | #5

On Tue, 11 Oct 2016 08:58:02 -0300
Eduardo Habkost <[e-mail]> 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 <[e-mail]> 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 <[e-mail]> 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.

Eduardo Habkost Oct. 11, 2016, 1:24 p.m. UTC | #6

On Tue, Oct 11, 2016 at 03:21:05PM +0200, Igor Mammedov wrote:
> On Tue, 11 Oct 2016 08:58:02 -0300
> Eduardo Habkost <[e-mail]> 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 <[e-mail]> 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 <[e-mail]> 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

Igor Mammedov Oct. 11, 2016, 1:51 p.m. UTC | #7

On Tue, 11 Oct 2016 10:24:38 -0300
Eduardo Habkost <[e-mail]> 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 <[e-mail]> 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 <[e-mail]> 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 <[e-mail]> 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.

Igor Mammedov Oct. 11, 2016, 2:13 p.m. UTC | #8

On Fri,  7 Oct 2016 17:29:02 -0300
Eduardo Habkost <[e-mail]> wrote:

> Fill the "unavailable-features" field on the x86 implementation
> of query-cpu-definitions.
> 
> Cc: Jiri Denemark <[e-mail]>
> Cc: [e-mail]
> Signed-off-by: Eduardo Habkost <[e-mail]>
Reviewed-by: Igor Mammedov <[e-mail]>

> ---
> Changes v5 -> v6:
> * Call x86_cpu_filter_features(), now that x86_cpu_load_features()
>   won't run it automatically
> 
> Changes v4 -> v5:
> * (none)
> 
> Changes v3 -> v4:
> * Handle missing XSAVE components cleanly, but looking up
>   the original feature that required it
> * Use x86_cpu_load_features() function
> 
> Changes v2 -> v3:
> * Create a x86_cpu_feature_name() function, to
>   isolate the code that returns the property name
> 
> Changes v1 -> v2:
> * Updated to the new schema: no @runnable field, and
>   always report @unavailable-features as present
> ---
>  target-i386/cpu.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 23cc19b..63330ce 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1945,6 +1945,27 @@ static inline void feat2prop(char *s)
>      }
>  }
>  
> +/* Return the feature property name for a feature flag bit */
> +static const char *x86_cpu_feature_name(FeatureWord w, int bitnr)
> +{
> +    /* XSAVE components are automatically enabled by other features,
> +     * so return the original feature name instead
> +     */
> +    if (w == FEAT_XSAVE_COMP_LO || w == FEAT_XSAVE_COMP_HI) {
> +        int comp = (w == FEAT_XSAVE_COMP_HI) ? bitnr + 32 : bitnr;
> +
> +        if (comp < ARRAY_SIZE(x86_ext_save_areas) &&
> +            x86_ext_save_areas[comp].bits) {
> +            w = x86_ext_save_areas[comp].feature;
> +            bitnr = ctz32(x86_ext_save_areas[comp].bits);
> +        }
> +    }
> +
> +    assert(bitnr < 32);
> +    assert(w < FEATURE_WORDS);
> +    return feature_word_info[w].feat_names[bitnr];
> +}
> +
>  /* Compatibily hack to maintain legacy +-feat semantic,
>   * where +-feat overwrites any feature set by
>   * feat=on|feat even if the later is parsed after +-feat
> @@ -2030,6 +2051,59 @@ static void x86_cpu_parse_featurestr(const char *typename, char *features,
>      }
>  }
>  
> +static void x86_cpu_load_features(X86CPU *cpu, Error **errp);
> +static int x86_cpu_filter_features(X86CPU *cpu);
> +
> +/* Check for missing features that may prevent the CPU class from
> + * running using the current machine and accelerator.
> + */
> +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;
> +            }
> +        }
> +    }
> +
> +    object_unref(OBJECT(xc));
> +}
> +
>  /* Print all cpuid feature names in featureset
>   */
>  static void listflags(FILE *f, fprintf_function print, const char **featureset)
> @@ -2122,6 +2196,8 @@ static void x86_cpu_definition_entry(gpointer data, gpointer user_data)
>  
>      info = g_malloc0(sizeof(*info));
>      info->name = x86_cpu_class_get_model_name(cc);
> +    x86_cpu_class_check_missing_features(cc, &info->unavailable_features);
> +    info->has_unavailable_features = true;
>  
>      entry = g_malloc0(sizeof(*entry));
>      entry->value = info;

Eduardo Habkost Oct. 14, 2016, 2:59 p.m. UTC | #9

On Tue, Oct 11, 2016 at 04:13:44PM +0200, Igor Mammedov wrote:
> On Fri,  7 Oct 2016 17:29:02 -0300
> Eduardo Habkost <[e-mail]> wrote:
> 
> > Fill the "unavailable-features" field on the x86 implementation
> > of query-cpu-definitions.
> > 
> > Cc: Jiri Denemark <[e-mail]>
> > Cc: [e-mail]
> > Signed-off-by: Eduardo Habkost <[e-mail]>
> Reviewed-by: Igor Mammedov <[e-mail]>

Thanks. Applied to x86-next.

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 23cc19b..63330ce 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1945,6 +1945,27 @@  static inline void feat2prop(char *s)
     }
 }
 
+/* Return the feature property name for a feature flag bit */
+static const char *x86_cpu_feature_name(FeatureWord w, int bitnr)
+{
+    /* XSAVE components are automatically enabled by other features,
+     * so return the original feature name instead
+     */
+    if (w == FEAT_XSAVE_COMP_LO || w == FEAT_XSAVE_COMP_HI) {
+        int comp = (w == FEAT_XSAVE_COMP_HI) ? bitnr + 32 : bitnr;
+
+        if (comp < ARRAY_SIZE(x86_ext_save_areas) &&
+            x86_ext_save_areas[comp].bits) {
+            w = x86_ext_save_areas[comp].feature;
+            bitnr = ctz32(x86_ext_save_areas[comp].bits);
+        }
+    }
+
+    assert(bitnr < 32);
+    assert(w < FEATURE_WORDS);
+    return feature_word_info[w].feat_names[bitnr];
+}
+
 /* Compatibily hack to maintain legacy +-feat semantic,
  * where +-feat overwrites any feature set by
  * feat=on|feat even if the later is parsed after +-feat
@@ -2030,6 +2051,59 @@  static void x86_cpu_parse_featurestr(const char *typename, char *features,
     }
 }
 
+static void x86_cpu_load_features(X86CPU *cpu, Error **errp);
+static int x86_cpu_filter_features(X86CPU *cpu);
+
+/* Check for missing features that may prevent the CPU class from
+ * running using the current machine and accelerator.
+ */
+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;
+            }
+        }
+    }
+
+    object_unref(OBJECT(xc));
+}
+
 /* Print all cpuid feature names in featureset
  */
 static void listflags(FILE *f, fprintf_function print, const char **featureset)
@@ -2122,6 +2196,8 @@  static void x86_cpu_definition_entry(gpointer data, gpointer user_data)
 
     info = g_malloc0(sizeof(*info));
     info->name = x86_cpu_class_get_model_name(cc);
+    x86_cpu_class_check_missing_features(cc, &info->unavailable_features);
+    info->has_unavailable_features = true;
 
     entry = g_malloc0(sizeof(*entry));
     entry->value = info;