Re: [Qemu-devel] [PATCH 02/10] apic_common: vapic_paddr synchronization fix

[Paolo Bonzini <pbonzini@redhat.com>]

Il 09/09/2014 16:00, Michael S. Tsirkin ha scritto:
> On Tue, Sep 09, 2014 at 02:30:01PM +0200, Paolo Bonzini wrote:
>> From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
>>
>> This patch postpones vapic_paddr initialization, which is performed
>> during migration. When vapic_paddr is synchronized within the migration
>> process, apic_common functions could operate with incorrect apic state,
>> if it hadn't loaded yet. This patch postpones the synchronization until
>> the virtual machine is started, ensuring that the whole virtual machine
>> state has been loaded.
>>
>> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hw/i386/kvmvapic.c | 37 ++++++++++++++++++++++++++-----------
>>  1 file changed, 26 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
>> index ee95963..2bcc249 100644
>> --- a/hw/i386/kvmvapic.c
>> +++ b/hw/i386/kvmvapic.c
>> @@ -59,6 +59,7 @@ typedef struct VAPICROMState {
>>      GuestROMState rom_state;
>>      size_t rom_size;
>>      bool rom_mapped_writable;
>> +    VMChangeStateEntry *vmsentry;
>>  } VAPICROMState;
>>  
>>  #define TYPE_VAPIC "kvmvapic"
>> @@ -734,11 +735,34 @@ static void do_vapic_enable(void *data)
>>      vapic_enable(s, cpu);
>>  }
>>  
>> -static int vapic_post_load(void *opaque, int version_id)
>> +static void kvmvapic_vm_state_change(void *opaque, int running,
>> +				     RunState state)
>>  {
>>      VAPICROMState *s = opaque;
>>      uint8_t *zero;
>>  
>> +    if (!running) {
>> +        return;
>> +    }
>> +
>> +    if (s->state == VAPIC_ACTIVE) {
>> +        if (smp_cpus == 1) {
> 
> maybe add a comment explaining why we need
> to special case smp_cpus?

I don't know honestly.  I'm just moving code around.

>> +            run_on_cpu(first_cpu, do_vapic_enable, s);
> 
> Is this safe to do in vmstate handler? cpu isn't running yet is it?

Yes, it isn't running and run_on_cpu is synchronous.

Paolo

>> +        } else {
>> +            zero = g_malloc0(s->rom_state.vapic_size);
>> +            cpu_physical_memory_write(s->vapic_paddr, zero,
>> +                                      s->rom_state.vapic_size);
>> +            g_free(zero);
>> +        }
>> +    }
>> +
>> +    qemu_del_vm_change_state_handler(s->vmsentry);
>> +}
>> +
>> +static int vapic_post_load(void *opaque, int version_id)
>> +{
>> +    VAPICROMState *s = opaque;
>> +
>>      /*
>>       * The old implementation of qemu-kvm did not provide the state
>>       * VAPIC_STANDBY. Reconstruct it.
>> @@ -752,17 +776,8 @@ static int vapic_post_load(void *opaque, int version_id)
>>              return -1;
>>          }
>>      }
>> -    if (s->state == VAPIC_ACTIVE) {
>> -        if (smp_cpus == 1) {
>> -            run_on_cpu(first_cpu, do_vapic_enable, s);
>> -        } else {
>> -            zero = g_malloc0(s->rom_state.vapic_size);
>> -            cpu_physical_memory_write(s->vapic_paddr, zero,
>> -                                      s->rom_state.vapic_size);
>> -            g_free(zero);
>> -        }
>> -    }
>>  
>> +    s->vmsentry = qemu_add_vm_change_state_handler(kvmvapic_vm_state_change, s);
>>      return 0;
>>  }
>>  
>> -- 
>> 2.1.0
>>
>>