Greg Kurz <gr...@kaod.org> wrote: > On Fri, 20 Oct 2023 09:30:44 +0200 > Juan Quintela <quint...@redhat.com> wrote: > >> Greg Kurz <gr...@kaod.org> wrote: >> > On Thu, 19 Oct 2023 21:08:25 +0200 >> > Juan Quintela <quint...@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 <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> >> >> --- >> >> 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 <gr...@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.