[Qemu-devel] [RFC PATCH v3 07/49] kvmapic: fixing loading vmstate

[Qemu-devel] [RFC PATCH v3 07/49] kvmapic: fixing loading vmstate

[Pavel Dovgalyuk]

vapic state should not be synchronized with APIC while loading,
because APIC state could be not loaded yet at that moment.
We just save vapic_paddr in APIC VMState instead of synchronization.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 hw/i386/kvmvapic.c              |   22 +++++++++++++++
 hw/intc/apic_common.c           |   56 ++++++++++++++++++++++++++++++++++++++-
 include/hw/i386/apic_internal.h |    2 +
 3 files changed, 77 insertions(+), 3 deletions(-)

diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
index cb855c7..417ab6a 100644
--- a/hw/i386/kvmvapic.c
+++ b/hw/i386/kvmvapic.c
@@ -351,6 +351,24 @@ static int get_kpcr_number(X86CPU *cpu)
     return kpcr.number;
 }
 
+static int vapic_enable_post_load(VAPICROMState *s, X86CPU *cpu)
+{
+    int cpu_number = get_kpcr_number(cpu);
+    hwaddr vapic_paddr;
+    static const uint8_t enabled = 1;
+
+    if (cpu_number < 0) {
+        return -1;
+    }
+    vapic_paddr = s->vapic_paddr +
+        (((hwaddr)cpu_number) << VAPIC_CPU_SHIFT);
+    cpu_physical_memory_rw(vapic_paddr + offsetof(VAPICState, enabled),
+                           (void *)&enabled, sizeof(enabled), 1);
+    s->state = VAPIC_ACTIVE;
+
+    return 0;
+}
+
 static int vapic_enable(VAPICROMState *s, X86CPU *cpu)
 {
     int cpu_number = get_kpcr_number(cpu);
@@ -731,7 +749,9 @@ static void do_vapic_enable(void *data)
     VAPICROMState *s = data;
     X86CPU *cpu = X86_CPU(first_cpu);
 
-    vapic_enable(s, cpu);
+    /* Do not synchronize with APIC, because it was not loaded yet.
+       Just call the enable function which does not have synchronization. */
+    vapic_enable_post_load(s, cpu);
 }
 
 static int vapic_post_load(void *opaque, int version_id)
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index ce3d903..5afd5b2 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -324,6 +324,15 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
 
 }
 
