Patchwork [24/26] acpi, acpi_piix: factor out GPE logic

login
register
mail settings
Submitter Isaku Yamahata
Date April 17, 2011, 1:50 p.m.
Message ID <20110417135045.GA8741@valinux.co.jp>
Download mbox | patch
Permalink /patch/91558/
State New
Headers show

Comments

Isaku Yamahata - April 17, 2011, 1:50 p.m.
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?
Avi Kivity - April 17, 2011, 3:53 p.m.
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
Isaku Yamahata - April 18, 2011, 7:47 a.m.
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
>
Avi Kivity - April 18, 2011, 8:22 a.m.
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.
Isaku Yamahata - April 18, 2011, 1:45 p.m.
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,

Patch

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),  \
  }