[Qemu-devel] [PATCH 0/5] target-i386: move cpu reset and tcg intialization inside CPU object

[Qemu-devel] [PATCH 0/5] target-i386: move cpu reset and tcg intialization inside CPU object

[Igor Mammedov]

There is no much sence in holding back rather independed cpu_reset and tcg init patches from:
 "[PATCH qom-next v3 0/12] target-i386: re-factor CPU creation/initialization to QOM"
 http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg04077.html

Reposting ammended and rebased patches.

git tree for testing: 
  https://github.com/imammedo/qemu/tree/x86cpu_qom_reset_tcg

Compile & Run tested:
  target-i386: tcg and kvm mode
  i386-linux-user: running of /bin/ls

Igor Mammedov (5):
  target-i386: move reset callback to cpu.c and call cpu_reset() in x86_cpu_realize()
  target-i386: move cpu halted decision into x86_cpu_reset
  target-i386: call x86_cpu_realize() only after APIC is initialized.
  target-i386: move tcg initialization into x86_cpu_initfn()
  target-i386: drop usage of prev_debug_excp_handler

 hw/apic.h            |    2 +-
 hw/apic_common.c     |   20 ++++++++++++++------
 hw/pc.c              |   10 +---------
 target-i386/cpu.c    |   19 +++++++++++++++++++
 target-i386/cpu.h    |    1 +
 target-i386/helper.c |   19 +------------------
 target-i386/kvm.c    |    5 +++--
 7 files changed, 40 insertions(+), 36 deletions(-)

[Qemu-devel] [PATCH 1/5] target-i386: drop usage of prev_debug_excp_handler

[Igor Mammedov]

Chain of exception handlers are currently unused feature, drop it
for now to avoid moving prev_debug_excp_handler variable at global
scope when moving tcg initialization into target-i386/cpu.c

Later we probably could re-invent better interface for this.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/helper.c |    7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/target-i386/helper.c b/target-i386/helper.c
index 2cc8097..b9384f6 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -941,8 +941,6 @@ int check_hw_breakpoints(CPUX86State *env, int force_dr6_update)
     return hit_enabled;
 }
 
-static CPUDebugExcpHandler *prev_debug_excp_handler;
-
 static void breakpoint_handler(CPUX86State *env)
 {
     CPUBreakpoint *bp;
@@ -965,8 +963,6 @@ static void breakpoint_handler(CPUX86State *env)
                 break;
             }
     }
-    if (prev_debug_excp_handler)
-        prev_debug_excp_handler(env);
 }
 
 typedef struct MCEInjectionParams {
@@ -1166,8 +1162,7 @@ X86CPU *cpu_x86_init(const char *cpu_model)
         inited = 1;
         optimize_flags_init();
 #ifndef CONFIG_USER_ONLY
-        prev_debug_excp_handler =
-            cpu_set_debug_excp_handler(breakpoint_handler);
+        cpu_set_debug_excp_handler(breakpoint_handler);
 #endif
     }
     if (cpu_x86_register(cpu, cpu_model) < 0) {
-- 
1.7.10.2

Re: [Qemu-devel] [PATCH 1/5] target-i386: drop usage of prev_debug_excp_handler

[Jan Kiszka]

On 2012-06-20 14:59, Igor Mammedov wrote:
> Chain of exception handlers are currently unused feature, drop it
> for now to avoid moving prev_debug_excp_handler variable at global
> scope when moving tcg initialization into target-i386/cpu.c
> 
> Later we probably could re-invent better interface for this.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  target-i386/helper.c |    7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index 2cc8097..b9384f6 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -941,8 +941,6 @@ int check_hw_breakpoints(CPUX86State *env, int force_dr6_update)
>      return hit_enabled;
>  }
>  
> -static CPUDebugExcpHandler *prev_debug_excp_handler;
> -
>  static void breakpoint_handler(CPUX86State *env)
>  {
>      CPUBreakpoint *bp;
> @@ -965,8 +963,6 @@ static void breakpoint_handler(CPUX86State *env)
>                  break;
>              }
>      }
> -    if (prev_debug_excp_handler)
> -        prev_debug_excp_handler(env);
>  }
>  
>  typedef struct MCEInjectionParams {
> @@ -1166,8 +1162,7 @@ X86CPU *cpu_x86_init(const char *cpu_model)
>          inited = 1;
>          optimize_flags_init();
>  #ifndef CONFIG_USER_ONLY
> -        prev_debug_excp_handler =
> -            cpu_set_debug_excp_handler(breakpoint_handler);
> +        cpu_set_debug_excp_handler(breakpoint_handler);
>  #endif
>      }
>      if (cpu_x86_register(cpu, cpu_model) < 0) {
> 

That's inconsistent. Let's remove this for all targets and drop the
return value of cpu_set_debug_excp_handler.

Jan

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

Re: [Qemu-devel] [PATCH 1/5] target-i386: drop usage of prev_debug_excp_handler

[Igor Mammedov]

On 06/20/2012 03:28 PM, Jan Kiszka wrote:
> On 2012-06-20 14:59, Igor Mammedov wrote:
>> Chain of exception handlers are currently unused feature, drop it
>> for now to avoid moving prev_debug_excp_handler variable at global
>> scope when moving tcg initialization into target-i386/cpu.c
>>
>> Later we probably could re-invent better interface for this.
>>
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>> ---
>>   target-i386/helper.c |    7 +------
>>   1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/target-i386/helper.c b/target-i386/helper.c
>> index 2cc8097..b9384f6 100644
>> --- a/target-i386/helper.c
>> +++ b/target-i386/helper.c
>> @@ -941,8 +941,6 @@ int check_hw_breakpoints(CPUX86State *env, int force_dr6_update)
>>       return hit_enabled;
>>   }
>>
>> -static CPUDebugExcpHandler *prev_debug_excp_handler;
>> -
>>   static void breakpoint_handler(CPUX86State *env)
>>   {
>>       CPUBreakpoint *bp;
>> @@ -965,8 +963,6 @@ static void breakpoint_handler(CPUX86State *env)
>>                   break;
>>               }
>>       }
>> -    if (prev_debug_excp_handler)
>> -        prev_debug_excp_handler(env);
>>   }
>>
>>   typedef struct MCEInjectionParams {
>> @@ -1166,8 +1162,7 @@ X86CPU *cpu_x86_init(const char *cpu_model)
>>           inited = 1;
>>           optimize_flags_init();
>>   #ifndef CONFIG_USER_ONLY
>> -        prev_debug_excp_handler =
>> -            cpu_set_debug_excp_handler(breakpoint_handler);
>> +        cpu_set_debug_excp_handler(breakpoint_handler);
>>   #endif
>>       }
>>       if (cpu_x86_register(cpu, cpu_model) < 0) {
>>
>
> That's inconsistent. Let's remove this for all targets and drop the
> return value of cpu_set_debug_excp_handler.
>
> Jan
>
Thanks, I'll fix it.

-- 
-----
  Igor

[Qemu-devel] [PATCH 2/5] target-i386: move tcg initialization into x86_cpu_initfn()

[Igor Mammedov]

In order to make cpu object not depended on external ad-hoc
initialization routines, move tcg initialization from cpu_x86_init
inside cpu object x86_cpu_initfn().

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c    |   10 ++++++++++
 target-i386/cpu.h    |    1 +
 target-i386/helper.c |   11 +----------
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 0b61162..7ace126 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1708,6 +1708,7 @@ static void x86_cpu_initfn(Object *obj)
 {
     X86CPU *cpu = X86_CPU(obj);
     CPUX86State *env = &cpu->env;
+    static int inited;
 
     cpu_exec_init(env);
 
@@ -1737,6 +1738,15 @@ static void x86_cpu_initfn(Object *obj)
                         x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
 
     env->cpuid_apic_id = env->cpu_index;
+
+    /* init various static tables used in TCG mode */
+    if (tcg_enabled() && !inited) {
+        inited = 1;
+        optimize_flags_init();
+#ifndef CONFIG_USER_ONLY
+        cpu_set_debug_excp_handler(breakpoint_handler);
+#endif
+    }
 }
 
 static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index bcf663e..80dcb49 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -933,6 +933,7 @@ static inline int hw_breakpoint_len(unsigned long dr7, int index)
 void hw_breakpoint_insert(CPUX86State *env, int index);
 void hw_breakpoint_remove(CPUX86State *env, int index);
 int check_hw_breakpoints(CPUX86State *env, int force_dr6_update);
+void breakpoint_handler(CPUX86State *env);
 
 /* will be suppressed */
 void cpu_x86_update_cr0(CPUX86State *env, uint32_t new_cr0);
diff --git a/target-i386/helper.c b/target-i386/helper.c
index b9384f6..c52ec13 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -941,7 +941,7 @@ int check_hw_breakpoints(CPUX86State *env, int force_dr6_update)
     return hit_enabled;
 }
 
