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

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

[Bharata B Rao]

Hi,

Nikunj and I realized that migrating pseries-2.6 guest from QEMU-2.6
to newer QEMU-2.7 is broken like this:

qemu-system-ppc64: error while loading state for instance 0x0 of device 'cpu'
qemu-system-ppc64: load of migration failed: Invalid argument

Bisecting tells us that 4e0806110c8b896ceff3490f15a616e8b3165efe
(ppc: Add PPC_64H instruction flag to POWER7 and POWER8) is the
first bad commit.  Along with this there are other 3 similar commits
which add new bits to insns_flags and insns_flags2 fields of POWER7
and POWER8 CPUs.

4e0806110c8b896ceff3490f15a616e8b3165efe Adds PPC_64H to POWER7 and POWER8
dfdd3e43620a6cd4f2be31da5a257c84a16fc000 Adds PPC_64BX to POWER7
b781537560e3b968b6fe1395e3d07bd67f0009ba Adds PPC_CILDST to POWER7 and POWER8
7778a575c7055276afdd01737e9d1029a65f923d Adds PPC2_PM_ISA206 to POWER7 and POWER8

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.

Regards,
Bharata.

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

[David Gibson]

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

On Thu, Sep 22, 2016 at 10:51:05AM +0530, Bharata B Rao wrote:
> Hi,
> 
> Nikunj and I realized that migrating pseries-2.6 guest from QEMU-2.6
> to newer QEMU-2.7 is broken like this:
> 
> qemu-system-ppc64: error while loading state for instance 0x0 of device 'cpu'
> qemu-system-ppc64: load of migration failed: Invalid argument
> 
> Bisecting tells us that 4e0806110c8b896ceff3490f15a616e8b3165efe
> (ppc: Add PPC_64H instruction flag to POWER7 and POWER8) is the
> first bad commit.  Along with this there are other 3 similar commits
> which add new bits to insns_flags and insns_flags2 fields of POWER7
> and POWER8 CPUs.
> 
> 4e0806110c8b896ceff3490f15a616e8b3165efe Adds PPC_64H to POWER7 and POWER8
> dfdd3e43620a6cd4f2be31da5a257c84a16fc000 Adds PPC_64BX to POWER7
> b781537560e3b968b6fe1395e3d07bd67f0009ba Adds PPC_CILDST to POWER7 and POWER8
> 7778a575c7055276afdd01737e9d1029a65f923d Adds PPC2_PM_ISA206 to POWER7 and POWER8
> 
> 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.

Can you step me through how the new flags are breaking the migration?
It's not immediately obvious to me.

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

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

[Bharata B Rao]

On Thu, Sep 22, 2016 at 03:30:08PM +1000, David Gibson wrote:
> On Thu, Sep 22, 2016 at 10:51:05AM +0530, Bharata B Rao wrote:
> > Hi,
> > 
> > Nikunj and I realized that migrating pseries-2.6 guest from QEMU-2.6
> > to newer QEMU-2.7 is broken like this:
> > 
> > qemu-system-ppc64: error while loading state for instance 0x0 of device 'cpu'
> > qemu-system-ppc64: load of migration failed: Invalid argument
> > 
> > Bisecting tells us that 4e0806110c8b896ceff3490f15a616e8b3165efe
> > (ppc: Add PPC_64H instruction flag to POWER7 and POWER8) is the
> > first bad commit.  Along with this there are other 3 similar commits
> > which add new bits to insns_flags and insns_flags2 fields of POWER7
> > and POWER8 CPUs.
> > 
> > 4e0806110c8b896ceff3490f15a616e8b3165efe Adds PPC_64H to POWER7 and POWER8
> > dfdd3e43620a6cd4f2be31da5a257c84a16fc000 Adds PPC_64BX to POWER7
> > b781537560e3b968b6fe1395e3d07bd67f0009ba Adds PPC_CILDST to POWER7 and POWER8
> > 7778a575c7055276afdd01737e9d1029a65f923d Adds PPC2_PM_ISA206 to POWER7 and POWER8
> > 
> > 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.
> 
> Can you step me through how the new flags are breaking the migration?
> It's not immediately obvious to me.

Here is what I understand. Given below is the pruned vmstate_ppc_cpu
data structure.

const VMStateDescription vmstate_ppc_cpu = {
    .name = "cpu",
    .fields = (VMStateField[]) {
        /* Sanity checking */
        VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU),
        VMSTATE_UINT64_EQUAL(env.insns_flags2, PowerPCCPU),
        VMSTATE_END_OF_LIST()
    },
};

When pseries-2.6 guest is started at the source with QEMU-2.6, insns_flags
and insns_flags2 will not have PPC_64H, PPC_64BX, PPC_CILDST, PPC2_PM_ISA206
set. However at the target when pseries-2.6 guest is started with QEMU-2.7,
these flags will be set. And I believe VMSTATE_UINT64_EQUAL checks above
will cause migration to fail.

Regards,
Bharata.

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

[Benjamin Herrenschmidt]

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.

This migration business is making life really really really hard ...

Ben.

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

[Bharata B Rao]

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.

Regards,
Bharata.

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

[Cédric Le Goater]

