diff mbox

[2/2] openpic: convert to vmstate

Message ID 1423233416-25962-3-git-send-email-mark.cave-ayland@ilande.co.uk
State New
Headers show

Commit Message

Mark Cave-Ayland Feb. 6, 2015, 2:36 p.m. UTC
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/intc/openpic.c |  253 +++++++++++++++++++++++++----------------------------
 1 file changed, 119 insertions(+), 134 deletions(-)

Comments

Juan Quintela Feb. 9, 2015, 12:39 p.m. UTC | #1
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/intc/openpic.c |  253 +++++++++++++++++++++++++----------------------------
>  1 file changed, 119 insertions(+), 134 deletions(-)
>

> +static const VMStateDescription vmstate_openpic_irq_queue = {
> +    .name = "openpic_irq_queue",
> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_BITMAP(queue, IRQQueue, 0, queue_size),

Can I assume that this layout is compatible with the one given by:

-    for (i = 0; i < BITS_TO_LONGS(IRQQUEUE_SIZE_BITS); i++) {
-        /* Always put the lower half of a 64-bit long first, in case we
-         * restore on a 32-bit host.  The least significant bits correspond
-         * to lower IRQ numbers in the bitmap.
-         */
-        qemu_put_be32(f, (uint32_t)q->queue[i]);
-#if LONG_MAX > 0x7FFFFFFF
-        qemu_put_be32(f, (uint32_t)(q->queue[i] >> 32));
-#endif
-    }


> +        VMSTATE_INT32(next, IRQQueue),
> +        VMSTATE_INT32(priority, IRQQueue),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static const VMStateDescription vmstate_openpic_irqdest = {
> +    .name = "openpic_irqdest",
> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_INT32(ctpr, IRQDest),
> +        VMSTATE_STRUCT(raised, IRQDest, 0, vmstate_openpic_irq_queue,
> +                       IRQQueue),
> +        VMSTATE_STRUCT(servicing, IRQDest, 0, vmstate_openpic_irq_queue,
> +                       IRQQueue),
> +        VMSTATE_UINT32_ARRAY(outputs_active, IRQDest, OPENPIC_OUTPUT_NB),

This change would make big/little endian migration unhappy. (no, I don't
know if it is more correct the new or the old code, just that they give
different results).

> +        VMSTATE_END_OF_LIST()
> +    }
> +};

> +static const VMStateDescription vmstate_openpic = {
> +    .name = "openpic",
       .version_id = 2,
       .minimum_version_id = 2
> +    .post_load = openpic_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(gcr, OpenPICState),
> +        VMSTATE_UINT32(vir, OpenPICState),
> +        VMSTATE_UINT32(pir, OpenPICState),
> +        VMSTATE_UINT32(spve, OpenPICState),
> +        VMSTATE_UINT32(tfrr, OpenPICState),
> +        VMSTATE_UINT32(max_irq, OpenPICState),

           VMSTATE_UINT32_EQUAL(nb_cpus, OpenPICState),

           VMSTATE_STRUCT_VARRAY_UINT32(dst, OpenPICState, nb_cpus, 0,
                                        vmstate_openpic_irqdest, IRQDest),

           VMSTATE_STRUCT_ARRAY(timers, OpenPICState, OPENPIC_MAX_TMR, 0,
                                vmstate_openpic_timer, OpenPICTimer),

           VMSTATE_STRUCT_VARRAY_UINT32(src, OpenPICState, max_irq, 0,
                                        vmstate_openpic_irqsource, IRQSource),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};

If you do this changes, this should get the v2 format working, right?
Notice the two questions that I asked above, if that is true, we can go
from here making the change compatible.

If so, we could go from here about ading the following ones with a
subsection or so.


> +        VMSTATE_STRUCT_ARRAY(msi, OpenPICState, MAX_MSI, 0,
> +                             vmstate_openpic_msi, OpenPICMSI),
> +        VMSTATE_UINT32(irq_ipi0, OpenPICState),
> +        VMSTATE_UINT32(irq_tim0, OpenPICState),
> +        VMSTATE_UINT32(irq_msi, OpenPICState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};

If this is only used when MSI is used, and there is a check for that, we
could use a new subsection.  If it always used, we should change format
version to three.

Anyways, spliting this patch in two would make clear what is the
equivalent of the old code and what is new.

What do you think?

