Re: [for-5.0 4/4] spapr: Simplify ovec diff

[Cédric Le Goater]

On 29/11/2019 06:33, David Gibson wrote:
> spapr_ovec_diff(ov, old, new) has somewhat complex semantics.  ov is set
> to those bits which are in new but not old, and it returns as a boolean
> whether or not there are any bits in old but not new.
> 
> It turns out that both callers only care about the second, not the first.
> This is basically equivalent to a bitmap subset operation, which is easier
> to understand and implement.  So replace spapr_ovec_diff() with
> spapr_ovec_subset().
> 
> Cc: Mike Roth <address@hidden>
> 
> Signed-off-by: David Gibson <address@hidden>


Reviewed-by: Cedric Le Goater <address@hidden>

> ---
>  hw/ppc/spapr.c              | 14 +++-----------
>  hw/ppc/spapr_hcall.c        |  8 ++------
>  hw/ppc/spapr_ovec.c         | 30 ++++++++++--------------------
>  include/hw/ppc/spapr_ovec.h |  4 +---
>  4 files changed, 16 insertions(+), 40 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 5187f5b0a5..32e1cc1d3f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1840,8 +1840,6 @@ static bool spapr_ov5_cas_needed(void *opaque)
>  {
>      SpaprMachineState *spapr = opaque;
>      SpaprOptionVector *ov5_mask = spapr_ovec_new();
> -    SpaprOptionVector *ov5_legacy = spapr_ovec_new();
> -    SpaprOptionVector *ov5_removed = spapr_ovec_new();
>      bool cas_needed;
>  
>      /* Prior to the introduction of SpaprOptionVector, we had two option
> @@ -1873,17 +1871,11 @@ static bool spapr_ov5_cas_needed(void *opaque)
>      spapr_ovec_set(ov5_mask, OV5_DRCONF_MEMORY);
>      spapr_ovec_set(ov5_mask, OV5_DRMEM_V2);
>  
> -    /* spapr_ovec_diff returns true if bits were removed. we avoid using
> -     * the mask itself since in the future it's possible "legacy" bits may be
> -     * removed via machine options, which could generate a false positive
> -     * that breaks migration.
> -     */
> -    spapr_ovec_intersect(ov5_legacy, spapr->ov5, ov5_mask);
> -    cas_needed = spapr_ovec_diff(ov5_removed, spapr->ov5, ov5_legacy);
> +    /* We need extra information if we have any bits outside the mask
> +     * defined above */
> +    cas_needed = !spapr_ovec_subset(spapr->ov5, ov5_mask);
>  
>      spapr_ovec_cleanup(ov5_mask);
> -    spapr_ovec_cleanup(ov5_legacy);
> -    spapr_ovec_cleanup(ov5_removed);
>  
>      return cas_needed;
>  }
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 0f19be794c..f1799b1b70 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1671,7 +1671,7 @@ static target_ulong 
> h_client_architecture_support(PowerPCCPU *cpu,
>      target_ulong fdt_bufsize = args[2];
>      target_ulong ov_table;
>      uint32_t cas_pvr;
> -    SpaprOptionVector *ov1_guest, *ov5_guest, *ov5_cas_old, *ov5_updates;
> +    SpaprOptionVector *ov1_guest, *ov5_guest, *ov5_cas_old;
>      bool guest_radix;
>      Error *local_err = NULL;
>      bool raw_mode_supported = false;
> @@ -1770,9 +1770,7 @@ static target_ulong 
> h_client_architecture_support(PowerPCCPU *cpu,
>      /* capabilities that have been added since CAS-generated guest reset.
>       * if capabilities have since been removed, generate another reset
>       */
> -    ov5_updates = spapr_ovec_new();
> -    spapr->cas_reboot = spapr_ovec_diff(ov5_updates,
> -                                        ov5_cas_old, spapr->ov5_cas);
> +    spapr->cas_reboot = !spapr_ovec_subset(ov5_cas_old, spapr->ov5_cas);
>      spapr_ovec_cleanup(ov5_cas_old);
>      /* Now that processing is finished, set the radix/hash bit for the
>       * guest if it requested a valid mode; otherwise terminate the boot. */
> @@ -1849,8 +1847,6 @@ static target_ulong 
> h_client_architecture_support(PowerPCCPU *cpu,
>          spapr->fdt_blob = fdt;
>      }
>  
> -    spapr_ovec_cleanup(ov5_updates);
> -
>      if (spapr->cas_reboot) {
>          qemu_system_reset_request(SHUTDOWN_CAUSE_SUBSYSTEM_RESET);
>      }
> diff --git a/hw/ppc/spapr_ovec.c b/hw/ppc/spapr_ovec.c
> index 811fadf143..0ff6d1aeae 100644
> --- a/hw/ppc/spapr_ovec.c
> +++ b/hw/ppc/spapr_ovec.c
> @@ -76,31 +76,21 @@ void spapr_ovec_intersect(SpaprOptionVector *ov,
>      bitmap_and(ov->bitmap, ov1->bitmap, ov2->bitmap, OV_MAXBITS);
>  }
>  
> -/* returns true if options bits were removed, false otherwise */
> -bool spapr_ovec_diff(SpaprOptionVector *ov,
> -                     SpaprOptionVector *ov_old,
> -                     SpaprOptionVector *ov_new)
> +/* returns true if ov1 has a subset of bits in ov2 */
> +bool spapr_ovec_subset(SpaprOptionVector *ov1, SpaprOptionVector *ov2)
>  {
> -    unsigned long *change_mask = bitmap_new(OV_MAXBITS);
> -    unsigned long *removed_bits = bitmap_new(OV_MAXBITS);
> -    bool bits_were_removed = false;
> +    unsigned long *tmp = bitmap_new(OV_MAXBITS);
> +    bool result;
>  
> -    g_assert(ov);
> -    g_assert(ov_old);
> -    g_assert(ov_new);
> -
> -    bitmap_xor(change_mask, ov_old->bitmap, ov_new->bitmap, OV_MAXBITS);
> -    bitmap_and(ov->bitmap, ov_new->bitmap, change_mask, OV_MAXBITS);
> -    bitmap_and(removed_bits, ov_old->bitmap, change_mask, OV_MAXBITS);
> +    g_assert(ov1);
> +    g_assert(ov2);
>  
> -    if (!bitmap_empty(removed_bits, OV_MAXBITS)) {
> -        bits_were_removed = true;
> -    }
> +    bitmap_andnot(tmp, ov1->bitmap, ov2->bitmap, OV_MAXBITS);
> +    result = bitmap_empty(tmp, OV_MAXBITS);
>  
> -    g_free(change_mask);
> -    g_free(removed_bits);
> +    g_free(tmp);
>  
> -    return bits_were_removed;
> +    return result;
>  }
>  
>  void spapr_ovec_cleanup(SpaprOptionVector *ov)
> diff --git a/include/hw/ppc/spapr_ovec.h b/include/hw/ppc/spapr_ovec.h
> index 7891e9caac..2bed517a2b 100644
> --- a/include/hw/ppc/spapr_ovec.h
> +++ b/include/hw/ppc/spapr_ovec.h
> @@ -66,9 +66,7 @@ SpaprOptionVector *spapr_ovec_clone(SpaprOptionVector 
> *ov_orig);
>  void spapr_ovec_intersect(SpaprOptionVector *ov,
>                            SpaprOptionVector *ov1,
>                            SpaprOptionVector *ov2);
> -bool spapr_ovec_diff(SpaprOptionVector *ov,
> -                     SpaprOptionVector *ov_old,
> -                     SpaprOptionVector *ov_new);
> +bool spapr_ovec_subset(SpaprOptionVector *ov1, SpaprOptionVector *ov2);
>  void spapr_ovec_cleanup(SpaprOptionVector *ov);
>  void spapr_ovec_set(SpaprOptionVector *ov, long bitnr);
>  void spapr_ovec_clear(SpaprOptionVector *ov, long bitnr);
>