Re: [Qemu-devel] [Qemu-ppc] [RFC 16/17] ppc: Remove counter-productive "sanity checks" in migration

[Greg Kurz <groug@kaod.org>]

[-- Attachment #1: Type: text/plain, Size: 2827 bytes --]

On Tue, 8 Nov 2016 16:31:08 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Nov 04, 2016 at 04:52:39PM +1100, Alexey Kardashevskiy wrote:
> > On 30/10/16 22:12, David Gibson wrote:  
> > > When vmstate for the ppc cpu was introduced in a90db158 "target-ppc:
> > > Convert ppc cpu savevm to VMStateDescription", several "sanity check"
> > > fields were included, verifying that certain cpu parameters matched between
> > > source and destination.
> > > 
> > > This turns out not to have been a good idea.  For one thing it's redundant
> > > with existing checks for a compatible cpu version at either end.  But more
> > > importantly the insns_flags and insns_flags2 checks actively break things:
> > > they expose what's essentially an internal TCG implementation detail in the
> > > migration stream.  That means that when new instruction classes are added
> > > or rearranged, migration can break.
> > > 
> > > This removes these ill-considered sanity checks.
> > > 
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  target-ppc/machine.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> > > index 62b9e94..453ef0a 100644
> > > --- a/target-ppc/machine.c
> > > +++ b/target-ppc/machine.c
> > > @@ -602,10 +602,10 @@ const VMStateDescription vmstate_ppc_cpu = {
> > >          /* FIXME: access_type? */
> > >  
> > >          /* Sanity checking */
> > > -        VMSTATE_UINTTL_EQUAL(env.msr_mask, PowerPCCPU),
> > > -        VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU),
> > > -        VMSTATE_UINT64_EQUAL(env.insns_flags2, PowerPCCPU),
> > > -        VMSTATE_UINT32_EQUAL(env.nb_BATs, PowerPCCPU),
> > > +        VMSTATE_UNUSED(sizeof(target_ulong) /* msr_mask */
> > > +                       + sizeof(uint64_t) /* insns_flags */
> > > +                       + sizeof(uint64_t) /* insns_flags2 */
> > > +                       + sizeof(uint32_t)), /* nb_BATs */  
> > 
> > 
> > This breaks migration to older QEMU:
> > 
> > 25055@1478238734.537761:vmstate_load_field_error field "env.msr_mask" load
> > failed, ret = -22  
> 
> Again, I don't think we generally support backwards migration.
> 
> That said, it would be nice here to do a "set to this field on
> ourgoing migration, but ignore on incoming migration".  Do you know a
> way to do that?
> 

This doesn't exist in vmstate but it is certainly doable. An alternative
would be to always send the fields, but have the destination to use
pre_load and post_load callbacks to preserve its state.

> a
> > 
> > 
> >   
> > >          VMSTATE_END_OF_LIST()
> > >      },
> > >      .subsections = (const VMStateDescription*[]) {
> > >   
> > 
> >   
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]