qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] Fix migration uint8 arrys handled
@ 2011-03-15 14:53 Juan Quintela
  2011-03-18 12:41 ` Yoshiaki Tamura
  2011-03-22  0:25 ` Stefan Berger
  0 siblings, 2 replies; 21+ messages in thread
From: Juan Quintela @ 2011-03-15 14:53 UTC (permalink / raw)
  To: qemu-devel; +Cc: aliguori

commit 82fa39b75181b730d6d4d09f443bd26bcfcd045c

only contains half of the fix.  It forgots the save state fix for
UINT8 indexes.

Anthony, please apply, without this migration using hpet is broken.
(only current user).

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 savevm.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/savevm.c b/savevm.c
index 60d2f2a..67459a7 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1395,6 +1395,8 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
                 n_elems = *(int32_t *)(opaque+field->num_offset);
             } else if (field->flags & VMS_VARRAY_UINT16) {
                 n_elems = *(uint16_t *)(opaque+field->num_offset);
+            } else if (field->flags & VMS_VARRAY_UINT8) {
+                n_elems = *(uint8_t *)(opaque+field->num_offset);
             }
             if (field->flags & VMS_POINTER) {
                 base_addr = *(void **)base_addr + field->start;
-- 
1.7.4

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH] Fix migration uint8 arrys handled
  2011-03-15 14:53 [Qemu-devel] [PATCH] Fix migration uint8 arrys handled Juan Quintela
@ 2011-03-18 12:41 ` Yoshiaki Tamura
  2011-03-22  0:25 ` Stefan Berger
  1 sibling, 0 replies; 21+ messages in thread
From: Yoshiaki Tamura @ 2011-03-18 12:41 UTC (permalink / raw)
  To: Juan Quintela; +Cc: aliguori, qemu-devel

2011/3/15 Juan Quintela <quintela@redhat.com>:
> commit 82fa39b75181b730d6d4d09f443bd26bcfcd045c
>
> only contains half of the fix.  It forgots the save state fix for
> UINT8 indexes.
>
> Anthony, please apply, without this migration using hpet is broken.
> (only current user).
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  savevm.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/savevm.c b/savevm.c
> index 60d2f2a..67459a7 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1395,6 +1395,8 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>                 n_elems = *(int32_t *)(opaque+field->num_offset);
>             } else if (field->flags & VMS_VARRAY_UINT16) {
>                 n_elems = *(uint16_t *)(opaque+field->num_offset);
> +            } else if (field->flags & VMS_VARRAY_UINT8) {
> +                n_elems = *(uint8_t *)(opaque+field->num_offset);
>             }
>             if (field->flags & VMS_POINTER) {
>                 base_addr = *(void **)base_addr + field->start;
> --
> 1.7.4

Acked-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH] Fix migration uint8 arrys handled
  2011-03-15 14:53 [Qemu-devel] [PATCH] Fix migration uint8 arrys handled Juan Quintela
  2011-03-18 12:41 ` Yoshiaki Tamura
@ 2011-03-22  0:25 ` Stefan Berger
  2011-03-22  1:46   ` Anthony Liguori
  1 sibling, 1 reply; 21+ messages in thread
From: Stefan Berger @ 2011-03-22  0:25 UTC (permalink / raw)
  To: Juan Quintela; +Cc: aliguori, qemu-devel

On 03/15/2011 10:53 AM, Juan Quintela wrote:
> commit 82fa39b75181b730d6d4d09f443bd26bcfcd045c
>
> only contains half of the fix.  It forgots the save state fix for
> UINT8 indexes.
>
> Anthony, please apply, without this migration using hpet is broken.
> (only current user).
I have just been bisecting the code (from the tip) due to suspend/resume 
problems and it looks like commit 82fa39b7 is introducing the 
suspend/resume problem I am seeing (frozen screen).

    Stefan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH] Fix migration uint8 arrys handled
  2011-03-22  0:25 ` Stefan Berger
@ 2011-03-22  1:46   ` Anthony Liguori
  2011-03-22  9:28     ` Avi Kivity
  0 siblings, 1 reply; 21+ messages in thread
From: Anthony Liguori @ 2011-03-22  1:46 UTC (permalink / raw)
  To: Stefan Berger; +Cc: qemu-devel, Juan Quintela

On 03/21/2011 07:25 PM, Stefan Berger wrote:
> On 03/15/2011 10:53 AM, Juan Quintela wrote:
>> commit 82fa39b75181b730d6d4d09f443bd26bcfcd045c
>>
>> only contains half of the fix.  It forgots the save state fix for
>> UINT8 indexes.
>>
>> Anthony, please apply, without this migration using hpet is broken.
>> (only current user).
> I have just been bisecting the code (from the tip) due to 
> suspend/resume problems and it looks like commit 82fa39b7 is 
> introducing the suspend/resume problem I am seeing (frozen screen).

It's in tip now.

Regards,

Anthony Liguori

>    Stefan
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH] Fix migration uint8 arrys handled
  2011-03-22  1:46   ` Anthony Liguori
@ 2011-03-22  9:28     ` Avi Kivity
  2011-03-22 10:23       ` Stefan Berger
  0 siblings, 1 reply; 21+ messages in thread
From: Avi Kivity @ 2011-03-22  9:28 UTC (permalink / raw)
  To: Anthony Liguori; +Cc: Juan Quintela, qemu-devel, Stefan Berger

On 03/22/2011 03:46 AM, Anthony Liguori wrote:
> On 03/21/2011 07:25 PM, Stefan Berger wrote:
>> On 03/15/2011 10:53 AM, Juan Quintela wrote:
>>> commit 82fa39b75181b730d6d4d09f443bd26bcfcd045c
>>>
>>> only contains half of the fix.  It forgots the save state fix for
>>> UINT8 indexes.
>>>
>>> Anthony, please apply, without this migration using hpet is broken.
>>> (only current user).
>> I have just been bisecting the code (from the tip) due to 
>> suspend/resume problems and it looks like commit 82fa39b7 is 
>> introducing the suspend/resume problem I am seeing (frozen screen).
>
> It's in tip now.

Great, spent some lovely time bisecting and fixing it as well.

-- 
error compiling committee.c: too many arguments to function

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH] Fix migration uint8 arrys handled
  2011-03-22  9:28     ` Avi Kivity
@ 2011-03-22 10:23       ` Stefan Berger
  2011-03-22 10:40         ` Avi Kivity
  0 siblings, 1 reply; 21+ messages in thread
From: Stefan Berger @ 2011-03-22 10:23 UTC (permalink / raw)
  To: Avi Kivity; +Cc: Anthony Liguori, qemu-devel, Juan Quintela

On 03/22/2011 05:28 AM, Avi Kivity wrote:
> On 03/22/2011 03:46 AM, Anthony Liguori wrote:
>> On 03/21/2011 07:25 PM, Stefan Berger wrote:
>>> On 03/15/2011 10:53 AM, Juan Quintela wrote:
>>>> commit 82fa39b75181b730d6d4d09f443bd26bcfcd045c
>>>>
>>>> only contains half of the fix.  It forgots the save state fix for
>>>> UINT8 indexes.
>>>>
>>>> Anthony, please apply, without this migration using hpet is broken.
>>>> (only current user).
>>> I have just been bisecting the code (from the tip) due to 
>>> suspend/resume problems and it looks like commit 82fa39b7 is 
>>> introducing the suspend/resume problem I am seeing (frozen screen).
>>
>> It's in tip now.
>
> Great, spent some lovely time bisecting and fixing it as well.
>
It doesn't work better now than it did before... Trying a suspend/resume 
while in grub leaves me with a black screen upon resume...

    Stefan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH] Fix migration uint8 arrys handled
  2011-03-22 10:23       ` Stefan Berger
@ 2011-03-22 10:40         ` Avi Kivity
  2011-03-22 11:50           ` Supsend/resume regression in c995b4 WAS: " Stefan Berger
  0 siblings, 1 reply; 21+ messages in thread
From: Avi Kivity @ 2011-03-22 10:40 UTC (permalink / raw)
  To: Stefan Berger; +Cc: Anthony Liguori, qemu-devel, Juan Quintela

On 03/22/2011 12:23 PM, Stefan Berger wrote:
> On 03/22/2011 05:28 AM, Avi Kivity wrote:
>> On 03/22/2011 03:46 AM, Anthony Liguori wrote:
>>> On 03/21/2011 07:25 PM, Stefan Berger wrote:
>>>> On 03/15/2011 10:53 AM, Juan Quintela wrote:
>>>>> commit 82fa39b75181b730d6d4d09f443bd26bcfcd045c
>>>>>
>>>>> only contains half of the fix.  It forgots the save state fix for
>>>>> UINT8 indexes.
>>>>>
>>>>> Anthony, please apply, without this migration using hpet is broken.
>>>>> (only current user).
>>>> I have just been bisecting the code (from the tip) due to 
>>>> suspend/resume problems and it looks like commit 82fa39b7 is 
>>>> introducing the suspend/resume problem I am seeing (frozen screen).
>>>
>>> It's in tip now.
>>
>> Great, spent some lovely time bisecting and fixing it as well.
>>
> It doesn't work better now than it did before... Trying a 
> suspend/resume while in grub leaves me with a black screen upon resume...

Well, it fixed it for me (autotest migration tests).

Do you mean that 82fa39b7^ works but b784421ce4c doesn't?  What about 
b784421ce4c^ with 82fa39b7 reverted?  That will rule out some other bug.

-- 
error compiling committee.c: too many arguments to function

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Supsend/resume regression in c995b4 WAS: Re: [Qemu-devel] [PATCH] Fix migration uint8 arrys handled
  2011-03-22 10:40         ` Avi Kivity
@ 2011-03-22 11:50           ` Stefan Berger
  2011-03-22 11:56             ` Stefan Berger
  0 siblings, 1 reply; 21+ messages in thread
From: Stefan Berger @ 2011-03-22 11:50 UTC (permalink / raw)
  To: Avi Kivity; +Cc: Jan Kiszka, Anthony Liguori, qemu-devel, Juan Quintela

On 03/22/2011 06:40 AM, Avi Kivity wrote:
> On 03/22/2011 12:23 PM, Stefan Berger wrote:
>> On 03/22/2011 05:28 AM, Avi Kivity wrote:
>>> On 03/22/2011 03:46 AM, Anthony Liguori wrote:
>>>> On 03/21/2011 07:25 PM, Stefan Berger wrote:
>>>>> On 03/15/2011 10:53 AM, Juan Quintela wrote:
>>>>>> commit 82fa39b75181b730d6d4d09f443bd26bcfcd045c
>>>>>>
>>>>>> only contains half of the fix.  It forgots the save state fix for
>>>>>> UINT8 indexes.
>>>>>>
>>>>>> Anthony, please apply, without this migration using hpet is broken.
>>>>>> (only current user).
>>>>> I have just been bisecting the code (from the tip) due to 
>>>>> suspend/resume problems and it looks like commit 82fa39b7 is 
>>>>> introducing the suspend/resume problem I am seeing (frozen screen).
>>>>
>>>> It's in tip now.
>>>
>>> Great, spent some lovely time bisecting and fixing it as well.
>>>
>> It doesn't work better now than it did before... Trying a 
>> suspend/resume while in grub leaves me with a black screen upon 
>> resume...
>
> Well, it fixed it for me (autotest migration tests).
>
> Do you mean that 82fa39b7^ works but b784421ce4c doesn't?  What about 
> b784421ce4c^ with 82fa39b7 reverted?  That will rule out some other bug.
>
b784421 works for me. It's the tip that is again broken for 
suspend/resume, this time pointing to

