From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH] Activate Virtualization On Demand v2 Date: Wed, 05 Nov 2008 12:06:13 +0200 Message-ID: <49117015.7040902@redhat.com> References: <1225874896-13186-1-git-send-email-agraf@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, kraxel@redhat.com, anthony@codemonkey.ws, Sander.Vanleeuwen@sun.com, zach@vmware.com, brogers@novell.com To: Alexander Graf Return-path: Received: from mx2.redhat.com ([66.187.237.31]:53242 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754414AbYKEKGX (ORCPT ); Wed, 5 Nov 2008 05:06:23 -0500 In-Reply-To: <1225874896-13186-1-git-send-email-agraf@suse.de> Sender: kvm-owner@vger.kernel.org List-ID: Alexander Graf wrote: > X86 CPUs need to have some magic happening to enable the virtualization > extensions on them. This magic can result in unpleasant results for > users, like blocking other VMMs from working (vmx) or using invalid TLB > entries (svm). > > Currently KVM activates virtualization when the respective kernel module > is loaded. This blocks us from autoloading KVM modules without breaking > other VMMs. > > To circumvent this problem at least a bit, this patch introduces on > demand activation of virtualization. This means, that instead > virtualization is enabled on creation of the first virtual machine > and disabled on removal of the last one. > > So using this, KVM can be easily autoloaded, while keeping other > hypervisors usable. > > v2 adds returns to non-x86 hardware_enables and adds IA64 change > > @@ -563,19 +566,27 @@ static const struct mmu_notifier_ops kvm_mmu_notifier_ops = { > > static struct kvm *kvm_create_vm(void) > { > + int r; > struct kvm *kvm = kvm_arch_create_vm(); > #ifdef KVM_COALESCED_MMIO_PAGE_OFFSET > struct page *page; > #endif > > if (IS_ERR(kvm)) > - goto out; > + return kvm; > + > + if (atomic_add_return(1, &kvm_usage_count) == 1) { > + on_each_cpu(hardware_enable, &r, 1); > + > + if (r) > + goto out_err; > + } > This can race -- if we're preempted immediately after atomic_add_return(), a second vm creation will see the count elevated and can start executing without virtualization enabled. > + > +out_err: > + if (atomic_dec_and_test(&kvm_usage_count)) > + on_each_cpu(hardware_disable, NULL, 1); > Similar race. > @@ -660,6 +674,8 @@ static void kvm_destroy_vm(struct kvm *kvm) > mmu_notifier_unregister(&kvm->mmu_notifier, kvm->mm); > #endif > kvm_arch_destroy_vm(kvm); > + if (atomic_dec_and_test(&kvm_usage_count)) > + on_each_cpu(hardware_disable, NULL, 1); > mmdrop(mm); > } > And again. I suggest returning to spinlocks (and placing the duplicated disable code in a function). > > -static void hardware_enable(void *junk) > +static void hardware_enable(void *_r) > { > int cpu = raw_smp_processor_id(); > + int r; > > if (cpu_isset(cpu, cpus_hardware_enabled)) > return; > + r = kvm_arch_hardware_enable(NULL); > + if (_r) > + *((int*)_r) = r; > + if (r) { > + printk(KERN_INFO "kvm: enabling virtualization on " > + "CPU%d failed\n", cpu); > + return; > + } > + > cpu_set(cpu, cpus_hardware_enabled); > - kvm_arch_hardware_enable(NULL); > } > We'll be in a nice fix if we can only enable virtualization on some processors; that's the reason hardware_enable() was originally specified as returning void. I don't see an easy way out, but it's hardly a likely event. > case CPU_UP_CANCELED: > printk(KERN_INFO "kvm: disabling virtualization on CPU%d\n", > cpu); > - smp_call_function_single(cpu, hardware_disable, NULL, 1); > + if (atomic_read(&kvm_usage_count)) > + smp_call_function_single(cpu, hardware_disable, > + NULL, 1); > break; > case CPU_ONLINE: > printk(KERN_INFO "kvm: enabling virtualization on CPU%d\n", > cpu); > - smp_call_function_single(cpu, hardware_enable, NULL, 1); > + if (atomic_read(&kvm_usage_count)) > + smp_call_function_single(cpu, hardware_enable, > + NULL, 1); > break; > Are these called in a point where processes can't run? Otherwise there's a race here. > static int kvm_resume(struct sys_device *dev) > { > - hardware_enable(NULL); > + if (atomic_read(&kvm_usage_count)) > + hardware_enable(NULL); > return 0; > } > Move the test to hardware_enable()? It's repeated too often. -- error compiling committee.c: too many arguments to function