-static void breakpoint_handler(CPUX86State *env)
+void breakpoint_handler(CPUX86State *env)
 {
     CPUBreakpoint *bp;
 
@@ -1151,20 +1151,11 @@ X86CPU *cpu_x86_init(const char *cpu_model)
 {
     X86CPU *cpu;
     CPUX86State *env;
-    static int inited;
 
     cpu = X86_CPU(object_new(TYPE_X86_CPU));
     env = &cpu->env;
     env->cpu_model_str = cpu_model;
 
-    /* init various static tables used in TCG mode */
-    if (tcg_enabled() && !inited) {
-        inited = 1;
-        optimize_flags_init();
-#ifndef CONFIG_USER_ONLY
-        cpu_set_debug_excp_handler(breakpoint_handler);
-#endif
-    }
     if (cpu_x86_register(cpu, cpu_model) < 0) {
         object_delete(OBJECT(cpu));
         return NULL;
-- 
1.7.10.2

Re: [Qemu-devel] [PATCH 2/5] target-i386: move tcg initialization into x86_cpu_initfn()

[Andreas Färber]

Am 20.06.2012 14:59, schrieb Igor Mammedov:
> In order to make cpu object not depended on external ad-hoc
> initialization routines, move tcg initialization from cpu_x86_init
> inside cpu object x86_cpu_initfn().
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Andreas Färber <afaerber@suse.de>

/-F

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

[Qemu-devel] [PATCH 3/5] target-i386: call x86_cpu_realize() after APIC is initialized.

[Igor Mammedov]

It's not correct to make CPU runnable (i.e. calling x86_cpu_realize())
when not all properties are set (APIC in this case).

Fix it by calling x86_cpu_realize() at board level after APIC is
initialized, right before cpu_reset().

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/pc.c              |    1 +
 target-i386/helper.c |    2 --
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 8368701..8a662cf 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -948,6 +948,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model)
         env->apic_state = apic_init(env, env->cpuid_apic_id);
     }
     qemu_register_reset(pc_cpu_reset, cpu);
+    x86_cpu_realize(OBJECT(cpu), NULL);
     pc_cpu_reset(cpu);
     return cpu;
 }
diff --git a/target-i386/helper.c b/target-i386/helper.c
index c52ec13..b38ea7f 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1161,8 +1161,6 @@ X86CPU *cpu_x86_init(const char *cpu_model)
         return NULL;
     }
 
-    x86_cpu_realize(OBJECT(cpu), NULL);
-
     return cpu;
 }
 
-- 
1.7.10.2

Re: [Qemu-devel] [PATCH 3/5] target-i386: call x86_cpu_realize() after APIC is initialized.

[Andreas Färber]

Am 20.06.2012 14:59, schrieb Igor Mammedov:
> It's not correct to make CPU runnable (i.e. calling x86_cpu_realize())
> when not all properties are set (APIC in this case).
> 
> Fix it by calling x86_cpu_realize() at board level after APIC is
> initialized, right before cpu_reset().
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/pc.c              |    1 +
>  target-i386/helper.c |    2 --
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/pc.c b/hw/pc.c
> index 8368701..8a662cf 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -948,6 +948,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model)
>          env->apic_state = apic_init(env, env->cpuid_apic_id);
>      }
>      qemu_register_reset(pc_cpu_reset, cpu);
> +    x86_cpu_realize(OBJECT(cpu), NULL);
>      pc_cpu_reset(cpu);
>      return cpu;
>  }
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index c52ec13..b38ea7f 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1161,8 +1161,6 @@ X86CPU *cpu_x86_init(const char *cpu_model)
>          return NULL;
>      }
>  
> -    x86_cpu_realize(OBJECT(cpu), NULL);
> -
>      return cpu;
>  }
>  

This will require changes in linux-user and possibly bsd-user. Having a
cpu_realize() would probably help with avoiding #ifdef'ery.
Unfortunately deriving CPUState from DeviceState proves a bit difficult
in the meantime (it worked at one point, now there's lots of circular
header dependencies), and realize support for Object got stopped.

Andreas

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

Re: [Qemu-devel] [PATCH 3/5] target-i386: call x86_cpu_realize() after APIC is initialized.

[Igor Mammedov]

On 06/20/2012 03:35 PM, Andreas Färber wrote:
> Am 20.06.2012 14:59, schrieb Igor Mammedov:
>> It's not correct to make CPU runnable (i.e. calling x86_cpu_realize())
>> when not all properties are set (APIC in this case).
>>
>> Fix it by calling x86_cpu_realize() at board level after APIC is
>> initialized, right before cpu_reset().
>>
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>> ---
>>   hw/pc.c              |    1 +
>>   target-i386/helper.c |    2 --
>>   2 files changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/hw/pc.c b/hw/pc.c
>> index 8368701..8a662cf 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -948,6 +948,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model)
>>           env->apic_state = apic_init(env, env->cpuid_apic_id);
>>       }
>>       qemu_register_reset(pc_cpu_reset, cpu);
>> +    x86_cpu_realize(OBJECT(cpu), NULL);
>>       pc_cpu_reset(cpu);
>>       return cpu;
>>   }
>> diff --git a/target-i386/helper.c b/target-i386/helper.c
>> index c52ec13..b38ea7f 100644
>> --- a/target-i386/helper.c
>> +++ b/target-i386/helper.c
>> @@ -1161,8 +1161,6 @@ X86CPU *cpu_x86_init(const char *cpu_model)
>>           return NULL;
>>       }
>>
>> -    x86_cpu_realize(OBJECT(cpu), NULL);
>> -
>>       return cpu;
>>   }
>>
>
> This will require changes in linux-user and possibly bsd-user. Having a
Why it would require changes in linux-user? So far x86_cpu_realize() does
nothing useful in linux-user,  compiled and tested. It should be harmless
for linux-user not to execute it.
But I haven't compiled/tested bsd-user, do I need BSD for this?

> cpu_realize() would probably help with avoiding #ifdef'ery.
> Unfortunately deriving CPUState from DeviceState proves a bit difficult
> in the meantime (it worked at one point, now there's lots of circular
> header dependencies), and realize support for Object got stopped.
I'm in process of untangling this header mayhem (at least to a point
that allows compilation complete when CPU is derived from Device)

>
> Andreas
>

-- 
-----
  Igor

Re: [Qemu-devel] [PATCH 3/5] target-i386: call x86_cpu_realize() after APIC is initialized.

[Andreas Färber]

