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 <mdr...@linux.vnet.ibm.com>
>
> Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
Reviewed-by: Cedric Le Goater <c...@fr.ibm.com>
> ---
> 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);
>
