Re: [Qemu-devel] [PATCH 2/4] spapr: introduce helpers to migrate HPT chunks and the end marker

[David Gibson]

On Fri, Sep 15, 2017 at 03:16:01PM +0200, Greg Kurz wrote:
> This consolidates some duplicated code in a single helper.
> 
> Signed-off-by: Greg Kurz <address@hidden>
> ---
>  hw/ppc/spapr.c |   38 +++++++++++++++++++++-----------------
>  1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f680f28a15ea..841117f6d185 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1708,6 +1708,23 @@ static int htab_save_setup(QEMUFile *f, void *opaque)
>      return 0;
>  }
>  
> +static void htab_save_chunk(QEMUFile *f, sPAPRMachineState *spapr,
> +                            int chunkstart, int n_valid, int n_invalid)
> +{
> +    qemu_put_be32(f, chunkstart);
> +    qemu_put_be16(f, n_valid);
> +    qemu_put_be16(f, n_invalid);
> +    if (spapr) {

I like the basic idea, but passing NULL for spapr is *only* valid for
the end marker case.  The general test here is misleading.  I'd prefer
to either see the writes opencoded for the end marker, or have an
assert here (spapr can only be NULL if n_valid == n_invalid == 0).

> +        qemu_put_buffer(f, HPTE(spapr->htab, chunkstart),
> +                        HASH_PTE_SIZE_64 * n_valid);
> +    }
> +}
> +
> +static void htab_save_end_marker(QEMUFile *f)
> +{
> +    htab_save_chunk(f, NULL, 0, 0, 0);
> +}
> +
>  static void htab_save_first_pass(QEMUFile *f, sPAPRMachineState *spapr,
>                                   int64_t max_ns)
>  {
> @@ -1739,11 +1756,7 @@ static void htab_save_first_pass(QEMUFile *f, 
> sPAPRMachineState *spapr,
>          if (index > chunkstart) {
>              int n_valid = index - chunkstart;
>  
> -            qemu_put_be32(f, chunkstart);
> -            qemu_put_be16(f, n_valid);
> -            qemu_put_be16(f, 0);
> -            qemu_put_buffer(f, HPTE(spapr->htab, chunkstart),
> -                            HASH_PTE_SIZE_64 * n_valid);
> +            htab_save_chunk(f, spapr, chunkstart, n_valid, 0);
>  
>              if (has_timeout &&
>                  (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - starttime) > 
> max_ns) {
> @@ -1805,11 +1818,7 @@ static int htab_save_later_pass(QEMUFile *f, 
> sPAPRMachineState *spapr,
>              int n_valid = invalidstart - chunkstart;
>              int n_invalid = index - invalidstart;
>  
> -            qemu_put_be32(f, chunkstart);
> -            qemu_put_be16(f, n_valid);
> -            qemu_put_be16(f, n_invalid);
> -            qemu_put_buffer(f, HPTE(spapr->htab, chunkstart),
> -                            HASH_PTE_SIZE_64 * n_valid);
> +            htab_save_chunk(f, spapr, chunkstart, n_valid, n_invalid);
>              sent += index - chunkstart;
>  
>              if (!final && (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - 
> starttime) > max_ns) {
> @@ -1872,10 +1881,7 @@ static int htab_save_iterate(QEMUFile *f, void *opaque)
>          rc = htab_save_later_pass(f, spapr, MAX_ITERATION_NS);
>      }
>  
> -    /* End marker */
> -    qemu_put_be32(f, 0);
> -    qemu_put_be16(f, 0);
> -    qemu_put_be16(f, 0);
> +    htab_save_end_marker(f);
>  
>      return rc;
>  }
> @@ -1915,9 +1921,7 @@ static int htab_save_complete(QEMUFile *f, void *opaque)
>      }
>  
>      /* End marker */
> -    qemu_put_be32(f, 0);
> -    qemu_put_be16(f, 0);
> -    qemu_put_be16(f, 0);
> +    htab_save_end_marker(f);
>  
>      return 0;
>  }
> 

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