Am 21.06.2012 11:43, schrieb Igor Mammedov:
> On 06/20/2012 03:35 PM, Andreas Färber wrote:
>> Am 20.06.2012 14:59, schrieb Igor Mammedov:
>>> It's not correct to make CPU runnable (i.e. calling x86_cpu_realize())
>>> when not all properties are set (APIC in this case).
>>>
>>> Fix it by calling x86_cpu_realize() at board level after APIC is
>>> initialized, right before cpu_reset().
>>>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>>   hw/pc.c              |    1 +
>>>   target-i386/helper.c |    2 --
>>>   2 files changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/hw/pc.c b/hw/pc.c
>>> index 8368701..8a662cf 100644
>>> --- a/hw/pc.c
>>> +++ b/hw/pc.c
>>> @@ -948,6 +948,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model)
>>>           env->apic_state = apic_init(env, env->cpuid_apic_id);
>>>       }
>>>       qemu_register_reset(pc_cpu_reset, cpu);
>>> +    x86_cpu_realize(OBJECT(cpu), NULL);
>>>       pc_cpu_reset(cpu);
>>>       return cpu;
>>>   }
>>> diff --git a/target-i386/helper.c b/target-i386/helper.c
>>> index c52ec13..b38ea7f 100644
>>> --- a/target-i386/helper.c
>>> +++ b/target-i386/helper.c
>>> @@ -1161,8 +1161,6 @@ X86CPU *cpu_x86_init(const char *cpu_model)
>>>           return NULL;
>>>       }
>>>
>>> -    x86_cpu_realize(OBJECT(cpu), NULL);
>>> -
>>>       return cpu;
>>>   }
>>>
>>
>> This will require changes in linux-user and possibly bsd-user. Having a
> Why it would require changes in linux-user? So far x86_cpu_realize() does
> nothing useful in linux-user,  compiled and tested. It should be harmless
> for linux-user not to execute it.

Hm, I'd need to recheck...

> But I haven't compiled/tested bsd-user, do I need BSD for this?

Yes, you do. But if it's not needed for linux-user then it shouldn't be
needed for bsd-user either.

>> cpu_realize() would probably help with avoiding #ifdef'ery.
>> Unfortunately deriving CPUState from DeviceState proves a bit difficult
>> in the meantime (it worked at one point, now there's lots of circular
>> header dependencies), and realize support for Object got stopped.

> I'm in process of untangling this header mayhem (at least to a point
> that allows compilation complete when CPU is derived from Device)

So am I... A few weeks ago my qom-cpu-dev branch on GitHub used to
compile, now something has changed and I needed to take cpu.h out of
qemu-common.h and move lots of, e.g., ARM devices into libhw to avoid
cpu.h dependencies and add cpu.h includes elsewhere. Would be good to
coordinate that, are you on IRC later today?

Andreas

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

Re: [Qemu-devel] [PATCH 3/5] target-i386: call x86_cpu_realize() after APIC is initialized.

[Igor Mammedov]

On 06/21/2012 12:14 PM, Andreas Färber wrote:
> Am 21.06.2012 11:43, schrieb Igor Mammedov:
>> On 06/20/2012 03:35 PM, Andreas Färber wrote:
>>> Am 20.06.2012 14:59, schrieb Igor Mammedov:
>>>> It's not correct to make CPU runnable (i.e. calling x86_cpu_realize())
>>>> when not all properties are set (APIC in this case).
>>>>
>>>> Fix it by calling x86_cpu_realize() at board level after APIC is
>>>> initialized, right before cpu_reset().
>>>>
>>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>>> ---
>>>>    hw/pc.c              |    1 +
>>>>    target-i386/helper.c |    2 --
>>>>    2 files changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/pc.c b/hw/pc.c
>>>> index 8368701..8a662cf 100644
>>>> --- a/hw/pc.c
>>>> +++ b/hw/pc.c
>>>> @@ -948,6 +948,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model)
>>>>            env->apic_state = apic_init(env, env->cpuid_apic_id);
>>>>        }
>>>>        qemu_register_reset(pc_cpu_reset, cpu);
>>>> +    x86_cpu_realize(OBJECT(cpu), NULL);
>>>>        pc_cpu_reset(cpu);
>>>>        return cpu;
>>>>    }
>>>> diff --git a/target-i386/helper.c b/target-i386/helper.c
>>>> index c52ec13..b38ea7f 100644
>>>> --- a/target-i386/helper.c
>>>> +++ b/target-i386/helper.c
>>>> @@ -1161,8 +1161,6 @@ X86CPU *cpu_x86_init(const char *cpu_model)
>>>>            return NULL;
>>>>        }
>>>>
>>>> -    x86_cpu_realize(OBJECT(cpu), NULL);
>>>> -
>>>>        return cpu;
>>>>    }
>>>>
>>>
>>> This will require changes in linux-user and possibly bsd-user. Having a
>> Why it would require changes in linux-user? So far x86_cpu_realize() does
>> nothing useful in linux-user,  compiled and tested. It should be harmless
>> for linux-user not to execute it.
>
> Hm, I'd need to recheck...
>
>> But I haven't compiled/tested bsd-user, do I need BSD for this?
>
> Yes, you do. But if it's not needed for linux-user then it shouldn't be
> needed for bsd-user either.
>
>>> cpu_realize() would probably help with avoiding #ifdef'ery.
>>> Unfortunately deriving CPUState from DeviceState proves a bit difficult
>>> in the meantime (it worked at one point, now there's lots of circular
>>> header dependencies), and realize support for Object got stopped.
>
>> I'm in process of untangling this header mayhem (at least to a point
>> that allows compilation complete when CPU is derived from Device)
>
> So am I... A few weeks ago my qom-cpu-dev branch on GitHub used to
> compile, now something has changed and I needed to take cpu.h out of
> qemu-common.h and move lots of, e.g., ARM devices into libhw to avoid
> cpu.h dependencies and add cpu.h includes elsewhere. Would be good to
> coordinate that, are you on IRC later today?

Sure, I'd like to.
Could you send me connection details for "IRC"? I do not know where it is.

>
> Andreas
>

-- 
-----
  Igor

Re: [Qemu-devel] [PATCH 3/5] target-i386: call x86_cpu_realize() after APIC is initialized.

[igor]

On 06/20/2012 03:35 PM, Andreas Färber wrote:
> Am 20.06.2012 14:59, schrieb Igor Mammedov:
>> It's not correct to make CPU runnable (i.e. calling x86_cpu_realize())
>> when not all properties are set (APIC in this case).
>>
>> Fix it by calling x86_cpu_realize() at board level after APIC is
>> initialized, right before cpu_reset().
>>
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>> ---
>>   hw/pc.c              |    1 +
>>   target-i386/helper.c |    2 --
>>   2 files changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/hw/pc.c b/hw/pc.c
>> index 8368701..8a662cf 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -948,6 +948,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model)
>>           env->apic_state = apic_init(env, env->cpuid_apic_id);
>>       }
>>       qemu_register_reset(pc_cpu_reset, cpu);
>> +    x86_cpu_realize(OBJECT(cpu), NULL);
>>       pc_cpu_reset(cpu);
>>       return cpu;
>>   }
>> diff --git a/target-i386/helper.c b/target-i386/helper.c
>> index c52ec13..b38ea7f 100644
>> --- a/target-i386/helper.c
>> +++ b/target-i386/helper.c
>> @@ -1161,8 +1161,6 @@ X86CPU *cpu_x86_init(const char *cpu_model)
>>           return NULL;
>>       }
>>
>> -    x86_cpu_realize(OBJECT(cpu), NULL);
>> -
>>       return cpu;
>>   }
>>
>
> This will require changes in linux-user and possibly bsd-user. Having a
> cpu_realize() would probably help with avoiding #ifdef'ery.
> Unfortunately deriving CPUState from DeviceState proves a bit difficult
> in the meantime (it worked at one point, now there's lots of circular
> header dependencies), and realize support for Object got stopped.
>
> Andreas
>
As alternative to keep, I could leave x86_cpu_realize() in 
cpu_x86_init() and keep pc_cpu_reset() in pc_new_cpu(). That will result 
in calling cpu_reset() 3 instead of 2 times.
Later when apic_init is moved inside cpu.c, a pc_cpu_reset() in 
pc_new_cpu() would be unnecessary and could be cleaned up then.

Re: [Qemu-devel] [PATCH 3/5] target-i386: call x86_cpu_realize() after APIC is initialized.

[Andreas Färber]