On 09/22/2016 08:00 AM, Bharata B Rao wrote:
> On Thu, Sep 22, 2016 at 03:30:08PM +1000, David Gibson wrote:
>> On Thu, Sep 22, 2016 at 10:51:05AM +0530, Bharata B Rao wrote:
>>> Hi,
>>>
>>> Nikunj and I realized that migrating pseries-2.6 guest from QEMU-2.6
>>> to newer QEMU-2.7 is broken like this:
>>>
>>> qemu-system-ppc64: error while loading state for instance 0x0 of device 'cpu'
>>> qemu-system-ppc64: load of migration failed: Invalid argument
>>>
>>> Bisecting tells us that 4e0806110c8b896ceff3490f15a616e8b3165efe
>>> (ppc: Add PPC_64H instruction flag to POWER7 and POWER8) is the
>>> first bad commit.  Along with this there are other 3 similar commits
>>> which add new bits to insns_flags and insns_flags2 fields of POWER7
>>> and POWER8 CPUs.
>>>
>>> 4e0806110c8b896ceff3490f15a616e8b3165efe Adds PPC_64H to POWER7 and POWER8
>>> dfdd3e43620a6cd4f2be31da5a257c84a16fc000 Adds PPC_64BX to POWER7
>>> b781537560e3b968b6fe1395e3d07bd67f0009ba Adds PPC_CILDST to POWER7 and POWER8
>>> 7778a575c7055276afdd01737e9d1029a65f923d Adds PPC2_PM_ISA206 to POWER7 and POWER8
>>>
>>> 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.
>>
>> Can you step me through how the new flags are breaking the migration?
>> It's not immediately obvious to me.
> 
> Here is what I understand. Given below is the pruned vmstate_ppc_cpu
> data structure.
> 
> const VMStateDescription vmstate_ppc_cpu = {
>     .name = "cpu",
>     .fields = (VMStateField[]) {
>         /* Sanity checking */
>         VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU),
>         VMSTATE_UINT64_EQUAL(env.insns_flags2, PowerPCCPU),
>         VMSTATE_END_OF_LIST()
>     },
> };
> 
> When pseries-2.6 guest is started at the source with QEMU-2.6, insns_flags
> and insns_flags2 will not have PPC_64H, PPC_64BX, PPC_CILDST, PPC2_PM_ISA206
> set. However at the target when pseries-2.6 guest is started with QEMU-2.7,
> these flags will be set. And I believe VMSTATE_UINT64_EQUAL checks above
> will cause migration to fail.

So does this mean that we can not add support for new instructions in 
qemu without breaking migration with older versions ? 

If so, that is really bad, we need to find a way to fix this. Should we
add a 'version' to insns_flags* ? 


C.

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

[Benjamin Herrenschmidt]

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 !

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 ?

Ben.

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

[Nikunj A Dadhania]

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()
     },

TCG migration still remains broken with this.

Regards,
Nikunj

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

[Dr. David Alan Gilbert]

* Bharata B Rao (bharata@linux.vnet.ibm.com) wrote:
> On Thu, Sep 22, 2016 at 03:30:08PM +1000, David Gibson wrote:
> > On Thu, Sep 22, 2016 at 10:51:05AM +0530, Bharata B Rao wrote:
> > > Hi,
> > > 
> > > Nikunj and I realized that migrating pseries-2.6 guest from QEMU-2.6
> > > to newer QEMU-2.7 is broken like this:
> > > 
> > > qemu-system-ppc64: error while loading state for instance 0x0 of device 'cpu'
> > > qemu-system-ppc64: load of migration failed: Invalid argument
> > > 
> > > Bisecting tells us that 4e0806110c8b896ceff3490f15a616e8b3165efe
> > > (ppc: Add PPC_64H instruction flag to POWER7 and POWER8) is the
> > > first bad commit.  Along with this there are other 3 similar commits
> > > which add new bits to insns_flags and insns_flags2 fields of POWER7
> > > and POWER8 CPUs.
> > > 
> > > 4e0806110c8b896ceff3490f15a616e8b3165efe Adds PPC_64H to POWER7 and POWER8
> > > dfdd3e43620a6cd4f2be31da5a257c84a16fc000 Adds PPC_64BX to POWER7
> > > b781537560e3b968b6fe1395e3d07bd67f0009ba Adds PPC_CILDST to POWER7 and POWER8
> > > 7778a575c7055276afdd01737e9d1029a65f923d Adds PPC2_PM_ISA206 to POWER7 and POWER8
> > > 
> > > 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.
> > 
> > Can you step me through how the new flags are breaking the migration?
> > It's not immediately obvious to me.
> 
> Here is what I understand. Given below is the pruned vmstate_ppc_cpu
> data structure.
> 
> const VMStateDescription vmstate_ppc_cpu = {
>     .name = "cpu",
>     .fields = (VMStateField[]) {
>         /* Sanity checking */
>         VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU),
>         VMSTATE_UINT64_EQUAL(env.insns_flags2, PowerPCCPU),
>         VMSTATE_END_OF_LIST()
>     },
> };
> 
> When pseries-2.6 guest is started at the source with QEMU-2.6, insns_flags
> and insns_flags2 will not have PPC_64H, PPC_64BX, PPC_CILDST, PPC2_PM_ISA206
> set. However at the target when pseries-2.6 guest is started with QEMU-2.7,
> these flags will be set. And I believe VMSTATE_UINT64_EQUAL checks above
> will cause migration to fail.

You might find the first two patches in:
   https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg03681.html
useful in debugging this; it prints the values when the _EQUAL macros fail and prints
the field name that fails.

Dave

> Regards,
> Bharata.
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Nikunj A Dadhania]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Bharata B Rao (bharata@linux.vnet.ibm.com) wrote:
>> On Thu, Sep 22, 2016 at 03:30:08PM +1000, David Gibson wrote:
>> > On Thu, Sep 22, 2016 at 10:51:05AM +0530, Bharata B Rao wrote:
>> > > Hi,
>> > > 
>> > > Nikunj and I realized that migrating pseries-2.6 guest from QEMU-2.6
>> > > to newer QEMU-2.7 is broken like this:
>> > > 
>> > > qemu-system-ppc64: error while loading state for instance 0x0 of device 'cpu'
>> > > qemu-system-ppc64: load of migration failed: Invalid argument
>> > > 
>> > > Bisecting tells us that 4e0806110c8b896ceff3490f15a616e8b3165efe
>> > > (ppc: Add PPC_64H instruction flag to POWER7 and POWER8) is the
>> > > first bad commit.  Along with this there are other 3 similar commits
>> > > which add new bits to insns_flags and insns_flags2 fields of POWER7
>> > > and POWER8 CPUs.
>> > > 
>> > > 4e0806110c8b896ceff3490f15a616e8b3165efe Adds PPC_64H to POWER7 and POWER8
>> > > dfdd3e43620a6cd4f2be31da5a257c84a16fc000 Adds PPC_64BX to POWER7
>> > > b781537560e3b968b6fe1395e3d07bd67f0009ba Adds PPC_CILDST to POWER7 and POWER8
>> > > 7778a575c7055276afdd01737e9d1029a65f923d Adds PPC2_PM_ISA206 to POWER7 and POWER8
>> > > 
>> > > 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.
>> > 
>> > Can you step me through how the new flags are breaking the migration?
>> > It's not immediately obvious to me.
>> 
>> Here is what I understand. Given below is the pruned vmstate_ppc_cpu
>> data structure.
>> 
>> const VMStateDescription vmstate_ppc_cpu = {
>>     .name = "cpu",
>>     .fields = (VMStateField[]) {
>>         /* Sanity checking */
>>         VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU),
>>         VMSTATE_UINT64_EQUAL(env.insns_flags2, PowerPCCPU),
>>         VMSTATE_END_OF_LIST()
>>     },
>> };
>> 
>> When pseries-2.6 guest is started at the source with QEMU-2.6, insns_flags
>> and insns_flags2 will not have PPC_64H, PPC_64BX, PPC_CILDST, PPC2_PM_ISA206
>> set. However at the target when pseries-2.6 guest is started with QEMU-2.7,
>> these flags will be set. And I believe VMSTATE_UINT64_EQUAL checks above
>> will cause migration to fail.
>
> You might find the first two patches in:
>    https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg03681.html
> useful in debugging this; it prints the values when the _EQUAL macros fail and prints
> the field name that fails.

Thanks, we were using trace, this is very helpful without trace
during error conditions.

qemu-system-ppc64: 9223477658187168481 != 9223477658187151905
qemu-system-ppc64: Failed to load cpu:env.insns_flags
qemu-system-ppc64: error while loading state for instance 0x0 of device 'cpu'
qemu-system-ppc64: load of migration failed: Invalid argument

Regards,
Nikunj

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

[Benjamin Herrenschmidt]

On Thu, 2016-09-22 at 14:34 +0530, Nikunj A Dadhania wrote:
> 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.

Can we have conditionally present flags and a post-load that does some
matching ?

Cheers,
Ben.

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

[Dr. David Alan Gilbert]

* Nikunj A Dadhania (nikunj@linux.vnet.ibm.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > * Bharata B Rao (bharata@linux.vnet.ibm.com) wrote:
> >> On Thu, Sep 22, 2016 at 03:30:08PM +1000, David Gibson wrote:
> >> > On Thu, Sep 22, 2016 at 10:51:05AM +0530, Bharata B Rao wrote:
> >> > > Hi,
> >> > > 
> >> > > Nikunj and I realized that migrating pseries-2.6 guest from QEMU-2.6
> >> > > to newer QEMU-2.7 is broken like this:
> >> > > 
> >> > > qemu-system-ppc64: error while loading state for instance 0x0 of device 'cpu'
> >> > > qemu-system-ppc64: load of migration failed: Invalid argument
> >> > > 
> >> > > Bisecting tells us that 4e0806110c8b896ceff3490f15a616e8b3165efe
> >> > > (ppc: Add PPC_64H instruction flag to POWER7 and POWER8) is the
> >> > > first bad commit.  Along with this there are other 3 similar commits
> >> > > which add new bits to insns_flags and insns_flags2 fields of POWER7
> >> > > and POWER8 CPUs.
> >> > > 
> >> > > 4e0806110c8b896ceff3490f15a616e8b3165efe Adds PPC_64H to POWER7 and POWER8
> >> > > dfdd3e43620a6cd4f2be31da5a257c84a16fc000 Adds PPC_64BX to POWER7
> >> > > b781537560e3b968b6fe1395e3d07bd67f0009ba Adds PPC_CILDST to POWER7 and POWER8
> >> > > 7778a575c7055276afdd01737e9d1029a65f923d Adds PPC2_PM_ISA206 to POWER7 and POWER8
> >> > > 
> >> > > 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.
> >> > 
> >> > Can you step me through how the new flags are breaking the migration?
> >> > It's not immediately obvious to me.
> >> 
> >> Here is what I understand. Given below is the pruned vmstate_ppc_cpu
> >> data structure.
> >> 
> >> const VMStateDescription vmstate_ppc_cpu = {
> >>     .name = "cpu",
> >>     .fields = (VMStateField[]) {
> >>         /* Sanity checking */
> >>         VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU),
> >>         VMSTATE_UINT64_EQUAL(env.insns_flags2, PowerPCCPU),
> >>         VMSTATE_END_OF_LIST()
> >>     },
> >> };
> >> 
> >> When pseries-2.6 guest is started at the source with QEMU-2.6, insns_flags
> >> and insns_flags2 will not have PPC_64H, PPC_64BX, PPC_CILDST, PPC2_PM_ISA206
> >> set. However at the target when pseries-2.6 guest is started with QEMU-2.7,
> >> these flags will be set. And I believe VMSTATE_UINT64_EQUAL checks above
> >> will cause migration to fail.
> >
> > You might find the first two patches in:
> >    https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg03681.html
> > useful in debugging this; it prints the values when the _EQUAL macros fail and prints
> > the field name that fails.
> 
> Thanks, we were using trace, this is very helpful without trace
> during error conditions.
> 
> qemu-system-ppc64: 9223477658187168481 != 9223477658187151905
> qemu-system-ppc64: Failed to load cpu:env.insns_flags
> qemu-system-ppc64: error while loading state for instance 0x0 of device 'cpu'
> qemu-system-ppc64: load of migration failed: Invalid argument

