[Qemu-devel] [PATCH 0/7] KVM SMP support, early version

[Qemu-devel] [PATCH 0/7] KVM SMP support, early version

[Glauber Costa]

Hi guys,

This is an early version of smp support in kvm that kinda works.
It has some known problems that I am still tracking. For example,
it does not reset very well. Also, initialization is a bit slow,
probably because of the number of remote ioctl calls involved.
But I believe Jan's patch to do a single ioctl can make it better.

Normal operation seem to be going smooth for me. I can boot the
machine and get it going decently. I'll keep working on the issues,
but comments are welcome in the mean time.

[Qemu-devel] [PATCH 1/7] Don't mess with halted state.

[Glauber Costa]

When we have irqchip in kernel, halted state is kernel
business. So don't initialize it in our code.

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

diff --git a/hw/apic-kvm.c b/hw/apic-kvm.c
index 089fa45..e5a0bfc 100644
--- a/hw/apic-kvm.c
+++ b/hw/apic-kvm.c
@@ -122,10 +122,9 @@ static void kvm_apic_reset(void *opaque)
     s->dev.next_time = 0;
     s->dev.wait_for_sipi = 1;
 
-    s->cpu_env->halted = !(s->dev.apicbase & MSR_IA32_APICBASE_BSP);
 
     s->cpu_env->mp_state
-            = s->cpu_env->halted ? KVM_MP_STATE_UNINITIALIZED : KVM_MP_STATE_RUNNABLE;
+            = bsp ? KVM_MP_STATE_RUNNABLE : KVM_MP_STATE_UNINITIALIZED;
 
     kvm_put_mp_state(s->cpu_env);
 
-- 
1.6.5.2

[Qemu-devel] [PATCH 2/7] store thread-specific env information

[Glauber Costa]

Since we'll have multiple cpu threads, at least for kvm, we need a way to store
and retrieve the CPUState associated with the current execution thread.
For the I/O thread, this will be NULL.

I am using pthread functions for that, for portability, but we could as well
use __thread keyword.

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

diff --git a/vl.c b/vl.c
index ee43808..9afe4b6 100644
--- a/vl.c
+++ b/vl.c
@@ -3436,6 +3436,24 @@ static void block_io_signals(void);
 static void unblock_io_signals(void);
 static int tcg_has_work(void);
 
+static pthread_key_t current_env;
+
+CPUState *qemu_get_current_env(void);
+CPUState *qemu_get_current_env(void)
+{
+    return pthread_getspecific(current_env);
+}
+
+static void qemu_set_current_env(CPUState *env)
+{
+    pthread_setspecific(current_env, env);
+}
+
+static void qemu_init_current_env(void)
+{
+    pthread_key_create(&current_env, NULL);
+}
+
 static int qemu_init_main_loop(void)
 {
     int ret;
@@ -3448,6 +3466,7 @@ static int qemu_init_main_loop(void)
     qemu_mutex_init(&qemu_fair_mutex);
     qemu_mutex_init(&qemu_global_mutex);
     qemu_mutex_lock(&qemu_global_mutex);
+    qemu_init_current_env();
 
     unblock_io_signals();
     qemu_thread_self(&io_thread);
@@ -3486,6 +3505,8 @@ static void *kvm_cpu_thread_fn(void *arg)
 
     block_io_signals();
     qemu_thread_self(env->thread);
+    qemu_set_current_env(env);
+
     if (kvm_enabled())
         kvm_init_vcpu(env);
 
-- 
1.6.5.2

[Qemu-devel] [PATCH 3/7] update halted state on mp_state sync

[Glauber Costa]

If we are using in-kernel irqchip, halted state belongs in the kernel.
So everytime we grab kernel's idea of mpstate, we also need to propagate
halted state to userspace.

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

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 78f6d65..1c4f660 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -825,6 +825,11 @@ static int kvm_get_mp_state(CPUState *env)
         return ret;
     }
     env->mp_state = mp_state.mp_state;
+
+    if (kvm_irqchip_in_kernel()) {
+        env->halted = (env->mp_state == KVM_MP_STATE_HALTED);
+    }
+
     return 0;
 }
 
-- 
1.6.5.2

[Qemu-devel] [PATCH 4/7] qemu_flush_work for remote vcpu execution

[Glauber Costa]

This function is similar to qemu-kvm's on_vcpu mechanism. Totally synchronous,
and guarantees that a given function will be executed at the specified vcpu.

The approach I am taking is to put it under the hood, in kvm_vcpu_ioctl.
This way, the kvm_vcpu_ioctl can be used anywhere, and we guarantee it will never
be executed outside its realm.

This is not much of a problem, since remote execution is rare. It does happen at
lot at machine bootup, because saving/restoring registers spans a lot of ioctls,
but this should get better if we move to Jan's patch of doing it all at once.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 cpu-all.h  |    3 ++
 cpu-defs.h |   14 +++++++++++
 kvm-all.c  |   75 ++++++++++++++++++++++++-----------------------------------
 kvm.h      |    7 +++++
 vl.c       |   63 ++++++++++++++++++++++++++++++++++++++++++++++----
 5 files changed, 113 insertions(+), 49 deletions(-)

diff --git a/cpu-all.h b/cpu-all.h
index ebe8bfb..3eb865d 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -763,6 +763,9 @@ extern CPUState *cpu_single_env;
 extern int64_t qemu_icount;
 extern int use_icount;
 
+void qemu_queue_work(CPUState *env, void (*func)(void *data), void *data);
+void qemu_flush_work(CPUState *env);
+
 #define CPU_INTERRUPT_HARD   0x02 /* hardware interrupt pending */
 #define CPU_INTERRUPT_EXITTB 0x04 /* exit the current TB (use for x86 a20 case) */
 #define CPU_INTERRUPT_TIMER  0x08 /* internal timer exception pending */
