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

[David Gibson]

On Mon, Nov 14, 2016 at 01:34:55PM +1100, Alexey Kardashevskiy wrote:
> On 12/11/16 05:13, Greg Kurz wrote:
> > On Tue, 8 Nov 2016 16:31:08 +1100
> > David Gibson <address@hidden> 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 <address@hidden>
> >>>> ---
> >>>>  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:
> >>>
> >>> address@hidden: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 simpler way would be:
> 
> 1. add a copy for each field (s/msr_mask/mig_msr_mask/),
> 2. get rid of _EQUAL bits,
> 3. implement .pre_save callback which would initialize mig_xxx
> 
> And that's it, .pre_load/.post_load is not needed.

Yeah, it works, but crufts up the runtime structures with extra fields
related only to migration with old versions.  I think just dropping
the backwards migration is a better option in this case.

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




-- 
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