Re: [PATCH] Activate Virtualization On Demand v3

[Eduardo Habkost <ehabkost@redhat.com>]

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 <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.
>
> 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