On Wed, Apr 17, 2024 at 09:15:39 +0200, Peter Krempa wrote: > On Tue, Apr 16, 2024 at 19:53:25 +0200, Jiri Denemark wrote: > > Ages ago origCPU in domain private data was introduced to provide > > backward compatibility when migrating to an old libvirt, which did not > > support fetching updated CPU definition from QEMU. Thus origCPU will > > contain the original CPU definition before such update. But only if the > > update actually changed anything. Let's always fill origCPU with the > > original definition when starting a domain so that we can rely on it > > being always set, even if it matches the updated definition. > > > > This fixes migration or save operations with custom domain XML after > > commit v10.1.0-88-g14d3517410, which expected origCPU to be always set > > to the CPU definition from inactive XML to check features explicitly > > requested by a user. > > > > https://issues.redhat.com/browse/RHEL-30622 > > > > Signed-off-by: Jiri Denemark <jdenemar@xxxxxxxxxx> > > --- > > src/qemu/qemu_domain.c | 38 +++++++++++++++++++++----------------- > > src/qemu/qemu_process.c | 14 ++------------ > > 2 files changed, 23 insertions(+), 29 deletions(-) > > > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > > index 6b869797a8..ca50d435d2 100644 > > --- a/src/qemu/qemu_domain.c > > +++ b/src/qemu/qemu_domain.c > > @@ -11013,15 +11014,25 @@ qemuDomainUpdateCPU(virDomainObj *vm, > > > > *origCPU = NULL; > > > > - if (!cpu || !vm->def->cpu || > > - !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION) || > > - virCPUDefIsEqual(vm->def->cpu, cpu, false)) > > + if (!vm->def->cpu) > > return 0; > > > > - if (!(cpu = virCPUDefCopy(cpu))) > > + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION)) > > + return 0; > > + > > + /* nothing to do if only topology part of CPU def is used */ > > + if (vm->def->cpu->mode == VIR_CPU_MODE_CUSTOM && !vm->def->cpu->model) > > + return 0; > > + > > + if (cpu) > > + cpu = virCPUDefCopy(cpu); > > + else > > + cpu = virCPUDefCopy(vm->def->cpu); > > + > > + if (!cpu) > > return -1; > > > First two reads were very confusing as this is overwriting the argument > .... > > > > > - VIR_DEBUG("Replacing CPU def with the updated one"); > > + VIR_DEBUG("Replacing CPU definition"); > > > > *origCPU = vm->def->cpu; > > vm->def->cpu = cpu; > > ... just to store it here. At this point it's no longer necessary. You > can first move the old def into 'origCPU' and then directly assign into > the variable to avoid the overwrite for readability. Oh right, virCPUDefCopy can never return NULL. > > > @@ -11064,13 +11075,6 @@ qemuDomainFixupCPUs(virDomainObj *vm, > > !vm->def->cpu->model) > > return 0; > > > > - /* Missing origCPU means QEMU created exactly the same virtual CPU which > > - * we asked for or libvirt was too old to mess up the translation from > > - * host-model. > > - */ > > - if (!*origCPU) > > - return 0; > > This function is called from two places: > > src/qemu/qemu_process.c=qemuProcessStartWithMemoryState(virConnectPtr conn, > src/qemu/qemu_process.c: qemuDomainFixupCPUs(vm, &cookie->cpu) < 0) Oops, nice catch. Can I pretend I added this hunk to check if reviewers pay enough attention? :-) The call is guarded by cookie != NULL, but no check for cookie->cpu is done. I'll drop this hunk. > src/qemu/qemu_process.c=qemuProcessRefreshCPU(virQEMUDriver *driver, > src/qemu/qemu_process.c: if (qemuDomainFixupCPUs(vm, &priv->origCPU) < 0 > > 'priv->origCPU' which is used in the latter is AFAIU filled only in > 'qemuDomainUpdateCPU' called when starting up a qemu process and in > 'qemuDomainObjPrivateXMLParse' when loading a status XML. > > In both cases there are code paths which may leave 'priv->origCPU' to be > NULL. > > The following code will dereference it unconditionally: > > if (virCPUDefFindFeature(*origCPU, "cmt")) { This specific issue would not exist if I didn't forget to update one more place... see below. > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > > index e4bcb628cf..8165c28dbc 100644 > > --- a/src/qemu/qemu_process.c > > +++ b/src/qemu/qemu_process.c > > @@ -4449,19 +4447,11 @@ qemuProcessUpdateLiveGuestCPU(virDomainObj *vm, > > !def->cpu->model)) > > return 0; > > > > - orig = virCPUDefCopy(def->cpu); > > - > > - if ((rc = virCPUUpdateLive(def->os.arch, def->cpu, enabled, disabled)) < 0) { > > + if ((rc = virCPUUpdateLive(def->os.arch, def->cpu, enabled, disabled)) < 0) > > return -1; > > - } else if (rc == 0) { > > - /* Store the original CPU in priv if QEMU changed it and we didn't > > - * get the original CPU via migration, restore, or snapshot revert. > > - */ > > - if (!priv->origCPU && !virCPUDefIsEqual(def->cpu, orig, false)) > > - priv->origCPU = g_steal_pointer(&orig); > > > > + if (rc == 0) > > def->cpu->check = VIR_CPU_CHECK_FULL; > > - } > > This code path can also theoretically be called on a status XML created > by libvirt which didn't unconditionally fill origCPU IIUC. I forgot to make sure the original CPU is also saved when reconnecting to a domain started by older libvirt which did not set it. Jirka _______________________________________________ Devel mailing list -- devel@xxxxxxxxxxxxxxxxx To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
