The response for query-cpu-definitions didn't include the
unavailable-features field, which is used by libvirt to figure
out whether a certain cpu model is usable on the host.

The unavailable features are now computed by obtaining the host CPU
model and comparing it against the known CPU models. The comparison
takes into account the generation, the GA level and the feature
bitmaps. In the case of a CPU generation/GA level mismatch
a feature called "type" is reported to be missing.

As a result, the output of virsh domcapabilities would change
from something like
 ...
     <mode name='custom' supported='yes'>
      <model usable='unknown'>z10EC-base</model>
      <model usable='unknown'>z9EC-base</model>
      <model usable='unknown'>z196.2-base</model>
      <model usable='unknown'>z900-base</model>
      <model usable='unknown'>z990</model>
 ...
to
 ...
     <mode name='custom' supported='yes'>
      <model usable='yes'>z10EC-base</model>
      <model usable='yes'>z9EC-base</model>
      <model usable='no'>z196.2-base</model>
      <model usable='yes'>z900-base</model>
      <model usable='yes'>z990</model>
 ...

Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
---
 target/s390x/cpu_models.c | 58 +++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 53 insertions(+), 5 deletions(-)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 63903c2..51440ff 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -283,14 +283,50 @@ void s390_cpu_list(FILE *f, fprintf_function print)
     }
 }
 
+static S390CPUModel *get_max_cpu_model(Error **errp);
+
 #ifndef CONFIG_USER_ONLY
+static void list_add_feat(const char *name, void *opaque);
+
+static void check_unavailable_features(const S390CPUModel *max_model,
+                                       const S390CPUModel *model,
+                                       strList **unavailable)
+{
+    S390FeatBitmap missing;
+
+    /* check general model compatibility */
+    if (max_model->def->gen < model->def->gen ||
+        (max_model->def->gen == model->def->gen &&
+         max_model->def->ec_ga < model->def->ec_ga)) {
+        list_add_feat("type", unavailable);
+    }
+
+    /* detect missing features if any to properly report them */
+    bitmap_andnot(missing, model->features, max_model->features,
+                  S390_FEAT_MAX);
+    if (!bitmap_empty(missing, S390_FEAT_MAX)) {
+        s390_feat_bitmap_to_ascii(missing,
+                                  unavailable,
+                                  list_add_feat);
+    }
+}
+
+struct CpuDefinitionInfoListData {
+    CpuDefinitionInfoList *list;
+    Error **errp;
+};
+
 static void create_cpu_model_list(ObjectClass *klass, void *opaque)
 {
-    CpuDefinitionInfoList **cpu_list = opaque;
+    struct CpuDefinitionInfoListData *cpu_list_data = opaque;
+    CpuDefinitionInfoList **cpu_list = &cpu_list_data->list;
     CpuDefinitionInfoList *entry;
     CpuDefinitionInfo *info;
     char *name = g_strdup(object_class_get_name(klass));
     S390CPUClass *scc = S390_CPU_CLASS(klass);
+    Object *obj;
+    S390CPU *sc;
+    S390CPUModel *scm;
 
     /* strip off the -s390-cpu */
     g_strrstr(name, "-" TYPE_S390_CPU)[0] = 0;
@@ -300,21 +336,33 @@ static void create_cpu_model_list(ObjectClass *klass, void *opaque)
     info->migration_safe = scc->is_migration_safe;
     info->q_static = scc->is_static;
     info->q_typename = g_strdup(object_class_get_name(klass));
-
+    /* check for unavailable features */
+    obj = object_new(object_class_get_name(klass));
+    sc = S390_CPU(obj);
+    scm = get_max_cpu_model(cpu_list_data->errp);
+    if (scm && sc->model) {
+        info->has_unavailable_features = true;
+        check_unavailable_features(scm, sc->model, &info->unavailable_features);
+    }
 
     entry = g_malloc0(sizeof(*entry));
     entry->value = info;
     entry->next = *cpu_list;
     *cpu_list = entry;
+    object_unref(obj);
 }
 
 CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
 {
-    CpuDefinitionInfoList *list = NULL;
+    struct CpuDefinitionInfoListData list_data = {
+        .list = NULL,
+        .errp = errp,
+    };
 
-    object_class_foreach(create_cpu_model_list, TYPE_S390_CPU, false, &list);
+    object_class_foreach(create_cpu_model_list, TYPE_S390_CPU, false,
+                         &list_data);
 
-    return list;
+    return list_data.list;
 }
 
 static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
-- 
1.9.1

>  
> +static S390CPUModel *get_max_cpu_model(Error **errp);
> +
>  #ifndef CONFIG_USER_ONLY
> +static void list_add_feat(const char *name, void *opaque);

Wonder if we should declare all these prototypes at the beginning of the
file.

