| Message ID | 20231019190831.20363-8-quintela@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: Check for duplicates on vmstate_register() | expand |
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") ? 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,
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, > > >
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>
On 19/10/2023 23.15, Cédric Le Goater wrote: > 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. In that case: Please also put the pseries-2.1 machine up to pseries-2.9 onto the deprecation list, so that they can finally get removed after two releases. It does not make sense to keep compat machines around if the compatibility is not available anymore. Thomas
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. And what ppc is doing here is (and has always been) a hack and an abuse about how vmstate registrations is supposed to work. Thanks, Juan.
On 10/20/23 07:10, Thomas Huth wrote: > On 19/10/2023 23.15, Cédric Le Goater wrote: >> 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. > > In that case: Please also put the pseries-2.1 machine up to pseries-2.9 onto the deprecation list, so that they can finally get removed after two releases. It does not make sense to keep compat machines around if the compatibility is not available anymore. This would be a really good cleanup for PPC to deprecate pseries-2.1/2.9. We did a few workarounds in that time frame which wouldn't be necessary anymore. Thanks, C.
On Fri Oct 20, 2023 at 7:39 AM AEST, Greg Kurz 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> So the reason we can't have duplicate names registered, aside from it surely going bad if we actually send or receive a stream at the point they are registered, is the duplcate check introduced in patch 9? But before that, this hack does seem to actually work because the duplicate is unregistered right away. If I understand the workaround, there is an asymmetry in the migration sequence in that receiving an unexpected object would cause a failure, but going from newer to older would just skip some "expected" objects and that didn't cause a problem. So you only have to deal with ignoring the unexpected ones going form older to newer. Side question, is it possible to flag the problem of *not* receiving an object that you did expect? That might be a source of bugs too. Anyway, I wonder if we could fix this spapr problem by adding a special case wild card instance matcher to ignore it? It's still a bit hacky but maybe a bit nicer. I don't mind deprecating the machine soon if you want to clear the wildcard hack away soon, but it would be nice to separate the deprecation and removal from the fix, if possible. This patch is not tested but hopefully helps illustrate the idea. Thanks, Nick diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h index 1a31fb7293..8ce03edefa 100644 --- a/include/migration/vmstate.h +++ b/include/migration/vmstate.h @@ -1205,6 +1205,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd, bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque); #define VMSTATE_INSTANCE_ID_ANY -1 +#define VMSTATE_INSTANCE_ID_WILD -2 /* Returns: 0 on success, -1 on failure */ int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id, diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index cb840676d3..2418899dd4 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -155,16 +155,10 @@ static const VMStateDescription pre_2_10_vmstate_dummy_icp = { }, }; -static void pre_2_10_vmstate_register_dummy_icp(int i) +static void pre_2_10_vmstate_register_dummy_icp(void) { - vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp, - (void *)(uintptr_t) i); -} - -static void pre_2_10_vmstate_unregister_dummy_icp(int i) -{ - vmstate_unregister(NULL, &pre_2_10_vmstate_dummy_icp, - (void *)(uintptr_t) i); + vmstate_register(NULL, VMSTATE_INSTANCE_ID_WILD, + &pre_2_10_vmstate_dummy_icp, NULL); } int spapr_max_server_number(SpaprMachineState *spapr) @@ -2665,12 +2659,10 @@ static void spapr_init_cpus(SpaprMachineState *spapr) } if (smc->pre_2_10_has_unused_icps) { - for (i = 0; i < spapr_max_server_number(spapr); i++) { - /* Dummy entries get deregistered when real ICPState objects - * are registered during CPU core hotplug. - */ - pre_2_10_vmstate_register_dummy_icp(i); - } + /* Dummy entries get deregistered when real ICPState objects + * are registered during CPU core hotplug. + */ + pre_2_10_vmstate_register_dummy_icp(); } for (i = 0; i < possible_cpus->len; i++) { @@ -3873,21 +3865,9 @@ void spapr_core_release(DeviceState *dev) static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev) { MachineState *ms = MACHINE(hotplug_dev); - SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms); CPUCore *cc = CPU_CORE(dev); CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL); - if (smc->pre_2_10_has_unused_icps) { - SpaprCpuCore *sc = SPAPR_CPU_CORE(OBJECT(dev)); - int i; - - for (i = 0; i < cc->nr_threads; i++) { - CPUState *cs = CPU(sc->threads[i]); - - pre_2_10_vmstate_register_dummy_icp(cs->cpu_index); - } - } - assert(core_slot); core_slot->cpu = NULL; qdev_unrealize(dev); @@ -3968,10 +3948,8 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev) { SpaprMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev)); MachineClass *mc = MACHINE_GET_CLASS(spapr); - SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc); SpaprCpuCore *core = SPAPR_CPU_CORE(OBJECT(dev)); CPUCore *cc = CPU_CORE(dev); - CPUState *cs; SpaprDrc *drc; CPUArchId *core_slot; int index; @@ -4018,13 +3996,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev) &error_abort); } } - - if (smc->pre_2_10_has_unused_icps) { - for (i = 0; i < cc->nr_threads; i++) { - cs = CPU(core->threads[i]); - pre_2_10_vmstate_unregister_dummy_icp(cs->cpu_index); - } - } } static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, diff --git a/migration/savevm.c b/migration/savevm.c index 497ce02bd7..f33449e208 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -989,6 +989,10 @@ static int vmstate_save(QEMUFile *f, SaveStateEntry *se, JSONWriter *vmdesc) trace_savevm_section_skip(se->idstr, se->section_id); return 0; } + if (se->instance_id == VMSTATE_INSTANCE_ID_WILD) { + warn_report("Wildcard vmstate entry must set needed=false"); + return 0; + } trace_savevm_section_start(se->idstr, se->section_id); save_section_header(f, se, QEMU_VM_SECTION_FULL); @@ -1731,13 +1735,16 @@ int qemu_save_device_state(QEMUFile *f) static SaveStateEntry *find_se(const char *idstr, uint32_t instance_id) { + SaveStateEntry *se_wild = NULL; SaveStateEntry *se; QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { - if (!strcmp(se->idstr, idstr) && - (instance_id == se->instance_id || - instance_id == se->alias_id)) - return se; + if (!strcmp(se->idstr, idstr)) { + if (instance_id == se->instance_id || instance_id == se->alias_id) + return se; + if (se->instance_id == VMSTATE_INSTANCE_ID_WILD) + se_wild = se; + } /* Migrating from an older version? */ if (strstr(se->idstr, idstr) && se->compat) { if (!strcmp(se->compat->idstr, idstr) && @@ -1746,7 +1753,7 @@ static SaveStateEntry *find_se(const char *idstr, uint32_t instance_id) return se; } } - return NULL; + return se_wild; } enum LoadVMExitCodes {
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. :-) > Thanks, Juan. > Cheers,
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
"Nicholas Piggin" <npiggin@gmail.com> wrote: > On Fri Oct 20, 2023 at 7:39 AM AEST, Greg Kurz wrote: >> On Thu, 19 Oct 2023 21:08:25 +0200 >> Juan Quintela <quintela@redhat.com> wrote: > So the reason we can't have duplicate names registered, aside from it > surely going bad if we actually send or receive a stream at the point > they are registered, is the duplcate check introduced in patch 9? But > before that, this hack does seem to actually work because the duplicate > is unregistered right away. You are creating a new general case that has only a single use that you agree it is "hacky" O:-) The problem here is that you haven't made your mind what "ipc/server" means. You want sometimes to mean pre_2_10, sometimes to mean other thing. That is not how this is supposed to work. See my proposed change, it is one line change, and just do the right thing. I know, it breaks backwards compatibility. But for one machine type that people are proposing to deprecate/remove. > If I understand the workaround, there is an asymmetry in the migration > sequence in that receiving an unexpected object would cause a failure, > but going from newer to older would just skip some "expected" objects > and that didn't cause a problem. So you only have to deal with ignoring > the unexpected ones going form older to newer. Ok, found a different workaround. Sending a new version of the series with a different hack that maintains backwards compatibility. Later, Juan.
On Fri, 20 Oct 2023 17:49:38 +1000 "Nicholas Piggin" <npiggin@gmail.com> wrote: > On Fri Oct 20, 2023 at 7:39 AM AEST, Greg Kurz 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> > > So the reason we can't have duplicate names registered, aside from it > surely going bad if we actually send or receive a stream at the point > they are registered, is the duplcate check introduced in patch 9? But > before that, this hack does seem to actually work because the duplicate > is unregistered right away. > Correct. > If I understand the workaround, there is an asymmetry in the migration > sequence in that receiving an unexpected object would cause a failure, > but going from newer to older would just skip some "expected" objects > and that didn't cause a problem. So you only have to deal with ignoring > the unexpected ones going form older to newer. > Correct. > Side question, is it possible to flag the problem of *not* receiving > an object that you did expect? That might be a source of bugs too. > AFAICR we try to only migrate state that differs from reset : the destination cannot really assume it will receive anything for a given device. > Anyway, I wonder if we could fix this spapr problem by adding a special > case wild card instance matcher to ignore it? It's still a bit hacky > but maybe a bit nicer. I don't mind deprecating the machine soon if > you want to clear the wildcard hack away soon, but it would be nice to > separate the deprecation and removal from the fix, if possible. > > This patch is not tested but hopefully helps illustrate the idea. > I'm not sure this will fly with older QEMUs that don't know about VMSTATE_INSTANCE_ID_WILD... but I'll let Juan comment on that. > Thanks, > Nick > Cheers, -- Greg > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h > index 1a31fb7293..8ce03edefa 100644 > --- a/include/migration/vmstate.h > +++ b/include/migration/vmstate.h > @@ -1205,6 +1205,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd, > bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque); > > #define VMSTATE_INSTANCE_ID_ANY -1 > +#define VMSTATE_INSTANCE_ID_WILD -2 > > /* Returns: 0 on success, -1 on failure */ > int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id, > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index cb840676d3..2418899dd4 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -155,16 +155,10 @@ static const VMStateDescription pre_2_10_vmstate_dummy_icp = { > }, > }; > > -static void pre_2_10_vmstate_register_dummy_icp(int i) > +static void pre_2_10_vmstate_register_dummy_icp(void) > { > - vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp, > - (void *)(uintptr_t) i); > -} > - > -static void pre_2_10_vmstate_unregister_dummy_icp(int i) > -{ > - vmstate_unregister(NULL, &pre_2_10_vmstate_dummy_icp, > - (void *)(uintptr_t) i); > + vmstate_register(NULL, VMSTATE_INSTANCE_ID_WILD, > + &pre_2_10_vmstate_dummy_icp, NULL); > } > > int spapr_max_server_number(SpaprMachineState *spapr) > @@ -2665,12 +2659,10 @@ static void spapr_init_cpus(SpaprMachineState *spapr) > } > > if (smc->pre_2_10_has_unused_icps) { > - for (i = 0; i < spapr_max_server_number(spapr); i++) { > - /* Dummy entries get deregistered when real ICPState objects > - * are registered during CPU core hotplug. > - */ > - pre_2_10_vmstate_register_dummy_icp(i); > - } > + /* Dummy entries get deregistered when real ICPState objects > + * are registered during CPU core hotplug. > + */ > + pre_2_10_vmstate_register_dummy_icp(); > } > > for (i = 0; i < possible_cpus->len; i++) { > @@ -3873,21 +3865,9 @@ void spapr_core_release(DeviceState *dev) > static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev) > { > MachineState *ms = MACHINE(hotplug_dev); > - SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms); > CPUCore *cc = CPU_CORE(dev); > CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL); > > - if (smc->pre_2_10_has_unused_icps) { > - SpaprCpuCore *sc = SPAPR_CPU_CORE(OBJECT(dev)); > - int i; > - > - for (i = 0; i < cc->nr_threads; i++) { > - CPUState *cs = CPU(sc->threads[i]); > - > - pre_2_10_vmstate_register_dummy_icp(cs->cpu_index); > - } > - } > - > assert(core_slot); > core_slot->cpu = NULL; > qdev_unrealize(dev); > @@ -3968,10 +3948,8 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev) > { > SpaprMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev)); > MachineClass *mc = MACHINE_GET_CLASS(spapr); > - SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc); > SpaprCpuCore *core = SPAPR_CPU_CORE(OBJECT(dev)); > CPUCore *cc = CPU_CORE(dev); > - CPUState *cs; > SpaprDrc *drc; > CPUArchId *core_slot; > int index; > @@ -4018,13 +3996,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev) > &error_abort); > } > } > - > - if (smc->pre_2_10_has_unused_icps) { > - for (i = 0; i < cc->nr_threads; i++) { > - cs = CPU(core->threads[i]); > - pre_2_10_vmstate_unregister_dummy_icp(cs->cpu_index); > - } > - } > } > > static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > diff --git a/migration/savevm.c b/migration/savevm.c > index 497ce02bd7..f33449e208 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -989,6 +989,10 @@ static int vmstate_save(QEMUFile *f, SaveStateEntry *se, JSONWriter *vmdesc) > trace_savevm_section_skip(se->idstr, se->section_id); > return 0; > } > + if (se->instance_id == VMSTATE_INSTANCE_ID_WILD) { > + warn_report("Wildcard vmstate entry must set needed=false"); > + return 0; > + } > > trace_savevm_section_start(se->idstr, se->section_id); > save_section_header(f, se, QEMU_VM_SECTION_FULL); > @@ -1731,13 +1735,16 @@ int qemu_save_device_state(QEMUFile *f) > > static SaveStateEntry *find_se(const char *idstr, uint32_t instance_id) > { > + SaveStateEntry *se_wild = NULL; > SaveStateEntry *se; > > QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { > - if (!strcmp(se->idstr, idstr) && > - (instance_id == se->instance_id || > - instance_id == se->alias_id)) > - return se; > + if (!strcmp(se->idstr, idstr)) { > + if (instance_id == se->instance_id || instance_id == se->alias_id) > + return se; > + if (se->instance_id == VMSTATE_INSTANCE_ID_WILD) > + se_wild = se; > + } > /* Migrating from an older version? */ > if (strstr(se->idstr, idstr) && se->compat) { > if (!strcmp(se->compat->idstr, idstr) && > @@ -1746,7 +1753,7 @@ static SaveStateEntry *find_se(const char *idstr, uint32_t instance_id) > return se; > } > } > - return NULL; > + return se_wild; > } > > enum LoadVMExitCodes {
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.
On Fri Oct 20, 2023 at 6:33 PM AEST, Greg Kurz wrote: > On Fri, 20 Oct 2023 17:49:38 +1000 > "Nicholas Piggin" <npiggin@gmail.com> wrote: > > > On Fri Oct 20, 2023 at 7:39 AM AEST, Greg Kurz 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> > > > > So the reason we can't have duplicate names registered, aside from it > > surely going bad if we actually send or receive a stream at the point > > they are registered, is the duplcate check introduced in patch 9? But > > before that, this hack does seem to actually work because the duplicate > > is unregistered right away. > > > > Correct. > > > If I understand the workaround, there is an asymmetry in the migration > > sequence in that receiving an unexpected object would cause a failure, > > but going from newer to older would just skip some "expected" objects > > and that didn't cause a problem. So you only have to deal with ignoring > > the unexpected ones going form older to newer. > > > > Correct. > > > Side question, is it possible to flag the problem of *not* receiving > > an object that you did expect? That might be a source of bugs too. > > > > AFAICR we try to only migrate state that differs from reset : the > destination cannot really assume it will receive anything for a > given device. That's true, I guess you could always add some flag yourself if you certainly need something. > > > Anyway, I wonder if we could fix this spapr problem by adding a special > > case wild card instance matcher to ignore it? It's still a bit hacky > > but maybe a bit nicer. I don't mind deprecating the machine soon if > > you want to clear the wildcard hack away soon, but it would be nice to > > separate the deprecation and removal from the fix, if possible. > > > > This patch is not tested but hopefully helps illustrate the idea. > > > > I'm not sure this will fly with older QEMUs that don't know about > VMSTATE_INSTANCE_ID_WILD... but I'll let Juan comment on that. You could be right about that. He gave a simpler solution now anyway. Thanks, Nick
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,
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(-)