[Qemu-devel] [PATCH 6/6] apic: avoid using CPUState internals

[Qemu-devel] [PATCH 6/6] apic: avoid using CPUState internals

[Blue Swirl]

Use only an opaque CPUState pointer and move the actual CPUState
contents handling to cpu.h and cpuid.c.

Set env->halted in pc.c and add a function to get the local APIC state
of the current CPU for the MMIO.

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 hw/apic.c           |   40 +++++++++++++++-------------------------
 hw/apic.h           |    9 ++++++++-
 hw/pc.c             |   12 +++++++++++-
 target-i386/cpu.h   |   27 ++++++++++++++++-----------
 target-i386/cpuid.c |    6 ++++++
 5 files changed, 56 insertions(+), 38 deletions(-)

diff --git a/hw/apic.c b/hw/apic.c
index 91c8d93..332c66e 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -95,7 +95,7 @@
 #define MSI_ADDR_SIZE                   0x100000

 struct APICState {
-    CPUState *cpu_env;
+    void *cpu_env;
     uint32_t apicbase;
     uint8_t id;
     uint8_t arb_id;
@@ -320,7 +320,7 @@ void cpu_set_apic_base(APICState *s, uint64_t val)
     /* if disabled, cannot be enabled again */
     if (!(val & MSR_IA32_APICBASE_ENABLE)) {
         s->apicbase &= ~MSR_IA32_APICBASE_ENABLE;
-        s->cpu_env->cpuid_features &= ~CPUID_APIC;
+        cpu_clear_apic_feature(s->cpu_env);
         s->spurious_vec &= ~APIC_SV_ENABLE;
     }
 }
@@ -508,8 +508,6 @@ void apic_init_reset(APICState *s)
     s->initial_count_load_time = 0;
     s->next_time = 0;
     s->wait_for_sipi = 1;
-
-    s->cpu_env->halted = !(s->apicbase & MSR_IA32_APICBASE_BSP);
 }

 static void apic_startup(APICState *s, int vector_num)
@@ -524,13 +522,7 @@ void apic_sipi(APICState *s)

     if (!s->wait_for_sipi)
         return;
-
-    s->cpu_env->eip = 0;
-    cpu_x86_load_seg_cache(s->cpu_env, R_CS, s->sipi_vector << 8,
-                           s->sipi_vector << 12,
-                           s->cpu_env->segs[R_CS].limit,
-                           s->cpu_env->segs[R_CS].flags);
-    s->cpu_env->halted = 0;
+    cpu_x86_load_seg_cache_sipi(s->cpu_env, s->sipi_vector);
     s->wait_for_sipi = 0;
 }

@@ -692,15 +684,14 @@ static void apic_mem_writew(void *opaque,
target_phys_addr_t addr, uint32_t val)

 static uint32_t apic_mem_readl(void *opaque, target_phys_addr_t addr)
 {
-    CPUState *env;
     APICState *s;
     uint32_t val;
     int index;

-    env = cpu_single_env;
-    if (!env)
+    s = cpu_get_current_apic();
+    if (!s) {
         return 0;
-    s = env->apic_state;
+    }

     index = (addr >> 4) & 0xff;
     switch(index) {
@@ -782,7 +773,6 @@ static void apic_send_msi(target_phys_addr_t addr,
uint32 data)

 static void apic_mem_writel(void *opaque, target_phys_addr_t addr,
uint32_t val)
 {
-    CPUState *env;
     APICState *s;
     int index = (addr >> 4) & 0xff;
     if (addr > 0xfff || !index) {
@@ -795,10 +785,10 @@ static void apic_mem_writel(void *opaque,
target_phys_addr_t addr, uint32_t val)
         return;
     }

-    env = cpu_single_env;
-    if (!env)
+    s = cpu_get_current_apic();
+    if (!s) {
         return;
-    s = env->apic_state;
+    }

     DPRINTF("write: " TARGET_FMT_plx " = %08x\n", addr, val);

@@ -974,16 +964,16 @@ static CPUWriteMemoryFunc * const apic_mem_write[3] = {
     apic_mem_writel,
 };

-int apic_init(CPUState *env)
+APICState *apic_init(void *env, uint32_t apic_id)
 {
     APICState *s;

-    if (last_apic_idx >= MAX_APICS)
-        return -1;
+    if (last_apic_idx >= MAX_APICS) {
+        return NULL;
+    }
     s = qemu_mallocz(sizeof(APICState));
-    env->apic_state = s;
     s->idx = last_apic_idx++;
-    s->id = env->cpuid_apic_id;
+    s->id = apic_id;
     s->cpu_env = env;

     msix_supported = 1;
@@ -1004,5 +994,5 @@ int apic_init(CPUState *env)
     qemu_register_reset(apic_reset, s);

     local_apics[s->idx] = s;
-    return 0;
+    return s;
 }
diff --git a/hw/apic.h b/hw/apic.h
index 7dc7c62..a6af078 100644
--- a/hw/apic.h
+++ b/hw/apic.h
@@ -7,14 +7,21 @@ void apic_deliver_irq(uint8_t dest, uint8_t dest_mode,
                              uint8_t delivery_mode,
                              uint8_t vector_num, uint8_t polarity,
                              uint8_t trigger_mode);
-int apic_init(CPUState *env);
+APICState *apic_init(void *env, uint32_t apic_id);
 int apic_accept_pic_intr(APICState *s);
 void apic_deliver_pic_intr(APICState *s, int level);
 int apic_get_interrupt(APICState *s);
 void apic_reset_irq_delivered(void);
 int apic_get_irq_delivered(void);
+void cpu_set_apic_base(APICState *s, uint64_t val);
+uint64_t cpu_get_apic_base(APICState *s);
+void cpu_set_apic_tpr(APICState *s, uint8_t val);
+uint8_t cpu_get_apic_tpr(APICState *s);
+void apic_init_reset(APICState *s);
+void apic_sipi(APICState *s);

 /* pc.c */
 int cpu_is_bsp(CPUState *env);
+APICState *cpu_get_current_apic(void);

 #endif
diff --git a/hw/pc.c b/hw/pc.c
index fe4ebbe..913e461 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -749,6 +749,15 @@ int cpu_is_bsp(CPUState *env)
     return env->cpu_index == 0;
 }

+APICState *cpu_get_current_apic(void)
+{
+    if (cpu_single_env) {
+        return cpu_single_env->apic_state;
+    } else {
+        return NULL;
+    }
+}
+
 /* set CMOS shutdown status register (index 0xF) as S3_resume(0xFE)
    BIOS will read it and start S3 resume at POST Entry */
 void pc_cmos_set_s3_resume(void *opaque, int irq, int level)
@@ -781,10 +790,11 @@ static CPUState *pc_new_cpu(const char *cpu_model)
     if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
         env->cpuid_apic_id = env->cpu_index;
         /* APIC reset callback resets cpu */
-        apic_init(env);
+        env->apic_state = apic_init(env, env->cpuid_apic_id);
     } else {
         qemu_register_reset((QEMUResetHandler*)cpu_reset, env);
     }
+    env->halted = cpu_is_bsp(env);
     return env;
 }

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 0b19fe3..619a747 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -790,6 +790,17 @@ static inline void cpu_x86_load_seg_cache(CPUX86State *env,
     }
 }

+static inline void cpu_x86_load_seg_cache_sipi(CPUX86State *env,
+                                               int sipi_vector)
+{
+    env->eip = 0;
+    cpu_x86_load_seg_cache(env, R_CS, sipi_vector << 8,
+                           sipi_vector << 12,
+                           env->segs[R_CS].limit,
+                           env->segs[R_CS].flags);
+    env->halted = 0;
+}
+
 int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector,
                             target_ulong *base, unsigned int *limit,
                             unsigned int *flags);
@@ -827,6 +838,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t
index, uint32_t count,
                    uint32_t *eax, uint32_t *ebx,
                    uint32_t *ecx, uint32_t *edx);
 int cpu_x86_register (CPUX86State *env, const char *cpu_model);
