[06/12] kvmvapic: fixing loading vmstate

Il 27/08/2014 15:03, Pavel Dovgaluk ha scritto:
>> > 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.

In KVM mode, it is not a problem to call apic_enable_vapic before APIC
state is loaded; all vapic processing is delayed anyway to after the
VCPUs are started.

In TCG mode, apic_enable_vapic calls apic_sync_vapic.

Taking inspiration from what KVM does, the fix could be even simpler
than a change state handler.  run_on_cpu functions do not run while the
VM is stopped, so the following should work:

 void apic_handle_tpr_access_report(DeviceState *dev, target_ulong ip,

> All fields that were added to vmstate are not affected by this function.

The sipi_vector and wait_for_sipi parts of this patch are okay.  You
should split those in a separate patch.

Paolo

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> Il 27/08/2014 15:03, Pavel Dovgaluk ha scritto:
> >> > 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.
> 
> Taking inspiration from what KVM does, the fix could be even simpler
> than a change state handler.  run_on_cpu functions do not run while the
> VM is stopped, so the following should work:
> 
> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index ce3d903..81d1ad7 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -91,13 +91,20 @@ void apic_enable_tpr_access_reporting(DeviceState
> *dev, bool enable)
>      }
>  }
> 
> +static void do_apic_enable_vapic(void *data)
> +{
> +    APICCommonState *s = APIC_COMMON(data);
> +    APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
> +
> +    info->vapic_base_update(s);
> +}
> +
>  void apic_enable_vapic(DeviceState *dev, hwaddr paddr)
>  {
>      APICCommonState *s = APIC_COMMON(dev);
> -    APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
> 
>      s->vapic_paddr = paddr;
> -    info->vapic_base_update(s);
> +    run_on_cpu(CPU(s->cpu), do_apic_enable_vapic, s);
>  }

I've tried this one and it doesn't work.
do_apic_enable_vapic runs on cpu, at the same time the VM state is loaded.
APIC state still remains broken because of this.

Pavel Dovgalyuk

Il 09/09/2014 12:30, Pavel Dovgaluk ha scritto:
>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>> Il 27/08/2014 15:03, Pavel Dovgaluk ha scritto:
>>>>> 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.
>>
>> Taking inspiration from what KVM does, the fix could be even simpler
>> than a change state handler.  run_on_cpu functions do not run while the
>> VM is stopped, so the following should work:
>>
>> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
>> index ce3d903..81d1ad7 100644
>> --- a/hw/intc/apic_common.c
>> +++ b/hw/intc/apic_common.c
>> @@ -91,13 +91,20 @@ void apic_enable_tpr_access_reporting(DeviceState
>> *dev, bool enable)
>>      }
>>  }
>>
>> +static void do_apic_enable_vapic(void *data)
>> +{
>> +    APICCommonState *s = APIC_COMMON(data);
>> +    APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
>> +
>> +    info->vapic_base_update(s);
>> +}
>> +
>>  void apic_enable_vapic(DeviceState *dev, hwaddr paddr)
>>  {
>>      APICCommonState *s = APIC_COMMON(dev);
>> -    APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
>>
>>      s->vapic_paddr = paddr;
>> -    info->vapic_base_update(s);
>> +    run_on_cpu(CPU(s->cpu), do_apic_enable_vapic, s);
>>  }
> 
> I've tried this one and it doesn't work.
> do_apic_enable_vapic runs on cpu, at the same time the VM state is loaded.
> APIC state still remains broken because of this.

You're right (in fact run_on_cpu is synchronous so the alternative would
have been deadlock).  A change state handler can work.  I'll submit your
patches to the migration maintainers, including an alternative fix for
this VAPIC problem.

Paolo