[Qemu-devel] [PATCH 00/15] [uq/master] Patch queue, part III

[Qemu-devel] [PATCH 00/15] [uq/master] Patch queue, part III

[Jan Kiszka]

This round is kind of an assorted collection:
 - make kvm_cpu_exec ready for qemu-kvm reuse
 - reworked vm_stop handling (as preparation for kvm_cpu_exec reuse)
 - the return of kvmclock
 - cleanups around KVM's dirty logging/slot management
 - style cleanups of cpus.c

The patches related to the first item allowed me to remove another 400
lines of duplicate qemu-kvm code. Current local stats:

# wc -l qemu-kvm.[ch] qemu-kvm-x86.c
 1142 qemu-kvm.c
  309 qemu-kvm.h
  230 qemu-kvm-x86.c
 1681 total (vs. 3513 in current master)

I'm about to look into the last issue soon: consolidation of the
threading code.

For upstream qemu, there is now only the MCE rework pending in my queue.
That will be sent as part IV after this series has passed the review.

CC: Alexander Graf <agraf@suse.de>
CC: Alex Williamson <alex.williamson@redhat.com>
CC: Anthony PERARD <anthony.perard@citrix.com>
CC: Glauber Costa <glommer@redhat.com>

Anthony PERARD (1):
  Introduce log_start/log_stop in CPUPhysMemoryClient

Jan Kiszka (14):
  Refactor kvm&tcg function names in cpus.c
  Refactor cpu_has_work/any_cpu_has_work in cpus.c
  Fix a few coding style violations in cpus.c
  Improve vm_stop reason declarations
  Refactor debug and vmstop request interface
  Move debug exception handling out of cpu_exec
  kvm: Separate TCG from KVM cpu execution
  kvm: x86: Prepare VCPU loop for in-kernel irqchip
  kvm: Drop return values from kvm_arch_pre/post_run
  kvm: x86: Catch and report failing IRQ and NMI injections
  kvm: Remove unneeded memory slot reservation
  cirrus: Remove obsolete kvm.h include
  kvm: Make kvm_state globally available
  kvm: x86: Introduce kvmclock device to save/restore its state

 Makefile.target    |    4 +-
 cpu-all.h          |    6 ++
 cpu-common.h       |    4 +
 cpu-exec.c         |   43 ++-----------
 cpus.c             |  182 +++++++++++++++++++++++++++++++---------------------
 cpus.h             |    2 -
 exec.c             |   30 +++++++++
 gdbstub.c          |   19 +++---
 hw/cirrus_vga.c    |    1 -
 hw/ide/core.c      |    2 +-
 hw/kvmclock.c      |  125 +++++++++++++++++++++++++++++++++++
 hw/kvmclock.h      |   14 ++++
 hw/pc_piix.c       |   31 +++++++--
 hw/scsi-disk.c     |    2 +-
 hw/vga.c           |   31 +++++----
 hw/vhost.c         |    2 +
 hw/virtio-blk.c    |    2 +-
 hw/watchdog.c      |    2 +-
 kvm-all.c          |   35 +++++-----
 kvm-stub.c         |   10 ---
 kvm.h              |    9 +--
 migration.c        |    2 +-
 monitor.c          |    4 +-
 savevm.c           |    4 +-
 sysemu.h           |   10 +++
 target-i386/kvm.c  |   93 +++++++++++++++-----------
 target-ppc/kvm.c   |    6 +-
 target-s390x/kvm.c |    6 +-
 vl.c               |   22 +++++--
 29 files changed, 462 insertions(+), 241 deletions(-)
 create mode 100644 hw/kvmclock.c
 create mode 100644 hw/kvmclock.h

[Qemu-devel] [PATCH 01/15] Refactor kvm&tcg function names in cpus.c

[Jan Kiszka]

Pure interface cosmetics: Ensure that only kvm core services (as
declared in kvm.h) start with "kvm_". Prepend "qemu_" to those that
violate this rule in cpus.c. Also rename the corresponding tcg functions
for the sake of consistency.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 cpus.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/cpus.c b/cpus.c
index 6a85dc8..d54ec7d 100644
--- a/cpus.c
+++ b/cpus.c
@@ -774,7 +774,7 @@ static void qemu_kvm_wait_io_event(CPUState *env)
 
 static int qemu_cpu_exec(CPUState *env);
 
-static void *kvm_cpu_thread_fn(void *arg)
+static void *qemu_kvm_cpu_thread_fn(void *arg)
 {
     CPUState *env = arg;
     int r;
@@ -807,7 +807,7 @@ static void *kvm_cpu_thread_fn(void *arg)
     return NULL;
 }
 
-static void *tcg_cpu_thread_fn(void *arg)
+static void *qemu_tcg_cpu_thread_fn(void *arg)
 {
     CPUState *env = arg;
 
@@ -926,7 +926,7 @@ void resume_all_vcpus(void)
     }
 }
 
-static void tcg_init_vcpu(void *_env)
+static void qemu_tcg_init_vcpu(void *_env)
 {
     CPUState *env = _env;
     /* share a single thread for all cpus with TCG */
@@ -934,7 +934,7 @@ static void tcg_init_vcpu(void *_env)
         env->thread = qemu_mallocz(sizeof(QemuThread));
         env->halt_cond = qemu_mallocz(sizeof(QemuCond));
         qemu_cond_init(env->halt_cond);
-        qemu_thread_create(env->thread, tcg_cpu_thread_fn, env);
+        qemu_thread_create(env->thread, qemu_tcg_cpu_thread_fn, env);
         while (env->created == 0)
             qemu_cond_timedwait(&qemu_cpu_cond, &qemu_global_mutex, 100);
         tcg_cpu_thread = env->thread;
@@ -945,12 +945,12 @@ static void tcg_init_vcpu(void *_env)
     }
 }
 
-static void kvm_start_vcpu(CPUState *env)
+static void qemu_kvm_start_vcpu(CPUState *env)
 {
     env->thread = qemu_mallocz(sizeof(QemuThread));
     env->halt_cond = qemu_mallocz(sizeof(QemuCond));
     qemu_cond_init(env->halt_cond);
-    qemu_thread_create(env->thread, kvm_cpu_thread_fn, env);
+    qemu_thread_create(env->thread, qemu_kvm_cpu_thread_fn, env);
     while (env->created == 0)
         qemu_cond_timedwait(&qemu_cpu_cond, &qemu_global_mutex, 100);
 }
@@ -962,9 +962,9 @@ void qemu_init_vcpu(void *_env)
     env->nr_cores = smp_cores;
     env->nr_threads = smp_threads;
     if (kvm_enabled())
-        kvm_start_vcpu(env);
+        qemu_kvm_start_vcpu(env);
     else
-        tcg_init_vcpu(env);
+        qemu_tcg_init_vcpu(env);
 }
 
 void qemu_notify_event(void)
-- 
1.7.1

[Qemu-devel] [PATCH 02/15] Refactor cpu_has_work/any_cpu_has_work in cpus.c

[Jan Kiszka]

Avoid duplicate use of the function name cpu_has_work, it's confusing.
Refactor cpu_has_work to cpu_is_idle and do the same with
any_cpu_has_work.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 cpus.c |   43 +++++++++++++++++++++++--------------------
 1 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/cpus.c b/cpus.c
index d54ec7d..cd764f2 100644
--- a/cpus.c
+++ b/cpus.c
@@ -137,29 +137,30 @@ static int cpu_can_run(CPUState *env)
     return 1;
 }
 
-static int cpu_has_work(CPUState *env)
+static bool cpu_is_idle(CPUState *env)
 {
-    if (env->stop)
-        return 1;
-    if (env->queued_work_first)
-        return 1;
-    if (env->stopped || !vm_running)
-        return 0;
-    if (!env->halted)
-        return 1;
-    if (qemu_cpu_has_work(env))
-        return 1;
-    return 0;
+    if (env->stop || env->queued_work_first) {
+        return false;
+    }
+    if (env->stopped || !vm_running) {
+        return true;
+    }
+    if (!env->halted || qemu_cpu_has_work(env)) {
+        return false;
+    }
+    return true;
 }
 
-static int any_cpu_has_work(void)
+static bool all_cpus_idle(void)
 {
     CPUState *env;
 
-    for (env = first_cpu; env != NULL; env = env->next_cpu)
-        if (cpu_has_work(env))
-            return 1;
-    return 0;
+    for (env = first_cpu; env != NULL; env = env->next_cpu) {
+        if (!cpu_is_idle(env)) {
+            return false;
+        }
+    }
+    return true;
 }
 
 static void cpu_debug_handler(CPUState *env)
@@ -743,8 +744,9 @@ static void qemu_tcg_wait_io_event(void)
 {
     CPUState *env;
 
-    while (!any_cpu_has_work())
+    while (all_cpus_idle()) {
         qemu_cond_timedwait(tcg_halt_cond, &qemu_global_mutex, 1000);
+    }
 
     qemu_mutex_unlock(&qemu_global_mutex);
 
@@ -765,8 +767,9 @@ static void qemu_tcg_wait_io_event(void)
 
 static void qemu_kvm_wait_io_event(CPUState *env)
 {
-    while (!cpu_has_work(env))
+    while (cpu_is_idle(env)) {
         qemu_cond_timedwait(env->halt_cond, &qemu_global_mutex, 1000);
+    }
 
     qemu_kvm_eat_signals(env);
     qemu_wait_io_event_common(env);
@@ -1070,7 +1073,7 @@ bool cpu_exec_all(void)
         }
     }
     exit_request = 0;
-    return any_cpu_has_work();
+    return !all_cpus_idle();
 }
 
 void set_numa_modes(void)
-- 
1.7.1

[Qemu-devel] Re: [PATCH 02/15] Refactor cpu_has_work/any_cpu_has_work in cpus.c

[Marcelo Tosatti]

On Mon, Feb 07, 2011 at 12:19:13PM +0100, Jan Kiszka wrote:
> Avoid duplicate use of the function name cpu_has_work, it's confusing.
> Refactor cpu_has_work to cpu_is_idle and do the same with
> any_cpu_has_work.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  cpus.c |   43 +++++++++++++++++++++++--------------------
>  1 files changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index d54ec7d..cd764f2 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -137,29 +137,30 @@ static int cpu_can_run(CPUState *env)
>      return 1;
>  }
>  
> -static int cpu_has_work(CPUState *env)
> +static bool cpu_is_idle(CPUState *env)
>  {
> -    if (env->stop)
> -        return 1;
> -    if (env->queued_work_first)
> -        return 1;
> -    if (env->stopped || !vm_running)
> -        return 0;
> -    if (!env->halted)
> -        return 1;
> -    if (qemu_cpu_has_work(env))
> -        return 1;
> -    return 0;
> +    if (env->stop || env->queued_work_first) {
> +        return false;
> +    }
> +    if (env->stopped || !vm_running) {
> +        return true;
> +    }
> +    if (!env->halted || qemu_cpu_has_work(env)) {
> +        return false;
> +    }
> +    return true;
>  }

Do you really find it easier to read evaluations grouped with || ? I
don't.

Sorry but the name change does not feel right either: CPU is still idle
if the vm is not running.

[Qemu-devel] Re: [PATCH 02/15] Refactor cpu_has_work/any_cpu_has_work in cpus.c

[Jan Kiszka]

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

On 2011-02-08 19:50, Marcelo Tosatti wrote:
> On Mon, Feb 07, 2011 at 12:19:13PM +0100, Jan Kiszka wrote:
>> Avoid duplicate use of the function name cpu_has_work, it's confusing.
>> Refactor cpu_has_work to cpu_is_idle and do the same with
>> any_cpu_has_work.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  cpus.c |   43 +++++++++++++++++++++++--------------------
>>  1 files changed, 23 insertions(+), 20 deletions(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index d54ec7d..cd764f2 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -137,29 +137,30 @@ static int cpu_can_run(CPUState *env)
>>      return 1;
>>  }
>>  
>> -static int cpu_has_work(CPUState *env)
>> +static bool cpu_is_idle(CPUState *env)
>>  {
>> -    if (env->stop)
>> -        return 1;
>> -    if (env->queued_work_first)
>> -        return 1;
>> -    if (env->stopped || !vm_running)
>> -        return 0;
>> -    if (!env->halted)
>> -        return 1;
>> -    if (qemu_cpu_has_work(env))
>> -        return 1;
>> -    return 0;
>> +    if (env->stop || env->queued_work_first) {
>> +        return false;
>> +    }
>> +    if (env->stopped || !vm_running) {
>> +        return true;
>> +    }
>> +    if (!env->halted || qemu_cpu_has_work(env)) {
>> +        return false;
>> +    }
>> +    return true;
>>  }
> 
> Do you really find it easier to read evaluations grouped with || ? I
> don't.

I do, specifically as the old version was even more confusing in that
important detail "return 0" vs. "return 1". But even the new benefits
from the grouping IMHO.

> 
> Sorry but the name change does not feel right either: CPU is still idle
> if the vm is not running.

But that's exactly what the function returns. Or is it confusing if we
are talking about the vcpu or the whole thread here? What about
"cpu_thread_is_idle" then?

Jan


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

[Qemu-devel] Re: [PATCH 02/15] Refactor cpu_has_work/any_cpu_has_work in cpus.c

[Marcelo Tosatti]

On Wed, Feb 09, 2011 at 09:07:50AM +0100, Jan Kiszka wrote:
> > Do you really find it easier to read evaluations grouped with || ? I
> > don't.
> 
> I do, specifically as the old version was even more confusing in that
> important detail "return 0" vs. "return 1". But even the new benefits
> from the grouping IMHO.

Well alright.

> > Sorry but the name change does not feel right either: CPU is still idle
> > if the vm is not running.
> 
> But that's exactly what the function returns. Or is it confusing if we
> are talking about the vcpu or the whole thread here? What about
> "cpu_thread_is_idle" then?

Yes thats better.

[Qemu-devel] [PATCH v2 02/15] Refactor cpu_has_work/any_cpu_has_work in cpus.c

[Jan Kiszka]

Avoid duplicate use of the function name cpu_has_work, it's confusing,
also their scope. Refactor cpu_has_work to cpu_thread_is_idle and do the
same with any_cpu_has_work.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Changes in v2:
 - renaming: cpu_thread_is_idle and all_cpu_threads_idle

 cpus.c |   43 +++++++++++++++++++++++--------------------
 1 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/cpus.c b/cpus.c
index d54ec7d..e963208 100644
--- a/cpus.c
+++ b/cpus.c
@@ -137,29 +137,30 @@ static int cpu_can_run(CPUState *env)
     return 1;
 }
 
-static int cpu_has_work(CPUState *env)
+static bool cpu_thread_is_idle(CPUState *env)
 {
-    if (env->stop)
-        return 1;
-    if (env->queued_work_first)
-        return 1;
-    if (env->stopped || !vm_running)
-        return 0;
-    if (!env->halted)
-        return 1;
-    if (qemu_cpu_has_work(env))
-        return 1;
-    return 0;
+    if (env->stop || env->queued_work_first) {
+        return false;
+    }
+    if (env->stopped || !vm_running) {
+        return true;
+    }
+    if (!env->halted || qemu_cpu_has_work(env)) {
+        return false;
+    }
+    return true;
 }
 
-static int any_cpu_has_work(void)
+static bool all_cpu_threads_idle(void)
 {
     CPUState *env;
 
-    for (env = first_cpu; env != NULL; env = env->next_cpu)
-        if (cpu_has_work(env))
-            return 1;
-    return 0;
+    for (env = first_cpu; env != NULL; env = env->next_cpu) {
+        if (!cpu_thread_is_idle(env)) {
+            return false;
+        }
+    }
+    return true;
 }
 
 static void cpu_debug_handler(CPUState *env)
@@ -743,8 +744,9 @@ static void qemu_tcg_wait_io_event(void)
 {
     CPUState *env;
 
-    while (!any_cpu_has_work())
+    while (all_cpu_threads_idle()) {
         qemu_cond_timedwait(tcg_halt_cond, &qemu_global_mutex, 1000);
+    }
 
     qemu_mutex_unlock(&qemu_global_mutex);
 
@@ -765,8 +767,9 @@ static void qemu_tcg_wait_io_event(void)
 
 static void qemu_kvm_wait_io_event(CPUState *env)
 {
-    while (!cpu_has_work(env))
+    while (cpu_thread_is_idle(env)) {
         qemu_cond_timedwait(env->halt_cond, &qemu_global_mutex, 1000);
+    }
 
     qemu_kvm_eat_signals(env);
     qemu_wait_io_event_common(env);
@@ -1070,7 +1073,7 @@ bool cpu_exec_all(void)
         }
     }
     exit_request = 0;
-    return any_cpu_has_work();
+    return !all_cpu_threads_idle();
 }
 
 void set_numa_modes(void)
-- 
1.7.1

[Qemu-devel] [PATCH 03/15] Fix a few coding style violations in cpus.c

[Jan Kiszka]

No functional changes.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 cpus.c |   71 +++++++++++++++++++++++++++++++++++++++------------------------
 1 files changed, 44 insertions(+), 27 deletions(-)

diff --git a/cpus.c b/cpus.c
index cd764f2..e93d8b9 100644
--- a/cpus.c
+++ b/cpus.c
@@ -130,10 +130,12 @@ static void do_vm_stop(int reason)
 
 static int cpu_can_run(CPUState *env)
 {
-    if (env->stop)
+    if (env->stop) {
         return 0;
-    if (env->stopped || !vm_running)
+    }
+    if (env->stopped || !vm_running) {
         return 0;
+    }
     return 1;
 }
 
@@ -225,9 +227,9 @@ static void qemu_event_increment(void)
     static const uint64_t val = 1;
     ssize_t ret;
 
-    if (io_thread_fd == -1)
+    if (io_thread_fd == -1) {
         return;
-
+    }
     do {
         ret = write(io_thread_fd, &val, sizeof(val));
     } while (ret < 0 && errno == EINTR);
@@ -258,17 +260,17 @@ static int qemu_event_init(void)
     int fds[2];
 
     err = qemu_eventfd(fds);
-    if (err == -1)
+    if (err == -1) {
         return -errno;
-
+    }
     err = fcntl_setfl(fds[0], O_NONBLOCK);
-    if (err < 0)
+    if (err < 0) {
         goto fail;
-
+    }
     err = fcntl_setfl(fds[1], O_NONBLOCK);
-    if (err < 0)
+    if (err < 0) {
         goto fail;
-
+    }
     qemu_set_fd_handler2(fds[0], NULL, qemu_event_read, NULL,
                          (void *)(unsigned long)fds[0]);
 
@@ -527,7 +529,6 @@ void pause_all_vcpus(void)
 
 void qemu_cpu_kick(void *env)
 {
-    return;
 }
 
 void qemu_cpu_kick_self(void)
@@ -660,13 +661,15 @@ int qemu_init_main_loop(void)
     blocked_signals = block_io_signals();
 
     ret = qemu_signalfd_init(blocked_signals);
-    if (ret)
+    if (ret) {
         return ret;
+    }
 
     /* Note eventfd must be drained before signalfd handlers run */
     ret = qemu_event_init();
-    if (ret)
+    if (ret) {
         return ret;
+    }
 
     qemu_cond_init(&qemu_pause_cond);
     qemu_cond_init(&qemu_system_cond);
@@ -696,10 +699,11 @@ void run_on_cpu(CPUState *env, void (*func)(void *data), void *data)
 
     wi.func = func;
     wi.data = data;
-    if (!env->queued_work_first)
+    if (!env->queued_work_first) {
         env->queued_work_first = &wi;
-    else
+    } else {
         env->queued_work_last->next = &wi;
+    }
     env->queued_work_last = &wi;
     wi.next = NULL;
     wi.done = false;
