Re: [PATCH 12/24] s390x: Register all CPU properties as class properties

[David Hildenbrand]

On 22.09.20 00:10, Eduardo Habkost wrote:
> Class properties make QOM introspection simpler and easier, as
> they don't require an object to be instantiated.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Halil Pasic <pasic@linux.ibm.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: qemu-s390x@nongnu.org
> Cc: qemu-devel@nongnu.org
> ---
>  target/s390x/internal.h   |  1 -
>  target/s390x/cpu.c        |  1 -
>  target/s390x/cpu_models.c | 35 ++++++++++++++++-------------------
>  3 files changed, 16 insertions(+), 21 deletions(-)
> 
> diff --git a/target/s390x/internal.h b/target/s390x/internal.h
> index b1e0ebf67f8..bac0d3c67b2 100644
> --- a/target/s390x/internal.h
> +++ b/target/s390x/internal.h
> @@ -260,7 +260,6 @@ static inline void s390_cpu_unhalt(S390CPU *cpu)
>  
>  
>  /* cpu_models.c */
> -void s390_cpu_model_register_props(Object *obj);
>  void s390_cpu_model_class_register_props(ObjectClass *oc);
>  void s390_realize_cpu_model(CPUState *cs, Error **errp);
>  ObjectClass *s390_cpu_class_by_name(const char *name);
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 749cd548f0f..1dc45ca11b9 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -302,7 +302,6 @@ static void s390_cpu_initfn(Object *obj)
>          timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);
>      s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
>  #endif
> -    s390_cpu_model_register_props(obj);
>  }
>  
>  static void s390_cpu_finalize(Object *obj)
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index c2af2261743..b97e9596ab0 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -1089,25 +1089,6 @@ static void set_feature_group(Object *obj, Visitor *v, 
> const char *name,
>      }
>  }
>  
> -void s390_cpu_model_register_props(Object *obj)
> -{
> -    S390FeatGroup group;
> -    S390Feat feat;
> -
> -    for (feat = 0; feat < S390_FEAT_MAX; feat++) {
> -        const S390FeatDef *def = s390_feat_def(feat);
> -        object_property_add(obj, def->name, "bool", get_feature,
> -                            set_feature, NULL, (void *) feat);
> -        object_property_set_description(obj, def->name, def->desc);
> -    }
> -    for (group = 0; group < S390_FEAT_GROUP_MAX; group++) {
> -        const S390FeatGroupDef *def = s390_feat_group_def(group);
> -        object_property_add(obj, def->name, "bool", get_feature_group,
> -                            set_feature_group, NULL, (void *) group);
> -        object_property_set_description(obj, def->name, def->desc);
> -    }
> -}
> -
>  static void s390_cpu_model_initfn(Object *obj)
>  {
>      S390CPU *cpu = S390_CPU(obj);
> @@ -1215,11 +1196,27 @@ static char *get_description(Object *obj, Error 
> **errp)
>  
>  void s390_cpu_model_class_register_props(ObjectClass *oc)
>  {
> +    S390FeatGroup group;
> +    S390Feat feat;
> +
>      object_class_property_add_bool(oc, "migration-safe", 
> get_is_migration_safe,
>                                     NULL);
>      object_class_property_add_bool(oc, "static", get_is_static,
>                                     NULL);
>      object_class_property_add_str(oc, "description", get_description, NULL);
> +
> +    for (feat = 0; feat < S390_FEAT_MAX; feat++) {
> +        const S390FeatDef *def = s390_feat_def(feat);
> +        object_class_property_add(oc, def->name, "bool", get_feature,
> +                                  set_feature, NULL, (void *) feat);
> +        object_class_property_set_description(oc, def->name, def->desc);
> +    }
> +    for (group = 0; group < S390_FEAT_GROUP_MAX; group++) {
> +        const S390FeatGroupDef *def = s390_feat_group_def(group);
> +        object_class_property_add(oc, def->name, "bool", get_feature_group,
> +                                  set_feature_group, NULL, (void *) group);
> +        object_class_property_set_description(oc, def->name, def->desc);
> +    }

It would be even nicer to only register what's actually valid for a CPU
generation - but it's tricky for host/max, where that is determined
later at runtime.

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb