Re: [PATCH] Activate Virtualization On Demand v3

[Eduardo Habkost <ehabkost@redhat.com>]

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 <agraf@suse.de>
> ---
<snip>
>  
> -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.

> +
> +static int hardware_enable_all(void)
> +{
> +	int r = 0;
> +
> +	spin_lock(&kvm_usage_lock);
> +	kvm_usage_count++;
> +	if (kvm_usage_count == 1)
> +		on_each_cpu(hardware_enable, &r, 1);
> +	spin_unlock(&kvm_usage_lock);
> +
> +	return r;
>  }
>  
<snip>

-- 
Eduardo