On 17/01/2020 12:49, Greg Kurz wrote:
> On Wed, 15 Jan 2020 19:26:18 +0100
> Laurent Vivier <lviv...@redhat.com> wrote:
>
>> On 15/01/2020 19:10, Laurent Vivier wrote:
>>> Hi,
>>>
>>> On 15/01/2020 18:48, Greg Kurz wrote:
>>>> Migration can potentially race with CAS reboot. If the migration thread
>>>> completes migration after CAS has set spapr->cas_reboot but before the
>>>> mainloop could pick up the reset request and reset the machine, the
>>>> guest is migrated unrebooted and the destination doesn't reboot it
>>>> either because it isn't aware a CAS reboot was needed (eg, because a
>>>> device was added before CAS). This likely result in a broken or hung
>>>> guest.
>>>>
>>>> Even if it is small, the window between CAS and CAS reboot is enough to
>>>> re-qualify spapr->cas_reboot as state that we should migrate. Add a new
>>>> subsection for that and always send it when a CAS reboot is pending.
>>>> This may cause migration to older QEMUs to fail but it is still better
>>>> than end up with a broken guest.
>>>>
>>>> The destination cannot honour the CAS reboot request from a post load
>>>> handler because this must be done after the guest is fully restored.
>>>> It is thus done from a VM change state handler.
>>>>
>>>> Reported-by: Lukáš Doktor <ldok...@redhat.com>
>>>> Signed-off-by: Greg Kurz <gr...@kaod.org>
>>>> ---
>>>>
>>>
>>> I'm wondering if the problem can be related with the fact that
>>> main_loop_should_exit() could release qemu_global_mutex in
>>> pause_all_vcpus() in the reset case?
>>>
>>> 1602 static bool main_loop_should_exit(void)
>>> 1603 {
>>> ...
>>> 1633 request = qemu_reset_requested();
>>> 1634 if (request) {
>>> 1635 pause_all_vcpus();
>>> 1636 qemu_system_reset(request);
>>> 1637 resume_all_vcpus();
>>> 1638 if (!runstate_check(RUN_STATE_RUNNING) &&
>>> 1639 !runstate_check(RUN_STATE_INMIGRATE)) {
>>> 1640 runstate_set(RUN_STATE_PRELAUNCH);
>>> 1641 }
>>> 1642 }
>>> ...
>>>
>>> I already sent a patch for this kind of problem (in current Juan pull
>>> request):
>>>
>>> "runstate: ignore finishmigrate -> prelaunch transition"
>>>
>>> but I don't know if it could fix this one.
>>
>> I think it should be interesting to have the state transition on source
>> and destination when the problem occurs (with something like "-trace
>> runstate_set").
>>
>
> With "-serial mon:stdio -trace runstate_set -trace -trace guest_cpu_reset" :
>
> OF stdout device is: /vdevice/vty@71000000
> Preparing to boot Linux version 4.18.0-80.el8.ppc64le
> (mockbu...@ppc-061.build.eng.bos.redhat.com) (gcc version 8.2.1 20180905 (Red
> Hat 8.2.1-3) (GCC)) #1 SMP Wed Mar 13 11:26:21 UTC 2019
> Detected machine type: 0000000000000101
> command line: BOOT_IMAGE=/boot/vmlinuz-4.18.0-80.el8.ppc64le
> root=UUID=012b83a5-2594-48ac-b936-12fec7cdbb9a ro console=ttyS0
> console=ttyS0,115200n8 no_timer_check net.ifnames=0 crashkernel=auto
> Max number of cores passed to firmware: 2048 (NR_CPUS = 2048)
> Calling ibm,client-architecture-support.
>
> Migration starts here.
>
> ..qemu-system-ppc64: warning: kernel_irqchip allowed but unavailable:
> IRQ_XIVE capability must be present for KVM
> Falling back to kernel-irqchip=off
>
> This ^^ indicates that CAS was called and switched to XIVE, for which
> we lack proper KVM support on GA boston machines.
>
> 23348@1579260982.315795:runstate_set current_run_state 9 (running) new_state
> 7 (finish-migrate)
> 23348@1579260982.360821:runstate_set current_run_state 7 (finish-migrate)
> new_state 5 (postmigrate)
>
> The migration thread is holding the global QEMU mutex at this point. It
> has stopped all CPUs. It now streams the full state to the destination
> before releasing the mutex.
>
> 23340@1579260982.797279:guest_cpu_reset cpu=0xf9dbb48a5e0
> 23340@1579260982.797319:guest_cpu_reset cpu=0xf9dbb4d56a0
>
> The main loop regained control and could process the CAS reboot request
> but it is too late...
>
> 23340@1579260982.866071:runstate_set current_run_state 5 (postmigrate)
> new_state 6 (prelaunch)
Thank you Greg.
So I think the best we can do is to migrate cas_reboot.
To delay the H_CAS call would be cleaner but I don't know if H_BUSY is a
valid return state and this forces to update SLOF too.
Reviewed-by: Laurent Vivier <lviv...@redhat.com>
Thanks,
Laurent
