Re: [Qemu-devel] [PATCH] kvm: First step to push iothread lock out of inner run loop

[Marcelo Tosatti]

On Sat, Jun 23, 2012 at 11:22:07AM +0200, Jan Kiszka wrote:
> On 2012-06-23 02:22, Marcelo Tosatti wrote:
> > On Sat, Jun 23, 2012 at 12:55:49AM +0200, Jan Kiszka wrote:
> >> Should have declared this [RFC] in the subject and CC'ed kvm...
> >>
> >> On 2012-06-23 00:45, Jan Kiszka wrote:
> >>> This sketches a possible path to get rid of the iothread lock on vmexits
> >>> in KVM mode. On x86, the the in-kernel irqchips has to be used because
> >>> we otherwise need to synchronize APIC and other per-cpu state accesses
> >>> that could be changed concurrently. Not yet fully analyzed is the NMI
> >>> injection path in the absence of an APIC.
> >>>
> >>> s390x should be fine without specific locking as their pre/post-run
> >>> callbacks are empty. Power requires locking for the pre-run callback.
> >>>
> >>> This patch is untested, but a similar version was successfully used in
> >>> a x86 setup with a network I/O path that needed no central iothread
> >>> locking anymore (required special MMIO exit handling).
> >>> ---
> >>>  kvm-all.c         |   18 ++++++++++++++++--
> >>>  target-i386/kvm.c |    7 +++++++
> >>>  target-ppc/kvm.c  |    4 ++++
> >>>  3 files changed, 27 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/kvm-all.c b/kvm-all.c
> >>> index f8e4328..9c3e26f 100644
> >>> --- a/kvm-all.c
> >>> +++ b/kvm-all.c
> >>> @@ -1460,6 +1460,8 @@ int kvm_cpu_exec(CPUArchState *env)
> >>>          return EXCP_HLT;
> >>>      }
> >>>  
> >>> +    qemu_mutex_unlock_iothread();
> >>> +
> >>>      do {
> >>>          if (env->kvm_vcpu_dirty) {
> >>>              kvm_arch_put_registers(env, KVM_PUT_RUNTIME_STATE);
> >>> @@ -1476,14 +1478,16 @@ int kvm_cpu_exec(CPUArchState *env)
> >>>               */
> >>>              qemu_cpu_kick_self();
> >>>          }
> >>> -        qemu_mutex_unlock_iothread();
> >>>  
> >>>          run_ret = kvm_vcpu_ioctl(env, KVM_RUN, 0);
> >>>  
> >>> -        qemu_mutex_lock_iothread();
> >>>          kvm_arch_post_run(env, run);
> > 
> > target-i386/kvm.c
> > 
> > void kvm_arch_post_run(CPUX86State *env, struct kvm_run *run)
> > {       
> >     if (run->if_flag) {
> >         env->eflags |= IF_MASK;
> >     } else {
> >         env->eflags &= ~IF_MASK;
> >     }
> >     cpu_set_apic_tpr(env->apic_state, run->cr8);
> >     cpu_set_apic_base(env->apic_state, run->apic_base);
> > }
> > 
> > Clearly there is no structure to any of the writes around the writes
> > in x86's kvm_arch_post_run, so it is unsafe.
> 
> Can't parse this yet.
> 
> None of the fields touched above should be modified outside of the vcpu
> thread context (as long as that thread is inside the inner loop).
> Therefore, it should be safe to run that functions without any lock. Am
> I missing something?

Maybe no in practice, for env->eflags. But it should be formalized,
eg BUG_ON(!vcpu) env->xxx, or some other form of making it not
accessible outside vcpu context.

However, as an example

APIC_COMMON_GET_CLASS -> OBJECT_GET_CLASS -> ....

static TypeImpl *type_table_lookup(const char *name)
{
    return g_hash_table_lookup(type_table_get(), name);
}

Is that safe?