Re: [PATCH 07/13] RFC migration: icp/server is a mess

[Greg Kurz]

Hi Juan,

On Thu, 19 Oct 2023 21:08:25 +0200
Juan Quintela <quintela@redhat.com> 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.
> 

Heh I remember about this hack... it was caused by some rework in
the interrupt controller that broke migration.

> This is wrong at many levels:
> - we shouldn't have two VMSTATEDescriptions with the same name

I don't know how bad it is. The idea here is to send extra
state in the stream because older QEMU expect it (but won't use
it), so it made sense to keep 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
> 
> As the initialization of this machine is already complex enough, I
> need help from PPC maintainers to fix this.
> 

What about dropping all this code, i.e. basically reverting 46f7afa37096 
("spapr:
fix migration of ICPState objects from/to older QEMU") ?

Unless we still care to migrate pseries machine types from 2017 of
course...

> Volunteers?
> 

Not working on PPC anymore since almost two years, I certainly don't have time,
nor motivation to fix this. I might be able to answer some questions or to
review someone else's patch that gets rid of the offending code, at best.

Cheers,

--
Greg


> 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>
> ---
>  hw/ppc/spapr.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index cb840676d3..8531d13492 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -143,7 +143,12 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void 
> *opaque)
>  }
>  
>  static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
> -    .name = "icp/server",
> +    /*
> +     * 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 = "pre-2.10-icp/server",
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .needed = pre_2_10_vmstate_dummy_icp_needed,



-- 
Greg