| Message ID | 1496230005-12265-2-git-send-email-bharata@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 31, 2017 at 04:56:44PM +0530, Bharata B Rao wrote: > Add a "no HPT" encoding (using value -1) to the HTAB migration > stream (in the place of HPT size) when the guest doesn't allocate HPT. > This will help the target side to match target HPT with the source HPT > and thus enable successful migration. > > A few more fixes to enable TCG migration to work correctly are also > included in this commit: > > - HTAB savevm handlers have a few asserts on kvm_enabled() when > spapr->htab != 0. Convert these into conditional checks as it is now > possible to have no HTAB with TCG radix guests. > - htab_save_setup() asserts for kvm_enabled() when spapr->htab != 0. > Remove this as we can't assert this for TCG radix guests. > > Suggested-by: David Gibson <david@gibson.dropbear.id.au> > [no HPT encoding suggestion] > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> Looks basically ok, but there are still some details to address. > --- > hw/ppc/spapr.c | 31 +++++++++++++++++-------------- > 1 file changed, 17 insertions(+), 14 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index ab3aab1..b589ed4 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1559,17 +1559,18 @@ static int htab_save_setup(QEMUFile *f, void *opaque) > { > sPAPRMachineState *spapr = opaque; > > - /* "Iteration" header */ > - qemu_put_be32(f, spapr->htab_shift); > + /* "Iteration" header: no-HPT or HPT size encoding */ > + if (!spapr->htab_shift) { > + qemu_put_be32(f, -1); We're already using htab_shift == 0 to represent no HPT in the runtime structure; we might as well do the same on the wire. As a bonus it slightly simplifies the logic here. > + } else { > + qemu_put_be32(f, spapr->htab_shift); > + } > > if (spapr->htab) { > spapr->htab_save_index = 0; > spapr->htab_first_pass = true; > - } else { > - assert(kvm_enabled()); I think you've oversimplified the assert()s a little. As above we're using htab_shift != 0 to canonically indicate the presence of an HPT, whether it's qemu or kernel managed. So really the new assert is if (spapr->htab_shift) { assert(spapr->htab || kvm_enabled()); } (which can be simplified depending on context, of course). > } > > - > return 0; > } > > @@ -1714,9 +1715,7 @@ static int htab_save_iterate(QEMUFile *f, void *opaque) > /* Iteration header */ > qemu_put_be32(f, 0); > > - if (!spapr->htab) { > - assert(kvm_enabled()); > - > + if (!spapr->htab && kvm_enabled()) { This isn't quite right. !spapr->htab means one of two things: there is no HPT (radix mode) or the HPT is kernel-managed. kvm_enabled() is not enough to discern the latter. In radix mode with KVM you'd execute this block, which I don't think you want. > fd = get_htab_fd(spapr); > if (fd < 0) { > return fd; > @@ -1748,7 +1747,7 @@ static int htab_save_complete(QEMUFile *f, void *opaque) > /* Iteration header */ > qemu_put_be32(f, 0); > > - if (!spapr->htab) { > + if (!spapr->htab && kvm_enabled()) { Same here. > int rc; > > assert(kvm_enabled()); > @@ -1793,6 +1792,12 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id) > if (section_hdr) { > Error *local_err = NULL; > > + if (section_hdr == -1) { > + spapr_free_hpt(spapr); > + unregister_savevm(NULL, "spapr/htab", spapr); I don't think we want to register/unregister the htab handlers at all. This above would break if you migrated a radix guest, then rebooted into a hash guest and tried to migrate again. > + return 0; > + } > + > /* First section gives the htab size */ > spapr_reallocate_hpt(spapr, section_hdr, &local_err); > if (local_err) { > @@ -1802,9 +1807,7 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id) > return 0; > } > > - if (!spapr->htab) { > - assert(kvm_enabled()); > - > + if (!spapr->htab && kvm_enabled()) { Again, not quite right here. > fd = kvmppc_get_htab_fd(true); > if (fd < 0) { > error_report("Unable to open fd to restore KVM hash table: %s", > @@ -1843,7 +1846,7 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id) > memset(HPTE(spapr->htab, index + n_valid), 0, > HASH_PTE_SIZE_64 * n_invalid); > } > - } else { > + } else if (kvm_enabled()) { > int rc; > > assert(fd >= 0); > @@ -1855,7 +1858,7 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id) > } > } > > - if (!spapr->htab) { > + if (!spapr->htab && kvm_enabled()) { Or here. > assert(fd >= 0); > close(fd); > }
On Thu, Jun 01, 2017 at 02:54:48PM +1000, David Gibson wrote: > On Wed, May 31, 2017 at 04:56:44PM +0530, Bharata B Rao wrote: > > Add a "no HPT" encoding (using value -1) to the HTAB migration > > stream (in the place of HPT size) when the guest doesn't allocate HPT. > > This will help the target side to match target HPT with the source HPT > > and thus enable successful migration. > > > > A few more fixes to enable TCG migration to work correctly are also > > included in this commit: > > > > - HTAB savevm handlers have a few asserts on kvm_enabled() when > > spapr->htab != 0. Convert these into conditional checks as it is now > > possible to have no HTAB with TCG radix guests. > > - htab_save_setup() asserts for kvm_enabled() when spapr->htab != 0. > > Remove this as we can't assert this for TCG radix guests. > > > > Suggested-by: David Gibson <david@gibson.dropbear.id.au> > > [no HPT encoding suggestion] > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > Looks basically ok, but there are still some details to address. > > > --- > > hw/ppc/spapr.c | 31 +++++++++++++++++-------------- > > 1 file changed, 17 insertions(+), 14 deletions(-) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index ab3aab1..b589ed4 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -1559,17 +1559,18 @@ static int htab_save_setup(QEMUFile *f, void *opaque) > > { > > sPAPRMachineState *spapr = opaque; > > > > - /* "Iteration" header */ > > - qemu_put_be32(f, spapr->htab_shift); > > + /* "Iteration" header: no-HPT or HPT size encoding */ > > + if (!spapr->htab_shift) { > > + qemu_put_be32(f, -1); > > We're already using htab_shift == 0 to represent no HPT in the runtime > structure; we might as well do the same on the wire. As a bonus it > slightly simplifies the logic here. Non-zero value of iteration header (which is htab_shift) results in htab_load() at the target to reallocate HTAB. zero value of iteration header is used by htab_save_iterate() and htab_save_complete() to tell htab_load() not to freshly allocate HTAB at the target. Hence we can't use 0 value to mean no-HPT. I have addressed the rest of the comments on asserts by ensuring that those code paths are taken only when HPT is present. v5 has those changes. Regards, Bharata.
On Wed, Jun 07, 2017 at 01:23:32PM +0530, Bharata B Rao wrote: > On Thu, Jun 01, 2017 at 02:54:48PM +1000, David Gibson wrote: > > On Wed, May 31, 2017 at 04:56:44PM +0530, Bharata B Rao wrote: > > > Add a "no HPT" encoding (using value -1) to the HTAB migration > > > stream (in the place of HPT size) when the guest doesn't allocate HPT. > > > This will help the target side to match target HPT with the source HPT > > > and thus enable successful migration. > > > > > > A few more fixes to enable TCG migration to work correctly are also > > > included in this commit: > > > > > > - HTAB savevm handlers have a few asserts on kvm_enabled() when > > > spapr->htab != 0. Convert these into conditional checks as it is now > > > possible to have no HTAB with TCG radix guests. > > > - htab_save_setup() asserts for kvm_enabled() when spapr->htab != 0. > > > Remove this as we can't assert this for TCG radix guests. > > > > > > Suggested-by: David Gibson <david@gibson.dropbear.id.au> > > > [no HPT encoding suggestion] > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > Looks basically ok, but there are still some details to address. > > > > > --- > > > hw/ppc/spapr.c | 31 +++++++++++++++++-------------- > > > 1 file changed, 17 insertions(+), 14 deletions(-) > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > index ab3aab1..b589ed4 100644 > > > --- a/hw/ppc/spapr.c > > > +++ b/hw/ppc/spapr.c > > > @@ -1559,17 +1559,18 @@ static int htab_save_setup(QEMUFile *f, void *opaque) > > > { > > > sPAPRMachineState *spapr = opaque; > > > > > > - /* "Iteration" header */ > > > - qemu_put_be32(f, spapr->htab_shift); > > > + /* "Iteration" header: no-HPT or HPT size encoding */ > > > + if (!spapr->htab_shift) { > > > + qemu_put_be32(f, -1); > > > > We're already using htab_shift == 0 to represent no HPT in the runtime > > structure; we might as well do the same on the wire. As a bonus it > > slightly simplifies the logic here. > > Non-zero value of iteration header (which is htab_shift) results in > htab_load() at the target to reallocate HTAB. Yeah.. but you can change that, right. Have htab_load() remove the HPT instead, if section_hdr == 0. Older qemus that don't understand that certainly weren't going to be able to take an incoming RPT guest anyway. > zero value of iteration header is used by htab_save_iterate() and > htab_save_complete() to tell htab_load() not to freshly allocate HTAB > at the target. But that makes no sense anyway. How the destination goes about allocating or not allocating the HPT shouldn't be the choice of the source. Since the source doesn't know what the destination has already allocated, it can't possibly know if it's of a matching size, likewise if the destination doesn't get a size, it can't know if the current size is correct. > Hence we can't use 0 value to mean no-HPT. I really think we can.. > I have addressed the rest of the comments on asserts by ensuring that > those code paths are taken only when HPT is present. v5 has those > changes. > > Regards, > Bharata. >
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index ab3aab1..b589ed4 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -1559,17 +1559,18 @@ static int htab_save_setup(QEMUFile *f, void *opaque) { sPAPRMachineState *spapr = opaque; - /* "Iteration" header */ - qemu_put_be32(f, spapr->htab_shift); + /* "Iteration" header: no-HPT or HPT size encoding */ + if (!spapr->htab_shift) { + qemu_put_be32(f, -1); + } else { + qemu_put_be32(f, spapr->htab_shift); + } if (spapr->htab) { spapr->htab_save_index = 0; spapr->htab_first_pass = true; - } else { - assert(kvm_enabled()); } - return 0; } @@ -1714,9 +1715,7 @@ static int htab_save_iterate(QEMUFile *f, void *opaque) /* Iteration header */ qemu_put_be32(f, 0); - if (!spapr->htab) { - assert(kvm_enabled()); - + if (!spapr->htab && kvm_enabled()) { fd = get_htab_fd(spapr); if (fd < 0) { return fd; @@ -1748,7 +1747,7 @@ static int htab_save_complete(QEMUFile *f, void *opaque) /* Iteration header */ qemu_put_be32(f, 0); - if (!spapr->htab) { + if (!spapr->htab && kvm_enabled()) { int rc; assert(kvm_enabled()); @@ -1793,6 +1792,12 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id) if (section_hdr) { Error *local_err = NULL; + if (section_hdr == -1) { + spapr_free_hpt(spapr); + unregister_savevm(NULL, "spapr/htab", spapr); + return 0; + } + /* First section gives the htab size */ spapr_reallocate_hpt(spapr, section_hdr, &local_err); if (local_err) { @@ -1802,9 +1807,7 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id) return 0; } - if (!spapr->htab) { - assert(kvm_enabled()); - + if (!spapr->htab && kvm_enabled()) { fd = kvmppc_get_htab_fd(true); if (fd < 0) { error_report("Unable to open fd to restore KVM hash table: %s", @@ -1843,7 +1846,7 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id) memset(HPTE(spapr->htab, index + n_valid), 0, HASH_PTE_SIZE_64 * n_invalid); } - } else { + } else if (kvm_enabled()) { int rc; assert(fd >= 0); @@ -1855,7 +1858,7 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id) } } - if (!spapr->htab) { + if (!spapr->htab && kvm_enabled()) { assert(fd >= 0); close(fd); }
Add a "no HPT" encoding (using value -1) to the HTAB migration stream (in the place of HPT size) when the guest doesn't allocate HPT. This will help the target side to match target HPT with the source HPT and thus enable successful migration. A few more fixes to enable TCG migration to work correctly are also included in this commit: - HTAB savevm handlers have a few asserts on kvm_enabled() when spapr->htab != 0. Convert these into conditional checks as it is now possible to have no HTAB with TCG radix guests. - htab_save_setup() asserts for kvm_enabled() when spapr->htab != 0. Remove this as we can't assert this for TCG radix guests. Suggested-by: David Gibson <david@gibson.dropbear.id.au> [no HPT encoding suggestion] Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> --- hw/ppc/spapr.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)