[Qemu-devel] Re: [PATCH] fix halt emulation with icount and CONFIG_IOTHREAD (v2)

[Jan Kiszka]

On 2011-02-17 04:15, Marcelo Tosatti wrote:
> On Wed, Feb 16, 2011 at 10:32:25AM +0100, Paolo Bonzini wrote:
>> On 02/15/2011 09:56 PM, Marcelo Tosatti wrote:
>>> Note: to be applied to uq/master.
>>>
>>> In icount mode, halt emulation should take into account the nearest
>>> event when sleeping.
>>
>> I agree with Jan that this patch is not the best solution, if not incorrect.
>>
>> However, in the iothread, the main loop can kick the VCPU thread
>> instead of running cpu_exec_all like it does in non-iothread mode.
>> Something like this:
>>
>> diff --git a/vl.c b/vl.c
>> index b436952..7835317 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -1425,7 +1425,9 @@ static void main_loop(void)
>>      qemu_main_loop_start();
>>
>>      for (;;) {
>> -#ifndef CONFIG_IOTHREAD
>> +#ifdef CONFIG_IOTHREAD
>> +        qemu_cpu_kick(first_cpu);
>> +#else
>>          nonblocking = cpu_exec_all();
>>          if (vm_request_pending()) {
>>              nonblocking = true;
>>
>> I don't like this 100% because it relies on the fact that there is
>> only one TCG execution thread.  In a multithreaded world you would:
>>
>> 1) have each CPU register its own instruction counter;
>>
>> 2) have each CPU register its own QEMU_CLOCK_REALTIME timer based on
>> qemu_icount_delta() and arm it just before going to sleep; the timer
>> kicks the CPU.
>>
>> 3) remove all icount business from qemu_calculate_timeout.
>>
>> Item (3) is what makes me prefer my patch above (if it works) to
>> Marcelo's.  Marcelo's patch is tying even more
>> qemu_calculate_timeout to the icount.  So if anything, a patch
>> tweaking the timedwait like Marcelo's should use something based on
>> qemu_icount_delta().
> 
> Yes, using qemu_icount_delta directly in tcg_wait_io_event timedwait 
> is explicit (partially the reason for confusion with my patch).
> 
> So the reasoning for the patch is:
> 
> With icount vm_timer timers expire on virtual CPU time. If a CPU halts,
> you cannot expect passage of realtime to trigger vm_timers expiration.
> 
> So instead vm_timer expiration is converted to realtime, and used as
> halt timeout.

The changing the calculation is trying to cure a symptom. A halt with
timeout is already broken, but we fortunately have a patch against that.
Let's shake potential remaining bugs out of *that*.

Jan

signature.asc
Description: OpenPGP digital signature