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

[Daniel Henrique Barboza]

On 5/25/20 7:38 AM, Jiri Denemark wrote:
> 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 at redhat.com>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413 at gmail.com>
>> ---
>>   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;
> 

Got it. I'll also update the commit msg to mention that the problem resides in
doing the proper handling in qemuProcessRefreshCPU(), and that your patch in
qemuProcessUpdateCPU() I mentioned there simply exposed the problem.



Thanks,

DHB


>          ...
> 
> Jirka
>