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 <da...@gibson.dropbear.id.au> > [no HPT encoding suggestion] > Signed-off-by: Bharata B Rao <bhar...@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);
> }
--
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
