Re: [Qemu-devel] [PATCH v2] mttcg/i386: Patch instruction using async_safe_* framework

[Pranith Kumar]

Hi Alex,

Alex Bennée writes:

> Pranith Kumar <address@hidden> writes:
>
>> In mttcg, calling pause_all_vcpus() during execution from the
>> generated TBs causes a deadlock if some vCPU is waiting for exclusive
>> execution in start_exclusive(). Fix this by using the aync_safe_*
>> framework instead of pausing vcpus for patching instructions.
>>

<...>

>> diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
>> index 82a4955..11b0d49 100644
>> --- a/hw/i386/kvmvapic.c
>>  
>> -    resume_all_vcpus();
>> +    g_free(info);
>> +}
>> +
>> +static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong 
>> ip)
>> +{
>> +    CPUState *cs = CPU(cpu);
>> +    VAPICHandlers *handlers;
>> +    struct PatchInfo *info;
>> +
>> +    if (smp_cpus == 1) {
>> +        handlers = &s->rom_state.up;
>> +    } else {
>> +        handlers = &s->rom_state.mp;
>> +    }
>> +
>> +    info  = g_new(struct PatchInfo, 1);
>> +    info->handler = handlers;
>> +    info->ip = ip;
>>  
>>      if (!kvm_enabled()) {
>> -        /* Both tb_lock and iothread_mutex will be reset when
>> -         *  longjmps back into the cpu_exec loop. */
>> -        tb_lock();
>> -        tb_gen_code(cs, current_pc, current_cs_base, current_flags, 1);
>> -        cpu_loop_exit_noexc(cs);
>> +        const run_on_cpu_func fn = do_patch_instruction;
>> +
>> +        async_safe_run_on_cpu(cs, fn, RUN_ON_CPU_HOST_PTR(info));
>> +        cs->exception_index = EXCP_INTERRUPT;
>> +        cpu_loop_exit(cs);
>>      }
>> +
>> +    pause_all_vcpus();
>> +
>> +    do_patch_instruction(cs, RUN_ON_CPU_HOST_PTR(info));
>> +
>> +    resume_all_vcpus();
>> +    g_free(info);
>
> I don't know if there is any benefit scheduling this as async work for
> KVM but I'll leave that up to Paolo to decide. From a TCG point of view
> I think its good:
>

We are scheduling this as async work only for non-KVM cases. For KVM, we use
go to the pause/resume path above and patch it there itself.

Thanks,
-- 
Pranith