+static int apic_pre_load(void *opaque)
+{
+    APICCommonState *s = APIC_COMMON(opaque);
+    s->sipi_vector = 0;
+    s->wait_for_sipi = 0;
+    s->vapic_paddr = 0;
+    return 0;
+}
+
 static void apic_dispatch_pre_save(void *opaque)
 {
     APICCommonState *s = APIC_COMMON(opaque);
@@ -345,12 +354,46 @@ static int apic_dispatch_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static bool apic_common_sipi_needed(void *opaque)
+{
+    APICCommonState *s = APIC_COMMON(opaque);
+    return s->wait_for_sipi != 0;
+}
+
+static const VMStateDescription vmstate_apic_common_sipi = {
+    .name = "apic_sipi",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT32(sipi_vector, APICCommonState),
+        VMSTATE_INT32(wait_for_sipi, APICCommonState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static bool apic_common_vapic_paddr_needed(void *opaque)
+{
+    APICCommonState *s = APIC_COMMON(opaque);
+    return s->vapic_paddr != 0;
+}
+
+static const VMStateDescription vmstate_apic_common_vapic_paddr = {
+    .name = "apic_vapic_paddr",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(vapic_paddr, APICCommonState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_apic_common = {
     .name = "apic",
-    .version_id = 3,
+    .version_id = 4,
     .minimum_version_id = 3,
     .minimum_version_id_old = 1,
     .load_state_old = apic_load_old,
+    .pre_load = apic_pre_load,
     .pre_save = apic_dispatch_pre_save,
     .post_load = apic_dispatch_post_load,
     .fields = (VMStateField[]) {
@@ -375,6 +418,17 @@ static const VMStateDescription vmstate_apic_common = {
         VMSTATE_INT64(timer_expiry,
                       APICCommonState), /* open-coded timer state */
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (VMStateSubsection[]) {
+        {
+            .vmsd = &vmstate_apic_common_sipi,
+            .needed = apic_common_sipi_needed,
+        },
+        {
+            .vmsd = &vmstate_apic_common_vapic_paddr,
+            .needed = apic_common_vapic_paddr_needed,
+        },
+        VMSTATE_END_OF_LIST()
     }
 };
 
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 83e2a42..df4381c 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -124,7 +124,7 @@ struct APICCommonState {
 
     uint32_t vapic_control;
     DeviceState *vapic;
-    hwaddr vapic_paddr; /* note: persistence via kvmvapic */
+    hwaddr vapic_paddr;
 };
 
 typedef struct VAPICState {

Re: [Qemu-devel] [RFC PATCH v3 07/49] kvmapic: fixing loading vmstate

[Paolo Bonzini]

Il 31/07/2014 14:54, Pavel Dovgalyuk ha scritto:
> +static int apic_pre_load(void *opaque)
> +{
> +    APICCommonState *s = APIC_COMMON(opaque);
> +    s->sipi_vector = 0;
> +    s->wait_for_sipi = 0;
> +    s->vapic_paddr = 0;
> +    return 0;

Is this necessary?  Or does reset already do that?

> +}
> +
>  static void apic_dispatch_pre_save(void *opaque)
>  {
>      APICCommonState *s = APIC_COMMON(opaque);
> @@ -345,12 +354,46 @@ static int apic_dispatch_post_load(void *opaque, int version_id)
>      return 0;
>  }
>  
> +static bool apic_common_sipi_needed(void *opaque)
> +{
> +    APICCommonState *s = APIC_COMMON(opaque);
> +    return s->wait_for_sipi != 0;
> +}
> +
> +static const VMStateDescription vmstate_apic_common_sipi = {
> +    .name = "apic_sipi",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_INT32(sipi_vector, APICCommonState),
> +        VMSTATE_INT32(wait_for_sipi, APICCommonState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static bool apic_common_vapic_paddr_needed(void *opaque)
> +{
> +    APICCommonState *s = APIC_COMMON(opaque);
> +    return s->vapic_paddr != 0;
> +}
> +
> +static const VMStateDescription vmstate_apic_common_vapic_paddr = {
> +    .name = "apic_vapic_paddr",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT64(vapic_paddr, APICCommonState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_apic_common = {
>      .name = "apic",
> -    .version_id = 3,
> +    .version_id = 4,

You know what I'm going to say. :)

Paolo

Re: [Qemu-devel] [RFC PATCH v3 07/49] kvmapic: fixing loading vmstate

[Pavel Dovgalyuk]

[-- Attachment #1: Type: text/plain, Size: 2496 bytes --]

Pre load is necessary, because we switched off resetting VM while loading in the replay mode. Calling reset handlers generates irqs, that make loading process non-deterministic.


Sent from mobile device



-------- Исходное сообщение --------
От: Paolo Bonzini <pbonzini@redhat.com> 
Дата:31.07.2014  17:01  (GMT+04:00) 
Кому: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>,qemu-devel@nongnu.org 
Копия: peter.maydell@linaro.org,peter.crosthwaite@xilinx.com,mark.burton@greensocs.com,real@ispras.ru,batuzovk@ispras.ru,maria.klimushenkova@ispras.ru,eblake@redhat.com,afaerber@suse.de,fred.konrad@greensocs.com 
Тема: Re: [RFC PATCH v3 07/49] kvmapic: fixing loading vmstate 

Il 31/07/2014 14:54, Pavel Dovgalyuk ha scritto:
> +static int apic_pre_load(void *opaque)
> +{
> +    APICCommonState *s = APIC_COMMON(opaque);
> +    s->sipi_vector = 0;
> +    s->wait_for_sipi = 0;
> +    s->vapic_paddr = 0;
> +    return 0;

Is this necessary?  Or does reset already do that?

> +}
> +
>  static void apic_dispatch_pre_save(void *opaque)
>  {
>      APICCommonState *s = APIC_COMMON(opaque);
> @@ -345,12 +354,46 @@ static int apic_dispatch_post_load(void *opaque, int version_id)
>      return 0;
>  }
>  
> +static bool apic_common_sipi_needed(void *opaque)
> +{
> +    APICCommonState *s = APIC_COMMON(opaque);
> +    return s->wait_for_sipi != 0;
> +}
> +
> +static const VMStateDescription vmstate_apic_common_sipi = {
> +    .name = "apic_sipi",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_INT32(sipi_vector, APICCommonState),
> +        VMSTATE_INT32(wait_for_sipi, APICCommonState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static bool apic_common_vapic_paddr_needed(void *opaque)
> +{
> +    APICCommonState *s = APIC_COMMON(opaque);
> +    return s->vapic_paddr != 0;
> +}
> +
> +static const VMStateDescription vmstate_apic_common_vapic_paddr = {
> +    .name = "apic_vapic_paddr",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT64(vapic_paddr, APICCommonState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_apic_common = {
>      .name = "apic",
> -    .version_id = 3,
> +    .version_id = 4,

You know what I'm going to say. :)

Paolo

[-- Attachment #2: Type: text/html, Size: 3518 bytes --]

Re: [Qemu-devel] [RFC PATCH v3 07/49] kvmapic: fixing loading vmstate

[Paolo Bonzini]

Il 31/07/2014 17:21, Pavel Dovgalyuk ha scritto:
> Pre load is necessary, because we switched off resetting VM while
> loading in the replay mode.

Then you should not add it now, but rather when you add replay.  Treat
this part of the series as merely fixing migration bugs.  In fact, I
suggest that you start by progressively refining these patches.  You can
also post the other patches that I mentioned were good to go in my
review of v1.  Then pass to the next steps (in the meanwhile, Fred's
icount reverse-exec patches might get merged too).

Related to this: do _not_ assume that people review the entire series.
It's normal to notice that the initial part should stand on its own
legs, and then review it as if the remaining patches do not exist!  It's
up to _you_, in the commit messages, to annotate things that you do now
for the sake of future patches.

> Calling reset handlers generates irqs, that
> make loading process non-deterministic.

What irqs are these, and why do they make the loading process
non-deterministic?  Is it important that they not be generated, as long
as the final state is deterministic?

Have you audited all subsections and add a pre-load function there?
Where do you do this in the series?  These non-mechanical, sweeping
changes suggest to me that you find a way to keep resets during load.
Compare for example the patches at

   http://lists.gnu.org/archive/html/qemu-devel/2013-09/msg00477.html

with the series at

   http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg02652.html
   http://lists.gnu.org/archive/html/qemu-devel/2014-07/msg03934.html

The former adds 230 lines of code and adds code to 40-odd files.

The latter includes a preparatory part that is complicated but only
touches 4 files, handling of the odd cases that touches about 10 files,
and the final sweeping change that is mechanical and hardly requires
review.  Overall it adds ~20 lines of code, and adds actual
functionality in addition to fixing the same bug as the first!

Paolo

Re: [Qemu-devel] [RFC PATCH v3 07/49] kvmapic: fixing loading vmstate

[Pavel Dovgaluk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> Il 31/07/2014 17:21, Pavel Dovgalyuk ha scritto:
> > Pre load is necessary, because we switched off resetting VM while
> > loading in the replay mode.
> 
> Then you should not add it now, but rather when you add replay.  Treat
> this part of the series as merely fixing migration bugs.  In fact, I
> suggest that you start by progressively refining these patches.  You can
> also post the other patches that I mentioned were good to go in my
> review of v1.  Then pass to the next steps (in the meanwhile, Fred's
> icount reverse-exec patches might get merged too).

 Do you mean, that I should separate the migration patches from 
the replay ones and submit them in another series?

Pavel Dovgalyuk

Re: [Qemu-devel] [RFC PATCH v3 07/49] kvmapic: fixing loading vmstate

[Paolo Bonzini]

Il 25/08/2014 13:40, Pavel Dovgaluk ha scritto:
>  Do you mean, that I should separate the migration patches from 
> the replay ones and submit them in another series?

Yes, that will help getting the less controversial pieces in first.

Paolo