+void cpu_clear_apic_feature(CPUX86State *env);

 /* helper.c */
 int cpu_x86_handle_mmu_fault(CPUX86State *env, target_ulong addr,
@@ -859,15 +871,6 @@ void cpu_x86_update_cr0(CPUX86State *env,
uint32_t new_cr0);
 void cpu_x86_update_cr3(CPUX86State *env, target_ulong new_cr3);
 void cpu_x86_update_cr4(CPUX86State *env, uint32_t new_cr4);

-/* hw/apic.c */
-typedef struct APICState APICState;
-void cpu_set_apic_base(APICState *s, uint64_t val);
-uint64_t cpu_get_apic_base(APICState *s);
-void cpu_set_apic_tpr(APICState *s, uint8_t val);
-#ifndef NO_CPU_IO_DEFS
-uint8_t cpu_get_apic_tpr(APICState *s);
-#endif
-
 /* hw/pc.c */
 void cpu_smm_update(CPUX86State *env);
 uint64_t cpu_get_tsc(CPUX86State *env);
@@ -929,6 +932,10 @@ static inline void cpu_clone_regs(CPUState *env,
target_ulong newsp)

 #include "svm.h"

+#if !defined(CONFIG_USER_ONLY)
+#include "hw/apic.h"
+#endif
+
 static inline void cpu_pc_from_tb(CPUState *env, TranslationBlock *tb)
 {
     env->eip = tb->pc - tb->cs_base;
@@ -943,8 +950,6 @@ static inline void cpu_get_tb_cpu_state(CPUState
*env, target_ulong *pc,
         (env->eflags & (IOPL_MASK | TF_MASK | RF_MASK | VM_MASK));
 }

-void apic_init_reset(APICState *s);
-void apic_sipi(APICState *s);
 void do_cpu_init(CPUState *env);
 void do_cpu_sipi(CPUState *env);
 #endif /* CPU_I386_H */
diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c
index 7a11215..6a0f7ca 100644
--- a/target-i386/cpuid.c
+++ b/target-i386/cpuid.c
@@ -944,6 +944,12 @@ static int cpudef_register(QemuOpts *opts, void *opaque)
     x86_defs = def;
     return (0);
 }
+
+void cpu_clear_apic_feature(CPUX86State *env)
+{
+    env->cpuid_features &= ~CPUID_APIC;
+}
+
 #endif /* !CONFIG_USER_ONLY */

 /* register "cpudef" models defined in configuration file.  Here we first
-- 
1.7.1

[Qemu-devel] Re: [PATCH 6/6] apic: avoid using CPUState internals

[Jan Kiszka]

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

Blue Swirl wrote:
> Use only an opaque CPUState pointer and move the actual CPUState
> contents handling to cpu.h and cpuid.c.
> 
> Set env->halted in pc.c and add a function to get the local APIC state
> of the current CPU for the MMIO.
> 
> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
> ---
>  hw/apic.c           |   40 +++++++++++++++-------------------------
>  hw/apic.h           |    9 ++++++++-
>  hw/pc.c             |   12 +++++++++++-
>  target-i386/cpu.h   |   27 ++++++++++++++++-----------
>  target-i386/cpuid.c |    6 ++++++
>  5 files changed, 56 insertions(+), 38 deletions(-)
> 
> diff --git a/hw/apic.c b/hw/apic.c
> index 91c8d93..332c66e 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -95,7 +95,7 @@
>  #define MSI_ADDR_SIZE                   0x100000
> 
>  struct APICState {
> -    CPUState *cpu_env;
> +    void *cpu_env;
>      uint32_t apicbase;
>      uint8_t id;
>      uint8_t arb_id;
> @@ -320,7 +320,7 @@ void cpu_set_apic_base(APICState *s, uint64_t val)
>      /* if disabled, cannot be enabled again */
>      if (!(val & MSR_IA32_APICBASE_ENABLE)) {
>          s->apicbase &= ~MSR_IA32_APICBASE_ENABLE;
> -        s->cpu_env->cpuid_features &= ~CPUID_APIC;
> +        cpu_clear_apic_feature(s->cpu_env);
>          s->spurious_vec &= ~APIC_SV_ENABLE;
>      }
>  }
> @@ -508,8 +508,6 @@ void apic_init_reset(APICState *s)
>      s->initial_count_load_time = 0;
>      s->next_time = 0;
>      s->wait_for_sipi = 1;
> -
> -    s->cpu_env->halted = !(s->apicbase & MSR_IA32_APICBASE_BSP);

