Re: [Qemu-devel] Holding the BQL for emulate_ppc_hypercall

[Alex Bennée]

Nikunj A Dadhania <address@hidden> writes:

> Alex Bennée <address@hidden> writes:
>
>> Hi,
>>
>> In the MTTCG patch set one of the big patches is to remove the
>> requirement to hold the BQL while running code:
>>
>>   tcg: drop global lock during TCG code execution
>>
>> And this broke the PPC code because emulate_ppc_hypercall can cause
>> changes to the global state. This function just calls spapr_hypercall()
>> and puts the results into the TCG register file. Normally
>> spapr_hypercall() is called under the BQL in KVM as
>> kvm_arch_handle_exit() does things with the BQL held.
>>
>> I blithely wrapped the called in a lock/unlock pair only to find the
>> ppc64 check builds failed as the hypercall was made during the
>> cc->do_interrupt() code which also holds the BQL.
>>
>> I'm a little confused by the nature of PPC hypercalls in TCG? Are they
>> not all detectable at code generation time? What is the case that causes
>> an exception to occur rather than the helper function doing the
>> hypercall?
>>
>> I guess it comes down to can I avoid doing:
>>
>>   /* If we come via cc->do_interrupt BQL may already be held */
>>   if (!qemu_mutex_iothread_locked()) {
>>       g_mutex_lock_iothread();
>>       env->gpr[3] = spapr_hypercall(cpu, env->gpr[3], &env->gpr[4]);
>>       g_muetx_unlock_iothread();
>>   } else {
>>       env->gpr[3] = spapr_hypercall(cpu, env->gpr[3], &env->gpr[4]);
>>   }
>>
>> Any thoughts?
>
> Similar discussions happened on this patch:
> https://lists.gnu.org/archive/html/qemu-ppc/2016-09/msg00015.html
>
> This was just working for TCG case, need to fix for KVM. I would need to
> handle KVM case to avoid a deadlock.

Thanks for the pointer I missed that before.

But I think the fix here is too far down the call stack as the
spapr_hypercall is called by both TCG and KVM paths. But as discussed
on my reply to Dave I think the correct fix is to ensure
cpu-exec.c:cpu_handle_exception also takes the BQL when delivering
exceptions:

            if (replay_exception()) {
                CPUClass *cc = CPU_GET_CLASS(cpu);
                qemu_mutex_lock_iothread();
                cc->do_interrupt(cpu);
                qemu_mutex_unlock_iothread();
                cpu->exception_index = -1;
            } else if (!replay_has_interrupt()) {

I got confused by the if(replay_exception()) which is a bit non-obvious.

>
> Regards
> Nikunj


--
Alex Bennée