Re: [Qemu-devel] [RFC PATCH 2/2] target-ppc: add CPU IRQ state to PPC VMStateDescription

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 09/16/2015 07:09 AM, Mark Cave-Ayland wrote:
> On 15/09/15 00:10, Alexey Kardashevskiy wrote:
>
>> On 09/15/2015 05:30 AM, Mark Cave-Ayland wrote:
>>> Commit a90db15 "target-ppc: Convert ppc cpu savevm to VMStateDescription"
>>> appears to drop the internal CPU IRQ state from the migration stream.
>>> Whilst
>>> testing migration on g3beige/mac99 machines, test images would
>>> randomly fail to
>>> resume unless a key was pressed on the VGA console.
>>>
>>> Further investigation suggests that internal CPU IRQ state isn't being
>>> preserved and so interrupts asserted at the time of migration are
>>> lost. Adding
>>> the pending_interrupts and irq_input_state fields back into the migration
>>> stream appears to fix the problem here during local tests.
>>
>> On spapr, interrupt state migrates with XICS interrupt controller and it
>> resets the CPU bits you are adding to the migration descriptor. I'd
>> expect openpic (this one is used for mac99?) to do the same.
>
> Interesting. I wrote the patch that converted openpic to
> VMStateDescription at the end of last year, and my understanding from
> the feedback was that ideally interrupt state should be maintained so
> that no post_load function was required. I guess spapr is very different
> from the basic Mac machines though.
>
> Also I see that you also removed the reference to cpu_write_xer() which
> appears to set some related internal state variables. Is this now not
> necessary either?


Not sure here, looks like a bug actually, cpu_post_load() should call it. 
But it should only affect TCG migration (which we have not extensively 
tested :) ).



>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>>    target-ppc/machine.c |    2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
>>> index bd99844..968a7d6 100644
>>> --- a/target-ppc/machine.c
>>> +++ b/target-ppc/machine.c
>>> @@ -528,6 +528,8 @@ const VMStateDescription vmstate_ppc_cpu = {
>>>
>>>            /* Internal state */
>>>            VMSTATE_UINTTL(env.hflags_nmsr, PowerPCCPU),
>>> +        VMSTATE_UINT32(env.pending_interrupts, PowerPCCPU),
>>> +        VMSTATE_UINT32(env.irq_input_state, PowerPCCPU),
>>
>> This update requires a "version" increment for vmstate_ppc_cpu and
>> VMSTATE_UINT32_V instead of VMSTATE_UINT32.
>
> So this means you're happy with the basic patch if I go ahead and make
> the version changes too?

Yes, I suppose.


-- 
Alexey