[Qemu-devel] [PATCH for 4.1?] pl330: fix vmstate description

[Qemu-devel] [PATCH for 4.1?] pl330: fix vmstate description

[Damien Hedde]

Fix the pl330 main and queue vmstate description.
There were missing POINTER flags causing crashes during
incoming migration because:
+ PL330State chan field is a pointer to an array
+ PL330Queue queue field is a pointer to an array

Also bump corresponding vmsd version numbers.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---

I found this while working on reset with xilinx-zynq machine.

I'm not sure what's the vmsd version policy in such cases (for
backward compatibility). I've simply bumped them since migration
was not working anyway (vmstate_load_state was erasing critical part
of PL330State and causing segfaults while loading following fields).

Tested doing migration with the xilinx-zynq-a9 machine.

---
 hw/dma/pl330.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/hw/dma/pl330.c b/hw/dma/pl330.c
index 58df965a46..a56a3e7771 100644
--- a/hw/dma/pl330.c
+++ b/hw/dma/pl330.c
@@ -218,11 +218,12 @@ typedef struct PL330Queue {
 
 static const VMStateDescription vmstate_pl330_queue = {
     .name = "pl330_queue",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]) {
-        VMSTATE_STRUCT_VARRAY_UINT32(queue, PL330Queue, queue_size, 1,
-                                 vmstate_pl330_queue_entry, PL330QueueEntry),
+        VMSTATE_STRUCT_VARRAY_POINTER_UINT32(queue, PL330Queue, queue_size,
+                                             vmstate_pl330_queue_entry,
+                                             PL330QueueEntry),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -278,12 +279,12 @@ struct PL330State {
 
 static const VMStateDescription vmstate_pl330 = {
     .name = "pl330",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]) {
         VMSTATE_STRUCT(manager, PL330State, 0, vmstate_pl330_chan, PL330Chan),
-        VMSTATE_STRUCT_VARRAY_UINT32(chan, PL330State, num_chnls, 0,
-                                     vmstate_pl330_chan, PL330Chan),
+        VMSTATE_STRUCT_VARRAY_POINTER_UINT32(chan, PL330State, num_chnls,
+                                             vmstate_pl330_chan, PL330Chan),
         VMSTATE_VBUFFER_UINT32(lo_seqn, PL330State, 1, NULL, num_chnls),
         VMSTATE_VBUFFER_UINT32(hi_seqn, PL330State, 1, NULL, num_chnls),
         VMSTATE_STRUCT(fifo, PL330State, 0, vmstate_pl330_fifo, PL330Fifo),
-- 
2.22.0

Re: [Qemu-devel] [PATCH for 4.1?] pl330: fix vmstate description

[Philippe Mathieu-Daudé]

On 7/24/19 4:35 PM, Damien Hedde wrote:
> Fix the pl330 main and queue vmstate description.
> There were missing POINTER flags causing crashes during
> incoming migration because:
> + PL330State chan field is a pointer to an array
> + PL330Queue queue field is a pointer to an array
> 
> Also bump corresponding vmsd version numbers.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> 
> I found this while working on reset with xilinx-zynq machine.
> 
> I'm not sure what's the vmsd version policy in such cases (for
> backward compatibility). I've simply bumped them since migration
> was not working anyway (vmstate_load_state was erasing critical part
> of PL330State and causing segfaults while loading following fields).

I still not understand versioning and migration, so I can't say, but
then you use the correct macro, since we have:

    s->chan = g_new0(PL330Chan, s->num_chnls);

So:
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

> 
> Tested doing migration with the xilinx-zynq-a9 machine.
> 
> ---
>  hw/dma/pl330.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/dma/pl330.c b/hw/dma/pl330.c
> index 58df965a46..a56a3e7771 100644
> --- a/hw/dma/pl330.c
> +++ b/hw/dma/pl330.c
> @@ -218,11 +218,12 @@ typedef struct PL330Queue {
>  
>  static const VMStateDescription vmstate_pl330_queue = {
>      .name = "pl330_queue",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>      .fields = (VMStateField[]) {
> -        VMSTATE_STRUCT_VARRAY_UINT32(queue, PL330Queue, queue_size, 1,
> -                                 vmstate_pl330_queue_entry, PL330QueueEntry),
> +        VMSTATE_STRUCT_VARRAY_POINTER_UINT32(queue, PL330Queue, queue_size,
> +                                             vmstate_pl330_queue_entry,
> +                                             PL330QueueEntry),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> @@ -278,12 +279,12 @@ struct PL330State {
>  
>  static const VMStateDescription vmstate_pl330 = {
>      .name = "pl330",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>      .fields = (VMStateField[]) {
>          VMSTATE_STRUCT(manager, PL330State, 0, vmstate_pl330_chan, PL330Chan),
> -        VMSTATE_STRUCT_VARRAY_UINT32(chan, PL330State, num_chnls, 0,
> -                                     vmstate_pl330_chan, PL330Chan),
> +        VMSTATE_STRUCT_VARRAY_POINTER_UINT32(chan, PL330State, num_chnls,
> +                                             vmstate_pl330_chan, PL330Chan),
>          VMSTATE_VBUFFER_UINT32(lo_seqn, PL330State, 1, NULL, num_chnls),
>          VMSTATE_VBUFFER_UINT32(hi_seqn, PL330State, 1, NULL, num_chnls),
>          VMSTATE_STRUCT(fifo, PL330State, 0, vmstate_pl330_fifo, PL330Fifo),
> 

Re: [Qemu-devel] [PATCH for 4.1?] pl330: fix vmstate description

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> On 7/24/19 4:35 PM, Damien Hedde wrote:
> > Fix the pl330 main and queue vmstate description.
> > There were missing POINTER flags causing crashes during
> > incoming migration because:
> > + PL330State chan field is a pointer to an array
> > + PL330Queue queue field is a pointer to an array
> > 
> > Also bump corresponding vmsd version numbers.
> > 
> > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> > ---
> > 
> > I found this while working on reset with xilinx-zynq machine.
> > 
> > I'm not sure what's the vmsd version policy in such cases (for
> > backward compatibility). I've simply bumped them since migration
> > was not working anyway (vmstate_load_state was erasing critical part
> > of PL330State and causing segfaults while loading following fields).
> 
> I still not understand versioning and migration

Incrementing the version (and minimum) is the right thing
to do if you conclude the old one was hopelessly broken.
Migration to and from old qemu breaks, but who cares since it was toast
anyway.
As far as I can tell pl330 is only on our zynq and exynos models
so wont break our versioned 'virt' type.
So from a migration point of view:


Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>


> so I can't say, but
> then you use the correct macro, since we have:
> 
>     s->chan = g_new0(PL330Chan, s->num_chnls);
> 
> So:
> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
> 
> > 
> > Tested doing migration with the xilinx-zynq-a9 machine.
> > 
> > ---
> >  hw/dma/pl330.c | 17 +++++++++--------
> >  1 file changed, 9 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/dma/pl330.c b/hw/dma/pl330.c
> > index 58df965a46..a56a3e7771 100644
> > --- a/hw/dma/pl330.c
> > +++ b/hw/dma/pl330.c
> > @@ -218,11 +218,12 @@ typedef struct PL330Queue {
> >  
> >  static const VMStateDescription vmstate_pl330_queue = {
> >      .name = "pl330_queue",
> > -    .version_id = 1,
> > -    .minimum_version_id = 1,
> > +    .version_id = 2,
> > +    .minimum_version_id = 2,
> >      .fields = (VMStateField[]) {
> > -        VMSTATE_STRUCT_VARRAY_UINT32(queue, PL330Queue, queue_size, 1,
> > -                                 vmstate_pl330_queue_entry, PL330QueueEntry),
> > +        VMSTATE_STRUCT_VARRAY_POINTER_UINT32(queue, PL330Queue, queue_size,
> > +                                             vmstate_pl330_queue_entry,
> > +                                             PL330QueueEntry),
> >          VMSTATE_END_OF_LIST()
> >      }
> >  };
> > @@ -278,12 +279,12 @@ struct PL330State {
> >  
> >  static const VMStateDescription vmstate_pl330 = {
> >      .name = "pl330",
> > -    .version_id = 1,
> > -    .minimum_version_id = 1,
> > +    .version_id = 2,
> > +    .minimum_version_id = 2,
> >      .fields = (VMStateField[]) {
> >          VMSTATE_STRUCT(manager, PL330State, 0, vmstate_pl330_chan, PL330Chan),
> > -        VMSTATE_STRUCT_VARRAY_UINT32(chan, PL330State, num_chnls, 0,
> > -                                     vmstate_pl330_chan, PL330Chan),
> > +        VMSTATE_STRUCT_VARRAY_POINTER_UINT32(chan, PL330State, num_chnls,
> > +                                             vmstate_pl330_chan, PL330Chan),
> >          VMSTATE_VBUFFER_UINT32(lo_seqn, PL330State, 1, NULL, num_chnls),
> >          VMSTATE_VBUFFER_UINT32(hi_seqn, PL330State, 1, NULL, num_chnls),
> >          VMSTATE_STRUCT(fifo, PL330State, 0, vmstate_pl330_fifo, PL330Fifo),
> > 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH for 4.1?] pl330: fix vmstate description

[Damien Hedde]

On 7/24/19 6:38 PM, Dr. David Alan Gilbert wrote:
> * Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
>> On 7/24/19 4:35 PM, Damien Hedde wrote:
>>> Fix the pl330 main and queue vmstate description.
>>> There were missing POINTER flags causing crashes during
>>> incoming migration because:
>>> + PL330State chan field is a pointer to an array
>>> + PL330Queue queue field is a pointer to an array
>>>
>>> Also bump corresponding vmsd version numbers.
>>>
>>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>>> ---
>>>
>>> I found this while working on reset with xilinx-zynq machine.
>>>
>>> I'm not sure what's the vmsd version policy in such cases (for
>>> backward compatibility). I've simply bumped them since migration
>>> was not working anyway (vmstate_load_state was erasing critical part
>>> of PL330State and causing segfaults while loading following fields).
>>
>> I still not understand versioning and migration
> 
> Incrementing the version (and minimum) is the right thing
> to do if you conclude the old one was hopelessly broken.
> Migration to and from old qemu breaks, but who cares since it was toast
> anyway.
> As far as I can tell pl330 is only on our zynq and exynos models
> so wont break our versioned 'virt' type.
> So from a migration point of view:

Since switching from VARRAY to VARRAY_POINTER does not change the size
of what's migrated, it should be possible to accept migration from old
qemu if we can ignore the data in such cases and default to something
(but what ? put the pl330 in reset state ?)

Thanks,
Damien

Re: [Qemu-devel] [PATCH for 4.1?] pl330: fix vmstate description

[Dr. David Alan Gilbert]

* Damien Hedde (damien.hedde@greensocs.com) wrote:
> 
> 
> On 7/24/19 6:38 PM, Dr. David Alan Gilbert wrote:
> > * Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> >> On 7/24/19 4:35 PM, Damien Hedde wrote:
> >>> Fix the pl330 main and queue vmstate description.
> >>> There were missing POINTER flags causing crashes during
> >>> incoming migration because:
> >>> + PL330State chan field is a pointer to an array
> >>> + PL330Queue queue field is a pointer to an array
> >>>
> >>> Also bump corresponding vmsd version numbers.
> >>>
> >>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> >>> ---
> >>>
> >>> I found this while working on reset with xilinx-zynq machine.
> >>>
> >>> I'm not sure what's the vmsd version policy in such cases (for
> >>> backward compatibility). I've simply bumped them since migration
> >>> was not working anyway (vmstate_load_state was erasing critical part
> >>> of PL330State and causing segfaults while loading following fields).
> >>
> >> I still not understand versioning and migration
> > 
> > Incrementing the version (and minimum) is the right thing
> > to do if you conclude the old one was hopelessly broken.
> > Migration to and from old qemu breaks, but who cares since it was toast
> > anyway.
> > As far as I can tell pl330 is only on our zynq and exynos models
> > so wont break our versioned 'virt' type.
> > So from a migration point of view:
> 
> Since switching from VARRAY to VARRAY_POINTER does not change the size
> of what's migrated, it should be possible to accept migration from old
> qemu if we can ignore the data in such cases and default to something
> (but what ? put the pl330 in reset state ?)

I don't think it's worth worrying about doing that unless you need to
preserve migration compatibility - which is less important for
stuff where it's used for dev rather than VMs


Dave

> Thanks,
> Damien
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH for 4.1?] pl330: fix vmstate description

[Peter Maydell]

On Wed, 24 Jul 2019 at 15:36, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> Fix the pl330 main and queue vmstate description.
> There were missing POINTER flags causing crashes during
> incoming migration because:
> + PL330State chan field is a pointer to an array
> + PL330Queue queue field is a pointer to an array
>
> Also bump corresponding vmsd version numbers.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>
> I found this while working on reset with xilinx-zynq machine.
>
> I'm not sure what's the vmsd version policy in such cases (for
> backward compatibility). I've simply bumped them since migration
> was not working anyway (vmstate_load_state was erasing critical part
> of PL330State and causing segfaults while loading following fields).
>
> Tested doing migration with the xilinx-zynq-a9 machine.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

I worked out that we can catch this category of bug by adding
type-checking to the VMSTATE_STRUCT_VARRAY_UINT32 macro and
friends that ensures that the passed in field name is really
an array and not a pointer. This also caught at least one
other bug of the same type...patches to follow later.

thanks
-- PMM

Re: [Qemu-devel] [PATCH for 4.1?] pl330: fix vmstate description

[Peter Maydell]

On Thu, 25 Jul 2019 at 17:14, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 24 Jul 2019 at 15:36, Damien Hedde <damien.hedde@greensocs.com> wrote:
> >
> > Fix the pl330 main and queue vmstate description.
> > There were missing POINTER flags causing crashes during
> > incoming migration because:
> > + PL330State chan field is a pointer to an array
> > + PL330Queue queue field is a pointer to an array
> >
> > Also bump corresponding vmsd version numbers.
> >
> > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> > ---
> >
> > I found this while working on reset with xilinx-zynq machine.
> >
> > I'm not sure what's the vmsd version policy in such cases (for
> > backward compatibility). I've simply bumped them since migration
> > was not working anyway (vmstate_load_state was erasing critical part
> > of PL330State and causing segfaults while loading following fields).
> >
> > Tested doing migration with the xilinx-zynq-a9 machine.
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

and applied to target-arm.next for 4.1.

thanks
-- PMM