Re: [PULL 19/39] hw/s390x/s390-skeys: Don't call register_savevm_live() during instance_init()

[Thomas Huth <thuth@redhat.com>]

On 24/10/2023 15.12, Juan Quintela wrote:
> From: Thomas Huth <thuth@redhat.com>
> 
> Since the instance_init() function immediately tries to set the
> property to "true", the s390_skeys_set_migration_enabled() tries
> to register a savevm handler during instance_init(). However,
> instance_init() functions can be called multiple times, e.g. for
> introspection of devices. That means multiple instances of devices
> can be created during runtime (which is fine as long as they all
> don't get realized, too), so the "Prevent double registration of
> savevm handler" check in the s390_skeys_set_migration_enabled()
> function does not work at all as expected (since there could be
> more than one instance).
> 
> Thus we must not call register_savevm_live() from an instance_init()
> function at all. Move this to the realize() function instead. This
> way we can also get rid of the property getter and setter functions
> completely, simplifying the code along the way quite a bit.
> 
> Acked-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Eric Farman <farman@linux.ibm.com>
> Acked-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Message-ID: <20231020150554.664422-2-thuth@redhat.com>
> ---
>   hw/s390x/s390-skeys.c | 35 ++++++++---------------------------
>   1 file changed, 8 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
> index 5024faf411..8e9d9e41e8 100644
> --- a/hw/s390x/s390-skeys.c
> +++ b/hw/s390x/s390-skeys.c
> @@ -12,6 +12,7 @@
>   #include "qemu/osdep.h"
>   #include "qemu/units.h"
>   #include "hw/boards.h"
> +#include "hw/qdev-properties.h"
>   #include "hw/s390x/storage-keys.h"
>   #include "qapi/error.h"
>   #include "qapi/qapi-commands-misc-target.h"
> @@ -432,58 +433,38 @@ static int s390_storage_keys_load(QEMUFile *f, void *opaque, int version_id)
>       return ret;
>   }
>   
> -static inline bool s390_skeys_get_migration_enabled(Object *obj, Error **errp)
> -{
> -    S390SKeysState *ss = S390_SKEYS(obj);
> -
> -    return ss->migration_enabled;
> -}
> -
>   static SaveVMHandlers savevm_s390_storage_keys = {
>       .save_state = s390_storage_keys_save,
>       .load_state = s390_storage_keys_load,
>   };
>   
> -static inline void s390_skeys_set_migration_enabled(Object *obj, bool value,
> -                                            Error **errp)
> +static void s390_skeys_realize(DeviceState *dev, Error **errp)
>   {
> -    S390SKeysState *ss = S390_SKEYS(obj);
> -
> -    /* Prevent double registration of savevm handler */
> -    if (ss->migration_enabled == value) {
> -        return;
> -    }
> -
> -    ss->migration_enabled = value;
> +    S390SKeysState *ss = S390_SKEYS(dev);
>   
>       if (ss->migration_enabled) {
>           register_savevm_live(TYPE_S390_SKEYS, 0, 1,
>                                &savevm_s390_storage_keys, ss);
> -    } else {
> -        unregister_savevm(VMSTATE_IF(ss), TYPE_S390_SKEYS, ss);
>       }
>   }
>   
> -static void s390_skeys_instance_init(Object *obj)
> -{
> -    object_property_add_bool(obj, "migration-enabled",
> -                             s390_skeys_get_migration_enabled,
> -                             s390_skeys_set_migration_enabled);
> -    object_property_set_bool(obj, "migration-enabled", true, NULL);
> -}
> +static Property s390_skeys_props[] = {
> +    DEFINE_PROP_BOOL("migration-enabled", S390SKeysState, migration_enabled, true),

This needs a  DEFINE_PROP_END_OF_LIST() here ... mea culpa!

  Thomas


> +};
>   
>   static void s390_skeys_class_init(ObjectClass *oc, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(oc);
>   
>       dc->hotpluggable = false;
> +    dc->realize = s390_skeys_realize;
> +    device_class_set_props(dc, s390_skeys_props);
>       set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>   }
>   
>   static const TypeInfo s390_skeys_info = {
>       .name          = TYPE_S390_SKEYS,
>       .parent        = TYPE_DEVICE,
> -    .instance_init = s390_skeys_instance_init,
>       .instance_size = sizeof(S390SKeysState),
>       .class_init    = s390_skeys_class_init,
>       .class_size    = sizeof(S390SKeysClass),