Re: [Qemu-ppc] [PATCH 2/2] openpic: convert to vmstate

[Juan Quintela]

Mark Cave-Ayland <address@hidden> wrote:
> Signed-off-by: Mark Cave-Ayland <address@hidden>
> ---
>  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.