Re: [Qemu-devel] pseries-2.6 migration from QEMU-2.6 to QEMU-2.7 broken

[David Gibson <david@gibson.dropbear.id.au>]

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

On Thu, Sep 22, 2016 at 02:34:19PM +0530, Nikunj A Dadhania wrote:
> Benjamin Herrenschmidt <benh@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?
> 
> > 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 ?
> 
> 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()
>      },

This looks like the right solution to me.  AFAICT this was just a
sanity check that wasn't thought through well enough.

> TCG migration still remains broken with this.

Uh.. why?

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]