On Fri Oct 20, 2023 at 7:07 PM AEST, Juan Quintela 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 > > Created vmstate_replace_hack_for_ppc() with warnings left and right > that it is a hack.
Thanks. We'll look at deprecating 2.9 soon so this can all be removed. Reviewed-by: Nicholas Piggin <npig...@gmail.com> > > CC: Cedric Le Goater <c...@kaod.org> > CC: Daniel Henrique Barboza <danielhb...@gmail.com> > CC: David Gibson <da...@gibson.dropbear.id.au> > CC: Greg Kurz <gr...@kaod.org> > > Signed-off-by: Juan Quintela <quint...@redhat.com> > --- > include/migration/vmstate.h | 11 +++++++++++ > hw/intc/xics.c | 17 +++++++++++++++-- > hw/ppc/spapr.c | 25 +++++++++++++++++++++++-- > migration/savevm.c | 18 ++++++++++++++++++ > 4 files changed, 67 insertions(+), 4 deletions(-) > > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h > index 9ca7e9cc48..65deaecc92 100644 > --- a/include/migration/vmstate.h > +++ b/include/migration/vmstate.h > @@ -1230,6 +1230,17 @@ static inline int vmstate_register(VMStateIf *obj, int > instance_id, > opaque, -1, 0, NULL); > } > > +/** > + * vmstate_replace_hack_for_ppc() - ppc used to abuse vmstate_register > + * > + * Don't even think about using this function in new code. > + * > + * Returns: 0 on success, -1 on failure > + */ > +int vmstate_replace_hack_for_ppc(VMStateIf *obj, int instance_id, > + const VMStateDescription *vmsd, > + void *opaque); > + > /** > * vmstate_register_any() - legacy function to register state > * serialisation description and let the function choose the id > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > index c7f8abd71e..a03979e72a 100644 > --- a/hw/intc/xics.c > +++ b/hw/intc/xics.c > @@ -335,8 +335,21 @@ static void icp_realize(DeviceState *dev, Error **errp) > return; > } > } > - > - vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp); > + /* > + * The way that pre_2_10_icp is handling is really, really hacky. > + * We used to have here this call: > + * > + * vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp); > + * > + * But we were doing: > + * pre_2_10_vmstate_register_dummy_icp() > + * this vmstate_register() > + * pre_2_10_vmstate_unregister_dummy_icp() > + * > + * So for a short amount of time we had to vmstate entries with > + * the same name. This fixes it. > + */ > + vmstate_replace_hack_for_ppc(NULL, icp->cs->cpu_index, > &vmstate_icp_server, icp); > } > > static void icp_unrealize(DeviceState *dev) > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index cb840676d3..a75cf475ad 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -143,6 +143,11 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void > *opaque) > } > > static const VMStateDescription pre_2_10_vmstate_dummy_icp = { > + /* > + * 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 = "icp/server", > .version_id = 1, > .minimum_version_id = 1, > @@ -155,16 +160,32 @@ static const VMStateDescription > pre_2_10_vmstate_dummy_icp = { > }, > }; > > +/* > + * See comment in hw/intc/xics.c:icp_realize() > + * > + * You have to remove vmstate_replace_hack_for_ppc() when you remove > + * the machine types that need the following function. > + */ > static void pre_2_10_vmstate_register_dummy_icp(int i) > { > vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp, > (void *)(uintptr_t) i); > } > > +/* > + * See comment in hw/intc/xics.c:icp_realize() > + * > + * You have to remove vmstate_replace_hack_for_ppc() when you remove > + * the machine types that need the following function. > + */ > static void pre_2_10_vmstate_unregister_dummy_icp(int i) > { > - vmstate_unregister(NULL, &pre_2_10_vmstate_dummy_icp, > - (void *)(uintptr_t) i); > + /* > + * This used to be: > + * > + * vmstate_unregister(NULL, &pre_2_10_vmstate_dummy_icp, > + * (void *)(uintptr_t) i); > + */ > } > > int spapr_max_server_number(SpaprMachineState *spapr) > diff --git a/migration/savevm.c b/migration/savevm.c > index 8622f229e5..d3a30686d4 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -846,6 +846,24 @@ static void vmstate_check(const VMStateDescription *vmsd) > } > } > > +/* > + * See comment in hw/intc/xics.c:icp_realize() > + * > + * This function can be removed when > + * pre_2_10_vmstate_register_dummy_icp() is removed. > + */ > +int vmstate_replace_hack_for_ppc(VMStateIf *obj, int instance_id, > + const VMStateDescription *vmsd, > + void *opaque) > +{ > + SaveStateEntry *se = find_se(vmsd->name, instance_id); > + > + if (se) { > + savevm_state_handler_remove(se); > + } > + return vmstate_register(obj, instance_id, vmsd, opaque); > +} > + > int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id, > const VMStateDescription *vmsd, > void *opaque, int alias_id,