c995b495b9d6e60ab1e390bd398a22425d0b3c8c is the first bad commit
commit c995b495b9d6e60ab1e390bd398a22425d0b3c8c
Author: Jan Kiszka <jan.kiszka@siemens.com>
Date:   Tue Mar 15 12:26:22 2011 +0100

     x86: Save/restore PAT MSR

     Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
     Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

diff --git a/target-i386/machine.c b/target-i386/machine.c
index d78eceb..6384f54 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -491,6 +491,8 @@ static const VMStateDescription vmstate_cpu = {
          VMSTATE_UINT64_V(xcr0, CPUState, 12),
          VMSTATE_UINT64_V(xstate_bv, CPUState, 12),
          VMSTATE_YMMH_REGS_VARS(ymmh_regs, CPUState, CPU_NB_REGS, 12),
+
+        VMSTATE_UINT64_V(pat, CPUState, 13),
          VMSTATE_END_OF_LIST()
          /* The above list is not sorted /wrt version numbers, watch 
out! */
      },

Doesn't look bad, but I get a black screen when resuming while in grub.

    Stefan

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: Supsend/resume regression in c995b4 WAS: Re: [Qemu-devel] [PATCH] Fix migration uint8 arrys handled
  2011-03-22 11:50           ` Supsend/resume regression in c995b4 WAS: " Stefan Berger
@ 2011-03-22 11:56             ` Stefan Berger
  2011-03-22 12:00               ` Jan Kiszka
  0 siblings, 1 reply; 21+ messages in thread
From: Stefan Berger @ 2011-03-22 11:56 UTC (permalink / raw)
  To: Avi Kivity; +Cc: Jan Kiszka, Anthony Liguori, qemu-devel, Juan Quintela

On 03/22/2011 07:50 AM, Stefan Berger wrote:
> On 03/22/2011 06:40 AM, Avi Kivity wrote:
>> On 03/22/2011 12:23 PM, Stefan Berger wrote:
>>> On 03/22/2011 05:28 AM, Avi Kivity wrote:
>>>> On 03/22/2011 03:46 AM, Anthony Liguori wrote:
>>>>> On 03/21/2011 07:25 PM, Stefan Berger wrote:
>>>>>> On 03/15/2011 10:53 AM, Juan Quintela wrote:
>>>>>>> commit 82fa39b75181b730d6d4d09f443bd26bcfcd045c
>>>>>>>
>>>>>>> only contains half of the fix.  It forgots the save state fix for
>>>>>>> UINT8 indexes.
>>>>>>>
>>>>>>> Anthony, please apply, without this migration using hpet is broken.
>>>>>>> (only current user).
>>>>>> I have just been bisecting the code (from the tip) due to 
>>>>>> suspend/resume problems and it looks like commit 82fa39b7 is 
>>>>>> introducing the suspend/resume problem I am seeing (frozen screen).
>>>>>
>>>>> It's in tip now.
>>>>
>>>> Great, spent some lovely time bisecting and fixing it as well.
>>>>
>>> It doesn't work better now than it did before... Trying a 
>>> suspend/resume while in grub leaves me with a black screen upon 
>>> resume...
>>
>> Well, it fixed it for me (autotest migration tests).
>>
>> Do you mean that 82fa39b7^ works but b784421ce4c doesn't?  What about 
>> b784421ce4c^ with 82fa39b7 reverted?  That will rule out some other bug.
>>
> b784421 works for me. It's the tip that is again broken for 
> suspend/resume, this time pointing to
>
> c995b495b9d6e60ab1e390bd398a22425d0b3c8c is the first bad commit
> commit c995b495b9d6e60ab1e390bd398a22425d0b3c8c
> Author: Jan Kiszka <jan.kiszka@siemens.com>
> Date:   Tue Mar 15 12:26:22 2011 +0100
>
>     x86: Save/restore PAT MSR
>
>     Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>     Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>
> diff --git a/target-i386/machine.c b/target-i386/machine.c
> index d78eceb..6384f54 100644
> --- a/target-i386/machine.c
> +++ b/target-i386/machine.c
> @@ -491,6 +491,8 @@ static const VMStateDescription vmstate_cpu = {
>          VMSTATE_UINT64_V(xcr0, CPUState, 12),
>          VMSTATE_UINT64_V(xstate_bv, CPUState, 12),
>          VMSTATE_YMMH_REGS_VARS(ymmh_regs, CPUState, CPU_NB_REGS, 12),
> +
> +        VMSTATE_UINT64_V(pat, CPUState, 13),
>          VMSTATE_END_OF_LIST()
>          /* The above list is not sorted /wrt version numbers, watch 
> out! */
>      },
>
> Doesn't look bad, but I get a black screen when resuming while in grub.
>
I think that patch was probably not necessary:

target-i386/machine.c line 452 has this:

         VMSTATE_UINT64_V(pat, CPUState, 5),

then again in line 495:

         VMSTATE_UINT64_V(pat, CPUState, 13),

    Stefan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Supsend/resume regression in c995b4 WAS: Re: [Qemu-devel] [PATCH] Fix migration uint8 arrys handled
  2011-03-22 11:56             ` Stefan Berger
