diff mbox

kvm: Simplify cpu_synchronize_state()

Message ID 1250540393-3998-1-git-send-email-avi@redhat.com
State Superseded
Headers show

Commit Message

Avi Kivity Aug. 17, 2009, 8:19 p.m. UTC 
cpu_synchronize_state() is a little unreadable since the 'modified'
argument isn't self-explanatory.  Simplify it by making it always
synchronize the kernel state into qemu, and automatically flush the
registers back to the kernel if they've been synchronized on this
exit.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 exec.c                |    4 ++--
 gdbstub.c             |    8 ++++----
 hw/apic.c             |    7 ++++---
 kvm-all.c             |   14 ++++++++++++++
 kvm.h                 |    8 +++-----
 monitor.c             |    4 ++--
 target-i386/machine.c |    4 ++--
 target-ppc/machine.c  |    6 +++---
 8 files changed, 34 insertions(+), 21 deletions(-)

Comments

Reimar Döffinger Aug. 17, 2009, 8:52 p.m. UTC | #1 
On Mon, Aug 17, 2009 at 11:19:53PM +0300, Avi Kivity wrote:
> diff --git a/kvm-all.c b/kvm-all.c
> index f669c3a..15c30d4 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -57,6 +57,7 @@ struct KVMState
>      KVMSlot slots[32];
>      int fd;
>      int vmfd;
> +    int regs_modified;
>      int coalesced_mmio;
>      int broken_set_mem_region;
>      int migration_log;