Ah good, that's what I was hoping for (I'll change them to hex before I
repost that series).

Dave

> 
> Regards,
> Nikunj
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Paolo Bonzini]

On 22/09/2016 12:04, Benjamin Herrenschmidt wrote:
> On Thu, 2016-09-22 at 14:34 +0530, Nikunj A Dadhania wrote:
>> 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.
> 
> Can we have conditionally present flags and a post-load that does some
> matching ?

Yes, you can use something like

	VMSTATE_UINT64(env.src_insns_flags, PowerPCCPU),
	VMSTATE_UINT64(env.src_insns_flags2, PowerPCCPU),

and a post_load that compares them if kvm_enabled() only.

*However* a better fix would be to preserve the old flags for
pseries-2.6, and only set the newer flags for pseries-2.7.  I'm not
saying you have to do this, but if it's not hard (no idea) why not learn
how to do it right.

The design is not stupid, it's just that compatibility is harder than
you think and you are going through the same learning experiences that
x86 went though.

Paolo

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

[Alexey Kardashevskiy]

On 22/09/16 19:04, 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?


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

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

[Nikunj A Dadhania]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> On Thu, 2016-09-22 at 14:34 +0530, Nikunj A Dadhania wrote:
>> 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.
>
> Can we have conditionally present flags and a post-load that does some
> matching ?

I think its possible like this:

diff --git a/target-ppc/machine.c b/target-ppc/machine.c
index 4820f22..dc4704e 100644
--- a/target-ppc/machine.c
+++ b/target-ppc/machine.c
@@ -528,6 +528,42 @@ static const VMStateDescription vmstate_tlbmas = {
     }
 };
 
+static bool ppc_kvm_enabled(void *opaque, int version_id)
+{
+    printf("%s: is kvm enabled %d\n", __func__, kvm_enabled());
+    return !kvm_enabled();
+}
+
+static int get_insns_equal(QEMUFile *f, void *pv, size_t size)
+{
+    uint64_t *v = pv;
+    uint64_t v2;
+    qemu_get_be64s(f, &v2);
+
+    printf("%s: \n", __func__);
+
+    if (*v == v2) {
+        return 0;
+    }
+    printf("Did not match, ignore %" PRIu64 " != %" PRIu64 "\n", *v, v2);
+    return 0;
+}
+
+static void put_insns(QEMUFile *f, void *pv, size_t size)
+{
+    uint64_t *v = pv;
+    qemu_put_be64s(f, v);
+}
+
+const VMStateInfo vmstate_info_insns_equal = {
+    .name = "insns equal",
+    .get  = get_insns_equal,
+    .put  = put_insns,
+};
+
+#define VMSTATE_INSNS_EQUAL(_f, _s, _t)                                 \
+    VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_info_insns_equal, uint64_t)
+
 const VMStateDescription vmstate_ppc_cpu = {
     .name = "cpu",
     .version_id = 5,
@@ -563,8 +599,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_INSNS_EQUAL(env.insns_flags, PowerPCCPU, ppc_kvm_enabled),
+        VMSTATE_INSNS_EQUAL(env.insns_flags2, PowerPCCPU, ppc_kvm_enabled),
         VMSTATE_UINT32_EQUAL(env.nb_BATs, PowerPCCPU),
         VMSTATE_END_OF_LIST()
     },


TCG migration succeeds and proceeds ahead. But fails somewhere ahead in
powerpc exception handler:

[qemu]$ ./ppc64-softmmu/qemu-system-ppc64  -machine pseries-2.6,usb=off -vga none -nographic -m 2G   ../../imgs/guest.disk -monitor pty --incoming tcp:localhost:4444 
char device redirected to /dev/pts/5 (label compat_monitor0)
ppc_kvm_enabled: is kvm enabled 0
get_insns_equal: 
Did not match, ignore 9223477658187168481 != 9223477658187151905
ppc_kvm_enabled: is kvm enabled 0
get_insns_equal: 
Did not match, ignore 331702 != 69558
Cannot open font file True
Cannot open font file True
qemu: fatal: Trying to deliver HV exception 4 with no HV support

NIP c0000000000795c8   LR d00000000074407c CTR c000000000079544 XER 0000000000000000 CPU#0
MSR 8000000000009032 HID0 0000000000000000  HF 8000000000000030 iidx 1 didx 1
TB 00000007 32202510341 DECR 00596259

Regards,
Nikunj

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

[Benjamin Herrenschmidt]

On Thu, 2016-09-22 at 12:32 +0200, Paolo Bonzini wrote:
> *However* a better fix would be to preserve the old flags for
> pseries-2.6, and only set the newer flags for pseries-2.7.  I'm not
> saying you have to do this, but if it's not hard (no idea) why not learn
> how to do it right.
> 
> The design is not stupid, it's just that compatibility is harder than
> you think and you are going through the same learning experiences that
> x86 went though.