@ 2011-03-22 12:00               ` Jan Kiszka
  2011-03-22 12:21                 ` Avi Kivity
                                   ` (2 more replies)
  0 siblings, 3 replies; 21+ messages in thread
From: Jan Kiszka @ 2011-03-22 12:00 UTC (permalink / raw)
  To: Stefan Berger
  Cc: Anthony Liguori, Juan Quintela, Avi Kivity, qemu-devel@nongnu.org

On 2011-03-22 12:56, Stefan Berger wrote:
> On 03/22/2011 07:50 AM, Stefan Berger wrote:
>> On 03/22/2011 06:40 AM, Avi Kivity wrote:
>>> On 03/22/2011 12:23 PM, Stefan Berger wrote:
>>>> On 03/22/2011 05:28 AM, Avi Kivity wrote:
>>>>> On 03/22/2011 03:46 AM, Anthony Liguori wrote:
>>>>>> On 03/21/2011 07:25 PM, Stefan Berger wrote:
>>>>>>> On 03/15/2011 10:53 AM, Juan Quintela wrote:
>>>>>>>> commit 82fa39b75181b730d6d4d09f443bd26bcfcd045c
>>>>>>>>
>>>>>>>> only contains half of the fix.  It forgots the save state fix for
>>>>>>>> UINT8 indexes.
>>>>>>>>
>>>>>>>> Anthony, please apply, without this migration using hpet is broken.
>>>>>>>> (only current user).
>>>>>>> I have just been bisecting the code (from the tip) due to 
>>>>>>> suspend/resume problems and it looks like commit 82fa39b7 is 
>>>>>>> introducing the suspend/resume problem I am seeing (frozen screen).
>>>>>>
>>>>>> It's in tip now.
>>>>>
>>>>> Great, spent some lovely time bisecting and fixing it as well.
>>>>>
>>>> It doesn't work better now than it did before... Trying a 
>>>> suspend/resume while in grub leaves me with a black screen upon 
>>>> resume...
>>>
>>> Well, it fixed it for me (autotest migration tests).
>>>
>>> Do you mean that 82fa39b7^ works but b784421ce4c doesn't?  What about 
>>> b784421ce4c^ with 82fa39b7 reverted?  That will rule out some other bug.
>>>
>> b784421 works for me. It's the tip that is again broken for 
>> suspend/resume, this time pointing to
>>
>> c995b495b9d6e60ab1e390bd398a22425d0b3c8c is the first bad commit
>> commit c995b495b9d6e60ab1e390bd398a22425d0b3c8c
>> Author: Jan Kiszka <jan.kiszka@siemens.com>
>> Date:   Tue Mar 15 12:26:22 2011 +0100
>>
>>     x86: Save/restore PAT MSR
>>
>>     Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>     Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>>
>> diff --git a/target-i386/machine.c b/target-i386/machine.c
>> index d78eceb..6384f54 100644
>> --- a/target-i386/machine.c
>> +++ b/target-i386/machine.c
>> @@ -491,6 +491,8 @@ static const VMStateDescription vmstate_cpu = {
>>          VMSTATE_UINT64_V(xcr0, CPUState, 12),
>>          VMSTATE_UINT64_V(xstate_bv, CPUState, 12),
>>          VMSTATE_YMMH_REGS_VARS(ymmh_regs, CPUState, CPU_NB_REGS, 12),
>> +
>> +        VMSTATE_UINT64_V(pat, CPUState, 13),
>>          VMSTATE_END_OF_LIST()
>>          /* The above list is not sorted /wrt version numbers, watch 
>> out! */
>>      },
>>
>> Doesn't look bad, but I get a black screen when resuming while in grub.
>>
> I think that patch was probably not necessary:
> 
> target-i386/machine.c line 452 has this:
> 
>          VMSTATE_UINT64_V(pat, CPUState, 5),
> 
> then again in line 495:
> 
>          VMSTATE_UINT64_V(pat, CPUState, 13),
> 

Ouch, indeed. Moreover, CPU_SAVE_VERSION was not updated (likely the
reason for the breakage). Thanks for debugging this!

Anthony (or whoever), please revert this unneeded commit in qemu.git.

We had a few migration related regressions recently. Do we have
sufficient test cases in autotest for them? Also for migrating from
older to the latest version?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Supsend/resume regression in c995b4 WAS: Re: [Qemu-devel] [PATCH] Fix migration uint8 arrys handled
  2011-03-22 12:00               ` Jan Kiszka
@ 2011-03-22 12:21                 ` Avi Kivity
  2011-03-22 12:30                   ` Jan Kiszka
  2011-03-22 12:33                 ` [Qemu-devel] Re: Supsend/resume regression in c995b4 WAS: " Juan Quintela
  2011-03-22 13:26                 ` Supsend/resume regression in c995b4 WAS: Re: [Qemu-devel] " Anthony Liguori
  2 siblings, 1 reply; 21+ messages in thread
From: Avi Kivity @ 2011-03-22 12:21 UTC (permalink / raw)
  To: Jan Kiszka
  Cc: Anthony Liguori, Juan Quintela, qemu-devel@nongnu.org,
	Stefan Berger

