Re: [Qemu-devel] [RFC qom-next v5 1/8] x86: move apic_state field from CPUX86State to X86CPU

[Andreas Färber]

Am 23.12.2013 10:04, schrieb Chen Fan:
> This motion is preparing for refactoring vCPU apic subsequently.
> 
> Signed-off-by: Chen Fan <address@hidden>
> ---
>  cpu-exec.c                |  2 +-
>  cpus.c                    |  5 ++---
>  hw/i386/kvmvapic.c        |  8 +++-----
>  hw/i386/pc.c              | 17 ++++++++---------
>  target-i386/cpu-qom.h     |  4 ++++
>  target-i386/cpu.c         | 22 ++++++++++------------
>  target-i386/cpu.h         |  4 ----
>  target-i386/helper.c      |  9 ++++-----
>  target-i386/kvm.c         | 23 ++++++++++-------------
>  target-i386/misc_helper.c |  8 ++++----
>  10 files changed, 46 insertions(+), 56 deletions(-)
> 
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 30cfa2a..2711c58 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -320,7 +320,7 @@ int cpu_exec(CPUArchState *env)
>  #if !defined(CONFIG_USER_ONLY)
>                      if (interrupt_request & CPU_INTERRUPT_POLL) {
>                          cpu->interrupt_request &= ~CPU_INTERRUPT_POLL;
> -                        apic_poll_irq(env->apic_state);
> +                        apic_poll_irq(x86_env_get_cpu(env)->apic_state);

These are starting to become too many inline usages inside that double
loop, I'll look into providing a follow-up patch to clean this up.

>                      }
>  #endif
>                      if (interrupt_request & CPU_INTERRUPT_INIT) {
[...]
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index e9831ca..d000995 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -172,13 +172,14 @@ void cpu_smm_update(CPUX86State *env)
>  int cpu_get_pic_interrupt(CPUX86State *env)
>  {
>      int intno;
> +    X86CPU *cpu = x86_env_get_cpu(env);

I've swapped these two lines to keep cpu and env close together, with a
view to a function argument type change.

>  
> -    intno = apic_get_interrupt(env->apic_state);
> +    intno = apic_get_interrupt(cpu->apic_state);
>      if (intno >= 0) {
>          return intno;
>      }
>      /* read the irq from the PIC */
> -    if (!apic_accept_pic_intr(env->apic_state)) {
> +    if (!apic_accept_pic_intr(cpu->apic_state)) {
>          return -1;
>      }
>  
[...]
> diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
> index f4fab15..775c82d 100644
> --- a/target-i386/cpu-qom.h
> +++ b/target-i386/cpu-qom.h
> @@ -66,6 +66,10 @@ typedef struct X86CPU {
>  
>      CPUX86State env;
>  
> +    /* in order to simplify APIC support, we leave this pointer to the
> +       user */
> +    struct DeviceState *apic_state;

Moving this further down since used as a child<> property, with a view
to refactoring this further into a non-pointer field.

> +
>      bool hyperv_vapic;
>      bool hyperv_relaxed_timing;
>      int hyperv_spinlock_attempts;
[...]
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index 7c196ff..f2e76ad 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1248,7 +1248,8 @@ void cpu_report_tpr_access(CPUX86State *env, TPRAccess 
> access)
>      } else {
>          cpu_restore_state(env, env->mem_io_pc);
>  
> -        apic_handle_tpr_access_report(env->apic_state, env->eip, access);
> +        apic_handle_tpr_access_report(x86_env_get_cpu(env)->apic_state,
> +                                      env->eip, access);
>      }
>  }
>  #endif /* !CONFIG_USER_ONLY */
[snip]

Since we would now be using x86_env_get_cpu() in both arms of 'if' (and
tpr_access_type being another candidate for a field movement), I'm
changing this as follows:

diff --git a/target-i386/helper.c b/target-i386/helper.c
index f2e76ad..8132ca8 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1241,15 +1241,16 @@ void cpu_x86_inject_mce(Monitor *mon, X86CPU
*cpu, int bank,

 void cpu_report_tpr_access(CPUX86State *env, TPRAccess access)
 {
+    X86CPU *cpu = x86_env_get_cpu(env);
+
     if (kvm_enabled()) {
         env->tpr_access_type = access;

-        cpu_interrupt(CPU(x86_env_get_cpu(env)), CPU_INTERRUPT_TPR);
+        cpu_interrupt(CPU(cpu), CPU_INTERRUPT_TPR);
     } else {
         cpu_restore_state(env, env->mem_io_pc);

-        apic_handle_tpr_access_report(x86_env_get_cpu(env)->apic_state,
-                                      env->eip, access);
+        apic_handle_tpr_access_report(cpu->apic_state, env->eip, access);
     }
 }
 #endif /* !CONFIG_USER_ONLY */


Despite this still being an RFC, this patch is a really nice cleanup
contribution, so I'm applying this to qom-cpu already with the
above-mentioned modifications:
https://github.com/afaerber/qemu-cpu/commits/qom-cpu

Thanks,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg