arm_gic_kvm: Disable live migration if not supported

PATCH Applied Pavel Fedin 2 participants 4 messages Oct 16, 2015 – Oct 19, 2015

Share & tools

Pavel Fedin

author

2015-10-16 13:43 UTC #0 lore raw

PATCH v2 1/1 arm_gic_kvm: Disable live migration if not supported

Currently, if the kernel does not have live migration API, the migration will still be attempted, but vGIC save/restore functions will just not do anything. This will result in a broken machine state.

This patch fixes the problem by adding migration blocker if kernel API is not supported.

Signed-off-by Pavel Fedin <[email protected]>

---
v1 => v2:
Do not hack VMState, use migrate_add_blocker() instead

---
 hw/intc/arm_gic_kvm.c            | 22 +++++++++++-----------
 include/hw/intc/arm_gic_common.h |  1 +
 2 files changed, 12 insertions(+), 11 deletions(-)

hw/intc/arm_gic_kvm.c +11 ~~-11~~

index e8b2386..0ceebbf 100644
@@ -20,6 +20,7 @@
  */
 
 #include "hw/sysbus.h"
+#include "migration/migration.h"
 #include "sysemu/kvm.h"
 #include "kvm_arm.h"
 #include "gic_internal.h"

@@ -307,11 +308,6 @@ static void kvm_arm_gic_put(GICState *s)
     int num_cpu;
     int num_irq;
 
-    if (!kvm_arm_gic_can_save_restore(s)) {
-            DPRINTF("Cannot put kernel gic state, no kernel interface");
-            return;
-    }
-
     /* Note: We do the restore in a slightly different order than the save
      * (where the order doesn't matter and is simply ordered according to the
      * register offset values */

@@ -411,11 +407,6 @@ static void kvm_arm_gic_get(GICState *s)
     int i;
     int cpu;
 
-    if (!kvm_arm_gic_can_save_restore(s)) {
-            DPRINTF("Cannot get kernel gic state, no kernel interface");
-            return;
-    }
-
     /*****************************************************************
      * Distributor State
      */

@@ -503,7 +494,10 @@ static void kvm_arm_gic_reset(DeviceState *dev)
     KVMARMGICClass *kgc = KVM_ARM_GIC_GET_CLASS(s);
 
     kgc->parent_reset(dev);
-    kvm_arm_gic_put(s);
+
+    if (kvm_arm_gic_can_save_restore(s)) {
+        kvm_arm_gic_put(s);
+    }
 }
 
 static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)

@@ -573,6 +567,12 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
                             KVM_DEV_ARM_VGIC_GRP_ADDR,
                             KVM_VGIC_V2_ADDR_TYPE_CPU,
                             s->dev_fd);
+
+    if (!kvm_arm_gic_can_save_restore(s)) {
+        error_setg(&s->migration_blocker, "This operating system kernel does "
+                                          "not support vGICv2 migration");
+        migrate_add_blocker(s->migration_blocker);
+    }
 }
 
 static void kvm_arm_gic_class_init(ObjectClass *klass, void *data)

include/hw/intc/arm_gic_common.h +1 -0

index 564a72b..f4c349a 100644
@@ -111,6 +111,7 @@ typedef struct GICState {
     bool security_extn;
     bool irq_reset_nonsecure; /* configure IRQs as group 1 (NS) on reset? */
     int dev_fd; /* kvm device fd if backed by kvm vgic support */
+    Error *migration_blocker;
 } GICState;
 
 #define TYPE_ARM_GIC_COMMON "arm_gic_common"

Signature · 1 line

1.9.5.msysgit.0

Peter Maydell

2015-10-16 13:46 UTC #1 lore raw

Quoted 53 lines · Pavel Fedin

This patch fixes the problem by adding migration blocker if kernel API is not supported.

Signed-off-by Pavel Fedin <[email protected]>

---
v1 => v2:
Do not hack VMState, use migrate_add_blocker() instead

---
 hw/intc/arm_gic_kvm.c            | 22 +++++++++++-----------
 include/hw/intc/arm_gic_common.h |  1 +
 2 files changed, 12 insertions(+), 11 deletions(-)