On 03/22/2011 02:00 PM, Jan Kiszka wrote:
> We had a few migration related regressions recently. Do we have
> sufficient test cases in autotest for them? Also for migrating from
> older to the latest version?
>

Autotest did catch the uint8 varray thing.  c995b4 isn't yet in 
qemu-kvm.git, so it wasn't autotested by me (Lucas does autotest 
upstream qemu.git, not sure if it was tested yet).

-- 
error compiling committee.c: too many arguments to function

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Supsend/resume regression in c995b4 WAS: Re: [Qemu-devel] [PATCH] Fix migration uint8 arrys handled
  2011-03-22 12:21                 ` Avi Kivity
@ 2011-03-22 12:30                   ` Jan Kiszka
  2011-03-22 12:35                     ` Avi Kivity
  0 siblings, 1 reply; 21+ messages in thread
From: Jan Kiszka @ 2011-03-22 12:30 UTC (permalink / raw)
  To: Avi Kivity
  Cc: Anthony Liguori, Juan Quintela, qemu-devel@nongnu.org,
	Stefan Berger

On 2011-03-22 13:21, Avi Kivity wrote:
> On 03/22/2011 02:00 PM, Jan Kiszka wrote:
>> We had a few migration related regressions recently. Do we have
>> sufficient test cases in autotest for them? Also for migrating from
>> older to the latest version?
>>
> 
> Autotest did catch the uint8 varray thing.  c995b4 isn't yet in 
> qemu-kvm.git, so it wasn't autotested by me (Lucas does autotest 
> upstream qemu.git, not sure if it was tested yet).

Upstream was and current is qemu-kvm.git is broken /wrt migrating
i8254[-kvm] from older versions. We also had some issues around vmmouse.
Therefore my question regarding a forward migration tests.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [Qemu-devel] Re: Supsend/resume regression in c995b4 WAS: Re: [PATCH] Fix migration uint8 arrys handled
  2011-03-22 12:00               ` Jan Kiszka
  2011-03-22 12:21                 ` Avi Kivity
@ 2011-03-22 12:33                 ` Juan Quintela
  2011-03-22 12:39                   ` Jan Kiszka
  2011-03-22 13:26                 ` Supsend/resume regression in c995b4 WAS: Re: [Qemu-devel] " Anthony Liguori
  2 siblings, 1 reply; 21+ messages in thread
From: Juan Quintela @ 2011-03-22 12:33 UTC (permalink / raw)
  To: Jan Kiszka
  Cc: qemu-devel@nongnu.org, Anthony Liguori, Avi Kivity, Stefan Berger

Jan Kiszka <jan.kiszka@siemens.com> wrote:

>> 
>>          VMSTATE_UINT64_V(pat, CPUState, 13),
>> 
>
> Ouch, indeed. Moreover, CPU_SAVE_VERSION was not updated (likely the
> reason for the breakage). Thanks for debugging this!
>
> Anthony (or whoever), please revert this unneeded commit in qemu.git.
>
> We had a few migration related regressions recently. Do we have
> sufficient test cases in autotest for them? Also for migrating from
> older to the latest version?

The UINT8 problem was my "brown paper bug" fault.  About autotest, it
"learnt" to use two machines to test migration a couple of months ago.
Haven't had the time to write test for it thought.

Later, Juan.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Supsend/resume regression in c995b4 WAS: Re: [Qemu-devel] [PATCH] Fix migration uint8 arrys handled
  2011-03-22 12:30                   ` Jan Kiszka
@ 2011-03-22 12:35                     ` Avi Kivity
  0 siblings, 0 replies; 21+ messages in thread
From: Avi Kivity @ 2011-03-22 12:35 UTC (permalink / raw)
  To: Jan Kiszka
  Cc: Anthony Liguori, Stefan Berger, qemu-devel@nongnu.org,
	Juan Quintela

On 03/22/2011 02:30 PM, Jan Kiszka wrote:
> On 2011-03-22 13:21, Avi Kivity wrote:
> >  On 03/22/2011 02:00 PM, Jan Kiszka wrote:
> >>  We had a few migration related regressions recently. Do we have
> >>  sufficient test cases in autotest for them? Also for migrating from
> >>  older to the latest version?
> >>
> >
> >  Autotest did catch the uint8 varray thing.  c995b4 isn't yet in
> >  qemu-kvm.git, so it wasn't autotested by me (Lucas does autotest
> >  upstream qemu.git, not sure if it was tested yet).
>
> Upstream was and current is qemu-kvm.git is broken /wrt migrating
> i8254[-kvm] from older versions. We also had some issues around vmmouse.
> Therefore my question regarding a forward migration tests.

Yeah, forgot to reply to that.  I don't test forward migrations and I 
don't think autotest has the infrastructure for it.

-- 
error compiling committee.c: too many arguments to function

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [Qemu-devel] Re: Supsend/resume regression in c995b4 WAS: Re: [PATCH] Fix migration uint8 arrys handled
  2011-03-22 12:33                 ` [Qemu-devel] Re: Supsend/resume regression in c995b4 WAS: " Juan Quintela
@ 2011-03-22 12:39                   ` Jan Kiszka
  0 siblings, 0 replies; 21+ messages in thread
From: Jan Kiszka @ 2011-03-22 12:39 UTC (permalink / raw)
  To: quintela@redhat.com
  Cc: qemu-devel@nongnu.org, Anthony Liguori, Avi Kivity, Stefan Berger

On 2011-03-22 13:33, Juan Quintela wrote:
> Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
>>>
>>>          VMSTATE_UINT64_V(pat, CPUState, 13),
>>>
>>
>> Ouch, indeed. Moreover, CPU_SAVE_VERSION was not updated (likely the
>> reason for the breakage). Thanks for debugging this!
>>
>> Anthony (or whoever), please revert this unneeded commit in qemu.git.
>>
>> We had a few migration related regressions recently. Do we have
>> sufficient test cases in autotest for them? Also for migrating from
>> older to the latest version?
> 
> The UINT8 problem was my "brown paper bug" fault.  About autotest, it
> "learnt" to use two machines to test migration a couple of months ago.
> Haven't had the time to write test for it thought.

A basic migrate to file / resume from file on the same host would
already suffice in fact.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Supsend/resume regression in c995b4 WAS: Re: [Qemu-devel] [PATCH] Fix migration uint8 arrys handled
  2011-03-22 12:00               ` Jan Kiszka
  2011-03-22 12:21                 ` Avi Kivity
  2011-03-22 12:33                 ` [Qemu-devel] Re: Supsend/resume regression in c995b4 WAS: " Juan Quintela
@ 2011-03-22 13:26                 ` Anthony Liguori
  2011-03-22 13:55                   ` [Qemu-devel] Re: Supsend/resume regression in c995b4 WAS: " Juan Quintela
  2011-03-23  9:10                   ` Supsend/resume regression in c995b4 WAS: Re: [Qemu-devel] " Avi Kivity
  2 siblings, 2 replies; 21+ messages in thread
From: Anthony Liguori @ 2011-03-22 13:26 UTC (permalink / raw)
  To: Jan Kiszka
  Cc: qemu-devel@nongnu.org, Juan Quintela, Avi Kivity, Stefan Berger

[-- Attachment #1: Type: text/plain, Size: 984 bytes --]

On 03/22/2011 07:00 AM, Jan Kiszka wrote:
> We had a few migration related regressions recently. Do we have
> sufficient test cases in autotest for them? Also for migrating from
> older to the latest version?

Autotest is too late and also not nearly rigorous enough for what you're 
trying to catch.

Here's how I propose we tackle this.  This patch adds a -dump-savevm 
option that takes a version.  It spits out all of the fields we save for 
a particular version (well, not really, but it should).  We also can add 
type information.  The idea is that we'd write a simple test case (using 
gtester) that ran through and dumped the schema for each version.  We'd 
store the schema's in the tree and the test can compare old schema's to 
the current schema to check for failure.

This was thrown together in just a few minutes.  I'll try to put 
together something more complete later today but I wanted to share this 
before the call at least.

Regards,

Anthony Liguori

> Jan
>


[-- Attachment #2: 0001-vl-add-dump-savevm-option-to-display-VMState-schema.patch --]
[-- Type: text/x-patch, Size: 3501 bytes --]

>From 43fb56c4ee68905d886ea21dad338f3e76350ba5 Mon Sep 17 00:00:00 2001
From: Anthony Liguori <aliguori@us.ibm.com>
Date: Tue, 22 Mar 2011 08:21:00 -0500
Subject: [PATCH] vl: add -dump-savevm option to display VMState schema

This can be used to verify whether the savevm schema has changed for a given
version of the migration protocol.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/hw.h         |    2 ++
 qemu-options.hx |    9 +++++++++
 savevm.c        |   22 ++++++++++++++++++++++
 vl.c            |    9 +++++++++
 4 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/hw/hw.h b/hw/hw.h
index 1b09039..55816da 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -914,4 +914,6 @@ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
                                    int required_for_version);
 void vmstate_unregister(DeviceState *dev, const VMStateDescription *vmsd,
                         void *opaque);
+void vmstate_dump(FILE *f, int version);
+
 #endif
diff --git a/qemu-options.hx b/qemu-options.hx
index badb730..801757b 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2349,6 +2349,15 @@ Specify a trace file to log output traces to.
 ETEXI
 #endif
 
+DEF("dump-savevm", HAS_ARG, QEMU_OPTION_dump_savevm,
+    "-dump-savevm VERSION\n"
+    "                dump the savevm schema for a given version\n",
+    QEMU_ARCH_ALL)
+STEXI
+@item -dump-savevm
+@findex -dump-savevm
+ETEXI
+
 HXCOMM This is the last statement. Insert new options before this line!
 STEXI
 @end table
diff --git a/savevm.c b/savevm.c
index 03fce62..da1fea1 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1436,6 +1436,28 @@ static void vmstate_save(QEMUFile *f, SaveStateEntry *se)
     vmstate_save_state(f,se->vmsd, se->opaque);
 }
 
+void vmstate_dump(FILE *f, int version)
+{
+    SaveStateEntry *se;
+    bool first = true;
+
+    QTAILQ_FOREACH(se, &savevm_handlers, entry) {
+        VMStateField *field;
+        if (!se->vmsd) {
+            continue;
+        }
+        if (first) {
+            first = false;
+        } else {
+            fprintf(f, "\n");
+        }
+        fprintf(f, "%s.__version__ = %d\n", se->vmsd->name, se->version_id);
+        for (field = se->vmsd->fields; field->name; field++) {
+            fprintf(f, "%s.%s\n", se->vmsd->name, field->name);
+        }
+    }
+}
+
 #define QEMU_VM_FILE_MAGIC           0x5145564d
 #define QEMU_VM_FILE_VERSION_COMPAT  0x00000002
 #define QEMU_VM_FILE_VERSION         0x00000003
