[RFC,v2,07/49] kvmapic: 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 |    5 ++++-
 2 files changed, 25 insertions(+), 2 deletions(-)

Il 17/07/2014 13:02, 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.

This comment is now obsolete:

include/hw/i386/apic_internal.h:    hwaddr vapic_paddr; /* note: persistence via kvmvapic */

> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> ---
>  hw/i386/kvmvapic.c    |   22 +++++++++++++++++++++-
>  hw/intc/apic_common.c |    5 ++++-
>  2 files changed, 25 insertions(+), 2 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..9d75ee0 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -347,7 +347,7 @@ static int apic_dispatch_post_load(void *opaque, int version_id)
>  
>  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,
> @@ -374,6 +374,9 @@ static const VMStateDescription vmstate_apic_common = {
>          VMSTATE_INT64(next_time, APICCommonState),
>          VMSTATE_INT64(timer_expiry,
>                        APICCommonState), /* open-coded timer state */
> +        VMSTATE_INT32_V(sipi_vector, APICCommonState, 4),
> +        VMSTATE_INT32_V(wait_for_sipi, APICCommonState, 4),

This could be a subsection.  sipi_vector is only used (needed) if wait_for_sipi != 0.

> +        VMSTATE_UINT64_V(vapic_paddr, APICCommonState, 4),

Here you could also use a subsection, where the "needed" function returns false if vapic_paddr == 0.

Paolo


>          VMSTATE_END_OF_LIST()
>      }
>  };
> 
> 
>

> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
> Il 17/07/2014 13:02, Pavel Dovgalyuk ha scritto:
> > diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> > index ce3d903..9d75ee0 100644
> > --- a/hw/intc/apic_common.c
> > +++ b/hw/intc/apic_common.c
> > @@ -347,7 +347,7 @@ static int apic_dispatch_post_load(void *opaque, int version_id)
> >
> >  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,
> > @@ -374,6 +374,9 @@ static const VMStateDescription vmstate_apic_common = {
> >          VMSTATE_INT64(next_time, APICCommonState),
> >          VMSTATE_INT64(timer_expiry,
> >                        APICCommonState), /* open-coded timer state */
> > +        VMSTATE_INT32_V(sipi_vector, APICCommonState, 4),
> > +        VMSTATE_INT32_V(wait_for_sipi, APICCommonState, 4),
> 
> This could be a subsection.  sipi_vector is only used (needed) if wait_for_sipi != 0.

  Right, sipi_vector is used when wait_for_sipi != 0. But we can set sipi_vector to non-zero,
save the snapshot, and then set wait_for_sipi. If that snapshot will be loaded, sipi_vector become incorrect.
Isn't this scenario possible?

Pavel Dovgalyuk

Il 29/07/2014 14:03, Pavel Dovgaluk ha scritto:
>> > 
>> > This could be a subsection.  sipi_vector is only used (needed) if wait_for_sipi != 0.
>   Right, sipi_vector is used when wait_for_sipi != 0. But we can set sipi_vector to non-zero,
> save the snapshot, and then set wait_for_sipi. If that snapshot will be loaded, sipi_vector become incorrect.
> Isn't this scenario possible?

sipi_vector will not be used until CPU_INTERRUPT_SIPI is set, and then
sipi_vector will have been overwritten with a new value.  The
architecture guarantees that.

Paolo