* [Qemu-devel] [PATCH v3] s390: return unavailable features via query-cpu-definitions
@ 2017-07-03 11:48 Viktor Mihajlovski
2017-07-04 9:35 ` David Hildenbrand
2017-07-04 12:06 ` Christian Borntraeger
0 siblings, 2 replies; 6+ messages in thread
From: Viktor Mihajlovski @ 2017-07-03 11:48 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, jjherne, david
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>
---
v1 -> v2:
Account for model generation and GA level, not only on features.
v2 -> v3:
Prevent repetitive failures by calling get_max_cpu_model only once.
Ignore get_max_cpu_model failures in query_cpu_definitions in order
return CPU models even if the host CPU model is not available.
target/s390x/cpu_models.c | 62 +++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 57 insertions(+), 5 deletions(-)
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 63903c2..7cb55dc 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -283,10 +283,41 @@ 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;
+ S390CPUModel *model;
+};
+
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));
@@ -300,7 +331,19 @@ 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 */
+ if (cpu_list_data->model) {
+ Object *obj;
+ S390CPU *sc;
+ obj = object_new(object_class_get_name(klass));
+ sc = S390_CPU(obj);
+ if (sc->model) {
+ info->has_unavailable_features = true;
+ check_unavailable_features(cpu_list_data->model, sc->model,
+ &info->unavailable_features);
+ }
+ object_unref(obj);
+ }
entry = g_malloc0(sizeof(*entry));
entry->value = info;
@@ -310,11 +353,20 @@ static void create_cpu_model_list(ObjectClass *klass, void *opaque)
CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
{
- CpuDefinitionInfoList *list = NULL;
+ struct CpuDefinitionInfoListData list_data = {
+ .list = NULL,
+ };
+
+ list_data.model = get_max_cpu_model(errp);
+ if (*errp) {
+ error_free(*errp);
+ *errp = NULL;
+ }
- 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] s390: return unavailable features via query-cpu-definitions
2017-07-03 11:48 [Qemu-devel] [PATCH v3] s390: return unavailable features via query-cpu-definitions Viktor Mihajlovski
@ 2017-07-04 9:35 ` David Hildenbrand
2017-07-04 9:44 ` Christian Borntraeger
2017-07-04 11:22 ` Viktor Mihajlovski
2017-07-04 12:06 ` Christian Borntraeger
1 sibling, 2 replies; 6+ messages in thread
From: David Hildenbrand @ 2017-07-04 9:35 UTC (permalink / raw)
To: Viktor Mihajlovski, qemu-devel; +Cc: borntraeger, jjherne
> +
> 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));
> @@ -300,7 +331,19 @@ 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));
> -
Wonder if we should add the following:
/* the host model can never be used under TCG */
if (scc->kvm_required && !kvm_enabled())) {
info->has_unavailable_features = true;
list_add_feat("type", &info->unavailable_features);
} else if (cpu_list_data->model ...
> + /* check for unavailable features */
> + if (cpu_list_data->model) {
> + Object *obj;
> + S390CPU *sc;
> + obj = object_new(object_class_get_name(klass));
> + sc = S390_CPU(obj);
So we can drop the check here (if we have a host/max_model definition
under KVM, then all models have sc->model set). But the current code
should also work (it simply will not indicate runability information for
the "host" model under TCG, as sc->model will be NULL).
> + if (sc->model) {
> + info->has_unavailable_features = true;
> + check_unavailable_features(cpu_list_data->model, sc->model,
> + &info->unavailable_features);
> + }
> + object_unref(obj);
> + }
>
Looks good to me and produced the expected result under TCG.
Feel free to add my
Reviewed-by: David Hildenbrand <david@redhat.com>
with or without the modification.
--
Thanks,
David
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] s390: return unavailable features via query-cpu-definitions
2017-07-04 9:35 ` David Hildenbrand
@ 2017-07-04 9:44 ` Christian Borntraeger
2017-07-04 11:22 ` Viktor Mihajlovski
1 sibling, 0 replies; 6+ messages in thread
From: Christian Borntraeger @ 2017-07-04 9:44 UTC (permalink / raw)
To: David Hildenbrand, Viktor Mihajlovski, qemu-devel; +Cc: jjherne
On 07/04/2017 11:35 AM, David Hildenbrand wrote:
>> +
>> 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));
>> @@ -300,7 +331,19 @@ 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));
>> -
>
> Wonder if we should add the following:
>
> /* the host model can never be used under TCG */
> if (scc->kvm_required && !kvm_enabled())) {
> info->has_unavailable_features = true;
> list_add_feat("type", &info->unavailable_features);
> } else if (cpu_list_data->model ...
>
>> + /* check for unavailable features */
>> + if (cpu_list_data->model) {
>> + Object *obj;
>> + S390CPU *sc;
>> + obj = object_new(object_class_get_name(klass));
>> + sc = S390_CPU(obj);
>
> So we can drop the check here (if we have a host/max_model definition
> under KVM, then all models have sc->model set). But the current code
> should also work (it simply will not indicate runability information for
> the "host" model under TCG, as sc->model will be NULL).
>
>> + if (sc->model) {
>> + info->has_unavailable_features = true;
>> + check_unavailable_features(cpu_list_data->model, sc->model,
>> + &info->unavailable_features);
>> + }
>> + object_unref(obj);
>> + }
>>
> Looks good to me and produced the expected result under TCG.
>
> Feel free to add my
>
> Reviewed-by: David Hildenbrand <david@redhat.com>
Thanks. Viktor I can pick this version or wait for a v4. What do you prefer?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] s390: return unavailable features via query-cpu-definitions
2017-07-04 9:35 ` David Hildenbrand
2017-07-04 9:44 ` Christian Borntraeger
@ 2017-07-04 11:22 ` Viktor Mihajlovski
2017-07-04 12:03 ` Viktor Mihajlovski
1 sibling, 1 reply; 6+ messages in thread
From: Viktor Mihajlovski @ 2017-07-04 11:22 UTC (permalink / raw)
To: David Hildenbrand, qemu-devel; +Cc: borntraeger, jjherne
On 04.07.2017 11:35, David Hildenbrand wrote:
>> +
>> 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));
>> @@ -300,7 +331,19 @@ 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));
>> -
>
> Wonder if we should add the following:
>
> /* the host model can never be used under TCG */
> if (scc->kvm_required && !kvm_enabled())) {
> info->has_unavailable_features = true;
> list_add_feat("type", &info->unavailable_features);
> } else if (cpu_list_data->model ...
>
>> + /* check for unavailable features */
>> + if (cpu_list_data->model) {
>> + Object *obj;
>> + S390CPU *sc;
>> + obj = object_new(object_class_get_name(klass));
>> + sc = S390_CPU(obj);
>
> So we can drop the check here (if we have a host/max_model definition
> under KVM, then all models have sc->model set). But the current code
> should also work (it simply will not indicate runability information for
> the "host" model under TCG, as sc->model will be NULL).
Hmm ... that made me think that we could remove the check here and make
check_unavailable_features interpret a NULL model as not
usable/incompatible (a bit more robust in the unlikely event of a memory
allocation failure). Thoughts?
> >> + if (sc->model) {
>> + info->has_unavailable_features = true;
>> + check_unavailable_features(cpu_list_data->model, sc->model,
>> + &info->unavailable_features);
>> + }
>> + object_unref(obj);
>> + }
>>
> Looks good to me and produced the expected result under TCG.
>
> Feel free to add my
>
> Reviewed-by: David Hildenbrand <david@redhat.com>
>
> with or without the modification.
>
--
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] s390: return unavailable features via query-cpu-definitions
2017-07-04 11:22 ` Viktor Mihajlovski
@ 2017-07-04 12:03 ` Viktor Mihajlovski
0 siblings, 0 replies; 6+ messages in thread
From: Viktor Mihajlovski @ 2017-07-04 12:03 UTC (permalink / raw)
To: David Hildenbrand, qemu-devel; +Cc: borntraeger, jjherne
On 04.07.2017 13:22, Viktor Mihajlovski wrote:
> On 04.07.2017 11:35, David Hildenbrand wrote:
>>> +
>>> 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));
>>> @@ -300,7 +331,19 @@ 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));
>>> -
>>
>> Wonder if we should add the following:
>>
>> /* the host model can never be used under TCG */
>> if (scc->kvm_required && !kvm_enabled())) {
>> info->has_unavailable_features = true;
>> list_add_feat("type", &info->unavailable_features);
>> } else if (cpu_list_data->model ...
>>
>>> + /* check for unavailable features */
>>> + if (cpu_list_data->model) {
>>> + Object *obj;
>>> + S390CPU *sc;
>>> + obj = object_new(object_class_get_name(klass));
>>> + sc = S390_CPU(obj);
>>
>> So we can drop the check here (if we have a host/max_model definition
>> under KVM, then all models have sc->model set). But the current code
>> should also work (it simply will not indicate runability information for
>> the "host" model under TCG, as sc->model will be NULL).
> Hmm ... that made me think that we could remove the check here and make
> check_unavailable_features interpret a NULL model as not
> usable/incompatible (a bit more robust in the unlikely event of a memory
> allocation failure). Thoughts?
In fact, all of this may be moot anyway: libvirt blacklists the host
model as I found out, after wondering why there was no "host" CPU with
unknown usability in the virsh domcapabilities output.
I'll take your r-b then, and abstain from posting my v4 :-).
>>>> + if (sc->model) {
>>> + info->has_unavailable_features = true;
>>> + check_unavailable_features(cpu_list_data->model, sc->model,
>>> + &info->unavailable_features);
>>> + }
>>> + object_unref(obj);
>>> + }
>>>
>> Looks good to me and produced the expected result under TCG.
>>
>> Feel free to add my
>>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>>
>> with or without the modification.
>>
>
>
--
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] s390: return unavailable features via query-cpu-definitions
2017-07-03 11:48 [Qemu-devel] [PATCH v3] s390: return unavailable features via query-cpu-definitions Viktor Mihajlovski
2017-07-04 9:35 ` David Hildenbrand
@ 2017-07-04 12:06 ` Christian Borntraeger
1 sibling, 0 replies; 6+ messages in thread
From: Christian Borntraeger @ 2017-07-04 12:06 UTC (permalink / raw)
To: Viktor Mihajlovski, qemu-devel; +Cc: jjherne, david
On 07/03/2017 01:48 PM, 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 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>
Thanks applied.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-07-04 12:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-03 11:48 [Qemu-devel] [PATCH v3] s390: return unavailable features via query-cpu-definitions Viktor Mihajlovski
2017-07-04 9:35 ` David Hildenbrand
2017-07-04 9:44 ` Christian Borntraeger
2017-07-04 11:22 ` Viktor Mihajlovski
2017-07-04 12:03 ` Viktor Mihajlovski
2017-07-04 12:06 ` Christian Borntraeger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).