Re: [PATCH 5/5] qemuProcessUpdateCPU: do not change 'fallback' to ALLOW for pSeries guests

On Fri, May 22, 2020 at 16:56:20 -0300, Daniel Henrique Barboza wrote: > Commit v3.10.0-182-g237f045d9a ("qemu: Ignore fallback CPU attribute > on reconnect") forced CPU 'fallback' to ALLOW, regardless of user > choice. This fixed a situation in which guests created with older > Libvirt versions, which used CPU mode 'host-model' in runtime, would > fail to launch in a newer Libvirt if the fallback was set to FORBID. > This would lead to a scenario where the CPU was translated to 'host-model' > to 'custom', but then the FORBID setting would make the translation > process fail. > > This fix has a side effect for PSeries guests. PSeries can operate > with 'host-model' in runtime due to specific PPC64 mechanics regarding > compatibility mode. In fact, the update() implementation of the > cpuDriverPPC64 driver is a NO-OP if CPU mode is 'host-model', and > the driver does not implement translate(). The result is that PSeries > guests aren't affected by the problem, but they are being affected by > the fix - users are seeing 'fallback' mode being changed without > necessity during daemon restart. > > All other cpuArchDrivers implements update() and changes guest mode > to VIR_CPU_MODE_CUSTOM, meaning that PSeries is currently the only > exception to this logic. Let's make it official. > > https://bugzilla.redhat.com/show_bug.cgi?id=1660711 > > CC: Jiri Denemark <jdenemar(a)redhat.com&gt; > Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; > --- > src/qemu/qemu_process.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index a1ef1d42b0..fec1720f33 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -4347,8 +4347,14 @@ qemuProcessUpdateCPU(virQEMUDriverPtr driver, > > /* The host CPU model comes from host caps rather than QEMU caps so > * fallback must be allowed no matter what the user specified in the XML. > + * > + * Note: PSeries domains are able to run with host-model CPU by design, > + * even on Libvirt newer than 2.3, never replacing host-model with > + * custom in the virCPUUpdate() call prior to this function. It is not > + * needed to change the user defined 'fallback' attribute in this case. > */ > - vm->def->cpu->fallback = VIR_CPU_FALLBACK_ALLOW; > + if (!qemuDomainIsPSeries(vm->def)) > + vm->def->cpu->fallback = VIR_CPU_FALLBACK_ALLOW; > > if (qemuProcessFetchGuestCPU(driver, vm, asyncJob, &cpu, &disabled) < 0) > return -1; qemuProcessUpdateCPU should not be called at all in this case. The caller (qemuProcessRefreshCPU) is supposed to decide whether the guest CPU needs to be changed: /* If the domain with a host-model CPU was started by an old libvirt * (< 2.3) which didn't replace the CPU with a custom one, let's do it now * since the rest of our code does not really expect a host-model CPU in a * running domain. */ if (vm->def->cpu->mode == VIR_CPU_MODE_HOST_MODEL) { if (!(hostmig = virCPUCopyMigratable(host->arch, host))) return -1; if (!(cpu = virCPUDefCopyWithoutModel(hostmig)) || virCPUDefCopyModelFilter(cpu, hostmig, false, virQEMUCapsCPUFilterFeatures, &host->arch) < 0) return -1; if (virCPUUpdate(vm->def->os.arch, vm->def->cpu, cpu) < 0) return -1; if (qemuProcessUpdateCPU(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) return -1; So better solution would be to move your new comment and check just after the check for VIR_CPU_MODE_HOST_MODEL: if (vm->def->cpu->mode == VIR_CPU_MODE_HOST_MODEL) { /* PSeries... */ if (qemuDomainIsPSeries(vm->def)) return 0;

... Jirka