Am 09.07.2012 12:59, schrieb igor:
> On 06/20/2012 03:35 PM, Andreas Färber wrote:
>> Am 20.06.2012 14:59, schrieb Igor Mammedov:
>>> It's not correct to make CPU runnable (i.e. calling x86_cpu_realize())
>>> when not all properties are set (APIC in this case).
>>>
>>> Fix it by calling x86_cpu_realize() at board level after APIC is
>>> initialized, right before cpu_reset().
>>>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>>   hw/pc.c              |    1 +
>>>   target-i386/helper.c |    2 --
>>>   2 files changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/hw/pc.c b/hw/pc.c
>>> index 8368701..8a662cf 100644
>>> --- a/hw/pc.c
>>> +++ b/hw/pc.c
>>> @@ -948,6 +948,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model)
>>>           env->apic_state = apic_init(env, env->cpuid_apic_id);
>>>       }
>>>       qemu_register_reset(pc_cpu_reset, cpu);
>>> +    x86_cpu_realize(OBJECT(cpu), NULL);
>>>       pc_cpu_reset(cpu);
>>>       return cpu;
>>>   }
>>> diff --git a/target-i386/helper.c b/target-i386/helper.c
>>> index c52ec13..b38ea7f 100644
>>> --- a/target-i386/helper.c
>>> +++ b/target-i386/helper.c
>>> @@ -1161,8 +1161,6 @@ X86CPU *cpu_x86_init(const char *cpu_model)
>>>           return NULL;
>>>       }
>>>
>>> -    x86_cpu_realize(OBJECT(cpu), NULL);
>>> -
>>>       return cpu;
>>>   }
>>>
>>
>> This will require changes in linux-user and possibly bsd-user. Having a
>> cpu_realize() would probably help with avoiding #ifdef'ery.
>> Unfortunately deriving CPUState from DeviceState proves a bit difficult
>> in the meantime (it worked at one point, now there's lots of circular
>> header dependencies), and realize support for Object got stopped.
>>
> As alternative to keep, I could leave x86_cpu_realize() in
> cpu_x86_init() and keep pc_cpu_reset() in pc_new_cpu(). That will result
> in calling cpu_reset() 3 instead of 2 times.
> Later when apic_init is moved inside cpu.c, a pc_cpu_reset() in
> pc_new_cpu() would be unnecessary and could be cleaned up then.

Let me explain in more detail what I was thinking about: cpu_init() and
cpu_x86_init() today return an initialized/realized object. I don't want
bugs to creep into the user emulators because someone is not aware that
x86 is semantically differing from all other targets.

What I did for a qemu-rl78 machine is to inline cpu_rl78_init() so that
I could put code in between, i.e., for x86: object_new(), APIC/BSP
stuff, x86_cpu_realize(). That way any addition to the realize function
will still affect the user emulators.
The downside is that when we add x86 CPU subclasses we'd have to
remember to update two places. The solution to that would be to split
out a x86_cpu_new() function used from cpu_x86_init() and wherever you
need it for the PC machine. Then the code is still maintainable in one
central place and you get to do your APIC cleanups, and we don't depend
on a central realize implementation or device parent, what do you think?

Regards,
Andreas

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

Re: [Qemu-devel] [PATCH 3/5] target-i386: call x86_cpu_realize() after APIC is initialized.

[Igor Mammedov]

On 07/09/2012 02:57 PM, Andreas Färber wrote:
> Am 09.07.2012 12:59, schrieb igor:
>> On 06/20/2012 03:35 PM, Andreas Färber wrote:
>>> Am 20.06.2012 14:59, schrieb Igor Mammedov:
>>>> It's not correct to make CPU runnable (i.e. calling x86_cpu_realize())
>>>> when not all properties are set (APIC in this case).
>>>>
>>>> Fix it by calling x86_cpu_realize() at board level after APIC is
>>>> initialized, right before cpu_reset().
>>>>
>>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>>> ---
>>>>    hw/pc.c              |    1 +
>>>>    target-i386/helper.c |    2 --
>>>>    2 files changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/pc.c b/hw/pc.c
>>>> index 8368701..8a662cf 100644
>>>> --- a/hw/pc.c
>>>> +++ b/hw/pc.c
>>>> @@ -948,6 +948,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model)
>>>>            env->apic_state = apic_init(env, env->cpuid_apic_id);
>>>>        }
>>>>        qemu_register_reset(pc_cpu_reset, cpu);
>>>> +    x86_cpu_realize(OBJECT(cpu), NULL);
>>>>        pc_cpu_reset(cpu);
>>>>        return cpu;
>>>>    }
>>>> diff --git a/target-i386/helper.c b/target-i386/helper.c
>>>> index c52ec13..b38ea7f 100644
>>>> --- a/target-i386/helper.c
>>>> +++ b/target-i386/helper.c
>>>> @@ -1161,8 +1161,6 @@ X86CPU *cpu_x86_init(const char *cpu_model)
>>>>            return NULL;
>>>>        }
>>>>
>>>> -    x86_cpu_realize(OBJECT(cpu), NULL);
>>>> -
>>>>        return cpu;
>>>>    }
>>>>
>>>
>>> This will require changes in linux-user and possibly bsd-user. Having a
>>> cpu_realize() would probably help with avoiding #ifdef'ery.
>>> Unfortunately deriving CPUState from DeviceState proves a bit difficult
>>> in the meantime (it worked at one point, now there's lots of circular
>>> header dependencies), and realize support for Object got stopped.
>>>
>> As alternative to keep, I could leave x86_cpu_realize() in
>> cpu_x86_init() and keep pc_cpu_reset() in pc_new_cpu(). That will result
>> in calling cpu_reset() 3 instead of 2 times.
>> Later when apic_init is moved inside cpu.c, a pc_cpu_reset() in
>> pc_new_cpu() would be unnecessary and could be cleaned up then.
>
> Let me explain in more detail what I was thinking about: cpu_init() and
> cpu_x86_init() today return an initialized/realized object. I don't want
> bugs to creep into the user emulators because someone is not aware that
> x86 is semantically differing from all other targets.
>
> What I did for a qemu-rl78 machine is to inline cpu_rl78_init() so that
> I could put code in between, i.e., for x86: object_new(), APIC/BSP
> stuff, x86_cpu_realize(). That way any addition to the realize function
> will still affect the user emulators.
> The downside is that when we add x86 CPU subclasses we'd have to
> remember to update two places. The solution to that would be to split
> out a x86_cpu_new() function used from cpu_x86_init() and wherever you
> need it for the PC machine. Then the code is still maintainable in one
> central place and you get to do your APIC cleanups, and we don't depend
> on a central realize implementation or device parent, what do you think?

If you mean x86_cpu_new() == pc_new_cpu() that calls cpu_x86_init(),
then I'd like get rid of pc_new_cpu() completely, eventually replacing it by
cpu_x86_init() in hw/pc.c:pc_cpus_init(), something like this:

-static X86CPU *pc_new_cpu(const char *cpu_model)
-{
-    X86CPU *cpu;
-    CPUX86State *env;
-
-    cpu = cpu_x86_init(cpu_model);
-    if (cpu == NULL) {
-        fprintf(stderr, "Unable to find x86 CPU definition\n");
-        exit(1);
-    }
-    env = &cpu->env;
-    if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
-        env->apic_state = apic_init(env, env->cpuid_apic_id);
-    }
-    cpu_reset(CPU(cpu));
-    return cpu;
-}
-
  void pc_cpus_init(const char *cpu_model)
  {
      int i;
@@ -950,7 +932,7 @@ void pc_cpus_init(const char *cpu_model)
      }

      for(i = 0; i < smp_cpus; i++) {
-        pc_new_cpu(cpu_model);
+        cpu_x86_init(cpu_model);
      }
  }