> +
> +static void check_unavailable_features(const S390CPUModel *max_model,
> +                                       const S390CPUModel *model,
> +                                       strList **unavailable)
> +{
> +    S390FeatBitmap missing;
> +
> +    /* check general model compatibility */
> +    if (max_model->def->gen < model->def->gen ||
> +        (max_model->def->gen == model->def->gen &&
> +         max_model->def->ec_ga < model->def->ec_ga)) {
> +        list_add_feat("type", unavailable);
> +    }
> +
> +    /* detect missing features if any to properly report them */
> +    bitmap_andnot(missing, model->features, max_model->features,
> +                  S390_FEAT_MAX);
> +    if (!bitmap_empty(missing, S390_FEAT_MAX)) {
> +        s390_feat_bitmap_to_ascii(missing,
> +                                  unavailable,
> +                                  list_add_feat);

This certainly fits into one line.

> +    }
> +}
> +
> +struct CpuDefinitionInfoListData {
> +    CpuDefinitionInfoList *list;
> +    Error **errp;
> +};
> +
>  static void create_cpu_model_list(ObjectClass *klass, void *opaque)
>  {
> -    CpuDefinitionInfoList **cpu_list = opaque;
> +    struct CpuDefinitionInfoListData *cpu_list_data = opaque;
> +    CpuDefinitionInfoList **cpu_list = &cpu_list_data->list;
>      CpuDefinitionInfoList *entry;
>      CpuDefinitionInfo *info;
>      char *name = g_strdup(object_class_get_name(klass));
>      S390CPUClass *scc = S390_CPU_CLASS(klass);
> +    Object *obj;
> +    S390CPU *sc;
> +    S390CPUModel *scm;
>  
>      /* strip off the -s390-cpu */
>      g_strrstr(name, "-" TYPE_S390_CPU)[0] = 0;
> @@ -300,21 +336,33 @@ static void create_cpu_model_list(ObjectClass *klass, void *opaque)
>      info->migration_safe = scc->is_migration_safe;
>      info->q_static = scc->is_static;
>      info->q_typename = g_strdup(object_class_get_name(klass));
> -
> +    /* check for unavailable features */
> +    obj = object_new(object_class_get_name(klass));
> +    sc = S390_CPU(obj);
> +    scm = get_max_cpu_model(cpu_list_data->errp);

Hmmmm, if this function fails, we will create the same error multiple
times (as there is no way to stop this function from iterating). And we
will fail to create a cpu model list in case there is no host cpu model,
which is a change in behavior (as we will report an error).

Would it be better to simply get the max model in
arch_query_cpu_definitions() and pass it via CpuDefinitionInfoListData,
instead of the errp variable?

Then you could simply skip the checks and set
info->has_unavailable_features = false in case there is no max model
(get_max_cpu_model() returned NULL / an error). (same behavior as for now)

Errors from get_max_cpu_model() then should be ignored and not reported.



> +    if (scm && sc->model) {
> +        info->has_unavailable_features = true;
> +        check_unavailable_features(scm, sc->model, &info->unavailable_features);
> +    }
>  
>      entry = g_malloc0(sizeof(*entry));
>      entry->value = info;
>      entry->next = *cpu_list;
>      *cpu_list = entry;
> +    object_unref(obj);
>  }
>  
>  CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
>  {
> -    CpuDefinitionInfoList *list = NULL;
> +    struct CpuDefinitionInfoListData list_data = {
> +        .list = NULL,
> +        .errp = errp,
> +    };
>  
> -    object_class_foreach(create_cpu_model_list, TYPE_S390_CPU, false, &list);
> +    object_class_foreach(create_cpu_model_list, TYPE_S390_CPU, false,
> +                         &list_data);
>  
> -    return list;
> +    return list_data.list;
>  }
>  
>  static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
> 


-- 

Thanks,

David