diff --git a/vl.c b/vl.c
index ac47211..361fcef 100644
--- a/vl.c
+++ b/vl.c
@@ -1941,6 +1941,7 @@ int main(int argc, char **argv, char **envp)
     int show_vnc_port = 0;
     int defconfig = 1;
     const char *trace_file = NULL;
+    int dump_savevm = -1;
 
     atexit(qemu_run_exit_notifiers);
     error_set_progname(argv[0]);
@@ -2760,6 +2761,9 @@ int main(int argc, char **argv, char **envp)
                     fclose(fp);
                     break;
                 }
+            case QEMU_OPTION_dump_savevm:
+                dump_savevm = atoi(optarg);
+                break;
             default:
                 os_parse_cmd_args(popt->index, optarg);
             }
@@ -3013,6 +3017,11 @@ int main(int argc, char **argv, char **envp)
 
     cpu_synchronize_all_post_init();
 
+    if (dump_savevm != -1) {
+        vmstate_dump(stdout, dump_savevm);
+        exit(0);
+    }
+
     /* must be after terminal init, SDL library changes signal handlers */
     os_setup_signal_handling();
 
-- 
1.7.0.4


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Qemu-devel] Re: Supsend/resume regression in c995b4 WAS: Re: [PATCH] Fix migration uint8 arrys handled
  2011-03-22 13:26                 ` Supsend/resume regression in c995b4 WAS: Re: [Qemu-devel] " Anthony Liguori
@ 2011-03-22 13:55                   ` Juan Quintela
  2011-03-22 13:59                     ` Anthony Liguori
  2011-03-23  9:10                   ` Supsend/resume regression in c995b4 WAS: Re: [Qemu-devel] " Avi Kivity
  1 sibling, 1 reply; 21+ messages in thread
From: Juan Quintela @ 2011-03-22 13:55 UTC (permalink / raw)
  To: Anthony Liguori
  Cc: qemu-devel@nongnu.org, Jan Kiszka, Avi Kivity, Stefan Berger

Anthony Liguori <aliguori@us.ibm.com> wrote:
> On 03/22/2011 07:00 AM, Jan Kiszka wrote:
>> We had a few migration related regressions recently. Do we have
>> sufficient test cases in autotest for them? Also for migrating from
>> older to the latest version?
>
> Autotest is too late and also not nearly rigorous enough for what
> you're trying to catch.
>
> Here's how I propose we tackle this.  This patch adds a -dump-savevm
> option that takes a version.  It spits out all of the fields we save
> for a particular version (well, not really, but it should).  We also
> can add type information.  The idea is that we'd write a simple test
> case (using gtester) that ran through and dumped the schema for each
> version.  We'd store the schema's in the tree and the test can compare
> old schema's to the current schema to check for failure.
>
> This was thrown together in just a few minutes.  I'll try to put
> together something more complete later today but I wanted to share
> this before the call at least.

This would be an start, althought I still think that a way to dump a
single device, and a way to dump the state of a device in a specific
version is needed.  Information as:
- is this always saved
- size of arrays
- ....

that is there is not saved.

Later, Juan.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [Qemu-devel] Re: Supsend/resume regression in c995b4 WAS: Re: [PATCH] Fix migration uint8 arrys handled
  2011-03-22 13:55                   ` [Qemu-devel] Re: Supsend/resume regression in c995b4 WAS: " Juan Quintela
@ 2011-03-22 13:59                     ` Anthony Liguori
  0 siblings, 0 replies; 21+ messages in thread
From: Anthony Liguori @ 2011-03-22 13:59 UTC (permalink / raw)
  To: quintela; +Cc: qemu-devel@nongnu.org, Jan Kiszka, Avi Kivity, Stefan Berger

On 03/22/2011 08:55 AM, Juan Quintela wrote:
> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>> On 03/22/2011 07:00 AM, Jan Kiszka wrote:
>>> We had a few migration related regressions recently. Do we have
>>> sufficient test cases in autotest for them? Also for migrating from
>>> older to the latest version?
>> Autotest is too late and also not nearly rigorous enough for what
>> you're trying to catch.
>>
>> Here's how I propose we tackle this.  This patch adds a -dump-savevm
>> option that takes a version.  It spits out all of the fields we save
>> for a particular version (well, not really, but it should).  We also
>> can add type information.  The idea is that we'd write a simple test
>> case (using gtester) that ran through and dumped the schema for each
>> version.  We'd store the schema's in the tree and the test can compare
>> old schema's to the current schema to check for failure.
>>
>> This was thrown together in just a few minutes.  I'll try to put
>> together something more complete later today but I wanted to share
>> this before the call at least.
> This would be an start, althought I still think that a way to dump a
> single device, and a way to dump the state of a device in a specific
> version is needed.  Information as:
> - is this always saved
> - size of arrays
> - ....
>
> that is there is not saved.

Yeah, we can add that down the road though.  With just something as 
simple as this, we can catch quite a few regressions.

Regards,

Anthony Liguori

> Later, Juan.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Supsend/resume regression in c995b4 WAS: Re: [Qemu-devel] [PATCH] Fix migration uint8 arrys handled
  2011-03-22 13:26                 ` Supsend/resume regression in c995b4 WAS: Re: [Qemu-devel] " Anthony Liguori
  2011-03-22 13:55                   ` [Qemu-devel] Re: Supsend/resume regression in c995b4 WAS: " Juan Quintela
@ 2011-03-23  9:10                   ` Avi Kivity
  2011-03-23 11:22                     ` Yoshiaki Tamura
  2011-03-23 12:15                     ` Anthony Liguori
  1 sibling, 2 replies; 21+ messages in thread
From: Avi Kivity @ 2011-03-23  9:10 UTC (permalink / raw)
  To: Anthony Liguori
  Cc: Jan Kiszka, Juan Quintela, qemu-devel@nongnu.org, Stefan Berger

On 03/22/2011 03:26 PM, Anthony Liguori wrote:
>
> Here's how I propose we tackle this.  This patch adds a -dump-savevm 
> option that takes a version.  It spits out all of the fields we save 
> for a particular version (well, not really, but it should).  We also 
> can add type information.  The idea is that we'd write a simple test 
> case (using gtester) that ran through and dumped the schema for each 
> version.  We'd store the schema's in the tree and the test can compare 
> old schema's to the current schema to check for failure.
>

Instead of generating the schema and comparing, what about the other way 
round?  Write vmstate in a formal schema, and generate the code at runtime.

-- 
error compiling committee.c: too many arguments to function

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Supsend/resume regression in c995b4 WAS: Re: [Qemu-devel] [PATCH] Fix migration uint8 arrys handled
  2011-03-23  9:10                   ` Supsend/resume regression in c995b4 WAS: Re: [Qemu-devel] " Avi Kivity
@ 2011-03-23 11:22                     ` Yoshiaki Tamura
  2011-03-23 12:15                     ` Anthony Liguori
  1 sibling, 0 replies; 21+ messages in thread
From: Yoshiaki Tamura @ 2011-03-23 11:22 UTC (permalink / raw)
  To: Avi Kivity
  Cc: Jan Kiszka, Anthony Liguori, Stefan Berger, qemu-devel@nongnu.org,
	Juan Quintela

2011/3/23 Avi Kivity <avi@redhat.com>:
> On 03/22/2011 03:26 PM, Anthony Liguori wrote:
>>
>> Here's how I propose we tackle this.  This patch adds a -dump-savevm
>> option that takes a version.  It spits out all of the fields we save for a
>> particular version (well, not really, but it should).  We also can add type
>> information.  The idea is that we'd write a simple test case (using gtester)
>> that ran through and dumped the schema for each version.  We'd store the
>> schema's in the tree and the test can compare old schema's to the current
>> schema to check for failure.
>>
>
> Instead of generating the schema and comparing, what about the other way
> round?  Write vmstate in a formal schema, and generate the code at runtime.

I agree :)

Yoshi

>
> --
> error compiling committee.c: too many arguments to function
>
>
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Supsend/resume regression in c995b4 WAS: Re: [Qemu-devel] [PATCH] Fix migration uint8 arrys handled
  2011-03-23  9:10                   ` Supsend/resume regression in c995b4 WAS: Re: [Qemu-devel] " Avi Kivity
  2011-03-23 11:22                     ` Yoshiaki Tamura
@ 2011-03-23 12:15                     ` Anthony Liguori
  1 sibling, 0 replies; 21+ messages in thread
From: Anthony Liguori @ 2011-03-23 12:15 UTC (permalink / raw)
  To: Avi Kivity
  Cc: Jan Kiszka, Anthony Liguori, Stefan Berger, qemu-devel@nongnu.org,
	Juan Quintela

On 03/23/2011 04:10 AM, Avi Kivity wrote:
> On 03/22/2011 03:26 PM, Anthony Liguori wrote:
>>
>> Here's how I propose we tackle this.  This patch adds a -dump-savevm 
>> option that takes a version.  It spits out all of the fields we save 
>> for a particular version (well, not really, but it should).  We also 
>> can add type information.  The idea is that we'd write a simple test 
>> case (using gtester) that ran through and dumped the schema for each 
>> version.  We'd store the schema's in the tree and the test can 
>> compare old schema's to the current schema to check for failure.
>>
>
> Instead of generating the schema and comparing, what about the other 
> way round?  Write vmstate in a formal schema, and generate the code at 
> runtime.

This is exactly where I want to go in the future.

Regards,

Anthony Liguori

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2011-03-23 12:16 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-03-15 14:53 [Qemu-devel] [PATCH] Fix migration uint8 arrys handled Juan Quintela
2011-03-18 12:41 ` Yoshiaki Tamura
2011-03-22  0:25 ` Stefan Berger
2011-03-22  1:46   ` Anthony Liguori
2011-03-22  9:28     ` Avi Kivity
2011-03-22 10:23       ` Stefan Berger
2011-03-22 10:40         ` Avi Kivity
2011-03-22 11:50           ` Supsend/resume regression in c995b4 WAS: " Stefan Berger
2011-03-22 11:56             ` Stefan Berger
2011-03-22 12:00               ` Jan Kiszka
2011-03-22 12:21                 ` Avi Kivity
2011-03-22 12:30                   ` Jan Kiszka
2011-03-22 12:35                     ` Avi Kivity
2011-03-22 12:33                 ` [Qemu-devel] Re: Supsend/resume regression in c995b4 WAS: " Juan Quintela
2011-03-22 12:39                   ` Jan Kiszka
2011-03-22 13:26                 ` Supsend/resume regression in c995b4 WAS: Re: [Qemu-devel] " Anthony Liguori
2011-03-22 13:55                   ` [Qemu-devel] Re: Supsend/resume regression in c995b4 WAS: " Juan Quintela
2011-03-22 13:59                     ` Anthony Liguori
2011-03-23  9:10                   ` Supsend/resume regression in c995b4 WAS: Re: [Qemu-devel] " Avi Kivity
2011-03-23 11:22                     ` Yoshiaki Tamura
2011-03-23 12:15                     ` Anthony Liguori

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).