[PATCH v2 0/6]

[PATCH v2 0/6]

[Glauber Costa]

Marcelo,

since you told me that you were not confrotable with the --enable-kvm
change without avi's ack, I'm dropping it, and sending the rest of the 
series again. On top of that, I'm also including some more cleanup patches:
one removes kvm_out{b,w,l}, in the same way I've already done with kvm_in{b,w,l},
and the other removes specific mmio functions.

Thanks!

[PATCH v2 1/6] remove kvm_in* functions

[Glauber Costa]

We can use plain qemu's here, and save a couple of lines/complexity.
I'm leaving outb for later, because the SMM thing makes it a little bit
less trivial.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 qemu-kvm.c |   25 ++++---------------------
 1 files changed, 4 insertions(+), 21 deletions(-)

diff --git a/qemu-kvm.c b/qemu-kvm.c
index 3c892e6..0f5f14f 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -97,24 +97,6 @@ static int kvm_debug(void *opaque, void *data,
 }
 #endif
 
-static int kvm_inb(void *opaque, uint16_t addr, uint8_t *data)
-{
-    *data = cpu_inb(0, addr);
-    return 0;
-}
-
-static int kvm_inw(void *opaque, uint16_t addr, uint16_t *data)
-{
-    *data = cpu_inw(0, addr);
-    return 0;
-}
-
-static int kvm_inl(void *opaque, uint16_t addr, uint32_t *data)
-{
-    *data = cpu_inl(0, addr);
-    return 0;
-}
-
 #define PM_IO_BASE 0xb000
 
 static int kvm_outb(void *opaque, uint16_t addr, uint8_t data)
@@ -853,15 +835,16 @@ static int handle_io(kvm_vcpu_context_t vcpu)
 	for (i = 0; i < run->io.count; ++i) {
 		switch (run->io.direction) {
 		case KVM_EXIT_IO_IN:
+			r = 0;
 			switch (run->io.size) {
 			case 1:
-				r = kvm_inb(kvm->opaque, addr, p);
+				*(uint8_t *)p = cpu_inb(kvm->opaque, addr);
 				break;
 			case 2:
-				r = kvm_inw(kvm->opaque, addr, p);
+				*(uint16_t *)p = cpu_inw(kvm->opaque, addr);
 				break;
 			case 4:
-				r = kvm_inl(kvm->opaque, addr, p);
+				*(uint32_t *)p = cpu_inl(kvm->opaque, addr);
 				break;
 			default:
 				fprintf(stderr, "bad I/O size %d\n", run->io.size);
-- 
1.6.2.2

[PATCH v2 2/6] reuse env stop and stopped states

[Glauber Costa]

qemu CPUState already provides "stop" and "stopped" states. And they
mean exactly that. There is no need for us to provide our own.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 cpu-defs.h |    2 --
 qemu-kvm.c |   30 ++++++++++++------------------
 vl.c       |    2 +-
 3 files changed, 13 insertions(+), 21 deletions(-)

diff --git a/cpu-defs.h b/cpu-defs.h
index 7570096..fce366f 100644
--- a/cpu-defs.h
+++ b/cpu-defs.h
@@ -142,8 +142,6 @@ struct qemu_work_item;
 struct KVMCPUState {
     pthread_t thread;
     int signalled;
-    int stop;
-    int stopped;
     int created;
     void *vcpu_ctx;
     struct qemu_work_item *queued_work_first, *queued_work_last;
diff --git a/qemu-kvm.c b/qemu-kvm.c
index 0f5f14f..8eeace4 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -91,7 +91,7 @@ static int kvm_debug(void *opaque, void *data,
 
     if (handle) {
 	kvm_debug_cpu_requested = env;
-	env->kvm_cpu_state.stopped = 1;
+	env->stopped = 1;
     }
     return handle;
 }
@@ -977,7 +977,7 @@ int handle_halt(kvm_vcpu_context_t vcpu)
 int handle_shutdown(kvm_context_t kvm, CPUState *env)
 {
     /* stop the current vcpu from going back to guest mode */
-    env->kvm_cpu_state.stopped = 1;
+    env->stopped = 1;
 
     qemu_system_reset_request();
     return 1;
@@ -1815,7 +1815,7 @@ int kvm_cpu_exec(CPUState *env)
 
 static int is_cpu_stopped(CPUState *env)
 {
-    return !vm_running || env->kvm_cpu_state.stopped;
+    return !vm_running || env->stopped;
 }
 
 static void flush_queued_work(CPUState *env)
@@ -1861,9 +1861,9 @@ static void kvm_main_loop_wait(CPUState *env, int timeout)
     cpu_single_env = env;
     flush_queued_work(env);
 
-    if (env->kvm_cpu_state.stop) {
-	env->kvm_cpu_state.stop = 0;
-	env->kvm_cpu_state.stopped = 1;
+    if (env->stop) {
+	env->stop = 0;
+	env->stopped = 1;
 	pthread_cond_signal(&qemu_pause_cond);
     }
 
@@ -1875,7 +1875,7 @@ static int all_threads_paused(void)
     CPUState *penv = first_cpu;
 
     while (penv) {
-        if (penv->kvm_cpu_state.stop)
+        if (penv->stop)
             return 0;
         penv = (CPUState *)penv->next_cpu;
     }
@@ -1889,11 +1889,11 @@ static void pause_all_threads(void)
 
     while (penv) {
         if (penv != cpu_single_env) {
-            penv->kvm_cpu_state.stop = 1;
+            penv->stop = 1;
             pthread_kill(penv->kvm_cpu_state.thread, SIG_IPI);
         } else {
-            penv->kvm_cpu_state.stop = 0;
-            penv->kvm_cpu_state.stopped = 1;
+            penv->stop = 0;
+            penv->stopped = 1;
             cpu_exit(penv);
         }
         penv = (CPUState *)penv->next_cpu;
@@ -1910,8 +1910,8 @@ static void resume_all_threads(void)
     assert(!cpu_single_env);
 
     while (penv) {
-        penv->kvm_cpu_state.stop = 0;
-        penv->kvm_cpu_state.stopped = 0;
+        penv->stop = 0;
+        penv->stopped = 0;
         pthread_kill(penv->kvm_cpu_state.thread, SIG_IPI);
         penv = (CPUState *)penv->next_cpu;
     }
@@ -2676,12 +2676,6 @@ int kvm_log_stop(target_phys_addr_t phys_addr, target_phys_addr_t len)
     return 0;
 }
 
-void qemu_kvm_cpu_stop(CPUState *env)
-{
-    if (kvm_enabled())
-        env->kvm_cpu_state.stopped = 1;
-}
-
 int kvm_set_boot_cpu_id(uint32_t id)
 {
 	return kvm_set_boot_vcpu_id(kvm_context, id);
diff --git a/vl.c b/vl.c
index b3df596..6ef7690 100644
--- a/vl.c
+++ b/vl.c
@@ -3553,7 +3553,7 @@ void qemu_system_reset_request(void)
         reset_requested = 1;
     }
     if (cpu_single_env) {
-        qemu_kvm_cpu_stop(cpu_single_env);
+        cpu_single_env->stopped = 1;
     }
     qemu_notify_event();
 }
-- 
1.6.2.2

[PATCH v2 3/6] remove kvm_abi variable

[Glauber Costa]

We're not using this for anything

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 qemu-kvm.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/qemu-kvm.c b/qemu-kvm.c
index 8eeace4..a8298e4 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -43,7 +43,6 @@ int kvm_pit = 1;
 int kvm_pit_reinject = 1;
 int kvm_nested = 0;
 
-
 static KVMState *kvm_state;
 kvm_context_t kvm_context;
 
@@ -79,7 +78,6 @@ static LIST_HEAD(, ioperm_data) ioperm_head;
 
 #define ALIGN(x, y) (((x)+(y)-1) & ~((y)-1))
 
-int kvm_abi = EXPECTED_KVM_API_VERSION;
 int kvm_page_size;
 
 #ifdef KVM_CAP_SET_GUEST_DEBUG
@@ -429,7 +427,6 @@ int kvm_init(int smp_cpus)
 	    fprintf(stderr, "kvm userspace version too old\n");
 	    goto out_close;
 	}
-	kvm_abi = r;
 	kvm_page_size = getpagesize();
 	kvm_state = qemu_mallocz(sizeof(*kvm_state));
     kvm_context = &kvm_state->kvm_context;
-- 
1.6.2.2

[PATCH v2 4/6] remove created from kvm_state

[Glauber Costa]

Again, CPUState has it, and it means exactly that.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 cpu-defs.h |    1 -
 qemu-kvm.c |   10 +++++-----
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/cpu-defs.h b/cpu-defs.h
index fce366f..ce9f96a 100644
--- a/cpu-defs.h
+++ b/cpu-defs.h
@@ -142,7 +142,6 @@ struct qemu_work_item;
 struct KVMCPUState {
     pthread_t thread;
     int signalled;
-    int created;
     void *vcpu_ctx;
     struct qemu_work_item *queued_work_first, *queued_work_last;
 };
diff --git a/qemu-kvm.c b/qemu-kvm.c
index a8298e4..cef522d 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -1727,12 +1727,12 @@ void kvm_update_interrupt_request(CPUState *env)
     int signal = 0;
 
     if (env) {
-        if (!current_env || !current_env->kvm_cpu_state.created)
+        if (!current_env || !current_env->created)
             signal = 1;
         /*
          * Testing for created here is really redundant
          */
-        if (current_env && current_env->kvm_cpu_state.created &&
+        if (current_env && current_env->created &&
             env != current_env && !env->kvm_cpu_state.signalled)
             signal = 1;
 
@@ -2012,7 +2012,7 @@ static void *ap_main_loop(void *_env)
 
     /* signal VCPU creation */
     pthread_mutex_lock(&qemu_mutex);
-    current_env->kvm_cpu_state.created = 1;
+    current_env->created = 1;
     pthread_cond_signal(&qemu_vcpu_cond);
 
     /* and wait for machine initialization */
@@ -2028,13 +2028,13 @@ void kvm_init_vcpu(CPUState *env)
 {
     pthread_create(&env->kvm_cpu_state.thread, NULL, ap_main_loop, env);
 
-    while (env->kvm_cpu_state.created == 0)
+    while (env->created == 0)
 	qemu_cond_wait(&qemu_vcpu_cond);
 }
 
 int kvm_vcpu_inited(CPUState *env)
 {
-    return env->kvm_cpu_state.created;
+    return env->created;
 }
 
 #ifdef TARGET_I386
-- 
1.6.2.2

[PATCH v2 5/6] remove kvm_specific kvm_out* functions

[Glauber Costa]

As example of what was already done with inb.
This is a little bit more tricky, because of SMM, but those
bits are handled directly in apic anyway.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 qemu-kvm.c |   60 +++---------------------------------------------------------
 1 files changed, 3 insertions(+), 57 deletions(-)

diff --git a/qemu-kvm.c b/qemu-kvm.c
index cef522d..0724c28 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -95,55 +95,6 @@ static int kvm_debug(void *opaque, void *data,
 }
 #endif
 
-#define PM_IO_BASE 0xb000
-
-static int kvm_outb(void *opaque, uint16_t addr, uint8_t data)
-{
-    if (addr == 0xb2) {
-	switch (data) {
-	case 0: {
-	    cpu_outb(0, 0xb3, 0);
-	    break;
-	}
-	case 0xf0: {
-	    unsigned x;
-
-	    /* enable acpi */
-	    x = cpu_inw(0, PM_IO_BASE + 4);
-	    x &= ~1;
-	    cpu_outw(0, PM_IO_BASE + 4, x);
-	    break;
-	}
-	case 0xf1: {
-	    unsigned x;
-
-	    /* enable acpi */
-	    x = cpu_inw(0, PM_IO_BASE + 4);
-	    x |= 1;
-	    cpu_outw(0, PM_IO_BASE + 4, x);
-	    break;
-	}
-	default:
-	    break;
-	}
-	return 0;
-    }
-    cpu_outb(0, addr, data);
-    return 0;
-}
-
-static int kvm_outw(void *opaque, uint16_t addr, uint16_t data)
-{
-    cpu_outw(0, addr, data);
-    return 0;
-}
-
-static int kvm_outl(void *opaque, uint16_t addr, uint32_t data)
-{
-    cpu_outl(0, addr, data);
-    return 0;
-}
-
 int kvm_mmio_read(void *opaque, uint64_t addr, uint8_t *data, int len)
 {
 	cpu_physical_memory_rw(addr, data, len, 0);
@@ -825,14 +776,12 @@ static int handle_io(kvm_vcpu_context_t vcpu)
 	struct kvm_run *run = vcpu->run;
 	kvm_context_t kvm = vcpu->kvm;
 	uint16_t addr = run->io.port;
-	int r;
 	int i;
 	void *p = (void *)run + run->io.data_offset;
 
 	for (i = 0; i < run->io.count; ++i) {
 		switch (run->io.direction) {
 		case KVM_EXIT_IO_IN:
-			r = 0;
 			switch (run->io.size) {
 			case 1:
 				*(uint8_t *)p = cpu_inb(kvm->opaque, addr);
@@ -851,16 +800,13 @@ static int handle_io(kvm_vcpu_context_t vcpu)
 		case KVM_EXIT_IO_OUT:
 			switch (run->io.size) {
 			case 1:
-				r = kvm_outb(kvm->opaque, addr,
-						     *(uint8_t *)p);
+				 cpu_outb(kvm->opaque, addr, *(uint8_t *)p);
 				break;
 			case 2:
-				r = kvm_outw(kvm->opaque, addr,
-						     *(uint16_t *)p);
+				cpu_outw(kvm->opaque, addr, *(uint16_t *)p);
 				break;
 			case 4:
-				r = kvm_outl(kvm->opaque, addr,
-						     *(uint32_t *)p);
+				cpu_outl(kvm->opaque, addr, *(uint32_t *)p);
 				break;
 			default:
 				fprintf(stderr, "bad I/O size %d\n", run->io.size);
-- 
1.6.2.2

[PATCH v2 6/6] remove kvm_mmio_read and kvm_mmio_write

[Glauber Costa]

all they did was to call a qemu function. Call this function instead.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 qemu-kvm-x86.c |    7 +------
 qemu-kvm.c     |   34 ++++++++--------------------------
 2 files changed, 9 insertions(+), 32 deletions(-)

diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
index 350f272..741ae0a 100644
--- a/qemu-kvm-x86.c
+++ b/qemu-kvm-x86.c
@@ -344,7 +344,6 @@ void kvm_show_code(kvm_vcpu_context_t vcpu)
 	unsigned char code;
 	char code_str[SHOW_CODE_LEN * 3 + 1];
 	unsigned long rip;
-	kvm_context_t kvm = vcpu->kvm;
 
 	r = ioctl(fd, KVM_GET_SREGS, &sregs);
 	if (r == -1) {
@@ -364,11 +363,7 @@ void kvm_show_code(kvm_vcpu_context_t vcpu)
 	for (n = -back_offset; n < SHOW_CODE_LEN-back_offset; ++n) {
 		if (n == 0)
 			strcat(code_str, " -->");
-		r = kvm_mmio_read(kvm->opaque, rip + n, &code, 1);
-		if (r < 0) {
-			strcat(code_str, " xx");
-			continue;
-		}
+		cpu_physical_memory_rw(rip + n, &code, 1, 0);
 		sprintf(code_str + strlen(code_str), " %02x", code);
 	}
 	fprintf(stderr, "code:%s\n", code_str);
diff --git a/qemu-kvm.c b/qemu-kvm.c
index 0724c28..9b1c506 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -95,18 +95,6 @@ static int kvm_debug(void *opaque, void *data,
 }
 #endif
 
-int kvm_mmio_read(void *opaque, uint64_t addr, uint8_t *data, int len)
-{
-	cpu_physical_memory_rw(addr, data, len, 0);
-	return 0;
-}
-
-int kvm_mmio_write(void *opaque, uint64_t addr, uint8_t *data, int len)
-{
-	cpu_physical_memory_rw(addr, data, len, 1);
-	return 0;
-}
-
 static int handle_unhandled(kvm_context_t kvm, kvm_vcpu_context_t vcpu,
                             uint64_t reason)
 {
@@ -888,23 +876,17 @@ int kvm_set_mpstate(kvm_vcpu_context_t vcpu, struct kvm_mp_state *mp_state)
 }
 #endif
 
-static int handle_mmio(kvm_vcpu_context_t vcpu)
+static void handle_mmio(kvm_vcpu_context_t vcpu)
 {
 	unsigned long addr = vcpu->run->mmio.phys_addr;
-	kvm_context_t kvm = vcpu->kvm;
 	struct kvm_run *kvm_run = vcpu->run;
 	void *data = kvm_run->mmio.data;
 
 	/* hack: Red Hat 7.1 generates these weird accesses. */
 	if ((addr > 0xa0000-4 && addr <= 0xa0000) && kvm_run->mmio.len == 3)
-	    return 0;
+	    return;
 
-	if (kvm_run->mmio.is_write)
-		return kvm_mmio_write(kvm->opaque, addr, data,
-					kvm_run->mmio.len);
-	else
-		return kvm_mmio_read(kvm->opaque, addr, data,
-					kvm_run->mmio.len);
+    cpu_physical_memory_rw(addr, data, kvm_run->mmio.len, kvm_run->mmio.is_write);
 }
 
 int handle_io_window(kvm_context_t kvm)
@@ -991,10 +973,9 @@ again:
 	        struct kvm_coalesced_mmio_ring *ring = (void *)run +
 						kvm->coalesced_mmio * PAGE_SIZE;
 		while (ring->first != ring->last) {
-			kvm_mmio_write(kvm->opaque,
-				 ring->coalesced_mmio[ring->first].phys_addr,
-				&ring->coalesced_mmio[ring->first].data[0],
-				 ring->coalesced_mmio[ring->first].len);
+            cpu_physical_memory_rw(ring->coalesced_mmio[ring->first].phys_addr,
+                                   &ring->coalesced_mmio[ring->first].data[0],
+                                   ring->coalesced_mmio[ring->first].len, 1);
 			smp_wmb();
 			ring->first = (ring->first + 1) %
 							KVM_COALESCED_MMIO_MAX;
@@ -1033,7 +1014,8 @@ again:
 			r = handle_debug(vcpu, env);
 			break;
 		case KVM_EXIT_MMIO:
-			r = handle_mmio(vcpu);
+            r = 0;
+			handle_mmio(vcpu);
 			break;
 		case KVM_EXIT_HLT:
 			r = handle_halt(vcpu);
-- 
1.6.2.2

Re: [PATCH v2 6/6] remove kvm_mmio_read and kvm_mmio_write

[Marcelo Tosatti]

On Tue, Jul 21, 2009 at 06:13:12PM -0400, Glauber Costa wrote:
> all they did was to call a qemu function. Call this function instead.
> 
> Signed-off-by: Glauber Costa <glommer@redhat.com>
> ---
>  qemu-kvm-x86.c |    7 +------
>  qemu-kvm.c     |   34 ++++++++--------------------------
>  2 files changed, 9 insertions(+), 32 deletions(-)
> 
> diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
> index 350f272..741ae0a 100644
> --- a/qemu-kvm-x86.c
> +++ b/qemu-kvm-x86.c
> @@ -344,7 +344,6 @@ void kvm_show_code(kvm_vcpu_context_t vcpu)
>  	unsigned char code;
>  	char code_str[SHOW_CODE_LEN * 3 + 1];
>  	unsigned long rip;
> -	kvm_context_t kvm = vcpu->kvm;
>  
>  	r = ioctl(fd, KVM_GET_SREGS, &sregs);
>  	if (r == -1) {
> @@ -364,11 +363,7 @@ void kvm_show_code(kvm_vcpu_context_t vcpu)
>  	for (n = -back_offset; n < SHOW_CODE_LEN-back_offset; ++n) {
>  		if (n == 0)
>  			strcat(code_str, " -->");
> -		r = kvm_mmio_read(kvm->opaque, rip + n, &code, 1);
> -		if (r < 0) {
> -			strcat(code_str, " xx");
> -			continue;
> -		}
> +		cpu_physical_memory_rw(rip + n, &code, 1, 0);
>  		sprintf(code_str + strlen(code_str), " %02x", code);
>  	}
>  	fprintf(stderr, "code:%s\n", code_str);
> diff --git a/qemu-kvm.c b/qemu-kvm.c
> index 0724c28..9b1c506 100644
> --- a/qemu-kvm.c
> +++ b/qemu-kvm.c
> @@ -95,18 +95,6 @@ static int kvm_debug(void *opaque, void *data,
>  }
>  #endif
>  
> -int kvm_mmio_read(void *opaque, uint64_t addr, uint8_t *data, int len)
> -{
> -	cpu_physical_memory_rw(addr, data, len, 0);
> -	return 0;
> -}
> -
> -int kvm_mmio_write(void *opaque, uint64_t addr, uint8_t *data, int len)
> -{
> -	cpu_physical_memory_rw(addr, data, len, 1);
> -	return 0;
> -}
> -
>  static int handle_unhandled(kvm_context_t kvm, kvm_vcpu_context_t vcpu,
>                              uint64_t reason)
>  {
> @@ -888,23 +876,17 @@ int kvm_set_mpstate(kvm_vcpu_context_t vcpu, struct kvm_mp_state *mp_state)
>  }
>  #endif
>  
> -static int handle_mmio(kvm_vcpu_context_t vcpu)
> +static void handle_mmio(kvm_vcpu_context_t vcpu)
>  {
>  	unsigned long addr = vcpu->run->mmio.phys_addr;
> -	kvm_context_t kvm = vcpu->kvm;
>  	struct kvm_run *kvm_run = vcpu->run;
>  	void *data = kvm_run->mmio.data;
>  
>  	/* hack: Red Hat 7.1 generates these weird accesses. */
>  	if ((addr > 0xa0000-4 && addr <= 0xa0000) && kvm_run->mmio.len == 3)
> -	    return 0;
> +	    return;
>  
> -	if (kvm_run->mmio.is_write)
> -		return kvm_mmio_write(kvm->opaque, addr, data,
> -					kvm_run->mmio.len);
> -	else
> -		return kvm_mmio_read(kvm->opaque, addr, data,
> -					kvm_run->mmio.len);
> +    cpu_physical_memory_rw(addr, data, kvm_run->mmio.len, kvm_run->mmio.is_write);

Glauber,

The indentation now looks horrible. Applied the kvm_init order patches.

Re: [PATCH v2 6/6] remove kvm_mmio_read and kvm_mmio_write

[Glauber Costa]

On Sat, Jul 25, 2009 at 12:24:41PM -0300, Marcelo Tosatti wrote:
> On Tue, Jul 21, 2009 at 06:13:12PM -0400, Glauber Costa wrote:
> > all they did was to call a qemu function. Call this function instead.
> > 
> > Signed-off-by: Glauber Costa <glommer@redhat.com>
> > ---
> >  qemu-kvm-x86.c |    7 +------
> >  qemu-kvm.c     |   34 ++++++++--------------------------
> >  2 files changed, 9 insertions(+), 32 deletions(-)
> > 
> > diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
> > index 350f272..741ae0a 100644
> > --- a/qemu-kvm-x86.c
> > +++ b/qemu-kvm-x86.c
> > @@ -344,7 +344,6 @@ void kvm_show_code(kvm_vcpu_context_t vcpu)
> >  	unsigned char code;
> >  	char code_str[SHOW_CODE_LEN * 3 + 1];
> >  	unsigned long rip;
> > -	kvm_context_t kvm = vcpu->kvm;
> >  
> >  	r = ioctl(fd, KVM_GET_SREGS, &sregs);
> >  	if (r == -1) {
> > @@ -364,11 +363,7 @@ void kvm_show_code(kvm_vcpu_context_t vcpu)
> >  	for (n = -back_offset; n < SHOW_CODE_LEN-back_offset; ++n) {
> >  		if (n == 0)
> >  			strcat(code_str, " -->");
> > -		r = kvm_mmio_read(kvm->opaque, rip + n, &code, 1);
> > -		if (r < 0) {
> > -			strcat(code_str, " xx");
> > -			continue;
> > -		}
> > +		cpu_physical_memory_rw(rip + n, &code, 1, 0);
> >  		sprintf(code_str + strlen(code_str), " %02x", code);
> >  	}
> >  	fprintf(stderr, "code:%s\n", code_str);
> > diff --git a/qemu-kvm.c b/qemu-kvm.c
> > index 0724c28..9b1c506 100644
> > --- a/qemu-kvm.c
> > +++ b/qemu-kvm.c
> > @@ -95,18 +95,6 @@ static int kvm_debug(void *opaque, void *data,
> >  }
> >  #endif
> >  
> > -int kvm_mmio_read(void *opaque, uint64_t addr, uint8_t *data, int len)
> > -{
> > -	cpu_physical_memory_rw(addr, data, len, 0);
> > -	return 0;
> > -}
> > -
> > -int kvm_mmio_write(void *opaque, uint64_t addr, uint8_t *data, int len)
> > -{
> > -	cpu_physical_memory_rw(addr, data, len, 1);
> > -	return 0;
> > -}
> > -
> >  static int handle_unhandled(kvm_context_t kvm, kvm_vcpu_context_t vcpu,
> >                              uint64_t reason)
> >  {
> > @@ -888,23 +876,17 @@ int kvm_set_mpstate(kvm_vcpu_context_t vcpu, struct kvm_mp_state *mp_state)
> >  }
> >  #endif
> >  
> > -static int handle_mmio(kvm_vcpu_context_t vcpu)
> > +static void handle_mmio(kvm_vcpu_context_t vcpu)
> >  {
> >  	unsigned long addr = vcpu->run->mmio.phys_addr;
> > -	kvm_context_t kvm = vcpu->kvm;
> >  	struct kvm_run *kvm_run = vcpu->run;
> >  	void *data = kvm_run->mmio.data;
> >  
> >  	/* hack: Red Hat 7.1 generates these weird accesses. */
> >  	if ((addr > 0xa0000-4 && addr <= 0xa0000) && kvm_run->mmio.len == 3)
> > -	    return 0;
> > +	    return;
> >  
> > -	if (kvm_run->mmio.is_write)
> > -		return kvm_mmio_write(kvm->opaque, addr, data,
> > -					kvm_run->mmio.len);
> > -	else
> > -		return kvm_mmio_read(kvm->opaque, addr, data,
> > -					kvm_run->mmio.len);
> > +    cpu_physical_memory_rw(addr, data, kvm_run->mmio.len, kvm_run->mmio.is_write);
> 
> Glauber,
> 
> The indentation now looks horrible. Applied the kvm_init order patches.
Gosh... I propose we fix the identation of that file once and for all.

It should be pretty easy and automatic if we convert all tabs to 4-spaces, which
can be done with a straightforward sed script. Do I have your okay to proceed with this,
Master Kivity?

Re: [PATCH v2 5/6] remove kvm_specific kvm_out* functions

[Marcelo Tosatti]

Gleb,

Can you please review this to make sure the handling in acpi.c 
is correct and complete?

On Tue, Jul 21, 2009 at 06:13:11PM -0400, Glauber Costa wrote:
> As example of what was already done with inb.
> This is a little bit more tricky, because of SMM, but those
> bits are handled directly in apic anyway.
> 
> Signed-off-by: Glauber Costa <glommer@redhat.com>
> ---
>  qemu-kvm.c |   60 +++---------------------------------------------------------
>  1 files changed, 3 insertions(+), 57 deletions(-)
> 
> diff --git a/qemu-kvm.c b/qemu-kvm.c
> index cef522d..0724c28 100644
> --- a/qemu-kvm.c
> +++ b/qemu-kvm.c
> @@ -95,55 +95,6 @@ static int kvm_debug(void *opaque, void *data,
>  }
>  #endif
>  
> -#define PM_IO_BASE 0xb000
> -
> -static int kvm_outb(void *opaque, uint16_t addr, uint8_t data)
> -{
> -    if (addr == 0xb2) {
> -	switch (data) {
> -	case 0: {
> -	    cpu_outb(0, 0xb3, 0);
> -	    break;
> -	}
> -	case 0xf0: {
> -	    unsigned x;
> -
> -	    /* enable acpi */
> -	    x = cpu_inw(0, PM_IO_BASE + 4);
> -	    x &= ~1;
> -	    cpu_outw(0, PM_IO_BASE + 4, x);
> -	    break;
> -	}
> -	case 0xf1: {
> -	    unsigned x;
> -
> -	    /* enable acpi */
> -	    x = cpu_inw(0, PM_IO_BASE + 4);
> -	    x |= 1;
> -	    cpu_outw(0, PM_IO_BASE + 4, x);
> -	    break;
> -	}
> -	default:
> -	    break;
> -	}
> -	return 0;
> -    }
> -    cpu_outb(0, addr, data);
> -    return 0;
> -}
> -
> -static int kvm_outw(void *opaque, uint16_t addr, uint16_t data)
> -{
> -    cpu_outw(0, addr, data);
> -    return 0;
> -}
> -
> -static int kvm_outl(void *opaque, uint16_t addr, uint32_t data)
> -{
> -    cpu_outl(0, addr, data);
> -    return 0;
> -}
> -
>  int kvm_mmio_read(void *opaque, uint64_t addr, uint8_t *data, int len)
>  {
>  	cpu_physical_memory_rw(addr, data, len, 0);
> @@ -825,14 +776,12 @@ static int handle_io(kvm_vcpu_context_t vcpu)
>  	struct kvm_run *run = vcpu->run;
>  	kvm_context_t kvm = vcpu->kvm;
>  	uint16_t addr = run->io.port;
> -	int r;
>  	int i;
>  	void *p = (void *)run + run->io.data_offset;
>  
>  	for (i = 0; i < run->io.count; ++i) {
>  		switch (run->io.direction) {
>  		case KVM_EXIT_IO_IN:
> -			r = 0;
>  			switch (run->io.size) {
>  			case 1:
>  				*(uint8_t *)p = cpu_inb(kvm->opaque, addr);
> @@ -851,16 +800,13 @@ static int handle_io(kvm_vcpu_context_t vcpu)
>  		case KVM_EXIT_IO_OUT:
>  			switch (run->io.size) {
>  			case 1:
> -				r = kvm_outb(kvm->opaque, addr,
> -						     *(uint8_t *)p);
> +				 cpu_outb(kvm->opaque, addr, *(uint8_t *)p);
>  				break;
>  			case 2:
> -				r = kvm_outw(kvm->opaque, addr,
> -						     *(uint16_t *)p);
> +				cpu_outw(kvm->opaque, addr, *(uint16_t *)p);
>  				break;
>  			case 4:
> -				r = kvm_outl(kvm->opaque, addr,
> -						     *(uint32_t *)p);
> +				cpu_outl(kvm->opaque, addr, *(uint32_t *)p);
>  				break;
>  			default:
>  				fprintf(stderr, "bad I/O size %d\n", run->io.size);
> -- 
> 1.6.2.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/6] remove kvm_specific kvm_out* functions

[Gleb Natapov]

On Wed, Jul 22, 2009 at 04:50:28PM -0300, Marcelo Tosatti wrote:
> Gleb,
> 
> Can you please review this to make sure the handling in acpi.c 
> is correct and complete?
> 
The handling in acpi.c is the same except this bit:
   case 0: {
       cpu_outb(0, 0xb3, 0);
       break;
But this doesn't make sense according to a spec and if it is needed
(which I doubt) it should be handled in acpi.c too.

> On Tue, Jul 21, 2009 at 06:13:11PM -0400, Glauber Costa wrote:
> > As example of what was already done with inb.
> > This is a little bit more tricky, because of SMM, but those
> > bits are handled directly in apic anyway.
> > 
> > Signed-off-by: Glauber Costa <glommer@redhat.com>
> > ---
> >  qemu-kvm.c |   60 +++---------------------------------------------------------
> >  1 files changed, 3 insertions(+), 57 deletions(-)
> > 
> > diff --git a/qemu-kvm.c b/qemu-kvm.c
> > index cef522d..0724c28 100644
> > --- a/qemu-kvm.c
> > +++ b/qemu-kvm.c
> > @@ -95,55 +95,6 @@ static int kvm_debug(void *opaque, void *data,
> >  }
> >  #endif
> >  
> > -#define PM_IO_BASE 0xb000
> > -
> > -static int kvm_outb(void *opaque, uint16_t addr, uint8_t data)
> > -{
> > -    if (addr == 0xb2) {
> > -	switch (data) {
> > -	case 0: {
> > -	    cpu_outb(0, 0xb3, 0);
> > -	    break;
> > -	}
> > -	case 0xf0: {
> > -	    unsigned x;
> > -
> > -	    /* enable acpi */
> > -	    x = cpu_inw(0, PM_IO_BASE + 4);
> > -	    x &= ~1;
> > -	    cpu_outw(0, PM_IO_BASE + 4, x);
> > -	    break;
> > -	}
> > -	case 0xf1: {
> > -	    unsigned x;
> > -
> > -	    /* enable acpi */
> > -	    x = cpu_inw(0, PM_IO_BASE + 4);
> > -	    x |= 1;
> > -	    cpu_outw(0, PM_IO_BASE + 4, x);
> > -	    break;
> > -	}
> > -	default:
> > -	    break;
> > -	}
> > -	return 0;
> > -    }
> > -    cpu_outb(0, addr, data);
> > -    return 0;
> > -}
> > -
> > -static int kvm_outw(void *opaque, uint16_t addr, uint16_t data)
> > -{
> > -    cpu_outw(0, addr, data);
> > -    return 0;
> > -}
> > -
> > -static int kvm_outl(void *opaque, uint16_t addr, uint32_t data)
> > -{
> > -    cpu_outl(0, addr, data);
> > -    return 0;
> > -}
> > -
> >  int kvm_mmio_read(void *opaque, uint64_t addr, uint8_t *data, int len)
> >  {
> >  	cpu_physical_memory_rw(addr, data, len, 0);
> > @@ -825,14 +776,12 @@ static int handle_io(kvm_vcpu_context_t vcpu)
> >  	struct kvm_run *run = vcpu->run;
> >  	kvm_context_t kvm = vcpu->kvm;
> >  	uint16_t addr = run->io.port;
> > -	int r;
> >  	int i;
> >  	void *p = (void *)run + run->io.data_offset;
> >  
> >  	for (i = 0; i < run->io.count; ++i) {
> >  		switch (run->io.direction) {
> >  		case KVM_EXIT_IO_IN:
> > -			r = 0;
> >  			switch (run->io.size) {
> >  			case 1:
> >  				*(uint8_t *)p = cpu_inb(kvm->opaque, addr);
> > @@ -851,16 +800,13 @@ static int handle_io(kvm_vcpu_context_t vcpu)
> >  		case KVM_EXIT_IO_OUT:
> >  			switch (run->io.size) {
> >  			case 1:
> > -				r = kvm_outb(kvm->opaque, addr,
> > -						     *(uint8_t *)p);
> > +				 cpu_outb(kvm->opaque, addr, *(uint8_t *)p);
> >  				break;
> >  			case 2:
> > -				r = kvm_outw(kvm->opaque, addr,
> > -						     *(uint16_t *)p);
> > +				cpu_outw(kvm->opaque, addr, *(uint16_t *)p);
> >  				break;
> >  			case 4:
> > -				r = kvm_outl(kvm->opaque, addr,
> > -						     *(uint32_t *)p);
> > +				cpu_outl(kvm->opaque, addr, *(uint32_t *)p);
> >  				break;
> >  			default:
> >  				fprintf(stderr, "bad I/O size %d\n", run->io.size);
> > -- 
> > 1.6.2.2
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.

Re: [PATCH v2 3/6] remove kvm_abi variable

[Marcelo Tosatti]

On Tue, Jul 21, 2009 at 06:13:09PM -0400, Glauber Costa wrote:
> We're not using this for anything
> 
> Signed-off-by: Glauber Costa <glommer@redhat.com>
> ---
>  qemu-kvm.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-kvm.c b/qemu-kvm.c
> index 8eeace4..a8298e4 100644
> --- a/qemu-kvm.c
> +++ b/qemu-kvm.c
> @@ -43,7 +43,6 @@ int kvm_pit = 1;
>  int kvm_pit_reinject = 1;
>  int kvm_nested = 0;
>  
> -
>  static KVMState *kvm_state;
>  kvm_context_t kvm_context;
>  
> @@ -79,7 +78,6 @@ static LIST_HEAD(, ioperm_data) ioperm_head;
>  
>  #define ALIGN(x, y) (((x)+(y)-1) & ~((y)-1))
>  
> -int kvm_abi = EXPECTED_KVM_API_VERSION;
>  int kvm_page_size;
>  
>  #ifdef KVM_CAP_SET_GUEST_DEBUG
> @@ -429,7 +427,6 @@ int kvm_init(int smp_cpus)
>  	    fprintf(stderr, "kvm userspace version too old\n");
>  	    goto out_close;
>  	}
> -	kvm_abi = r;
>  	kvm_page_size = getpagesize();
>  	kvm_state = qemu_mallocz(sizeof(*kvm_state));
>      kvm_context = &kvm_state->kvm_context;
> -- 

Forgot qemu-kvm.h? Applied 1,2,3 and 5 (hopefully nobody outside vl.c
is playing with stopped/stop/created?).

Re: [PATCH v2 2/6] reuse env stop and stopped states

[Avi Kivity]

On 07/22/2009 01:13 AM, Glauber Costa wrote:
> qemu CPUState already provides "stop" and "stopped" states. And they
> mean exactly that. There is no need for us to provide our own.
>
>    

This patch (known as dd0e1c1a589 in qemu-kvm.git) breaks reboot.  My 
test case is FC6 i386 -smp 2, running the reboot command in rc.local.  
In about 15 minutes qemu hangs hard.  Please check what's gone wrong.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH v2 2/6] reuse env stop and stopped states

[Glauber Costa]

On Mon, Jul 27, 2009 at 06:43:47PM +0300, Avi Kivity wrote:
> On 07/22/2009 01:13 AM, Glauber Costa wrote:
>> qemu CPUState already provides "stop" and "stopped" states. And they
>> mean exactly that. There is no need for us to provide our own.
>>
>>    
>
> This patch (known as dd0e1c1a589 in qemu-kvm.git) breaks reboot.  My  
> test case is FC6 i386 -smp 2, running the reboot command in rc.local.   
> In about 15 minutes qemu hangs hard.  Please check what's gone wrong.
I found out that doing kill -38 <your_pid> makes it run again, so we're likely
hanging somewhere while holding qemu_mutex. The state of the process is "D",
so we're holding qemu_mutex, and then calling something that can block.

It's hard for me to believe that this patch introduced it. At best, it might have
made it more likely. Also, I also verified that it sometimes takes a while until
it happen for the first time. Are you sure this is the first patch that makes it happen?

Re: [PATCH v2 2/6] reuse env stop and stopped states

[Avi Kivity]

On 07/28/2009 03:48 AM, Glauber Costa wrote:
> On Mon, Jul 27, 2009 at 06:43:47PM +0300, Avi Kivity wrote:
>    
>> On 07/22/2009 01:13 AM, Glauber Costa wrote:
>>      
>>> qemu CPUState already provides "stop" and "stopped" states. And they
>>> mean exactly that. There is no need for us to provide our own.
>>>
>>>
>>>        
>> This patch (known as dd0e1c1a589 in qemu-kvm.git) breaks reboot.  My
>> test case is FC6 i386 -smp 2, running the reboot command in rc.local.
>> In about 15 minutes qemu hangs hard.  Please check what's gone wrong.
>>      
> I found out that doing kill -38<your_pid>  makes it run again, so we're likely
> hanging somewhere while holding qemu_mutex. The state of the process is "D",
> so we're holding qemu_mutex, and then calling something that can block.
>    

Sounds like we call a vcpu ioctl from the iothread (or from a different 
vcpu thread).

> It's hard for me to believe that this patch introduced it. At best, it might have
> made it more likely. Also, I also verified that it sometimes takes a while until
> it happen for the first time. Are you sure this is the first patch that makes it happen?
>    

I haven't been able to reproduce it before this patch.  Maybe this patch 
doesn't introduce it, only exposes it.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH v2 2/6] reuse env stop and stopped states

[Gleb Natapov]

On Tue, Jul 28, 2009 at 09:17:05AM +0300, Avi Kivity wrote:
> On 07/28/2009 03:48 AM, Glauber Costa wrote:
>> On Mon, Jul 27, 2009 at 06:43:47PM +0300, Avi Kivity wrote:
>>    
>>> On 07/22/2009 01:13 AM, Glauber Costa wrote:
>>>      
>>>> qemu CPUState already provides "stop" and "stopped" states. And they
>>>> mean exactly that. There is no need for us to provide our own.
>>>>
>>>>
>>>>        
>>> This patch (known as dd0e1c1a589 in qemu-kvm.git) breaks reboot.  My
>>> test case is FC6 i386 -smp 2, running the reboot command in rc.local.
>>> In about 15 minutes qemu hangs hard.  Please check what's gone wrong.
>>>      
>> I found out that doing kill -38<your_pid>  makes it run again, so we're likely
>> hanging somewhere while holding qemu_mutex. The state of the process is "D",
>> so we're holding qemu_mutex, and then calling something that can block.
>>    
>
> Sounds like we call a vcpu ioctl from the iothread (or from a different  
> vcpu thread).
>
>> It's hard for me to believe that this patch introduced it. At best, it might have
>> made it more likely. Also, I also verified that it sometimes takes a while until
>> it happen for the first time. Are you sure this is the first patch that makes it happen?
>>    
>
> I haven't been able to reproduce it before this patch.  Maybe this patch  
> doesn't introduce it, only exposes it.
>
What are backtraces of all threads when it happens?

--
			Gleb.

Re: [PATCH v2 2/6] reuse env stop and stopped states

[Avi Kivity]

On 07/28/2009 09:24 AM, Gleb Natapov wrote:
>
> What are backtraces of all threads when it happens?
>    

I wasn't able to attach with gdb.  But I thought you reproduced it?

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH v2 2/6] reuse env stop and stopped states

[Gleb Natapov]

On Tue, Jul 28, 2009 at 09:28:26AM +0300, Avi Kivity wrote:
> On 07/28/2009 09:24 AM, Gleb Natapov wrote:
>>
>> What are backtraces of all threads when it happens?
>>    
>
> I wasn't able to attach with gdb.  But I thought you reproduced it?
>
Glauber may be yes. Me not even tried :)

--
			Gleb.

Re: [PATCH v2 2/6] reuse env stop and stopped states

[Avi Kivity]

On 07/28/2009 09:29 AM, Gleb Natapov wrote:
> On Tue, Jul 28, 2009 at 09:28:26AM +0300, Avi Kivity wrote:
>    
>> On 07/28/2009 09:24 AM, Gleb Natapov wrote:
>>      
>>> What are backtraces of all threads when it happens?
>>>
>>>        
>> I wasn't able to attach with gdb.  But I thought you reproduced it?
>>
>>      
> Glauber may be yes. Me not even tried :)
>
>    

Ah sorry.  I only read the first two letters of any name.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH v2 2/6] reuse env stop and stopped states

[Avi Kivity]

On 07/28/2009 09:17 AM, Avi Kivity wrote:
>> I found out that doing kill -38<your_pid>  makes it run again, so 
>> we're likely
>> hanging somewhere while holding qemu_mutex. The state of the process 
>> is "D",
>> so we're holding qemu_mutex, and then calling something that can block.
>
> Sounds like we call a vcpu ioctl from the iothread (or from a 
> different vcpu thread).

That's indeed the case.  We reload the local apic state from the 
iothread instead of the vcpu thread.  Please write a patch to fix this.

>> It's hard for me to believe that this patch introduced it. At best, 
>> it might have
>> made it more likely. Also, I also verified that it sometimes takes a 
>> while until
>> it happen for the first time. Are you sure this is the first patch 
>> that makes it happen?
>
> I haven't been able to reproduce it before this patch.  Maybe this 
> patch doesn't introduce it, only exposes it.
>

It does.  The root problem is that env->stopped is cleared during reset, 
so pause_all_threads() doesn't work:

     uint32_t stop;   /* Stop request */                                 \
     uint32_t stopped; /* Artificially stopped */                        \
...
     /* from this point: preserved by CPU reset */                       \

This kind of bug is incredibly hard to find - you now owe Gleb a solar 
mass worth of beer.  IMO we shouldn't be coding like this, please patch 
upstream to explicitly clear what needs clearing.

I'm now testing the simple fix (moving the variables after the memset 
point).

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