On 03.07.2017 11:25, David Hildenbrand wrote:
> 
>>  
>> +static S390CPUModel *get_max_cpu_model(Error **errp);
>> +
>>  #ifndef CONFIG_USER_ONLY
>> +static void list_add_feat(const char *name, void *opaque);
> 
> Wonder if we should declare all these prototypes at the beginning of the
> file.
> 
Don't know either. Looking around in QEMU, forward declarations for
static functions seem to be used sparsely (only when needed). I could
have reordered the functions to get around without forward decls but
this would have obscured the change. Maybe defer and clean up in a
general cleanup/refactoring?
>> +
>> +static void check_unavailable_features(const S390CPUModel *max_model,
>> +                                       const S390CPUModel *model,
>> +                                       strList **unavailable)
>> +{
>> +    S390FeatBitmap missing;
>> +
>> +    /* check general model compatibility */
>> +    if (max_model->def->gen < model->def->gen ||
>> +        (max_model->def->gen == model->def->gen &&
>> +         max_model->def->ec_ga < model->def->ec_ga)) {
>> +        list_add_feat("type", unavailable);
>> +    }
>> +
>> +    /* detect missing features if any to properly report them */
>> +    bitmap_andnot(missing, model->features, max_model->features,
>> +                  S390_FEAT_MAX);
>> +    if (!bitmap_empty(missing, S390_FEAT_MAX)) {
>> +        s390_feat_bitmap_to_ascii(missing,
>> +                                  unavailable,
>> +                                  list_add_feat);
> 
> This certainly fits into one line.
True, will change.
> 
>> +    }
>> +}
>> +
>> +struct CpuDefinitionInfoListData {
>> +    CpuDefinitionInfoList *list;
>> +    Error **errp;
>> +};
>> +
>>  static void create_cpu_model_list(ObjectClass *klass, void *opaque)
>>  {
>> -    CpuDefinitionInfoList **cpu_list = opaque;
>> +    struct CpuDefinitionInfoListData *cpu_list_data = opaque;
>> +    CpuDefinitionInfoList **cpu_list = &cpu_list_data->list;
>>      CpuDefinitionInfoList *entry;
>>      CpuDefinitionInfo *info;
>>      char *name = g_strdup(object_class_get_name(klass));
>>      S390CPUClass *scc = S390_CPU_CLASS(klass);
>> +    Object *obj;
>> +    S390CPU *sc;
>> +    S390CPUModel *scm;
>>  
>>      /* strip off the -s390-cpu */
>>      g_strrstr(name, "-" TYPE_S390_CPU)[0] = 0;
>> @@ -300,21 +336,33 @@ static void create_cpu_model_list(ObjectClass *klass, void *opaque)
>>      info->migration_safe = scc->is_migration_safe;
>>      info->q_static = scc->is_static;
>>      info->q_typename = g_strdup(object_class_get_name(klass));
>> -
>> +    /* check for unavailable features */
>> +    obj = object_new(object_class_get_name(klass));
>> +    sc = S390_CPU(obj);
>> +    scm = get_max_cpu_model(cpu_list_data->errp);
> 
> Hmmmm, if this function fails, we will create the same error multiple
> times (as there is no way to stop this function from iterating). And we
> will fail to create a cpu model list in case there is no host cpu model,
> which is a change in behavior (as we will report an error).
> 
> Would it be better to simply get the max model in
> arch_query_cpu_definitions() and pass it via CpuDefinitionInfoListData,
> instead of the errp variable?Simplifies things, I like it.
> 
> Then you could simply skip the checks and set
> info->has_unavailable_features = false in case there is no max model
> (get_max_cpu_model() returned NULL / an error). (same behavior as for now)
> 
> Errors from get_max_cpu_model() then should be ignored and not reported.
> 
Just to be sure: you suggest that I should call error_free() after
calling get_max_cpu_model, in order to prevent that the QMP command
query-cpu-definitions fails, right?
> 
> 
>> +    if (scm && sc->model) {
>> +        info->has_unavailable_features = true;
>> +        check_unavailable_features(scm, sc->model, &info->unavailable_features);
>> +    }
>>  
>>      entry = g_malloc0(sizeof(*entry));
>>      entry->value = info;
>>      entry->next = *cpu_list;
>>      *cpu_list = entry;
>> +    object_unref(obj);
>>  }
>>  
>>  CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
>>  {
>> -    CpuDefinitionInfoList *list = NULL;
>> +    struct CpuDefinitionInfoListData list_data = {
>> +        .list = NULL,
>> +        .errp = errp,
>> +    };
>>  
>> -    object_class_foreach(create_cpu_model_list, TYPE_S390_CPU, false, &list);
>> +    object_class_foreach(create_cpu_model_list, TYPE_S390_CPU, false,
>> +                         &list_data);
>>  
>> -    return list;
>> +    return list_data.list;
>>  }
>>  
>>  static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
>>
> 
> 


-- 

Mit freundlichen Grüßen/Kind Regards
   Viktor Mihajlovski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

>> Wonder if we should declare all these prototypes at the beginning of the
>> file.
>>
> Don't know either. Looking around in QEMU, forward declarations for
> static functions seem to be used sparsely (only when needed). I could
> have reordered the functions to get around without forward decls but
> this would have obscured the change. Maybe defer and clean up in a
> general cleanup/refactoring?

Yes, fine with me!
[...]

>>
>> Hmmmm, if this function fails, we will create the same error multiple
>> times (as there is no way to stop this function from iterating). And we
>> will fail to create a cpu model list in case there is no host cpu model,
>> which is a change in behavior (as we will report an error).
>>
>> Would it be better to simply get the max model in
>> arch_query_cpu_definitions() and pass it via CpuDefinitionInfoListData,
>> instead of the errp variable?Simplifies things, I like it.
>>
>> Then you could simply skip the checks and set
>> info->has_unavailable_features = false in case there is no max model
>> (get_max_cpu_model() returned NULL / an error). (same behavior as for now)
>>
>> Errors from get_max_cpu_model() then should be ignored and not reported.
>>
> Just to be sure: you suggest that I should call error_free() after
> calling get_max_cpu_model, in order to prevent that the QMP command
> query-cpu-definitions fails, right?

That would be my suggestion simply don't provide runability information,
if we can't tell (because the max model is not available - e.g. with old
KVM versions without complete CPU model support), hiding the error.

-- 

Thanks,

David