Re: [PATCH] qemu-kvm: fix guest resetting

Jan Kiszka
2008-05-09T08:13:33Z

Marcelo Tosatti wrote:
> Hi Jan,
>
> On Thu, May 08, 2008 at 10:29:32AM +0200, Jan Kiszka wrote:
>> Resetting guests used to be racy, deadlock-prone, or simply broken (for
>> SMP). This patch fixes the issues - at least for me on x86 (tested on
>> Intel SMP host, UP and SMP guest, in-kernel und user space irqchip,
>> guest- and monitor-issued resets). Note that ia64 and powerpc may need
>> to look into the SMP thing as well (=>kvm_arch_cpu_reset).
>>
>> At this chance, the patch also cleans up some unneeded reset fragments.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>> @@ -317,11 +317,18 @@ void qemu_kvm_system_reset_request(void)
>> {
>> int i;
>>
>> + pause_all_threads();
>> +
>> + qemu_system_reset();
>> +
>> + for (i = 0; i < smp_cpus; ++i)
>> + kvm_arch_cpu_reset(vcpu_info[i].env);
>> +
>> for (i = 0; i < smp_cpus; ++i) {
>> - vcpu_info[i].reload_regs = 1;
>> + vcpu_info[i].stop = 0;
>> + vcpu_info[i].stopped = 0;
>> pthread_kill(vcpu_info[i].thread, SIG_IPI);
>> }
>> - qemu_system_reset();
>
> Why don't you signal the IO thread to pause all vcpu's and place their
> registers and "run state" in the proper condition if the reset request
> comes from the guest? It should simplify things a lot (and avoid any
> changes to vl.c).
>
> After signalling the vcpu should stop instead of returning to guest
> mode.

Hmm, need to think a bit more about it as I don't see the benefit yet
(code suggestions are welcome in the meantime :)!).

The changes to vl.c are actually cleanups, as pause_all_threads() is now
context-agnostic and we no longer need to go through the qemu way of
raising reset. This new property of pause_all_threads() is something we
should desire for simplicity and robustness reasons anyway (e.g. to
simplify guest debugging later on). And the way
qemu_kvm_system_reset_request() is implemented would not change, we need
this serialization to avoid races between IO and VCPU threads.

Jan

--
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux