From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eduardo Habkost Subject: Re: [PATCH] Activate Virtualization On Demand v3 Date: Thu, 6 Nov 2008 11:51:00 -0200 Message-ID: <20081106135100.GQ5247@blackpad> References: <1225881664-16643-1-git-send-email-agraf@suse.de> <20081105205856.GL5247@blackpad> <39D1B096-F900-4BF8-9F30-1A7146093707@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, avi@redhat.com, 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]:50166 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753860AbYKFNvY (ORCPT ); Thu, 6 Nov 2008 08:51:24 -0500 Content-Disposition: inline In-Reply-To: <39D1B096-F900-4BF8-9F30-1A7146093707@suse.de> Sender: kvm-owner@vger.kernel.org List-ID: On Thu, Nov 06, 2008 at 01:57:52PM +0100, Alexander Graf wrote: > > On 05.11.2008, at 21:58, Eduardo Habkost wrote: > >> On Wed, Nov 05, 2008 at 11:41:04AM +0100, 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 >>> v3 changes: >>> - use spin_lock instead of atomics >>> - put locking to new functions hardware_{en,dis}able_all that get >>> called >>> on VM creation/destruction >>> - remove usage counter checks where not necessary >>> - return -EINVAL for IA64 slot < 0 case >>> >>> Signed-off-by: Alexander Graf >>> --- >> >>> >>> -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 (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); >>> +} >> >> Doesn't on_each_cpu() run the function in parallel on all CPUs? If so, >> there is a race between checking *_r and setting *_r. > > Good question - it doesn't really hurt to write the value though, if we > only write it on error. > So I guess we could just remove the first check and check on if( r && > _r) later on. I think the first check doesn't hurt, and using if(_r && r) on the second check should work. I am not sure if there are no pitfalls here due to memory ordering or delayed write on some arches, however. -- Eduardo