Yeah well, the design is stupid inside target-ppc is what I meant in
the sense that it should have been clearer that those flags should not
have affected KVM, especially knowing that TCG still needed a lot of
work to add all the proper HV stuff.

Also most/all those flags concern instructions that are not relevant to
the "PAPR" mode which is running the guest with HV disabled, so
additionally, we might want to consider being smarter in the compare as
well to make sure that only the flags relevant to guest mode are
compared when the vCPU is in PAPR mode.

Cheers,
Ben.

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

[Benjamin Herrenschmidt]

On Thu, 2016-09-22 at 20:34 +1000, Alexey Kardashevskiy wrote:
> > 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.

TCG migration doesn't matter much ... yet, I think.

KVM is what actual customers use, we can probably live with some TCG
migration breakage. Hopefully we'll be done with P8 soon and it will be
stable enough, and we'll be more careful with P9.

Cheers,
Ben.

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

[Nikunj A Dadhania]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
>> > You might find the first two patches in:
>> >    https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg03681.html
>> > useful in debugging this; it prints the values when the _EQUAL macros fail and prints
>> > the field name that fails.
>> 
>> Thanks, we were using trace, this is very helpful without trace
>> during error conditions.
>> 
>> qemu-system-ppc64: 9223477658187168481 != 9223477658187151905
>> qemu-system-ppc64: Failed to load cpu:env.insns_flags
>> qemu-system-ppc64: error while loading state for instance 0x0 of device 'cpu'
>> qemu-system-ppc64: load of migration failed: Invalid argument
>
> Ah good, that's what I was hoping for (I'll change them to hex before I
> repost that series).

Yes, hex will be better.

Regards,
Nikunj

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

[Cédric Le Goater]

On 09/22/2016 01:07 PM, Nikunj A Dadhania wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> 
>> On Thu, 2016-09-22 at 14:34 +0530, Nikunj A Dadhania wrote:
>>> 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.
>>
>> Can we have conditionally present flags and a post-load that does some
>> matching ?
> 
> I think its possible like this:
> 
> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> index 4820f22..dc4704e 100644
> --- a/target-ppc/machine.c
> +++ b/target-ppc/machine.c
> @@ -528,6 +528,42 @@ static const VMStateDescription vmstate_tlbmas = {
>      }
>  };
>  
> +static bool ppc_kvm_enabled(void *opaque, int version_id)
> +{
> +    printf("%s: is kvm enabled %d\n", __func__, kvm_enabled());
> +    return !kvm_enabled();
> +}
> +
> +static int get_insns_equal(QEMUFile *f, void *pv, size_t size)
> +{
> +    uint64_t *v = pv;
> +    uint64_t v2;
> +    qemu_get_be64s(f, &v2);
> +
> +    printf("%s: \n", __func__);
> +
> +    if (*v == v2) {
> +        return 0;
> +    }
> +    printf("Did not match, ignore %" PRIu64 " != %" PRIu64 "\n", *v, v2);
> +    return 0;
> +}
> +
> +static void put_insns(QEMUFile *f, void *pv, size_t size)
> +{
> +    uint64_t *v = pv;
> +    qemu_put_be64s(f, v);
> +}
> +
> +const VMStateInfo vmstate_info_insns_equal = {
> +    .name = "insns equal",
> +    .get  = get_insns_equal,
> +    .put  = put_insns,
> +};
> +
> +#define VMSTATE_INSNS_EQUAL(_f, _s, _t)                                 \
> +    VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_info_insns_equal, uint64_t)
> +
>  const VMStateDescription vmstate_ppc_cpu = {
>      .name = "cpu",
>      .version_id = 5,
> @@ -563,8 +599,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_INSNS_EQUAL(env.insns_flags, PowerPCCPU, ppc_kvm_enabled),
> +        VMSTATE_INSNS_EQUAL(env.insns_flags2, PowerPCCPU, ppc_kvm_enabled),
>          VMSTATE_UINT32_EQUAL(env.nb_BATs, PowerPCCPU),
>          VMSTATE_END_OF_LIST()
>      },
> 
> 
> TCG migration succeeds and proceeds ahead. But fails somewhere ahead in
> powerpc exception handler:
> 
> [qemu]$ ./ppc64-softmmu/qemu-system-ppc64  -machine pseries-2.6,usb=off -vga none -nographic -m 2G   ../../imgs/guest.disk -monitor pty --incoming tcp:localhost:4444 
> char device redirected to /dev/pts/5 (label compat_monitor0)
> ppc_kvm_enabled: is kvm enabled 0
> get_insns_equal: 
> Did not match, ignore 9223477658187168481 != 9223477658187151905
> ppc_kvm_enabled: is kvm enabled 0
> get_insns_equal: 
> Did not match, ignore 331702 != 69558
> Cannot open font file True
> Cannot open font file True
> qemu: fatal: Trying to deliver HV exception 4 with no HV support

hmm, this is because we added MSR_HVB in msr_mask AFAICT. we should have 
a similar vmstate op for it I think

C. 

> 
> NIP c0000000000795c8   LR d00000000074407c CTR c000000000079544 XER 0000000000000000 CPU#0
> MSR 8000000000009032 HID0 0000000000000000  HF 8000000000000030 iidx 1 didx 1
> TB 00000007 32202510341 DECR 00596259
> 
> Regards,
> Nikunj
> 

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

[Benjamin Herrenschmidt]

On Thu, 2016-09-22 at 13:27 +0200, Cédric Le Goater wrote:

