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

[David Gibson]

On Mon, Jun 12, 2017 at 11:02:34AM +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.
> 
> Suggested-by: David Gibson <address@hidden>
> Signed-off-by: Bharata B Rao <address@hidden>
> ---
>  hw/ppc/spapr.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 8b541d9..c425499 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1558,13 +1558,19 @@ static int htab_save_setup(QEMUFile *f, void *opaque)
>      sPAPRMachineState *spapr = opaque;
>  
>      /* "Iteration" header */
> -    qemu_put_be32(f, spapr->htab_shift);
> +    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());
> +        if (spapr->htab_shift) {
> +            assert(kvm_enabled());
> +        }
>      }
>  
>  
> @@ -1710,7 +1716,12 @@ static int htab_save_iterate(QEMUFile *f, void *opaque)
>      int rc = 0;
>  
>      /* Iteration header */
> -    qemu_put_be32(f, 0);
> +    if (!spapr->htab_shift) {
> +        qemu_put_be32(f, -1);
> +        return 0;
> +    } else {
> +        qemu_put_be32(f, 0);
> +    }
>  
>      if (!spapr->htab) {
>          assert(kvm_enabled());
> @@ -1744,7 +1755,12 @@ static int htab_save_complete(QEMUFile *f, void 
> *opaque)
>      int fd;
>  
>      /* Iteration header */
> -    qemu_put_be32(f, 0);
> +    if (!spapr->htab_shift) {
> +        qemu_put_be32(f, -1);
> +        return 0;
> +    } else {
> +        qemu_put_be32(f, 0);
> +    }

Do you actually need the modifications for _iterate and _complete?  I
would have thought you just wouldn't need to send any more of the HPT
stream at all after sending the -1 header.  

We should also adjust the downtime estimation logic so we don't allow
for transferring an HPT that isn't there.

>      if (!spapr->htab) {
>          int rc;
> @@ -1788,6 +1804,11 @@ static int htab_load(QEMUFile *f, void *opaque, int 
> version_id)
>  
>      section_hdr = qemu_get_be32(f);
>  
> +    if (section_hdr == -1) {
> +        spapr_free_hpt(spapr);
> +        return 0;
> +    }

Strictly speaking we probably shouldn't just return here.  We should
wait and see if there is more data in the stream.  Any actual content
(i.e. section_hdr == 0 sections) would be an error at this point.
However, a reset to an HPT guest would be represented by a new
non-zero section header, then more data.  This isn't urgent, but it
would be nice to fix at some point.

>      if (section_hdr) {
>          Error *local_err = NULL;
>  

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