* David Gibson (da...@gibson.dropbear.id.au) wrote:
> On Mon, Sep 11, 2017 at 10:30:33AM +0100, Dr. David Alan Gilbert wrote:
> > * Greg Kurz (gr...@kaod.org) wrote:
> > > On Sun, 10 Sep 2017 15:37:33 +0100
> > > Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk> 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.
Yes, agreed, where possible the contents of the stream should reflect
'real' state that's actually being modelled and be as independent of
the implementation as possible.
Dave
> >
> > Dave
> >
> > > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>
> > > > ---
> > > > 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
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
