* [PATCH v2 1/3] hw/s390x/s390-skeys: Don't call register_savevm_live() during instance_init()
2023-10-20 15:05 [PATCH v2 0/3] hw/s390x: Don't call register_savevm_live() during instance_init() Thomas Huth
@ 2023-10-20 15:05 ` Thomas Huth
2023-10-20 15:05 ` [PATCH v2 2/3] hw/s390x/s390-stattrib: Simplify handling of the "migration-enabled" property Thomas Huth
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Thomas Huth @ 2023-10-20 15:05 UTC (permalink / raw)
To: qemu-devel, Christian Borntraeger
Cc: qemu-s390x, Halil Pasic, Eric Farman, David Hildenbrand,
Claudio Imbrenda, Juan Quintela
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>
Signed-off-by: Thomas Huth <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),
+};
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),
--
2.41.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v2 2/3] hw/s390x/s390-stattrib: Simplify handling of the "migration-enabled" property
2023-10-20 15:05 [PATCH v2 0/3] hw/s390x: Don't call register_savevm_live() during instance_init() Thomas Huth
2023-10-20 15:05 ` [PATCH v2 1/3] hw/s390x/s390-skeys: " Thomas Huth
@ 2023-10-20 15:05 ` Thomas Huth
2023-10-20 15:05 ` [PATCH v2 3/3] hw/s390x/s390-stattrib: Don't call register_savevm_live() during instance_init() Thomas Huth
2023-10-23 11:32 ` [PATCH v2 0/3] hw/s390x: " Juan Quintela
3 siblings, 0 replies; 6+ messages in thread
From: Thomas Huth @ 2023-10-20 15:05 UTC (permalink / raw)
To: qemu-devel, Christian Borntraeger
Cc: qemu-s390x, Halil Pasic, Eric Farman, David Hildenbrand,
Claudio Imbrenda, Juan Quintela
There's no need for dedicated handlers here if they don't do anything
special.
Acked-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Eric Farman <farman@linux.ibm.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
hw/s390x/s390-stattrib.c | 26 ++++++--------------------
1 file changed, 6 insertions(+), 20 deletions(-)
diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
index 220e845d12..52f9fc036e 100644
--- a/hw/s390x/s390-stattrib.c
+++ b/hw/s390x/s390-stattrib.c
@@ -13,6 +13,7 @@
#include "qemu/units.h"
#include "migration/qemu-file.h"
#include "migration/register.h"
+#include "hw/qdev-properties.h"
#include "hw/s390x/storage-attributes.h"
#include "qemu/error-report.h"
#include "exec/ram_addr.h"
@@ -340,6 +341,10 @@ static void s390_stattrib_realize(DeviceState *dev, Error **errp)
}
}
+static Property s390_stattrib_props[] = {
+ DEFINE_PROP_BOOL("migration-enabled", S390StAttribState, migration_enabled, true),
+};
+
static void s390_stattrib_class_init(ObjectClass *oc, void *data)
{
DeviceClass *dc = DEVICE_CLASS(oc);
@@ -347,22 +352,7 @@ static void s390_stattrib_class_init(ObjectClass *oc, void *data)
dc->hotpluggable = false;
set_bit(DEVICE_CATEGORY_MISC, dc->categories);
dc->realize = s390_stattrib_realize;
-}
-
-static inline bool s390_stattrib_get_migration_enabled(Object *obj,
- Error **errp)
-{
- S390StAttribState *s = S390_STATTRIB(obj);
-
- return s->migration_enabled;
-}
-
-static inline void s390_stattrib_set_migration_enabled(Object *obj, bool value,
- Error **errp)
-{
- S390StAttribState *s = S390_STATTRIB(obj);
-
- s->migration_enabled = value;
+ device_class_set_props(dc, s390_stattrib_props);
}
static SaveVMHandlers savevm_s390_stattrib_handlers = {
@@ -383,10 +373,6 @@ static void s390_stattrib_instance_init(Object *obj)
register_savevm_live(TYPE_S390_STATTRIB, 0, 0,
&savevm_s390_stattrib_handlers, sas);
- object_property_add_bool(obj, "migration-enabled",
- s390_stattrib_get_migration_enabled,
- s390_stattrib_set_migration_enabled);
- object_property_set_bool(obj, "migration-enabled", true, NULL);
sas->migration_cur_gfn = 0;
}
--
2.41.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v2 3/3] hw/s390x/s390-stattrib: Don't call register_savevm_live() during instance_init()
2023-10-20 15:05 [PATCH v2 0/3] hw/s390x: Don't call register_savevm_live() during instance_init() Thomas Huth
2023-10-20 15:05 ` [PATCH v2 1/3] hw/s390x/s390-skeys: " Thomas Huth
2023-10-20 15:05 ` [PATCH v2 2/3] hw/s390x/s390-stattrib: Simplify handling of the "migration-enabled" property Thomas Huth
@ 2023-10-20 15:05 ` Thomas Huth
2023-10-23 11:32 ` [PATCH v2 0/3] hw/s390x: " Juan Quintela
3 siblings, 0 replies; 6+ messages in thread
From: Thomas Huth @ 2023-10-20 15:05 UTC (permalink / raw)
To: qemu-devel, Christian Borntraeger
Cc: qemu-s390x, Halil Pasic, Eric Farman, David Hildenbrand,
Claudio Imbrenda, Juan Quintela
We must not call register_savevm_live() from an instance_init() function
(since this could be called multiple times during device introspection).
Move this to the realize() function instead.
Acked-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Eric Farman <farman@linux.ibm.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
hw/s390x/s390-stattrib.c | 29 +++++++++++++++--------------
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
index 52f9fc036e..54a138011c 100644
--- a/hw/s390x/s390-stattrib.c
+++ b/hw/s390x/s390-stattrib.c
@@ -331,6 +331,17 @@ static const TypeInfo qemu_s390_stattrib_info = {
/* Generic abstract object: */
+static SaveVMHandlers savevm_s390_stattrib_handlers = {
+ .save_setup = cmma_save_setup,
+ .save_live_iterate = cmma_save_iterate,
+ .save_live_complete_precopy = cmma_save_complete,
+ .state_pending_exact = cmma_state_pending,
+ .state_pending_estimate = cmma_state_pending,
+ .save_cleanup = cmma_save_cleanup,
+ .load_state = cmma_load,
+ .is_active = cmma_active,
+};
+
static void s390_stattrib_realize(DeviceState *dev, Error **errp)
{
bool ambiguous = false;
@@ -338,7 +349,11 @@ static void s390_stattrib_realize(DeviceState *dev, Error **errp)
object_resolve_path_type("", TYPE_S390_STATTRIB, &ambiguous);
if (ambiguous) {
error_setg(errp, "storage_attributes device already exists");
+ return;
}
+
+ register_savevm_live(TYPE_S390_STATTRIB, 0, 0,
+ &savevm_s390_stattrib_handlers, dev);
}
static Property s390_stattrib_props[] = {
@@ -355,24 +370,10 @@ static void s390_stattrib_class_init(ObjectClass *oc, void *data)
device_class_set_props(dc, s390_stattrib_props);
}
-static SaveVMHandlers savevm_s390_stattrib_handlers = {
- .save_setup = cmma_save_setup,
- .save_live_iterate = cmma_save_iterate,
- .save_live_complete_precopy = cmma_save_complete,
- .state_pending_exact = cmma_state_pending,
- .state_pending_estimate = cmma_state_pending,
- .save_cleanup = cmma_save_cleanup,
- .load_state = cmma_load,
- .is_active = cmma_active,
-};
-
static void s390_stattrib_instance_init(Object *obj)
{
S390StAttribState *sas = S390_STATTRIB(obj);
- register_savevm_live(TYPE_S390_STATTRIB, 0, 0,
- &savevm_s390_stattrib_handlers, sas);
-
sas->migration_cur_gfn = 0;
}
--
2.41.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2 0/3] hw/s390x: Don't call register_savevm_live() during instance_init()
2023-10-20 15:05 [PATCH v2 0/3] hw/s390x: Don't call register_savevm_live() during instance_init() Thomas Huth
` (2 preceding siblings ...)
2023-10-20 15:05 ` [PATCH v2 3/3] hw/s390x/s390-stattrib: Don't call register_savevm_live() during instance_init() Thomas Huth
@ 2023-10-23 11:32 ` Juan Quintela
2023-10-23 11:35 ` Thomas Huth
3 siblings, 1 reply; 6+ messages in thread
From: Juan Quintela @ 2023-10-23 11:32 UTC (permalink / raw)
To: Thomas Huth
Cc: qemu-devel, Christian Borntraeger, qemu-s390x, Halil Pasic,
Eric Farman, David Hildenbrand, Claudio Imbrenda
Thomas Huth <thuth@redhat.com> wrote:
> We must not call register_savevm_live() during instance_init()
> since instances can be created at any time, e.g. during introspection
> of a device. We must register the savevm handler during realize()
> instead. Fix it now in the s390x devices.
>
> v2:
> - Fixed bug in the third patch ("sas" -> "dev")
> - Add an early return in case of error in the third patch
> - Added Acked-by/Reviewed by from David and Eric (thanks!)
>
> Thomas Huth (3):
> hw/s390x/s390-skeys: Don't call register_savevm_live() during
> instance_init()
> hw/s390x/s390-stattrib: Simplify handling of the "migration-enabled"
> property
> hw/s390x/s390-stattrib: Don't call register_savevm_live() during
> instance_init()
>
> hw/s390x/s390-skeys.c | 35 ++++++-------------------
> hw/s390x/s390-stattrib.c | 55 +++++++++++++++-------------------------
> 2 files changed, 29 insertions(+), 61 deletions(-)
Acked-by: Juan Quintela <quintela@redhat.com>
It makes things work for my test, Thanks.
Should I get those through the migration tree?
Later, Juan.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2 0/3] hw/s390x: Don't call register_savevm_live() during instance_init()
2023-10-23 11:32 ` [PATCH v2 0/3] hw/s390x: " Juan Quintela
@ 2023-10-23 11:35 ` Thomas Huth
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Huth @ 2023-10-23 11:35 UTC (permalink / raw)
To: quintela
Cc: qemu-devel, Christian Borntraeger, qemu-s390x, Halil Pasic,
Eric Farman, David Hildenbrand, Claudio Imbrenda
On 23/10/2023 13.32, Juan Quintela wrote:
> Thomas Huth <thuth@redhat.com> wrote:
>> We must not call register_savevm_live() during instance_init()
>> since instances can be created at any time, e.g. during introspection
>> of a device. We must register the savevm handler during realize()
>> instead. Fix it now in the s390x devices.
>>
>> v2:
>> - Fixed bug in the third patch ("sas" -> "dev")
>> - Add an early return in case of error in the third patch
>> - Added Acked-by/Reviewed by from David and Eric (thanks!)
>>
>> Thomas Huth (3):
>> hw/s390x/s390-skeys: Don't call register_savevm_live() during
>> instance_init()
>> hw/s390x/s390-stattrib: Simplify handling of the "migration-enabled"
>> property
>> hw/s390x/s390-stattrib: Don't call register_savevm_live() during
>> instance_init()
>>
>> hw/s390x/s390-skeys.c | 35 ++++++-------------------
>> hw/s390x/s390-stattrib.c | 55 +++++++++++++++-------------------------
>> 2 files changed, 29 insertions(+), 61 deletions(-)
>
> Acked-by: Juan Quintela <quintela@redhat.com>
>
> It makes things work for my test, Thanks.
>
> Should I get those through the migration tree?
Sure, if you plan to do a pull request soon, feel free to take them!
(Otherwise I'll queue them for my next s390x pull request ... likely not
happening within the next two weeks, though)
Thomas
^ permalink raw reply [flat|nested] 6+ messages in thread