| Message ID | 015801d10818$95084700$bf18d500$@samsung.com |
|---|---|
| State | New |
| Headers | show |
Hello! > > + 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
On 16 October 2015 at 14:55, Pavel Fedin <p.fedin@samsung.com> wrote: > Hello! > >> > + 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. Ah yes, I see now. (Reset is broken on old kernels without the save/restore ABI, but not much we can do about that.) > 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
diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c index e8b2386..0ceebbf 100644 --- a/hw/intc/arm_gic_kvm.c +++ b/hw/intc/arm_gic_kvm.c @@ -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) diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h index 564a72b..f4c349a 100644 --- a/include/hw/intc/arm_gic_common.h +++ b/include/hw/intc/arm_gic_common.h @@ -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"
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 <p.fedin@samsung.com> --- 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(-)