[06/12] kvmvapic: fixing loading vmstate

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           |   44 +++++++++++++++++++++++++++++++++++++++
 include/hw/i386/apic_internal.h |    2 +-
 3 files changed, 66 insertions(+), 2 deletions(-)

Il 26/08/2014 09:15, Pavel Dovgalyuk ha scritto:
> 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.

Can you use a vm_change_state_handler, or a QEMU_CLOCK_VIRTUAL timer
with expiration time in the past (e.g. at time zero) to run the sync
code as soon as possible?  Then you can preserve the current migration
format and avoid using the invalid APIC state.

Paolo

> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> ---
>  hw/i386/kvmvapic.c              |   22 +++++++++++++++++++-
>  hw/intc/apic_common.c           |   44 +++++++++++++++++++++++++++++++++++++++
>  include/hw/i386/apic_internal.h |    2 +-
>  3 files changed, 66 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
> index ee95963..3c403c5 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..8d672bd 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -345,6 +345,39 @@ 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,
> @@ -375,6 +408,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 {
> 
> 
>

> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
> Il 26/08/2014 09:15, Pavel Dovgalyuk ha scritto:
> > 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.
> 
> Can you use a vm_change_state_handler, or a QEMU_CLOCK_VIRTUAL timer
> with expiration time in the past (e.g. at time zero) to run the sync
> code as soon as possible?  Then you can preserve the current migration
> format and avoid using the invalid APIC state.

Does this method guarantee, that nobody (like other timers) will access
APIC between loading the vmstate and invocation of the timer?

Pavel Dovgalyuk

Il 27/08/2014 14:16, Pavel Dovgaluk ha scritto:
>> > Can you use a vm_change_state_handler, or a QEMU_CLOCK_VIRTUAL timer
>> > with expiration time in the past (e.g. at time zero) to run the sync
>> > code as soon as possible?  Then you can preserve the current migration
>> > format and avoid using the invalid APIC state.
> Does this method guarantee, that nobody (like other timers) will access
> APIC between loading the vmstate and invocation of the timer?

Hmm, probably not.  The bug would not be other timers accessing the
APIC, because that would also call apic_sync_vapic and the only effect
would be an extra useless synchronization.  The bug would happen if the
APIC is accessed by the CPU before the timer has the occasion to run.

However, a vm_change_state_handler should work.  It runs before VCPUs
are started.

Paolo

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> Il 27/08/2014 14:16, Pavel Dovgaluk ha scritto:
> >> > Can you use a vm_change_state_handler, or a QEMU_CLOCK_VIRTUAL timer
> >> > with expiration time in the past (e.g. at time zero) to run the sync
> >> > code as soon as possible?  Then you can preserve the current migration
> >> > format and avoid using the invalid APIC state.
> > Does this method guarantee, that nobody (like other timers) will access
> > APIC between loading the vmstate and invocation of the timer?
> 
> Hmm, probably not.  The bug would not be other timers accessing the
> APIC, because that would also call apic_sync_vapic and the only effect
> would be an extra useless synchronization.  The bug would happen if the
> APIC is accessed by the CPU before the timer has the occasion to run.

Sorry, but I don't understand which problem we will solve with apic_sync_vapic.
All fields that were added to vmstate are not affected by this function.
Could you explain your idea?

Pavel Dovgalyuk