| Message ID | 20110417135045.GA8741@valinux.co.jp |
|---|---|
| State | New |
| Headers | show |
On 04/17/2011 04:50 PM, Isaku Yamahata wrote: > On Sun, Apr 17, 2011 at 04:17:51PM +0300, Avi Kivity wrote: > > On 03/16/2011 11:29 AM, Isaku Yamahata wrote: > >> factor out ACPI GPE logic. Later it will be used by ICH9 ACPI. > >> > > > > I think this patch is causing qemu-kvm failures on migration: > > (gdb) bt > > #0 0x000000000049aff4 in qemu_put_be16s (f=0x1a74490, pv=0x2c02580, > > size=2) at hw/hw.h:108 > > #1 put_uint16 (f=0x1a74490, pv=0x2c02580, size=2) at savevm.c:855 > > #2 0x000000000049c3e4 in vmstate_save_state (f=0x1a74490, > > vmsd=0x6f0b00, opaque=0x1842ef0) at savevm.c:1436 > > #3 0x000000000049c3b6 in vmstate_save_state (f=0x1a74490, > > vmsd=0x6f0aa0, opaque=0x1842b90) at savevm.c:1434 > > #4 0x000000000049c6f1 in vmstate_save (mon=<value optimized out>, > > f=0x1a74490) at savevm.c:1459 > > #5 qemu_savevm_state_complete (mon=<value optimized out>, f=0x1a74490) > > at savevm.c:1600 > > #6 0x000000000049455a in migrate_fd_put_ready (opaque=0x1847890) at > > migration.c:383 > > #7 0x00000000004ce2eb in qemu_run_timers (clock=<value optimized out>) > > at qemu-timer.c:505 > > #8 0x00000000004ce806 in qemu_run_all_timers () at qemu-timer.c:619 > > #9 0x0000000000419463 in main_loop_wait (nonblocking=<value optimized > > out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:1339 > > #10 0x0000000000433927 in kvm_main_loop () at > > /build/home/tlv/akivity/qemu-kvm/qemu-kvm.c:1590 > > #11 0x000000000041a3a6 in main_loop (argc=<value optimized out>, > > argv=<value optimized out>, envp=<value optimized out>) > > at /build/home/tlv/akivity/qemu-kvm/vl.c:1369 > > #12 main (argc=<value optimized out>, argv=<value optimized out>, > > envp=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:3257 > > > > The vmstate being migrated is "gpe". > > > > > > > >> > >> +#define VMSTATE_GPE_ARRAY(_field, _state) \ > >> + { \ > >> + .name = (stringify(_field)), \ > >> + .version_id = 0, \ > >> + .num = GPE_LEN, \ > >> + .info =&vmstate_info_uint16, \ > >> + .size = sizeof(uint16_t), \ > >> + .flags = VMS_ARRAY | VMS_POINTER, \ > >> + .offset = vmstate_offset_pointer(_state, _field, uint8_t), \ > >> + } > >> + > >> static const VMStateDescription vmstate_gpe = { > >> .name = "gpe", > >> .version_id = 1, > >> .minimum_version_id = 1, > >> .minimum_version_id_old = 1, > >> .fields = (VMStateField []) { > >> - VMSTATE_UINT16(sts, struct gpe_regs), > >> - VMSTATE_UINT16(en, struct gpe_regs), > >> + VMSTATE_GPE_ARRAY(sts, ACPIGPE), > >> + VMSTATE_GPE_ARRAY(en, ACPIGPE), > >> VMSTATE_END_OF_LIST() > >> } > >> }; > > > > I'm no vmstate expert, but this does look odd. Why both VMS_ARRAY and > > VMS_POINTER? aren't we trying to save/restore a simple 16-bit value? Or > > at least we did before this patch. > > That's right. the difference is, the new member type became uint8_t*. > Does the following help? > Unforunately not. The core is very similar though: #0 0x000000000049aff4 in qemu_put_be16s (f=0x2427490, pv=0x1ae7580, size=2) at hw/hw.h:108 #1 put_uint16 (f=0x2427490, pv=0x1ae7580, size=2) at savevm.c:855 #2 0x000000000049c3e4 in vmstate_save_state (f=0x2427490, vmsd=0x6f0b00, opaque=0x21f5ef0) at savevm.c:1436 #3 0x000000000049c3b6 in vmstate_save_state (f=0x2427490, vmsd=0x6f0aa0, opaque=0x21f5b90) at savevm.c:1434 #4 0x000000000049c6f1 in vmstate_save (mon=<value optimized out>, f=0x2427490) at savevm.c:1459 #5 qemu_savevm_state_complete (mon=<value optimized out>, f=0x2427490) at savevm.c:1600 #6 0x000000000049455a in migrate_fd_put_ready (opaque=0x21fa8c0) at migration.c:383 #7 0x00000000004ce2eb in qemu_run_timers (clock=<value optimized out>) at qemu-timer.c:505 #8 0x00000000004ce806 in qemu_run_all_timers () at qemu-timer.c:619 #9 0x0000000000419463 in main_loop_wait (nonblocking=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:1339 #10 0x0000000000433927 in kvm_main_loop () at /build/home/tlv/akivity/qemu-kvm/qemu-kvm.c:1590 #11 0x000000000041a3a6 in main_loop (argc=<value optimized out>, argv=<value optimized out>, envp=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:1369 #12 main (argc=<value optimized out>, argv=<value optimized out>, envp=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:3257
On Sun, Apr 17, 2011 at 06:53:12PM +0300, Avi Kivity wrote: > On 04/17/2011 04:50 PM, Isaku Yamahata wrote: >> On Sun, Apr 17, 2011 at 04:17:51PM +0300, Avi Kivity wrote: >> > On 03/16/2011 11:29 AM, Isaku Yamahata wrote: >> >> factor out ACPI GPE logic. Later it will be used by ICH9 ACPI. >> >> >> > >> > I think this patch is causing qemu-kvm failures on migration: >> > (gdb) bt >> > #0 0x000000000049aff4 in qemu_put_be16s (f=0x1a74490, pv=0x2c02580, >> > size=2) at hw/hw.h:108 >> > #1 put_uint16 (f=0x1a74490, pv=0x2c02580, size=2) at savevm.c:855 >> > #2 0x000000000049c3e4 in vmstate_save_state (f=0x1a74490, >> > vmsd=0x6f0b00, opaque=0x1842ef0) at savevm.c:1436 >> > #3 0x000000000049c3b6 in vmstate_save_state (f=0x1a74490, >> > vmsd=0x6f0aa0, opaque=0x1842b90) at savevm.c:1434 >> > #4 0x000000000049c6f1 in vmstate_save (mon=<value optimized out>, >> > f=0x1a74490) at savevm.c:1459 >> > #5 qemu_savevm_state_complete (mon=<value optimized out>, f=0x1a74490) >> > at savevm.c:1600 >> > #6 0x000000000049455a in migrate_fd_put_ready (opaque=0x1847890) at >> > migration.c:383 >> > #7 0x00000000004ce2eb in qemu_run_timers (clock=<value optimized out>) >> > at qemu-timer.c:505 >> > #8 0x00000000004ce806 in qemu_run_all_timers () at qemu-timer.c:619 >> > #9 0x0000000000419463 in main_loop_wait (nonblocking=<value optimized >> > out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:1339 >> > #10 0x0000000000433927 in kvm_main_loop () at >> > /build/home/tlv/akivity/qemu-kvm/qemu-kvm.c:1590 >> > #11 0x000000000041a3a6 in main_loop (argc=<value optimized out>, >> > argv=<value optimized out>, envp=<value optimized out>) >> > at /build/home/tlv/akivity/qemu-kvm/vl.c:1369 >> > #12 main (argc=<value optimized out>, argv=<value optimized out>, >> > envp=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:3257 >> > >> > The vmstate being migrated is "gpe". >> > >> > >> > >> >> >> >> +#define VMSTATE_GPE_ARRAY(_field, _state) \ >> >> + { \ >> >> + .name = (stringify(_field)), \ >> >> + .version_id = 0, \ >> >> + .num = GPE_LEN, \ >> >> + .info =&vmstate_info_uint16, \ >> >> + .size = sizeof(uint16_t), \ >> >> + .flags = VMS_ARRAY | VMS_POINTER, \ >> >> + .offset = vmstate_offset_pointer(_state, _field, uint8_t), \ >> >> + } >> >> + >> >> static const VMStateDescription vmstate_gpe = { >> >> .name = "gpe", >> >> .version_id = 1, >> >> .minimum_version_id = 1, >> >> .minimum_version_id_old = 1, >> >> .fields = (VMStateField []) { >> >> - VMSTATE_UINT16(sts, struct gpe_regs), >> >> - VMSTATE_UINT16(en, struct gpe_regs), >> >> + VMSTATE_GPE_ARRAY(sts, ACPIGPE), >> >> + VMSTATE_GPE_ARRAY(en, ACPIGPE), >> >> VMSTATE_END_OF_LIST() >> >> } >> >> }; >> > >> > I'm no vmstate expert, but this does look odd. Why both VMS_ARRAY and >> > VMS_POINTER? aren't we trying to save/restore a simple 16-bit value? Or >> > at least we did before this patch. >> >> That's right. the difference is, the new member type became uint8_t*. >> Does the following help? >> > > Unforunately not. The core is very similar though: Hmm but it seems that there is another issue. With the previous patch, I confirmed sts and en are saved as expected. The related VMStateDescription are defined in acpi_piix.c in the following order. vmstate_acpi calls vmstate_gpe. and &vmstate_gpe < &vmstate_acpi. However, your call trace shows something different. (I'm assuming the stack trace is correct. gdb or compiler optimization?) static const VMStateDescription vmstate_gpe = { ... static const VMStateDescription vmstate_acpi = { .fields = (VMStateField []) { ... VMSTATE_STRUCT(gpe, PIIX4PMState, 2, vmstate_gpe, ACPIGPE), > > #0 0x000000000049aff4 in qemu_put_be16s (f=0x2427490, pv=0x1ae7580, > size=2) at hw/hw.h:108 > #1 put_uint16 (f=0x2427490, pv=0x1ae7580, size=2) at savevm.c:855 > #2 0x000000000049c3e4 in vmstate_save_state (f=0x2427490, > vmsd=0x6f0b00, opaque=0x21f5ef0) at savevm.c:1436 vmsd = 0x6f0b00 = &vmstate_gpe <<<<<< correct??? opaque = 0x21f5ef0 = &PIIX4PMState->gpe > #3 0x000000000049c3b6 in vmstate_save_state (f=0x2427490, > vmsd=0x6f0aa0, opaque=0x21f5b90) at savevm.c:1434 vmsd = 0x6f0aa0 = &vmstate_acpi <<<<<<<<<<<<< correct? opaque = 0x21f5b90 = &PIIX4PMState opaque seems correct as 0x21f5b90 - 0x21f5ef0 = 0x348 = offsetof(PIIX4PMState, gpe) On my environment the stack trace looks like (gdb) where #0 vmstate_save_state (f=0x25d16d0, vmsd=0x695940, opaque=0x283a218) at savevm.c:1374 #1 0x00000000004bb4f7 in vmstate_save_state (f=0x25d16d0, vmsd=0x695a40, opaque=0x2839ed0) at savevm.c:1412 ... (gdb) print &vmstate_gpe $32 = (const VMStateDescription *) 0x695940 (gdb) print &vmstate_acpi $33 = (const VMStateDescription *) 0x695a40 thanks, > #4 0x000000000049c6f1 in vmstate_save (mon=<value optimized out>, > f=0x2427490) at savevm.c:1459 > #5 qemu_savevm_state_complete (mon=<value optimized out>, f=0x2427490) > at savevm.c:1600 > #6 0x000000000049455a in migrate_fd_put_ready (opaque=0x21fa8c0) at > migration.c:383 > #7 0x00000000004ce2eb in qemu_run_timers (clock=<value optimized out>) > at qemu-timer.c:505 > #8 0x00000000004ce806 in qemu_run_all_timers () at qemu-timer.c:619 > #9 0x0000000000419463 in main_loop_wait (nonblocking=<value optimized > out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:1339 > #10 0x0000000000433927 in kvm_main_loop () at > /build/home/tlv/akivity/qemu-kvm/qemu-kvm.c:1590 > #11 0x000000000041a3a6 in main_loop (argc=<value optimized out>, > argv=<value optimized out>, envp=<value optimized out>) > at /build/home/tlv/akivity/qemu-kvm/vl.c:1369 > #12 main (argc=<value optimized out>, argv=<value optimized out>, > envp=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:3257 > > > -- > error compiling committee.c: too many arguments to function >
On 04/18/2011 10:47 AM, Isaku Yamahata wrote: > Hmm but it seems that there is another issue. > With the previous patch, I confirmed sts and en are saved as expected. > > The related VMStateDescription are defined in acpi_piix.c in > the following order. > vmstate_acpi calls vmstate_gpe. and&vmstate_gpe< &vmstate_acpi. > (gdb) p &vmstate_gpe $1 = (const VMStateDescription *) 0x6f0b00 (gdb) p &vmstate_acpi $2 = (const VMStateDescription *) 0x6f0aa0 So no. > However, your call trace shows something different. > (I'm assuming the stack trace is correct. gdb or compiler optimization?) > > static const VMStateDescription vmstate_gpe = { > ... > > static const VMStateDescription vmstate_acpi = { > .fields = (VMStateField []) { > ... > VMSTATE_STRUCT(gpe, PIIX4PMState, 2, vmstate_gpe, ACPIGPE), > > > > > > #0 0x000000000049aff4 in qemu_put_be16s (f=0x2427490, pv=0x1ae7580, > > size=2) at hw/hw.h:108 > > #1 put_uint16 (f=0x2427490, pv=0x1ae7580, size=2) at savevm.c:855 > > #2 0x000000000049c3e4 in vmstate_save_state (f=0x2427490, > > vmsd=0x6f0b00, opaque=0x21f5ef0) at savevm.c:1436 > > vmsd = 0x6f0b00 =&vmstate_gpe<<<<<< correct??? Correct. > opaque = 0x21f5ef0 =&PIIX4PMState->gpe > > > #3 0x000000000049c3b6 in vmstate_save_state (f=0x2427490, > > vmsd=0x6f0aa0, opaque=0x21f5b90) at savevm.c:1434 > > vmsd = 0x6f0aa0 =&vmstate_acpi<<<<<<<<<<<<< correct? Correct. > opaque = 0x21f5b90 =&PIIX4PMState > > opaque seems correct as > 0x21f5b90 - 0x21f5ef0 = 0x348 = offsetof(PIIX4PMState, gpe) > > > On my environment the stack trace looks like > (gdb) where > #0 vmstate_save_state (f=0x25d16d0, vmsd=0x695940, opaque=0x283a218) > at savevm.c:1374 > #1 0x00000000004bb4f7 in vmstate_save_state (f=0x25d16d0, vmsd=0x695a40, > opaque=0x2839ed0) at savevm.c:1412 > ... > > (gdb) print&vmstate_gpe > $32 = (const VMStateDescription *) 0x695940 > (gdb) print&vmstate_acpi > $33 = (const VMStateDescription *) 0x695a40 > Are you using qemu-kvm.git or qemu.git? I think there are indeed two issues, your patch fixes the first and there is another that is specific to qemu-kvm.
On Mon, Apr 18, 2011 at 11:22:40AM +0300, Avi Kivity wrote: > Are you using qemu-kvm.git or qemu.git? I think there are indeed two > issues, your patch fixes the first and there is another that is specific > to qemu-kvm. I'm using qemu.git. I've start to have a look at qemu-kvm.git,
diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c index 96f5222..3a8fece 100644 --- a/hw/acpi_piix4.c +++ b/hw/acpi_piix4.c @@ -214,10 +214,9 @@ static int vmstate_acpi_post_load(void *opaque, int version_id) { \ .name = (stringify(_field)), \ .version_id = 0, \ - .num = GPE_LEN, \ .info = &vmstate_info_uint16, \ .size = sizeof(uint16_t), \ - .flags = VMS_ARRAY | VMS_POINTER, \ + .flags = VMS_SINGLE | VMS_POINTER, \ .offset = vmstate_offset_pointer(_state, _field, uint8_t), \ }