diff --git a/cpu-defs.h b/cpu-defs.h
index 95068b5..18792fc 100644
--- a/cpu-defs.h
+++ b/cpu-defs.h
@@ -31,6 +31,8 @@
 #include "qemu-queue.h"
 #include "targphys.h"
 
+#include "qemu-thread.h"
+
 #ifndef TARGET_LONG_BITS
 #error TARGET_LONG_BITS must be defined before including this header
 #endif
@@ -134,6 +136,13 @@ typedef struct CPUWatchpoint {
     QTAILQ_ENTRY(CPUWatchpoint) entry;
 } CPUWatchpoint;
 
+typedef struct QemuWorkItem {
+    void (*func)(void *data);
+    void *data;
+    int done;
+} QemuWorkItem;
+
+
 #define CPU_TEMP_BUF_NLONGS 128
 #define CPU_COMMON                                                      \
     struct TranslationBlock *current_tb; /* currently executing TB  */  \
@@ -175,6 +184,10 @@ typedef struct CPUWatchpoint {
     QTAILQ_HEAD(watchpoints_head, CPUWatchpoint) watchpoints;            \
     CPUWatchpoint *watchpoint_hit;                                      \
                                                                         \
+    QemuWorkItem queued_work;                                           \
+    uint64_t queued_local, queued_total;                                \
+    struct QemuMutex queue_lock;                                        \
+                                                                        \
     struct GDBRegisterState *gdb_regs;                                  \
                                                                         \
     /* Core interrupt code */                                           \
@@ -194,6 +207,7 @@ typedef struct CPUWatchpoint {
     uint32_t created;                                                   \
     struct QemuThread *thread;                                          \
     struct QemuCond *halt_cond;                                         \
+    struct QemuCond work_cond;                                          \
     const char *cpu_model_str;                                          \
     struct KVMState *kvm_state;                                         \
     struct kvm_run *kvm_run;                                            \
diff --git a/kvm-all.c b/kvm-all.c
index 0c0b3c3..28f7ab7 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -636,7 +636,7 @@ int kvm_cpu_exec(CPUState *env)
     struct kvm_run *run = env->kvm_run;
     int ret;
 
-    dprintf("kvm_cpu_exec()\n");
+    dprintf("kvm_cpu_exec() %d\n", env->cpu_index);
 
     do {
         if (env->exit_request) {
@@ -904,9 +904,22 @@ int kvm_vm_ioctl(KVMState *s, int type, ...)
     return ret;
 }
 
-int kvm_vcpu_ioctl(CPUState *env, int type, ...)
+CPUState *qemu_get_current_env(void);
+static void kvm_remote_ioctl(void *data)
 {
+    KVMIoctl *arg = data;
     int ret;
+
+    ret = ioctl(arg->fd, arg->type, arg->data);
+    if (ret == -1)
+        ret = -errno;
+    arg->ret = ret;
+}
+
+KVMIoctl data;
+
+int kvm_vcpu_ioctl(CPUState *env, int type, ...)
+{
     void *arg;
     va_list ap;
 
@@ -914,11 +927,12 @@ int kvm_vcpu_ioctl(CPUState *env, int type, ...)
     arg = va_arg(ap, void *);
     va_end(ap);
 
-    ret = ioctl(env->kvm_fd, type, arg);
-    if (ret == -1)
-        ret = -errno;
+    data.type = type;
+    data.data = arg;
+    data.fd = env->kvm_fd;
 
-    return ret;
+    qemu_queue_work(env, kvm_remote_ioctl, (void *)&data);
+    return data.ret;
 }
 
 int kvm_has_sync_mmu(void)
@@ -951,19 +965,6 @@ void kvm_setup_guest_memory(void *start, size_t size)
 }
 
 #ifdef KVM_CAP_SET_GUEST_DEBUG
-static void on_vcpu(CPUState *env, void (*func)(void *data), void *data)
-{
-#ifdef CONFIG_IOTHREAD
-    if (env == cpu_single_env) {
-        func(data);
-        return;
-    }
-    abort();
-#else
-    func(data);
-#endif
-}
-
 struct kvm_sw_breakpoint *kvm_find_sw_breakpoint(CPUState *env,
                                                  target_ulong pc)
 {
@@ -981,38 +982,24 @@ int kvm_sw_breakpoints_active(CPUState *env)
     return !QTAILQ_EMPTY(&env->kvm_state->kvm_sw_breakpoints);
 }
 
-struct kvm_set_guest_debug_data {
-    struct kvm_guest_debug dbg;
-    CPUState *env;
-    int err;
-};
-
-static void kvm_invoke_set_guest_debug(void *data)
+int kvm_update_guest_debug(CPUState *env, unsigned long reinject_trap)
 {
-    struct kvm_set_guest_debug_data *dbg_data = data;
-    CPUState *env = dbg_data->env;
+    struct kvm_guest_debug dbg;
+  
+    dbg.control = 0;
+    if (env->singlestep_enabled)
+        dbg.control = KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
+  
+    kvm_arch_update_guest_debug(env, &dbg);
+    dbg.control |= reinject_trap;
 
     if (env->kvm_state->regs_modified) {
         kvm_arch_put_registers(env);
         env->kvm_state->regs_modified = 0;
     }
-    dbg_data->err = kvm_vcpu_ioctl(env, KVM_SET_GUEST_DEBUG, &dbg_data->dbg);
-}
-
-int kvm_update_guest_debug(CPUState *env, unsigned long reinject_trap)
-{
-    struct kvm_set_guest_debug_data data;
-
-    data.dbg.control = 0;
-    if (env->singlestep_enabled)
-        data.dbg.control = KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
-
-    kvm_arch_update_guest_debug(env, &data.dbg);
-    data.dbg.control |= reinject_trap;
-    data.env = env;
+ 
+    return kvm_vcpu_ioctl(env, KVM_SET_GUEST_DEBUG, &dbg);
 
-    on_vcpu(env, kvm_invoke_set_guest_debug, &data);
-    return data.err;
 }
 
 int kvm_insert_breakpoint(CPUState *current_env, target_ulong addr,
diff --git a/kvm.h b/kvm.h
index a474d95..d42b720 100644
--- a/kvm.h
+++ b/kvm.h
@@ -149,4 +149,11 @@ static inline void cpu_synchronize_state(CPUState *env)
     }
 }
 
+typedef struct KVMIoctl {
+    int fd;
+    int type;
+    int ret;
+    void *data;
+} KVMIoctl;
+
 #endif
diff --git a/vl.c b/vl.c
index 9afe4b6..91e2264 100644
--- a/vl.c
+++ b/vl.c
@@ -3405,6 +3405,11 @@ void qemu_notify_event(void)
     }
 }
 
