[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 06/12] kvmvapic: fixing loading vmstate
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH 06/12] kvmvapic: fixing loading vmstate |
Date: |
Tue, 09 Sep 2014 14:14:36 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.0 |
Il 09/09/2014 12:30, Pavel Dovgaluk ha scritto:
>> From: Paolo Bonzini [mailto:address@hidden
>> 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