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

Simon Kagstrom
2006-11-14T13:04:19Z

At Tue, 14 Nov 2006 14:24:20 +0200,
Avi Kivity wrote:
> 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've attached a new patch below. It's still setting kvm_allowed to 0
in two places - if kvm_qemu_init() fails and if the -no-kvm option is
passed, but it now does a kind of "try-and-see" and sets it to zero if
it fails. This is done regardless of if -no-kvm is passed or not.

I split initialization and creation of a vm in qemu-kvm.c.

The code duplication was there before, and I have not changed that.

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

Sorry, true.

I was actually mostly thinking of the naming (I prefer the simpler
kvm_t).

// Simon

diff -u ../orig/kvm-2/qemu/qemu-kvm.c qemu/qemu-kvm.c
--- ../orig/kvm-2/qemu/qemu-kvm.c 2006-11-05 10:18:52.000000000 +0100
+++ qemu/qemu-kvm.c 2006-11-14 13:56:35.000000000 +0100
@@ -450,10 +450,30 @@
.io_window = kvm_io_window,
};

-void kvm_qemu_init()
+int kvm_qemu_init()
{
+ /* Try to initialize kvm */
kvm_context = kvm_init(&qemu_kvm_ops, saved_env);
- kvm_create(kvm_context, phys_ram_size, (void**)&phys_ram_base);
+ if (!kvm_context) {
+ return -1;
+ }
+
+ return 0;
+}
+
+int kvm_qemu_create_context(void)
+{
+ if (kvm_create(kvm_context, phys_ram_size, (void**)&phys_ram_base) < 0) {
+ kvm_qemu_destroy();
+ return -1;
+ }
+
+ return 0;
+}
+
+void kvm_qemu_destroy(void)
+{
+ kvm_finalize(kvm_context);
}

int kvm_update_debugger(CPUState *env)
diff -u ../orig/kvm-2/qemu/qemu-kvm.h qemu/qemu-kvm.h
--- ../orig/kvm-2/qemu/qemu-kvm.h 2006-10-16 11:01:02.000000000 +0200
+++ qemu/qemu-kvm.h 2006-11-14 13:55:56.000000000 +0100
@@ -3,7 +3,9 @@

#include "kvmctl.h"

-void kvm_qemu_init(void);
+int kvm_qemu_init(void);
+int kvm_qemu_create_context(void);
+void kvm_qemu_destroy(void);
void kvm_load_registers(CPUState *env);
int kvm_cpu_exec(CPUState *env);
int kvm_update_debugger(CPUState *env);
diff -u ../orig/kvm-2/qemu/vl.c qemu/vl.c
--- ../orig/kvm-2/qemu/vl.c 2006-11-05 10:18:52.000000000 +0100
+++ qemu/vl.c 2006-11-14 13:56:32.000000000 +0100
@@ -5706,6 +5706,10 @@

nb_nics = 0;
/* default mac address of the first network interface */
+#if USE_KVM
+ if (kvm_qemu_init() < 0)
+ kvm_allowed = 0;
+#endif

optind = 1;
for(;;) {
@@ -6099,11 +6103,15 @@
}

/* init the memory */
-#if USE_KVM
phys_ram_size = ram_size + vga_ram_size + bios_size;
+#if USE_KVM
+ /* Initialize kvm */
if (kvm_allowed) {
phys_ram_size += KVM_EXTRA_PAGES * 4096;
- kvm_qemu_init();
+ if (kvm_qemu_create_context() < 0) {
+ fprintf(stderr, "Could not create KVM context\n");
+ exit(1);
+ }
} else {
phys_ram_base = qemu_vmalloc(phys_ram_size);
if (!phys_ram_base) {
@@ -6112,7 +6120,6 @@
}
}
#else
- phys_ram_size = ram_size + vga_ram_size + bios_size;
phys_ram_base = qemu_vmalloc(phys_ram_size);
if (!phys_ram_base) {
fprintf(stderr, "Could not allocate physical memory\n");