Re: [Qemu-devel] [PATCH] Revert "apic: save apic_delivered flag"

[Paolo Bonzini]

On 23/03/2017 08:34, Pavel Dovgalyuk wrote:
> This value is used by mc146818rtc.
> Therefore it affects the vitrual machine state.

It is used indeed, but it should not be used across virtual machine
migration or savevm.  The value doesn't matter as soon as
apic_get_irq_delivered is called, because there will be an
apic_reset_irq_delivered call before the next access; and when migrating
or saving a virtual machine, you cannot be between the calls to
apic_reset_irq_delivered and apic_get_irq_delivered.

It would be interesting to try something like this:

diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 7a6e771..10a114d 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -32,6 +32,7 @@
 #include "hw/sysbus.h"

 static int apic_irq_delivered;
+static bool apic_irq_delivered_valid;
 bool apic_report_tpr_access;

 void cpu_set_apic_base(DeviceState *dev, uint64_t val)
@@ -136,13 +137,18 @@ void apic_reset_irq_delivered(void)
     volatile int a_i_d = apic_irq_delivered;
     trace_apic_reset_irq_delivered(a_i_d);

+    assert(!apic_irq_delivered_valid);
     apic_irq_delivered = 0;
+    apic_irq_delivered_valid = true;
 }

 int apic_get_irq_delivered(void)
 {
     trace_apic_get_irq_delivered(apic_irq_delivered);

+    assert(apic_irq_delivered_valid);
+    apic_irq_delivered_valid = false;
+
     return apic_irq_delivered;
 }


Paolo

> I've encountered the cases when replay was broken without migrating of
> this variable.
> Отправлено с помощью BlueMail <http://www.bluemail.me/r?b=9226>
> На 22 Мар 2017 г., в 15:13, Paolo Bonzini <address@hidden
> <mailto:address@hidden>> написал:
> 
>     This reverts commit 07bfa354772f2de67008dc66c201b627acff0106.
>     The global variable is only read as part of a
> 
>                 apic_reset_irq_delivered();
>                 qemu_irq_raise(s->irq);
>                 if (!apic_get_irq_delivered()) {
> 
>     sequence, so the value never matters at migration time.
> 
>     Reported-by: Dr. David Alan Gilbert <address@hidden>
>     Cc: Pavel Dovgalyuk <address@hidden>
>     Signed-off-by: Paolo Bonzini <address@hidden>
>     ---
>      hw/intc/apic_common.c           | 33
>     ------------------------------------------------------------------------
> 
>      include/hw/i386/apic_internal.h |  2 --
>      2 files changed, 35 deletions(-)
> 
>     diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
>     index 7a6e771..c3829e3 100644
>     --- a/hw/intc/apic_common.c
>     +++ b/hw/intc/apic_common.c
>     @@ -387,25 +387,6 @@ static bool apic_common_sipi_needed(void *opaque)
>          return s->wait_for_sipi != 0;
>      }
>      
>     -static bool apic_irq_delivered_needed(void *opaque)
>     -{
>     -    APICCommonState *s = APIC_COMMON(opaque);
>     -    return s->cpu == X86_CPU(first_cpu) && apic_irq_delivered != 0;
>     -}
>     -
>     -static void apic_irq_delivered_pre_save(void *opaque)
>     -{
>     -    APICCommonState *s = APIC_COMMON(opaque);
>     -    s->apic_irq_delivered = apic_irq_delivered;
>     -}
>     -
>     -static int apic_irq_delivered_post_load(void *opaque, int version_id)
>     -{
>     -    APICCommonState *s = APIC_COMMON(opaque);
>     -    apic_irq_delivered = s->apic_irq_delivered;
>     -    return 0;
>     -}
>     -
>      static const VMStateDescription vmstate_apic_common_sipi = {
>          .name = "apic_sipi",
>          .version_id = 1,
>     @@ -418,19 +399,6 @@ static const VMStateDescription 
> vmstate_apic_common_sipi = {
>          }
>      };
>      
>     -static const VMStateDescription vmstate_apic_irq_delivered = {
>     -    .name = "apic_irq_delivered",
>     -    .version_id = 1,
>     -    .minimum_version_id = 1,
>     -    .needed = apic_irq_delivered_needed,
>     -    .pre_save = apic_irq_delivered_pre_save,
>     -    .post_load = apic_irq_delivered_post_load,
>     -    .fields = (VMStateField[]) {
>     -        VMSTATE_INT32(apic_irq_delivered, APICCommonState),
>     -        VMSTATE_END_OF_LIST()
>     -    }
>     -};
>     -
>      static const VMStateDescription vmstate_apic_common = {
>          .name = "apic",
>          .version_id = 3,
>     @@ -465,7 +433,6 @@ static const VMStateDescription vmstate_apic_common = 
> {
>          },
>          .subsections = (const VMStateDescription*[]) {
>              &vmstate_apic_common_sipi,
>     -        &vmstate_apic_irq_delivered,
>              NULL
>          }
>      };
>     diff --git a/include/hw/i386/apic_internal.h 
> b/include/hw/i386/apic_internal.h
>     index 20ad28c..1209eb4 100644
>     --- a/include/hw/i386/apic_internal.h
>     +++ b/include/hw/i386/apic_internal.h
>     @@ -189,8 +189,6 @@ struct APICCommonState {
>          DeviceState *vapic;
>          hwaddr vapic_paddr; /* note: persistence via kvmvapic */
>          bool legacy_instance_id;
>     -
>     -    int apic_irq_delivered; /* for saving static variable */
>      };
>      
>      typedef struct VAPICState {
>