goal I'm aiming at is to have a working cpu object that could be created
using qdev_device_add without any adhoc calls. So in the end cpu_x86_init()
should become object_new(x86_cpu), [set props], realize() and nothing else.
And maybe in some far future removing cpu_init -> cpu_x86_init() completely.
That would give us a single implementation of CPU one place (cpu.c)
-- 
-----
Regards,
  Igor

Re: [Qemu-devel] [PATCH 3/5] target-i386: call x86_cpu_realize() after APIC is initialized.

[liu ping fan]

On Tue, Jul 10, 2012 at 9:35 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> On 07/09/2012 02:57 PM, Andreas Färber wrote:
>>
>> Am 09.07.2012 12:59, schrieb igor:
>>>
>>> On 06/20/2012 03:35 PM, Andreas Färber wrote:
>>>>
>>>> Am 20.06.2012 14:59, schrieb Igor Mammedov:
>>>>>
>>>>> It's not correct to make CPU runnable (i.e. calling x86_cpu_realize())
>>>>> when not all properties are set (APIC in this case).
>>>>>
>>>>> Fix it by calling x86_cpu_realize() at board level after APIC is
>>>>> initialized, right before cpu_reset().
>>>>>
>>>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>>>> ---
>>>>>    hw/pc.c              |    1 +
>>>>>    target-i386/helper.c |    2 --
>>>>>    2 files changed, 1 insertion(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/hw/pc.c b/hw/pc.c
>>>>> index 8368701..8a662cf 100644
>>>>> --- a/hw/pc.c
>>>>> +++ b/hw/pc.c
>>>>> @@ -948,6 +948,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model)
>>>>>            env->apic_state = apic_init(env, env->cpuid_apic_id);
>>>>>        }
>>>>>        qemu_register_reset(pc_cpu_reset, cpu);
>>>>> +    x86_cpu_realize(OBJECT(cpu), NULL);
>>>>>        pc_cpu_reset(cpu);
>>>>>        return cpu;
>>>>>    }
>>>>> diff --git a/target-i386/helper.c b/target-i386/helper.c
>>>>> index c52ec13..b38ea7f 100644
>>>>> --- a/target-i386/helper.c
>>>>> +++ b/target-i386/helper.c
>>>>> @@ -1161,8 +1161,6 @@ X86CPU *cpu_x86_init(const char *cpu_model)
>>>>>            return NULL;
>>>>>        }
>>>>>
>>>>> -    x86_cpu_realize(OBJECT(cpu), NULL);
>>>>> -
>>>>>        return cpu;
>>>>>    }
>>>>>
>>>>
>>>> This will require changes in linux-user and possibly bsd-user. Having a
>>>> cpu_realize() would probably help with avoiding #ifdef'ery.
>>>> Unfortunately deriving CPUState from DeviceState proves a bit difficult
>>>> in the meantime (it worked at one point, now there's lots of circular
>>>> header dependencies), and realize support for Object got stopped.
>>>>
>>> As alternative to keep, I could leave x86_cpu_realize() in
>>> cpu_x86_init() and keep pc_cpu_reset() in pc_new_cpu(). That will result
>>> in calling cpu_reset() 3 instead of 2 times.
>>> Later when apic_init is moved inside cpu.c, a pc_cpu_reset() in
>>> pc_new_cpu() would be unnecessary and could be cleaned up then.
>>
>>
>> Let me explain in more detail what I was thinking about: cpu_init() and
>> cpu_x86_init() today return an initialized/realized object. I don't want
>> bugs to creep into the user emulators because someone is not aware that
>> x86 is semantically differing from all other targets.
>>
>> What I did for a qemu-rl78 machine is to inline cpu_rl78_init() so that
>> I could put code in between, i.e., for x86: object_new(), APIC/BSP
>> stuff, x86_cpu_realize(). That way any addition to the realize function
>> will still affect the user emulators.
>> The downside is that when we add x86 CPU subclasses we'd have to
>> remember to update two places. The solution to that would be to split
>> out a x86_cpu_new() function used from cpu_x86_init() and wherever you
>> need it for the PC machine. Then the code is still maintainable in one
>> central place and you get to do your APIC cleanups, and we don't depend
>> on a central realize implementation or device parent, what do you think?
>
>
> If you mean x86_cpu_new() == pc_new_cpu() that calls cpu_x86_init(),
> then I'd like get rid of pc_new_cpu() completely, eventually replacing it by
> cpu_x86_init() in hw/pc.c:pc_cpus_init(), something like this:
>
> -static X86CPU *pc_new_cpu(const char *cpu_model)
> -{
> -    X86CPU *cpu;
> -    CPUX86State *env;
> -
> -    cpu = cpu_x86_init(cpu_model);
> -    if (cpu == NULL) {
> -        fprintf(stderr, "Unable to find x86 CPU definition\n");
> -        exit(1);
> -    }
> -    env = &cpu->env;
> -    if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
> -        env->apic_state = apic_init(env, env->cpuid_apic_id);
> -    }
> -    cpu_reset(CPU(cpu));
> -    return cpu;
> -}
> -
>  void pc_cpus_init(const char *cpu_model)
>  {
>      int i;
> @@ -950,7 +932,7 @@ void pc_cpus_init(const char *cpu_model)
>      }
>
>      for(i = 0; i < smp_cpus; i++) {
> -        pc_new_cpu(cpu_model);
> +        cpu_x86_init(cpu_model);
>      }
>  }
>
> goal I'm aiming at is to have a working cpu object that could be created
> using qdev_device_add without any adhoc calls. So in the end cpu_x86_init()
> should become object_new(x86_cpu), [set props], realize() and nothing else.

Could we think apic's "creation + realize" as part of
x86_cpu_realize(), but not [set props]?
For the concept of building sub log unit inside chip.

Regards,
pingfan
> And maybe in some far future removing cpu_init -> cpu_x86_init() completely.
> That would give us a single implementation of CPU one place (cpu.c)
> --
> -----
> Regards,
>  Igor
>
>
>

Re: [Qemu-devel] [PATCH 3/5] target-i386: call x86_cpu_realize() after APIC is initialized.

[Igor Mammedov]

