On Sat, Jun 23, 2012 at 7:45 PM, Jan Kiszka <jan.kiszka@xxxxxx> wrote: > On 2012-06-23 11:06, Marcelo Tosatti wrote: >> On Fri, Jun 22, 2012 at 09:22:59PM -0300, 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. >> >> No access protocol to the CPUState and apic devices (who can write when, >> who can read when). >> > > Hmm, we may need the iothread lock around cpu_set_apic_tpr for > !kvm_irqchip_in_kernel(). And as we are at it, apic_base manipulation > can be but there as well. > As to !kvm_irqchip_in_kernel(),I think there is quite a few fields in CPUState (interrupt_request,eflags,halted,exit_request,exception_injected), which we must care about. They are heavily depended on env->apic_state, so we consider them as a atomic context, that is say during the updating of env's these context, we do not allow apic_state changed. And that is why I introduce cpu_lock in the patch "[PATCH 2/2] kvm: use per-cpu lock to free vcpu thread out of the big lock" Regards, pingfan > With in-kernel irqchip, there is no such need. Also, no one accesses > eflags outside of the vcpu thread, independent of the irqchip mode. > > Jan > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html