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

Simon Kagstrom
2006-11-14T12:08:12Z

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

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);
>
> 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);

// Simon