From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH] Activate Virtualization On Demand Date: Tue, 17 Mar 2009 14:04:38 +0200 Message-ID: <49BF91D6.7000008@redhat.com> References: <1237279648-7839-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 To: Alexander Graf Return-path: Received: from mx1.redhat.com ([66.187.233.31]:51808 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752634AbZCQMEr (ORCPT ); Tue, 17 Mar 2009 08:04:47 -0400 In-Reply-To: <1237279648-7839-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 destruction of the last one. > > So using this, KVM can be easily autoloaded, while keeping other > hypervisors usable. > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 68b217e..7c40743 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -65,6 +65,8 @@ DEFINE_SPINLOCK(kvm_lock); > LIST_HEAD(vm_list); > > static cpumask_var_t cpus_hardware_enabled; > +static int kvm_usage_count = 0; > +static DEFINE_SPINLOCK(kvm_usage_lock); > Please use kvm_lock for this. > > @@ -2327,14 +2341,40 @@ static struct miscdevice kvm_dev = { > &kvm_chardev_ops, > }; > > -static void hardware_enable(void *junk) > +static void hardware_enable(void *_r) > { > int cpu = raw_smp_processor_id(); > + int r; > + > + /* If enabling a previous CPU failed already, let's not continue */ > + if (_r && *((int*)_r)) > + return; > > > if (cpumask_test_cpu(cpu, cpus_hardware_enabled)) > return; > + r = kvm_arch_hardware_enable(NULL); > + if (_r) > + *((int*)_r) = r; > Racy. If one cpu succeeds and another fails, the successful one could overwrite the failing one's result. While the race will never happen (start two VMMs simultaneously) it will cause an endless stream of complaints. Let's use an atomic_t incremented on each failure. Oh, and it can be global since we're inside a lock, so some of the changes to add a return value become unnecessary. > > +static void hardware_disable_all(void) > +{ > + if (!kvm_usage_count) > + return; > Can this happen? > + > + spin_lock(&kvm_usage_lock); > + kvm_usage_count--; > + if (!kvm_usage_count) > + on_each_cpu(hardware_disable, NULL, 1); > + spin_unlock(&kvm_usage_lock); > +} > + > Please make sure cpu hotplug/hotunplug (and this suspend/resume) still work. -- error compiling committee.c: too many arguments to function