Re: [PATCH v2 07/13] migration: Hack to maintain backwards compatibility for ppc

["Nicholas Piggin" <npiggin@gmail.com>]

On Fri Oct 20, 2023 at 7:07 PM AEST, Juan Quintela wrote:
> Current code does:
> - register pre_2_10_vmstate_dummy_icp with "icp/server" and instance
>   dependinfg on cpu number
> - for newer machines, it register vmstate_icp with "icp/server" name
>   and instance 0
> - now it unregisters "icp/server" for the 1st instance.
>
> This is wrong at many levels:
> - we shouldn't have two VMSTATEDescriptions with the same name
> - In case this is the only solution that we can came with, it needs to
>   be:
>   * register pre_2_10_vmstate_dummy_icp
>   * unregister pre_2_10_vmstate_dummy_icp
>   * register real vmstate_icp
>
> Created vmstate_replace_hack_for_ppc() with warnings left and right
> that it is a hack.

Thanks. We'll look at deprecating 2.9 soon so this can all be removed.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

>
> CC: Cedric Le Goater <clg@kaod.org>
> CC: Daniel Henrique Barboza <danielhb413@gmail.com>
> CC: David Gibson <david@gibson.dropbear.id.au>
> CC: Greg Kurz <groug@kaod.org>
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  include/migration/vmstate.h | 11 +++++++++++
>  hw/intc/xics.c              | 17 +++++++++++++++--
>  hw/ppc/spapr.c              | 25 +++++++++++++++++++++++--
>  migration/savevm.c          | 18 ++++++++++++++++++
>  4 files changed, 67 insertions(+), 4 deletions(-)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 9ca7e9cc48..65deaecc92 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -1230,6 +1230,17 @@ static inline int vmstate_register(VMStateIf *obj, int instance_id,
>                                            opaque, -1, 0, NULL);
>  }
>  
> +/**
> + * vmstate_replace_hack_for_ppc() - ppc used to abuse vmstate_register
> + *
> + * Don't even think about using this function in new code.
> + *
> + * Returns: 0 on success, -1 on failure
> + */
> +int vmstate_replace_hack_for_ppc(VMStateIf *obj, int instance_id,
> +                                 const VMStateDescription *vmsd,
> +                                 void *opaque);
> +
>  /**
>   * vmstate_register_any() - legacy function to register state
>   * serialisation description and let the function choose the id
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index c7f8abd71e..a03979e72a 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -335,8 +335,21 @@ static void icp_realize(DeviceState *dev, Error **errp)
>              return;
>          }
>      }
> -
> -    vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
> +    /*
> +     * The way that pre_2_10_icp is handling is really, really hacky.
> +     * We used to have here this call:
> +     *
> +     * vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
> +     *
> +     * But we were doing:
> +     *     pre_2_10_vmstate_register_dummy_icp()
> +     *     this vmstate_register()
> +     *     pre_2_10_vmstate_unregister_dummy_icp()
> +     *
> +     * So for a short amount of time we had to vmstate entries with
> +     * the same name.  This fixes it.
> +     */
> +    vmstate_replace_hack_for_ppc(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
>  }
>  
>  static void icp_unrealize(DeviceState *dev)
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index cb840676d3..a75cf475ad 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -143,6 +143,11 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
>  }
>  
>  static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
> +    /*
> +     * Hack ahead.  We can't have two devices with the same name and
> +     * instance id.  So I rename this to pass make check.
> +     * Real help from people who knows the hardware is needed.
> +     */
>      .name = "icp/server",
>      .version_id = 1,
>      .minimum_version_id = 1,
> @@ -155,16 +160,32 @@ static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
>      },
>  };
>  
> +/*
> + * See comment in hw/intc/xics.c:icp_realize()
> + *
> + * You have to remove vmstate_replace_hack_for_ppc() when you remove
> + * the machine types that need the following function.
> + */
>  static void pre_2_10_vmstate_register_dummy_icp(int i)
>  {
>      vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp,
>                       (void *)(uintptr_t) i);
>  }
>  
> +/*
> + * See comment in hw/intc/xics.c:icp_realize()
> + *
> + * You have to remove vmstate_replace_hack_for_ppc() when you remove
> + * the machine types that need the following function.
> + */
>  static void pre_2_10_vmstate_unregister_dummy_icp(int i)
>  {
> -    vmstate_unregister(NULL, &pre_2_10_vmstate_dummy_icp,
> -                       (void *)(uintptr_t) i);
> +    /*
> +     * This used to be:
> +     *
> +     *    vmstate_unregister(NULL, &pre_2_10_vmstate_dummy_icp,
> +     * 	                    (void *)(uintptr_t) i);
> +     */
>  }
>  
>  int spapr_max_server_number(SpaprMachineState *spapr)
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 8622f229e5..d3a30686d4 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -846,6 +846,24 @@ static void vmstate_check(const VMStateDescription *vmsd)
>      }
>  }
>  
> +/*
> + * See comment in hw/intc/xics.c:icp_realize()
> + *
> + * This function can be removed when
> + * pre_2_10_vmstate_register_dummy_icp() is removed.
> + */
> +int vmstate_replace_hack_for_ppc(VMStateIf *obj, int instance_id,
> +                                 const VMStateDescription *vmsd,
> +                                 void *opaque)
> +{
> +    SaveStateEntry *se = find_se(vmsd->name, instance_id);
> +
> +    if (se) {
> +        savevm_state_handler_remove(se);
> +    }
> +    return vmstate_register(obj, instance_id, vmsd, opaque);
> +}
> +
>  int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id,
>                                     const VMStateDescription *vmsd,
>                                     void *opaque, int alias_id,