Later, Juan.
Alexander Graf Feb. 9, 2015, 12:43 p.m. UTC | #2
On 09.02.15 13:39, Juan Quintela wrote:
> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  hw/intc/openpic.c |  253 +++++++++++++++++++++++++----------------------------
>>  1 file changed, 119 insertions(+), 134 deletions(-)
>>
> 
>> +static const VMStateDescription vmstate_openpic_irq_queue = {
>> +    .name = "openpic_irq_queue",
>> +    .version_id = 0,
>> +    .minimum_version_id = 0,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_BITMAP(queue, IRQQueue, 0, queue_size),
> 
> Can I assume that this layout is compatible with the one given by:
> 
> -    for (i = 0; i < BITS_TO_LONGS(IRQQUEUE_SIZE_BITS); i++) {
> -        /* Always put the lower half of a 64-bit long first, in case we
> -         * restore on a 32-bit host.  The least significant bits correspond
> -         * to lower IRQ numbers in the bitmap.
> -         */
> -        qemu_put_be32(f, (uint32_t)q->queue[i]);
> -#if LONG_MAX > 0x7FFFFFFF
> -        qemu_put_be32(f, (uint32_t)(q->queue[i] >> 32));
> -#endif
> -    }
> 
> 
>> +        VMSTATE_INT32(next, IRQQueue),
>> +        VMSTATE_INT32(priority, IRQQueue),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +static const VMStateDescription vmstate_openpic_irqdest = {
>> +    .name = "openpic_irqdest",
>> +    .version_id = 0,
>> +    .minimum_version_id = 0,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_INT32(ctpr, IRQDest),
>> +        VMSTATE_STRUCT(raised, IRQDest, 0, vmstate_openpic_irq_queue,
>> +                       IRQQueue),
>> +        VMSTATE_STRUCT(servicing, IRQDest, 0, vmstate_openpic_irq_queue,
>> +                       IRQQueue),
>> +        VMSTATE_UINT32_ARRAY(outputs_active, IRQDest, OPENPIC_OUTPUT_NB),
> 
> This change would make big/little endian migration unhappy. (no, I don't
> know if it is more correct the new or the old code, just that they give
> different results).
> 
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
> 
>> +static const VMStateDescription vmstate_openpic = {
>> +    .name = "openpic",
>        .version_id = 2,
>        .minimum_version_id = 2
>> +    .post_load = openpic_post_load,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT32(gcr, OpenPICState),
>> +        VMSTATE_UINT32(vir, OpenPICState),
>> +        VMSTATE_UINT32(pir, OpenPICState),
>> +        VMSTATE_UINT32(spve, OpenPICState),
>> +        VMSTATE_UINT32(tfrr, OpenPICState),
>> +        VMSTATE_UINT32(max_irq, OpenPICState),
> 
>            VMSTATE_UINT32_EQUAL(nb_cpus, OpenPICState),
> 
>            VMSTATE_STRUCT_VARRAY_UINT32(dst, OpenPICState, nb_cpus, 0,
>                                         vmstate_openpic_irqdest, IRQDest),
> 
>            VMSTATE_STRUCT_ARRAY(timers, OpenPICState, OPENPIC_MAX_TMR, 0,
>                                 vmstate_openpic_timer, OpenPICTimer),
> 
>            VMSTATE_STRUCT_VARRAY_UINT32(src, OpenPICState, max_irq, 0,
>                                         vmstate_openpic_irqsource, IRQSource),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
> 
> If you do this changes, this should get the v2 format working, right?
> Notice the two questions that I asked above, if that is true, we can go
> from here making the change compatible.
> 
> If so, we could go from here about ading the following ones with a
> subsection or so.
> 
> 
>> +        VMSTATE_STRUCT_ARRAY(msi, OpenPICState, MAX_MSI, 0,
>> +                             vmstate_openpic_msi, OpenPICMSI),
>> +        VMSTATE_UINT32(irq_ipi0, OpenPICState),
>> +        VMSTATE_UINT32(irq_tim0, OpenPICState),
>> +        VMSTATE_UINT32(irq_msi, OpenPICState),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
> 
> If this is only used when MSI is used, and there is a check for that, we
> could use a new subsection.  If it always used, we should change format
> version to three.

Yes, please, just bump the version number and ignore all the legacy
cruft. OpenPIC state saving is broken for years, any old state that we
could potentially save is not usable anyway :)


Alex

> 
> Anyways, spliting this patch in two would make clear what is the
> equivalent of the old code and what is new.
> 
> What do you think?
> 
> Later, Juan.
>
Mark Cave-Ayland Feb. 9, 2015, 1:26 p.m. UTC | #3
On 09/02/15 12:43, Alexander Graf wrote:

Hi Juan,

> On 09.02.15 13:39, Juan Quintela wrote:
>> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>>  hw/intc/openpic.c |  253 +++++++++++++++++++++++++----------------------------
>>>  1 file changed, 119 insertions(+), 134 deletions(-)
>>>
>>
>>> +static const VMStateDescription vmstate_openpic_irq_queue = {
>>> +    .name = "openpic_irq_queue",
>>> +    .version_id = 0,
>>> +    .minimum_version_id = 0,
>>> +    .fields = (VMStateField[]) {
>>> +        VMSTATE_BITMAP(queue, IRQQueue, 0, queue_size),
>>
>> Can I assume that this layout is compatible with the one given by:
>>
>> -    for (i = 0; i < BITS_TO_LONGS(IRQQUEUE_SIZE_BITS); i++) {
>> -        /* Always put the lower half of a 64-bit long first, in case we
>> -         * restore on a 32-bit host.  The least significant bits correspond
>> -         * to lower IRQ numbers in the bitmap.
>> -         */
>> -        qemu_put_be32(f, (uint32_t)q->queue[i]);
>> -#if LONG_MAX > 0x7FFFFFFF
>> -        qemu_put_be32(f, (uint32_t)(q->queue[i] >> 32));
>> -#endif
>> -    }

It won't be over-the-wire migration compatible, however the important
part is that by using VMSTATE_BITMAP (which internally uses longs) then
it should preserve the ability to correctly migrate between a 32-bit and
a 64-bit host as mentioned in the comments.

One minor nit is that VMSTATE_BITMAP uses an in32_t field to specify the
size of the bitmap for each IRQQueue, which in this case is always fixed
because the size of the queue is related to the number of interrupt
pins. As this is likely an edge case, I've just added queue_size to the
IRQQueue struct for the moment even though it is otherwise redundant.

>>> +        VMSTATE_INT32(next, IRQQueue),
>>> +        VMSTATE_INT32(priority, IRQQueue),
>>> +        VMSTATE_END_OF_LIST()
>>> +    }
>>> +};
>>> +
>>> +static const VMStateDescription vmstate_openpic_irqdest = {
>>> +    .name = "openpic_irqdest",
>>> +    .version_id = 0,
>>> +    .minimum_version_id = 0,
>>> +    .fields = (VMStateField[]) {
>>> +        VMSTATE_INT32(ctpr, IRQDest),
>>> +        VMSTATE_STRUCT(raised, IRQDest, 0, vmstate_openpic_irq_queue,
>>> +                       IRQQueue),
>>> +        VMSTATE_STRUCT(servicing, IRQDest, 0, vmstate_openpic_irq_queue,
>>> +                       IRQQueue),
>>> +        VMSTATE_UINT32_ARRAY(outputs_active, IRQDest, OPENPIC_OUTPUT_NB),
>>
>> This change would make big/little endian migration unhappy. (no, I don't
>> know if it is more correct the new or the old code, just that they give
>> different results).

According to the comment in the struct definition, outputs_active is
described as "Count of IRQ sources asserting on non-INT outputs" so this
should be fine transferring between endians moving forwards. I'm not
exactly sure why the old code used the get_buffer/put_buffer routines
for this in the first place, but I'm quite new to this particular
section of code.

>>> +        VMSTATE_END_OF_LIST()
>>> +    }
>>> +};
>>
>>> +static const VMStateDescription vmstate_openpic = {
>>> +    .name = "openpic",
>>        .version_id = 2,
>>        .minimum_version_id = 2
>>> +    .post_load = openpic_post_load,
>>> +    .fields = (VMStateField[]) {
>>> +        VMSTATE_UINT32(gcr, OpenPICState),
>>> +        VMSTATE_UINT32(vir, OpenPICState),
>>> +        VMSTATE_UINT32(pir, OpenPICState),
>>> +        VMSTATE_UINT32(spve, OpenPICState),
>>> +        VMSTATE_UINT32(tfrr, OpenPICState),
>>> +        VMSTATE_UINT32(max_irq, OpenPICState),
>>
>>            VMSTATE_UINT32_EQUAL(nb_cpus, OpenPICState),
>>
>>            VMSTATE_STRUCT_VARRAY_UINT32(dst, OpenPICState, nb_cpus, 0,
>>                                         vmstate_openpic_irqdest, IRQDest),
>>
>>            VMSTATE_STRUCT_ARRAY(timers, OpenPICState, OPENPIC_MAX_TMR, 0,
>>                                 vmstate_openpic_timer, OpenPICTimer),
>>
>>            VMSTATE_STRUCT_VARRAY_UINT32(src, OpenPICState, max_irq, 0,
>>                                         vmstate_openpic_irqsource, IRQSource),
>>> +        VMSTATE_END_OF_LIST()
>>> +    }
>>> +};
>>
>> If you do this changes, this should get the v2 format working, right?
>> Notice the two questions that I asked above, if that is true, we can go
>> from here making the change compatible.
>>
>> If so, we could go from here about ading the following ones with a
>> subsection or so.
>>
>>
>>> +        VMSTATE_STRUCT_ARRAY(msi, OpenPICState, MAX_MSI, 0,
>>> +                             vmstate_openpic_msi, OpenPICMSI),
>>> +        VMSTATE_UINT32(irq_ipi0, OpenPICState),
>>> +        VMSTATE_UINT32(irq_tim0, OpenPICState),
>>> +        VMSTATE_UINT32(irq_msi, OpenPICState),
>>> +        VMSTATE_END_OF_LIST()
>>> +    }
>>> +};
>>
>> If this is only used when MSI is used, and there is a check for that, we
>> could use a new subsection.  If it always used, we should change format
>> version to three.
> 
> Yes, please, just bump the version number and ignore all the legacy
> cruft. OpenPIC state saving is broken for years, any old state that we
> could potentially save is not usable anyway :)

