| Message ID | 1499068251-6164-1-git-send-email-mihajlov@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> > +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, >
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, >> > >
>> 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.
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,
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(-)