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

[Thomas Huth <thuth@redhat.com>]

On 20/10/2023 10.06, Greg Kurz wrote:
> On Fri, 20 Oct 2023 09:30:44 +0200
> Juan Quintela <quintela@redhat.com> wrote:
> 
>> 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.
> 
> Yeah and, especially, the deprecation of all these machine types is
> itself a massive chunk of work as it will call to identify and
> remove other related workarounds as well. Given that pretty much
> everyone working in PPC/PAPR moved away, can the community handle
> such a big change ?

I think you could treat that as two work items. First the deprecation and 
removal of old machine types. Second the (optional) cleanups. If we don't 
immediately manage to find and remove each and every possible spot that 
could be cleaned up after the removal of the machine types, so be it. But 
better at least *start* to remove the old cruft, beginning with the machine 
type, than dragging this stuff along forever.

  Thomas