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

[Juan Quintela]

Greg Kurz <groug@kaod.org> wrote:
> 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.
>> 
>> 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
>> 
>> As the initialization of this machine is already complex enough, I
>> need help from PPC maintainers to fix this.
>> 
>> Volunteers?
>> 
>> 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,
>
> I guess this fix is acceptable as well and a lot simpler than
> reverting the hack actually. Outcome is the same : drop
> compat with pseries-2.9 and older.
>
> Reviewed-by: Greg Kurz <groug@kaod.org>

I fully agree with you here.
The other options given on this thread is deprecate that machines, but I
would like to have this series sooner than 2 releases.  And what ppc is
doing here is (and has always been) a hack and an abuse about how
vmstate registrations is supposed to work.

Thanks, Juan.