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 <[email protected]>
> > ---
> > 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 -- [email protected]
To unsubscribe send an email to [email protected]