Ha! I did actually do this but I accidentally messed up a rebase just
before sending out the patchset so it got lost. But yes, I agree a
version bump to 3 should be sufficient here.

Just to put this in context: before the PPC loadvm/savevm patchset was
posted to the list, I was bisecting down to around the 0.10-0.11
timeframe to figure out where things broke, and even then the Mac
machines wouldn't run correctly after loadvm. So I agree with Alex that
any snapshots this old would be useless anyway.

Based upon this, are there any other changes you would like before a respin?


ATB,

Mark.
Juan Quintela Feb. 9, 2015, 1:47 p.m. UTC | #4
Alexander Graf <agraf@suse.de> wrote:


> Yes, please, just bump the version number and ignore all the legacy
> cruft. OpenPIC state saving is broken for years, any old state that we
> could potentially save is not usable anyway :)

I didn't knew that bit O:-)

Then you get a;

Reviewed-by: Juan Quintela <quintela@redhat.com>
Juan Quintela Feb. 9, 2015, 1:48 p.m. UTC | #5
Alexander Graf <agraf@suse.de> wrote:
> On 09.02.15 13:39, Juan Quintela wrote:
>> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>>  hw/intc/openpic.c |  253 +++++++++++++++++++++++++----------------------------
>>            VMSTATE_UINT32_EQUAL(nb_cpus, OpenPICState),

