| Message ID | 1410530338-17615-14-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
Hi, Paolo! > From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini > 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. > > > +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; I've tested this patch with replay. I enabled VM reset (which was previously disabled for replay) while loading the VM state and discovered the following problem. vapic_enable function in kvmapic.c retrieves cpu number with the get_kpcr_number() function. When cpu number is -1 vapic_enable exits and does not call apic_enable_vapic, which should setup vapic_paddr field. Without this call vapic_paddr remains initialized with default value and behavior of the virtual machine becomes different. Pavel Dovgalyuk
Il 19/09/2014 12:43, Pavel Dovgaluk ha scritto: > I've tested this patch with replay. I enabled VM reset (which was previously disabled for replay) > while loading the VM state and discovered the following problem. > vapic_enable function in kvmapic.c retrieves cpu number with the get_kpcr_number() function. > When cpu number is -1 vapic_enable exits and does not call apic_enable_vapic, which should > setup vapic_paddr field. > Without this call vapic_paddr remains initialized with default value and behavior of the virtual > machine becomes different. IIUC the fix would be to move part of vapic_enable out to its separate function, and call it from do_vapic_enable? Could you prepare a patch? Paolo
> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini > Il 19/09/2014 12:43, Pavel Dovgaluk ha scritto: > > I've tested this patch with replay. I enabled VM reset (which was previously disabled for > replay) > > while loading the VM state and discovered the following problem. > > vapic_enable function in kvmapic.c retrieves cpu number with the get_kpcr_number() function. > > When cpu number is -1 vapic_enable exits and does not call apic_enable_vapic, which should > > setup vapic_paddr field. > > Without this call vapic_paddr remains initialized with default value and behavior of the > virtual > > machine becomes different. > > IIUC the fix would be to move part of vapic_enable out to its separate > function, and call it from do_vapic_enable? Could you prepare a patch? static int vapic_enable(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_write(vapic_paddr + offsetof(VAPICState, enabled), &enabled, sizeof(enabled)); apic_enable_vapic(cpu->apic_state, vapic_paddr); s->state = VAPIC_ACTIVE; return 0; } vapic_paddr depends on cpu_number. cpu_number cannot be retrieved when do_vapic_enable executes. Thus we cannot reconstruct vapic_paddr in that function. Pavel Dovgalyuk
Il 19/09/2014 14:50, Pavel Dovgaluk ha scritto: > vapic_paddr depends on cpu_number. cpu_number cannot be retrieved when do_vapic_enable executes. > Thus we cannot reconstruct vapic_paddr in that function. cpu_number will always be zero, because do_vapic_enable is only executed for smp_cpus == 1. Paolo
Il 22/09/2014 10:21, Pavel Dovgaluk ha scritto: > >> -----Original Message----- >> From: Paolo Bonzini [mailto:pbonzini@redhat.com] >> Sent: Friday, September 19, 2014 5:42 PM >> To: Pavel Dovgaluk; qemu-devel@nongnu.org >> Subject: Re: [PULL 13/21] apic_common: vapic_paddr synchronization fix >> >> Il 19/09/2014 14:50, Pavel Dovgaluk ha scritto: >>> vapic_paddr depends on cpu_number. cpu_number cannot be retrieved when do_vapic_enable >> executes. >>> Thus we cannot reconstruct vapic_paddr in that function. >> >> cpu_number will always be zero, because do_vapic_enable is only executed >> for smp_cpus == 1. > > Right. Here it is: This patch was already applied. Please make a new patch on top of current master, and send it with your Signed-off-by. Paolo
> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini > > 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> > Tested-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(-) One more issue for this patch. > -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) { Exitting here doesn't remove vmsentry. When we load VM state for multiple times, list of the handlers will be filled with garbage. > + return; > + } > + > + 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); > + } > + } > + > + qemu_del_vm_change_state_handler(s->vmsentry); > +} Pavel Dovgalyuk
Il 26/09/2014 13:18, Pavel Dovgaluk ha scritto: >> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini >> >> 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> >> Tested-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(-) > > One more issue for this patch. > >> -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) { > > Exitting here doesn't remove vmsentry. When we load VM state for multiple times, > list of the handlers will be filled with garbage. Thanks. Paolo >> + return; >> + } >> + >> + 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); >> + } >> + } >> + >> + qemu_del_vm_change_state_handler(s->vmsentry); >> +} > > > Pavel Dovgalyuk >
diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c index ee95963..2cca7a4 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) { + 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); + } + } + + 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; }