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

[Mark Cave-Ayland]

On 09/02/15 12:43, Alexander Graf wrote:

Hi Juan,

> On 09.02.15 13:39, Juan Quintela wrote:
>> 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
>> -    }

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.