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 its feature bitmap with the feature bitmaps of
the known CPU models.

I.e. the output of virsh domcapabilities would change from
 ...
     <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 | 51 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 46 insertions(+), 5 deletions(-)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 63903c2..dc3371f 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -283,14 +283,43 @@ 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;
+
+    /* 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 +329,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

On 30.06.2017 15:25, Viktor Mihajlovski wrote:
> 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 its feature bitmap with the feature bitmaps of
> the known CPU models.
> 
> I.e. the output of virsh domcapabilities would change from
>  ...
>      <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 | 51 ++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 46 insertions(+), 5 deletions(-)
> 
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 63903c2..dc3371f 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -283,14 +283,43 @@ 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;
> +
> +    /* 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);

I remember discussing this with Eduardo when he introduced this.

There is one additional case to handle here that might turn a CPU model
not runnable. This can happen when trying to run a new CPU model on an
old host, where the features are not the problem, but the model itself.
E.g. running a GA2 version on a GA1 is not allowed. But this can happen
more generally also between hardware generations.

Therefore, whenever the model is never than the host model, we have to
add here the special property "type" as missing feature. The
documentation partly coveres what we had in mind.

qapi-schema.json:
# @unavailable-features is a list of QOM property names that

# represent CPU model attributes that prevent the CPU from running.

# If the QOM property is read-only, that means there's no known

# way to make the CPU model run in the current host. Implementations

# that choose not to provide specific information return the

# property name "type".

We discussed that "type" should always be added if there is no way to
make the model runnable (by simply removing features).

See check_compatibility() for details. For these cases, add "type" to
the list. (you might be able to extend check_compatibility(), making
e.g. the *errp and *unavailable parameters optional).

-- 

Thanks,

David

On 30.06.2017 18:47, David Hildenbrand wrote:
> On 30.06.2017 15:25, Viktor Mihajlovski wrote:
>> 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 its feature bitmap with the feature bitmaps of
>> the known CPU models.
>>
>> I.e. the output of virsh domcapabilities would change from
>>  ...
>>      <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 | 51 ++++++++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 46 insertions(+), 5 deletions(-)
>>
>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>> index 63903c2..dc3371f 100644
>> --- a/target/s390x/cpu_models.c
>> +++ b/target/s390x/cpu_models.c
>> @@ -283,14 +283,43 @@ 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;
>> +
>> +    /* 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);
> 
> I remember discussing this with Eduardo when he introduced this.
> 
> There is one additional case to handle here that might turn a CPU model
> not runnable. This can happen when trying to run a new CPU model on an
> old host, where the features are not the problem, but the model itself.
> E.g. running a GA2 version on a GA1 is not allowed. But this can happen
> more generally also between hardware generations.
> 
> Therefore, whenever the model is never than the host model, we have to
> add here the special property "type" as missing feature. The
> documentation partly coveres what we had in mind.>
> qapi-schema.json:
> # @unavailable-features is a list of QOM property names that
> 
> # represent CPU model attributes that prevent the CPU from running.
> 
> # If the QOM property is read-only, that means there's no known
> 
> # way to make the CPU model run in the current host. Implementations
> 
> # that choose not to provide specific information return the
> 
> # property name "type".
> 
> We discussed that "type" should always be added if there is no way to
> make the model runnable (by simply removing features).
Thanks for the clarification.
I guess I was reading that too narrow-minded (no specific information vs
type), although I had the suspicion that features alone wouldn't
suffice. I will follow the query_cpu_model_comparison pattern and return
both "type" and the real features.
> > See check_compatibility() for details. For these cases, add "type" to
> the list. (you might be able to extend check_compatibility(), making
> e.g. the *errp and *unavailable parameters optional).
> 

-- 

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