Re: [Qemu-devel] [RFC PATCH v4 13/23] cpus: only take BQL for sleeping threads

[Paolo Bonzini]

On 19/01/2018 13:05, Pavel Dovgalyuk wrote:
>> From: Paolo Bonzini [mailto:address@hidden
>> On 19/01/2018 09:44, Pavel Dovgalyuk wrote:
>>>      while (all_cpu_threads_idle()) {
>>> +        qemu_mutex_lock_iothread();
>>>          stop_tcg_kick_timer();
>>>          qemu_cond_wait(cpu->halt_cond, &qemu_global_mutex);
>>> +        qemu_mutex_unlock_iothread();
>>>      }
>>
>> cpu_has_work cannot be called outside BQL yet.  You first need to access
>> cpu->interrupt_request with atomics.
>>
>> In general, testing the condition outside the mutex is a very dangerous
>> pattern (and I'm usually the one who enjoys dangerous patterns).
> 
> It means, that I'll have to fix all the has_work function to avoid races,
> because x86_cpu_has_work may have them?

Why only x86_cpu_has_work?

Even reading cs->interrupt_request outside the mutex is unsafe.

Paolo

> static bool x86_cpu_has_work(CPUState *cs)
> {
>     X86CPU *cpu = X86_CPU(cs);
>     CPUX86State *env = &cpu->env;
> 
>     return ((cs->interrupt_request & (CPU_INTERRUPT_HARD |
>                                       CPU_INTERRUPT_POLL)) &&
>             (env->eflags & IF_MASK)) ||
>            (cs->interrupt_request & (CPU_INTERRUPT_NMI |
>                                      CPU_INTERRUPT_INIT |
>                                      CPU_INTERRUPT_SIPI |
>                                      CPU_INTERRUPT_MCE)) ||
>            ((cs->interrupt_request & CPU_INTERRUPT_SMI) &&
>             !(env->hflags & HF_SMM_MASK));
> }
> 
> Pavel Dovgalyuk
>