> > TCG migration succeeds and proceeds ahead. But fails somewhere
> > ahead in
> > powerpc exception handler:
> > 
> > [qemu]$ ./ppc64-softmmu/qemu-system-ppc64  -machine pseries-
> > 2.6,usb=off -vga none -nographic -m 2G   ../../imgs/guest.disk
> > -monitor pty --incoming tcp:localhost:4444 
> > char device redirected to /dev/pts/5 (label compat_monitor0)
> > ppc_kvm_enabled: is kvm enabled 0
> > get_insns_equal: 
> > Did not match, ignore 9223477658187168481 != 9223477658187151905
> > ppc_kvm_enabled: is kvm enabled 0
> > get_insns_equal: 
> > Did not match, ignore 331702 != 69558
> > Cannot open font file True
> > Cannot open font file True
> > qemu: fatal: Trying to deliver HV exception 4 with no HV support
> 
> hmm, this is because we added MSR_HVB in msr_mask AFAICT. we should
> have a similar vmstate op for it I think

We also need to be careful about now allowing KVM migration to/from
wildly different CPU generations, or is that handled elsewhere ? (PVR
match ?)

> C. 
> 
> > 
> > 
> > NIP c0000000000795c8   LR d00000000074407c CTR c000000000079544 XER
> > 0000000000000000 CPU#0
> > MSR 8000000000009032 HID0 0000000000000000  HF 8000000000000030
> > iidx 1 didx 1
> > TB 00000007 32202510341 DECR 00596259
> > 
> > Regards,
> > Nikunj
> > 

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

[Dr. David Alan Gilbert]

* Nikunj A Dadhania (nikunj@linux.vnet.ibm.com) wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> 
> > On Thu, 2016-09-22 at 14:34 +0530, Nikunj A Dadhania wrote:
> >> 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.
> >
> > Can we have conditionally present flags and a post-load that does some
> > matching ?
> 
> I think its possible like this:
> 
> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> index 4820f22..dc4704e 100644
> --- a/target-ppc/machine.c
> +++ b/target-ppc/machine.c
> @@ -528,6 +528,42 @@ static const VMStateDescription vmstate_tlbmas = {
>      }
>  };
>  
> +static bool ppc_kvm_enabled(void *opaque, int version_id)
> +{
> +    printf("%s: is kvm enabled %d\n", __func__, kvm_enabled());
> +    return !kvm_enabled();
> +}
> +
> +static int get_insns_equal(QEMUFile *f, void *pv, size_t size)
> +{
> +    uint64_t *v = pv;
> +    uint64_t v2;
> +    qemu_get_be64s(f, &v2);
> +
> +    printf("%s: \n", __func__);
> +
> +    if (*v == v2) {
> +        return 0;
> +    }
> +    printf("Did not match, ignore %" PRIu64 " != %" PRIu64 "\n", *v, v2);
> +    return 0;
> +}
> +
> +static void put_insns(QEMUFile *f, void *pv, size_t size)
> +{
> +    uint64_t *v = pv;
> +    qemu_put_be64s(f, v);
> +}
> +
> +const VMStateInfo vmstate_info_insns_equal = {
> +    .name = "insns equal",
> +    .get  = get_insns_equal,
> +    .put  = put_insns,
> +};
> +

I'd prefer it if you can avoid adding qemu_get/put's unless
really desperate; I'm trying to squash all the read/writing back into
standard macros; but I understand it can be tricky.

I'd agree that a post_load is the nicest way; it can return
an error value.
(Oh and ideally use error_report)

Dave

> +#define VMSTATE_INSNS_EQUAL(_f, _s, _t)                                 \
> +    VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_info_insns_equal, uint64_t)
> +
>  const VMStateDescription vmstate_ppc_cpu = {
>      .name = "cpu",
>      .version_id = 5,
> @@ -563,8 +599,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_INSNS_EQUAL(env.insns_flags, PowerPCCPU, ppc_kvm_enabled),
> +        VMSTATE_INSNS_EQUAL(env.insns_flags2, PowerPCCPU, ppc_kvm_enabled),
>          VMSTATE_UINT32_EQUAL(env.nb_BATs, PowerPCCPU),
>          VMSTATE_END_OF_LIST()
>      },
> 
> 
> TCG migration succeeds and proceeds ahead. But fails somewhere ahead in
> powerpc exception handler:
> 
> [qemu]$ ./ppc64-softmmu/qemu-system-ppc64  -machine pseries-2.6,usb=off -vga none -nographic -m 2G   ../../imgs/guest.disk -monitor pty --incoming tcp:localhost:4444 
> char device redirected to /dev/pts/5 (label compat_monitor0)
> ppc_kvm_enabled: is kvm enabled 0
> get_insns_equal: 
> Did not match, ignore 9223477658187168481 != 9223477658187151905
> ppc_kvm_enabled: is kvm enabled 0
> get_insns_equal: 
> Did not match, ignore 331702 != 69558
> Cannot open font file True
> Cannot open font file True
> qemu: fatal: Trying to deliver HV exception 4 with no HV support
> 
> NIP c0000000000795c8   LR d00000000074407c CTR c000000000079544 XER 0000000000000000 CPU#0
> MSR 8000000000009032 HID0 0000000000000000  HF 8000000000000030 iidx 1 didx 1
> TB 00000007 32202510341 DECR 00596259
> 
> Regards,
> Nikunj
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Nikunj A Dadhania]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Nikunj A Dadhania (nikunj@linux.vnet.ibm.com) wrote:
>> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
>> 
>> > On Thu, 2016-09-22 at 14:34 +0530, Nikunj A Dadhania wrote:
>> >> 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.
>> >
>> > Can we have conditionally present flags and a post-load that does some
>> > matching ?
>> 
>> I think its possible like this:
>> 
>> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
>> index 4820f22..dc4704e 100644
>> --- a/target-ppc/machine.c
>> +++ b/target-ppc/machine.c
>> @@ -528,6 +528,42 @@ static const VMStateDescription vmstate_tlbmas = {
>>      }
>>  };
>>  
>> +static bool ppc_kvm_enabled(void *opaque, int version_id)
>> +{
>> +    printf("%s: is kvm enabled %d\n", __func__, kvm_enabled());
>> +    return !kvm_enabled();
>> +}
>> +
>> +static int get_insns_equal(QEMUFile *f, void *pv, size_t size)
>> +{
>> +    uint64_t *v = pv;
>> +    uint64_t v2;
>> +    qemu_get_be64s(f, &v2);
>> +
>> +    printf("%s: \n", __func__);
>> +
>> +    if (*v == v2) {
>> +        return 0;
>> +    }
>> +    printf("Did not match, ignore %" PRIu64 " != %" PRIu64 "\n", *v, v2);
>> +    return 0;
>> +}
>> +
>> +static void put_insns(QEMUFile *f, void *pv, size_t size)
>> +{
>> +    uint64_t *v = pv;
>> +    qemu_put_be64s(f, v);
>> +}
>> +
>> +const VMStateInfo vmstate_info_insns_equal = {
>> +    .name = "insns equal",
>> +    .get  = get_insns_equal,
>> +    .put  = put_insns,
>> +};
>> +
>
> I'd prefer it if you can avoid adding qemu_get/put's unless
> really desperate; I'm trying to squash all the read/writing back into
> standard macros; but I understand it can be tricky.

