Re: [PATCH] 2/2: Fall back to plain Qemu if KVM is not present, report errors from libkvm

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

Simon Kagstrom wrote:
> (I've edited the patch comments to show the reasoning)
>
> At Tue, 14 Nov 2006 13:44:31 +0200,
> Avi Kivity wrote:
>   
>>     /* Initialize kvm */
>>     if (kvm_allowed) {
>> 	if (kvm_qemu_init() < 0) {
>> 	    fprintf(stderr, "Could not initialize KVM\n");
>> 	    kvm_allowed = 0;
>> 	}
>>     }
>>     /* Fall back to normal qemu if KVM fails */
>>     if (!kvm_allowed) {
>> 	phys_ram_base = qemu_vmalloc(phys_ram_size);
>> 	if (!phys_ram_base) {
>> 	    fprintf(stderr, "Could not allocate physical memory\n");
>> 	    exit(1);
>> 	}
>>     }
>>
>> This bit is rather awkward, since it disabled kvm after it has been 
>> enabled.  It's a cleaner to never enable kvm_allowed in the first place 
>> (i.e. during argument parsing).
>>     
>
> OK. My reasoning was this: qemu with KVM defaults to use kvm and
> therefore tries to enable it (kvm_qemu_init), if it cannot be
> initialized, it will disable it again.
>   

Yes, but it leads to code duplication (there's a qemu_vmalloc() outside 
USE_KVM). Also, if some code prior to this depended on kvm_allowed, it 
would be invalidated.

Best to set kvm_allowed once you're sure it's really allowed, and then 
don't touch it.

> I can submit a new patch where kvm_init and kvm_create is separated
> and kvm_init is done at argument-parsing time if you like, but
> personally I think this way is clearer :-)
>
>   
>> How about a
>>
>>     int libkvm_init(kvm_libcontext_t *libcontext);
>>     void libkvm_exit(kvm_libcontext_t libcontext);
>>    
>>     int kvm_create(kvm_libcontext_t libcontext, kvm_context_t* vm);
>>     void kvm_destroy(kvm_context_t *vm);
>>     

I meant kvm_destroy(kvm_context_t vm);

>> at present, libkvm_init() can just call access(2) on /dev/kvm.  In the 
>> future, it is planned that open("/dev/kvm") will not create a VM (it 
>> just opens a communication channel to the driver), so that libkvm_init() 
>> will open the device and store the fd.
>>     
>
> Personally, I would prefer something like
>
>    int kvm_open(kvm_t *kvm;)
>    void kvm_close(kvm_t *kvm);
>
>    int kvm_create_vm(kvm_t *kvm, kvm_context_t *vm);
>    void kvm_destroy_vm(kvm_t *kvm, kvm_context_t *vm);
>   

That means that if sizeof(kvm_t) changes, we need to recompile 
everything. With kvm_context_t and kvm_t as opaque pointers, we preserve 
binary compatibility when their definitions change.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642