Patchwork [qom-cpu,v2,4/7] cpus: Pass CPUState to qemu_cpu_is_self()

login
register
mail settings
Submitter Andreas Färber
Date Oct. 12, 2012, 1:26 a.m.
Message ID <1350005203-15405-5-git-send-email-afaerber@suse.de>
Download mbox | patch
Permalink /patch/191040/
State New
Headers show

Comments

Andreas Färber - Oct. 12, 2012, 1:26 a.m.
Change return type to bool, move to include/qemu/cpu.h and
add documentation.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 cpus.c             |   10 ++++------
 exec.c             |    3 ++-
 hw/apic.c          |    6 ++++--
 include/qemu/cpu.h |   10 ++++++++++
 kvm-all.c          |    4 +++-
 qemu-common.h      |    1 -
 target-i386/kvm.c  |    6 ++++--
 7 Dateien geändert, 27 Zeilen hinzugefügt(+), 13 Zeilen entfernt(-)
Igor Mammedov - Oct. 16, 2012, 12:49 a.m.
On Fri, 12 Oct 2012 03:26:40 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Change return type to bool, move to include/qemu/cpu.h and
> add documentation.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  cpus.c             |   10 ++++------
>  exec.c             |    3 ++-
>  hw/apic.c          |    6 ++++--
>  include/qemu/cpu.h |   10 ++++++++++
>  kvm-all.c          |    4 +++-
>  qemu-common.h      |    1 -
>  target-i386/kvm.c  |    6 ++++--
>  7 Dateien geändert, 27 Zeilen hinzugefügt(+), 13 Zeilen entfernt(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 750a76f..849ea8a 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -638,9 +638,10 @@ void qemu_init_cpu_loop(void)
>  
>  void run_on_cpu(CPUArchState *env, void (*func)(void *data), void *data)
>  {
> +    CPUState *cpu = ENV_GET_CPU(env);
>      struct qemu_work_item wi;
>  
> -    if (qemu_cpu_is_self(env)) {
> +    if (qemu_cpu_is_self(cpu)) {
>          func(data);
>          return;
>      }
> @@ -855,7 +856,7 @@ static void qemu_cpu_kick_thread(CPUArchState *env)
>          exit(1);
>      }
>  #else /* _WIN32 */
> -    if (!qemu_cpu_is_self(env)) {
> +    if (!qemu_cpu_is_self(cpu)) {
>          SuspendThread(cpu->hThread);
>          cpu_signal(0);
>          ResumeThread(cpu->hThread);
> @@ -890,11 +891,8 @@ void qemu_cpu_kick_self(void)
>  #endif
>  }
>  
> -int qemu_cpu_is_self(void *_env)
> +bool qemu_cpu_is_self(CPUState *cpu)
>  {
> -    CPUArchState *env = _env;
> -    CPUState *cpu = ENV_GET_CPU(env);
> -
>      return qemu_thread_is_self(cpu->thread);
>  }
>  
> diff --git a/exec.c b/exec.c
> index 7899042..e21be32 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1685,6 +1685,7 @@ static void cpu_unlink_tb(CPUArchState *env)
>  /* mask must never be zero, except for A20 change call */
>  static void tcg_handle_interrupt(CPUArchState *env, int mask)
>  {
> +    CPUState *cpu = ENV_GET_CPU(env);
Is there any chance to get rid of expensive cast on this call path?

>      int old_mask;
>  
>      old_mask = env->interrupt_request;
> @@ -1694,7 +1695,7 @@ static void tcg_handle_interrupt(CPUArchState *env, int mask)
>       * If called from iothread context, wake the target cpu in
>       * case its halted.
>       */
> -    if (!qemu_cpu_is_self(env)) {
> +    if (!qemu_cpu_is_self(cpu)) {
>          qemu_cpu_kick(env);
>          return;
>      }
> diff --git a/hw/apic.c b/hw/apic.c
> index ccf2819..1b4cd2f 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -107,7 +107,7 @@ static void apic_sync_vapic(APICCommonState *s, int sync_type)
>          length = offsetof(VAPICState, enabled) - offsetof(VAPICState, isr);
>  
>          if (sync_type & SYNC_TO_VAPIC) {
> -            assert(qemu_cpu_is_self(&s->cpu->env));
> +            assert(qemu_cpu_is_self(CPU(s->cpu)));
>  
>              vapic_state.tpr = s->tpr;
>              vapic_state.enabled = 1;
> @@ -363,10 +363,12 @@ static int apic_irq_pending(APICCommonState *s)
>  /* signal the CPU if an irq is pending */
>  static void apic_update_irq(APICCommonState *s)
>  {
> +    CPUState *cpu = CPU(s->cpu);
> +
>      if (!(s->spurious_vec & APIC_SV_ENABLE)) {
>          return;
>      }
> -    if (!qemu_cpu_is_self(&s->cpu->env)) {
> +    if (!qemu_cpu_is_self(cpu)) {
>          cpu_interrupt(&s->cpu->env, CPU_INTERRUPT_POLL);
>      } else if (apic_irq_pending(s) > 0) {
>          cpu_interrupt(&s->cpu->env, CPU_INTERRUPT_HARD);
> diff --git a/include/qemu/cpu.h b/include/qemu/cpu.h
> index ad706a6..7be983d 100644
> --- a/include/qemu/cpu.h
> +++ b/include/qemu/cpu.h
> @@ -78,5 +78,15 @@ struct CPUState {
>   */
>  void cpu_reset(CPUState *cpu);
>  
> +/**
> + * qemu_cpu_is_self:
> + * @cpu: The vCPU to check against.
> + *
> + * Checks whether the caller is executing on the vCPU thread.
> + *
> + * Returns: %true if called from @cpu's thread, %false otherwise.
> + */
> +bool qemu_cpu_is_self(CPUState *cpu);
> +
>  
>  #endif
> diff --git a/kvm-all.c b/kvm-all.c
> index 92a7137..db01aeb 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -854,9 +854,11 @@ static MemoryListener kvm_memory_listener = {
>  
>  static void kvm_handle_interrupt(CPUArchState *env, int mask)
>  {
> +    CPUState *cpu = ENV_GET_CPU(env);
> +
>      env->interrupt_request |= mask;
>  
> -    if (!qemu_cpu_is_self(env)) {
> +    if (!qemu_cpu_is_self(cpu)) {
>          qemu_cpu_kick(env);
>      }
>  }
> diff --git a/qemu-common.h b/qemu-common.h
> index b54612b..2094742 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -326,7 +326,6 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id);
>  /* Unblock cpu */
>  void qemu_cpu_kick(void *env);
>  void qemu_cpu_kick_self(void);
> -int qemu_cpu_is_self(void *env);
>  
>  /* work queue */
>  struct qemu_work_item {
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 5b18383..cf3d2f1 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -1552,9 +1552,10 @@ static int kvm_get_debugregs(CPUX86State *env)
>  
>  int kvm_arch_put_registers(CPUX86State *env, int level)
>  {
> +    CPUState *cpu = ENV_GET_CPU(env);
>      int ret;
>  
> -    assert(cpu_is_stopped(env) || qemu_cpu_is_self(env));
> +    assert(cpu_is_stopped(env) || qemu_cpu_is_self(cpu));
>  
>      ret = kvm_getput_regs(env, 1);
>      if (ret < 0) {
> @@ -1609,9 +1610,10 @@ int kvm_arch_put_registers(CPUX86State *env, int level)
>  
>  int kvm_arch_get_registers(CPUX86State *env)
>  {
> +    CPUState *cpu = ENV_GET_CPU(env);
>      int ret;
>  
> -    assert(cpu_is_stopped(env) || qemu_cpu_is_self(env));
> +    assert(cpu_is_stopped(env) || qemu_cpu_is_self(cpu));
>  
>      ret = kvm_getput_regs(env, 0);
>      if (ret < 0) {
> -- 
> 1.7.10.4
> 
> 
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Andreas Färber - Oct. 30, 2012, 8:08 p.m.
Am 16.10.2012 02:49, schrieb Igor Mammedov:
> On Fri, 12 Oct 2012 03:26:40 +0200
> Andreas Färber <afaerber@suse.de> wrote:
> 
>> Change return type to bool, move to include/qemu/cpu.h and
>> add documentation.
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>>  cpus.c             |   10 ++++------
>>  exec.c             |    3 ++-
>>  hw/apic.c          |    6 ++++--
>>  include/qemu/cpu.h |   10 ++++++++++
>>  kvm-all.c          |    4 +++-
>>  qemu-common.h      |    1 -
>>  target-i386/kvm.c  |    6 ++++--
>>  7 Dateien geändert, 27 Zeilen hinzugefügt(+), 13 Zeilen entfernt(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index 750a76f..849ea8a 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -638,9 +638,10 @@ void qemu_init_cpu_loop(void)
>>  
>>  void run_on_cpu(CPUArchState *env, void (*func)(void *data), void *data)
>>  {
>> +    CPUState *cpu = ENV_GET_CPU(env);
>>      struct qemu_work_item wi;
>>  
>> -    if (qemu_cpu_is_self(env)) {
>> +    if (qemu_cpu_is_self(cpu)) {
>>          func(data);
>>          return;
>>      }
>> @@ -855,7 +856,7 @@ static void qemu_cpu_kick_thread(CPUArchState *env)
>>          exit(1);
>>      }
>>  #else /* _WIN32 */
>> -    if (!qemu_cpu_is_self(env)) {
>> +    if (!qemu_cpu_is_self(cpu)) {
>>          SuspendThread(cpu->hThread);
>>          cpu_signal(0);
>>          ResumeThread(cpu->hThread);
>> @@ -890,11 +891,8 @@ void qemu_cpu_kick_self(void)
>>  #endif
>>  }
>>  
>> -int qemu_cpu_is_self(void *_env)
>> +bool qemu_cpu_is_self(CPUState *cpu)
>>  {
>> -    CPUArchState *env = _env;
>> -    CPUState *cpu = ENV_GET_CPU(env);
>> -
>>      return qemu_thread_is_self(cpu->thread);
>>  }
>>  
>> diff --git a/exec.c b/exec.c
>> index 7899042..e21be32 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -1685,6 +1685,7 @@ static void cpu_unlink_tb(CPUArchState *env)
>>  /* mask must never be zero, except for A20 change call */
>>  static void tcg_handle_interrupt(CPUArchState *env, int mask)
>>  {
>> +    CPUState *cpu = ENV_GET_CPU(env);
> Is there any chance to get rid of expensive cast on this call path?

I've spent some time investigating this and it would be rather complicated:

* We can't change the argument to CPUState yet (pushing the
responsibility to the caller) since interrupt_request is being accessed
til the not-yet-committable end of original part 4 series, the TB is
being flushed, icount being handled, and it is being used for the first
if, so not movable to a deeper block.

* A pointer to this function (or the KVM version below) is stored as
cpu_interrupt_handler and accessed through cpu_interrupt(), which for
softmmu calls the handler and for *-user does something similar to the
TCG version. This function is being used in a lot of places, so adding a
CPUState argument in addition to the CPUArchState would touch a lot of
places for an interim solution.

* QOM makes no guarantees about subclass layout (.env placement), so
going from CPUState to CPUArchState involves quite some ugly #ifdef'fery
that I am trying to avoid, going only the I-know-my-type direction.

I'll therefore accept your Reviewed-by to apply this and will keep
working during the Soft Freeze on the follow-ups that allow changing
this and others completely over to CPUState. Alex has just given green
light for the ppc prerequisites so I can apply them together with xtensa
to qom-cpu and post the next batch if I haven't already.

Regards,
Andreas

> 
>>      int old_mask;
>>  
>>      old_mask = env->interrupt_request;
>> @@ -1694,7 +1695,7 @@ static void tcg_handle_interrupt(CPUArchState *env, int mask)
>>       * If called from iothread context, wake the target cpu in
>>       * case its halted.
>>       */
>> -    if (!qemu_cpu_is_self(env)) {
>> +    if (!qemu_cpu_is_self(cpu)) {
>>          qemu_cpu_kick(env);
>>          return;
>>      }
>> diff --git a/hw/apic.c b/hw/apic.c
>> index ccf2819..1b4cd2f 100644
>> --- a/hw/apic.c
>> +++ b/hw/apic.c
>> @@ -107,7 +107,7 @@ static void apic_sync_vapic(APICCommonState *s, int sync_type)
>>          length = offsetof(VAPICState, enabled) - offsetof(VAPICState, isr);
>>  
>>          if (sync_type & SYNC_TO_VAPIC) {
>> -            assert(qemu_cpu_is_self(&s->cpu->env));
>> +            assert(qemu_cpu_is_self(CPU(s->cpu)));
>>  
>>              vapic_state.tpr = s->tpr;
>>              vapic_state.enabled = 1;
>> @@ -363,10 +363,12 @@ static int apic_irq_pending(APICCommonState *s)
>>  /* signal the CPU if an irq is pending */
>>  static void apic_update_irq(APICCommonState *s)
>>  {
>> +    CPUState *cpu = CPU(s->cpu);
>> +
>>      if (!(s->spurious_vec & APIC_SV_ENABLE)) {
>>          return;
>>      }
>> -    if (!qemu_cpu_is_self(&s->cpu->env)) {
>> +    if (!qemu_cpu_is_self(cpu)) {
>>          cpu_interrupt(&s->cpu->env, CPU_INTERRUPT_POLL);
>>      } else if (apic_irq_pending(s) > 0) {
>>          cpu_interrupt(&s->cpu->env, CPU_INTERRUPT_HARD);
>> diff --git a/include/qemu/cpu.h b/include/qemu/cpu.h
>> index ad706a6..7be983d 100644
>> --- a/include/qemu/cpu.h
>> +++ b/include/qemu/cpu.h
>> @@ -78,5 +78,15 @@ struct CPUState {
>>   */
>>  void cpu_reset(CPUState *cpu);
>>  
>> +/**
>> + * qemu_cpu_is_self:
>> + * @cpu: The vCPU to check against.
>> + *
>> + * Checks whether the caller is executing on the vCPU thread.
>> + *
>> + * Returns: %true if called from @cpu's thread, %false otherwise.
>> + */
>> +bool qemu_cpu_is_self(CPUState *cpu);
>> +
>>  
>>  #endif
>> diff --git a/kvm-all.c b/kvm-all.c
>> index 92a7137..db01aeb 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -854,9 +854,11 @@ static MemoryListener kvm_memory_listener = {
>>  
>>  static void kvm_handle_interrupt(CPUArchState *env, int mask)
>>  {
>> +    CPUState *cpu = ENV_GET_CPU(env);
>> +
>>      env->interrupt_request |= mask;
>>  
>> -    if (!qemu_cpu_is_self(env)) {
>> +    if (!qemu_cpu_is_self(cpu)) {
>>          qemu_cpu_kick(env);
>>      }
>>  }
>> diff --git a/qemu-common.h b/qemu-common.h
>> index b54612b..2094742 100644
>> --- a/qemu-common.h
>> +++ b/qemu-common.h
>> @@ -326,7 +326,6 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id);
>>  /* Unblock cpu */
>>  void qemu_cpu_kick(void *env);
>>  void qemu_cpu_kick_self(void);
>> -int qemu_cpu_is_self(void *env);
>>  
>>  /* work queue */
>>  struct qemu_work_item {
>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
>> index 5b18383..cf3d2f1 100644
>> --- a/target-i386/kvm.c
>> +++ b/target-i386/kvm.c
>> @@ -1552,9 +1552,10 @@ static int kvm_get_debugregs(CPUX86State *env)
>>  
>>  int kvm_arch_put_registers(CPUX86State *env, int level)
>>  {
>> +    CPUState *cpu = ENV_GET_CPU(env);
>>      int ret;
>>  
>> -    assert(cpu_is_stopped(env) || qemu_cpu_is_self(env));
>> +    assert(cpu_is_stopped(env) || qemu_cpu_is_self(cpu));
>>  
>>      ret = kvm_getput_regs(env, 1);
>>      if (ret < 0) {
>> @@ -1609,9 +1610,10 @@ int kvm_arch_put_registers(CPUX86State *env, int level)
>>  
>>  int kvm_arch_get_registers(CPUX86State *env)
>>  {
>> +    CPUState *cpu = ENV_GET_CPU(env);
>>      int ret;
>>  
>> -    assert(cpu_is_stopped(env) || qemu_cpu_is_self(env));
>> +    assert(cpu_is_stopped(env) || qemu_cpu_is_self(cpu));
>>  
>>      ret = kvm_getput_regs(env, 0);
>>      if (ret < 0) {
>> -- 
>> 1.7.10.4
>>
>>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>

Patch

diff --git a/cpus.c b/cpus.c
index 750a76f..849ea8a 100644
--- a/cpus.c
+++ b/cpus.c
@@ -638,9 +638,10 @@  void qemu_init_cpu_loop(void)
 
 void run_on_cpu(CPUArchState *env, void (*func)(void *data), void *data)
 {
+    CPUState *cpu = ENV_GET_CPU(env);
     struct qemu_work_item wi;
 
-    if (qemu_cpu_is_self(env)) {
+    if (qemu_cpu_is_self(cpu)) {
         func(data);
         return;
     }
@@ -855,7 +856,7 @@  static void qemu_cpu_kick_thread(CPUArchState *env)
         exit(1);
     }
 #else /* _WIN32 */
-    if (!qemu_cpu_is_self(env)) {
+    if (!qemu_cpu_is_self(cpu)) {
         SuspendThread(cpu->hThread);
         cpu_signal(0);
         ResumeThread(cpu->hThread);
@@ -890,11 +891,8 @@  void qemu_cpu_kick_self(void)
 #endif
 }
 
-int qemu_cpu_is_self(void *_env)
+bool qemu_cpu_is_self(CPUState *cpu)
 {
-    CPUArchState *env = _env;
-    CPUState *cpu = ENV_GET_CPU(env);
-
     return qemu_thread_is_self(cpu->thread);
 }
 
diff --git a/exec.c b/exec.c
index 7899042..e21be32 100644
--- a/exec.c
+++ b/exec.c
@@ -1685,6 +1685,7 @@  static void cpu_unlink_tb(CPUArchState *env)
 /* mask must never be zero, except for A20 change call */
 static void tcg_handle_interrupt(CPUArchState *env, int mask)
 {
+    CPUState *cpu = ENV_GET_CPU(env);
     int old_mask;
 
     old_mask = env->interrupt_request;
@@ -1694,7 +1695,7 @@  static void tcg_handle_interrupt(CPUArchState *env, int mask)
      * If called from iothread context, wake the target cpu in
      * case its halted.
      */
-    if (!qemu_cpu_is_self(env)) {
+    if (!qemu_cpu_is_self(cpu)) {
         qemu_cpu_kick(env);
         return;
     }
diff --git a/hw/apic.c b/hw/apic.c
index ccf2819..1b4cd2f 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -107,7 +107,7 @@  static void apic_sync_vapic(APICCommonState *s, int sync_type)
         length = offsetof(VAPICState, enabled) - offsetof(VAPICState, isr);
 
         if (sync_type & SYNC_TO_VAPIC) {
-            assert(qemu_cpu_is_self(&s->cpu->env));
+            assert(qemu_cpu_is_self(CPU(s->cpu)));
 
             vapic_state.tpr = s->tpr;
             vapic_state.enabled = 1;
@@ -363,10 +363,12 @@  static int apic_irq_pending(APICCommonState *s)
 /* signal the CPU if an irq is pending */
 static void apic_update_irq(APICCommonState *s)
 {
+    CPUState *cpu = CPU(s->cpu);
+
     if (!(s->spurious_vec & APIC_SV_ENABLE)) {
         return;
     }
-    if (!qemu_cpu_is_self(&s->cpu->env)) {
+    if (!qemu_cpu_is_self(cpu)) {
         cpu_interrupt(&s->cpu->env, CPU_INTERRUPT_POLL);
     } else if (apic_irq_pending(s) > 0) {
         cpu_interrupt(&s->cpu->env, CPU_INTERRUPT_HARD);
diff --git a/include/qemu/cpu.h b/include/qemu/cpu.h
index ad706a6..7be983d 100644
--- a/include/qemu/cpu.h
+++ b/include/qemu/cpu.h
@@ -78,5 +78,15 @@  struct CPUState {
  */
 void cpu_reset(CPUState *cpu);
 
+/**
+ * qemu_cpu_is_self:
+ * @cpu: The vCPU to check against.
+ *
+ * Checks whether the caller is executing on the vCPU thread.
+ *
+ * Returns: %true if called from @cpu's thread, %false otherwise.
+ */
+bool qemu_cpu_is_self(CPUState *cpu);
+
 
 #endif
diff --git a/kvm-all.c b/kvm-all.c
index 92a7137..db01aeb 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -854,9 +854,11 @@  static MemoryListener kvm_memory_listener = {
 
 static void kvm_handle_interrupt(CPUArchState *env, int mask)
 {
+    CPUState *cpu = ENV_GET_CPU(env);
+
     env->interrupt_request |= mask;
 
-    if (!qemu_cpu_is_self(env)) {
+    if (!qemu_cpu_is_self(cpu)) {
         qemu_cpu_kick(env);
     }
 }
diff --git a/qemu-common.h b/qemu-common.h
index b54612b..2094742 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -326,7 +326,6 @@  int cpu_load(QEMUFile *f, void *opaque, int version_id);
 /* Unblock cpu */
 void qemu_cpu_kick(void *env);
 void qemu_cpu_kick_self(void);
-int qemu_cpu_is_self(void *env);
 
 /* work queue */
 struct qemu_work_item {
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 5b18383..cf3d2f1 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1552,9 +1552,10 @@  static int kvm_get_debugregs(CPUX86State *env)
 
 int kvm_arch_put_registers(CPUX86State *env, int level)
 {
+    CPUState *cpu = ENV_GET_CPU(env);
     int ret;
 
-    assert(cpu_is_stopped(env) || qemu_cpu_is_self(env));
+    assert(cpu_is_stopped(env) || qemu_cpu_is_self(cpu));
 
     ret = kvm_getput_regs(env, 1);
     if (ret < 0) {
@@ -1609,9 +1610,10 @@  int kvm_arch_put_registers(CPUX86State *env, int level)
 
 int kvm_arch_get_registers(CPUX86State *env)
 {
+    CPUState *cpu = ENV_GET_CPU(env);
     int ret;
 
-    assert(cpu_is_stopped(env) || qemu_cpu_is_self(env));
+    assert(cpu_is_stopped(env) || qemu_cpu_is_self(cpu));
 
     ret = kvm_getput_regs(env, 0);
     if (ret < 0) {