Re: [Qemu-devel] [PATCH 06/12] kvmvapic: fixing loading vmstate

[Paolo Bonzini <pbonzini@redhat.com>]

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 {
> 
> 
>