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

["Alex Bennée" <alex.bennee@linaro.org>]

Pranith Kumar <bobby.prani@gmail.com> writes:

> Hi Alex,
>
> Alex Bennée writes:
>
>> Pranith Kumar <bobby.prani@gmail.com> 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.

No I mean would it be more efficient to do that for KVM as safe work. To
be honest the code to pause_all_vcpus() seems a little hokey given
cpu_stop_current() somehow stops itself while cpu_exit'ing the rest of
the vcpus. But I guess you would involve an additional KVM transition
for the calling thread, I'm not sure hence the deference to the KVM
experts ;-)

>
> Thanks,


--
Alex Bennée