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

["Cédric Le Goater" <clg@kaod.org>]

On 10/19/23 22:49, Greg Kurz wrote:
> 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") ?

I'd vote for removing the dummy ICP states for pre-2.10 pseries machines.
Migration compatibility would be broken for these old versions but, with
a clear error report, it should be more than enough. I doubt anyone will
need such a feature now days.

C.


> 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,
> 
> 
>