Right, the above code is just experimental :-)

>
> I'd agree that a post_load is the nicest way; it can return
> an error value.
> (Oh and ideally use error_report)

Sure

Regards
Nikunj

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

[Nikunj A Dadhania]

Cédric Le Goater <clg@kaod.org> writes:

> On 09/22/2016 01:07 PM, Nikunj A Dadhania wrote:
>> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
>> 
>>> On Thu, 2016-09-22 at 14:34 +0530, Nikunj A Dadhania wrote:
>>>> 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.
>>>
>>> Can we have conditionally present flags and a post-load that does some
>>> matching ?
>> 
>> I think its possible like this:
>> 
>> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
>> index 4820f22..dc4704e 100644
>> --- a/target-ppc/machine.c
>> +++ b/target-ppc/machine.c
>> @@ -528,6 +528,42 @@ static const VMStateDescription vmstate_tlbmas = {
>>      }
>>  };
>>  
>> +static bool ppc_kvm_enabled(void *opaque, int version_id)
>> +{
>> +    printf("%s: is kvm enabled %d\n", __func__, kvm_enabled());
>> +    return !kvm_enabled();
>> +}
>> +
>> +static int get_insns_equal(QEMUFile *f, void *pv, size_t size)
>> +{
>> +    uint64_t *v = pv;
>> +    uint64_t v2;
>> +    qemu_get_be64s(f, &v2);
>> +
>> +    printf("%s: \n", __func__);
>> +
>> +    if (*v == v2) {
>> +        return 0;
>> +    }
>> +    printf("Did not match, ignore %" PRIu64 " != %" PRIu64 "\n", *v, v2);
>> +    return 0;
>> +}
>> +
>> +static void put_insns(QEMUFile *f, void *pv, size_t size)
>> +{
>> +    uint64_t *v = pv;
>> +    qemu_put_be64s(f, v);
>> +}
>> +
>> +const VMStateInfo vmstate_info_insns_equal = {
>> +    .name = "insns equal",
>> +    .get  = get_insns_equal,
>> +    .put  = put_insns,
>> +};
>> +
>> +#define VMSTATE_INSNS_EQUAL(_f, _s, _t)                                 \
>> +    VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_info_insns_equal, uint64_t)
>> +
>>  const VMStateDescription vmstate_ppc_cpu = {
>>      .name = "cpu",
>>      .version_id = 5,
>> @@ -563,8 +599,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_INSNS_EQUAL(env.insns_flags, PowerPCCPU, ppc_kvm_enabled),
>> +        VMSTATE_INSNS_EQUAL(env.insns_flags2, PowerPCCPU, ppc_kvm_enabled),
>>          VMSTATE_UINT32_EQUAL(env.nb_BATs, PowerPCCPU),
>>          VMSTATE_END_OF_LIST()
>>      },
>> 
>> 
>> TCG migration succeeds and proceeds ahead. But fails somewhere ahead in
>> powerpc exception handler:
>> 
>> [qemu]$ ./ppc64-softmmu/qemu-system-ppc64  -machine pseries-2.6,usb=off -vga none -nographic -m 2G   ../../imgs/guest.disk -monitor pty --incoming tcp:localhost:4444 
>> char device redirected to /dev/pts/5 (label compat_monitor0)
>> ppc_kvm_enabled: is kvm enabled 0
>> get_insns_equal: 
>> Did not match, ignore 9223477658187168481 != 9223477658187151905
>> ppc_kvm_enabled: is kvm enabled 0
>> get_insns_equal: 
>> Did not match, ignore 331702 != 69558
>> Cannot open font file True
>> Cannot open font file True
>> qemu: fatal: Trying to deliver HV exception 4 with no HV support
>
> hmm, this is because we added MSR_HVB in msr_mask AFAICT. we should have 
> a similar vmstate op for it I think

Not sure how will vmstate op help here. As vmstate is migrated
successfully. Do we need to copy msr features of source ?

Regards
Nikunj

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

[David Gibson]

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

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

[David Gibson]

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