We are now lacking 'halted' initialization after system reset. Could be
addressed by a special reset handler in hw/pc.c, I guess.

Jan



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

[Qemu-devel] Re: [PATCH 6/6] apic: avoid using CPUState internals

[Blue Swirl]

On Sun, Jun 6, 2010 at 7:36 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
> Blue Swirl wrote:
>> Use only an opaque CPUState pointer and move the actual CPUState
>> contents handling to cpu.h and cpuid.c.
>>
>> Set env->halted in pc.c and add a function to get the local APIC state
>> of the current CPU for the MMIO.
>>
>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>> ---
>>  hw/apic.c           |   40 +++++++++++++++-------------------------
>>  hw/apic.h           |    9 ++++++++-
>>  hw/pc.c             |   12 +++++++++++-
>>  target-i386/cpu.h   |   27 ++++++++++++++++-----------
>>  target-i386/cpuid.c |    6 ++++++
>>  5 files changed, 56 insertions(+), 38 deletions(-)
>>
>> diff --git a/hw/apic.c b/hw/apic.c
>> index 91c8d93..332c66e 100644
>> --- a/hw/apic.c
>> +++ b/hw/apic.c
>> @@ -95,7 +95,7 @@
>>  #define MSI_ADDR_SIZE                   0x100000
>>
>>  struct APICState {
>> -    CPUState *cpu_env;
>> +    void *cpu_env;
>>      uint32_t apicbase;
>>      uint8_t id;
>>      uint8_t arb_id;
>> @@ -320,7 +320,7 @@ void cpu_set_apic_base(APICState *s, uint64_t val)
>>      /* if disabled, cannot be enabled again */
>>      if (!(val & MSR_IA32_APICBASE_ENABLE)) {
>>          s->apicbase &= ~MSR_IA32_APICBASE_ENABLE;
>> -        s->cpu_env->cpuid_features &= ~CPUID_APIC;
>> +        cpu_clear_apic_feature(s->cpu_env);
>>          s->spurious_vec &= ~APIC_SV_ENABLE;
>>      }
>>  }
>> @@ -508,8 +508,6 @@ void apic_init_reset(APICState *s)
>>      s->initial_count_load_time = 0;
>>      s->next_time = 0;
>>      s->wait_for_sipi = 1;
>> -
>> -    s->cpu_env->halted = !(s->apicbase & MSR_IA32_APICBASE_BSP);
>
> We are now lacking 'halted' initialization after system reset. Could be
> addressed by a special reset handler in hw/pc.c, I guess.

Good catch, I forgot to do that.

[Qemu-devel] Re: [PATCH 6/6] apic: avoid using CPUState internals

[Paolo Bonzini]

