From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33563) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XRMbJ-0000WJ-0N for qemu-devel@nongnu.org; Tue, 09 Sep 2014 10:41:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XRMbE-0002E8-2u for qemu-devel@nongnu.org; Tue, 09 Sep 2014 10:40:56 -0400 Received: from mx1.redhat.com ([209.132.183.28]:20915) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XRMND-0004Od-Ok for qemu-devel@nongnu.org; Tue, 09 Sep 2014 10:26:23 -0400 Message-ID: <540F0E08.1030005@redhat.com> Date: Tue, 09 Sep 2014 16:26:16 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1410265809-27247-1-git-send-email-pbonzini@redhat.com> <1410265809-27247-3-git-send-email-pbonzini@redhat.com> <20140909140004.GB13212@redhat.com> In-Reply-To: <20140909140004.GB13212@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 02/10] apic_common: vapic_paddr synchronization fix List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: amit.shah@redhat.com, quintela@redhat.com, qemu-devel@nongnu.org, Pavel.Dovgaluk@ispras.ru, dgilbert@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 >> >> 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 >> Signed-off-by: Paolo Bonzini >> --- >> 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 >> >>