On 07/11/2012 09:35 AM, liu ping fan wrote:
> On Tue, Jul 10, 2012 at 9:35 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>> On 07/09/2012 02:57 PM, Andreas Färber wrote:
>>>
>>> Am 09.07.2012 12:59, schrieb igor:
>>>>
>>>> On 06/20/2012 03:35 PM, Andreas Färber wrote:
>>>>>
>>>>> Am 20.06.2012 14:59, schrieb Igor Mammedov:
>>>>>>
>>>>>> It's not correct to make CPU runnable (i.e. calling x86_cpu_realize())
>>>>>> when not all properties are set (APIC in this case).
>>>>>>
>>>>>> Fix it by calling x86_cpu_realize() at board level after APIC is
>>>>>> initialized, right before cpu_reset().
>>>>>>
>>>>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>>>>> ---
>>>>>>     hw/pc.c              |    1 +
>>>>>>     target-i386/helper.c |    2 --
>>>>>>     2 files changed, 1 insertion(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/pc.c b/hw/pc.c
>>>>>> index 8368701..8a662cf 100644
>>>>>> --- a/hw/pc.c
>>>>>> +++ b/hw/pc.c
>>>>>> @@ -948,6 +948,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model)
>>>>>>             env->apic_state = apic_init(env, env->cpuid_apic_id);
>>>>>>         }
>>>>>>         qemu_register_reset(pc_cpu_reset, cpu);
>>>>>> +    x86_cpu_realize(OBJECT(cpu), NULL);
>>>>>>         pc_cpu_reset(cpu);
>>>>>>         return cpu;
>>>>>>     }
>>>>>> diff --git a/target-i386/helper.c b/target-i386/helper.c
>>>>>> index c52ec13..b38ea7f 100644
>>>>>> --- a/target-i386/helper.c
>>>>>> +++ b/target-i386/helper.c
>>>>>> @@ -1161,8 +1161,6 @@ X86CPU *cpu_x86_init(const char *cpu_model)
>>>>>>             return NULL;
>>>>>>         }
>>>>>>
>>>>>> -    x86_cpu_realize(OBJECT(cpu), NULL);
>>>>>> -
>>>>>>         return cpu;
>>>>>>     }
>>>>>>
>>>>>
>>>>> This will require changes in linux-user and possibly bsd-user. Having a
>>>>> cpu_realize() would probably help with avoiding #ifdef'ery.
>>>>> Unfortunately deriving CPUState from DeviceState proves a bit difficult
>>>>> in the meantime (it worked at one point, now there's lots of circular
>>>>> header dependencies), and realize support for Object got stopped.
>>>>>
>>>> As alternative to keep, I could leave x86_cpu_realize() in
>>>> cpu_x86_init() and keep pc_cpu_reset() in pc_new_cpu(). That will result
>>>> in calling cpu_reset() 3 instead of 2 times.
>>>> Later when apic_init is moved inside cpu.c, a pc_cpu_reset() in
>>>> pc_new_cpu() would be unnecessary and could be cleaned up then.
>>>
>>>
>>> Let me explain in more detail what I was thinking about: cpu_init() and
>>> cpu_x86_init() today return an initialized/realized object. I don't want
>>> bugs to creep into the user emulators because someone is not aware that
>>> x86 is semantically differing from all other targets.
>>>
>>> What I did for a qemu-rl78 machine is to inline cpu_rl78_init() so that
>>> I could put code in between, i.e., for x86: object_new(), APIC/BSP
>>> stuff, x86_cpu_realize(). That way any addition to the realize function
>>> will still affect the user emulators.
>>> The downside is that when we add x86 CPU subclasses we'd have to
>>> remember to update two places. The solution to that would be to split
>>> out a x86_cpu_new() function used from cpu_x86_init() and wherever you
>>> need it for the PC machine. Then the code is still maintainable in one
>>> central place and you get to do your APIC cleanups, and we don't depend
>>> on a central realize implementation or device parent, what do you think?
>>
>>
>> If you mean x86_cpu_new() == pc_new_cpu() that calls cpu_x86_init(),
>> then I'd like get rid of pc_new_cpu() completely, eventually replacing it by
>> cpu_x86_init() in hw/pc.c:pc_cpus_init(), something like this:
>>
>> -static X86CPU *pc_new_cpu(const char *cpu_model)
>> -{
>> -    X86CPU *cpu;
>> -    CPUX86State *env;
>> -
>> -    cpu = cpu_x86_init(cpu_model);
>> -    if (cpu == NULL) {
>> -        fprintf(stderr, "Unable to find x86 CPU definition\n");
>> -        exit(1);
>> -    }
>> -    env = &cpu->env;
>> -    if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
>> -        env->apic_state = apic_init(env, env->cpuid_apic_id);
>> -    }
>> -    cpu_reset(CPU(cpu));
>> -    return cpu;
>> -}
>> -
>>   void pc_cpus_init(const char *cpu_model)
>>   {
>>       int i;
>> @@ -950,7 +932,7 @@ void pc_cpus_init(const char *cpu_model)
>>       }
>>
>>       for(i = 0; i < smp_cpus; i++) {
>> -        pc_new_cpu(cpu_model);
>> +        cpu_x86_init(cpu_model);
>>       }
>>   }
>>
>> goal I'm aiming at is to have a working cpu object that could be created
>> using qdev_device_add without any adhoc calls. So in the end cpu_x86_init()
>> should become object_new(x86_cpu), [set props], realize() and nothing else.
>
> Could we think apic's "creation + realize" as part of
> x86_cpu_realize(), but not [set props]?
> For the concept of building sub log unit inside chip.

Yes, sure.
Please look at https://github.com/imammedo/qemu/tree/x86_qom_apic
it lacks apic_reset() from cpu_reset() but it is easy to add.

>
> Regards,
> pingfan
>> And maybe in some far future removing cpu_init -> cpu_x86_init() completely.
>> That would give us a single implementation of CPU one place (cpu.c)
>> --
>> -----
>> Regards,
>>   Igor
>>
>>
>>

-- 
-----
  Igor

Re: [Qemu-devel] [PATCH 3/5] target-i386: call x86_cpu_realize() after APIC is initialized.

[liu ping fan]

On Wed, Jul 11, 2012 at 8:27 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> On 07/11/2012 09:35 AM, liu ping fan wrote:
>>
>> On Tue, Jul 10, 2012 at 9:35 PM, Igor Mammedov <imammedo@redhat.com>
>> wrote:
>>>
>>> On 07/09/2012 02:57 PM, Andreas Färber wrote:
>>>>
>>>>
>>>> Am 09.07.2012 12:59, schrieb igor:
>>>>>
>>>>>
>>>>> On 06/20/2012 03:35 PM, Andreas Färber wrote:
>>>>>>
>>>>>>
>>>>>> Am 20.06.2012 14:59, schrieb Igor Mammedov:
>>>>>>>
>>>>>>>
>>>>>>> It's not correct to make CPU runnable (i.e. calling
>>>>>>> x86_cpu_realize())
>>>>>>> when not all properties are set (APIC in this case).
>>>>>>>
>>>>>>> Fix it by calling x86_cpu_realize() at board level after APIC is
>>>>>>> initialized, right before cpu_reset().
>>>>>>>
>>>>>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>>>>>> ---
>>>>>>>     hw/pc.c              |    1 +
>>>>>>>     target-i386/helper.c |    2 --
>>>>>>>     2 files changed, 1 insertion(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/pc.c b/hw/pc.c
>>>>>>> index 8368701..8a662cf 100644
>>>>>>> --- a/hw/pc.c
>>>>>>> +++ b/hw/pc.c
>>>>>>> @@ -948,6 +948,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model)
>>>>>>>             env->apic_state = apic_init(env, env->cpuid_apic_id);
>>>>>>>         }
>>>>>>>         qemu_register_reset(pc_cpu_reset, cpu);
>>>>>>> +    x86_cpu_realize(OBJECT(cpu), NULL);
>>>>>>>         pc_cpu_reset(cpu);
>>>>>>>         return cpu;
>>>>>>>     }
>>>>>>> diff --git a/target-i386/helper.c b/target-i386/helper.c
>>>>>>> index c52ec13..b38ea7f 100644
>>>>>>> --- a/target-i386/helper.c
>>>>>>> +++ b/target-i386/helper.c
>>>>>>> @@ -1161,8 +1161,6 @@ X86CPU *cpu_x86_init(const char *cpu_model)
>>>>>>>             return NULL;
>>>>>>>         }
>>>>>>>
>>>>>>> -    x86_cpu_realize(OBJECT(cpu), NULL);
>>>>>>> -
>>>>>>>         return cpu;
>>>>>>>     }
>>>>>>>
>>>>>>
>>>>>> This will require changes in linux-user and possibly bsd-user. Having
>>>>>> a
>>>>>> cpu_realize() would probably help with avoiding #ifdef'ery.
>>>>>> Unfortunately deriving CPUState from DeviceState proves a bit
>>>>>> difficult
>>>>>> in the meantime (it worked at one point, now there's lots of circular
>>>>>> header dependencies), and realize support for Object got stopped.
>>>>>>
>>>>> As alternative to keep, I could leave x86_cpu_realize() in
>>>>> cpu_x86_init() and keep pc_cpu_reset() in pc_new_cpu(). That will
>>>>> result
>>>>> in calling cpu_reset() 3 instead of 2 times.
>>>>> Later when apic_init is moved inside cpu.c, a pc_cpu_reset() in
>>>>> pc_new_cpu() would be unnecessary and could be cleaned up then.
>>>>
>>>>
>>>>
>>>> Let me explain in more detail what I was thinking about: cpu_init() and
>>>> cpu_x86_init() today return an initialized/realized object. I don't want
>>>> bugs to creep into the user emulators because someone is not aware that
>>>> x86 is semantically differing from all other targets.
>>>>
>>>> What I did for a qemu-rl78 machine is to inline cpu_rl78_init() so that
>>>> I could put code in between, i.e., for x86: object_new(), APIC/BSP
>>>> stuff, x86_cpu_realize(). That way any addition to the realize function
>>>> will still affect the user emulators.
>>>> The downside is that when we add x86 CPU subclasses we'd have to
>>>> remember to update two places. The solution to that would be to split
>>>> out a x86_cpu_new() function used from cpu_x86_init() and wherever you
>>>> need it for the PC machine. Then the code is still maintainable in one
>>>> central place and you get to do your APIC cleanups, and we don't depend
>>>> on a central realize implementation or device parent, what do you think?
>>>
>>>
>>>
>>> If you mean x86_cpu_new() == pc_new_cpu() that calls cpu_x86_init(),
>>> then I'd like get rid of pc_new_cpu() completely, eventually replacing it
>>> by
>>> cpu_x86_init() in hw/pc.c:pc_cpus_init(), something like this:
>>>
>>> -static X86CPU *pc_new_cpu(const char *cpu_model)
>>> -{
>>> -    X86CPU *cpu;
>>> -    CPUX86State *env;
>>> -
>>> -    cpu = cpu_x86_init(cpu_model);
>>> -    if (cpu == NULL) {
>>> -        fprintf(stderr, "Unable to find x86 CPU definition\n");
>>> -        exit(1);
>>> -    }
>>> -    env = &cpu->env;
>>> -    if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
>>> -        env->apic_state = apic_init(env, env->cpuid_apic_id);
>>> -    }
>>> -    cpu_reset(CPU(cpu));
>>> -    return cpu;
>>> -}
>>> -
>>>   void pc_cpus_init(const char *cpu_model)
>>>   {
>>>       int i;
>>> @@ -950,7 +932,7 @@ void pc_cpus_init(const char *cpu_model)
>>>       }
>>>
>>>       for(i = 0; i < smp_cpus; i++) {
>>> -        pc_new_cpu(cpu_model);
>>> +        cpu_x86_init(cpu_model);
>>>       }
>>>   }
>>>
>>> goal I'm aiming at is to have a working cpu object that could be created
>>> using qdev_device_add without any adhoc calls. So in the end
>>> cpu_x86_init()
>>> should become object_new(x86_cpu), [set props], realize() and nothing
>>> else.
>>
>>
>> Could we think apic's "creation + realize" as part of
>> x86_cpu_realize(), but not [set props]?
>> For the concept of building sub log unit inside chip.
>
>
> Yes, sure.
> Please look at https://github.com/imammedo/qemu/tree/x86_qom_apic
> it lacks apic_reset() from cpu_reset() but it is easy to add.
>
Just wonder whether it is acceptable to call apic_reset directly , or
before that, we must make CPU a child of DeviceState, then using
qdev_reset_all()