On 06/05/2010 11:31 PM, Blue Swirl wrote:
> Use only an opaque CPUState pointer and move the actual CPUState
> contents handling to cpu.h and cpuid.c.
>
> Set env->halted in pc.c and add a function to get the local APIC state
> of the current CPU for the MMIO.
>
> Signed-off-by: Blue Swirl<blauwirbel@gmail.com>
> ---
>   hw/apic.c           |   40 +++++++++++++++-------------------------
>   hw/apic.h           |    9 ++++++++-
>   hw/pc.c             |   12 +++++++++++-
>   target-i386/cpu.h   |   27 ++++++++++++++++-----------
>   target-i386/cpuid.c |    6 ++++++
>   5 files changed, 56 insertions(+), 38 deletions(-)
>
> diff --git a/hw/apic.c b/hw/apic.c
> index 91c8d93..332c66e 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -95,7 +95,7 @@
>   #define MSI_ADDR_SIZE                   0x100000
>
>   struct APICState {
> -    CPUState *cpu_env;
> +    void *cpu_env;

I proposed having an opaque CPUState type in hw/ but it was rejected. 
But I don't think using a void pointer is any better.

Paolo

Re: [Qemu-devel] Re: [PATCH 6/6] apic: avoid using CPUState internals

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 06/05/2010 11:31 PM, Blue Swirl wrote:
>> Use only an opaque CPUState pointer and move the actual CPUState
>> contents handling to cpu.h and cpuid.c.
>>
>> Set env->halted in pc.c and add a function to get the local APIC state
>> of the current CPU for the MMIO.
>>
>> Signed-off-by: Blue Swirl<blauwirbel@gmail.com>
>> ---
>>   hw/apic.c           |   40 +++++++++++++++-------------------------
>>   hw/apic.h           |    9 ++++++++-
>>   hw/pc.c             |   12 +++++++++++-
>>   target-i386/cpu.h   |   27 ++++++++++++++++-----------
>>   target-i386/cpuid.c |    6 ++++++
>>   5 files changed, 56 insertions(+), 38 deletions(-)
>>
>> diff --git a/hw/apic.c b/hw/apic.c
>> index 91c8d93..332c66e 100644
>> --- a/hw/apic.c
>> +++ b/hw/apic.c
>> @@ -95,7 +95,7 @@
>>   #define MSI_ADDR_SIZE                   0x100000
>>
>>   struct APICState {
>> -    CPUState *cpu_env;
>> +    void *cpu_env;
>
> I proposed having an opaque CPUState type in hw/ but it was
> rejected. But I don't think using a void pointer is any better.

Using void * to hide implementation details of a monomorphic type is bad
style.  That's what incomplete types are for.  Lets the compiler check
types for you, and lets the debugger do a better job.

[Qemu-devel] Re: [PATCH 6/6] apic: avoid using CPUState internals

[Blue Swirl]

On Sun, Jun 6, 2010 at 5:39 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 06/05/2010 11:31 PM, Blue Swirl wrote:
>>
>> Use only an opaque CPUState pointer and move the actual CPUState
>> contents handling to cpu.h and cpuid.c.
>>
>> Set env->halted in pc.c and add a function to get the local APIC state
>> of the current CPU for the MMIO.
>>
>> Signed-off-by: Blue Swirl<blauwirbel@gmail.com>
>> ---
>>  hw/apic.c           |   40 +++++++++++++++-------------------------
>>  hw/apic.h           |    9 ++++++++-
>>  hw/pc.c             |   12 +++++++++++-
>>  target-i386/cpu.h   |   27 ++++++++++++++++-----------
>>  target-i386/cpuid.c |    6 ++++++
>>  5 files changed, 56 insertions(+), 38 deletions(-)
>>
>> diff --git a/hw/apic.c b/hw/apic.c
>> index 91c8d93..332c66e 100644
>> --- a/hw/apic.c
>> +++ b/hw/apic.c
>> @@ -95,7 +95,7 @@
>>  #define MSI_ADDR_SIZE                   0x100000
>>
>>  struct APICState {
>> -    CPUState *cpu_env;
>> +    void *cpu_env;
>
> I proposed having an opaque CPUState type in hw/ but it was rejected. But I
> don't think using a void pointer is any better.

It's not necessary for the patch. Maybe it's possible to avoid all
CPUState references in apic.c by pushing the dependencies to pc.c. It
could affect performance though.

[Qemu-devel] Re: [PATCH 6/6] apic: avoid using CPUState internals

[Paolo Bonzini]

On 06/09/2010 09:59 PM, Blue Swirl wrote:
>>>   struct APICState {
>>> -    CPUState *cpu_env;
>>> +    void *cpu_env;
>>
>> I proposed having an opaque CPUState type in hw/ but it was rejected. But I
>> don't think using a void pointer is any better.
>
> It's not necessary for the patch. Maybe it's possible to avoid all
> CPUState references in apic.c by pushing the dependencies to pc.c. It
> could affect performance though.

I think it's unnecessary.  But I'd leave CPUState

Paolo