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

[Juan Quintela]

Greg Kurz <groug@kaod.org> 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 ?
>
>>  And what ppc is
>> doing here is (and has always been) a hack and an abuse about how
>> vmstate registrations is supposed to work.
>> 
>
> Sorry again... We should have involved migration experts at the time. :-)

I would have told you that this can't be done O:-)

Sent another version with a vmstate hack to accomodate this.  You don't
have to deprecate the machines due to migration O:-)

And now that I have ppc gurus attention, could you comment in the other
question:

./hw/ppc/spapr_iommu.c

static void spapr_tce_table_realize(DeviceState *dev, Error **errp)
{
    ...
    vmstate_register(VMSTATE_IF(tcet), tcet->liobn, &vmstate_spapr_tce_table,
                     tcet);
}

./include/hw/ppc/spapr.h

struct SpaprTceTable {
    ...
    uint32_t liobn;
    ....
};

./include/migration.h

static inline int vmstate_register(VMStateIf *obj, int instance_id,
                                   const VMStateDescription *vmsd,
                                   void *opaque);


liobn is an uint32_t and insntance_id is an int.

For this series, I started with this:

static inline int vmstate_register(VMStateIf *obj, int instance_id,
                                   const VMStateDescription *vmsd,
                                   void *opaque)
{
    if (instance_id < 0) {
        error_report("vmstate_register: Invalid device: %s instance_id: %d",
                     vmsd->name, instance_id);
        return -1;
    }
    return vmstate_register_with_alias_id(obj, instance_id, vmsd,
                                          opaque, -1, 0, NULL);
}

And it failed on this.  So I change the test to

    if (instance_id == VM_INSTANCE_ID_ANY) {
       ....
    }

But we are still having troubles with signs here.

Posible actions:
- Look the other side and hope that liobn is never -1.
  (if it is -1, it would become 0, so not really a big trouble).
- change vmstate type to uint32_t and make VM_INSTANCE_ID_ANY to UINT32_MAX
  (exact same problem if liobn happens to be UINT32_MAX)

I have no clue what are the valid values of liobn.  So I am leaning to
just look the other way and do nothing.

Advise?

Thanks, Juan.