pingfan
>
>>
>> Regards,
>> pingfan
>>>
>>> And maybe in some far future removing cpu_init -> cpu_x86_init()
>>> completely.
>>> That would give us a single implementation of CPU one place (cpu.c)
>>> --
>>> -----
>>> Regards,
>>>   Igor
>>>
>>>
>>>
>
> --
> -----
>  Igor
>
>

Re: [Qemu-devel] [PATCH 3/5] target-i386: call x86_cpu_realize() after APIC is initialized.

[liu ping fan]

On Mon, Jul 9, 2012 at 8:57 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 09.07.2012 12:59, schrieb igor:
>> On 06/20/2012 03:35 PM, Andreas Färber wrote:
>>> Am 20.06.2012 14:59, schrieb Igor Mammedov:
>>>> It's not correct to make CPU runnable (i.e. calling x86_cpu_realize())
>>>> when not all properties are set (APIC in this case).
>>>>
>>>> Fix it by calling x86_cpu_realize() at board level after APIC is
>>>> initialized, right before cpu_reset().
>>>>
>>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>>> ---
>>>>   hw/pc.c              |    1 +
>>>>   target-i386/helper.c |    2 --
>>>>   2 files changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/pc.c b/hw/pc.c
>>>> index 8368701..8a662cf 100644
>>>> --- a/hw/pc.c
>>>> +++ b/hw/pc.c
>>>> @@ -948,6 +948,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model)
>>>>           env->apic_state = apic_init(env, env->cpuid_apic_id);
>>>>       }
>>>>       qemu_register_reset(pc_cpu_reset, cpu);
>>>> +    x86_cpu_realize(OBJECT(cpu), NULL);
>>>>       pc_cpu_reset(cpu);
>>>>       return cpu;
>>>>   }
>>>> diff --git a/target-i386/helper.c b/target-i386/helper.c
>>>> index c52ec13..b38ea7f 100644
>>>> --- a/target-i386/helper.c
>>>> +++ b/target-i386/helper.c
>>>> @@ -1161,8 +1161,6 @@ X86CPU *cpu_x86_init(const char *cpu_model)
>>>>           return NULL;
>>>>       }
>>>>
>>>> -    x86_cpu_realize(OBJECT(cpu), NULL);
>>>> -
>>>>       return cpu;
>>>>   }
>>>>
>>>
>>> This will require changes in linux-user and possibly bsd-user. Having a
>>> cpu_realize() would probably help with avoiding #ifdef'ery.
>>> Unfortunately deriving CPUState from DeviceState proves a bit difficult
>>> in the meantime (it worked at one point, now there's lots of circular
>>> header dependencies), and realize support for Object got stopped.
>>>
>> As alternative to keep, I could leave x86_cpu_realize() in
>> cpu_x86_init() and keep pc_cpu_reset() in pc_new_cpu(). That will result
>> in calling cpu_reset() 3 instead of 2 times.
>> Later when apic_init is moved inside cpu.c, a pc_cpu_reset() in
>> pc_new_cpu() would be unnecessary and could be cleaned up then.
>
> Let me explain in more detail what I was thinking about: cpu_init() and
> cpu_x86_init() today return an initialized/realized object. I don't want
> bugs to creep into the user emulators because someone is not aware that
> x86 is semantically differing from all other targets.
>
> What I did for a qemu-rl78 machine is to inline cpu_rl78_init() so that
> I could put code in between, i.e., for x86: object_new(), APIC/BSP
> stuff, x86_cpu_realize(). That way any addition to the realize function
> will still affect the user emulators.
> The downside is that when we add x86 CPU subclasses we'd have to

What do you mean " add x86 CPU subclasses" ? Derive from class X86CPU
? And any scene for that?

Thanks,
pingfan
> remember to update two places. The solution to that would be to split
> out a x86_cpu_new() function used from cpu_x86_init() and wherever you
> need it for the PC machine. Then the code is still maintainable in one
> central place and you get to do your APIC cleanups, and we don't depend
> on a central realize implementation or device parent, what do you think?
>
> Regards,
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>
>
>

[Qemu-devel] [PATCH 4/5] target-i386: move cpu halted decision into x86_cpu_reset

[Igor Mammedov]

MP initialization protocol differs between cpu families, and for P6 and
onward models it is up to CPU to decide if it will be BSP using this
protocol, so try to model this. However there is no point in implementing
MP initialization protocol in qemu. Thus first CPU is always marked as BSP.

This patch:
 - moves decision to designate BSP from board into cpu, making cpu
self-sufficient in this regard. Later it will allow to cleanup hw/pc.c
and remove cpu_reset and wrappers from there.
 - stores flag that CPU is BSP in IA32_APIC_BASE to model behavior
described in Inted SDM vol 3a part 1 chapter 8.4.1
 - uses MSR_IA32_APICBASE_BSP flag in apic_base for checking if cpu is BSP

patch is based on Jan Kiszka's proposal:
    http://thread.gmane.org/gmane.comp.emulators.qemu/100806

v2:
  - fix build for i386-linux-user
      spotted-by: Peter Maydell <peter.maydell@linaro.org>
