Re: [Qemu-devel] pseries-2.6 migration from QEMU-2.6 to QEMU-2.7 broken
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 signature.asc Description: PGP signature
Re: [Qemu-devel] pseries-2.6 migration from QEMU-2.6 to QEMU-2.7 broken
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 writes: > [ Unknown signature status ] > On Thu, Sep 22, 2016 at 02:34:19PM +0530, Nikunj A Dadhania wrote: >> Benjamin Herrenschmidt 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 c00795c8 LR d074407c CTR c0079544 XER CPU#0 MSR 80009032 HID0 HF 8030 iidx 1 didx 1 TB 0007 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
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 signature.asc Description: PGP signature
Re: [Qemu-devel] pseries-2.6 migration from QEMU-2.6 to QEMU-2.7 broken
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: > > > 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 signature.asc Description: PGP signature
Re: [Qemu-devel] pseries-2.6 migration from QEMU-2.6 to QEMU-2.7 broken
On Thu, Sep 22, 2016 at 02:34:19PM +0530, Nikunj A Dadhania wrote: > Benjamin Herrenschmidt 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 signature.asc Description: PGP signature
Re: [Qemu-devel] pseries-2.6 migration from QEMU-2.6 to QEMU-2.7 broken
Cédric Le Goater writes: > On 09/22/2016 01:07 PM, Nikunj A Dadhania wrote: >> Benjamin Herrenschmidt 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: >> 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
"Dr. David Alan Gilbert" writes: > * Nikunj A Dadhania (nik...@linux.vnet.ibm.com) wrote: >> Benjamin Herrenschmidt 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 (nik...@linux.vnet.ibm.com) wrote: > Benjamin Herrenschmidt 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: > 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 c00795c8 LR d074407c CTR c0079544 XER > CPU#0 > MSR 80009032 HID0 HF 8030 iidx 1 didx 1 > TB 0007 32202510341 DECR 00596259 > > Regards, > Nikunj > > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] pseries-2.6 migration from QEMU-2.6 to QEMU-2.7 broken
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: > > 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 c00795c8 LR d074407c CTR c0079544 XER > > CPU#0 > > MSR 80009032 HID0 HF 8030 > > iidx 1 didx 1 > > TB 0007 32202510341 DECR 00596259 > > > > Regards, > > Nikunj > >
Re: [Qemu-devel] pseries-2.6 migration from QEMU-2.6 to QEMU-2.7 broken
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
On 09/22/2016 01:07 PM, Nikunj A Dadhania wrote: > Benjamin Herrenschmidt 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: > 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 c00795c8 LR d074407c CTR c0079544 XER > CPU#0 > MSR 80009032 HID0 HF 8030 iidx 1 didx 1 > TB 0007 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" 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
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
Benjamin Herrenschmidt 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: 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 c00795c8 LR d074407c CTR c0079544 XER CPU#0 MSR 80009032 HID0 HF 8030 iidx 1 didx 1 TB 0007 32202510341 DECR 00596259 Regards, Nikunj
Re: [Qemu-devel] pseries-2.6 migration from QEMU-2.6 to QEMU-2.7 broken
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
On 22/09/16 19:04, Nikunj A Dadhania wrote: > Benjamin Herrenschmidt 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 (nik...@linux.vnet.ibm.com) wrote: > "Dr. David Alan Gilbert" writes: > > > * Bharata B Rao (bhar...@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 / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] pseries-2.6 migration from QEMU-2.6 to QEMU-2.7 broken
"Dr. David Alan Gilbert" writes: > * Bharata B Rao (bhar...@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
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
* Bharata B Rao (bhar...@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 / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] pseries-2.6 migration from QEMU-2.6 to QEMU-2.7 broken
Benjamin Herrenschmidt 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
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
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
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
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
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
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 signature.asc Description: PGP signature
[Qemu-devel] pseries-2.6 migration from QEMU-2.6 to QEMU-2.7 broken
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.