On Thu, Sep 22, 2016 at 12:32:24PM +0200, Paolo Bonzini wrote:
> 
> 
> On 22/09/2016 12:04, Benjamin Herrenschmidt wrote:
> > On Thu, 2016-09-22 at 14:34 +0530, Nikunj A Dadhania wrote:
> >> 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.
> > 
> > Can we have conditionally present flags and a post-load that does some
> > matching ?
> 
> Yes, you can use something like
> 
> 	VMSTATE_UINT64(env.src_insns_flags, PowerPCCPU),
> 	VMSTATE_UINT64(env.src_insns_flags2, PowerPCCPU),
> 
> and a post_load that compares them if kvm_enabled() only.

We could, but I'm not convinced there's any point.  I don't see that
migrating these flags actually has any value beyond a sanity check,
the consequences of which we obviously didn't think through fully.
They should just be a TCG internal matter.

> *However* a better fix would be to preserve the old flags for
> pseries-2.6, and only set the newer flags for pseries-2.7.  I'm not
> saying you have to do this, but if it's not hard (no idea) why not learn
> how to do it right.
> 
> The design is not stupid, it's just that compatibility is harder than
> you think and you are going through the same learning experiences that
> x86 went though.
> 
> Paolo
> 

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

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

[David Gibson]

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

On Thu, Sep 22, 2016 at 09:37:16PM +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2016-09-22 at 13:27 +0200, Cédric Le Goater wrote:
> 
> > > TCG migration succeeds and proceeds ahead. But fails somewhere
> > > ahead in
> > > powerpc exception handler:
> > > 
> > > [qemu]$ ./ppc64-softmmu/qemu-system-ppc64  -machine pseries-
> > > 2.6,usb=off -vga none -nographic -m 2G   ../../imgs/guest.disk
> > > -monitor pty --incoming tcp:localhost:4444 
> > > char device redirected to /dev/pts/5 (label compat_monitor0)
> > > ppc_kvm_enabled: is kvm enabled 0
> > > get_insns_equal: 
> > > Did not match, ignore 9223477658187168481 != 9223477658187151905
> > > ppc_kvm_enabled: is kvm enabled 0
> > > get_insns_equal: 
> > > Did not match, ignore 331702 != 69558
> > > Cannot open font file True
> > > Cannot open font file True
> > > qemu: fatal: Trying to deliver HV exception 4 with no HV support
> > 
> > hmm, this is because we added MSR_HVB in msr_mask AFAICT. we should
> > have a similar vmstate op for it I think
> 
> We also need to be careful about now allowing KVM migration to/from
> wildly different CPU generations, or is that handled elsewhere ? (PVR
> match ?)

Apparently not.  We do transfer the PVR value in the migration stream
(along with all actual and potential SPRs).  However in
cpu_post_load() from target-ppc/machine.c, we overwrite the incoming
value with the PVR for the command line selected CPU model.

We should check it though - that would make for a much, well, saner,
sanity check than comparing the instruction support bitmaps.

For TCG and KVM PR, just comparing for equality should be fine -
you're supposed to match PVRs at either end of the migration, just as
you have to match the rest of the hardware configuration.

For KVM HV there's a bit of a nit: that would disallow migration
between host cpus which aren't exactly the same model, but are close
enough that migration will work in practice.


Ok.. here's what I think we need to do:

    1) Remove the VMSTATE_EQUAL checks for the instruction bits, both
       in 2.8 and 2.7-stable.  That will allow migrations to work
       again, albeit requiring the user to be rather careful that the
       cpus really do match at either end.

    2) In 2.8-devel, change cpu_post_load() to check that the migrated
       PVR is the same as the destination PVR.  That will properly
       verify that we have matching CPUs using architected state.  It
       might break some cases of migrating between similar but not
       identical CPUs with -cpu host and KVM HV

    3) Before 2.8 is wrapped up, experiment to see just what cases (2)
       might have broken and come up with some mechanisms to re-allow
       them.

Thoughts?  Objections?

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

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

[Nikunj A Dadhania]

David Gibson <david@gibson.dropbear.id.au> writes:

> [ Unknown signature status ]
> 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?

Didn't debug it yet, reported on the other thread

      qemu: fatal: Trying to deliver HV exception 4 with no HV support

      NIP c0000000000795c8   LR d00000000074407c CTR c000000000079544 XER 0000000000000000 CPU#0
      MSR 8000000000009032 HID0 0000000000000000  HF 8000000000000030 iidx 1 didx 1
      TB 00000007 32202510341 DECR 00596259

Once it just hung, without any messages.

Regards
Nikunj

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

[Benjamin Herrenschmidt]

On Fri, 2016-09-23 at 11:37 +1000, David Gibson wrote:
> 
> For KVM HV there's a bit of a nit: that would disallow migration
> between host cpus which aren't exactly the same model, but are close
> enough that migration will work in practice.

In that case we should use the architected PVR

Cheers,
Ben.

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

[David Gibson]

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

On Fri, Sep 23, 2016 at 01:27:19PM +1000, Benjamin Herrenschmidt wrote:
> On Fri, 2016-09-23 at 11:37 +1000, David Gibson wrote:
> > 
> > For KVM HV there's a bit of a nit: that would disallow migration
> > between host cpus which aren't exactly the same model, but are close
> > enough that migration will work in practice.
> 
> In that case we should use the architected PVR

Uh... probably yes.  Working out how to do that isn't totally trivial,
since for TCG mode the actualy PVR SPR that qemu tracks must contain
the real PVR value (to implement mfpvr), though the spapr code is also
aware of the architected one.  We don't want to make things
gratuitously different for TCG.  Plus we need to make sure it works
for TCG, PR and HV and also for no compat mode specified, compat mode
specified on the command line and compat mode negotiated by CAS.

I don't think there's any showstopper there, but it will require a bit
of thinking.

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