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

[Jan K. <jan...@si...>]

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...@si...>
>> ---
>> @@ -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