This change is a good idea anyways.

Later, Juan.
Juan Quintela Feb. 9, 2015, 1:50 p.m. UTC | #6
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
> On 09/02/15 12:43, Alexander Graf wrote:

> Ha! I did actually do this but I accidentally messed up a rebase just
> before sending out the patchset so it got lost. But yes, I agree a
> version bump to 3 should be sufficient here.
>
> Just to put this in context: before the PPC loadvm/savevm patchset was
> posted to the list, I was bisecting down to around the 0.10-0.11
> timeframe to figure out where things broke, and even then the Mac
> machines wouldn't run correctly after loadvm. So I agree with Alex that
> any snapshots this old would be useless anyway.
>
> Based upon this, are there any other changes you would like before a respin?

I didn't knew that it was completely broken (TM).

As said in a different email, I think that using the _EQUAL() macro for
nb_cpus is a good idea.  For the rest, I agree with it.

Later, Juan.
Mark Cave-Ayland Feb. 9, 2015, 8:05 p.m. UTC | #7
On 09/02/15 13:50, Juan Quintela wrote:

> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
>> On 09/02/15 12:43, Alexander Graf wrote:
> 
>> Ha! I did actually do this but I accidentally messed up a rebase just
>> before sending out the patchset so it got lost. But yes, I agree a
>> version bump to 3 should be sufficient here.
>>
>> Just to put this in context: before the PPC loadvm/savevm patchset was
>> posted to the list, I was bisecting down to around the 0.10-0.11
>> timeframe to figure out where things broke, and even then the Mac
>> machines wouldn't run correctly after loadvm. So I agree with Alex that
>> any snapshots this old would be useless anyway.
>>
>> Based upon this, are there any other changes you would like before a respin?
> 
> I didn't knew that it was completely broken (TM).
> 
> As said in a different email, I think that using the _EQUAL() macro for
> nb_cpus is a good idea.  For the rest, I agree with it.

Great, thanks! I'll make the changes, add your Reviewed-by and then
re-spin out the complete patchset for Alex.


ATB,

Mark.
diff mbox

Patch

diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
index 2a3144f..1ceac97 100644
--- a/hw/intc/openpic.c
+++ b/hw/intc/openpic.c
@@ -207,6 +207,7 @@  typedef enum IRQType {
 
 typedef struct IRQQueue {
     unsigned long *queue;
+    int32_t queue_size; /* Only used for VMSTATE_BITMAP */
     int next;
     int priority;
 } IRQQueue;
@@ -242,6 +243,15 @@  typedef struct IRQSource {
 #define IDR_EP      0x80000000  /* external pin */
 #define IDR_CI      0x40000000  /* critical interrupt */
 
+typedef struct OpenPICTimer {
+    uint32_t tccr;  /* Global timer current count register */
+    uint32_t tbcr;  /* Global timer base count register */
+} OpenPICTimer;
+
+typedef struct OpenPICMSI {
+    uint32_t msir;   /* Shared Message Signaled Interrupt Register */
+} OpenPICMSI;
+
 typedef struct IRQDest {
     int32_t ctpr; /* CPU current task priority */
     IRQQueue raised;
@@ -290,14 +300,9 @@  typedef struct OpenPICState {
     IRQDest dst[MAX_CPU];
     uint32_t nb_cpus;
     /* Timer registers */
-    struct {
-        uint32_t tccr;  /* Global timer current count register */
-        uint32_t tbcr;  /* Global timer base count register */
-    } timers[OPENPIC_MAX_TMR];
+    OpenPICTimer timers[OPENPIC_MAX_TMR];
     /* Shared MSI registers */
-    struct {
-        uint32_t msir;   /* Shared Message Signaled Interrupt Register */
-    } msi[MAX_MSI];
+    OpenPICMSI msi[MAX_MSI];
     uint32_t max_irq;
     uint32_t irq_ipi0;
     uint32_t irq_tim0;
@@ -1289,130 +1294,6 @@  static const MemoryRegionOps openpic_summary_ops_be = {
     },
 };
 
-static void openpic_save_IRQ_queue(QEMUFile* f, IRQQueue *q)
-{
-    unsigned int i;
-
-    for (i = 0; i < BITS_TO_LONGS(IRQQUEUE_SIZE_BITS); i++) {
-        /* Always put the lower half of a 64-bit long first, in case we
-         * restore on a 32-bit host.  The least significant bits correspond
-         * to lower IRQ numbers in the bitmap.
-         */
-        qemu_put_be32(f, (uint32_t)q->queue[i]);
-#if LONG_MAX > 0x7FFFFFFF
-        qemu_put_be32(f, (uint32_t)(q->queue[i] >> 32));
-#endif
-    }
-
-    qemu_put_sbe32s(f, &q->next);
-    qemu_put_sbe32s(f, &q->priority);
-}
-
-static void openpic_save(QEMUFile* f, void *opaque)
-{
-    OpenPICState *opp = (OpenPICState *)opaque;
-    unsigned int i;
-
-    qemu_put_be32s(f, &opp->gcr);
-    qemu_put_be32s(f, &opp->vir);
-    qemu_put_be32s(f, &opp->pir);
-    qemu_put_be32s(f, &opp->spve);
-    qemu_put_be32s(f, &opp->tfrr);
-
-    qemu_put_be32s(f, &opp->nb_cpus);
-
-    for (i = 0; i < opp->nb_cpus; i++) {
-        qemu_put_sbe32s(f, &opp->dst[i].ctpr);
-        openpic_save_IRQ_queue(f, &opp->dst[i].raised);
-        openpic_save_IRQ_queue(f, &opp->dst[i].servicing);
-        qemu_put_buffer(f, (uint8_t *)&opp->dst[i].outputs_active,
-                        sizeof(opp->dst[i].outputs_active));
-    }
-
-    for (i = 0; i < OPENPIC_MAX_TMR; i++) {
-        qemu_put_be32s(f, &opp->timers[i].tccr);
-        qemu_put_be32s(f, &opp->timers[i].tbcr);
-    }
-
-    for (i = 0; i < opp->max_irq; i++) {
-        qemu_put_be32s(f, &opp->src[i].ivpr);
-        qemu_put_be32s(f, &opp->src[i].idr);
-        qemu_put_be32s(f, &opp->src[i].destmask);
-        qemu_put_sbe32s(f, &opp->src[i].last_cpu);
-        qemu_put_sbe32s(f, &opp->src[i].pending);
-    }
-}
-
-static void openpic_load_IRQ_queue(QEMUFile* f, IRQQueue *q)
-{
-    unsigned int i;
-
-    for (i = 0; i < BITS_TO_LONGS(IRQQUEUE_SIZE_BITS); i++) {
-        unsigned long val;
-
-        val = qemu_get_be32(f);
-#if LONG_MAX > 0x7FFFFFFF
-        val <<= 32;
-        val |= qemu_get_be32(f);
-#endif
-
-        q->queue[i] = val;
-    }
-
-    qemu_get_sbe32s(f, &q->next);
-    qemu_get_sbe32s(f, &q->priority);
-}
-
-static int openpic_load(QEMUFile* f, void *opaque, int version_id)
-{
-    OpenPICState *opp = (OpenPICState *)opaque;
-    unsigned int i, nb_cpus;
-
-    if (version_id != 2) {
-        return -EINVAL;
-    }
-
-    qemu_get_be32s(f, &opp->gcr);
-    qemu_get_be32s(f, &opp->vir);
-    qemu_get_be32s(f, &opp->pir);
-    qemu_get_be32s(f, &opp->spve);
-    qemu_get_be32s(f, &opp->tfrr);
-
-    qemu_get_be32s(f, &nb_cpus);
-    if (opp->nb_cpus != nb_cpus) {
-        return -EINVAL;
-    }
-    assert(nb_cpus > 0 && nb_cpus <= MAX_CPU);
-
-    for (i = 0; i < opp->nb_cpus; i++) {
-        qemu_get_sbe32s(f, &opp->dst[i].ctpr);
-        openpic_load_IRQ_queue(f, &opp->dst[i].raised);
-        openpic_load_IRQ_queue(f, &opp->dst[i].servicing);
-        qemu_get_buffer(f, (uint8_t *)&opp->dst[i].outputs_active,
-                        sizeof(opp->dst[i].outputs_active));
-    }
-
-    for (i = 0; i < OPENPIC_MAX_TMR; i++) {
-        qemu_get_be32s(f, &opp->timers[i].tccr);
-        qemu_get_be32s(f, &opp->timers[i].tbcr);
-    }
-
-    for (i = 0; i < opp->max_irq; i++) {
-        uint32_t val;
-
-        val = qemu_get_be32(f);
-        write_IRQreg_ivpr(opp, i, val);
-        val = qemu_get_be32(f);
-        write_IRQreg_idr(opp, i, val);
-
-        qemu_get_be32s(f, &opp->src[i].destmask);
-        qemu_get_sbe32s(f, &opp->src[i].last_cpu);
-        qemu_get_sbe32s(f, &opp->src[i].pending);
-    }
-
-    return 0;
-}
-
 static void openpic_reset(DeviceState *d)
 {
     OpenPICState *opp = OPENPIC(d);
@@ -1527,6 +1408,110 @@  static void map_list(OpenPICState *opp, const MemReg *list, int *count)
     }
 }
 
+static const VMStateDescription vmstate_openpic_irq_queue = {
+    .name = "openpic_irq_queue",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_BITMAP(queue, IRQQueue, 0, queue_size),
+        VMSTATE_INT32(next, IRQQueue),
+        VMSTATE_INT32(priority, IRQQueue),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription vmstate_openpic_irqdest = {
+    .name = "openpic_irqdest",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT32(ctpr, IRQDest),
+        VMSTATE_STRUCT(raised, IRQDest, 0, vmstate_openpic_irq_queue,
+                       IRQQueue),
+        VMSTATE_STRUCT(servicing, IRQDest, 0, vmstate_openpic_irq_queue,
+                       IRQQueue),
+        VMSTATE_UINT32_ARRAY(outputs_active, IRQDest, OPENPIC_OUTPUT_NB),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static int openpic_post_load(void *opaque, int version_id)
+{
+    OpenPICState *opp = (OpenPICState *)opaque;
+    int i;
+
+    /* Update internal ivpr and idr variables */
+    for (i = 0; i < opp->max_irq; i++) {
+        write_IRQreg_idr(opp, i, opp->src[i].idr);
+        write_IRQreg_ivpr(opp, i, opp->src[i].ivpr);
+    }
+
+    return 0;
+}
+
+static const VMStateDescription vmstate_openpic_irqsource = {
+    .name = "openpic_irqsource",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(ivpr, IRQSource),
+        VMSTATE_UINT32(idr, IRQSource),
+        VMSTATE_UINT32(destmask, IRQSource),
+        VMSTATE_INT32(last_cpu, IRQSource),
+        VMSTATE_INT32(pending, IRQSource),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription vmstate_openpic_timer = {
+    .name = "openpic_timer",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(tccr, OpenPICTimer),
+        VMSTATE_UINT32(tbcr, OpenPICTimer),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription vmstate_openpic_msi = {
+    .name = "openpic_msi",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(msir, OpenPICMSI),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription vmstate_openpic = {
+    .name = "openpic",
+    .version_id = 3,
+    .minimum_version_id = 3,
+    .post_load = openpic_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(gcr, OpenPICState),
+        VMSTATE_UINT32(vir, OpenPICState),
+        VMSTATE_UINT32(pir, OpenPICState),
+        VMSTATE_UINT32(spve, OpenPICState),
+        VMSTATE_UINT32(tfrr, OpenPICState),
+        VMSTATE_UINT32(max_irq, OpenPICState),
+        VMSTATE_STRUCT_VARRAY_UINT32(src, OpenPICState, max_irq, 0,
+                                     vmstate_openpic_irqsource, IRQSource),
+        VMSTATE_UINT32(nb_cpus, OpenPICState),
+        VMSTATE_STRUCT_VARRAY_UINT32(dst, OpenPICState, nb_cpus, 0,
+                                     vmstate_openpic_irqdest, IRQDest),
+        VMSTATE_STRUCT_ARRAY(timers, OpenPICState, OPENPIC_MAX_TMR, 0,
+                             vmstate_openpic_timer, OpenPICTimer),
+        VMSTATE_STRUCT_ARRAY(msi, OpenPICState, MAX_MSI, 0,
+                             vmstate_openpic_msi, OpenPICMSI),
+        VMSTATE_UINT32(irq_ipi0, OpenPICState),
+        VMSTATE_UINT32(irq_tim0, OpenPICState),
+        VMSTATE_UINT32(irq_msi, OpenPICState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void openpic_init(Object *obj)
 {
     OpenPICState *opp = OPENPIC(obj);
@@ -1634,13 +1619,12 @@  static void openpic_realize(DeviceState *dev, Error **errp)
             sysbus_init_irq(d, &opp->dst[i].irqs[j]);
         }
 
+        opp->dst[i].raised.queue_size = IRQQUEUE_SIZE_BITS;
         opp->dst[i].raised.queue = bitmap_new(IRQQUEUE_SIZE_BITS);
+        opp->dst[i].servicing.queue_size = IRQQUEUE_SIZE_BITS;
         opp->dst[i].servicing.queue = bitmap_new(IRQQUEUE_SIZE_BITS);
     }
 
-    register_savevm(dev, "openpic", 0, 2,
-                    openpic_save, openpic_load, opp);
-
     sysbus_init_mmio(d, &opp->mem);
     qdev_init_gpio_in(dev, openpic_set_irq, opp->max_irq);
 }
@@ -1658,6 +1642,7 @@  static void openpic_class_init(ObjectClass *oc, void *data)
     dc->realize = openpic_realize;
     dc->props = openpic_properties;
     dc->reset = openpic_reset;
+    dc->vmsd = &vmstate_openpic;
 }
 
 static const TypeInfo openpic_info = {