I think regs_modified is a really bad name since it has nothing at all
to do with whether the registers were modified.
IMO it actually indicates if the register copy in the KVM or in CPUState
is valid.
So maybe cpustate_regs_valid is a more straight-forward name? (implying
that when the cpustate regs are valid, the kvm ones probably aren't -
though the way qemu access registers we can't know).
One disadvantage is that code that only needs to read registers will now
uselessly do kvm_arch_put_registers.
A fancy way to do it would be to add a "mode" flag indicating if we only
want read or read-write access to registers, but since we can't enforce
that it might be a bad idea.
Avi Kivity Aug. 18, 2009, 7:51 a.m. UTC | #2 
On 08/17/2009 11:52 PM, Reimar Döffinger wrote:
> On Mon, Aug 17, 2009 at 11:19:53PM +0300, Avi Kivity wrote:
>    
>> diff --git a/kvm-all.c b/kvm-all.c
>> index f669c3a..15c30d4 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -57,6 +57,7 @@ struct KVMState
>>       KVMSlot slots[32];
>>       int fd;
>>       int vmfd;
>> +    int regs_modified;
>>       int coalesced_mmio;
>>       int broken_set_mem_region;
>>       int migration_log;
>>      
> I think regs_modified is a really bad name since it has nothing at all
> to do with whether the registers were modified.
> IMO it actually indicates if the register copy in the KVM or in CPUState
> is valid.
> So maybe cpustate_regs_valid is a more straight-forward name? (implying
> that when the cpustate regs are valid, the kvm ones probably aren't -
> though the way qemu access registers we can't know).
>    


I'm fine with such a change.

> One disadvantage is that code that only needs to read registers will now
> uselessly do kvm_arch_put_registers.
> A fancy way to do it would be to add a "mode" flag indicating if we only
> want read or read-write access to registers, but since we can't enforce
> that it might be a bad idea.
>    

This is so rare it's pointless to optimize.
diff mbox

Patch

diff --git a/exec.c b/exec.c
index fa5b619..dcfe3eb 100644
--- a/exec.c
+++ b/exec.c
@@ -520,7 +520,7 @@  static void cpu_common_save(QEMUFile *f, void *opaque)
 {
     CPUState *env = opaque;
 
-    cpu_synchronize_state(env, 0);
+    cpu_synchronize_state(env);
 
     qemu_put_be32s(f, &env->halted);
     qemu_put_be32s(f, &env->interrupt_request);
@@ -530,6 +530,7 @@  static int cpu_common_load(QEMUFile *f, void *opaque, int version_id)
 {
     CPUState *env = opaque;
 
+    cpu_synchronize_state(env);
     if (version_id != CPU_COMMON_SAVE_VERSION)
         return -EINVAL;
 
@@ -539,7 +540,6 @@  static int cpu_common_load(QEMUFile *f, void *opaque, int version_id)
        version_id is increased. */
     env->interrupt_request &= ~0x01;
     tlb_flush(env, 1);
-    cpu_synchronize_state(env, 1);
 
     return 0;
 }
diff --git a/gdbstub.c b/gdbstub.c
index ff4c86c..33d79eb 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1568,8 +1568,8 @@  static void gdb_breakpoint_remove_all(void)
 static void gdb_set_cpu_pc(GDBState *s, target_ulong pc)
 {
 #if defined(TARGET_I386)
+    cpu_synchronize_state(s->c_cpu);
     s->c_cpu->eip = pc;
-    cpu_synchronize_state(s->c_cpu, 1);
 #elif defined (TARGET_PPC)
     s->c_cpu->nip = pc;
 #elif defined (TARGET_SPARC)
@@ -1755,7 +1755,7 @@  static int gdb_handle_packet(GDBState *s, const char *line_buf)
         }
         break;
     case 'g':
-        cpu_synchronize_state(s->g_cpu, 0);
+        cpu_synchronize_state(s->g_cpu);
         len = 0;
         for (addr = 0; addr < num_g_regs; addr++) {
             reg_size = gdb_read_register(s->g_cpu, mem_buf + len, addr);
@@ -1765,6 +1765,7 @@  static int gdb_handle_packet(GDBState *s, const char *line_buf)
         put_packet(s, buf);
         break;
     case 'G':
+        cpu_synchronize_state(s->g_cpu);
         registers = mem_buf;
         len = strlen(p) / 2;
         hextomem((uint8_t *)registers, p, len);
@@ -1773,7 +1774,6 @@  static int gdb_handle_packet(GDBState *s, const char *line_buf)
             len -= reg_size;
             registers += reg_size;
         }
-        cpu_synchronize_state(s->g_cpu, 1);
         put_packet(s, "OK");
         break;
     case 'm':
@@ -1929,7 +1929,7 @@  static int gdb_handle_packet(GDBState *s, const char *line_buf)
             thread = strtoull(p+16, (char **)&p, 16);
             env = find_cpu(thread);
             if (env != NULL) {
-                cpu_synchronize_state(env, 0);
+                cpu_synchronize_state(env);
                 len = snprintf((char *)mem_buf, sizeof(mem_buf),
                                "CPU#%d [%s]", env->cpu_index,
                                env->halted ? "halted " : "running");
diff --git a/hw/apic.c b/hw/apic.c
index 1927811..0c5bc33 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -937,8 +937,11 @@  static int apic_load(QEMUFile *f, void *opaque, int version_id)
 static void apic_reset(void *opaque)
 {
     APICState *s = opaque;
-    int bsp = cpu_is_bsp(s->cpu_env);
+    int bsp;
 
+    cpu_synchronize_state(s->cpu_env);
+
+    bsp = cpu_is_bsp(s->cpu_env);
     s->apicbase = 0xfee00000 |
         (bsp ? MSR_IA32_APICBASE_BSP : 0) | MSR_IA32_APICBASE_ENABLE;
 
@@ -953,8 +956,6 @@  static void apic_reset(void *opaque)
          */
         s->lvt[APIC_LVT_LINT0] = 0x700;
     }
-
-    cpu_synchronize_state(s->cpu_env, 1);
 }
 
 static CPUReadMemoryFunc *apic_mem_read[3] = {
diff --git a/kvm-all.c b/kvm-all.c
index f669c3a..15c30d4 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -57,6 +57,7 @@  struct KVMState
     KVMSlot slots[32];
     int fd;
     int vmfd;
+    int regs_modified;
     int coalesced_mmio;
     int broken_set_mem_region;
     int migration_log;
@@ -576,6 +577,14 @@  static void kvm_run_coalesced_mmio(CPUState *env, struct kvm_run *run)
 #endif
 }
 
+void kvm_cpu_synchronize_state(CPUState *env)
+{
+    if (!env->kvm_state->regs_modified) {
+        kvm_arch_get_registers(env);
+        env->kvm_state->regs_modified = 1;
+    }
+}
+
 int kvm_cpu_exec(CPUState *env)
 {
     struct kvm_run *run = env->kvm_run;
@@ -590,6 +599,11 @@  int kvm_cpu_exec(CPUState *env)
             break;
         }
 
+        if (env->kvm_state->regs_modified) {
+            kvm_arch_put_registers(env);
+            env->kvm_state->regs_modified = 0;
+        }
+
         kvm_arch_pre_run(env, run);
         ret = kvm_vcpu_ioctl(env, KVM_RUN, 0);
         kvm_arch_post_run(env, run);
diff --git a/kvm.h b/kvm.h
index 0d4dd53..dbe825f 100644
--- a/kvm.h
+++ b/kvm.h
@@ -128,16 +128,14 @@  int kvm_check_extension(KVMState *s, unsigned int extension);
 
 uint32_t kvm_arch_get_supported_cpuid(CPUState *env, uint32_t function,
                                       int reg);
+void kvm_cpu_synchronize_state(CPUState *env);
 
 /* generic hooks - to be moved/refactored once there are more users */
 
-static inline void cpu_synchronize_state(CPUState *env, int modified)
+static inline void cpu_synchronize_state(CPUState *env)
 {
     if (kvm_enabled()) {
-        if (modified)
-            kvm_arch_put_registers(env);
-        else
-            kvm_arch_get_registers(env);
+        kvm_cpu_synchronize_state(env);
     }
 }
 
diff --git a/monitor.c b/monitor.c
index 362322b..30f050e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -331,7 +331,7 @@  static CPUState *mon_get_cpu(void)
     if (!cur_mon->mon_cpu) {
         mon_set_cpu(0);
     }
-    cpu_synchronize_state(cur_mon->mon_cpu, 0);
+    cpu_synchronize_state(cur_mon->mon_cpu);
     return cur_mon->mon_cpu;
 }
 
@@ -358,7 +358,7 @@  static void do_info_cpus(Monitor *mon)
     mon_get_cpu();
 
     for(env = first_cpu; env != NULL; env = env->next_cpu) {
-        cpu_synchronize_state(env, 0);
+        cpu_synchronize_state(env);
         monitor_printf(mon, "%c CPU #%d:",
                        (env == mon->mon_cpu) ? '*' : ' ',
                        env->cpu_index);
diff --git a/target-i386/machine.c b/target-i386/machine.c
index 8bf13cc..ab31329 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -32,7 +32,7 @@  void cpu_save(QEMUFile *f, void *opaque)
     int32_t pending_irq;
     int i, bit;
 
-    cpu_synchronize_state(env, 0);
+    cpu_synchronize_state(env);
 
     for(i = 0; i < CPU_NB_REGS; i++)
         qemu_put_betls(f, &env->regs[i]);
@@ -206,6 +206,7 @@  int cpu_load(QEMUFile *f, void *opaque, int version_id)
     int32_t a20_mask;
     int32_t pending_irq;
 
+    cpu_synchronize_state(env);
     if (version_id < 3 || version_id > CPU_SAVE_VERSION)
         return -EINVAL;
     for(i = 0; i < CPU_NB_REGS; i++)
@@ -380,6 +381,5 @@  int cpu_load(QEMUFile *f, void *opaque, int version_id)
     /* XXX: compute redundant hflags bits */
     env->hflags = hflags;
     tlb_flush(env, 1);
-    cpu_synchronize_state(env, 1);
     return 0;
 }
diff --git a/target-ppc/machine.c b/target-ppc/machine.c
index deb2e2d..4897c8a 100644
--- a/target-ppc/machine.c
+++ b/target-ppc/machine.c
@@ -7,7 +7,7 @@  void cpu_save(QEMUFile *f, void *opaque)
     CPUState *env = (CPUState *)opaque;
     unsigned int i, j;
 
-    cpu_synchronize_state(env, 0);
+    cpu_synchronize_state(env);
 
     for (i = 0; i < 32; i++)
         qemu_put_betls(f, &env->gpr[i]);
@@ -96,6 +96,8 @@  int cpu_load(QEMUFile *f, void *opaque, int version_id)
     CPUState *env = (CPUState *)opaque;
     unsigned int i, j;
 
+    cpu_synchronize_state(env);
+
     for (i = 0; i < 32; i++)
         qemu_get_betls(f, &env->gpr[i]);
 #if !defined(TARGET_PPC64)
@@ -177,7 +179,5 @@  int cpu_load(QEMUFile *f, void *opaque, int version_id)
     qemu_get_sbe32s(f, &env->mmu_idx);
     qemu_get_sbe32s(f, &env->power_mode);
 
-    cpu_synchronize_state(env, 1);
-
     return 0;
 }