v3:
  - style change requested by Andreas
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/apic.h            |    2 +-
 hw/apic_common.c     |   20 ++++++++++++++------
 hw/pc.c              |    9 ---------
 target-i386/cpu.c    |    9 +++++++++
 target-i386/helper.c |    1 -
 target-i386/kvm.c    |    5 +++--
 6 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/hw/apic.h b/hw/apic.h
index 62179ce..d961ed4 100644
--- a/hw/apic.h
+++ b/hw/apic.h
@@ -20,9 +20,9 @@ void apic_init_reset(DeviceState *s);
 void apic_sipi(DeviceState *s);
 void apic_handle_tpr_access_report(DeviceState *d, target_ulong ip,
                                    TPRAccess access);
+void apic_designate_bsp(DeviceState *d);
 
 /* pc.c */
-int cpu_is_bsp(CPUX86State *env);
 DeviceState *cpu_get_current_apic(void);
 
 #endif
diff --git a/hw/apic_common.c b/hw/apic_common.c
index 60b8259..095b09e 100644
--- a/hw/apic_common.c
+++ b/hw/apic_common.c
@@ -43,8 +43,8 @@ uint64_t cpu_get_apic_base(DeviceState *d)
         trace_cpu_get_apic_base((uint64_t)s->apicbase);
         return s->apicbase;
     } else {
-        trace_cpu_get_apic_base(0);
-        return 0;
+        trace_cpu_get_apic_base(MSR_IA32_APICBASE_BSP);
+        return MSR_IA32_APICBASE_BSP;
     }
 }
 
@@ -201,22 +201,30 @@ void apic_init_reset(DeviceState *d)
     s->timer_expiry = -1;
 }
 
+void apic_designate_bsp(DeviceState *d)
+{
+    if (d == NULL) {
+        return;
+    }
+
+    APICCommonState *s = APIC_COMMON(d);
+    s->apicbase |= MSR_IA32_APICBASE_BSP;
+}
+
 static void apic_reset_common(DeviceState *d)
 {
     APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
     APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
-    bool bsp;
 
-    bsp = cpu_is_bsp(s->cpu_env);
     s->apicbase = 0xfee00000 |
-        (bsp ? MSR_IA32_APICBASE_BSP : 0) | MSR_IA32_APICBASE_ENABLE;
+        (s->apicbase & MSR_IA32_APICBASE_BSP) | MSR_IA32_APICBASE_ENABLE;
 
     s->vapic_paddr = 0;
     info->vapic_base_update(s);
 
     apic_init_reset(d);
 
-    if (bsp) {
+    if (s->apicbase & MSR_IA32_APICBASE_BSP) {
         /*
          * LINT0 delivery mode on CPU #0 is set to ExtInt at initialization
          * time typically by BIOS, so PIC interrupt can be delivered to the
diff --git a/hw/pc.c b/hw/pc.c
index 8a662cf..2af066a 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -871,12 +871,6 @@ void pc_init_ne2k_isa(ISABus *bus, NICInfo *nd)
     nb_ne2k++;
 }
 
-int cpu_is_bsp(CPUX86State *env)
-{
-    /* We hard-wire the BSP to the first CPU. */
-    return env->cpu_index == 0;
-}
-
 DeviceState *cpu_get_current_apic(void)
 {
     if (cpu_single_env) {
@@ -927,10 +921,7 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
 static void pc_cpu_reset(void *opaque)
 {
     X86CPU *cpu = opaque;
-    CPUX86State *env = &cpu->env;
-
     cpu_reset(CPU(cpu));
-    env->halted = !cpu_is_bsp(env);
 }
 
 static X86CPU *pc_new_cpu(const char *cpu_model)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 7ace126..d400f4b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1678,6 +1678,15 @@ static void x86_cpu_reset(CPUState *s)
     env->dr[7] = DR7_FIXED_1;
     cpu_breakpoint_remove_all(env, BP_CPU);
     cpu_watchpoint_remove_all(env, BP_CPU);
+
+#if !defined(CONFIG_USER_ONLY)
+    /* We hard-wire the BSP to the first CPU. */
+    if (env->cpu_index == 0) {
+        apic_designate_bsp(env->apic_state);
+    }
+
+    env->halted = !(cpu_get_apic_base(env->apic_state) & MSR_IA32_APICBASE_BSP);
+#endif
 }
 
 static void mce_init(X86CPU *cpu)
diff --git a/target-i386/helper.c b/target-i386/helper.c
index b38ea7f..a8e403d 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1175,7 +1175,6 @@ void do_cpu_init(X86CPU *cpu)
     env->interrupt_request = sipi;
     env->pat = pat;
     apic_init_reset(env->apic_state);
-    env->halted = !cpu_is_bsp(env);
 }
 
 void do_cpu_sipi(X86CPU *cpu)
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 0d0d8f6..09621e5 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -583,8 +583,9 @@ void kvm_arch_reset_vcpu(CPUX86State *env)
     env->interrupt_injected = -1;
     env->xcr0 = 1;
     if (kvm_irqchip_in_kernel()) {
-        env->mp_state = cpu_is_bsp(env) ? KVM_MP_STATE_RUNNABLE :
-                                          KVM_MP_STATE_UNINITIALIZED;
+        env->mp_state =
+            cpu_get_apic_base(env->apic_state) & MSR_IA32_APICBASE_BSP ?
+            KVM_MP_STATE_RUNNABLE : KVM_MP_STATE_UNINITIALIZED;
     } else {
         env->mp_state = KVM_MP_STATE_RUNNABLE;
     }
-- 
1.7.10.2

[Qemu-devel] [PATCH 5/5] target-i386: move cpu_reset and reset callback to cpu.c

[Igor Mammedov]

Moving reset callback into cpu object from board level and
resetting cpu at the end of x86_cpu_realize() will allow properly
create cpu object during run-time (hotplug) without calling reset exteraly.

When reset over QOM hierarchy is implemented, reset callback
should be removed.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/pc.c           |    9 +--------
 target-i386/cpu.c |   14 ++++++++++++++
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 2af066a..adb0730 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -918,12 +918,6 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
     }
 }
 
-static void pc_cpu_reset(void *opaque)
-{
-    X86CPU *cpu = opaque;
-    cpu_reset(CPU(cpu));
-}
-
 static X86CPU *pc_new_cpu(const char *cpu_model)
 {
     X86CPU *cpu;
@@ -938,9 +932,8 @@ static X86CPU *pc_new_cpu(const char *cpu_model)
     if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
         env->apic_state = apic_init(env, env->cpuid_apic_id);
     }
-    qemu_register_reset(pc_cpu_reset, cpu);
+
     x86_cpu_realize(OBJECT(cpu), NULL);
-    pc_cpu_reset(cpu);
     return cpu;
 }
 
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index d400f4b..b4102c3 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1689,6 +1689,15 @@ static void x86_cpu_reset(CPUState *s)
 #endif
 }
 
+#ifndef CONFIG_USER_ONLY
+/* TODO: remove me, when reset over QOM tree is implemented */
+static void x86_cpu_machine_reset_cb(void *opaque)
+{
+    X86CPU *cpu = opaque;
+    cpu_reset(CPU(cpu));
+}
+#endif
+
 static void mce_init(X86CPU *cpu)
 {
     CPUX86State *cenv = &cpu->env;
@@ -1709,8 +1718,13 @@ void x86_cpu_realize(Object *obj, Error **errp)
 {
     X86CPU *cpu = X86_CPU(obj);
 
+#ifndef CONFIG_USER_ONLY
+    qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
+#endif
+
     mce_init(cpu);
     qemu_init_vcpu(&cpu->env);
+    cpu_reset(CPU(cpu));
 }
 
 static void x86_cpu_initfn(Object *obj)
-- 
1.7.10.2

Re: [Qemu-devel] [PATCH 5/5] target-i386: move cpu_reset and reset callback to cpu.c

[Igor Mammedov]

I've forgot to include hunk with hw/qdev.h in cpu.c.
I'll repost.

-- 
-----
  Igor