@@ -717,8 +721,9 @@ static void flush_queued_work(CPUState *env)
 {
     struct qemu_work_item *wi;
 
-    if (!env->queued_work_first)
+    if (!env->queued_work_first) {
         return;
+    }
 
     while ((wi = env->queued_work_first)) {
         env->queued_work_first = wi->next;
@@ -798,12 +803,14 @@ static void *qemu_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) {
         qemu_cond_timedwait(&qemu_system_cond, &qemu_global_mutex, 100);
+    }
 
     while (1) {
-        if (cpu_can_run(env))
+        if (cpu_can_run(env)) {
             qemu_cpu_exec(env);
+        }
         qemu_kvm_wait_io_event(env);
     }
 
@@ -819,13 +826,15 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
 
     /* signal CPU creation */
     qemu_mutex_lock(&qemu_global_mutex);
-    for (env = first_cpu; env != NULL; env = env->next_cpu)
+    for (env = first_cpu; env != NULL; env = env->next_cpu) {
         env->created = 1;
+    }
     qemu_cond_signal(&qemu_cpu_cond);
 
     /* and wait for machine initialization */
-    while (!qemu_system_ready)
+    while (!qemu_system_ready) {
         qemu_cond_timedwait(&qemu_system_cond, &qemu_global_mutex, 100);
+    }
 
     while (1) {
         cpu_exec_all();
@@ -838,6 +847,7 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
 void qemu_cpu_kick(void *_env)
 {
     CPUState *env = _env;
+
     qemu_cond_broadcast(env->halt_cond);
     if (!env->thread_kicked) {
         qemu_thread_signal(env->thread, SIG_IPI);
@@ -889,8 +899,9 @@ static int all_vcpus_paused(void)
     CPUState *penv = first_cpu;
 
     while (penv) {
-        if (!penv->stopped)
+        if (!penv->stopped) {
             return 0;
+        }
         penv = (CPUState *)penv->next_cpu;
     }
 
@@ -932,14 +943,16 @@ void resume_all_vcpus(void)
 static void qemu_tcg_init_vcpu(void *_env)
 {
     CPUState *env = _env;
+
     /* share a single thread for all cpus with TCG */
     if (!tcg_cpu_thread) {
         env->thread = qemu_mallocz(sizeof(QemuThread));
         env->halt_cond = qemu_mallocz(sizeof(QemuCond));
         qemu_cond_init(env->halt_cond);
         qemu_thread_create(env->thread, qemu_tcg_cpu_thread_fn, env);
-        while (env->created == 0)
+        while (env->created == 0) {
             qemu_cond_timedwait(&qemu_cpu_cond, &qemu_global_mutex, 100);
+        }
         tcg_cpu_thread = env->thread;
         tcg_halt_cond = env->halt_cond;
     } else {
@@ -954,8 +967,9 @@ static void qemu_kvm_start_vcpu(CPUState *env)
     env->halt_cond = qemu_mallocz(sizeof(QemuCond));
     qemu_cond_init(env->halt_cond);
     qemu_thread_create(env->thread, qemu_kvm_cpu_thread_fn, env);
-    while (env->created == 0)
+    while (env->created == 0) {
         qemu_cond_timedwait(&qemu_cpu_cond, &qemu_global_mutex, 100);
+    }
 }
 
 void qemu_init_vcpu(void *_env)
@@ -964,10 +978,11 @@ void qemu_init_vcpu(void *_env)
 
     env->nr_cores = smp_cores;
     env->nr_threads = smp_threads;
-    if (kvm_enabled())
+    if (kvm_enabled()) {
         qemu_kvm_start_vcpu(env);
-    else
+    } else {
         qemu_tcg_init_vcpu(env);
+    }
 }
 
 void qemu_notify_event(void)
@@ -1050,16 +1065,18 @@ bool cpu_exec_all(void)
 {
     int r;
 
-    if (next_cpu == NULL)
+    if (next_cpu == NULL) {
         next_cpu = first_cpu;
+    }
     for (; next_cpu != NULL && !exit_request; next_cpu = next_cpu->next_cpu) {
         CPUState *env = next_cpu;
 
         qemu_clock_enable(vm_clock,
                           (env->singlestep_enabled & SSTEP_NOTIMER) == 0);
 
-        if (qemu_alarm_pending())
+        if (qemu_alarm_pending()) {
             break;
+        }
         if (cpu_can_run(env)) {
             r = qemu_cpu_exec(env);
             if (kvm_enabled()) {
-- 
1.7.1

[Qemu-devel] [PATCH 04/15] Improve vm_stop reason declarations

[Jan Kiszka]

Define and use dedicated constants for vm_stop reasons, they actually
have nothing to do with the EXCP_* defines used so far. At this chance,
specify more detailed reasons so that VM state change handlers can
evaluate them.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 cpus.c          |    4 ++--
 gdbstub.c       |   19 ++++++++++---------
 hw/ide/core.c   |    2 +-
 hw/scsi-disk.c  |    2 +-
 hw/virtio-blk.c |    2 +-
 hw/watchdog.c   |    2 +-
 kvm-all.c       |    2 +-
 migration.c     |    2 +-
 monitor.c       |    4 ++--
 savevm.c        |    4 ++--
 sysemu.h        |    8 ++++++++
 vl.c            |    2 +-
 12 files changed, 31 insertions(+), 22 deletions(-)

diff --git a/cpus.c b/cpus.c
index e93d8b9..e08ec03 100644
--- a/cpus.c
+++ b/cpus.c
@@ -168,8 +168,8 @@ static bool all_cpus_idle(void)
 static void cpu_debug_handler(CPUState *env)
 {
     gdb_set_stop_cpu(env);
-    debug_requested = EXCP_DEBUG;
-    vm_stop(EXCP_DEBUG);
+    debug_requested = VMSTOP_DEBUG;
+    vm_stop(VMSTOP_DEBUG);
 }
 
 #ifdef CONFIG_LINUX
diff --git a/gdbstub.c b/gdbstub.c
index d6556c9..3397566 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2194,14 +2194,14 @@ static void gdb_vm_state_change(void *opaque, int running, int reason)
     const char *type;
     int ret;
 
-    if (running || (reason != EXCP_DEBUG && reason != EXCP_INTERRUPT) ||
-        s->state == RS_INACTIVE || s->state == RS_SYSCALL)
+    if (running || (reason != VMSTOP_DEBUG && reason != VMSTOP_INTERRUPT) ||
+        s->state == RS_INACTIVE || s->state == RS_SYSCALL) {
         return;
-
+    }
     /* disable single step if it was enable */
     cpu_single_step(env, 0);
 
-    if (reason == EXCP_DEBUG) {
+    if (reason == VMSTOP_DEBUG) {
         if (env->watchpoint_hit) {
             switch (env->watchpoint_hit->flags & BP_MEM_ACCESS) {
             case BP_MEM_READ:
@@ -2252,7 +2252,7 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...)
     gdb_current_syscall_cb = cb;
     s->state = RS_SYSCALL;
 #ifndef CONFIG_USER_ONLY
-    vm_stop(EXCP_DEBUG);
+    vm_stop(VMSTOP_DEBUG);
 #endif
     s->state = RS_IDLE;
     va_start(va, fmt);
@@ -2326,7 +2326,7 @@ static void gdb_read_byte(GDBState *s, int ch)
     if (vm_running) {
         /* when the CPU is running, we cannot do anything except stop
            it when receiving a char */
-        vm_stop(EXCP_INTERRUPT);
+        vm_stop(VMSTOP_INTERRUPT);
     } else
 #endif
     {
@@ -2588,7 +2588,7 @@ static void gdb_chr_event(void *opaque, int event)
 {
     switch (event) {
     case CHR_EVENT_OPENED:
-        vm_stop(EXCP_INTERRUPT);
+        vm_stop(VMSTOP_INTERRUPT);
         gdb_has_xml = 0;
         break;
     default:
@@ -2628,8 +2628,9 @@ static int gdb_monitor_write(CharDriverState *chr, const uint8_t *buf, int len)
 #ifndef _WIN32
 static void gdb_sigterm_handler(int signal)
 {
-    if (vm_running)
-        vm_stop(EXCP_INTERRUPT);
+    if (vm_running) {
+        vm_stop(VMSTOP_INTERRUPT);
+    }
 }
 #endif
 
diff --git a/hw/ide/core.c b/hw/ide/core.c
index dd63664..9c91a49 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -465,7 +465,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
         s->bus->dma->ops->set_unit(s->bus->dma, s->unit);
         s->bus->dma->ops->add_status(s->bus->dma, op);
         bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
-        vm_stop(0);
+        vm_stop(VMSTOP_DISKFULL);
     } else {
         if (op & BM_STATUS_DMA_RETRY) {
             dma_buf_commit(s, 0);
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 488eedd..b05e654 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -239,7 +239,7 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
         r->status |= SCSI_REQ_STATUS_RETRY | type;
 
         bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
-        vm_stop(0);
+        vm_stop(VMSTOP_DISKFULL);
     } else {
         if (type == SCSI_REQ_STATUS_RETRY_READ) {
             r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, 0);
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index ffac5a4..b14fb99 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -78,7 +78,7 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
         req->next = s->rq;
         s->rq = req;
         bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
-        vm_stop(0);
+        vm_stop(VMSTOP_DISKFULL);
     } else {
         virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
         bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
diff --git a/hw/watchdog.c b/hw/watchdog.c
index e9dd56e..1c900a1 100644
--- a/hw/watchdog.c
+++ b/hw/watchdog.c
@@ -132,7 +132,7 @@ void watchdog_perform_action(void)
 
     case WDT_PAUSE:             /* same as 'stop' command in monitor */
         watchdog_mon_event("pause");
-        vm_stop(0);
+        vm_stop(VMSTOP_WATCHDOG);
         break;
 
     case WDT_DEBUG:
diff --git a/kvm-all.c b/kvm-all.c
index 42dfed8..19cf188 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -994,7 +994,7 @@ int kvm_cpu_exec(CPUState *env)
 
     if (ret < 0) {
         cpu_dump_state(env, stderr, fprintf, CPU_DUMP_CODE);
-        vm_stop(0);
+        vm_stop(VMSTOP_PANIC);
         env->exit_request = 1;
     }
     if (env->exit_request) {
diff --git a/migration.c b/migration.c
index 3612572..20ea113 100644
--- a/migration.c
+++ b/migration.c
@@ -378,7 +378,7 @@ void migrate_fd_put_ready(void *opaque)
         int old_vm_running = vm_running;
 
         DPRINTF("done iterating\n");
-        vm_stop(0);
+        vm_stop(VMSTOP_RWSTATE);
 
         if ((qemu_savevm_state_complete(s->mon, s->file)) < 0) {
             if (old_vm_running) {
diff --git a/monitor.c b/monitor.c
index 7fc311d..1df3ad5 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1255,7 +1255,7 @@ static void do_singlestep(Monitor *mon, const QDict *qdict)
  */
 static int do_stop(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
-    vm_stop(EXCP_INTERRUPT);
+    vm_stop(VMSTOP_INTERRUPT);
     return 0;
 }
 
@@ -2783,7 +2783,7 @@ static void do_loadvm(Monitor *mon, const QDict *qdict)
     int saved_vm_running  = vm_running;
     const char *name = qdict_get_str(qdict, "name");
 
-    vm_stop(0);
+    vm_stop(VMSTOP_RWSTATE);
 
     if (load_vmstate(name) == 0 && saved_vm_running) {
         vm_start();
diff --git a/savevm.c b/savevm.c
index 4453217..b378d3d 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1575,7 +1575,7 @@ static int qemu_savevm_state(Monitor *mon, QEMUFile *f)
     int ret;
 
     saved_vm_running = vm_running;
-    vm_stop(0);
+    vm_stop(VMSTOP_RWSTATE);
 
     if (qemu_savevm_state_blocked(mon)) {
         ret = -EINVAL;
@@ -1896,7 +1896,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
     }
 
     saved_vm_running = vm_running;
-    vm_stop(0);
+    vm_stop(VMSTOP_RWSTATE);
 
     memset(sn, 0, sizeof(*sn));
 
diff --git a/sysemu.h b/sysemu.h
index 23ae17e..b23e722 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -37,6 +37,14 @@ VMChangeStateEntry *qemu_add_vm_change_state_handler(VMChangeStateHandler *cb,
                                                      void *opaque);
 void qemu_del_vm_change_state_handler(VMChangeStateEntry *e);
 
+#define VMSTOP_INTERRUPT 0
+#define VMSTOP_DEBUG     1
+#define VMSTOP_SHUTDOWN  2
+#define VMSTOP_DISKFULL  3
+#define VMSTOP_WATCHDOG  4
+#define VMSTOP_PANIC     5
+#define VMSTOP_RWSTATE   6
+
 void vm_start(void);
 void vm_stop(int reason);
 
diff --git a/vl.c b/vl.c
index 837be97..9440f98 100644
--- a/vl.c
+++ b/vl.c
@@ -1433,7 +1433,7 @@ static void main_loop(void)
         if (qemu_shutdown_requested()) {
             monitor_protocol_event(QEVENT_SHUTDOWN, NULL);
             if (no_shutdown) {
-                vm_stop(0);
+                vm_stop(VMSTOP_SHUTDOWN);
                 no_shutdown = 0;
             } else
                 break;
-- 
1.7.1

[Qemu-devel] Re: [PATCH 04/15] Improve vm_stop reason declarations

[Marcelo Tosatti]

On Mon, Feb 07, 2011 at 12:19:15PM +0100, Jan Kiszka wrote:
> index d6556c9..3397566 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -2194,14 +2194,14 @@ static void gdb_vm_state_change(void *opaque, int running, int reason)
>      const char *type;
>      int ret;
>  
> -    if (running || (reason != EXCP_DEBUG && reason != EXCP_INTERRUPT) ||
> -        s->state == RS_INACTIVE || s->state == RS_SYSCALL)
> +    if (running || (reason != VMSTOP_DEBUG && reason != VMSTOP_INTERRUPT) ||
> +        s->state == RS_INACTIVE || s->state == RS_SYSCALL) {
>          return;

What about VMSTOP_USER ?

VMSTOP_INTERRUPT -> "the VM is stopped by an interrupt".

VMSTOP_USER -> "the VM is stopped by the user".

> diff --git a/migration.c b/migration.c
> index 3612572..20ea113 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -378,7 +378,7 @@ void migrate_fd_put_ready(void *opaque)
>          int old_vm_running = vm_running;
>  
>          DPRINTF("done iterating\n");
> -        vm_stop(0);
> +        vm_stop(VMSTOP_RWSTATE);

VMSTOP_RWSTATE is cryptic. What about VMSTOP_SAVEVM, MIGRATE, etc.

Nice!

[Qemu-devel] Re: [PATCH 04/15] Improve vm_stop reason declarations

[Jan Kiszka]

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

On 2011-02-08 19:59, Marcelo Tosatti wrote:
> On Mon, Feb 07, 2011 at 12:19:15PM +0100, Jan Kiszka wrote:
>> index d6556c9..3397566 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -2194,14 +2194,14 @@ static void gdb_vm_state_change(void *opaque, int running, int reason)
>>      const char *type;
>>      int ret;
>>  
>> -    if (running || (reason != EXCP_DEBUG && reason != EXCP_INTERRUPT) ||
>> -        s->state == RS_INACTIVE || s->state == RS_SYSCALL)
>> +    if (running || (reason != VMSTOP_DEBUG && reason != VMSTOP_INTERRUPT) ||
>> +        s->state == RS_INACTIVE || s->state == RS_SYSCALL) {
>>          return;
> 
> What about VMSTOP_USER ?
> 
> VMSTOP_INTERRUPT -> "the VM is stopped by an interrupt".
> 
> VMSTOP_USER -> "the VM is stopped by the user".

Makes a lot of sense, will change.

> 
>> diff --git a/migration.c b/migration.c
>> index 3612572..20ea113 100644
>> --- a/migration.c
>> +++ b/migration.c
>> @@ -378,7 +378,7 @@ void migrate_fd_put_ready(void *opaque)
>>          int old_vm_running = vm_running;
>>  
>>          DPRINTF("done iterating\n");
>> -        vm_stop(0);
>> +        vm_stop(VMSTOP_RWSTATE);
> 
> VMSTOP_RWSTATE is cryptic. What about VMSTOP_SAVEVM, MIGRATE, etc.

Both alternatives are not completely accurate as we also stop of vmload.
What about VMSTOP_SYNC_STATE or UPDATE_STATE?

Jan


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

[Qemu-devel] Re: [PATCH 04/15] Improve vm_stop reason declarations

[Marcelo Tosatti]

On Wed, Feb 09, 2011 at 09:07:01AM +0100, Jan Kiszka wrote:
> On 2011-02-08 19:59, Marcelo Tosatti wrote:
> > On Mon, Feb 07, 2011 at 12:19:15PM +0100, Jan Kiszka wrote:
> >> index d6556c9..3397566 100644
> >> --- a/gdbstub.c
> >> +++ b/gdbstub.c
> >> @@ -2194,14 +2194,14 @@ static void gdb_vm_state_change(void *opaque, int running, int reason)
> >>      const char *type;
> >>      int ret;
> >>  
> >> -    if (running || (reason != EXCP_DEBUG && reason != EXCP_INTERRUPT) ||
> >> -        s->state == RS_INACTIVE || s->state == RS_SYSCALL)
> >> +    if (running || (reason != VMSTOP_DEBUG && reason != VMSTOP_INTERRUPT) ||
> >> +        s->state == RS_INACTIVE || s->state == RS_SYSCALL) {
> >>          return;
> > 
> > What about VMSTOP_USER ?
> > 
> > VMSTOP_INTERRUPT -> "the VM is stopped by an interrupt".
> > 
> > VMSTOP_USER -> "the VM is stopped by the user".
> 
> Makes a lot of sense, will change.
> 
> > 
> >> diff --git a/migration.c b/migration.c
> >> index 3612572..20ea113 100644
> >> --- a/migration.c
> >> +++ b/migration.c
> >> @@ -378,7 +378,7 @@ void migrate_fd_put_ready(void *opaque)
> >>          int old_vm_running = vm_running;
> >>  
> >>          DPRINTF("done iterating\n");
> >> -        vm_stop(0);
> >> +        vm_stop(VMSTOP_RWSTATE);
> > 
> > VMSTOP_RWSTATE is cryptic. What about VMSTOP_SAVEVM, MIGRATE, etc.
> 
> Both alternatives are not completely accurate as we also stop of vmload.
> What about VMSTOP_SYNC_STATE or UPDATE_STATE?

IMO it would be better to state the specific reason why the vm stopped,
not some generic action. VMSTOP_SAVEVM, VMSTOP_VMLOAD and
VMSTOP_MIGRATE.

[Qemu-devel] Re: [PATCH 04/15] Improve vm_stop reason declarations

[Jan Kiszka]

On 2011-02-09 15:17, Marcelo Tosatti wrote:
> On Wed, Feb 09, 2011 at 09:07:01AM +0100, Jan Kiszka wrote:
>> On 2011-02-08 19:59, Marcelo Tosatti wrote:
>>> On Mon, Feb 07, 2011 at 12:19:15PM +0100, Jan Kiszka wrote:
>>>> index d6556c9..3397566 100644
>>>> --- a/gdbstub.c
>>>> +++ b/gdbstub.c
>>>> @@ -2194,14 +2194,14 @@ static void gdb_vm_state_change(void *opaque, int running, int reason)
>>>>      const char *type;
>>>>      int ret;
>>>>  
>>>> -    if (running || (reason != EXCP_DEBUG && reason != EXCP_INTERRUPT) ||
>>>> -        s->state == RS_INACTIVE || s->state == RS_SYSCALL)
>>>> +    if (running || (reason != VMSTOP_DEBUG && reason != VMSTOP_INTERRUPT) ||
>>>> +        s->state == RS_INACTIVE || s->state == RS_SYSCALL) {
>>>>          return;
>>>
>>> What about VMSTOP_USER ?
>>>
>>> VMSTOP_INTERRUPT -> "the VM is stopped by an interrupt".
>>>
>>> VMSTOP_USER -> "the VM is stopped by the user".
>>
>> Makes a lot of sense, will change.
>>
>>>
>>>> diff --git a/migration.c b/migration.c
>>>> index 3612572..20ea113 100644
>>>> --- a/migration.c
>>>> +++ b/migration.c
>>>> @@ -378,7 +378,7 @@ void migrate_fd_put_ready(void *opaque)
>>>>          int old_vm_running = vm_running;
>>>>  
>>>>          DPRINTF("done iterating\n");
>>>> -        vm_stop(0);
>>>> +        vm_stop(VMSTOP_RWSTATE);
>>>
>>> VMSTOP_RWSTATE is cryptic. What about VMSTOP_SAVEVM, MIGRATE, etc.
>>
>> Both alternatives are not completely accurate as we also stop of vmload.
>> What about VMSTOP_SYNC_STATE or UPDATE_STATE?
> 
> IMO it would be better to state the specific reason why the vm stopped,
> not some generic action. VMSTOP_SAVEVM, VMSTOP_VMLOAD and
> VMSTOP_MIGRATE.
> 

Ah, OK. Can be done.

Jan

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

[Qemu-devel] [PATCH v2 04/15] Improve vm_stop reason declarations

[Jan Kiszka]

Define and use dedicated constants for vm_stop reasons, they actually
have nothing to do with the EXCP_* defines used so far. At this chance,
specify more detailed reasons so that VM state change handlers can
evaluate them.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Changes in v2:
 - VMSTOP_INTERRUPT -> VMSTOP_USER
 - differentiate between VMSTOP_SAVEVM/LOADVM/MIGRATE

 cpus.c          |    4 ++--
 gdbstub.c       |   19 ++++++++++---------
 hw/ide/core.c   |    2 +-
 hw/scsi-disk.c  |    2 +-
 hw/virtio-blk.c |    2 +-
 hw/watchdog.c   |    2 +-
 kvm-all.c       |    2 +-
 migration.c     |    2 +-
 monitor.c       |    4 ++--
 savevm.c        |    4 ++--
 sysemu.h        |   10 ++++++++++
 vl.c            |    2 +-
 12 files changed, 33 insertions(+), 22 deletions(-)

diff --git a/cpus.c b/cpus.c
index 802d15a..ca1f01d 100644
--- a/cpus.c
+++ b/cpus.c
@@ -168,8 +168,8 @@ static bool all_cpu_threads_idle(void)
 static void cpu_debug_handler(CPUState *env)
 {
     gdb_set_stop_cpu(env);
-    debug_requested = EXCP_DEBUG;
-    vm_stop(EXCP_DEBUG);
+    debug_requested = VMSTOP_DEBUG;
+    vm_stop(VMSTOP_DEBUG);
 }
 
 #ifdef CONFIG_LINUX
diff --git a/gdbstub.c b/gdbstub.c
index d6556c9..ed51a8a 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2194,14 +2194,14 @@ static void gdb_vm_state_change(void *opaque, int running, int reason)
     const char *type;
     int ret;
 
-    if (running || (reason != EXCP_DEBUG && reason != EXCP_INTERRUPT) ||
-        s->state == RS_INACTIVE || s->state == RS_SYSCALL)
+    if (running || (reason != VMSTOP_DEBUG && reason != VMSTOP_USER) ||
+        s->state == RS_INACTIVE || s->state == RS_SYSCALL) {
         return;
-
+    }
     /* disable single step if it was enable */
     cpu_single_step(env, 0);
 
-    if (reason == EXCP_DEBUG) {
+    if (reason == VMSTOP_DEBUG) {
         if (env->watchpoint_hit) {
             switch (env->watchpoint_hit->flags & BP_MEM_ACCESS) {
             case BP_MEM_READ:
@@ -2252,7 +2252,7 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...)
     gdb_current_syscall_cb = cb;
     s->state = RS_SYSCALL;
 #ifndef CONFIG_USER_ONLY
-    vm_stop(EXCP_DEBUG);
+    vm_stop(VMSTOP_DEBUG);
 #endif
     s->state = RS_IDLE;
     va_start(va, fmt);
@@ -2326,7 +2326,7 @@ static void gdb_read_byte(GDBState *s, int ch)
     if (vm_running) {
         /* when the CPU is running, we cannot do anything except stop
            it when receiving a char */
-        vm_stop(EXCP_INTERRUPT);
+        vm_stop(VMSTOP_USER);
     } else
 #endif
     {
@@ -2588,7 +2588,7 @@ static void gdb_chr_event(void *opaque, int event)
 {
     switch (event) {
     case CHR_EVENT_OPENED:
-        vm_stop(EXCP_INTERRUPT);
+        vm_stop(VMSTOP_USER);
         gdb_has_xml = 0;
         break;
     default:
@@ -2628,8 +2628,9 @@ static int gdb_monitor_write(CharDriverState *chr, const uint8_t *buf, int len)
 #ifndef _WIN32
 static void gdb_sigterm_handler(int signal)
 {
-    if (vm_running)
-        vm_stop(EXCP_INTERRUPT);
+    if (vm_running) {
+        vm_stop(VMSTOP_USER);
+    }
 }
 #endif
 
diff --git a/hw/ide/core.c b/hw/ide/core.c
index dd63664..9c91a49 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -465,7 +465,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
         s->bus->dma->ops->set_unit(s->bus->dma, s->unit);
         s->bus->dma->ops->add_status(s->bus->dma, op);
         bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
-        vm_stop(0);
+        vm_stop(VMSTOP_DISKFULL);
     } else {
         if (op & BM_STATUS_DMA_RETRY) {
             dma_buf_commit(s, 0);
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 488eedd..b05e654 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -239,7 +239,7 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
         r->status |= SCSI_REQ_STATUS_RETRY | type;
 
         bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
-        vm_stop(0);
+        vm_stop(VMSTOP_DISKFULL);
     } else {
         if (type == SCSI_REQ_STATUS_RETRY_READ) {
             r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, 0);
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index ffac5a4..b14fb99 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -78,7 +78,7 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
         req->next = s->rq;
         s->rq = req;
         bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
-        vm_stop(0);
+        vm_stop(VMSTOP_DISKFULL);
     } else {
         virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
         bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
diff --git a/hw/watchdog.c b/hw/watchdog.c
index e9dd56e..1c900a1 100644
--- a/hw/watchdog.c
+++ b/hw/watchdog.c
@@ -132,7 +132,7 @@ void watchdog_perform_action(void)
 
     case WDT_PAUSE:             /* same as 'stop' command in monitor */
         watchdog_mon_event("pause");
-        vm_stop(0);
+        vm_stop(VMSTOP_WATCHDOG);
         break;
 
     case WDT_DEBUG:
diff --git a/kvm-all.c b/kvm-all.c
index 42dfed8..19cf188 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -994,7 +994,7 @@ int kvm_cpu_exec(CPUState *env)
 
     if (ret < 0) {
         cpu_dump_state(env, stderr, fprintf, CPU_DUMP_CODE);
-        vm_stop(0);
+        vm_stop(VMSTOP_PANIC);
         env->exit_request = 1;
     }
     if (env->exit_request) {
diff --git a/migration.c b/migration.c
index 3612572..af3a1f2 100644
--- a/migration.c
+++ b/migration.c
@@ -378,7 +378,7 @@ void migrate_fd_put_ready(void *opaque)
         int old_vm_running = vm_running;
 
         DPRINTF("done iterating\n");
-        vm_stop(0);
+        vm_stop(VMSTOP_MIGRATE);
 
         if ((qemu_savevm_state_complete(s->mon, s->file)) < 0) {
             if (old_vm_running) {
diff --git a/monitor.c b/monitor.c
index 7fc311d..22ae3bb 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1255,7 +1255,7 @@ static void do_singlestep(Monitor *mon, const QDict *qdict)
  */
 static int do_stop(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
-    vm_stop(EXCP_INTERRUPT);
+    vm_stop(VMSTOP_USER);
     return 0;
 }
 
@@ -2783,7 +2783,7 @@ static void do_loadvm(Monitor *mon, const QDict *qdict)
     int saved_vm_running  = vm_running;
     const char *name = qdict_get_str(qdict, "name");
 
-    vm_stop(0);
+    vm_stop(VMSTOP_LOADVM);
 
     if (load_vmstate(name) == 0 && saved_vm_running) {
         vm_start();
diff --git a/savevm.c b/savevm.c
index 6d83b0f..a50fd31 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1575,7 +1575,7 @@ static int qemu_savevm_state(Monitor *mon, QEMUFile *f)
     int ret;
 
     saved_vm_running = vm_running;
-    vm_stop(0);
+    vm_stop(VMSTOP_SAVEVM);
 
     if (qemu_savevm_state_blocked(mon)) {
         ret = -EINVAL;
@@ -1904,7 +1904,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
     }
 
     saved_vm_running = vm_running;
-    vm_stop(0);
+    vm_stop(VMSTOP_SAVEVM);
 
     memset(sn, 0, sizeof(*sn));
 
diff --git a/sysemu.h b/sysemu.h
index 23ae17e..0628d3d 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -37,6 +37,16 @@ VMChangeStateEntry *qemu_add_vm_change_state_handler(VMChangeStateHandler *cb,
                                                      void *opaque);
 void qemu_del_vm_change_state_handler(VMChangeStateEntry *e);
 
+#define VMSTOP_USER      0
+#define VMSTOP_DEBUG     1
+#define VMSTOP_SHUTDOWN  2
+#define VMSTOP_DISKFULL  3
+#define VMSTOP_WATCHDOG  4
+#define VMSTOP_PANIC     5
+#define VMSTOP_SAVEVM    6
+#define VMSTOP_LOADVM    7
+#define VMSTOP_MIGRATE   8
+
 void vm_start(void);
 void vm_stop(int reason);
 
diff --git a/vl.c b/vl.c
index c9fa266..6d2d1d3 100644
--- a/vl.c
+++ b/vl.c
@@ -1433,7 +1433,7 @@ static void main_loop(void)
         if (qemu_shutdown_requested()) {
             monitor_protocol_event(QEVENT_SHUTDOWN, NULL);
             if (no_shutdown) {
-                vm_stop(0);
+                vm_stop(VMSTOP_SHUTDOWN);
                 no_shutdown = 0;
             } else
                 break;
-- 
1.7.1

[Qemu-devel] [PATCH 05/15] Refactor debug and vmstop request interface

[Jan Kiszka]

Instead of fiddling with debug_requested and vmstop_requested directly,
introduce qemu_system_debug_request and turn qemu_system_vmstop_request
into a public interface. This aligns those services with exiting ones in
vl.c.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 cpus.c   |    9 +--------
 cpus.h   |    2 --
 sysemu.h |    2 ++
 vl.c     |   20 ++++++++++++++++----
 4 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/cpus.c b/cpus.c
index e08ec03..85c680e 100644
--- a/cpus.c
+++ b/cpus.c
@@ -168,8 +168,7 @@ static bool all_cpus_idle(void)
 static void cpu_debug_handler(CPUState *env)
 {
     gdb_set_stop_cpu(env);
-    debug_requested = VMSTOP_DEBUG;
-    vm_stop(VMSTOP_DEBUG);
+    qemu_system_debug_request();
 }
 
 #ifdef CONFIG_LINUX
@@ -990,12 +989,6 @@ void qemu_notify_event(void)
     qemu_event_increment();
 }
 
-static void qemu_system_vmstop_request(int reason)
-{
-    vmstop_requested = reason;
-    qemu_notify_event();
-}
-
 void cpu_stop_current(void)
 {
     if (cpu_single_env) {
diff --git a/cpus.h b/cpus.h
index 4cadb64..e021126 100644
--- a/cpus.h
+++ b/cpus.h
@@ -11,8 +11,6 @@ void cpu_stop_current(void);
 /* vl.c */
 extern int smp_cores;
 extern int smp_threads;
-extern int debug_requested;
-extern int vmstop_requested;
 void vm_state_notify(int running, int reason);
 bool cpu_exec_all(void);
 void set_numa_modes(void);
diff --git a/sysemu.h b/sysemu.h
index b23e722..b496bde 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -59,6 +59,8 @@ void cpu_disable_ticks(void);
 void qemu_system_reset_request(void);
 void qemu_system_shutdown_request(void);
 void qemu_system_powerdown_request(void);
+void qemu_system_debug_request(void);
+void qemu_system_vmstop_request(int reason);
 int qemu_shutdown_requested(void);
 int qemu_reset_requested(void);
 int qemu_powerdown_requested(void);
diff --git a/vl.c b/vl.c
index 9440f98..afc0144 100644
--- a/vl.c
+++ b/vl.c
@@ -1217,8 +1217,8 @@ static QTAILQ_HEAD(reset_handlers, QEMUResetEntry) reset_handlers =
 static int reset_requested;
 static int shutdown_requested;
 static int powerdown_requested;
-int debug_requested;
-int vmstop_requested;
+static int debug_requested;
+static int vmstop_requested;
 
 int qemu_shutdown_requested(void)
 {
@@ -1312,6 +1312,18 @@ void qemu_system_powerdown_request(void)
     qemu_notify_event();
 }
 
+void qemu_system_debug_request(void)
+{
+    debug_requested = 1;
+    vm_stop(VMSTOP_DEBUG);
+}
+
+void qemu_system_vmstop_request(int reason)
+{
+    vmstop_requested = reason;
+    qemu_notify_event();
+}
+
 void main_loop_wait(int nonblocking)
 {
     IOHandlerRecord *ioh;
@@ -1427,8 +1439,8 @@ static void main_loop(void)
         dev_time += profile_getclock() - ti;
 #endif
 
-        if ((r = qemu_debug_requested())) {
-            vm_stop(r);
+        if (qemu_debug_requested()) {
+            vm_stop(VMSTOP_DEBUG);
         }
         if (qemu_shutdown_requested()) {
             monitor_protocol_event(QEVENT_SHUTDOWN, NULL);
-- 
1.7.1

[Qemu-devel] [PATCH 06/15] Move debug exception handling out of cpu_exec

[Jan Kiszka]

To prepare splitting up KVM and TCG CPU entry/exit, move the debug
exception into cpus.c and invoke cpu_handle_debug_exception on return
from qemu_cpu_exec.

This also allows to clean up the debug request signaling: We can assign
the job of informing main-loop to qemu_system_debug_request and stop the
calling cpu directly in cpu_handle_debug_exception. That means a debug
stop will now only be signaled via debug_requested and not additionally
via vmstop_requested.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 cpu-exec.c |   24 ------------------------
 cpus.c     |   35 ++++++++++++++++++++++++++++++-----
 vl.c       |    2 +-
 3 files changed, 31 insertions(+), 30 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 8c9fb8b..9c0b10d 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -196,28 +196,6 @@ static inline TranslationBlock *tb_find_fast(void)
     return tb;
 }
 
-static CPUDebugExcpHandler *debug_excp_handler;
-
-CPUDebugExcpHandler *cpu_set_debug_excp_handler(CPUDebugExcpHandler *handler)
-{
-    CPUDebugExcpHandler *old_handler = debug_excp_handler;
-
-    debug_excp_handler = handler;
-    return old_handler;
-}
-
-static void cpu_handle_debug_exception(CPUState *env)
-{
-    CPUWatchpoint *wp;
-
-    if (!env->watchpoint_hit)
-        QTAILQ_FOREACH(wp, &env->watchpoints, entry)
-            wp->flags &= ~BP_WATCHPOINT_HIT;
-
-    if (debug_excp_handler)
-        debug_excp_handler(env);
-}
-
 /* main execution loop */
 
 volatile sig_atomic_t exit_request;
@@ -287,8 +265,6 @@ int cpu_exec(CPUState *env1)
                 if (env->exception_index >= EXCP_INTERRUPT) {
                     /* exit request from the cpu execution loop */
                     ret = env->exception_index;
-                    if (ret == EXCP_DEBUG)
-                        cpu_handle_debug_exception(env);
                     break;
                 } else {
 #if defined(CONFIG_USER_ONLY)
diff --git a/cpus.c b/cpus.c
index 85c680e..d4b5e80 100644
--- a/cpus.c
+++ b/cpus.c
@@ -165,10 +165,34 @@ static bool all_cpus_idle(void)
     return true;
 }
 
-static void cpu_debug_handler(CPUState *env)
+static CPUDebugExcpHandler *debug_excp_handler;
+
+CPUDebugExcpHandler *cpu_set_debug_excp_handler(CPUDebugExcpHandler *handler)
+{
+    CPUDebugExcpHandler *old_handler = debug_excp_handler;
+
+    debug_excp_handler = handler;
+    return old_handler;
+}
+
+static void cpu_handle_debug_exception(CPUState *env)
 {
+    CPUWatchpoint *wp;
+
+    if (!env->watchpoint_hit) {
+        QTAILQ_FOREACH(wp, &env->watchpoints, entry) {
+            wp->flags &= ~BP_WATCHPOINT_HIT;
+        }
+    }
+    if (debug_excp_handler) {
+        debug_excp_handler(env);
+    }
+
     gdb_set_stop_cpu(env);
     qemu_system_debug_request();
+#ifdef CONFIG_IOTHREAD
+    env->stopped = 1;
+#endif
 }
 
 #ifdef CONFIG_LINUX
@@ -479,7 +503,6 @@ int qemu_init_main_loop(void)
         return ret;
     }
 #endif
-    cpu_set_debug_excp_handler(cpu_debug_handler);
 
     qemu_init_sigbus();
 
@@ -653,8 +676,6 @@ int qemu_init_main_loop(void)
     int ret;
     sigset_t blocked_signals;
 
-    cpu_set_debug_excp_handler(cpu_debug_handler);
-
     qemu_init_sigbus();
 
     blocked_signals = block_io_signals();
@@ -808,7 +829,10 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
 
     while (1) {
         if (cpu_can_run(env)) {
-            qemu_cpu_exec(env);
+            r = qemu_cpu_exec(env);
+            if (r == EXCP_DEBUG) {
+                cpu_handle_debug_exception(env);
+            }
         }
         qemu_kvm_wait_io_event(env);
     }
@@ -1076,6 +1100,7 @@ bool cpu_exec_all(void)
                 qemu_kvm_eat_signals(env);
             }
             if (r == EXCP_DEBUG) {
+                cpu_handle_debug_exception(env);
                 break;
             }
         } else if (env->stop) {
diff --git a/vl.c b/vl.c
index afc0144..79a2332 100644
--- a/vl.c
+++ b/vl.c
@@ -1315,7 +1315,7 @@ void qemu_system_powerdown_request(void)
 void qemu_system_debug_request(void)
 {
     debug_requested = 1;
-    vm_stop(VMSTOP_DEBUG);
+    qemu_notify_event();
 }
 
 void qemu_system_vmstop_request(int reason)
-- 
1.7.1

[Qemu-devel] [PATCH 07/15] kvm: Separate TCG from KVM cpu execution

[Jan Kiszka]

Mixing up TCG bits with KVM already led to problems around eflags
emulation on x86. Moreover, quite some code that TCG requires on cpu
enty/exit is useless for KVM. So dispatch between tcg_cpu_exec and
kvm_cpu_exec as early as possible.

The core logic of cpu_halted from cpu_exec is added to
kvm_arch_process_irqchip_events. Moving away from cpu_exec makes
exception_index meaningless for KVM, we can simply pass the exit reason
directly (only "EXCP_DEBUG vs. rest" is relevant).

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 cpu-exec.c        |   19 ++++++-------------
 cpus.c            |   10 +++++-----
 kvm-all.c         |   19 +++++++++----------
 target-i386/kvm.c |    6 +++---
 4 files changed, 23 insertions(+), 31 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 9c0b10d..b03b3a7 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -226,13 +226,11 @@ int cpu_exec(CPUState *env1)
     }
 
 #if defined(TARGET_I386)
-    if (!kvm_enabled()) {
-        /* put eflags in CPU temporary format */
-        CC_SRC = env->eflags & (CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C);
-        DF = 1 - (2 * ((env->eflags >> 10) & 1));
-        CC_OP = CC_OP_EFLAGS;
-        env->eflags &= ~(DF_MASK | CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C);
-    }
+    /* put eflags in CPU temporary format */
+    CC_SRC = env->eflags & (CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C);
+    DF = 1 - (2 * ((env->eflags >> 10) & 1));
+    CC_OP = CC_OP_EFLAGS;
+    env->eflags &= ~(DF_MASK | CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C);
 #elif defined(TARGET_SPARC)
 #elif defined(TARGET_M68K)
     env->cc_op = CC_OP_FLAGS;
@@ -257,7 +255,7 @@ int cpu_exec(CPUState *env1)
         if (setjmp(env->jmp_env) == 0) {
 #if defined(__sparc__) && !defined(CONFIG_SOLARIS)
 #undef env
-                    env = cpu_single_env;
+            env = cpu_single_env;
 #define env cpu_single_env
 #endif
             /* if an exception is pending, we execute it here */
@@ -316,11 +314,6 @@ int cpu_exec(CPUState *env1)
                 }
             }
 
-            if (kvm_enabled()) {
-                kvm_cpu_exec(env);
-                longjmp(env->jmp_env, 1);
-            }
-
             next_tb = 0; /* force lookup of first TB */
             for(;;) {
                 interrupt_request = env->interrupt_request;
diff --git a/cpus.c b/cpus.c
index d4b5e80..2414316 100644
--- a/cpus.c
+++ b/cpus.c
@@ -800,8 +800,6 @@ static void qemu_kvm_wait_io_event(CPUState *env)
     qemu_wait_io_event_common(env);
 }
 
-static int qemu_cpu_exec(CPUState *env);
-
 static void *qemu_kvm_cpu_thread_fn(void *arg)
 {
     CPUState *env = arg;
@@ -829,7 +827,7 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
 
     while (1) {
         if (cpu_can_run(env)) {
-            r = qemu_cpu_exec(env);
+            r = kvm_cpu_exec(env);
             if (r == EXCP_DEBUG) {
                 cpu_handle_debug_exception(env);
             }
@@ -1040,7 +1038,7 @@ void vm_stop(int reason)
 
 #endif
 
-static int qemu_cpu_exec(CPUState *env)
+static int tcg_cpu_exec(CPUState *env)
 {
     int ret;
 #ifdef CONFIG_PROFILER
@@ -1095,9 +1093,11 @@ bool cpu_exec_all(void)
             break;
         }
         if (cpu_can_run(env)) {
-            r = qemu_cpu_exec(env);
             if (kvm_enabled()) {
+                r = kvm_cpu_exec(env);
                 qemu_kvm_eat_signals(env);
+            } else {
+                r = tcg_cpu_exec(env);
             }
             if (r == EXCP_DEBUG) {
                 cpu_handle_debug_exception(env);
diff --git a/kvm-all.c b/kvm-all.c
index 19cf188..802c6b8 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -895,10 +895,11 @@ int kvm_cpu_exec(CPUState *env)
 
     if (kvm_arch_process_irqchip_events(env)) {
         env->exit_request = 0;
-        env->exception_index = EXCP_HLT;
-        return 0;
+        return EXCP_HLT;
     }
 
+    cpu_single_env = env;
+
     do {
         if (env->kvm_vcpu_dirty) {
             kvm_arch_put_registers(env, KVM_PUT_RUNTIME_STATE);
@@ -927,7 +928,6 @@ int kvm_cpu_exec(CPUState *env)
         kvm_flush_coalesced_mmio_buffer();
 
         if (ret == -EINTR || ret == -EAGAIN) {
-            cpu_exit(env);
             DPRINTF("io window exit\n");
             ret = 0;
             break;
@@ -978,8 +978,8 @@ int kvm_cpu_exec(CPUState *env)
             DPRINTF("kvm_exit_debug\n");
 #ifdef KVM_CAP_SET_GUEST_DEBUG
             if (kvm_arch_debug(&run->debug.arch)) {
-                env->exception_index = EXCP_DEBUG;
-                return 0;
+                ret = EXCP_DEBUG;
+                goto out;
             }
             /* re-enter, this exception was guest-internal */
             ret = 1;
@@ -995,13 +995,12 @@ int kvm_cpu_exec(CPUState *env)
     if (ret < 0) {
         cpu_dump_state(env, stderr, fprintf, CPU_DUMP_CODE);
         vm_stop(VMSTOP_PANIC);
-        env->exit_request = 1;
-    }
-    if (env->exit_request) {
-        env->exit_request = 0;
-        env->exception_index = EXCP_INTERRUPT;
     }
+    ret = EXCP_INTERRUPT;
 
+out:
+    env->exit_request = 0;
+    cpu_single_env = NULL;
     return ret;
 }
 
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index ba183c4..377a0a3 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1502,12 +1502,13 @@ int kvm_arch_post_run(CPUState *env, struct kvm_run *run)
 
 int kvm_arch_process_irqchip_events(CPUState *env)
 {
+    if (env->interrupt_request & (CPU_INTERRUPT_HARD | CPU_INTERRUPT_NMI)) {
+        env->halted = 0;
+    }
     if (env->interrupt_request & CPU_INTERRUPT_INIT) {
         kvm_cpu_synchronize_state(env);
         do_cpu_init(env);
-        env->exception_index = EXCP_HALTED;
     }
-
     if (env->interrupt_request & CPU_INTERRUPT_SIPI) {
         kvm_cpu_synchronize_state(env);
         do_cpu_sipi(env);
@@ -1522,7 +1523,6 @@ static int kvm_handle_halt(CPUState *env)
           (env->eflags & IF_MASK)) &&
         !(env->interrupt_request & CPU_INTERRUPT_NMI)) {
         env->halted = 1;
-        env->exception_index = EXCP_HLT;
         return 0;
     }
 
-- 
1.7.1

[Qemu-devel] Re: [PATCH 07/15] kvm: Separate TCG from KVM cpu execution

[Marcelo Tosatti]

On Mon, Feb 07, 2011 at 12:19:18PM +0100, Jan Kiszka wrote:
> Mixing up TCG bits with KVM already led to problems around eflags
> emulation on x86. Moreover, quite some code that TCG requires on cpu
> enty/exit is useless for KVM. So dispatch between tcg_cpu_exec and
> kvm_cpu_exec as early as possible.
> 
> The core logic of cpu_halted from cpu_exec is added to
> kvm_arch_process_irqchip_events. Moving away from cpu_exec makes
> exception_index meaningless for KVM, we can simply pass the exit reason
> directly (only "EXCP_DEBUG vs. rest" is relevant).
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  cpu-exec.c        |   19 ++++++-------------
>  cpus.c            |   10 +++++-----
>  kvm-all.c         |   19 +++++++++----------
>  target-i386/kvm.c |    6 +++---
>  4 files changed, 23 insertions(+), 31 deletions(-)
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index ba183c4..377a0a3 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -1502,12 +1502,13 @@ int kvm_arch_post_run(CPUState *env, struct kvm_run *run)
>  
>  int kvm_arch_process_irqchip_events(CPUState *env)
>  {
> +    if (env->interrupt_request & (CPU_INTERRUPT_HARD | CPU_INTERRUPT_NMI)) {
> +        env->halted = 0;
> +    }

Why is it necessary to clear env->halted here?

[Qemu-devel] Re: [PATCH 07/15] kvm: Separate TCG from KVM cpu execution

[Jan Kiszka]

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

On 2011-02-09 00:39, Marcelo Tosatti wrote:
> On Mon, Feb 07, 2011 at 12:19:18PM +0100, Jan Kiszka wrote:
>> Mixing up TCG bits with KVM already led to problems around eflags
>> emulation on x86. Moreover, quite some code that TCG requires on cpu
>> enty/exit is useless for KVM. So dispatch between tcg_cpu_exec and
>> kvm_cpu_exec as early as possible.
>>
>> The core logic of cpu_halted from cpu_exec is added to
>> kvm_arch_process_irqchip_events. Moving away from cpu_exec makes
>> exception_index meaningless for KVM, we can simply pass the exit reason
>> directly (only "EXCP_DEBUG vs. rest" is relevant).
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  cpu-exec.c        |   19 ++++++-------------
>>  cpus.c            |   10 +++++-----
>>  kvm-all.c         |   19 +++++++++----------
>>  target-i386/kvm.c |    6 +++---
>>  4 files changed, 23 insertions(+), 31 deletions(-)
>>
>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
>> index ba183c4..377a0a3 100644
>> --- a/target-i386/kvm.c
>> +++ b/target-i386/kvm.c
>> @@ -1502,12 +1502,13 @@ int kvm_arch_post_run(CPUState *env, struct kvm_run *run)
>>  
>>  int kvm_arch_process_irqchip_events(CPUState *env)
>>  {
>> +    if (env->interrupt_request & (CPU_INTERRUPT_HARD | CPU_INTERRUPT_NMI)) {
>> +        env->halted = 0;
>> +    }
> 
> Why is it necessary to clear env->halted here?

Because we no longer come along cpu_halted() in cpu_exec(). This
corresponds to the tail of process_irqchip_events() in qemu-kvm

Jan


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

[Qemu-devel] Re: [PATCH 07/15] kvm: Separate TCG from KVM cpu execution

[Marcelo Tosatti]

On Wed, Feb 09, 2011 at 08:59:23AM +0100, Jan Kiszka wrote:
> On 2011-02-09 00:39, Marcelo Tosatti wrote:
> > On Mon, Feb 07, 2011 at 12:19:18PM +0100, Jan Kiszka wrote:
> >> Mixing up TCG bits with KVM already led to problems around eflags
> >> emulation on x86. Moreover, quite some code that TCG requires on cpu
> >> enty/exit is useless for KVM. So dispatch between tcg_cpu_exec and
> >> kvm_cpu_exec as early as possible.
> >>
> >> The core logic of cpu_halted from cpu_exec is added to
> >> kvm_arch_process_irqchip_events. Moving away from cpu_exec makes
> >> exception_index meaningless for KVM, we can simply pass the exit reason
> >> directly (only "EXCP_DEBUG vs. rest" is relevant).
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> ---
> >>  cpu-exec.c        |   19 ++++++-------------
> >>  cpus.c            |   10 +++++-----
> >>  kvm-all.c         |   19 +++++++++----------
> >>  target-i386/kvm.c |    6 +++---
> >>  4 files changed, 23 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> >> index ba183c4..377a0a3 100644
> >> --- a/target-i386/kvm.c
> >> +++ b/target-i386/kvm.c
> >> @@ -1502,12 +1502,13 @@ int kvm_arch_post_run(CPUState *env, struct kvm_run *run)
> >>  
> >>  int kvm_arch_process_irqchip_events(CPUState *env)
> >>  {
> >> +    if (env->interrupt_request & (CPU_INTERRUPT_HARD | CPU_INTERRUPT_NMI)) {
> >> +        env->halted = 0;
> >> +    }
> > 
> > Why is it necessary to clear env->halted here?
> 
> Because we no longer come along cpu_halted() in cpu_exec(). This
> corresponds to the tail of process_irqchip_events() in qemu-kvm

This is not yet well integrated, we probably don't need env->halted
anymore (see cpu_has_work). Can be improved later though.

[Qemu-devel] Re: [PATCH 07/15] kvm: Separate TCG from KVM cpu execution

[Jan Kiszka]

On 2011-02-09 15:44, Marcelo Tosatti wrote:
> On Wed, Feb 09, 2011 at 08:59:23AM +0100, Jan Kiszka wrote:
>> On 2011-02-09 00:39, Marcelo Tosatti wrote:
>>> On Mon, Feb 07, 2011 at 12:19:18PM +0100, Jan Kiszka wrote:
>>>> Mixing up TCG bits with KVM already led to problems around eflags
>>>> emulation on x86. Moreover, quite some code that TCG requires on cpu
>>>> enty/exit is useless for KVM. So dispatch between tcg_cpu_exec and
>>>> kvm_cpu_exec as early as possible.
>>>>
>>>> The core logic of cpu_halted from cpu_exec is added to
>>>> kvm_arch_process_irqchip_events. Moving away from cpu_exec makes
>>>> exception_index meaningless for KVM, we can simply pass the exit reason
>>>> directly (only "EXCP_DEBUG vs. rest" is relevant).
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> ---
>>>>  cpu-exec.c        |   19 ++++++-------------
>>>>  cpus.c            |   10 +++++-----
>>>>  kvm-all.c         |   19 +++++++++----------
>>>>  target-i386/kvm.c |    6 +++---
>>>>  4 files changed, 23 insertions(+), 31 deletions(-)
>>>>
>>>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
>>>> index ba183c4..377a0a3 100644
>>>> --- a/target-i386/kvm.c
>>>> +++ b/target-i386/kvm.c
>>>> @@ -1502,12 +1502,13 @@ int kvm_arch_post_run(CPUState *env, struct kvm_run *run)
>>>>  
>>>>  int kvm_arch_process_irqchip_events(CPUState *env)
>>>>  {
>>>> +    if (env->interrupt_request & (CPU_INTERRUPT_HARD | CPU_INTERRUPT_NMI)) {
>>>> +        env->halted = 0;
>>>> +    }
>>>
>>> Why is it necessary to clear env->halted here?
>>
>> Because we no longer come along cpu_halted() in cpu_exec(). This
>> corresponds to the tail of process_irqchip_events() in qemu-kvm
> 
> This is not yet well integrated, we probably don't need env->halted
> anymore (see cpu_has_work). Can be improved later though.

So far we check for it, at least in cpu_[thread_]is_idle. And that's a
generic service.

Jan

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

[Qemu-devel] [PATCH 08/15] kvm: x86: Prepare VCPU loop for in-kernel irqchip

[Jan Kiszka]

Effectively no functional change yet as kvm_irqchip_in_kernel still only
returns 0, but this patch will allow qemu-kvm to adopt the VCPU loop of
upsteam KVM.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 target-i386/kvm.c |   69 +++++++++++++++++++++++++++++-----------------------
 1 files changed, 38 insertions(+), 31 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 377a0a3..f02f478 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1442,11 +1442,6 @@ int kvm_arch_get_registers(CPUState *env)
 
 int kvm_arch_pre_run(CPUState *env, struct kvm_run *run)
 {
-    /* Force the VCPU out of its inner loop to process the INIT request */
-    if (env->interrupt_request & CPU_INTERRUPT_INIT) {
-        env->exit_request = 1;
-    }
-
     /* Inject NMI */
     if (env->interrupt_request & CPU_INTERRUPT_NMI) {
         env->interrupt_request &= ~CPU_INTERRUPT_NMI;
@@ -1454,35 +1449,43 @@ int kvm_arch_pre_run(CPUState *env, struct kvm_run *run)
         kvm_vcpu_ioctl(env, KVM_NMI);
     }
 
-    /* Try to inject an interrupt if the guest can accept it */
-    if (run->ready_for_interrupt_injection &&
-        (env->interrupt_request & CPU_INTERRUPT_HARD) &&
-        (env->eflags & IF_MASK)) {
-        int irq;
-
-        env->interrupt_request &= ~CPU_INTERRUPT_HARD;
-        irq = cpu_get_pic_interrupt(env);
-        if (irq >= 0) {
-            struct kvm_interrupt intr;
-            intr.irq = irq;
-            /* FIXME: errors */
-            DPRINTF("injected interrupt %d\n", irq);
-            kvm_vcpu_ioctl(env, KVM_INTERRUPT, &intr);
+    if (!kvm_irqchip_in_kernel()) {
+        /* Force the VCPU out of its inner loop to process the INIT request */
+        if (env->interrupt_request & CPU_INTERRUPT_INIT) {
+            env->exit_request = 1;
         }
-    }
 
-    /* If we have an interrupt but the guest is not ready to receive an
-     * interrupt, request an interrupt window exit.  This will
-     * cause a return to userspace as soon as the guest is ready to
-     * receive interrupts. */
-    if ((env->interrupt_request & CPU_INTERRUPT_HARD)) {
-        run->request_interrupt_window = 1;
-    } else {
-        run->request_interrupt_window = 0;
-    }
+        /* Try to inject an interrupt if the guest can accept it */
+        if (run->ready_for_interrupt_injection &&
+            (env->interrupt_request & CPU_INTERRUPT_HARD) &&
+            (env->eflags & IF_MASK)) {
+            int irq;
+
+            env->interrupt_request &= ~CPU_INTERRUPT_HARD;
+            irq = cpu_get_pic_interrupt(env);
+            if (irq >= 0) {
+                struct kvm_interrupt intr;
+
+                intr.irq = irq;
+                /* FIXME: errors */
+                DPRINTF("injected interrupt %d\n", irq);
+                kvm_vcpu_ioctl(env, KVM_INTERRUPT, &intr);
+            }
+        }
 
-    DPRINTF("setting tpr\n");
-    run->cr8 = cpu_get_apic_tpr(env->apic_state);
+        /* If we have an interrupt but the guest is not ready to receive an
+         * interrupt, request an interrupt window exit.  This will
+         * cause a return to userspace as soon as the guest is ready to
+         * receive interrupts. */
+        if ((env->interrupt_request & CPU_INTERRUPT_HARD)) {
+            run->request_interrupt_window = 1;
+        } else {
+            run->request_interrupt_window = 0;
+        }
+
+        DPRINTF("setting tpr\n");
+        run->cr8 = cpu_get_apic_tpr(env->apic_state);
+    }
 
     return 0;
 }
@@ -1502,6 +1505,10 @@ int kvm_arch_post_run(CPUState *env, struct kvm_run *run)
 
 int kvm_arch_process_irqchip_events(CPUState *env)
 {
+    if (kvm_irqchip_in_kernel()) {
+        return 0;
+    }
+
     if (env->interrupt_request & (CPU_INTERRUPT_HARD | CPU_INTERRUPT_NMI)) {
         env->halted = 0;
     }
-- 
1.7.1

[Qemu-devel] [PATCH 09/15] kvm: Drop return values from kvm_arch_pre/post_run

[Jan Kiszka]

We do not check them, and the only arch with non-empty implementations
always returns 0 (this is also true for qemu-kvm).

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
CC: Alexander Graf <agraf@suse.de>
---
 kvm.h              |    5 ++---
 target-i386/kvm.c  |    8 ++------
 target-ppc/kvm.c   |    6 ++----
 target-s390x/kvm.c |    6 ++----
 4 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/kvm.h b/kvm.h
index b2fb5c6..7a4d550 100644
--- a/kvm.h
+++ b/kvm.h
@@ -99,12 +99,11 @@ int kvm_vcpu_ioctl(CPUState *env, int type, ...);
 
 extern const KVMCapabilityInfo kvm_arch_required_capabilities[];
 
-int kvm_arch_post_run(CPUState *env, struct kvm_run *run);
+void kvm_arch_pre_run(CPUState *env, struct kvm_run *run);
+void kvm_arch_post_run(CPUState *env, struct kvm_run *run);
 
 int kvm_arch_handle_exit(CPUState *env, struct kvm_run *run);
 
-int kvm_arch_pre_run(CPUState *env, struct kvm_run *run);
-
 int kvm_arch_process_irqchip_events(CPUState *env);
 
 int kvm_arch_get_registers(CPUState *env);
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index f02f478..8668cb3 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1440,7 +1440,7 @@ int kvm_arch_get_registers(CPUState *env)
     return 0;
 }
 
-int kvm_arch_pre_run(CPUState *env, struct kvm_run *run)
+void kvm_arch_pre_run(CPUState *env, struct kvm_run *run)
 {
     /* Inject NMI */
     if (env->interrupt_request & CPU_INTERRUPT_NMI) {
@@ -1486,11 +1486,9 @@ int kvm_arch_pre_run(CPUState *env, struct kvm_run *run)
         DPRINTF("setting tpr\n");
         run->cr8 = cpu_get_apic_tpr(env->apic_state);
     }
-
-    return 0;
 }
 
-int kvm_arch_post_run(CPUState *env, struct kvm_run *run)
+void kvm_arch_post_run(CPUState *env, struct kvm_run *run)
 {
     if (run->if_flag) {
         env->eflags |= IF_MASK;
@@ -1499,8 +1497,6 @@ int kvm_arch_post_run(CPUState *env, struct kvm_run *run)
     }
     cpu_set_apic_tpr(env->apic_state, run->cr8);
     cpu_set_apic_base(env->apic_state, run->apic_base);
-
-    return 0;
 }
 
 int kvm_arch_process_irqchip_events(CPUState *env)
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 93ecc57..bd4012a 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -256,14 +256,12 @@ int kvm_arch_pre_run(CPUState *env, struct kvm_run *run)
     return 0;
 }
 
-int kvm_arch_post_run(CPUState *env, struct kvm_run *run)
+void kvm_arch_post_run(CPUState *env, struct kvm_run *run)
 {
-    return 0;
 }
 
-int kvm_arch_process_irqchip_events(CPUState *env)
+void kvm_arch_process_irqchip_events(CPUState *env)
 {
-    return 0;
 }
 
 static int kvmppc_handle_halt(CPUState *env)
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index 1702c46..b349812 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -169,14 +169,12 @@ int kvm_arch_remove_sw_breakpoint(CPUState *env, struct kvm_sw_breakpoint *bp)
     return 0;
 }
 
-int kvm_arch_pre_run(CPUState *env, struct kvm_run *run)
+void kvm_arch_pre_run(CPUState *env, struct kvm_run *run)
 {
-    return 0;
 }
 
-int kvm_arch_post_run(CPUState *env, struct kvm_run *run)
+void kvm_arch_post_run(CPUState *env, struct kvm_run *run)
 {
-    return 0;
 }
 
 int kvm_arch_process_irqchip_events(CPUState *env)
-- 
1.7.1

[Qemu-devel] Re: [PATCH 09/15] kvm: Drop return values from kvm_arch_pre/post_run

[Alexander Graf]

On 07.02.2011, at 12:19, Jan Kiszka wrote:

> We do not check them, and the only arch with non-empty implementations
> always returns 0 (this is also true for qemu-kvm).
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> CC: Alexander Graf <agraf@suse.de>

Yeah, if there's anything going wrong we should do cpu_abort anyways.

Acked-by: Alexander Graf <agraf@suse.de>


Alex

[Qemu-devel] [PATCH] kvm: ppc: Fix breakage of kvm_arch_pre_run/process_irqchip_events

[Jan Kiszka]

On 2011-02-07 12:19, Jan Kiszka wrote:
> We do not check them, and the only arch with non-empty implementations
> always returns 0 (this is also true for qemu-kvm).
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> CC: Alexander Graf <agraf@suse.de>
> ---
>  kvm.h              |    5 ++---
>  target-i386/kvm.c  |    8 ++------
>  target-ppc/kvm.c   |    6 ++----
>  target-s390x/kvm.c |    6 ++----
>  4 files changed, 8 insertions(+), 17 deletions(-)
> 

...

> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 93ecc57..bd4012a 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -256,14 +256,12 @@ int kvm_arch_pre_run(CPUState *env, struct kvm_run *run)
>      return 0;
>  }
>  
> -int kvm_arch_post_run(CPUState *env, struct kvm_run *run)
> +void kvm_arch_post_run(CPUState *env, struct kvm_run *run)
>  {
> -    return 0;
>  }
>  
> -int kvm_arch_process_irqchip_events(CPUState *env)
> +void kvm_arch_process_irqchip_events(CPUState *env)
>  {
> -    return 0;
>  }

Oops. Do we already have a built-bot for KVM-enabled PPC (and s390)
targets somewhere?

Jan

-------8<----------

From: Jan Kiszka <jan.kiszka@siemens.com>

Commit 7a39fe5882 failed to convert the right arch function.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 target-ppc/kvm.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index bd4012a..3924f4b 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -222,7 +222,7 @@ int kvmppc_set_interrupt(CPUState *env, int irq, int level)
 #define PPC_INPUT_INT PPC6xx_INPUT_INT
 #endif
 
-int kvm_arch_pre_run(CPUState *env, struct kvm_run *run)
+void kvm_arch_pre_run(CPUState *env, struct kvm_run *run)
 {
     int r;
     unsigned irq;
@@ -253,15 +253,15 @@ int kvm_arch_pre_run(CPUState *env, struct kvm_run *run)
     /* We don't know if there are more interrupts pending after this. However,
      * the guest will return to userspace in the course of handling this one
      * anyways, so we will get a chance to deliver the rest. */
-    return 0;
 }
 
 void kvm_arch_post_run(CPUState *env, struct kvm_run *run)
 {
 }
 
-void kvm_arch_process_irqchip_events(CPUState *env)
+int kvm_arch_process_irqchip_events(CPUState *env)
 {
+    return 0;
 }
 
 static int kvmppc_handle_halt(CPUState *env)

[Qemu-devel] Re: [PATCH] kvm: ppc: Fix breakage of kvm_arch_pre_run/process_irqchip_events

[Alexander Graf]

On 17.02.2011, at 22:01, Jan Kiszka wrote:

> On 2011-02-07 12:19, Jan Kiszka wrote:
>> We do not check them, and the only arch with non-empty implementations
>> always returns 0 (this is also true for qemu-kvm).
>> 
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> CC: Alexander Graf <agraf@suse.de>
>> ---
>> kvm.h              |    5 ++---
>> target-i386/kvm.c  |    8 ++------
>> target-ppc/kvm.c   |    6 ++----
>> target-s390x/kvm.c |    6 ++----
>> 4 files changed, 8 insertions(+), 17 deletions(-)
>> 
> 
> ...
> 
>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>> index 93ecc57..bd4012a 100644
>> --- a/target-ppc/kvm.c
>> +++ b/target-ppc/kvm.c
>> @@ -256,14 +256,12 @@ int kvm_arch_pre_run(CPUState *env, struct kvm_run *run)
>>     return 0;
>> }
>> 
>> -int kvm_arch_post_run(CPUState *env, struct kvm_run *run)
>> +void kvm_arch_post_run(CPUState *env, struct kvm_run *run)
>> {
>> -    return 0;
>> }
>> 
>> -int kvm_arch_process_irqchip_events(CPUState *env)
>> +void kvm_arch_process_irqchip_events(CPUState *env)
>> {
>> -    return 0;
>> }
> 
> Oops. Do we already have a built-bot for KVM-enabled PPC (and s390)
> targets somewhere?

Just before leaving for vacation I prepared a machine for each and gave stefan access to them. Looks like they're not officially running though - will try to look at this asap.


Alex

[Qemu-devel] Re: [PATCH] kvm: ppc: Fix breakage of kvm_arch_pre_run/process_irqchip_events

[Stefan Hajnoczi]

On Fri, Mar 11, 2011 at 5:55 AM, Alexander Graf <agraf@suse.de> wrote:
>
> On 17.02.2011, at 22:01, Jan Kiszka wrote:
>
>> On 2011-02-07 12:19, Jan Kiszka wrote:
>>> We do not check them, and the only arch with non-empty implementations
>>> always returns 0 (this is also true for qemu-kvm).
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> CC: Alexander Graf <agraf@suse.de>
>>> ---
>>> kvm.h              |    5 ++---
>>> target-i386/kvm.c  |    8 ++------
>>> target-ppc/kvm.c   |    6 ++----
>>> target-s390x/kvm.c |    6 ++----
>>> 4 files changed, 8 insertions(+), 17 deletions(-)
>>>
>>
>> ...
>>
>>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>>> index 93ecc57..bd4012a 100644
>>> --- a/target-ppc/kvm.c
>>> +++ b/target-ppc/kvm.c
>>> @@ -256,14 +256,12 @@ int kvm_arch_pre_run(CPUState *env, struct kvm_run *run)
>>>     return 0;
>>> }
>>>
>>> -int kvm_arch_post_run(CPUState *env, struct kvm_run *run)
>>> +void kvm_arch_post_run(CPUState *env, struct kvm_run *run)
>>> {
>>> -    return 0;
>>> }
>>>
>>> -int kvm_arch_process_irqchip_events(CPUState *env)
>>> +void kvm_arch_process_irqchip_events(CPUState *env)
>>> {
>>> -    return 0;
>>> }
>>
>> Oops. Do we already have a built-bot for KVM-enabled PPC (and s390)
>> targets somewhere?
>
> Just before leaving for vacation I prepared a machine for each and gave stefan access to them. Looks like they're not officially running though - will try to look at this asap.

They are in the process of being added to the buildbot by Daniel
Gollub.  However, the ppc box is unable to build qemu.git because it
hits ENOMEM while compiling.  I doubled swap size but that didn't fix
the issue so I need to investigate more.  At least s390 should be good
to go soon and I will send an update when it is up and running.

Stefan

[Qemu-devel] Re: [PATCH] kvm: ppc: Fix breakage of kvm_arch_pre_run/process_irqchip_events

[Alexander Graf]

On 11.03.2011, at 07:26, Stefan Hajnoczi wrote:

> On Fri, Mar 11, 2011 at 5:55 AM, Alexander Graf <agraf@suse.de> wrote:
>> 
>> On 17.02.2011, at 22:01, Jan Kiszka wrote:
>> 
>>> On 2011-02-07 12:19, Jan Kiszka wrote:
>>>> We do not check them, and the only arch with non-empty implementations
>>>> always returns 0 (this is also true for qemu-kvm).
>>>> 
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> CC: Alexander Graf <agraf@suse.de>
>>>> ---
>>>> kvm.h              |    5 ++---
>>>> target-i386/kvm.c  |    8 ++------
>>>> target-ppc/kvm.c   |    6 ++----
>>>> target-s390x/kvm.c |    6 ++----
>>>> 4 files changed, 8 insertions(+), 17 deletions(-)
>>>> 
>>> 
>>> ...
>>> 
>>>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>>>> index 93ecc57..bd4012a 100644
>>>> --- a/target-ppc/kvm.c
>>>> +++ b/target-ppc/kvm.c
>>>> @@ -256,14 +256,12 @@ int kvm_arch_pre_run(CPUState *env, struct kvm_run *run)
>>>>     return 0;
>>>> }
>>>> 
>>>> -int kvm_arch_post_run(CPUState *env, struct kvm_run *run)
>>>> +void kvm_arch_post_run(CPUState *env, struct kvm_run *run)
>>>> {
>>>> -    return 0;
>>>> }
>>>> 
>>>> -int kvm_arch_process_irqchip_events(CPUState *env)
>>>> +void kvm_arch_process_irqchip_events(CPUState *env)
>>>> {
>>>> -    return 0;
>>>> }
>>> 
>>> Oops. Do we already have a built-bot for KVM-enabled PPC (and s390)
>>> targets somewhere?
>> 
>> Just before leaving for vacation I prepared a machine for each and gave stefan access to them. Looks like they're not officially running though - will try to look at this asap.
> 
> They are in the process of being added to the buildbot by Daniel
> Gollub.  However, the ppc box is unable to build qemu.git because it
> hits ENOMEM while compiling.  I doubled swap size but that didn't fix
> the issue so I need to investigate more.  At least s390 should be good

Hrm, yeah. I hit that one too just before I left. Maybe I should just search for another RAM bar :).

> to go soon and I will send an update when it is up and running.

Thanks!


Alex

[Qemu-devel] [PATCH 10/15] kvm: x86: Catch and report failing IRQ and NMI injections

[Jan Kiszka]

We do not need to abort, but the user should be notified that weird
things go on.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 target-i386/kvm.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 8668cb3..0aa0a41 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1442,11 +1442,17 @@ int kvm_arch_get_registers(CPUState *env)
 
 void kvm_arch_pre_run(CPUState *env, struct kvm_run *run)
 {
+    int ret;
+
     /* Inject NMI */
     if (env->interrupt_request & CPU_INTERRUPT_NMI) {
         env->interrupt_request &= ~CPU_INTERRUPT_NMI;
         DPRINTF("injected NMI\n");
-        kvm_vcpu_ioctl(env, KVM_NMI);
+        ret = kvm_vcpu_ioctl(env, KVM_NMI);
+        if (ret < 0) {
+            fprintf(stderr, "KVM: injection failed, NMI lost (%s)\n",
+                    strerror(-ret));
+        }
     }
 
     if (!kvm_irqchip_in_kernel()) {
@@ -1467,9 +1473,13 @@ void kvm_arch_pre_run(CPUState *env, struct kvm_run *run)
                 struct kvm_interrupt intr;
 
                 intr.irq = irq;
-                /* FIXME: errors */
                 DPRINTF("injected interrupt %d\n", irq);
-                kvm_vcpu_ioctl(env, KVM_INTERRUPT, &intr);
+                ret = kvm_vcpu_ioctl(env, KVM_INTERRUPT, &intr);
+                if (ret < 0) {
+                    fprintf(stderr,
+                            "KVM: injection failed, interrupt lost (%s)\n",
+                            strerror(-ret));
+                }
             }
         }
 
-- 
1.7.1

[Qemu-devel] [PATCH 11/15] kvm: Remove unneeded memory slot reservation

[Jan Kiszka]

The number of slots and the location of private ones changed several
times in KVM's early days. However, it's stable since 2.6.29 (our
required baseline), and slots 8..11 are no longer reserved since then.
So remove this unneeded restriction.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
CC: Alex Williamson <alex.williamson@redhat.com>
---
 kvm-all.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 802c6b8..14b6c1e 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -91,10 +91,6 @@ static KVMSlot *kvm_alloc_slot(KVMState *s)
     int i;
 
     for (i = 0; i < ARRAY_SIZE(s->slots); i++) {
-        /* KVM private memory slots */
-        if (i >= 8 && i < 12) {
-            continue;
-        }
         if (s->slots[i].memory_size == 0) {
             return &s->slots[i];
         }
-- 
1.7.1

[Qemu-devel] Re: [PATCH 11/15] kvm: Remove unneeded memory slot reservation

[Alex Williamson]

On Mon, 2011-02-07 at 12:19 +0100, Jan Kiszka wrote:
> The number of slots and the location of private ones changed several
> times in KVM's early days. However, it's stable since 2.6.29 (our
> required baseline), and slots 8..11 are no longer reserved since then.
> So remove this unneeded restriction.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> CC: Alex Williamson <alex.williamson@redhat.com>
> ---
>  kvm-all.c |    4 ----
>  1 files changed, 0 insertions(+), 4 deletions(-)

Acked-by: Alex Williamson <alex.williamson@redhat.com>

> diff --git a/kvm-all.c b/kvm-all.c
> index 802c6b8..14b6c1e 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -91,10 +91,6 @@ static KVMSlot *kvm_alloc_slot(KVMState *s)
>      int i;
>  
>      for (i = 0; i < ARRAY_SIZE(s->slots); i++) {
> -        /* KVM private memory slots */
> -        if (i >= 8 && i < 12) {
> -            continue;
> -        }
>          if (s->slots[i].memory_size == 0) {
>              return &s->slots[i];
>          }

[Qemu-devel] [PATCH 12/15] Introduce log_start/log_stop in CPUPhysMemoryClient

[Jan Kiszka]

From: Anthony PERARD <anthony.perard@citrix.com>

In order to use log_start/log_stop with Xen as well in the vga code,
this two operations have been put in CPUPhysMemoryClient.

The two new functions cpu_physical_log_start,cpu_physical_log_stop are
used in hw/vga.c and replace the kvm_log_start/stop. With this, vga does
no longer depends on kvm header.

[ Jan: rebasing and style fixlets ]

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 cpu-all.h    |    6 ++++++
 cpu-common.h |    4 ++++
 exec.c       |   30 ++++++++++++++++++++++++++++++
 hw/vga.c     |   31 ++++++++++++++++---------------
 hw/vhost.c   |    2 ++
 kvm-all.c    |    8 ++++++--
 kvm-stub.c   |   10 ----------
 kvm.h        |    3 ---
 8 files changed, 64 insertions(+), 30 deletions(-)

diff --git a/cpu-all.h b/cpu-all.h
index ffbd6a4..87b0f86 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -959,6 +959,12 @@ int cpu_physical_memory_get_dirty_tracking(void);
 int cpu_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
                                    target_phys_addr_t end_addr);
 
+int cpu_physical_log_start(target_phys_addr_t start_addr,
+                           ram_addr_t size);
+
+int cpu_physical_log_stop(target_phys_addr_t start_addr,
+                          ram_addr_t size);
+
 void dump_exec_info(FILE *f, fprintf_function cpu_fprintf);
 #endif /* !CONFIG_USER_ONLY */
 
diff --git a/cpu-common.h b/cpu-common.h
index 6d4a898..54d21d4 100644
--- a/cpu-common.h
+++ b/cpu-common.h
@@ -96,6 +96,10 @@ struct CPUPhysMemoryClient {
                              target_phys_addr_t end_addr);
     int (*migration_log)(struct CPUPhysMemoryClient *client,
                          int enable);
+    int (*log_start)(struct CPUPhysMemoryClient *client,
+                     target_phys_addr_t phys_addr, ram_addr_t size);
+    int (*log_stop)(struct CPUPhysMemoryClient *client,
+                    target_phys_addr_t phys_addr, ram_addr_t size);
     QLIST_ENTRY(CPUPhysMemoryClient) list;
 };
 
diff --git a/exec.c b/exec.c
index e950df2..72b25a4 100644
--- a/exec.c
+++ b/exec.c
@@ -2078,6 +2078,36 @@ int cpu_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
     return ret;
 }
 
+int cpu_physical_log_start(target_phys_addr_t start_addr,
+                           ram_addr_t size)
+{
+    CPUPhysMemoryClient *client;
+    QLIST_FOREACH(client, &memory_client_list, list) {
+        if (client->log_start) {
+            int r = client->log_start(client, start_addr, size);
+            if (r < 0) {
+                return r;
+            }
+        }
+    }
+    return 0;
+}
+
+int cpu_physical_log_stop(target_phys_addr_t start_addr,
+                          ram_addr_t size)
+{
+    CPUPhysMemoryClient *client;
+    QLIST_FOREACH(client, &memory_client_list, list) {
+        if (client->log_stop) {
+            int r = client->log_stop(client, start_addr, size);
+            if (r < 0) {
+                return r;
+            }
+        }
+    }
+    return 0;
+}
+
 static inline void tlb_update_dirty(CPUTLBEntry *tlb_entry)
 {
     ram_addr_t ram_addr;
diff --git a/hw/vga.c b/hw/vga.c
index e2151a2..c22b8af 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -28,7 +28,6 @@
 #include "vga_int.h"
 #include "pixel_ops.h"
 #include "qemu-timer.h"
-#include "kvm.h"
 
 //#define DEBUG_VGA
 //#define DEBUG_VGA_MEM
@@ -1573,34 +1572,36 @@ static void vga_sync_dirty_bitmap(VGACommonState *s)
 
 void vga_dirty_log_start(VGACommonState *s)
 {
-    if (kvm_enabled() && s->map_addr)
-        kvm_log_start(s->map_addr, s->map_end - s->map_addr);
+    if (s->map_addr) {
+        cpu_physical_log_start(s->map_addr, s->map_end - s->map_addr);
+    }
 
-    if (kvm_enabled() && s->lfb_vram_mapped) {
-        kvm_log_start(isa_mem_base + 0xa0000, 0x8000);
-        kvm_log_start(isa_mem_base + 0xa8000, 0x8000);
+    if (s->lfb_vram_mapped) {
+        cpu_physical_log_start(isa_mem_base + 0xa0000, 0x8000);
+        cpu_physical_log_start(isa_mem_base + 0xa8000, 0x8000);
     }
 
 #ifdef CONFIG_BOCHS_VBE
-    if (kvm_enabled() && s->vbe_mapped) {
-        kvm_log_start(VBE_DISPI_LFB_PHYSICAL_ADDRESS, s->vram_size);
+    if (s->vbe_mapped) {
+        cpu_physical_log_start(VBE_DISPI_LFB_PHYSICAL_ADDRESS, s->vram_size);
     }
 #endif
 }
 
 void vga_dirty_log_stop(VGACommonState *s)
 {
-    if (kvm_enabled() && s->map_addr)
-	kvm_log_stop(s->map_addr, s->map_end - s->map_addr);
+    if (s->map_addr) {
+        cpu_physical_log_stop(s->map_addr, s->map_end - s->map_addr);
+    }
 
-    if (kvm_enabled() && s->lfb_vram_mapped) {
-	kvm_log_stop(isa_mem_base + 0xa0000, 0x8000);
-	kvm_log_stop(isa_mem_base + 0xa8000, 0x8000);
+    if (s->lfb_vram_mapped) {
+        cpu_physical_log_stop(isa_mem_base + 0xa0000, 0x8000);
+        cpu_physical_log_stop(isa_mem_base + 0xa8000, 0x8000);
     }
 
 #ifdef CONFIG_BOCHS_VBE
-    if (kvm_enabled() && s->vbe_mapped) {
-	kvm_log_stop(VBE_DISPI_LFB_PHYSICAL_ADDRESS, s->vram_size);
+    if (s->vbe_mapped) {
+        cpu_physical_log_stop(VBE_DISPI_LFB_PHYSICAL_ADDRESS, s->vram_size);
     }
 #endif
 }
diff --git a/hw/vhost.c b/hw/vhost.c
index 38cc3b3..0ca3507 100644
--- a/hw/vhost.c
+++ b/hw/vhost.c
@@ -607,6 +607,8 @@ int vhost_dev_init(struct vhost_dev *hdev, int devfd, bool force)
     hdev->client.set_memory = vhost_client_set_memory;
     hdev->client.sync_dirty_bitmap = vhost_client_sync_dirty_bitmap;
     hdev->client.migration_log = vhost_client_migration_log;
+    hdev->client.log_start = NULL;
+    hdev->client.log_stop = NULL;
     hdev->mem = qemu_mallocz(offsetof(struct vhost_memory, regions));
     hdev->log = NULL;
     hdev->log_size = 0;
diff --git a/kvm-all.c b/kvm-all.c
index 14b6c1e..ecac0b3 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -274,13 +274,15 @@ static int kvm_dirty_pages_log_change(target_phys_addr_t phys_addr,
     return kvm_set_user_memory_region(s, mem);
 }
 
-int kvm_log_start(target_phys_addr_t phys_addr, ram_addr_t size)
+static int kvm_log_start(CPUPhysMemoryClient *client,
+                         target_phys_addr_t phys_addr, ram_addr_t size)
 {
     return kvm_dirty_pages_log_change(phys_addr, size, KVM_MEM_LOG_DIRTY_PAGES,
                                       KVM_MEM_LOG_DIRTY_PAGES);
 }
 
-int kvm_log_stop(target_phys_addr_t phys_addr, ram_addr_t size)
+static int kvm_log_stop(CPUPhysMemoryClient *client,
+                        target_phys_addr_t phys_addr, ram_addr_t size)
 {
     return kvm_dirty_pages_log_change(phys_addr, size, 0,
                                       KVM_MEM_LOG_DIRTY_PAGES);
@@ -644,6 +646,8 @@ static CPUPhysMemoryClient kvm_cpu_phys_memory_client = {
     .set_memory = kvm_client_set_memory,
     .sync_dirty_bitmap = kvm_client_sync_dirty_bitmap,
     .migration_log = kvm_client_migration_log,
+    .log_start = kvm_log_start,
+    .log_stop = kvm_log_stop,
 };
 
 int kvm_init(void)
diff --git a/kvm-stub.c b/kvm-stub.c
index d6b6c8e..30f6ec3 100644
--- a/kvm-stub.c
+++ b/kvm-stub.c
@@ -33,16 +33,6 @@ int kvm_init_vcpu(CPUState *env)
     return -ENOSYS;
 }
 
-int kvm_log_start(target_phys_addr_t phys_addr, ram_addr_t size)
-{
-    return -ENOSYS;
-}
-
-int kvm_log_stop(target_phys_addr_t phys_addr, ram_addr_t size)
-{
-    return -ENOSYS;
-}
-
 int kvm_coalesce_mmio_region(target_phys_addr_t start, ram_addr_t size)
 {
     return -ENOSYS;
diff --git a/kvm.h b/kvm.h
index 7a4d550..4caa6ec 100644
--- a/kvm.h
+++ b/kvm.h
@@ -58,9 +58,6 @@ int kvm_init_vcpu(CPUState *env);
 int kvm_cpu_exec(CPUState *env);
 
 #if !defined(CONFIG_USER_ONLY)
-int kvm_log_start(target_phys_addr_t phys_addr, ram_addr_t size);
-int kvm_log_stop(target_phys_addr_t phys_addr, ram_addr_t size);
-
 void kvm_setup_guest_memory(void *start, size_t size);
 
 int kvm_coalesce_mmio_region(target_phys_addr_t start, ram_addr_t size);
-- 
1.7.1

[Qemu-devel] [PATCH 13/15] cirrus: Remove obsolete kvm.h include

[Jan Kiszka]

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/cirrus_vga.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 5f45b5d..2724f7b 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -31,7 +31,6 @@
 #include "pci.h"
 #include "console.h"
 #include "vga_int.h"
-#include "kvm.h"
 #include "loader.h"
 
 /*
-- 
1.7.1

[Qemu-devel] [PATCH 14/15] kvm: Make kvm_state globally available

[Jan Kiszka]

KVM-assisted devices need access to it but we have no clean channel to
distribute a reference. As a workaround until there is a better
solution, export kvm_state for global use, though use should remain
restricted to the mentioned scenario.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 kvm-all.c |    2 +-
 kvm.h     |    1 +
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index ecac0b3..e6a7de4 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -78,7 +78,7 @@ struct KVMState
     int many_ioeventfds;
 };
 
-static KVMState *kvm_state;
+KVMState *kvm_state;
 
 static const KVMCapabilityInfo kvm_required_capabilites[] = {
     KVM_CAP_INFO(USER_MEMORY),
diff --git a/kvm.h b/kvm.h
index 4caa6ec..59b2c29 100644
--- a/kvm.h
+++ b/kvm.h
@@ -85,6 +85,7 @@ int kvm_on_sigbus(int code, void *addr);
 
 struct KVMState;
 typedef struct KVMState KVMState;
+extern KVMState *kvm_state;
 
 int kvm_ioctl(KVMState *s, int type, ...);
 
-- 
1.7.1

[Qemu-devel] [PATCH 15/15] kvm: x86: Introduce kvmclock device to save/restore its state

[Jan Kiszka]

If kvmclock is used, which implies the kernel supports it, register a
kvmclock device with the sysbus. Its main purpose is to save and restore
the kernel state on migration, but this will also allow to visualize it
one day.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
CC: Glauber Costa <glommer@redhat.com>
---
 Makefile.target |    4 +-
 hw/kvmclock.c   |  125 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/kvmclock.h   |   14 ++++++
 hw/pc_piix.c    |   31 +++++++++++---
 4 files changed, 165 insertions(+), 9 deletions(-)
 create mode 100644 hw/kvmclock.c
 create mode 100644 hw/kvmclock.h

diff --git a/Makefile.target b/Makefile.target
index b0ba95f..30232fa 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -37,7 +37,7 @@ ifndef CONFIG_HAIKU
 LIBS+=-lm
 endif
 
-kvm.o kvm-all.o vhost.o vhost_net.o: QEMU_CFLAGS+=$(KVM_CFLAGS)
+kvm.o kvm-all.o vhost.o vhost_net.o kvmclock.o: QEMU_CFLAGS+=$(KVM_CFLAGS)
 
 config-target.h: config-target.h-timestamp
 config-target.h-timestamp: config-target.mak
@@ -218,7 +218,7 @@ obj-i386-y += cirrus_vga.o apic.o ioapic.o piix_pci.o
 obj-i386-y += vmmouse.o vmport.o hpet.o applesmc.o
 obj-i386-y += device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.o
 obj-i386-y += debugcon.o multiboot.o
-obj-i386-y += pc_piix.o
+obj-i386-y += pc_piix.o kvmclock.o
 obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
 
 # shared objects
diff --git a/hw/kvmclock.c b/hw/kvmclock.c
new file mode 100644
index 0000000..b6ceddf
--- /dev/null
+++ b/hw/kvmclock.c
@@ -0,0 +1,125 @@
+/*
+ * QEMU KVM support, paravirtual clock device
+ *
+ * Copyright (C) 2011 Siemens AG
+ *
+ * Authors:
+ *  Jan Kiszka        <jan.kiszka@siemens.com>
+ *
+ * This work is licensed under the terms of the GNU GPL version 2.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu-common.h"
+#include "sysemu.h"
+#include "sysbus.h"
+#include "kvm.h"
+#include "kvmclock.h"
+
+#if defined(CONFIG_KVM_PARA) && defined(KVM_CAP_ADJUST_CLOCK)
+
+#include <linux/kvm.h>
+#include <linux/kvm_para.h>
+
+typedef struct KVMClockState {
+    SysBusDevice busdev;
+    uint64_t clock;
+    bool clock_valid;
+} KVMClockState;
+
+static void kvmclock_pre_save(void *opaque)
+{
+    KVMClockState *s = opaque;
+    struct kvm_clock_data data;
+    int ret;
+
+    if (s->clock_valid) {
+        return;
+    }
+    ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
+    if (ret < 0) {
+        fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
+        data.clock = 0;
+    }
+    s->clock = data.clock;
+    /*
+     * If the VM is stopped, declare the clock state valid to avoid re-reading
+     * it on next vmsave (which would return a different value). Will be reset
+     * when the VM is continued.
+     */
+    s->clock_valid = !vm_running;
+}
+
+static int kvmclock_post_load(void *opaque, int version_id)
+{
+    KVMClockState *s = opaque;
+    struct kvm_clock_data data;
+
+    data.clock = s->clock;
+    data.flags = 0;
+    return kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
+}
+
+static void kvmclock_vm_state_change(void *opaque, int running, int reason)
+{
+    KVMClockState *s = opaque;
+
+    if (running) {
+        s->clock_valid = false;
+    }
+}
+
+static int kvmclock_init(SysBusDevice *dev)
+{
+    KVMClockState *s = FROM_SYSBUS(KVMClockState, dev);
+
+    qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
+    return 0;
+}
+
+static const VMStateDescription kvmclock_vmsd = {
+    .name = "kvmclock",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .pre_save = kvmclock_pre_save,
+    .post_load = kvmclock_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(clock, KVMClockState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static SysBusDeviceInfo kvmclock_info = {
+    .qdev.name = "kvmclock",
+    .qdev.size = sizeof(KVMClockState),
+    .qdev.vmsd = &kvmclock_vmsd,
+    .qdev.no_user = 1,
+    .init = kvmclock_init,
+};
+
+/* Note: Must be called after VCPU initialization. */
+void kvmclock_create(void)
+{
+    if (kvm_enabled() &&
+        first_cpu->cpuid_kvm_features & (1ULL << KVM_FEATURE_CLOCKSOURCE)) {
+        sysbus_create_simple("kvmclock", -1, NULL);
+    }
+}
+
+static void kvmclock_register_device(void)
+{
+    if (kvm_enabled()) {
+        sysbus_register_withprop(&kvmclock_info);
+    }
+}
+
+device_init(kvmclock_register_device);
+
+#else /* !(CONFIG_KVM_PARA && KVM_CAP_ADJUST_CLOCK) */
+
+void kvmclock_create(void)
+{
+}
+#endif /* !(CONFIG_KVM_PARA && KVM_CAP_ADJUST_CLOCK) */
diff --git a/hw/kvmclock.h b/hw/kvmclock.h
new file mode 100644
index 0000000..7a83cbe
--- /dev/null
+++ b/hw/kvmclock.h
@@ -0,0 +1,14 @@
+/*
+ * QEMU KVM support, paravirtual clock device
+ *
+ * Copyright (C) 2011 Siemens AG
+ *
+ * Authors:
+ *  Jan Kiszka        <jan.kiszka@siemens.com>
+ *
+ * This work is licensed under the terms of the GNU GPL version 2.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+void kvmclock_create(void);
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 7b74473..9bc4659 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -32,6 +32,7 @@
 #include "boards.h"
 #include "ide.h"
 #include "kvm.h"
+#include "kvmclock.h"
 #include "sysemu.h"
 #include "sysbus.h"
 #include "arch_init.h"
@@ -66,7 +67,8 @@ static void pc_init1(ram_addr_t ram_size,
                      const char *kernel_cmdline,
                      const char *initrd_filename,
                      const char *cpu_model,
-                     int pci_enabled)
+                     int pci_enabled,
+                     int kvmclock_enabled)
 {
     int i;
     ram_addr_t below_4g_mem_size, above_4g_mem_size;
@@ -87,6 +89,9 @@ static void pc_init1(ram_addr_t ram_size,
     pc_cpus_init(cpu_model);
 
     vmport_init();
+    if (kvmclock_enabled) {
+        kvmclock_create();
+    }
 
     /* allocate ram and load rom/bios */
     pc_memory_init(ram_size, kernel_filename, kernel_cmdline, initrd_filename,
@@ -195,7 +200,19 @@ static void pc_init_pci(ram_addr_t ram_size,
 {
     pc_init1(ram_size, boot_device,
              kernel_filename, kernel_cmdline,
-             initrd_filename, cpu_model, 1);
+             initrd_filename, cpu_model, 1, 1);
+}
+
+static void pc_init_pci_no_kvmclock(ram_addr_t ram_size,
+                                    const char *boot_device,
+                                    const char *kernel_filename,
+                                    const char *kernel_cmdline,
+                                    const char *initrd_filename,
+                                    const char *cpu_model)
+{
+    pc_init1(ram_size, boot_device,
+             kernel_filename, kernel_cmdline,
+             initrd_filename, cpu_model, 1, 0);
 }
 
 static void pc_init_isa(ram_addr_t ram_size,
@@ -209,7 +226,7 @@ static void pc_init_isa(ram_addr_t ram_size,
         cpu_model = "486";
     pc_init1(ram_size, boot_device,
              kernel_filename, kernel_cmdline,
-             initrd_filename, cpu_model, 0);
+             initrd_filename, cpu_model, 0, 1);
 }
 
 static QEMUMachine pc_machine = {
@@ -224,7 +241,7 @@ static QEMUMachine pc_machine = {
 static QEMUMachine pc_machine_v0_13 = {
     .name = "pc-0.13",
     .desc = "Standard PC",
-    .init = pc_init_pci,
+    .init = pc_init_pci_no_kvmclock,
     .max_cpus = 255,
     .compat_props = (GlobalProperty[]) {
         {
@@ -251,7 +268,7 @@ static QEMUMachine pc_machine_v0_13 = {
 static QEMUMachine pc_machine_v0_12 = {
     .name = "pc-0.12",
     .desc = "Standard PC",
-    .init = pc_init_pci,
+    .init = pc_init_pci_no_kvmclock,
     .max_cpus = 255,
     .compat_props = (GlobalProperty[]) {
         {
@@ -282,7 +299,7 @@ static QEMUMachine pc_machine_v0_12 = {
 static QEMUMachine pc_machine_v0_11 = {
     .name = "pc-0.11",
     .desc = "Standard PC, qemu 0.11",
-    .init = pc_init_pci,
+    .init = pc_init_pci_no_kvmclock,
     .max_cpus = 255,
     .compat_props = (GlobalProperty[]) {
         {
@@ -321,7 +338,7 @@ static QEMUMachine pc_machine_v0_11 = {
 static QEMUMachine pc_machine_v0_10 = {
     .name = "pc-0.10",
     .desc = "Standard PC, qemu 0.10",
-    .init = pc_init_pci,
+    .init = pc_init_pci_no_kvmclock,
     .max_cpus = 255,
     .compat_props = (GlobalProperty[]) {
         {
-- 
1.7.1

[Qemu-devel] Re: [PATCH 15/15] kvm: x86: Introduce kvmclock device to save/restore its state

[Glauber Costa]

On Mon, 2011-02-07 at 12:19 +0100, Jan Kiszka wrote:
> If kvmclock is used, which implies the kernel supports it, register a
> kvmclock device with the sysbus. Its main purpose is to save and restore
> the kernel state on migration, but this will also allow to visualize it
> one day.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> CC: Glauber Costa <glommer@redhat.com>
> ---
>  Makefile.target |    4 +-
>  hw/kvmclock.c   |  125 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/kvmclock.h   |   14 ++++++
>  hw/pc_piix.c    |   31 +++++++++++---
>  4 files changed, 165 insertions(+), 9 deletions(-)
>  create mode 100644 hw/kvmclock.c
>  create mode 100644 hw/kvmclock.h
> 
> diff --git a/Makefile.target b/Makefile.target
> index b0ba95f..30232fa 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -37,7 +37,7 @@ ifndef CONFIG_HAIKU
>  LIBS+=-lm
>  endif
>  
> -kvm.o kvm-all.o vhost.o vhost_net.o: QEMU_CFLAGS+=$(KVM_CFLAGS)
> +kvm.o kvm-all.o vhost.o vhost_net.o kvmclock.o: QEMU_CFLAGS+=$(KVM_CFLAGS)
>  
>  config-target.h: config-target.h-timestamp
>  config-target.h-timestamp: config-target.mak
> @@ -218,7 +218,7 @@ obj-i386-y += cirrus_vga.o apic.o ioapic.o piix_pci.o
>  obj-i386-y += vmmouse.o vmport.o hpet.o applesmc.o
>  obj-i386-y += device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.o
>  obj-i386-y += debugcon.o multiboot.o
> -obj-i386-y += pc_piix.o
> +obj-i386-y += pc_piix.o kvmclock.o
>  obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
>  
>  # shared objects
> diff --git a/hw/kvmclock.c b/hw/kvmclock.c
> new file mode 100644
> index 0000000..b6ceddf
> --- /dev/null
> +++ b/hw/kvmclock.c
> @@ -0,0 +1,125 @@
> +/*
> + * QEMU KVM support, paravirtual clock device
> + *
> + * Copyright (C) 2011 Siemens AG
> + *
> + * Authors:
> + *  Jan Kiszka        <jan.kiszka@siemens.com>
> + *
> + * This work is licensed under the terms of the GNU GPL version 2.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "qemu-common.h"
> +#include "sysemu.h"
> +#include "sysbus.h"
> +#include "kvm.h"
> +#include "kvmclock.h"
> +
> +#if defined(CONFIG_KVM_PARA) && defined(KVM_CAP_ADJUST_CLOCK)
> +
> +#include <linux/kvm.h>
> +#include <linux/kvm_para.h>
> +
> +typedef struct KVMClockState {
> +    SysBusDevice busdev;
> +    uint64_t clock;
> +    bool clock_valid;
> +} KVMClockState;
> +
> +static void kvmclock_pre_save(void *opaque)
> +{
> +    KVMClockState *s = opaque;
> +    struct kvm_clock_data data;
> +    int ret;
> +
> +    if (s->clock_valid) {
> +        return;
> +    }
> +    ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
> +    if (ret < 0) {
> +        fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
> +        data.clock = 0;
> +    }
> +    s->clock = data.clock;
> +    /*
> +     * If the VM is stopped, declare the clock state valid to avoid re-reading
> +     * it on next vmsave (which would return a different value). Will be reset
> +     * when the VM is continued.
> +     */
> +    s->clock_valid = !vm_running;
> +}
> +
> +static int kvmclock_post_load(void *opaque, int version_id)
> +{
> +    KVMClockState *s = opaque;
> +    struct kvm_clock_data data;
> +
> +    data.clock = s->clock;
> +    data.flags = 0;
> +    return kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
> +}
> +
> +static void kvmclock_vm_state_change(void *opaque, int running, int reason)
> +{
> +    KVMClockState *s = opaque;
> +
> +    if (running) {
> +        s->clock_valid = false;
> +    }
> +}
> +
> +static int kvmclock_init(SysBusDevice *dev)
> +{
> +    KVMClockState *s = FROM_SYSBUS(KVMClockState, dev);
> +
> +    qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
> +    return 0;
> +}
> +
> +static const VMStateDescription kvmclock_vmsd = {
> +    .name = "kvmclock",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .pre_save = kvmclock_pre_save,
> +    .post_load = kvmclock_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT64(clock, KVMClockState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static SysBusDeviceInfo kvmclock_info = {
> +    .qdev.name = "kvmclock",
> +    .qdev.size = sizeof(KVMClockState),
> +    .qdev.vmsd = &kvmclock_vmsd,
> +    .qdev.no_user = 1,
> +    .init = kvmclock_init,
> +};
> +
> +/* Note: Must be called after VCPU initialization. */
> +void kvmclock_create(void)
> +{
> +    if (kvm_enabled() &&
> +        first_cpu->cpuid_kvm_features & (1ULL << KVM_FEATURE_CLOCKSOURCE)) {
> +        sysbus_create_simple("kvmclock", -1, NULL);
> +    }
> +}
> +
> +static void kvmclock_register_device(void)
> +{
> +    if (kvm_enabled()) {
> +        sysbus_register_withprop(&kvmclock_info);
> +    }
> +}
> +
> +device_init(kvmclock_register_device);
> +
> +#else /* !(CONFIG_KVM_PARA && KVM_CAP_ADJUST_CLOCK) */
> +
> +void kvmclock_create(void)
> +{
> +}
> +#endif /* !(CONFIG_KVM_PARA && KVM_CAP_ADJUST_CLOCK) */
> diff --git a/hw/kvmclock.h b/hw/kvmclock.h
> new file mode 100644
> index 0000000..7a83cbe
> --- /dev/null
> +++ b/hw/kvmclock.h
> @@ -0,0 +1,14 @@
> +/*
> + * QEMU KVM support, paravirtual clock device
> + *
> + * Copyright (C) 2011 Siemens AG
> + *
> + * Authors:
> + *  Jan Kiszka        <jan.kiszka@siemens.com>
> + *
> + * This work is licensed under the terms of the GNU GPL version 2.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +void kvmclock_create(void);
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 7b74473..9bc4659 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -32,6 +32,7 @@
>  #include "boards.h"
>  #include "ide.h"
>  #include "kvm.h"
> +#include "kvmclock.h"
>  #include "sysemu.h"
>  #include "sysbus.h"
>  #include "arch_init.h"
> @@ -66,7 +67,8 @@ static void pc_init1(ram_addr_t ram_size,
>                       const char *kernel_cmdline,
>                       const char *initrd_filename,
>                       const char *cpu_model,
> -                     int pci_enabled)
> +                     int pci_enabled,
> +                     int kvmclock_enabled)
>  
What exactly is your motivation to that ? I think mid/long-term
we should be making machine initialization more common among
architectures, not introducing more arch specific, or even worse, kvm
specific parameters here.

I'd like to understand what do we gain from that, since opting kvmclock
in our out is done by cpuid anyway - no need for a specific machine.

[Qemu-devel] Re: [PATCH 15/15] kvm: x86: Introduce kvmclock device to save/restore its state

[Jan Kiszka]

On 2011-02-07 13:27, Glauber Costa wrote:
> On Mon, 2011-02-07 at 12:19 +0100, Jan Kiszka wrote:
>> If kvmclock is used, which implies the kernel supports it, register a
>> kvmclock device with the sysbus. Its main purpose is to save and restore
>> the kernel state on migration, but this will also allow to visualize it
>> one day.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> CC: Glauber Costa <glommer@redhat.com>
>> ---
>>  Makefile.target |    4 +-
>>  hw/kvmclock.c   |  125 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  hw/kvmclock.h   |   14 ++++++
>>  hw/pc_piix.c    |   31 +++++++++++---
>>  4 files changed, 165 insertions(+), 9 deletions(-)
>>  create mode 100644 hw/kvmclock.c
>>  create mode 100644 hw/kvmclock.h
>>
>> diff --git a/Makefile.target b/Makefile.target
>> index b0ba95f..30232fa 100644
>> --- a/Makefile.target
>> +++ b/Makefile.target
>> @@ -37,7 +37,7 @@ ifndef CONFIG_HAIKU
>>  LIBS+=-lm
>>  endif
>>  
>> -kvm.o kvm-all.o vhost.o vhost_net.o: QEMU_CFLAGS+=$(KVM_CFLAGS)
>> +kvm.o kvm-all.o vhost.o vhost_net.o kvmclock.o: QEMU_CFLAGS+=$(KVM_CFLAGS)
>>  
>>  config-target.h: config-target.h-timestamp
>>  config-target.h-timestamp: config-target.mak
>> @@ -218,7 +218,7 @@ obj-i386-y += cirrus_vga.o apic.o ioapic.o piix_pci.o
>>  obj-i386-y += vmmouse.o vmport.o hpet.o applesmc.o
>>  obj-i386-y += device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.o
>>  obj-i386-y += debugcon.o multiboot.o
>> -obj-i386-y += pc_piix.o
>> +obj-i386-y += pc_piix.o kvmclock.o
>>  obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
>>  
>>  # shared objects
>> diff --git a/hw/kvmclock.c b/hw/kvmclock.c
>> new file mode 100644
>> index 0000000..b6ceddf
>> --- /dev/null
>> +++ b/hw/kvmclock.c
>> @@ -0,0 +1,125 @@
>> +/*
>> + * QEMU KVM support, paravirtual clock device
>> + *
>> + * Copyright (C) 2011 Siemens AG
>> + *
>> + * Authors:
>> + *  Jan Kiszka        <jan.kiszka@siemens.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL version 2.
>> + * See the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#include "qemu-common.h"
>> +#include "sysemu.h"
>> +#include "sysbus.h"
>> +#include "kvm.h"
>> +#include "kvmclock.h"
>> +
>> +#if defined(CONFIG_KVM_PARA) && defined(KVM_CAP_ADJUST_CLOCK)
>> +
>> +#include <linux/kvm.h>
>> +#include <linux/kvm_para.h>
>> +
>> +typedef struct KVMClockState {
>> +    SysBusDevice busdev;
>> +    uint64_t clock;
>> +    bool clock_valid;
>> +} KVMClockState;
>> +
>> +static void kvmclock_pre_save(void *opaque)
>> +{
>> +    KVMClockState *s = opaque;
>> +    struct kvm_clock_data data;
>> +    int ret;
>> +
>> +    if (s->clock_valid) {
>> +        return;
>> +    }
>> +    ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
>> +    if (ret < 0) {
>> +        fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
>> +        data.clock = 0;
>> +    }
>> +    s->clock = data.clock;
>> +    /*
>> +     * If the VM is stopped, declare the clock state valid to avoid re-reading
>> +     * it on next vmsave (which would return a different value). Will be reset
>> +     * when the VM is continued.
>> +     */
>> +    s->clock_valid = !vm_running;
>> +}
>> +
>> +static int kvmclock_post_load(void *opaque, int version_id)
>> +{
>> +    KVMClockState *s = opaque;
>> +    struct kvm_clock_data data;
>> +
>> +    data.clock = s->clock;
>> +    data.flags = 0;
>> +    return kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
>> +}
>> +
>> +static void kvmclock_vm_state_change(void *opaque, int running, int reason)
>> +{
>> +    KVMClockState *s = opaque;
>> +
>> +    if (running) {
>> +        s->clock_valid = false;
>> +    }
>> +}
>> +
>> +static int kvmclock_init(SysBusDevice *dev)
>> +{
>> +    KVMClockState *s = FROM_SYSBUS(KVMClockState, dev);
>> +
>> +    qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
>> +    return 0;
>> +}
>> +
>> +static const VMStateDescription kvmclock_vmsd = {
>> +    .name = "kvmclock",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .minimum_version_id_old = 1,
>> +    .pre_save = kvmclock_pre_save,
>> +    .post_load = kvmclock_post_load,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT64(clock, KVMClockState),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +static SysBusDeviceInfo kvmclock_info = {
>> +    .qdev.name = "kvmclock",
>> +    .qdev.size = sizeof(KVMClockState),
>> +    .qdev.vmsd = &kvmclock_vmsd,
>> +    .qdev.no_user = 1,
>> +    .init = kvmclock_init,
>> +};
>> +
>> +/* Note: Must be called after VCPU initialization. */
>> +void kvmclock_create(void)
>> +{
>> +    if (kvm_enabled() &&
>> +        first_cpu->cpuid_kvm_features & (1ULL << KVM_FEATURE_CLOCKSOURCE)) {
>> +        sysbus_create_simple("kvmclock", -1, NULL);
>> +    }
>> +}
>> +
>> +static void kvmclock_register_device(void)
>> +{
>> +    if (kvm_enabled()) {
>> +        sysbus_register_withprop(&kvmclock_info);
>> +    }
>> +}
>> +
>> +device_init(kvmclock_register_device);
>> +
>> +#else /* !(CONFIG_KVM_PARA && KVM_CAP_ADJUST_CLOCK) */
>> +
>> +void kvmclock_create(void)
>> +{
>> +}
>> +#endif /* !(CONFIG_KVM_PARA && KVM_CAP_ADJUST_CLOCK) */
>> diff --git a/hw/kvmclock.h b/hw/kvmclock.h
>> new file mode 100644
>> index 0000000..7a83cbe
>> --- /dev/null
>> +++ b/hw/kvmclock.h
>> @@ -0,0 +1,14 @@
>> +/*
>> + * QEMU KVM support, paravirtual clock device
>> + *
>> + * Copyright (C) 2011 Siemens AG
>> + *
>> + * Authors:
>> + *  Jan Kiszka        <jan.kiszka@siemens.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL version 2.
>> + * See the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +void kvmclock_create(void);
>> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
>> index 7b74473..9bc4659 100644
>> --- a/hw/pc_piix.c
>> +++ b/hw/pc_piix.c
>> @@ -32,6 +32,7 @@
>>  #include "boards.h"
>>  #include "ide.h"
>>  #include "kvm.h"
>> +#include "kvmclock.h"
>>  #include "sysemu.h"
>>  #include "sysbus.h"
>>  #include "arch_init.h"
>> @@ -66,7 +67,8 @@ static void pc_init1(ram_addr_t ram_size,
>>                       const char *kernel_cmdline,
>>                       const char *initrd_filename,
>>                       const char *cpu_model,
>> -                     int pci_enabled)
>> +                     int pci_enabled,
>> +                     int kvmclock_enabled)
>>  
> What exactly is your motivation to that ? I think mid/long-term
> we should be making machine initialization more common among
> architectures, not introducing more arch specific, or even worse, kvm
> specific parameters here.
> 
> I'd like to understand what do we gain from that, since opting kvmclock
> in our out is done by cpuid anyway - no need for a specific machine.

Is that really the case? I thought we were already shipping versions
where that CPU feature was enabled by default. If not, I'll happily drop
that admittedly clumsy approach above.

Jan

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

[Qemu-devel] Re: [PATCH 15/15] kvm: x86: Introduce kvmclock device to save/restore its state

[Glauber Costa]

On Mon, 2011-02-07 at 13:36 +0100, Jan Kiszka wrote:
> On 2011-02-07 13:27, Glauber Costa wrote:
> > On Mon, 2011-02-07 at 12:19 +0100, Jan Kiszka wrote:
> >> If kvmclock is used, which implies the kernel supports it, register a
> >> kvmclock device with the sysbus. Its main purpose is to save and restore
> >> the kernel state on migration, but this will also allow to visualize it
> >> one day.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> CC: Glauber Costa <glommer@redhat.com>
> >> ---
> >>  Makefile.target |    4 +-
> >>  hw/kvmclock.c   |  125 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  hw/kvmclock.h   |   14 ++++++
> >>  hw/pc_piix.c    |   31 +++++++++++---
> >>  4 files changed, 165 insertions(+), 9 deletions(-)
> >>  create mode 100644 hw/kvmclock.c
> >>  create mode 100644 hw/kvmclock.h
> >>
> >> diff --git a/Makefile.target b/Makefile.target
> >> index b0ba95f..30232fa 100644
> >> --- a/Makefile.target
> >> +++ b/Makefile.target
> >> @@ -37,7 +37,7 @@ ifndef CONFIG_HAIKU
> >>  LIBS+=-lm
> >>  endif
> >>  
> >> -kvm.o kvm-all.o vhost.o vhost_net.o: QEMU_CFLAGS+=$(KVM_CFLAGS)
> >> +kvm.o kvm-all.o vhost.o vhost_net.o kvmclock.o: QEMU_CFLAGS+=$(KVM_CFLAGS)
> >>  
> >>  config-target.h: config-target.h-timestamp
> >>  config-target.h-timestamp: config-target.mak
> >> @@ -218,7 +218,7 @@ obj-i386-y += cirrus_vga.o apic.o ioapic.o piix_pci.o
> >>  obj-i386-y += vmmouse.o vmport.o hpet.o applesmc.o
> >>  obj-i386-y += device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.o
> >>  obj-i386-y += debugcon.o multiboot.o
> >> -obj-i386-y += pc_piix.o
> >> +obj-i386-y += pc_piix.o kvmclock.o
> >>  obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
> >>  
> >>  # shared objects
> >> diff --git a/hw/kvmclock.c b/hw/kvmclock.c
> >> new file mode 100644
> >> index 0000000..b6ceddf
> >> --- /dev/null
> >> +++ b/hw/kvmclock.c
> >> @@ -0,0 +1,125 @@
> >> +/*
> >> + * QEMU KVM support, paravirtual clock device
> >> + *
> >> + * Copyright (C) 2011 Siemens AG
> >> + *
> >> + * Authors:
> >> + *  Jan Kiszka        <jan.kiszka@siemens.com>
> >> + *
> >> + * This work is licensed under the terms of the GNU GPL version 2.
> >> + * See the COPYING file in the top-level directory.
> >> + *
> >> + */
> >> +
> >> +#include "qemu-common.h"
> >> +#include "sysemu.h"
> >> +#include "sysbus.h"
> >> +#include "kvm.h"
> >> +#include "kvmclock.h"
> >> +
> >> +#if defined(CONFIG_KVM_PARA) && defined(KVM_CAP_ADJUST_CLOCK)
> >> +
> >> +#include <linux/kvm.h>
> >> +#include <linux/kvm_para.h>
> >> +
> >> +typedef struct KVMClockState {
> >> +    SysBusDevice busdev;
> >> +    uint64_t clock;
> >> +    bool clock_valid;
> >> +} KVMClockState;
> >> +
> >> +static void kvmclock_pre_save(void *opaque)
> >> +{
> >> +    KVMClockState *s = opaque;
> >> +    struct kvm_clock_data data;
> >> +    int ret;
> >> +
> >> +    if (s->clock_valid) {
> >> +        return;
> >> +    }
> >> +    ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
> >> +    if (ret < 0) {
> >> +        fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
> >> +        data.clock = 0;
> >> +    }
> >> +    s->clock = data.clock;
> >> +    /*
> >> +     * If the VM is stopped, declare the clock state valid to avoid re-reading
> >> +     * it on next vmsave (which would return a different value). Will be reset
> >> +     * when the VM is continued.
> >> +     */
> >> +    s->clock_valid = !vm_running;
> >> +}
> >> +
> >> +static int kvmclock_post_load(void *opaque, int version_id)
> >> +{
> >> +    KVMClockState *s = opaque;
> >> +    struct kvm_clock_data data;
> >> +
> >> +    data.clock = s->clock;
> >> +    data.flags = 0;
> >> +    return kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
> >> +}
> >> +
> >> +static void kvmclock_vm_state_change(void *opaque, int running, int reason)
> >> +{
> >> +    KVMClockState *s = opaque;
> >> +
> >> +    if (running) {
> >> +        s->clock_valid = false;
> >> +    }
> >> +}
> >> +
> >> +static int kvmclock_init(SysBusDevice *dev)
> >> +{
> >> +    KVMClockState *s = FROM_SYSBUS(KVMClockState, dev);
> >> +
> >> +    qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
> >> +    return 0;
> >> +}
> >> +
> >> +static const VMStateDescription kvmclock_vmsd = {
> >> +    .name = "kvmclock",
> >> +    .version_id = 1,
> >> +    .minimum_version_id = 1,
> >> +    .minimum_version_id_old = 1,
> >> +    .pre_save = kvmclock_pre_save,
> >> +    .post_load = kvmclock_post_load,
> >> +    .fields = (VMStateField[]) {
> >> +        VMSTATE_UINT64(clock, KVMClockState),
> >> +        VMSTATE_END_OF_LIST()
> >> +    }
> >> +};
> >> +
> >> +static SysBusDeviceInfo kvmclock_info = {
> >> +    .qdev.name = "kvmclock",
> >> +    .qdev.size = sizeof(KVMClockState),
> >> +    .qdev.vmsd = &kvmclock_vmsd,
> >> +    .qdev.no_user = 1,
> >> +    .init = kvmclock_init,
> >> +};
> >> +
> >> +/* Note: Must be called after VCPU initialization. */
> >> +void kvmclock_create(void)
> >> +{
> >> +    if (kvm_enabled() &&
> >> +        first_cpu->cpuid_kvm_features & (1ULL << KVM_FEATURE_CLOCKSOURCE)) {
> >> +        sysbus_create_simple("kvmclock", -1, NULL);
> >> +    }
> >> +}
> >> +
> >> +static void kvmclock_register_device(void)
> >> +{
> >> +    if (kvm_enabled()) {
> >> +        sysbus_register_withprop(&kvmclock_info);
> >> +    }
> >> +}
> >> +
> >> +device_init(kvmclock_register_device);
> >> +
> >> +#else /* !(CONFIG_KVM_PARA && KVM_CAP_ADJUST_CLOCK) */
> >> +
> >> +void kvmclock_create(void)
> >> +{
> >> +}
> >> +#endif /* !(CONFIG_KVM_PARA && KVM_CAP_ADJUST_CLOCK) */
> >> diff --git a/hw/kvmclock.h b/hw/kvmclock.h
> >> new file mode 100644
> >> index 0000000..7a83cbe
> >> --- /dev/null
> >> +++ b/hw/kvmclock.h
> >> @@ -0,0 +1,14 @@
> >> +/*
> >> + * QEMU KVM support, paravirtual clock device
> >> + *
> >> + * Copyright (C) 2011 Siemens AG
> >> + *
> >> + * Authors:
> >> + *  Jan Kiszka        <jan.kiszka@siemens.com>
> >> + *
> >> + * This work is licensed under the terms of the GNU GPL version 2.
> >> + * See the COPYING file in the top-level directory.
> >> + *
> >> + */
> >> +
> >> +void kvmclock_create(void);
> >> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> >> index 7b74473..9bc4659 100644
> >> --- a/hw/pc_piix.c
> >> +++ b/hw/pc_piix.c
> >> @@ -32,6 +32,7 @@
> >>  #include "boards.h"
> >>  #include "ide.h"
> >>  #include "kvm.h"
> >> +#include "kvmclock.h"
> >>  #include "sysemu.h"
> >>  #include "sysbus.h"
> >>  #include "arch_init.h"
> >> @@ -66,7 +67,8 @@ static void pc_init1(ram_addr_t ram_size,
> >>                       const char *kernel_cmdline,
> >>                       const char *initrd_filename,
> >>                       const char *cpu_model,
> >> -                     int pci_enabled)
> >> +                     int pci_enabled,
> >> +                     int kvmclock_enabled)
> >>  
> > What exactly is your motivation to that ? I think mid/long-term
> > we should be making machine initialization more common among
> > architectures, not introducing more arch specific, or even worse, kvm
> > specific parameters here.
> > 
> > I'd like to understand what do we gain from that, since opting kvmclock
> > in our out is done by cpuid anyway - no need for a specific machine.
> 
> Is that really the case? I thought we were already shipping versions
> where that CPU feature was enabled by default. If not, I'll happily drop
> that admittedly clumsy approach above.

Yes, AFAIK, kvmclock is enabled by default, disabled by cpuid-leaf, as
in
-cpu kvm64,-kvmclock

So your test for cpuid bit before starting kvmclock should already cover
it.

[Qemu-devel] Re: [PATCH 15/15] kvm: x86: Introduce kvmclock device to save/restore its state

[Jan Kiszka]

On 2011-02-07 14:40, Glauber Costa wrote:
> On Mon, 2011-02-07 at 13:36 +0100, Jan Kiszka wrote:
>> On 2011-02-07 13:27, Glauber Costa wrote:
>>> On Mon, 2011-02-07 at 12:19 +0100, Jan Kiszka wrote:
>>>> If kvmclock is used, which implies the kernel supports it, register a
>>>> kvmclock device with the sysbus. Its main purpose is to save and restore
>>>> the kernel state on migration, but this will also allow to visualize it
>>>> one day.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> CC: Glauber Costa <glommer@redhat.com>
>>>> ---
>>>>  Makefile.target |    4 +-
>>>>  hw/kvmclock.c   |  125 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  hw/kvmclock.h   |   14 ++++++
>>>>  hw/pc_piix.c    |   31 +++++++++++---
>>>>  4 files changed, 165 insertions(+), 9 deletions(-)
>>>>  create mode 100644 hw/kvmclock.c
>>>>  create mode 100644 hw/kvmclock.h
>>>>
>>>> diff --git a/Makefile.target b/Makefile.target
>>>> index b0ba95f..30232fa 100644
>>>> --- a/Makefile.target
>>>> +++ b/Makefile.target
>>>> @@ -37,7 +37,7 @@ ifndef CONFIG_HAIKU
>>>>  LIBS+=-lm
>>>>  endif
>>>>  
>>>> -kvm.o kvm-all.o vhost.o vhost_net.o: QEMU_CFLAGS+=$(KVM_CFLAGS)
>>>> +kvm.o kvm-all.o vhost.o vhost_net.o kvmclock.o: QEMU_CFLAGS+=$(KVM_CFLAGS)
>>>>  
>>>>  config-target.h: config-target.h-timestamp
>>>>  config-target.h-timestamp: config-target.mak
>>>> @@ -218,7 +218,7 @@ obj-i386-y += cirrus_vga.o apic.o ioapic.o piix_pci.o
>>>>  obj-i386-y += vmmouse.o vmport.o hpet.o applesmc.o
>>>>  obj-i386-y += device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.o
>>>>  obj-i386-y += debugcon.o multiboot.o
>>>> -obj-i386-y += pc_piix.o
>>>> +obj-i386-y += pc_piix.o kvmclock.o
>>>>  obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
>>>>  
>>>>  # shared objects
>>>> diff --git a/hw/kvmclock.c b/hw/kvmclock.c
>>>> new file mode 100644
>>>> index 0000000..b6ceddf
>>>> --- /dev/null
>>>> +++ b/hw/kvmclock.c
>>>> @@ -0,0 +1,125 @@
>>>> +/*
>>>> + * QEMU KVM support, paravirtual clock device
>>>> + *
>>>> + * Copyright (C) 2011 Siemens AG
>>>> + *
>>>> + * Authors:
>>>> + *  Jan Kiszka        <jan.kiszka@siemens.com>
>>>> + *
>>>> + * This work is licensed under the terms of the GNU GPL version 2.
>>>> + * See the COPYING file in the top-level directory.
>>>> + *
>>>> + */
>>>> +
>>>> +#include "qemu-common.h"
>>>> +#include "sysemu.h"
>>>> +#include "sysbus.h"
>>>> +#include "kvm.h"
>>>> +#include "kvmclock.h"
>>>> +
>>>> +#if defined(CONFIG_KVM_PARA) && defined(KVM_CAP_ADJUST_CLOCK)
>>>> +
>>>> +#include <linux/kvm.h>
>>>> +#include <linux/kvm_para.h>
>>>> +
>>>> +typedef struct KVMClockState {
>>>> +    SysBusDevice busdev;
>>>> +    uint64_t clock;
>>>> +    bool clock_valid;
>>>> +} KVMClockState;
>>>> +
>>>> +static void kvmclock_pre_save(void *opaque)
>>>> +{
>>>> +    KVMClockState *s = opaque;
>>>> +    struct kvm_clock_data data;
>>>> +    int ret;
>>>> +
>>>> +    if (s->clock_valid) {
>>>> +        return;
>>>> +    }
>>>> +    ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
>>>> +    if (ret < 0) {
>>>> +        fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
>>>> +        data.clock = 0;
>>>> +    }
>>>> +    s->clock = data.clock;
>>>> +    /*
>>>> +     * If the VM is stopped, declare the clock state valid to avoid re-reading
>>>> +     * it on next vmsave (which would return a different value). Will be reset
>>>> +     * when the VM is continued.
>>>> +     */
>>>> +    s->clock_valid = !vm_running;
>>>> +}
>>>> +
>>>> +static int kvmclock_post_load(void *opaque, int version_id)
>>>> +{
>>>> +    KVMClockState *s = opaque;
>>>> +    struct kvm_clock_data data;
>>>> +
>>>> +    data.clock = s->clock;
>>>> +    data.flags = 0;
>>>> +    return kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
>>>> +}
>>>> +
>>>> +static void kvmclock_vm_state_change(void *opaque, int running, int reason)
>>>> +{
>>>> +    KVMClockState *s = opaque;
>>>> +
>>>> +    if (running) {
>>>> +        s->clock_valid = false;
>>>> +    }
>>>> +}
>>>> +
>>>> +static int kvmclock_init(SysBusDevice *dev)
>>>> +{
>>>> +    KVMClockState *s = FROM_SYSBUS(KVMClockState, dev);
>>>> +
>>>> +    qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static const VMStateDescription kvmclock_vmsd = {
>>>> +    .name = "kvmclock",
>>>> +    .version_id = 1,
>>>> +    .minimum_version_id = 1,
>>>> +    .minimum_version_id_old = 1,
>>>> +    .pre_save = kvmclock_pre_save,
>>>> +    .post_load = kvmclock_post_load,
>>>> +    .fields = (VMStateField[]) {
>>>> +        VMSTATE_UINT64(clock, KVMClockState),
>>>> +        VMSTATE_END_OF_LIST()
>>>> +    }
>>>> +};
>>>> +
>>>> +static SysBusDeviceInfo kvmclock_info = {
>>>> +    .qdev.name = "kvmclock",
>>>> +    .qdev.size = sizeof(KVMClockState),
>>>> +    .qdev.vmsd = &kvmclock_vmsd,
>>>> +    .qdev.no_user = 1,
>>>> +    .init = kvmclock_init,
>>>> +};
>>>> +
>>>> +/* Note: Must be called after VCPU initialization. */
>>>> +void kvmclock_create(void)
>>>> +{
>>>> +    if (kvm_enabled() &&
>>>> +        first_cpu->cpuid_kvm_features & (1ULL << KVM_FEATURE_CLOCKSOURCE)) {
>>>> +        sysbus_create_simple("kvmclock", -1, NULL);
>>>> +    }
>>>> +}
>>>> +
>>>> +static void kvmclock_register_device(void)
>>>> +{
>>>> +    if (kvm_enabled()) {
>>>> +        sysbus_register_withprop(&kvmclock_info);
>>>> +    }
>>>> +}
>>>> +
>>>> +device_init(kvmclock_register_device);
>>>> +
>>>> +#else /* !(CONFIG_KVM_PARA && KVM_CAP_ADJUST_CLOCK) */
>>>> +
>>>> +void kvmclock_create(void)
>>>> +{
>>>> +}
>>>> +#endif /* !(CONFIG_KVM_PARA && KVM_CAP_ADJUST_CLOCK) */
>>>> diff --git a/hw/kvmclock.h b/hw/kvmclock.h
>>>> new file mode 100644
>>>> index 0000000..7a83cbe
>>>> --- /dev/null
>>>> +++ b/hw/kvmclock.h
>>>> @@ -0,0 +1,14 @@
>>>> +/*
>>>> + * QEMU KVM support, paravirtual clock device
>>>> + *
>>>> + * Copyright (C) 2011 Siemens AG
>>>> + *
>>>> + * Authors:
>>>> + *  Jan Kiszka        <jan.kiszka@siemens.com>
>>>> + *
>>>> + * This work is licensed under the terms of the GNU GPL version 2.
>>>> + * See the COPYING file in the top-level directory.
>>>> + *
>>>> + */
>>>> +
>>>> +void kvmclock_create(void);
>>>> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
>>>> index 7b74473..9bc4659 100644
>>>> --- a/hw/pc_piix.c
>>>> +++ b/hw/pc_piix.c
>>>> @@ -32,6 +32,7 @@
>>>>  #include "boards.h"
>>>>  #include "ide.h"
>>>>  #include "kvm.h"
>>>> +#include "kvmclock.h"
>>>>  #include "sysemu.h"
>>>>  #include "sysbus.h"
>>>>  #include "arch_init.h"
>>>> @@ -66,7 +67,8 @@ static void pc_init1(ram_addr_t ram_size,
>>>>                       const char *kernel_cmdline,
>>>>                       const char *initrd_filename,
>>>>                       const char *cpu_model,
>>>> -                     int pci_enabled)
>>>> +                     int pci_enabled,
>>>> +                     int kvmclock_enabled)
>>>>  
>>> What exactly is your motivation to that ? I think mid/long-term
>>> we should be making machine initialization more common among
>>> architectures, not introducing more arch specific, or even worse, kvm
>>> specific parameters here.
>>>
>>> I'd like to understand what do we gain from that, since opting kvmclock
>>> in our out is done by cpuid anyway - no need for a specific machine.
>>
>> Is that really the case? I thought we were already shipping versions
>> where that CPU feature was enabled by default. If not, I'll happily drop
>> that admittedly clumsy approach above.
> 
> Yes, AFAIK, kvmclock is enabled by default, disabled by cpuid-leaf, as
> in
> -cpu kvm64,-kvmclock
> 
> So your test for cpuid bit before starting kvmclock should already cover
> it.
> 

No, just the contrary: As kvmclock was always enabled in older versions
and the compat machines also expose it, we cannot rely on the flag to
enable this new (and therefore 0.15-only) vmstate.

Jan

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

[Qemu-devel] Re: [PATCH 15/15] kvm: x86: Introduce kvmclock device to save/restore its state

[Glauber Costa]

On Mon, 2011-02-07 at 15:03 +0100, Jan Kiszka wrote:
> On 2011-02-07 14:40, Glauber Costa wrote:
> > On Mon, 2011-02-07 at 13:36 +0100, Jan Kiszka wrote:
> >> On 2011-02-07 13:27, Glauber Costa wrote:
> >>> On Mon, 2011-02-07 at 12:19 +0100, Jan Kiszka wrote:
> >>>> If kvmclock is used, which implies the kernel supports it, register a
> >>>> kvmclock device with the sysbus. Its main purpose is to save and restore
> >>>> the kernel state on migration, but this will also allow to visualize it
> >>>> one day.
> >>>>
> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>> CC: Glauber Costa <glommer@redhat.com>
> >>>> ---
> >>>>  Makefile.target |    4 +-
> >>>>  hw/kvmclock.c   |  125 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>  hw/kvmclock.h   |   14 ++++++
> >>>>  hw/pc_piix.c    |   31 +++++++++++---
> >>>>  4 files changed, 165 insertions(+), 9 deletions(-)
> >>>>  create mode 100644 hw/kvmclock.c
> >>>>  create mode 100644 hw/kvmclock.h
> >>>>
> >>>> diff --git a/Makefile.target b/Makefile.target
> >>>> index b0ba95f..30232fa 100644
> >>>> --- a/Makefile.target
> >>>> +++ b/Makefile.target
> >>>> @@ -37,7 +37,7 @@ ifndef CONFIG_HAIKU
> >>>>  LIBS+=-lm
> >>>>  endif
> >>>>  
> >>>> -kvm.o kvm-all.o vhost.o vhost_net.o: QEMU_CFLAGS+=$(KVM_CFLAGS)
> >>>> +kvm.o kvm-all.o vhost.o vhost_net.o kvmclock.o: QEMU_CFLAGS+=$(KVM_CFLAGS)
> >>>>  
> >>>>  config-target.h: config-target.h-timestamp
> >>>>  config-target.h-timestamp: config-target.mak
> >>>> @@ -218,7 +218,7 @@ obj-i386-y += cirrus_vga.o apic.o ioapic.o piix_pci.o
> >>>>  obj-i386-y += vmmouse.o vmport.o hpet.o applesmc.o
> >>>>  obj-i386-y += device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.o
> >>>>  obj-i386-y += debugcon.o multiboot.o
> >>>> -obj-i386-y += pc_piix.o
> >>>> +obj-i386-y += pc_piix.o kvmclock.o
> >>>>  obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
> >>>>  
> >>>>  # shared objects
> >>>> diff --git a/hw/kvmclock.c b/hw/kvmclock.c
> >>>> new file mode 100644
> >>>> index 0000000..b6ceddf
> >>>> --- /dev/null
> >>>> +++ b/hw/kvmclock.c
> >>>> @@ -0,0 +1,125 @@
> >>>> +/*
> >>>> + * QEMU KVM support, paravirtual clock device
> >>>> + *
> >>>> + * Copyright (C) 2011 Siemens AG
> >>>> + *
> >>>> + * Authors:
> >>>> + *  Jan Kiszka        <jan.kiszka@siemens.com>
> >>>> + *
> >>>> + * This work is licensed under the terms of the GNU GPL version 2.
> >>>> + * See the COPYING file in the top-level directory.
> >>>> + *
> >>>> + */
> >>>> +
> >>>> +#include "qemu-common.h"
> >>>> +#include "sysemu.h"
> >>>> +#include "sysbus.h"
> >>>> +#include "kvm.h"
> >>>> +#include "kvmclock.h"
> >>>> +
> >>>> +#if defined(CONFIG_KVM_PARA) && defined(KVM_CAP_ADJUST_CLOCK)
> >>>> +
> >>>> +#include <linux/kvm.h>
> >>>> +#include <linux/kvm_para.h>
> >>>> +
> >>>> +typedef struct KVMClockState {
> >>>> +    SysBusDevice busdev;
> >>>> +    uint64_t clock;
> >>>> +    bool clock_valid;
> >>>> +} KVMClockState;
> >>>> +
> >>>> +static void kvmclock_pre_save(void *opaque)
> >>>> +{
> >>>> +    KVMClockState *s = opaque;
> >>>> +    struct kvm_clock_data data;
> >>>> +    int ret;
> >>>> +
> >>>> +    if (s->clock_valid) {
> >>>> +        return;
> >>>> +    }
> >>>> +    ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
> >>>> +    if (ret < 0) {
> >>>> +        fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
> >>>> +        data.clock = 0;
> >>>> +    }
> >>>> +    s->clock = data.clock;
> >>>> +    /*
> >>>> +     * If the VM is stopped, declare the clock state valid to avoid re-reading
> >>>> +     * it on next vmsave (which would return a different value). Will be reset
> >>>> +     * when the VM is continued.
> >>>> +     */
> >>>> +    s->clock_valid = !vm_running;
> >>>> +}
> >>>> +
> >>>> +static int kvmclock_post_load(void *opaque, int version_id)
> >>>> +{
> >>>> +    KVMClockState *s = opaque;
> >>>> +    struct kvm_clock_data data;
> >>>> +
> >>>> +    data.clock = s->clock;
> >>>> +    data.flags = 0;
> >>>> +    return kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
> >>>> +}
> >>>> +
> >>>> +static void kvmclock_vm_state_change(void *opaque, int running, int reason)
> >>>> +{
> >>>> +    KVMClockState *s = opaque;
> >>>> +
> >>>> +    if (running) {
> >>>> +        s->clock_valid = false;
> >>>> +    }
> >>>> +}
> >>>> +
> >>>> +static int kvmclock_init(SysBusDevice *dev)
> >>>> +{
> >>>> +    KVMClockState *s = FROM_SYSBUS(KVMClockState, dev);
> >>>> +
> >>>> +    qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
> >>>> +    return 0;
> >>>> +}
> >>>> +
> >>>> +static const VMStateDescription kvmclock_vmsd = {
> >>>> +    .name = "kvmclock",
> >>>> +    .version_id = 1,
> >>>> +    .minimum_version_id = 1,
> >>>> +    .minimum_version_id_old = 1,
> >>>> +    .pre_save = kvmclock_pre_save,
> >>>> +    .post_load = kvmclock_post_load,
> >>>> +    .fields = (VMStateField[]) {
> >>>> +        VMSTATE_UINT64(clock, KVMClockState),
> >>>> +        VMSTATE_END_OF_LIST()
> >>>> +    }
> >>>> +};
> >>>> +
> >>>> +static SysBusDeviceInfo kvmclock_info = {
> >>>> +    .qdev.name = "kvmclock",
> >>>> +    .qdev.size = sizeof(KVMClockState),
> >>>> +    .qdev.vmsd = &kvmclock_vmsd,
> >>>> +    .qdev.no_user = 1,
> >>>> +    .init = kvmclock_init,
> >>>> +};
> >>>> +
> >>>> +/* Note: Must be called after VCPU initialization. */
> >>>> +void kvmclock_create(void)
> >>>> +{
> >>>> +    if (kvm_enabled() &&
> >>>> +        first_cpu->cpuid_kvm_features & (1ULL << KVM_FEATURE_CLOCKSOURCE)) {
> >>>> +        sysbus_create_simple("kvmclock", -1, NULL);
> >>>> +    }
> >>>> +}
> >>>> +
> >>>> +static void kvmclock_register_device(void)
> >>>> +{
> >>>> +    if (kvm_enabled()) {
> >>>> +        sysbus_register_withprop(&kvmclock_info);
> >>>> +    }
> >>>> +}
> >>>> +
> >>>> +device_init(kvmclock_register_device);
> >>>> +
> >>>> +#else /* !(CONFIG_KVM_PARA && KVM_CAP_ADJUST_CLOCK) */
> >>>> +
> >>>> +void kvmclock_create(void)
> >>>> +{
> >>>> +}
> >>>> +#endif /* !(CONFIG_KVM_PARA && KVM_CAP_ADJUST_CLOCK) */
> >>>> diff --git a/hw/kvmclock.h b/hw/kvmclock.h
> >>>> new file mode 100644
> >>>> index 0000000..7a83cbe
> >>>> --- /dev/null
> >>>> +++ b/hw/kvmclock.h
> >>>> @@ -0,0 +1,14 @@
> >>>> +/*
> >>>> + * QEMU KVM support, paravirtual clock device
> >>>> + *
> >>>> + * Copyright (C) 2011 Siemens AG
> >>>> + *
> >>>> + * Authors:
> >>>> + *  Jan Kiszka        <jan.kiszka@siemens.com>
> >>>> + *
> >>>> + * This work is licensed under the terms of the GNU GPL version 2.
> >>>> + * See the COPYING file in the top-level directory.
> >>>> + *
> >>>> + */
> >>>> +
> >>>> +void kvmclock_create(void);
> >>>> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> >>>> index 7b74473..9bc4659 100644
> >>>> --- a/hw/pc_piix.c
> >>>> +++ b/hw/pc_piix.c
> >>>> @@ -32,6 +32,7 @@
> >>>>  #include "boards.h"
> >>>>  #include "ide.h"
> >>>>  #include "kvm.h"
> >>>> +#include "kvmclock.h"
> >>>>  #include "sysemu.h"
> >>>>  #include "sysbus.h"
> >>>>  #include "arch_init.h"
> >>>> @@ -66,7 +67,8 @@ static void pc_init1(ram_addr_t ram_size,
> >>>>                       const char *kernel_cmdline,
> >>>>                       const char *initrd_filename,
> >>>>                       const char *cpu_model,
> >>>> -                     int pci_enabled)
> >>>> +                     int pci_enabled,
> >>>> +                     int kvmclock_enabled)
> >>>>  
> >>> What exactly is your motivation to that ? I think mid/long-term
> >>> we should be making machine initialization more common among
> >>> architectures, not introducing more arch specific, or even worse, kvm
> >>> specific parameters here.
> >>>
> >>> I'd like to understand what do we gain from that, since opting kvmclock
> >>> in our out is done by cpuid anyway - no need for a specific machine.
> >>
> >> Is that really the case? I thought we were already shipping versions
> >> where that CPU feature was enabled by default. If not, I'll happily drop
> >> that admittedly clumsy approach above.
> > 
> > Yes, AFAIK, kvmclock is enabled by default, disabled by cpuid-leaf, as
> > in
> > -cpu kvm64,-kvmclock
> > 
> > So your test for cpuid bit before starting kvmclock should already cover
> > it.
> > 
> 
> No, just the contrary: As kvmclock was always enabled in older versions
> and the compat machines also expose it, we cannot rely on the flag to
> enable this new (and therefore 0.15-only) vmstate.

I see. You're not enabling kvmclock functionality, but rather, kvmclock
device.

It makes sense then, but I still would prefer it encoded somewhere else
(but not a hard constraint anymore). Can't qdev encode it, say, in the
value field of the device? 

[Qemu-devel] Re: [PATCH 15/15] kvm: x86: Introduce kvmclock device to save/restore its state

[Jan Kiszka]

On 2011-02-07 19:04, Glauber Costa wrote:
> On Mon, 2011-02-07 at 15:03 +0100, Jan Kiszka wrote:
>> On 2011-02-07 14:40, Glauber Costa wrote:
>>> On Mon, 2011-02-07 at 13:36 +0100, Jan Kiszka wrote:
>>>> On 2011-02-07 13:27, Glauber Costa wrote:
>>>>> On Mon, 2011-02-07 at 12:19 +0100, Jan Kiszka wrote:
>>>>>> If kvmclock is used, which implies the kernel supports it, register a
>>>>>> kvmclock device with the sysbus. Its main purpose is to save and restore
>>>>>> the kernel state on migration, but this will also allow to visualize it
>>>>>> one day.
>>>>>>
>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>> CC: Glauber Costa <glommer@redhat.com>
>>>>>> ---
>>>>>>  Makefile.target |    4 +-
>>>>>>  hw/kvmclock.c   |  125 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>  hw/kvmclock.h   |   14 ++++++
>>>>>>  hw/pc_piix.c    |   31 +++++++++++---
>>>>>>  4 files changed, 165 insertions(+), 9 deletions(-)
>>>>>>  create mode 100644 hw/kvmclock.c
>>>>>>  create mode 100644 hw/kvmclock.h
>>>>>>
>>>>>> diff --git a/Makefile.target b/Makefile.target
>>>>>> index b0ba95f..30232fa 100644
>>>>>> --- a/Makefile.target
>>>>>> +++ b/Makefile.target
>>>>>> @@ -37,7 +37,7 @@ ifndef CONFIG_HAIKU
>>>>>>  LIBS+=-lm
>>>>>>  endif
>>>>>>  
>>>>>> -kvm.o kvm-all.o vhost.o vhost_net.o: QEMU_CFLAGS+=$(KVM_CFLAGS)
>>>>>> +kvm.o kvm-all.o vhost.o vhost_net.o kvmclock.o: QEMU_CFLAGS+=$(KVM_CFLAGS)
>>>>>>  
>>>>>>  config-target.h: config-target.h-timestamp
>>>>>>  config-target.h-timestamp: config-target.mak
>>>>>> @@ -218,7 +218,7 @@ obj-i386-y += cirrus_vga.o apic.o ioapic.o piix_pci.o
>>>>>>  obj-i386-y += vmmouse.o vmport.o hpet.o applesmc.o
>>>>>>  obj-i386-y += device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.o
>>>>>>  obj-i386-y += debugcon.o multiboot.o
>>>>>> -obj-i386-y += pc_piix.o
>>>>>> +obj-i386-y += pc_piix.o kvmclock.o
>>>>>>  obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
>>>>>>  
>>>>>>  # shared objects
>>>>>> diff --git a/hw/kvmclock.c b/hw/kvmclock.c
>>>>>> new file mode 100644
>>>>>> index 0000000..b6ceddf
>>>>>> --- /dev/null
>>>>>> +++ b/hw/kvmclock.c
>>>>>> @@ -0,0 +1,125 @@
>>>>>> +/*
>>>>>> + * QEMU KVM support, paravirtual clock device
>>>>>> + *
>>>>>> + * Copyright (C) 2011 Siemens AG
>>>>>> + *
>>>>>> + * Authors:
>>>>>> + *  Jan Kiszka        <jan.kiszka@siemens.com>
>>>>>> + *
>>>>>> + * This work is licensed under the terms of the GNU GPL version 2.
>>>>>> + * See the COPYING file in the top-level directory.
>>>>>> + *
>>>>>> + */
>>>>>> +
>>>>>> +#include "qemu-common.h"
>>>>>> +#include "sysemu.h"
>>>>>> +#include "sysbus.h"
>>>>>> +#include "kvm.h"
>>>>>> +#include "kvmclock.h"
>>>>>> +
>>>>>> +#if defined(CONFIG_KVM_PARA) && defined(KVM_CAP_ADJUST_CLOCK)
>>>>>> +
>>>>>> +#include <linux/kvm.h>
>>>>>> +#include <linux/kvm_para.h>
>>>>>> +
>>>>>> +typedef struct KVMClockState {
>>>>>> +    SysBusDevice busdev;
>>>>>> +    uint64_t clock;
>>>>>> +    bool clock_valid;
>>>>>> +} KVMClockState;
>>>>>> +
>>>>>> +static void kvmclock_pre_save(void *opaque)
>>>>>> +{
>>>>>> +    KVMClockState *s = opaque;
>>>>>> +    struct kvm_clock_data data;
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    if (s->clock_valid) {
>>>>>> +        return;
>>>>>> +    }
>>>>>> +    ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
>>>>>> +    if (ret < 0) {
>>>>>> +        fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
>>>>>> +        data.clock = 0;
>>>>>> +    }
>>>>>> +    s->clock = data.clock;
>>>>>> +    /*
>>>>>> +     * If the VM is stopped, declare the clock state valid to avoid re-reading
>>>>>> +     * it on next vmsave (which would return a different value). Will be reset
>>>>>> +     * when the VM is continued.
>>>>>> +     */
>>>>>> +    s->clock_valid = !vm_running;
>>>>>> +}
>>>>>> +
>>>>>> +static int kvmclock_post_load(void *opaque, int version_id)
>>>>>> +{
>>>>>> +    KVMClockState *s = opaque;
>>>>>> +    struct kvm_clock_data data;
>>>>>> +
>>>>>> +    data.clock = s->clock;
>>>>>> +    data.flags = 0;
>>>>>> +    return kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
>>>>>> +}
>>>>>> +
>>>>>> +static void kvmclock_vm_state_change(void *opaque, int running, int reason)
>>>>>> +{
>>>>>> +    KVMClockState *s = opaque;
>>>>>> +
>>>>>> +    if (running) {
>>>>>> +        s->clock_valid = false;
>>>>>> +    }
>>>>>> +}
>>>>>> +
>>>>>> +static int kvmclock_init(SysBusDevice *dev)
>>>>>> +{
>>>>>> +    KVMClockState *s = FROM_SYSBUS(KVMClockState, dev);
>>>>>> +
>>>>>> +    qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static const VMStateDescription kvmclock_vmsd = {
>>>>>> +    .name = "kvmclock",
>>>>>> +    .version_id = 1,
>>>>>> +    .minimum_version_id = 1,
>>>>>> +    .minimum_version_id_old = 1,
>>>>>> +    .pre_save = kvmclock_pre_save,
>>>>>> +    .post_load = kvmclock_post_load,
>>>>>> +    .fields = (VMStateField[]) {
>>>>>> +        VMSTATE_UINT64(clock, KVMClockState),
>>>>>> +        VMSTATE_END_OF_LIST()
>>>>>> +    }
>>>>>> +};
>>>>>> +
>>>>>> +static SysBusDeviceInfo kvmclock_info = {
>>>>>> +    .qdev.name = "kvmclock",
>>>>>> +    .qdev.size = sizeof(KVMClockState),
>>>>>> +    .qdev.vmsd = &kvmclock_vmsd,
>>>>>> +    .qdev.no_user = 1,
>>>>>> +    .init = kvmclock_init,
>>>>>> +};
>>>>>> +
>>>>>> +/* Note: Must be called after VCPU initialization. */
>>>>>> +void kvmclock_create(void)
>>>>>> +{
>>>>>> +    if (kvm_enabled() &&
>>>>>> +        first_cpu->cpuid_kvm_features & (1ULL << KVM_FEATURE_CLOCKSOURCE)) {
>>>>>> +        sysbus_create_simple("kvmclock", -1, NULL);
>>>>>> +    }
>>>>>> +}
>>>>>> +
>>>>>> +static void kvmclock_register_device(void)
>>>>>> +{
>>>>>> +    if (kvm_enabled()) {
>>>>>> +        sysbus_register_withprop(&kvmclock_info);
>>>>>> +    }
>>>>>> +}
>>>>>> +
>>>>>> +device_init(kvmclock_register_device);
>>>>>> +
>>>>>> +#else /* !(CONFIG_KVM_PARA && KVM_CAP_ADJUST_CLOCK) */
>>>>>> +
>>>>>> +void kvmclock_create(void)
>>>>>> +{
>>>>>> +}
>>>>>> +#endif /* !(CONFIG_KVM_PARA && KVM_CAP_ADJUST_CLOCK) */
>>>>>> diff --git a/hw/kvmclock.h b/hw/kvmclock.h
>>>>>> new file mode 100644
>>>>>> index 0000000..7a83cbe
>>>>>> --- /dev/null
>>>>>> +++ b/hw/kvmclock.h
>>>>>> @@ -0,0 +1,14 @@
>>>>>> +/*
>>>>>> + * QEMU KVM support, paravirtual clock device
>>>>>> + *
>>>>>> + * Copyright (C) 2011 Siemens AG
>>>>>> + *
>>>>>> + * Authors:
>>>>>> + *  Jan Kiszka        <jan.kiszka@siemens.com>
>>>>>> + *
>>>>>> + * This work is licensed under the terms of the GNU GPL version 2.
>>>>>> + * See the COPYING file in the top-level directory.
>>>>>> + *
>>>>>> + */
>>>>>> +
>>>>>> +void kvmclock_create(void);
>>>>>> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
>>>>>> index 7b74473..9bc4659 100644
>>>>>> --- a/hw/pc_piix.c
>>>>>> +++ b/hw/pc_piix.c
>>>>>> @@ -32,6 +32,7 @@
>>>>>>  #include "boards.h"
>>>>>>  #include "ide.h"
>>>>>>  #include "kvm.h"
>>>>>> +#include "kvmclock.h"
>>>>>>  #include "sysemu.h"
>>>>>>  #include "sysbus.h"
>>>>>>  #include "arch_init.h"
>>>>>> @@ -66,7 +67,8 @@ static void pc_init1(ram_addr_t ram_size,
>>>>>>                       const char *kernel_cmdline,
>>>>>>                       const char *initrd_filename,
>>>>>>                       const char *cpu_model,
>>>>>> -                     int pci_enabled)
>>>>>> +                     int pci_enabled,
>>>>>> +                     int kvmclock_enabled)
>>>>>>  
>>>>> What exactly is your motivation to that ? I think mid/long-term
>>>>> we should be making machine initialization more common among
>>>>> architectures, not introducing more arch specific, or even worse, kvm
>>>>> specific parameters here.
>>>>>
>>>>> I'd like to understand what do we gain from that, since opting kvmclock
>>>>> in our out is done by cpuid anyway - no need for a specific machine.
>>>>
>>>> Is that really the case? I thought we were already shipping versions
>>>> where that CPU feature was enabled by default. If not, I'll happily drop
>>>> that admittedly clumsy approach above.
>>>
>>> Yes, AFAIK, kvmclock is enabled by default, disabled by cpuid-leaf, as
>>> in
>>> -cpu kvm64,-kvmclock
>>>
>>> So your test for cpuid bit before starting kvmclock should already cover
>>> it.
>>>
>>
>> No, just the contrary: As kvmclock was always enabled in older versions
>> and the compat machines also expose it, we cannot rely on the flag to
>> enable this new (and therefore 0.15-only) vmstate.
> 
> I see. You're not enabling kvmclock functionality, but rather, kvmclock
> device.
> 
> It makes sense then, but I still would prefer it encoded somewhere else
> (but not a hard constraint anymore). Can't qdev encode it, say, in the
> value field of the device? 

It would take some machine properties that could be pre-set according to
compat requirements. I have such machine-specific properties in mind
anyway in order to implement -machine in a more flexible way than
proposed so far.

I'll surely clean up the small ugliness this patch introduces to
pc_piix.c once there is a better way, I just don't want to let kvmclock
wait for that perfect infrastructure.

Jan

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

[Qemu-devel] Re: [PATCH 15/15] kvm: x86: Introduce kvmclock device to save/restore its state

[Glauber Costa]

On Mon, 2011-02-07 at 19:12 +0100, Jan Kiszka wrote:
> On 2011-02-07 19:04, Glauber Costa wrote:
> > On Mon, 2011-02-07 at 15:03 +0100, Jan Kiszka wrote:
> >> On 2011-02-07 14:40, Glauber Costa wrote:
> >>> On Mon, 2011-02-07 at 13:36 +0100, Jan Kiszka wrote:
> >>>> On 2011-02-07 13:27, Glauber Costa wrote:
> >>>>> On Mon, 2011-02-07 at 12:19 +0100, Jan Kiszka wrote:
> >>>>>> If kvmclock is used, which implies the kernel supports it, register a
> >>>>>> kvmclock device with the sysbus. Its main purpose is to save and restore
> >>>>>> the kernel state on migration, but this will also allow to visualize it
> >>>>>> one day.
> >>>>>>
> >>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>> CC: Glauber Costa <glommer@redhat.com>
> >>>>>> ---
> >>>>>>  Makefile.target |    4 +-
> >>>>>>  hw/kvmclock.c   |  125 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>>  hw/kvmclock.h   |   14 ++++++
> >>>>>>  hw/pc_piix.c    |   31 +++++++++++---
> >>>>>>  4 files changed, 165 insertions(+), 9 deletions(-)
> >>>>>>  create mode 100644 hw/kvmclock.c
> >>>>>>  create mode 100644 hw/kvmclock.h
> >>>>>>
> >>>>>> diff --git a/Makefile.target b/Makefile.target
> >>>>>> index b0ba95f..30232fa 100644
> >>>>>> --- a/Makefile.target
> >>>>>> +++ b/Makefile.target
> >>>>>> @@ -37,7 +37,7 @@ ifndef CONFIG_HAIKU
> >>>>>>  LIBS+=-lm
> >>>>>>  endif
> >>>>>>  
> >>>>>> -kvm.o kvm-all.o vhost.o vhost_net.o: QEMU_CFLAGS+=$(KVM_CFLAGS)
> >>>>>> +kvm.o kvm-all.o vhost.o vhost_net.o kvmclock.o: QEMU_CFLAGS+=$(KVM_CFLAGS)
> >>>>>>  
> >>>>>>  config-target.h: config-target.h-timestamp
> >>>>>>  config-target.h-timestamp: config-target.mak
> >>>>>> @@ -218,7 +218,7 @@ obj-i386-y += cirrus_vga.o apic.o ioapic.o piix_pci.o
> >>>>>>  obj-i386-y += vmmouse.o vmport.o hpet.o applesmc.o
> >>>>>>  obj-i386-y += device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.o
> >>>>>>  obj-i386-y += debugcon.o multiboot.o
> >>>>>> -obj-i386-y += pc_piix.o
> >>>>>> +obj-i386-y += pc_piix.o kvmclock.o
> >>>>>>  obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
> >>>>>>  
> >>>>>>  # shared objects
> >>>>>> diff --git a/hw/kvmclock.c b/hw/kvmclock.c
> >>>>>> new file mode 100644
> >>>>>> index 0000000..b6ceddf
> >>>>>> --- /dev/null
> >>>>>> +++ b/hw/kvmclock.c
> >>>>>> @@ -0,0 +1,125 @@
> >>>>>> +/*
> >>>>>> + * QEMU KVM support, paravirtual clock device
> >>>>>> + *
> >>>>>> + * Copyright (C) 2011 Siemens AG
> >>>>>> + *
> >>>>>> + * Authors:
> >>>>>> + *  Jan Kiszka        <jan.kiszka@siemens.com>
> >>>>>> + *
> >>>>>> + * This work is licensed under the terms of the GNU GPL version 2.
> >>>>>> + * See the COPYING file in the top-level directory.
> >>>>>> + *
> >>>>>> + */
> >>>>>> +
> >>>>>> +#include "qemu-common.h"
> >>>>>> +#include "sysemu.h"
> >>>>>> +#include "sysbus.h"
> >>>>>> +#include "kvm.h"
> >>>>>> +#include "kvmclock.h"
> >>>>>> +
> >>>>>> +#if defined(CONFIG_KVM_PARA) && defined(KVM_CAP_ADJUST_CLOCK)
> >>>>>> +
> >>>>>> +#include <linux/kvm.h>
> >>>>>> +#include <linux/kvm_para.h>
> >>>>>> +
> >>>>>> +typedef struct KVMClockState {
> >>>>>> +    SysBusDevice busdev;
> >>>>>> +    uint64_t clock;
> >>>>>> +    bool clock_valid;
> >>>>>> +} KVMClockState;
> >>>>>> +
> >>>>>> +static void kvmclock_pre_save(void *opaque)
> >>>>>> +{
> >>>>>> +    KVMClockState *s = opaque;
> >>>>>> +    struct kvm_clock_data data;
> >>>>>> +    int ret;
> >>>>>> +
> >>>>>> +    if (s->clock_valid) {
> >>>>>> +        return;
> >>>>>> +    }
> >>>>>> +    ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
> >>>>>> +    if (ret < 0) {
> >>>>>> +        fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
> >>>>>> +        data.clock = 0;
> >>>>>> +    }
> >>>>>> +    s->clock = data.clock;
> >>>>>> +    /*
> >>>>>> +     * If the VM is stopped, declare the clock state valid to avoid re-reading
> >>>>>> +     * it on next vmsave (which would return a different value). Will be reset
> >>>>>> +     * when the VM is continued.
> >>>>>> +     */
> >>>>>> +    s->clock_valid = !vm_running;
> >>>>>> +}
> >>>>>> +
> >>>>>> +static int kvmclock_post_load(void *opaque, int version_id)
> >>>>>> +{
> >>>>>> +    KVMClockState *s = opaque;
> >>>>>> +    struct kvm_clock_data data;
> >>>>>> +
> >>>>>> +    data.clock = s->clock;
> >>>>>> +    data.flags = 0;
> >>>>>> +    return kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void kvmclock_vm_state_change(void *opaque, int running, int reason)
> >>>>>> +{
> >>>>>> +    KVMClockState *s = opaque;
> >>>>>> +
> >>>>>> +    if (running) {
> >>>>>> +        s->clock_valid = false;
> >>>>>> +    }
> >>>>>> +}
> >>>>>> +
> >>>>>> +static int kvmclock_init(SysBusDevice *dev)
> >>>>>> +{
> >>>>>> +    KVMClockState *s = FROM_SYSBUS(KVMClockState, dev);
> >>>>>> +
> >>>>>> +    qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
> >>>>>> +    return 0;
> >>>>>> +}
> >>>>>> +
> >>>>>> +static const VMStateDescription kvmclock_vmsd = {
> >>>>>> +    .name = "kvmclock",
> >>>>>> +    .version_id = 1,
> >>>>>> +    .minimum_version_id = 1,
> >>>>>> +    .minimum_version_id_old = 1,
> >>>>>> +    .pre_save = kvmclock_pre_save,
> >>>>>> +    .post_load = kvmclock_post_load,
> >>>>>> +    .fields = (VMStateField[]) {
> >>>>>> +        VMSTATE_UINT64(clock, KVMClockState),
> >>>>>> +        VMSTATE_END_OF_LIST()
> >>>>>> +    }
> >>>>>> +};
> >>>>>> +
> >>>>>> +static SysBusDeviceInfo kvmclock_info = {
> >>>>>> +    .qdev.name = "kvmclock",
> >>>>>> +    .qdev.size = sizeof(KVMClockState),
> >>>>>> +    .qdev.vmsd = &kvmclock_vmsd,
> >>>>>> +    .qdev.no_user = 1,
> >>>>>> +    .init = kvmclock_init,
> >>>>>> +};
> >>>>>> +
> >>>>>> +/* Note: Must be called after VCPU initialization. */
> >>>>>> +void kvmclock_create(void)
> >>>>>> +{
> >>>>>> +    if (kvm_enabled() &&
> >>>>>> +        first_cpu->cpuid_kvm_features & (1ULL << KVM_FEATURE_CLOCKSOURCE)) {
> >>>>>> +        sysbus_create_simple("kvmclock", -1, NULL);
> >>>>>> +    }
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void kvmclock_register_device(void)
> >>>>>> +{
> >>>>>> +    if (kvm_enabled()) {
> >>>>>> +        sysbus_register_withprop(&kvmclock_info);
> >>>>>> +    }
> >>>>>> +}
> >>>>>> +
> >>>>>> +device_init(kvmclock_register_device);
> >>>>>> +
> >>>>>> +#else /* !(CONFIG_KVM_PARA && KVM_CAP_ADJUST_CLOCK) */
> >>>>>> +
> >>>>>> +void kvmclock_create(void)
> >>>>>> +{
> >>>>>> +}
> >>>>>> +#endif /* !(CONFIG_KVM_PARA && KVM_CAP_ADJUST_CLOCK) */
> >>>>>> diff --git a/hw/kvmclock.h b/hw/kvmclock.h
> >>>>>> new file mode 100644
> >>>>>> index 0000000..7a83cbe
> >>>>>> --- /dev/null
> >>>>>> +++ b/hw/kvmclock.h
> >>>>>> @@ -0,0 +1,14 @@
> >>>>>> +/*
> >>>>>> + * QEMU KVM support, paravirtual clock device
> >>>>>> + *
> >>>>>> + * Copyright (C) 2011 Siemens AG
> >>>>>> + *
> >>>>>> + * Authors:
> >>>>>> + *  Jan Kiszka        <jan.kiszka@siemens.com>
> >>>>>> + *
> >>>>>> + * This work is licensed under the terms of the GNU GPL version 2.
> >>>>>> + * See the COPYING file in the top-level directory.
> >>>>>> + *
> >>>>>> + */
> >>>>>> +
> >>>>>> +void kvmclock_create(void);
> >>>>>> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> >>>>>> index 7b74473..9bc4659 100644
> >>>>>> --- a/hw/pc_piix.c
> >>>>>> +++ b/hw/pc_piix.c
> >>>>>> @@ -32,6 +32,7 @@
> >>>>>>  #include "boards.h"
> >>>>>>  #include "ide.h"
> >>>>>>  #include "kvm.h"
> >>>>>> +#include "kvmclock.h"
> >>>>>>  #include "sysemu.h"
> >>>>>>  #include "sysbus.h"
> >>>>>>  #include "arch_init.h"
> >>>>>> @@ -66,7 +67,8 @@ static void pc_init1(ram_addr_t ram_size,
> >>>>>>                       const char *kernel_cmdline,
> >>>>>>                       const char *initrd_filename,
> >>>>>>                       const char *cpu_model,
> >>>>>> -                     int pci_enabled)
> >>>>>> +                     int pci_enabled,
> >>>>>> +                     int kvmclock_enabled)
> >>>>>>  
> >>>>> What exactly is your motivation to that ? I think mid/long-term
> >>>>> we should be making machine initialization more common among
> >>>>> architectures, not introducing more arch specific, or even worse, kvm
> >>>>> specific parameters here.
> >>>>>
> >>>>> I'd like to understand what do we gain from that, since opting kvmclock
> >>>>> in our out is done by cpuid anyway - no need for a specific machine.
> >>>>
> >>>> Is that really the case? I thought we were already shipping versions
> >>>> where that CPU feature was enabled by default. If not, I'll happily drop
> >>>> that admittedly clumsy approach above.
> >>>
> >>> Yes, AFAIK, kvmclock is enabled by default, disabled by cpuid-leaf, as
> >>> in
> >>> -cpu kvm64,-kvmclock
> >>>
> >>> So your test for cpuid bit before starting kvmclock should already cover
> >>> it.
> >>>
> >>
> >> No, just the contrary: As kvmclock was always enabled in older versions
> >> and the compat machines also expose it, we cannot rely on the flag to
> >> enable this new (and therefore 0.15-only) vmstate.
> > 
> > I see. You're not enabling kvmclock functionality, but rather, kvmclock
> > device.
> > 
> > It makes sense then, but I still would prefer it encoded somewhere else
> > (but not a hard constraint anymore). Can't qdev encode it, say, in the
> > value field of the device? 
> 
> It would take some machine properties that could be pre-set according to
> compat requirements. I have such machine-specific properties in mind
> anyway in order to implement -machine in a more flexible way than
> proposed so far.
> 
> I'll surely clean up the small ugliness this patch introduces to
> pc_piix.c once there is a better way, I just don't want to let kvmclock
> wait for that perfect infrastructure.

Perfect is the enemy of good, I'm all for it.

[Qemu-devel] Re: [PATCH 15/15] kvm: x86: Introduce kvmclock device to save/restore its state

[Avi Kivity]

On 02/07/2011 02:27 PM, Glauber Costa wrote:
> >
> What exactly is your motivation to that ? I think mid/long-term
> we should be making machine initialization more common among
> architectures, not introducing more arch specific, or even worse, kvm
> specific parameters here.
>

A general note: there are several ways of being "kvm specific"; one of 
them is being tied to the implementation of kvm as the implementation of 
a virtual cpu in qemu.  Another, with an example here, is a cpu feature 
that is only (or at first) present in kvm, but in principle may be 
supported by any cpu implementation, like tcg or a real cpu.  It's 
important not to confuse the two; only the first needs all those hooks 
tied into the -accel infrastructure.

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

Re: [Qemu-devel] [PATCH 15/15] kvm: x86: Introduce kvmclock device to save/restore its state

[Blue Swirl]

On Mon, Feb 7, 2011 at 1:19 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> If kvmclock is used, which implies the kernel supports it, register a
> kvmclock device with the sysbus. Its main purpose is to save and restore
> the kernel state on migration, but this will also allow to visualize it
> one day.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> CC: Glauber Costa <glommer@redhat.com>
> ---
>  Makefile.target |    4 +-
>  hw/kvmclock.c   |  125 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/kvmclock.h   |   14 ++++++
>  hw/pc_piix.c    |   31 +++++++++++---
>  4 files changed, 165 insertions(+), 9 deletions(-)
>  create mode 100644 hw/kvmclock.c
>  create mode 100644 hw/kvmclock.h
>
> diff --git a/Makefile.target b/Makefile.target
> index b0ba95f..30232fa 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -37,7 +37,7 @@ ifndef CONFIG_HAIKU
>  LIBS+=-lm
>  endif
>
> -kvm.o kvm-all.o vhost.o vhost_net.o: QEMU_CFLAGS+=$(KVM_CFLAGS)
> +kvm.o kvm-all.o vhost.o vhost_net.o kvmclock.o: QEMU_CFLAGS+=$(KVM_CFLAGS)
>
>  config-target.h: config-target.h-timestamp
>  config-target.h-timestamp: config-target.mak
> @@ -218,7 +218,7 @@ obj-i386-y += cirrus_vga.o apic.o ioapic.o piix_pci.o
>  obj-i386-y += vmmouse.o vmport.o hpet.o applesmc.o
>  obj-i386-y += device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.o
>  obj-i386-y += debugcon.o multiboot.o
> -obj-i386-y += pc_piix.o
> +obj-i386-y += pc_piix.o kvmclock.o

Please build kvmclock.o conditionally to CONFIG_something...

>  obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
>
>  # shared objects
> diff --git a/hw/kvmclock.c b/hw/kvmclock.c
> new file mode 100644
> index 0000000..b6ceddf
> --- /dev/null
> +++ b/hw/kvmclock.c
> @@ -0,0 +1,125 @@
> +/*
> + * QEMU KVM support, paravirtual clock device
> + *
> + * Copyright (C) 2011 Siemens AG
> + *
> + * Authors:
> + *  Jan Kiszka        <jan.kiszka@siemens.com>
> + *
> + * This work is licensed under the terms of the GNU GPL version 2.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "qemu-common.h"
> +#include "sysemu.h"
> +#include "sysbus.h"
> +#include "kvm.h"
> +#include "kvmclock.h"
> +
> +#if defined(CONFIG_KVM_PARA) && defined(KVM_CAP_ADJUST_CLOCK)
> +
> +#include <linux/kvm.h>
> +#include <linux/kvm_para.h>
> +
> +typedef struct KVMClockState {
> +    SysBusDevice busdev;
> +    uint64_t clock;
> +    bool clock_valid;
> +} KVMClockState;
> +
> +static void kvmclock_pre_save(void *opaque)
> +{
> +    KVMClockState *s = opaque;
> +    struct kvm_clock_data data;
> +    int ret;
> +
> +    if (s->clock_valid) {
> +        return;
> +    }
> +    ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
> +    if (ret < 0) {
> +        fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
> +        data.clock = 0;
> +    }
> +    s->clock = data.clock;
> +    /*
> +     * If the VM is stopped, declare the clock state valid to avoid re-reading
> +     * it on next vmsave (which would return a different value). Will be reset
> +     * when the VM is continued.
> +     */
> +    s->clock_valid = !vm_running;
> +}
> +
> +static int kvmclock_post_load(void *opaque, int version_id)
> +{
> +    KVMClockState *s = opaque;
> +    struct kvm_clock_data data;
> +
> +    data.clock = s->clock;
> +    data.flags = 0;
> +    return kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
> +}
> +
> +static void kvmclock_vm_state_change(void *opaque, int running, int reason)
> +{
> +    KVMClockState *s = opaque;
> +
> +    if (running) {
> +        s->clock_valid = false;
> +    }
> +}
> +
> +static int kvmclock_init(SysBusDevice *dev)
> +{
> +    KVMClockState *s = FROM_SYSBUS(KVMClockState, dev);
> +
> +    qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
> +    return 0;
> +}
> +
> +static const VMStateDescription kvmclock_vmsd = {
> +    .name = "kvmclock",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .pre_save = kvmclock_pre_save,
> +    .post_load = kvmclock_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT64(clock, KVMClockState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static SysBusDeviceInfo kvmclock_info = {
> +    .qdev.name = "kvmclock",
> +    .qdev.size = sizeof(KVMClockState),
> +    .qdev.vmsd = &kvmclock_vmsd,
> +    .qdev.no_user = 1,
> +    .init = kvmclock_init,
> +};
> +
> +/* Note: Must be called after VCPU initialization. */
> +void kvmclock_create(void)
> +{
> +    if (kvm_enabled() &&
> +        first_cpu->cpuid_kvm_features & (1ULL << KVM_FEATURE_CLOCKSOURCE)) {
> +        sysbus_create_simple("kvmclock", -1, NULL);
> +    }
> +}

... and with this moved to a header as a static inline function, it
should be possible to use sysbus_try_create() (coming soon) to try to
create the device. Then it's not fatal if the device can't be created,
that just means that the capability was not available at build time.

Re: [Qemu-devel] [PATCH 15/15] kvm: x86: Introduce kvmclock device to save/restore its state

[Jan Kiszka]

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

On 2011-02-07 20:39, Blue Swirl wrote:
> On Mon, Feb 7, 2011 at 1:19 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> If kvmclock is used, which implies the kernel supports it, register a
>> kvmclock device with the sysbus. Its main purpose is to save and restore
>> the kernel state on migration, but this will also allow to visualize it
>> one day.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> CC: Glauber Costa <glommer@redhat.com>
>> ---
>>  Makefile.target |    4 +-
>>  hw/kvmclock.c   |  125 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  hw/kvmclock.h   |   14 ++++++
>>  hw/pc_piix.c    |   31 +++++++++++---
>>  4 files changed, 165 insertions(+), 9 deletions(-)
>>  create mode 100644 hw/kvmclock.c
>>  create mode 100644 hw/kvmclock.h
>>
>> diff --git a/Makefile.target b/Makefile.target
>> index b0ba95f..30232fa 100644
>> --- a/Makefile.target
>> +++ b/Makefile.target
>> @@ -37,7 +37,7 @@ ifndef CONFIG_HAIKU
>>  LIBS+=-lm
>>  endif
>>
>> -kvm.o kvm-all.o vhost.o vhost_net.o: QEMU_CFLAGS+=$(KVM_CFLAGS)
>> +kvm.o kvm-all.o vhost.o vhost_net.o kvmclock.o: QEMU_CFLAGS+=$(KVM_CFLAGS)
>>
>>  config-target.h: config-target.h-timestamp
>>  config-target.h-timestamp: config-target.mak
>> @@ -218,7 +218,7 @@ obj-i386-y += cirrus_vga.o apic.o ioapic.o piix_pci.o
>>  obj-i386-y += vmmouse.o vmport.o hpet.o applesmc.o
>>  obj-i386-y += device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.o
>>  obj-i386-y += debugcon.o multiboot.o
>> -obj-i386-y += pc_piix.o
>> +obj-i386-y += pc_piix.o kvmclock.o
> 
> Please build kvmclock.o conditionally to CONFIG_something...
> 
>>  obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
>>
>>  # shared objects
>> diff --git a/hw/kvmclock.c b/hw/kvmclock.c
>> new file mode 100644
>> index 0000000..b6ceddf
>> --- /dev/null
>> +++ b/hw/kvmclock.c
>> @@ -0,0 +1,125 @@
>> +/*
>> + * QEMU KVM support, paravirtual clock device
>> + *
>> + * Copyright (C) 2011 Siemens AG
>> + *
>> + * Authors:
>> + *  Jan Kiszka        <jan.kiszka@siemens.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL version 2.
>> + * See the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#include "qemu-common.h"
>> +#include "sysemu.h"
>> +#include "sysbus.h"
>> +#include "kvm.h"
>> +#include "kvmclock.h"
>> +
>> +#if defined(CONFIG_KVM_PARA) && defined(KVM_CAP_ADJUST_CLOCK)
>> +
>> +#include <linux/kvm.h>
>> +#include <linux/kvm_para.h>
>> +
>> +typedef struct KVMClockState {
>> +    SysBusDevice busdev;
>> +    uint64_t clock;
>> +    bool clock_valid;
>> +} KVMClockState;
>> +
>> +static void kvmclock_pre_save(void *opaque)
>> +{
>> +    KVMClockState *s = opaque;
>> +    struct kvm_clock_data data;
>> +    int ret;
>> +
>> +    if (s->clock_valid) {
>> +        return;
>> +    }
>> +    ret = kvm_vm_ioctl(kvm_state, KVM_GET_CLOCK, &data);
>> +    if (ret < 0) {
>> +        fprintf(stderr, "KVM_GET_CLOCK failed: %s\n", strerror(ret));
>> +        data.clock = 0;
>> +    }
>> +    s->clock = data.clock;
>> +    /*
>> +     * If the VM is stopped, declare the clock state valid to avoid re-reading
>> +     * it on next vmsave (which would return a different value). Will be reset
>> +     * when the VM is continued.
>> +     */
>> +    s->clock_valid = !vm_running;
>> +}
>> +
>> +static int kvmclock_post_load(void *opaque, int version_id)
>> +{
>> +    KVMClockState *s = opaque;
>> +    struct kvm_clock_data data;
>> +
>> +    data.clock = s->clock;
>> +    data.flags = 0;
>> +    return kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
>> +}
>> +
>> +static void kvmclock_vm_state_change(void *opaque, int running, int reason)
>> +{
>> +    KVMClockState *s = opaque;
>> +
>> +    if (running) {
>> +        s->clock_valid = false;
>> +    }
>> +}
>> +
>> +static int kvmclock_init(SysBusDevice *dev)
>> +{
>> +    KVMClockState *s = FROM_SYSBUS(KVMClockState, dev);
>> +
>> +    qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
>> +    return 0;
>> +}
>> +
>> +static const VMStateDescription kvmclock_vmsd = {
>> +    .name = "kvmclock",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .minimum_version_id_old = 1,
>> +    .pre_save = kvmclock_pre_save,
>> +    .post_load = kvmclock_post_load,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT64(clock, KVMClockState),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +static SysBusDeviceInfo kvmclock_info = {
>> +    .qdev.name = "kvmclock",
>> +    .qdev.size = sizeof(KVMClockState),
>> +    .qdev.vmsd = &kvmclock_vmsd,
>> +    .qdev.no_user = 1,
>> +    .init = kvmclock_init,
>> +};
>> +
>> +/* Note: Must be called after VCPU initialization. */
>> +void kvmclock_create(void)
>> +{
>> +    if (kvm_enabled() &&
>> +        first_cpu->cpuid_kvm_features & (1ULL << KVM_FEATURE_CLOCKSOURCE)) {
>> +        sysbus_create_simple("kvmclock", -1, NULL);
>> +    }
>> +}
> 
> ... and with this moved to a header as a static inline function, it
> should be possible to use sysbus_try_create() (coming soon) to try to
> create the device. Then it's not fatal if the device can't be created,
> that just means that the capability was not available at build time.

I played with this, and while it is generally a nice thing, it doesn't
help us here. We would just push the logic around, from kvmclock.c to
the header or even to configure (KVM_FEATURE_CLOCKSOURCE is not
unconditionally available).

I rather hope we finally agree on merging the required kvm headers into
qemu so that all this usually broken #ifdef KVM_CAP_* can be removed.

Jan


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