Re: [PATCH] Activate Virtualization On Demand

[Avi Kivity <avi@redhat.com>]

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