On 22/09/16 19:04, Nikunj A Dadhania wrote:
> Benjamin Herrenschmidt <b...@kernel.crashing.org> writes:
>
>> On Thu, 2016-09-22 at 11:45 +0530, Bharata B Rao wrote:
>>> On Thu, Sep 22, 2016 at 04:07:21PM +1000, Benjamin Herrenschmidt wrote:
>>>>
>>>> On Thu, 2016-09-22 at 10:51 +0530, Bharata B Rao wrote:
>>>>>
>>>>> The flag values are expected to remain same for a machine version for
>>>>> the migration to succeed, but this expectation is broken now. Should
>>>>> we make the addition of these flags conditional on machine type
>>>>> version ?
>>>>> But these flags are part of POWER8 CPU definition which is common for
>>>>> both pseries and upcoming powernv.
>>>>
>>>> Does this affect KVM ? (And if yes why on earth would KVM give a flying
>>>> f*** about the TCG instruction flags ?) ... If not, then I think we can
>>>> safely not care.
>>>
>>> Yes, KVM migration is broken.
>>
>> Argh then ... stupid design in QEMU. We can't fix anything without
>> breaking migration, yay !
>
> Looking back in the history of the code:
>
> commit: a90db1584a00dc1d1439dc7729d99674b666b85e (target-ppc: Convert
> ppc cpu savevm to VMStateDescription) added this:
>
> + /* Sanity checking */
> + VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU),
> + VMSTATE_UINT64_EQUAL(env.insns_flags2, PowerPCCPU),
>
> These flags weren't part of vmstate, I am not sure what was the reason
> behind adding it though. Its a bit old, Alexey do you remember?
These flags say what QEMU can and cannot emulate, when we migrate, we want
to make sure that the QEMU machine remains the same.
_Today_ I would not do that and rather have added CPU flags to ensure
compatibility but those days VMSTATE_xxx_EQUAL() were not considered bad
idea yet :)
>> I don't know what to do to fix that to be honest. Do we have a way to filter
>> what flags actually matter and filter things out when KVM is enabled ?
imho we should migrate them (i.e. without _EQUAL) and let cpu_post_load()
fail if something incompatible arrived.
>
> Something like this works for KVM:
>
> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> index 4820f22..1cf3779 100644
> --- a/target-ppc/machine.c
> +++ b/target-ppc/machine.c
> @@ -563,8 +563,8 @@ const VMStateDescription vmstate_ppc_cpu = {
>
> /* Sanity checking */
> VMSTATE_UINTTL_EQUAL(env.msr_mask, PowerPCCPU),
> - VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU),
> - VMSTATE_UINT64_EQUAL(env.insns_flags2, PowerPCCPU),
> + VMSTATE_UNUSED(sizeof(target_ulong)), /* was _EQUAL(env.insns_flags)
> */
> + VMSTATE_UNUSED(sizeof(target_ulong)), /* was
> _EQUAL(env.insns_flags2) */
> VMSTATE_UINT32_EQUAL(env.nb_BATs, PowerPCCPU),
> VMSTATE_END_OF_LIST()
> },
>
> TCG migration still remains broken with this.
--
Alexey