+void qemu_queue_work(CPUState *env, void (*func)(void *data), void *data, int wait)
+{
+        func(data);
+}
+
 void qemu_mutex_lock_iothread(void) {}
 void qemu_mutex_unlock_iothread(void) {}
 
@@ -3438,8 +3443,7 @@ static int tcg_has_work(void);
 
 static pthread_key_t current_env;
 
-CPUState *qemu_get_current_env(void);
-CPUState *qemu_get_current_env(void)
+static CPUState *qemu_get_current_env(void)
 {
     return pthread_getspecific(current_env);
 }
@@ -3476,8 +3480,10 @@ static int qemu_init_main_loop(void)
 
 static void qemu_wait_io_event(CPUState *env)
 {
-    while (!tcg_has_work())
+    while (!tcg_has_work()) {
+	qemu_flush_work(env);
         qemu_cond_timedwait(env->halt_cond, &qemu_global_mutex, 1000);
+    }
 
     qemu_mutex_unlock(&qemu_global_mutex);
 
@@ -3490,6 +3496,7 @@ static void qemu_wait_io_event(CPUState *env)
     qemu_mutex_unlock(&qemu_fair_mutex);
 
     qemu_mutex_lock(&qemu_global_mutex);
+
     if (env->stop) {
         env->stop = 0;
         env->stopped = 1;
@@ -3516,8 +3523,11 @@ static void *kvm_cpu_thread_fn(void *arg)
     qemu_cond_signal(&qemu_cpu_cond);
 
     /* and wait for machine initialization */
-    while (!qemu_system_ready)
+    while (!qemu_system_ready) {
+        /* system reset and initialization is a heavy caller of queue_work() */
+        qemu_flush_work(env);
         qemu_cond_timedwait(&qemu_system_cond, &qemu_global_mutex, 100);
+    }
 
     while (1) {
         if (cpu_can_run(env))
@@ -3544,8 +3554,10 @@ static void *tcg_cpu_thread_fn(void *arg)
     qemu_cond_signal(&qemu_cpu_cond);
 
     /* and wait for machine initialization */
-    while (!qemu_system_ready)
+    while (!qemu_system_ready) {
+        qemu_flush_work(env);
         qemu_cond_timedwait(&qemu_system_cond, &qemu_global_mutex, 100);
+    }
 
     while (1) {
         tcg_cpu_exec();
@@ -3563,6 +3575,45 @@ void qemu_cpu_kick(void *_env)
         qemu_thread_signal(env->thread, SIGUSR1);
 }
 
+void qemu_queue_work(CPUState *env, void (*func)(void *data), void *data)
+{
+    QemuWorkItem *wii;
+
+    env->queued_total++;
+
+    if (env == qemu_get_current_env()) {
+        env->queued_local++;
+        func(data);
+        return;
+    }
+
+    wii = &env->queued_work;
+    wii->func = func;
+    wii->data = data;
+    wii->done = 0;
+
+    qemu_thread_signal(env->thread, SIGUSR1);
+
+    while (!wii->done) {
+        qemu_cond_wait(&env->work_cond, &qemu_global_mutex);
+    }
+
+    wii->func = NULL;
+}
+
+void qemu_flush_work(CPUState *env)
+{
+    QemuWorkItem *wi;
+
+    wi = &env->queued_work;
+    if (wi->func) {
+     //   printf("executing func %p\n", wi->func);
+        wi->func(wi->data);
+        wi->done = 1;
+    }
+    qemu_cond_broadcast(&env->work_cond);
+}
+
 int qemu_cpu_self(void *_env)
 {
     CPUState *env = _env;
@@ -3721,6 +3772,8 @@ void qemu_init_vcpu(void *_env)
 {
     CPUState *env = _env;
 
+    qemu_cond_init(&env->work_cond);
+
     if (kvm_enabled())
         kvm_start_vcpu(env);
     else
-- 
1.6.5.2

[Qemu-devel] [PATCH 5/7] tell kernel about all registers instead of just mp_state

[Glauber Costa]

This fix a bug with -smp in kvm. Since we have updated apic_base,
we also have to tell kernel about it. So instead of just updating
mp_state, update every regs.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 hw/apic-kvm.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/hw/apic-kvm.c b/hw/apic-kvm.c
index e5a0bfc..dc61386 100644
--- a/hw/apic-kvm.c
+++ b/hw/apic-kvm.c
@@ -126,7 +126,10 @@ static void kvm_apic_reset(void *opaque)
     s->cpu_env->mp_state
             = bsp ? KVM_MP_STATE_RUNNABLE : KVM_MP_STATE_UNINITIALIZED;
 
-    kvm_put_mp_state(s->cpu_env);
+    /* We have to tell the kernel about mp_state, but also save sregs, since
+     * apic base was just updated
+     */
+    kvm_arch_put_registers(s->cpu_env);
 
     if (bsp) {
         /*
-- 
1.6.5.2

[Qemu-devel] [PATCH 6/7] Don't call kvm cpu reset on initialization

[Glauber Costa]

All reset functions are called from the same place, and this was a leftover

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

diff --git a/kvm-all.c b/kvm-all.c
index 28f7ab7..dbd69f3 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -204,8 +204,6 @@ int kvm_init_vcpu(CPUState *env)
     ret = kvm_arch_init_vcpu(env);
     if (ret == 0) {
         qemu_register_reset(kvm_reset_vcpu, env);
-        kvm_arch_reset_vcpu(env);
-        ret = kvm_arch_put_registers(env);
     }
 err:
     return ret;
-- 
1.6.5.2

[Qemu-devel] [PATCH 7/7] remove smp restriction from kvm

[Glauber Costa]

We don't support smp without irqchip in kernel, so only abort in
that situation

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 kvm-all.c |   18 +++++++++++-------
 1 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index dbd69f3..4134be3 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -439,16 +439,25 @@ static int kvm_create_irqchip(KVMState *s)
 {
     int ret = 0;
 #if defined(CONFIG_IOTHREAD)
+    ret = -1;
     if (!kvm_check_extension(s, KVM_CAP_IRQCHIP))
-        return -1;
+        goto failed;
 
     ret = kvm_vm_ioctl(s, KVM_CREATE_IRQCHIP);
     if (ret < 0)
-        return ret;
+        goto failed;
 
     s->irqchip_in_kernel = 1;
+
 #endif
     return ret;
+
+failed:
+    if (smp_cpus > 1) {
+        fprintf(stderr, "No SMP KVM support, use '-smp 1'\n");
+        ret = -EINVAL;
+    }
+    return ret;
 }
 
 int kvm_init(int smp_cpus)
@@ -460,11 +469,6 @@ int kvm_init(int smp_cpus)
     int ret;
     int i;
 
-    if (smp_cpus > 1) {
-        fprintf(stderr, "No SMP KVM support, use '-smp 1'\n");
-        return -EINVAL;
-    }
-
     s = qemu_mallocz(sizeof(KVMState));
 
 #ifdef KVM_CAP_SET_GUEST_DEBUG
-- 
1.6.5.2

Re: [Qemu-devel] [PATCH 5/7] tell kernel about all registers instead of just mp_state

[Avi Kivity]

On 11/26/2009 07:24 PM, Glauber Costa wrote:
> This fix a bug with -smp in kvm. Since we have updated apic_base,
> we also have to tell kernel about it. So instead of just updating
> mp_state, update every regs.
>
> Signed-off-by: Glauber Costa<glommer@redhat.com>
> ---
>   hw/apic-kvm.c |    5 ++++-
>   1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/hw/apic-kvm.c b/hw/apic-kvm.c
> index e5a0bfc..dc61386 100644
> --- a/hw/apic-kvm.c
> +++ b/hw/apic-kvm.c
> @@ -126,7 +126,10 @@ static void kvm_apic_reset(void *opaque)
>       s->cpu_env->mp_state
>               = bsp ? KVM_MP_STATE_RUNNABLE : KVM_MP_STATE_UNINITIALIZED;
>
> -    kvm_put_mp_state(s->cpu_env);
> +    /* We have to tell the kernel about mp_state, but also save sregs, since
> +     * apic base was just updated
> +     */
> +    kvm_arch_put_registers(s->cpu_env);
>
>    

Better to use cpu_synchronize_state() instead.

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

Re: [Qemu-devel] [PATCH 5/7] tell kernel about all registers instead of just mp_state

[Glauber Costa]

>> since
>> +     * apic base was just updated
>> +     */
>> +    kvm_arch_put_registers(s->cpu_env);
>>
>>
>
> Better to use cpu_synchronize_state() instead.
>
I might be misreading it, but :

void kvm_cpu_synchronize_state(CPUState *env)
{
    if (!env->kvm_state->regs_modified) {
        kvm_arch_get_registers(env);
        env->kvm_state->regs_modified = 1;
    }
}

Only does get. And we need put.

Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [Qemu-devel] [PATCH 5/7] tell kernel about all registers instead of just mp_state

[Gleb Natapov]

On Mon, Nov 30, 2009 at 09:45:56AM -0200, Glauber Costa wrote:
> >> since
> >> +     * apic base was just updated
> >> +     */
> >> +    kvm_arch_put_registers(s->cpu_env);
> >>
> >>
> >
> > Better to use cpu_synchronize_state() instead.
> >
> I might be misreading it, but :
> 
> void kvm_cpu_synchronize_state(CPUState *env)
> {
>     if (!env->kvm_state->regs_modified) {
>         kvm_arch_get_registers(env);
>         env->kvm_state->regs_modified = 1;
>     }
> }
> 
> Only does get. And we need put.
> 
Put is on the entry into the kernel in vcpu_run.

--
			Gleb.

Re: [Qemu-devel] [PATCH 5/7] tell kernel about all registers instead of just mp_state

[Avi Kivity]

On 11/30/2009 01:45 PM, Glauber Costa wrote:
>>
>> Better to use cpu_synchronize_state() instead.
>>
>>      
> I might be misreading it, but :
>
> void kvm_cpu_synchronize_state(CPUState *env)
> {
>      if (!env->kvm_state->regs_modified) {
>          kvm_arch_get_registers(env);
>          env->kvm_state->regs_modified = 1;
>      }
> }
>
> Only does get. And we need put.
>    

It will autoput during the next guest entry.

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

Re: [Qemu-devel] [PATCH 5/7] tell kernel about all registers instead of just mp_state

[Glauber Costa]

On Mon, Nov 30, 2009 at 10:05 AM, Avi Kivity <avi@redhat.com> wrote:
> On 11/30/2009 01:45 PM, Glauber Costa wrote:
>>>
>>> Better to use cpu_synchronize_state() instead.
>>>
>>>
>>
>> I might be misreading it, but :
>>
>> void kvm_cpu_synchronize_state(CPUState *env)
>> {
>>     if (!env->kvm_state->regs_modified) {
>>         kvm_arch_get_registers(env);
>>         env->kvm_state->regs_modified = 1;
>>     }
>> }
>>
>> Only does get. And we need put.
>>
>
> It will autoput during the next guest entry.

So it should be working already, no?

We do a cpu_synchronize_state() on the beginning of that function, and
vcpu does not run
until it is finished.

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



-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [Qemu-devel] [PATCH 5/7] tell kernel about all registers instead of just mp_state

[Avi Kivity]

On 11/30/2009 03:31 PM, Glauber Costa wrote:
>
>>> Only does get. And we need put.
>>>
>>>        
>> It will autoput during the next guest entry.
>>      
> So it should be working already, no?
>
> We do a cpu_synchronize_state() on the beginning of that function, and
> vcpu does not run
> until it is finished

Yes.

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

Re: [Qemu-devel] [PATCH 4/7] qemu_flush_work for remote vcpu execution

[Avi Kivity]

On 11/26/2009 07:24 PM, Glauber Costa wrote:
> This function is similar to qemu-kvm's on_vcpu mechanism. Totally synchronous,
> and guarantees that a given function will be executed at the specified vcpu.
>
> The approach I am taking is to put it under the hood, in kvm_vcpu_ioctl.
> This way, the kvm_vcpu_ioctl can be used anywhere, and we guarantee it will never
> be executed outside its realm.
>
> This is not much of a problem, since remote execution is rare. It does happen at
> lot at machine bootup, because saving/restoring registers spans a lot of ioctls,
> but this should get better if we move to Jan's patch of doing it all at once.
>    

I really dislike this.  In general vcpu ioctls are used as components of 
some work to be done, for example RMW of some state.  In this case it is 
meaningless to execute the ioctls remotely, you need to execute the 
entire RMW remotely instead.

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

Re: [Qemu-devel] [PATCH 4/7] qemu_flush_work for remote vcpu execution

[Glauber Costa]

>
> I really dislike this.  In general vcpu ioctls are used as components of
> some work to be done, for example RMW of some state.  In this case it is
> meaningless to execute the ioctls remotely, you need to execute the entire
> RMW remotely instead.
>

Why? The "M" part of RMW is executed in shared memory.
Only the R and W parts have any restrictions on where to be executed.

-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [Qemu-devel] [PATCH 4/7] qemu_flush_work for remote vcpu execution

[Avi Kivity]

On 11/30/2009 01:44 PM, Glauber Costa wrote:
>> I really dislike this.  In general vcpu ioctls are used as components of
>> some work to be done, for example RMW of some state.  In this case it is
>> meaningless to execute the ioctls remotely, you need to execute the entire
>> RMW remotely instead.
>>
>>      
> Why? The "M" part of RMW is executed in shared memory.
> Only the R and W parts have any restrictions on where to be executed.
>
>    

If the guest continues to run during the RMW, you will get inconsistent 
results.

This may be prevented by qemu_mutex, but I'd rather not rely on it.

Also, I'd like remote operations to be visible.

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

[Qemu-devel] Re: [PATCH 4/7] qemu_flush_work for remote vcpu execution

[Paolo Bonzini]

On 11/26/2009 06:24 PM, Glauber Costa wrote:
> +CPUState *qemu_get_current_env(void);

Useless forward reference (to a function that doesn't exist in that file 
at all).

Paolo

Re: [Qemu-devel] [PATCH 2/7] store thread-specific env information

[Avi Kivity]

On 11/26/2009 07:24 PM, Glauber Costa wrote:
> Since we'll have multiple cpu threads, at least for kvm, we need a way to store
> and retrieve the CPUState associated with the current execution thread.
> For the I/O thread, this will be NULL.
>
> I am using pthread functions for that, for portability, but we could as well
> use __thread keyword.
>
>    

Where is __thread not supported?

It's likely a bit faster than pthread_getspecific().

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

Re: [Qemu-devel] [PATCH 2/7] store thread-specific env information

[Andreas Färber]

Am 29.11.2009 um 16:29 schrieb Avi Kivity:

> On 11/26/2009 07:24 PM, Glauber Costa wrote:
>> Since we'll have multiple cpu threads, at least for kvm, we need a  
>> way to store
>> and retrieve the CPUState associated with the current execution  
>> thread.
>> For the I/O thread, this will be NULL.
>>
>> I am using pthread functions for that, for portability, but we  
>> could as well
>> use __thread keyword.
>>
>>
>
> Where is __thread not supported?

Apple, Sun.

Andreas

Re: [Qemu-devel] [PATCH 2/7] store thread-specific env information

[Avi Kivity]

On 11/29/2009 05:38 PM, Andreas Färber wrote:
>
> Am 29.11.2009 um 16:29 schrieb Avi Kivity:
>
>> On 11/26/2009 07:24 PM, Glauber Costa wrote:
>>> Since we'll have multiple cpu threads, at least for kvm, we need a 
>>> way to store
>>> and retrieve the CPUState associated with the current execution thread.
>>> For the I/O thread, this will be NULL.
>>>
>>> I am using pthread functions for that, for portability, but we could 
>>> as well
>>> use __thread keyword.
>>>
>>>
>>
>> Where is __thread not supported?
>
> Apple, Sun.

Well, pthread_getspecific is around 130 bytes of code, whereas __thread 
is just on instruction.  Maybe we should support both.

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

Re: [Qemu-devel] [PATCH 2/7] store thread-specific env information

[Andreas Färber]

Am 29.11.2009 um 16:42 schrieb Avi Kivity:

> On 11/29/2009 05:38 PM, Andreas Färber wrote:
>>
>> Am 29.11.2009 um 16:29 schrieb Avi Kivity:
>>
>>> On 11/26/2009 07:24 PM, Glauber Costa wrote:
>>>> Since we'll have multiple cpu threads, at least for kvm, we need  
>>>> a way to store
>>>> and retrieve the CPUState associated with the current execution  
>>>> thread.
>>>> For the I/O thread, this will be NULL.
>>>>
>>>> I am using pthread functions for that, for portability, but we  
>>>> could as well
>>>> use __thread keyword.
>>>>
>>>>
>>>
>>> Where is __thread not supported?
>>
>> Apple, Sun.
>
> Well, pthread_getspecific is around 130 bytes of code, whereas  
> __thread is just on instruction.  Maybe we should support both.

Maybe. Mono does so, they have some autoconf-based check plus a  
configure switch --with-tls={pthread|__thread} to override.

Andreas

Re: [Qemu-devel] [PATCH 2/7] store thread-specific env information

[Jamie Lokier]

> On 11/29/2009 05:38 PM, Andreas Färber wrote:
>> Am 29.11.2009 um 16:29 schrieb Avi Kivity:
>>> Where is __thread not supported?
>> Apple, Sun.

Some flavours of uClinux :-)

Avi Kivity wrote:
> Well, pthread_getspecific is around 130 bytes of code, whereas __thread 
> is just on instruction.  Maybe we should support both.

It's easy enough, they are quite similar.  Except that
pthread_key_create lets you provide a destructor which is called as
each thread is destroyed (unfortunately no constructor for new
threads; and you can use both methods if you need a destructor and
speed together).

It's not always one instruction - it's more complicated in shared
libraries, but it's always close to that.

Anyway, I decided to measure them both as I wondered about this for
another program.

On my 2.0GHz Core Duo (32-bit), tight unrolled loop, everything in cache:

     Read void *__thread variable        ~ 0.6 ns
     Call pthread_getspecific(key)       ~ 8.8 ns

__thread is preferable but it's not much overhead to call pthread_getspecific().

Imho, it's not worth making code less portable or more complicated to
handle both, but it's a nice touch.

However, I did notice that the compiler optimises away references to
__thread variables much better, such as hoisting from inside loops.

In my programs I have taken to wrapping everything inside a
thread_specific(var) macro, similar to the one in the kernel, which
expands to call pthread_getspecific() or use __thread[*], That keeps the
complexity in one place, which is where the macro is defined.

( [*] - Windows has __thread, but it sometimes crashes when used in a
DLL, so I use the Windows equivalent of pthread_getspecific() in the
same wrapper macro, which is fine. )

-- Jamie

[Qemu-devel] Re: [PATCH 2/7] store thread-specific env information

[Paolo Bonzini]

On 11/29/2009 04:38 PM, Andreas Färber wrote:
>
> Am 29.11.2009 um 16:29 schrieb Avi Kivity:
>
>> On 11/26/2009 07:24 PM, Glauber Costa wrote:
>>> Since we'll have multiple cpu threads, at least for kvm, we need a
>>> way to store
>>> and retrieve the CPUState associated with the current execution thread.
>>> For the I/O thread, this will be NULL.
>>>
>>> I am using pthread functions for that, for portability, but we could
>>> as well
>>> use __thread keyword.
>>>
>>>
>>
>> Where is __thread not supported?
>
> Apple, Sun.

Not sure about Sun.

Anyway on Windows neither __thread nor pthread_getspecific is supported, 
so some configury is needed anyway.

Paolo

Re: [Qemu-devel] Re: [PATCH 2/7] store thread-specific env information

[Glauber Costa]

> Anyway on Windows neither __thread nor pthread_getspecific is supported, so
> some configury is needed anyway.
>
> Paolo

For the record, I am a big fan of __thread. The only reason I used the
pthread library was
portability. I can surely put in some configure knobs to use __thread
where available

-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [Qemu-devel] Re: [PATCH 2/7] store thread-specific env information

[Paolo Bonzini]

On 11/30/2009 12:41 PM, Glauber Costa wrote:
> For the record, I am a big fan of __thread. The only reason I used
> the pthread library was portability. I can surely put in some
> configure knobs to use __thread where available

Plus, do you really need to support SMP when __thread is not available?...

Paolo

Re: [Qemu-devel] Re: [PATCH 2/7] store thread-specific env information

[Avi Kivity]

On 11/30/2009 01:49 PM, Paolo Bonzini wrote:
> On 11/30/2009 12:41 PM, Glauber Costa wrote:
>> For the record, I am a big fan of __thread. The only reason I used
>> the pthread library was portability. I can surely put in some
>> configure knobs to use __thread where available
>
> Plus, do you really need to support SMP when __thread is not 
> available?...

Good point.

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

[Qemu-devel] Re: [PATCH 0/7] KVM SMP support, early version

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 597 bytes --]

Glauber Costa wrote:
> Hi guys,
> 
> This is an early version of smp support in kvm that kinda works.
> It has some known problems that I am still tracking. For example,
> it does not reset very well. Also, initialization is a bit slow,
> probably because of the number of remote ioctl calls involved.
> But I believe Jan's patch to do a single ioctl can make it better.

That approach has been buried, we decided to continue extending via new
ioctls.

But why do you need that many remote calls during init? Aren't the
initial ioctls issued by the vcpu threads themselves?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] Re: [PATCH 0/7] KVM SMP support, early version

[Glauber Costa]

On Sun, Nov 29, 2009 at 3:32 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
> Glauber Costa wrote:
>> Hi guys,
>>
>> This is an early version of smp support in kvm that kinda works.
>> It has some known problems that I am still tracking. For example,
>> it does not reset very well. Also, initialization is a bit slow,
>> probably because of the number of remote ioctl calls involved.
>> But I believe Jan's patch to do a single ioctl can make it better.
>
> That approach has been buried, we decided to continue extending via new
> ioctls.
>
> But why do you need that many remote calls during init? Aren't the
> initial ioctls issued by the vcpu threads themselves?

Yes, but system_reset is called from the io-thread, and it fires a lot
of ioctls.

However, I have an already working version that does a much better job
than that.
Will post today with details



-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [Qemu-devel] Re: [PATCH 0/7] KVM SMP support, early version

[Glauber Costa]

>> But why do you need that many remote calls during init? Aren't the
>> initial ioctls issued by the vcpu threads themselves?
>
> Yes, but system_reset is called from the io-thread, and it fires a lot
> of ioctls.
>
> However, I have an already working version that does a much better job
> than that.
> Will post today with details
>
>

Btw, The approach I've taken (code to follow), was to register reset
functions that
issues ioctls in a separate handler, that later calls all of them
already in the vcpu-thread.
So, no code movement, and thus, no races.

reset code is responsible for most remote calls in qemu. One of the
only ones we still
have left is the gdb stuff. Do you have any suggestion to do that
without the current
on_vcpu mechanism?


-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [Qemu-devel] Re: [PATCH 0/7] KVM SMP support, early version

[Avi Kivity]

On 11/30/2009 05:55 PM, Glauber Costa wrote:
>
> reset code is responsible for most remote calls in qemu. One of the
> only ones we still
> have left is the gdb stuff. Do you have any suggestion to do that
> without the current
> on_vcpu mechanism?
>    

No.  But what's wrong with on_vcpu?

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

Re: [Qemu-devel] Re: [PATCH 0/7] KVM SMP support, early version

[Glauber Costa]

On Mon, Nov 30, 2009 at 2:40 PM, Avi Kivity <avi@redhat.com> wrote:
> On 11/30/2009 05:55 PM, Glauber Costa wrote:
>>
>> reset code is responsible for most remote calls in qemu. One of the
>> only ones we still
>> have left is the gdb stuff. Do you have any suggestion to do that
>> without the current
>> on_vcpu mechanism?
>>
>
> No.  But what's wrong with on_vcpu?

intrinsically racy. signal passing slow down things, etc.

That said, as I've stated many times: I don't believe there's anything
fundamentally wrong with on_vcpu. But we might get benefits from a re-design
of things to avoid it whenever possible. (just like the vcpu_reset()
I've just posted)


-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [Qemu-devel] Re: [PATCH 0/7] KVM SMP support, early version

[Jan Kiszka]

Glauber Costa wrote:
> On Mon, Nov 30, 2009 at 2:40 PM, Avi Kivity <avi@redhat.com> wrote:
>> On 11/30/2009 05:55 PM, Glauber Costa wrote:
>>> reset code is responsible for most remote calls in qemu. One of the
>>> only ones we still
>>> have left is the gdb stuff. Do you have any suggestion to do that
>>> without the current
>>> on_vcpu mechanism?
>>>
>> No.  But what's wrong with on_vcpu?
> 
> intrinsically racy. signal passing slow down things, etc.
> 
> That said, as I've stated many times: I don't believe there's anything
> fundamentally wrong with on_vcpu. But we might get benefits from a re-design
> of things to avoid it whenever possible. (just like the vcpu_reset()
> I've just posted)
> 

If you don't want immediate execution of update_guest_debug, save the
state that shall be transferred, set some flag, and run the transfer
before guest entry inside the vcpu threads (after putting the registers
as older kernels may otherwise overwrite the flags register). Should
work, may even avoid redundant calls during a gdb session.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [PATCH 0/7] KVM SMP support, early version

[Avi Kivity]

On 11/30/2009 07:30 PM, Jan Kiszka wrote:
>>>
>>> No.  But what's wrong with on_vcpu?
>>>        
>> intrinsically racy. signal passing slow down things, etc.
>>
>> That said, as I've stated many times: I don't believe there's anything
>> fundamentally wrong with on_vcpu. But we might get benefits from a re-design
>> of things to avoid it whenever possible. (just like the vcpu_reset()
>> I've just posted)
>>
>>      
> If you don't want immediate execution of update_guest_debug, save the
> state that shall be transferred, set some flag, and run the transfer
> before guest entry inside the vcpu threads (after putting the registers
> as older kernels may otherwise overwrite the flags register). Should
> work, may even avoid redundant calls during a gdb session.
>    

There's no guarantee the vcpu will ever exit to qemu, so you have to 
signal the vcpu thread anyway.  When you do that, you might as well load 
the new state.

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

Re: [Qemu-devel] Re: [PATCH 0/7] KVM SMP support, early version

[Jan Kiszka]

Avi Kivity wrote:
> On 11/30/2009 07:30 PM, Jan Kiszka wrote:
>>>> No.  But what's wrong with on_vcpu?
>>>>        
>>> intrinsically racy. signal passing slow down things, etc.
>>>
>>> That said, as I've stated many times: I don't believe there's anything
>>> fundamentally wrong with on_vcpu. But we might get benefits from a re-design
>>> of things to avoid it whenever possible. (just like the vcpu_reset()
>>> I've just posted)
>>>
>>>      
>> If you don't want immediate execution of update_guest_debug, save the
>> state that shall be transferred, set some flag, and run the transfer
>> before guest entry inside the vcpu threads (after putting the registers
>> as older kernels may otherwise overwrite the flags register). Should
>> work, may even avoid redundant calls during a gdb session.
>>    
> 
> There's no guarantee the vcpu will ever exit to qemu, so you have to 
> signal the vcpu thread anyway.  When you do that, you might as well load 
> the new state.

Debugging is special here as it involves vmstop before you start playing
with the debug settings. But I may also oversee some corner case right now.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [PATCH 0/7] KVM SMP support, early version

[Glauber Costa]

On Tue, Dec 1, 2009 at 10:17 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> Avi Kivity wrote:
>> On 11/30/2009 07:30 PM, Jan Kiszka wrote:
>>>>> No.  But what's wrong with on_vcpu?
>>>>>
>>>> intrinsically racy. signal passing slow down things, etc.
>>>>
>>>> That said, as I've stated many times: I don't believe there's anything
>>>> fundamentally wrong with on_vcpu. But we might get benefits from a re-design
>>>> of things to avoid it whenever possible. (just like the vcpu_reset()
>>>> I've just posted)
>>>>
>>>>
>>> If you don't want immediate execution of update_guest_debug, save the
>>> state that shall be transferred, set some flag, and run the transfer
>>> before guest entry inside the vcpu threads (after putting the registers
>>> as older kernels may otherwise overwrite the flags register). Should
>>> work, may even avoid redundant calls during a gdb session.
>>>
>>
>> There's no guarantee the vcpu will ever exit to qemu, so you have to
>> signal the vcpu thread anyway.  When you do that, you might as well load
>> the new state.
>
> Debugging is special here as it involves vmstop before you start playing
> with the debug settings. But I may also oversee some corner case right now.
>

I imagined so. In this case, it might be better to set a flag before
vmstop, and then
honor it on vcpu entry. I had this feeling that in most cases where we
current signal the
vcpu, the VM will be already stopped anyway, so we don't need to resignal.

-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [Qemu-devel] Re: [PATCH 0/7] KVM SMP support, early version

[Jan Kiszka]

Glauber Costa wrote:
> On Tue, Dec 1, 2009 at 10:17 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> Avi Kivity wrote:
>>> On 11/30/2009 07:30 PM, Jan Kiszka wrote:
>>>>>> No.  But what's wrong with on_vcpu?
>>>>>>
>>>>> intrinsically racy. signal passing slow down things, etc.
>>>>>
>>>>> That said, as I've stated many times: I don't believe there's anything
>>>>> fundamentally wrong with on_vcpu. But we might get benefits from a re-design
>>>>> of things to avoid it whenever possible. (just like the vcpu_reset()
>>>>> I've just posted)
>>>>>
>>>>>
>>>> If you don't want immediate execution of update_guest_debug, save the
>>>> state that shall be transferred, set some flag, and run the transfer
>>>> before guest entry inside the vcpu threads (after putting the registers
>>>> as older kernels may otherwise overwrite the flags register). Should
>>>> work, may even avoid redundant calls during a gdb session.
>>>>
>>> There's no guarantee the vcpu will ever exit to qemu, so you have to
>>> signal the vcpu thread anyway.  When you do that, you might as well load
>>> the new state.
>> Debugging is special here as it involves vmstop before you start playing
>> with the debug settings. But I may also oversee some corner case right now.
>>
> 
> I imagined so. In this case, it might be better to set a flag before
> vmstop, and then
> honor it on vcpu entry. I had this feeling that in most cases where we
> current signal the
> vcpu, the VM will be already stopped anyway, so we don't need to resignal.
> 

Do not set the flag before the machine stop, otherwise you risk to race
with vcpus that exit and re-enter briefly before that. This manipulation
should really be done under vmstop protection.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [PATCH 0/7] KVM SMP support, early version

[Avi Kivity]

On Mon, Nov 30, 2009 at 2:40 PM, Avi Kivity <avi@redhat.com> wrote:
> On 11/30/2009 05:55 PM, Glauber Costa wrote:
>>
>> reset code is responsible for most remote calls in qemu. One of the
>> only ones we still
>> have left is the gdb stuff. Do you have any suggestion to do that
>> without the current
>> on_vcpu mechanism?
>>
>
> No.  But what's wrong with on_vcpu?

intrinsically racy. signal passing slow down things, etc.

That said, as I've stated many times: I don't believe there's anything
fundamentally wrong with on_vcpu. But we might get benefits from a re-design
of things to avoid it whenever possible. (just like the vcpu_reset()
I've just posted)



-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [Qemu-devel] [PATCH 0/7] KVM SMP support, early version

[Ed Swierk]

On Thu, Nov 26, 2009 at 9:24 AM, Glauber Costa <glommer@redhat.com> wrote:
> This is an early version of smp support in kvm that kinda works.

Can you please elaborate on "smp support"? The KVM FAQ makes some
rather broad claims about smp support already:

  Does KVM support SMP hosts?
  Yes.

  Does KVM support SMP guests?
  Yes. Up to 16 CPUs can be specified using the -smp option.

Thanks,
--Ed

Re: [Qemu-devel] [PATCH 0/7] KVM SMP support, early version

[Ed Swierk]

On Mon, Nov 30, 2009 at 4:31 PM, Ed Swierk <eswierk@aristanetworks.com> wrote:
> On Thu, Nov 26, 2009 at 9:24 AM, Glauber Costa <glommer@redhat.com> wrote:
>> This is an early version of smp support in kvm that kinda works.
>
> Can you please elaborate on "smp support"? The KVM FAQ makes some
> rather broad claims about smp support already:

I see now that you're integrating smp support into qemu proper from
the kvm fork of qemu. Sorry for the noise.

--Ed

Re: [Qemu-devel] [PATCH 0/7] KVM SMP support, early version

[Alexander Graf]

Glauber Costa wrote:
> Hi guys,
>
> This is an early version of smp support in kvm that kinda works.
> It has some known problems that I am still tracking. For example,
> it does not reset very well. Also, initialization is a bit slow,
> probably because of the number of remote ioctl calls involved.
> But I believe Jan's patch to do a single ioctl can make it better.
>
> Normal operation seem to be going smooth for me. I can boot the
> machine and get it going decently. I'll keep working on the issues,
> but comments are welcome in the mean time.
>   

Can you shut down with that series? I'm trying to get SMP working with
S390X and it looks pretty good, apart from the VCPU threads having
cpu_single_env == NULL on SIGUSR1, which makes them not get out of their
guest context.

Alex