| Message ID | 20231020090731.28701-8-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: Check for duplicates on vmstate_register() | expand |
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 <npiggin@gmail.com> > > 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> > --- > 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,
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,
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. 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> --- 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(-)