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

[David Gibson]

On Mon, Sep 11, 2017 at 10:30:33AM +0100, Dr. David Alan Gilbert wrote:
> * Greg Kurz (address@hidden) wrote:
> > On Sun, 10 Sep 2017 15:37:33 +0100
> > Mark Cave-Ayland <address@hidden> 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.
> > > 
> > > As part of this commit we bump the vmstate_ppc version from 5 to 6 to 
> > > handle
> > > the additional fields.
> > > 
> > 
> > And so this unconditionally breaks backward migration... what about adding
> > a subsection for this ?
> 
> and wiring it to a flag on the machine type so that older machine types
> don't send it.

Right, a subsection is certainly necessary to avoid breaking backwards
migration.

But apart from that I want to understand better exactly why this is
necessary.  What's the state that's being lost, and is it really not
recoverable from anywhere else.

The other thing that concerns me is how we're encoding the
information.  These are essentially internal fields, not reflecting
something with an architected encoding - adding those to the migration
stream is often a bad idea - it inhibits our ability to rework
internal encodings.
> 
> Dave
> 
> > > Signed-off-by: Mark Cave-Ayland <address@hidden>
> > > ---
> > >  target/ppc/machine.c |    6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> > > index e59049f..8fec1a4 100644
> > > --- a/target/ppc/machine.c
> > > +++ b/target/ppc/machine.c
> > > @@ -647,7 +647,7 @@ static const VMStateDescription vmstate_compat = {
> > >  
> > >  const VMStateDescription vmstate_ppc_cpu = {
> > >      .name = "cpu",
> > > -    .version_id = 5,
> > > +    .version_id = 6,
> > >      .minimum_version_id = 5,
> > >      .minimum_version_id_old = 4,
> > >      .load_state_old = cpu_load_old,
> > > @@ -678,6 +678,10 @@ const VMStateDescription vmstate_ppc_cpu = {
> > >          VMSTATE_UINTTL(env.hflags_nmsr, PowerPCCPU),
> > >          /* FIXME: access_type? */
> > >  
> > > +        /* Interrupt state */
> > > +        VMSTATE_UINT32_V(env.pending_interrupts, PowerPCCPU, 6),
> > > +        VMSTATE_UINT32_V(env.irq_input_state, PowerPCCPU, 6),
> > > +
> > >          /* Sanity checking */
> > >          VMSTATE_UINTTL_TEST(mig_msr_mask, PowerPCCPU, 
> > > cpu_pre_2_8_migration),
> > >          VMSTATE_UINT64_TEST(mig_insns_flags, PowerPCCPU, 
> > > cpu_pre_2_8_migration),
> > 
> 
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature