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

[Alexey Kardashevskiy]

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 <address@hidden>
> > > ---
> > >    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