Re: [Qemu-devel] [PATCH v4 1/2] spapr: Add a "no HPT" encoding to HTAB migration stream

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

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

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);
>      }

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