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

[Alexander Graf <agraf@suse.de>]

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.
>