hw/intc/arm_gic_kvm.c +1 ~~-10~~

index e8b2386..0ceebbf 100644
@@ -20,6 +20,7 @@
  */
 
 #include "hw/sysbus.h"
+#include "migration/migration.h"
 #include "sysemu/kvm.h"
 #include "kvm_arm.h"
 #include "gic_internal.h"

@@ -307,11 +308,6 @@ static void kvm_arm_gic_put(GICState *s)
     int num_cpu;
     int num_irq;
 
-    if (!kvm_arm_gic_can_save_restore(s)) {
-            DPRINTF("Cannot put kernel gic state, no kernel interface");
-            return;
-    }
-
     /* Note: We do the restore in a slightly different order than the save
      * (where the order doesn't matter and is simply ordered according to the
      * register offset values */

@@ -411,11 +407,6 @@ static void kvm_arm_gic_get(GICState *s)
     int i;
     int cpu;
 
-    if (!kvm_arm_gic_can_save_restore(s)) {
-            DPRINTF("Cannot get kernel gic state, no kernel interface");
-            return;
-    }
-
     /*****************************************************************
      * Distributor State
      */

hw/intc/arm_gic_kvm.c static void kvm_arm_gic_reset(DeviceState *dev) · line 500

Quoted 8 lines

hw/intc/arm_gic_kvm.c +4 -1

@@ -503,7 +494,10 @@ static void kvm_arm_gic_reset(DeviceState *dev)
     KVMARMGICClass *kgc = KVM_ARM_GIC_GET_CLASS(s);
 
     kgc->parent_reset(dev);
-    kvm_arm_gic_put(s);
+
+    if (kvm_arm_gic_can_save_restore(s)) {
+        kvm_arm_gic_put(s);
+    }

Why change the reset method in a patch that's dealing with adding a warning about migration?

thanks
-- PMM

Pavel Fedin

author

2015-10-16 13:55 UTC #2 lore raw

Hello!

Quoted 6 lines

hw/intc/arm_gic_kvm.c static void kvm_arm_gic_reset(DeviceState *dev) · line 500
Quoted 3 lines
hw/intc/arm_gic_kvm.c +3 -0
+    if (kvm_arm_gic_can_save_restore(s)) {
+        kvm_arm_gic_put(s);
+    }
Why change the reset method in a patch that's dealing with adding a warning about migration?

Since pre_save and post_load will never be called if migration is disabled, i removed unnecessary checks from there. But, since reset method calls post_load, i had to add the check there.
Actually, i would factor away kvm_arm_gic_reset() completely, because all it does is calling parent's reset, then calling post_load. This can perfectly be done in parent class. For software-emulated GICv3 post_load callback is simply not used, so that would be safe. But, i remember that you don't like just refactoring, so i decided to stop at the point which you see. :)

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Peter Maydell

Applied

2015-10-19 17:18 UTC #3 lore raw

Quoted 6 lines · Pavel Fedin

Hello!
Quoted 4 lines
Quoted 3 lines
hw/intc/arm_gic_kvm.c +3 -0
+    if (kvm_arm_gic_can_save_restore(s)) {
+        kvm_arm_gic_put(s);
+    }

Quoted 6 lines

Quoted 2 lines
Why change the reset method in a patch that's dealing with adding a warning about migration?
Since pre_save and post_load will never be called if migration
is disabled, i removed unnecessary checks from there. But, since reset method calls post_load, i had to add the check there.

Ah yes, I see now. (Reset is broken on old kernels without the save/restore ABI, but not much we can do about that.)

Quoted 5 lines

Actually, i would factor away kvm_arm_gic_reset() completely,
because all it does is calling parent's reset, then calling post_load. This can perfectly be done in parent class. For software-emulated GICv3 post_load callback is simply not used, so that would be safe.

Calling post_load in the parent class is the wrong thing in the wrong place, because the need to write the data back into the kernel is an implementation detail of the KVM GIC subclass, and so is the fact that the post_load hook happens to do the write-data-back job. Another subclass might potentially do different things in its post-load hook; it's just coincidence that the emulated GIC happens not to need to do any post-load operations at the moment.

I've applied this patch to target-arm.next.

thanks
-- PMM