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

[Alexey Kardashevskiy]

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.




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


-- 
Alexey

signature.asc
Description: OpenPGP digital signature