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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* 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