[Qemu-devel] [PATCH 0/2 v3] target-i386: refactor reset handling and move it into cpu.c

[Qemu-devel] [PATCH 0/2 v3] target-i386: refactor reset handling and move it into cpu.c

[Igor Mammedov]

v2:
  ommited moving of x86_cpu_realize() from cpu_x86_init() to pc_new_cpu(),
  to keep cpu_init implementation in -softmmu and -user targets the same
  in single place and maintanable.

v3:
  reuse cpu_is_bsp() rather than open code check if apicbase has BSP bit set

tree for testing:
  https://github.com/imammedo/qemu/tree/x86_reset_v3

comiple & run tested with x86_64-linux-user, x86_64-softmmu targets

Igor Mammedov (2):
  target-i386: move cpu halted decision into x86_cpu_reset
  target-i386: move cpu_reset and reset callback to cpu.c

 hw/apic.h            |    5 ++++-
 hw/apic_common.c     |   16 +++++++++++++---
 hw/pc.c              |   18 +-----------------
 target-i386/cpu.c    |   30 ++++++++++++++++++++++++++++++
 target-i386/helper.c |    1 -
 target-i386/kvm.c    |    4 +++-
 6 files changed, 51 insertions(+), 23 deletions(-)

[Qemu-devel] [PATCH 1/2] 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

Signed-off-by: Igor Mammedov <imammedo@redhat.com>

---
Changelog:
 v2:
   - fix build for i386-linux-user
      spotted-by: Peter Maydell <peter.maydell@linaro.org>
 v3:
   - style change requested by Andreas Färber <afaerber@suse.de>

 v4:
   - reuse cpu_is_bsp() rather than open code check if apicbase has BSP bit set
      requested by  Gleb Natapov <gleb@redhat.com>
   - hijacked patch [1] to use X86CPU instead of CPUX86State in cpu_is_bsp()

 v5:
   - move Changelog under ---
      requested by: Peter Maydell <peter.maydell@linaro.org>

 1) [PATCH qom-next 06/59] pc: Pass X86CPU to cpu_is_bsp()
   SoB: Andreas Färber <afaerber@suse.de>
   http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg03185.html

 hw/apic.h            |    5 ++++-
 hw/apic_common.c     |   16 +++++++++++++---
 hw/pc.c              |    9 ---------
 target-i386/cpu.c    |   16 ++++++++++++++++
 target-i386/helper.c |    1 -
 target-i386/kvm.c    |    4 +++-
 6 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/hw/apic.h b/hw/apic.h
index a89542b..1d48e02 100644
--- a/hw/apic.h
+++ b/hw/apic.h
@@ -21,9 +21,12 @@ void apic_sipi(DeviceState *s);
 void apic_handle_tpr_access_report(DeviceState *d, target_ulong ip,
                                    TPRAccess access);
 void apic_poll_irq(DeviceState *d);
+void apic_designate_bsp(DeviceState *d);
 
 /* pc.c */
-int cpu_is_bsp(CPUX86State *env);
 DeviceState *cpu_get_current_apic(void);
 
+/* cpu.c */
+bool cpu_is_bsp(X86CPU *cpu);
+
 #endif
diff --git a/hw/apic_common.c b/hw/apic_common.c
index 60b8259..58e63b0 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,13 +201,23 @@ 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);
+    bsp = cpu_is_bsp(x86_env_get_cpu(s->cpu_env));
     s->apicbase = 0xfee00000 |
         (bsp ? MSR_IA32_APICBASE_BSP : 0) | MSR_IA32_APICBASE_ENABLE;
 
diff --git a/hw/pc.c b/hw/pc.c
index 598267a..a920686 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -857,12 +857,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) {
@@ -913,10 +907,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 b3bcbac..cf612a8 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1686,8 +1686,24 @@ 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_is_bsp(cpu);
+#endif
 }
 
+#ifndef CONFIG_USER_ONLY
+bool cpu_is_bsp(X86CPU *cpu)
+{
+    return cpu_get_apic_base(cpu->env.apic_state) & MSR_IA32_APICBASE_BSP;
+}
+#endif
+
 static void mce_init(X86CPU *cpu)
 {
     CPUX86State *cenv = &cpu->env;
diff --git a/target-i386/helper.c b/target-i386/helper.c
index d3af6ea..b748d90 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1191,7 +1191,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 e53c2f6..4cfb3fa 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -584,11 +584,13 @@ int kvm_arch_init_vcpu(CPUX86State *env)
 
 void kvm_arch_reset_vcpu(CPUX86State *env)
 {
+    X86CPU *cpu = x86_env_get_cpu(env);
+
     env->exception_injected = -1;
     env->interrupt_injected = -1;
     env->xcr0 = 1;
     if (kvm_irqchip_in_kernel()) {
-        env->mp_state = cpu_is_bsp(env) ? KVM_MP_STATE_RUNNABLE :
+        env->mp_state = cpu_is_bsp(cpu) ? KVM_MP_STATE_RUNNABLE :
                                           KVM_MP_STATE_UNINITIALIZED;
     } else {
         env->mp_state = KVM_MP_STATE_RUNNABLE;
-- 
1.7.1

[Qemu-devel] [PATCH 2/2] 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 externaly.

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

v2:
  - leave cpu_reset in pc_new_cpu() for now, it's to be cleaned up when APIC
    init is moved in cpu.c

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 a920686..bd193f3 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -904,12 +904,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;
@@ -924,8 +918,7 @@ 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);
-    pc_cpu_reset(cpu);
+    cpu_reset(CPU(cpu));
     return cpu;
 }
 
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index cf612a8..60b8e13 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -31,6 +31,8 @@
 
 #include "hyperv.h"
 
+#include "hw/hw.h"
+
 /* feature flags taken from "Intel Processor Identification and the CPUID
  * Instruction" and AMD's "CPUID Specification".  In cases of disagreement
  * between feature naming conventions, aliases may be added.
@@ -1702,6 +1704,13 @@ bool cpu_is_bsp(X86CPU *cpu)
 {
     return cpu_get_apic_base(cpu->env.apic_state) & MSR_IA32_APICBASE_BSP;
 }
+
+/* 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)
@@ -1724,8 +1733,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.1

Re: [Qemu-devel] [PATCH 0/2 v3] target-i386: refactor reset handling and move it into cpu.c

[Gleb Natapov]

Looks good to me.

On Mon, Jul 23, 2012 at 03:22:26PM +0200, Igor Mammedov wrote:
> v2:
>   ommited moving of x86_cpu_realize() from cpu_x86_init() to pc_new_cpu(),
>   to keep cpu_init implementation in -softmmu and -user targets the same
>   in single place and maintanable.
> 
> v3:
>   reuse cpu_is_bsp() rather than open code check if apicbase has BSP bit set
> 
> tree for testing:
>   https://github.com/imammedo/qemu/tree/x86_reset_v3
> 
> comiple & run tested with x86_64-linux-user, x86_64-softmmu targets
> 
> Igor Mammedov (2):
>   target-i386: move cpu halted decision into x86_cpu_reset
>   target-i386: move cpu_reset and reset callback to cpu.c
> 
>  hw/apic.h            |    5 ++++-
>  hw/apic_common.c     |   16 +++++++++++++---
>  hw/pc.c              |   18 +-----------------
>  target-i386/cpu.c    |   30 ++++++++++++++++++++++++++++++
>  target-i386/helper.c |    1 -
>  target-i386/kvm.c    |    4 +++-
>  6 files changed, 51 insertions(+), 23 deletions(-)

--
			Gleb.

Re: [Qemu-devel] [PATCH 1/2] target-i386: move cpu halted decision into x86_cpu_reset

[Andreas Färber]

Am 23.07.2012 15:22, schrieb 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
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> ---
> Changelog:
>  v2:
>    - fix build for i386-linux-user
>       spotted-by: Peter Maydell <peter.maydell@linaro.org>
>  v3:
>    - style change requested by Andreas Färber <afaerber@suse.de>
> 
>  v4:
>    - reuse cpu_is_bsp() rather than open code check if apicbase has BSP bit set
>       requested by  Gleb Natapov <gleb@redhat.com>
>    - hijacked patch [1] to use X86CPU instead of CPUX86State in cpu_is_bsp()
> 
>  v5:
>    - move Changelog under ---
>       requested by: Peter Maydell <peter.maydell@linaro.org>
> 
>  1) [PATCH qom-next 06/59] pc: Pass X86CPU to cpu_is_bsp()
>    SoB: Andreas Färber <afaerber@suse.de>
>    http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg03185.html

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

I'm fine with this patch, including the squashing of my trivial patch
without SoB, but would appreciate someone who knows the APIC (Jan?) to
add an explicit Acked-by.

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 2/2] target-i386: move cpu_reset and reset callback to cpu.c

[Andreas Färber]

Am 23.07.2012 15:22, schrieb 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 externaly.
> 
> When reset over QOM hierarchy is implemented, reset callback
> should be removed.
> 
> v2:
>   - leave cpu_reset in pc_new_cpu() for now, it's to be cleaned up when APIC
>     init is moved in cpu.c
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

It's still not clear to me what this buys us. Wouldn't it be sufficient
to add the cpu_reset() call in x86_cpu_realize()? Otherwise this defers
from all other machines, so I'd rather do it completely or not at all.
Especially since it's only an interim solution.

Andreas

> ---
>  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 a920686..bd193f3 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -904,12 +904,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;
> @@ -924,8 +918,7 @@ 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);
> -    pc_cpu_reset(cpu);
> +    cpu_reset(CPU(cpu));
>      return cpu;
>  }
>  
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index cf612a8..60b8e13 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -31,6 +31,8 @@
>  
>  #include "hyperv.h"
>  
> +#include "hw/hw.h"
> +
>  /* feature flags taken from "Intel Processor Identification and the CPUID
>   * Instruction" and AMD's "CPUID Specification".  In cases of disagreement
>   * between feature naming conventions, aliases may be added.
> @@ -1702,6 +1704,13 @@ bool cpu_is_bsp(X86CPU *cpu)
>  {
>      return cpu_get_apic_base(cpu->env.apic_state) & MSR_IA32_APICBASE_BSP;
>  }
> +
> +/* 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)
> @@ -1724,8 +1733,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)
> 


-- 
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 0/2 v3] target-i386: refactor reset handling and move it into cpu.c

[Anthony Liguori]

Igor Mammedov <imammedo@redhat.com> writes:

> v2:
>   ommited moving of x86_cpu_realize() from cpu_x86_init() to pc_new_cpu(),
>   to keep cpu_init implementation in -softmmu and -user targets the same
>   in single place and maintanable.
>
> v3:
>   reuse cpu_is_bsp() rather than open code check if apicbase has BSP bit set
>
> tree for testing:
>   https://github.com/imammedo/qemu/tree/x86_reset_v3
>
> comiple & run tested with x86_64-linux-user, x86_64-softmmu targets
>
> Igor Mammedov (2):
>   target-i386: move cpu halted decision into x86_cpu_reset
>   target-i386: move cpu_reset and reset callback to cpu.c

Applied all.  Thanks.

Regards,

Anthony Liguori

>
>  hw/apic.h            |    5 ++++-
>  hw/apic_common.c     |   16 +++++++++++++---
>  hw/pc.c              |   18 +-----------------
>  target-i386/cpu.c    |   30 ++++++++++++++++++++++++++++++
>  target-i386/helper.c |    1 -
>  target-i386/kvm.c    |    4 +++-
>  6 files changed, 51 insertions(+), 23 deletions(-)

Re: [Qemu-devel] [PATCH 0/2 v3] target-i386: refactor reset handling and move it into cpu.c

[Andreas Färber]

Am 01.08.2012 17:43, schrieb Anthony Liguori:
> Igor Mammedov <imammedo@redhat.com> writes:
> 
>> v2:
>>   ommited moving of x86_cpu_realize() from cpu_x86_init() to pc_new_cpu(),
>>   to keep cpu_init implementation in -softmmu and -user targets the same
>>   in single place and maintanable.
>>
>> v3:
>>   reuse cpu_is_bsp() rather than open code check if apicbase has BSP bit set
>>
>> tree for testing:
>>   https://github.com/imammedo/qemu/tree/x86_reset_v3
>>
>> comiple & run tested with x86_64-linux-user, x86_64-softmmu targets
>>
>> Igor Mammedov (2):
>>   target-i386: move cpu halted decision into x86_cpu_reset
>>   target-i386: move cpu_reset and reset callback to cpu.c
> 
> Applied all.  Thanks.

So do you intend to refactor all machines accordingly or leave it
inconsistent now?

Andreas

> 
> Regards,
> 
> Anthony Liguori
> 
>>
>>  hw/apic.h            |    5 ++++-
>>  hw/apic_common.c     |   16 +++++++++++++---
>>  hw/pc.c              |   18 +-----------------
>>  target-i386/cpu.c    |   30 ++++++++++++++++++++++++++++++
>>  target-i386/helper.c |    1 -
>>  target-i386/kvm.c    |    4 +++-
>>  6 files changed, 51 insertions(+), 23 deletions(-)
> 


-- 
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 0/2 v3] target-i386: refactor reset handling and move it into cpu.c

[Anthony Liguori]

Andreas Färber <afaerber@suse.de> writes:

> Am 01.08.2012 17:43, schrieb Anthony Liguori:
>> Igor Mammedov <imammedo@redhat.com> writes:
>> 
>>> v2:
>>>   ommited moving of x86_cpu_realize() from cpu_x86_init() to pc_new_cpu(),
>>>   to keep cpu_init implementation in -softmmu and -user targets the same
>>>   in single place and maintanable.
>>>
>>> v3:
>>>   reuse cpu_is_bsp() rather than open code check if apicbase has BSP bit set
>>>
>>> tree for testing:
>>>   https://github.com/imammedo/qemu/tree/x86_reset_v3
>>>
>>> comiple & run tested with x86_64-linux-user, x86_64-softmmu targets
>>>
>>> Igor Mammedov (2):
>>>   target-i386: move cpu halted decision into x86_cpu_reset
>>>   target-i386: move cpu_reset and reset callback to cpu.c
>> 
>> Applied all.  Thanks.
>
> So do you intend to refactor all machines accordingly or leave it
> inconsistent now?

Are you asking me?

No, I have no intention of touching any other machine.  We're not going
to limit cleaning up target-i386 unless every other machine is cleaned
up too.

Reset logic should live in the CPU.  Seems like a no-brainer to me.

Regards,

Anthony Liguori

>
> Andreas
>
>> 
>> Regards,
>> 
>> Anthony Liguori
>> 
>>>
>>>  hw/apic.h            |    5 ++++-
>>>  hw/apic_common.c     |   16 +++++++++++++---
>>>  hw/pc.c              |   18 +-----------------
>>>  target-i386/cpu.c    |   30 ++++++++++++++++++++++++++++++
>>>  target-i386/helper.c |    1 -
>>>  target-i386/kvm.c    |    4 +++-
>>>  6 files changed, 51 insertions(+), 23 deletions(-)
>> 
>
>
> -- 
> 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 0/2 v3] target-i386: refactor reset handling and move it into cpu.c

[Andreas Färber]

Am 01.08.2012 20:25, schrieb Anthony Liguori:
> Andreas Färber <afaerber@suse.de> writes:
> 
>> Am 01.08.2012 17:43, schrieb Anthony Liguori:
>>> Igor Mammedov <imammedo@redhat.com> writes:
>>>
>>>> v2:
>>>>   ommited moving of x86_cpu_realize() from cpu_x86_init() to pc_new_cpu(),
>>>>   to keep cpu_init implementation in -softmmu and -user targets the same
>>>>   in single place and maintanable.
>>>>
>>>> v3:
>>>>   reuse cpu_is_bsp() rather than open code check if apicbase has BSP bit set
>>>>
>>>> tree for testing:
>>>>   https://github.com/imammedo/qemu/tree/x86_reset_v3
>>>>
>>>> comiple & run tested with x86_64-linux-user, x86_64-softmmu targets
>>>>
>>>> Igor Mammedov (2):
>>>>   target-i386: move cpu halted decision into x86_cpu_reset
>>>>   target-i386: move cpu_reset and reset callback to cpu.c
>>>
>>> Applied all.  Thanks.
>>
>> So do you intend to refactor all machines accordingly or leave it
>> inconsistent now?
> 
> Are you asking me?
> 
> No, I have no intention of touching any other machine.  We're not going
> to limit cleaning up target-i386 unless every other machine is cleaned
> up too.
> 
> Reset logic should live in the CPU.  Seems like a no-brainer to me.

Yes, I'm asking you, since you replied and applied the series without
responding to my review comment on patch 2/2. You probably applied it
locally before reading my comments but then I would still have expected
a reply on how to proceed in light of those comments:

Before applying this, as I've pointed out to Igor at least once before,
all machines do such reset handling themselves. Patch 2/2 that you
applied makes target-i386 break away from that scheme. (I wonder that
Peter hasn't protested yet...)

Anyway, that being the last patch in this series, I see no value in
doing this on its own for target-i386 only. So now we should either
revert that patch and later replace it with one that does a touch-all
change across the boards, or someone needs to volunteer (and you agree,
during the Freeze) to refactor all other machines accordingly, which
will take a while to get Acked-bys from machine maintainers... Or just
defer touching reset callbacks until we have the CPU as a device and
then drop the callbacks instead of moving them.

Note the point of disagreement here is not "reset logic" - it's great
that the APIC BSP fiddling is gone from PC with patch 1/2 - but the
registration of system-level callbacks in cpu.c in patch 2/2. I thought
we all agreed that we want to make CPU a device and have it reset as a
device? No such callback in cpu.c will be needed then and we thus seem
to be, in absence of follow-ups for 1.2, needlessly moving to-be-dead
code around. Not doing that seems like a no-brainer to me.

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 0/2 v3] target-i386: refactor reset handling and move it into cpu.c

[Anthony Liguori]

Andreas Färber <afaerber@suse.de> writes:

> Am 01.08.2012 20:25, schrieb Anthony Liguori:
>> Andreas Färber <afaerber@suse.de> writes:
>> 
>>> Am 01.08.2012 17:43, schrieb Anthony Liguori:
>>>> Igor Mammedov <imammedo@redhat.com> writes:
>>>>
>>>>> v2:
>>>>>   ommited moving of x86_cpu_realize() from cpu_x86_init() to pc_new_cpu(),
>>>>>   to keep cpu_init implementation in -softmmu and -user targets the same
>>>>>   in single place and maintanable.
>>>>>
>>>>> v3:
>>>>>   reuse cpu_is_bsp() rather than open code check if apicbase has BSP bit set
>>>>>
>>>>> tree for testing:
>>>>>   https://github.com/imammedo/qemu/tree/x86_reset_v3
>>>>>
>>>>> comiple & run tested with x86_64-linux-user, x86_64-softmmu targets
>>>>>
>>>>> Igor Mammedov (2):
>>>>>   target-i386: move cpu halted decision into x86_cpu_reset
>>>>>   target-i386: move cpu_reset and reset callback to cpu.c
>>>>
>>>> Applied all.  Thanks.
>>>
>>> So do you intend to refactor all machines accordingly or leave it
>>> inconsistent now?
>> 
>> Are you asking me?
>> 
>> No, I have no intention of touching any other machine.  We're not going
>> to limit cleaning up target-i386 unless every other machine is cleaned
>> up too.
>> 
>> Reset logic should live in the CPU.  Seems like a no-brainer to me.
>
> Yes, I'm asking you, since you replied and applied the series without
> responding to my review comment on patch 2/2. You probably applied it
> locally before reading my comments but then I would still have expected
> a reply on how to proceed in light of those comments:

No, I saw your comment, although I had already decided to apply it by
then.

> Before applying this, as I've pointed out to Igor at least once before,
> all machines do such reset handling themselves. Patch 2/2 that you
> applied makes target-i386 break away from that scheme. (I wonder that
> Peter hasn't protested yet...)

Devices manage their own reset.  CPUs are just another type of device.
It's completely logically that CPUs handle their own reset.

> Anyway, that being the last patch in this series, I see no value in
> doing this on its own for target-i386 only.

There's obvious value.  You would prefer all targets get refactored
too.  But that's an unrealistic expectation to place on contributors.

> So now we should either
> revert that patch and later replace it with one that does a touch-all
> change across the boards, or someone needs to volunteer (and you agree,
> during the Freeze) to refactor all other machines accordingly, which
> will take a while to get Acked-bys from machine maintainers... Or just
> defer touching reset callbacks until we have the CPU as a device and
> then drop the callbacks instead of moving them.

Sorry, but no, this is completely unreasonable.  Fighting against
improvements because you want more to be improved is
counter-productive.  No step in the right direction is too small.

> Note the point of disagreement here is not "reset logic" - it's great
> that the APIC BSP fiddling is gone from PC with patch 1/2 - but the
> registration of system-level callbacks in cpu.c in patch 2/2. I thought
> we all agreed that we want to make CPU a device and have it reset as a
> device? No such callback in cpu.c will be needed then and we thus seem
> to be, in absence of follow-ups for 1.2, needlessly moving to-be-dead
> code around. Not doing that seems like a no-brainer to me.

Devices do one of two things today:

1) register a reset callback

2) implement a reset method that is invoked through it's parent bus

Since I don't expect CPUs to exist on a bus, it's not immediately clear
to me that (1) isn't going to be what we do for quite some time.

Regards,

Anthony Liguori

>
> 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 0/2 v3] target-i386: refactor reset handling and move it into cpu.c

[Andreas Färber]

Am 01.08.2012 22:02, schrieb Anthony Liguori:
> Andreas Färber <afaerber@suse.de> writes:
> 
>> Am 01.08.2012 20:25, schrieb Anthony Liguori:
>>> Andreas Färber <afaerber@suse.de> writes:
>>>
>>>> Am 01.08.2012 17:43, schrieb Anthony Liguori:
>>>>> Igor Mammedov <imammedo@redhat.com> writes:
>>>>>
>>>>>> v2:
>>>>>>   ommited moving of x86_cpu_realize() from cpu_x86_init() to pc_new_cpu(),
>>>>>>   to keep cpu_init implementation in -softmmu and -user targets the same
>>>>>>   in single place and maintanable.
>>>>>>
>>>>>> v3:
>>>>>>   reuse cpu_is_bsp() rather than open code check if apicbase has BSP bit set
>>>>>>
>>>>>> tree for testing:
>>>>>>   https://github.com/imammedo/qemu/tree/x86_reset_v3
>>>>>>
>>>>>> comiple & run tested with x86_64-linux-user, x86_64-softmmu targets
>>>>>>
>>>>>> Igor Mammedov (2):
>>>>>>   target-i386: move cpu halted decision into x86_cpu_reset
>>>>>>   target-i386: move cpu_reset and reset callback to cpu.c
>>>>>
>>>>> Applied all.  Thanks.
>>>>
>>>> So do you intend to refactor all machines accordingly or leave it
>>>> inconsistent now?
>>>
>>> Are you asking me?
>>>
>>> No, I have no intention of touching any other machine.  We're not going
>>> to limit cleaning up target-i386 unless every other machine is cleaned
>>> up too.
>>>
>>> Reset logic should live in the CPU.  Seems like a no-brainer to me.
>>
>> Yes, I'm asking you, since you replied and applied the series without
>> responding to my review comment on patch 2/2. You probably applied it
>> locally before reading my comments but then I would still have expected
>> a reply on how to proceed in light of those comments:
> 
> No, I saw your comment, although I had already decided to apply it by
> then.
> 
>> Before applying this, as I've pointed out to Igor at least once before,
>> all machines do such reset handling themselves. Patch 2/2 that you
>> applied makes target-i386 break away from that scheme. (I wonder that
>> Peter hasn't protested yet...)
> 
> Devices manage their own reset.  CPUs are just another type of device.
> It's completely logically that CPUs handle their own reset.
> 
>> Anyway, that being the last patch in this series, I see no value in
>> doing this on its own for target-i386 only.
> 
> There's obvious value.  You would prefer all targets get refactored
> too.  But that's an unrealistic expectation to place on contributors.
> 
>> So now we should either
>> revert that patch and later replace it with one that does a touch-all
>> change across the boards, or someone needs to volunteer (and you agree,
>> during the Freeze) to refactor all other machines accordingly, which
>> will take a while to get Acked-bys from machine maintainers... Or just
>> defer touching reset callbacks until we have the CPU as a device and
>> then drop the callbacks instead of moving them.
> 
> Sorry, but no, this is completely unreasonable.  Fighting against
> improvements because you want more to be improved is
> counter-productive.  No step in the right direction is too small.
> 
>> Note the point of disagreement here is not "reset logic" - it's great
>> that the APIC BSP fiddling is gone from PC with patch 1/2 - but the
>> registration of system-level callbacks in cpu.c in patch 2/2. I thought
>> we all agreed that we want to make CPU a device and have it reset as a
>> device? No such callback in cpu.c will be needed then and we thus seem
>> to be, in absence of follow-ups for 1.2, needlessly moving to-be-dead
>> code around. Not doing that seems like a no-brainer to me.
> 
> Devices do one of two things today:
> 
> 1) register a reset callback
> 
> 2) implement a reset method that is invoked through it's parent bus
> 
> Since I don't expect CPUs to exist on a bus, it's not immediately clear
> to me that (1) isn't going to be what we do for quite some time.

Err, I thought devices implement a function assigned to a
DeviceClass::reset, no? That would be (2) on your list and we've been
working on ripping out (1) for devices, on sPAPR for instance.
(2) is what we already have with CPUClass::reset.

The only remaining issue is that the CPUClass::reset callback is not
automatically called on machine/bus reset yet.

And what I was saying is that moving the code is NOT an improvement. It
is NO functional change and it is NOT a prerequisite for any change on
the list today. So it is not needed for the to be released 1.2.

A very low hanging fruit for 1.2 would be to register a SINGLE central
reset callback that iterates through the globally available CPU list and
calls ->reset on each! Then we can drop the reset callbacks in most
machines rather than moving old code around.

Regards,
Andreas

> 
> Regards,
> 
> Anthony Liguori
> 
>>
>> 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
> 


-- 
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 0/2 v3] target-i386: refactor reset handling and move it into cpu.c

[Anthony Liguori]

Andreas Färber <afaerber@suse.de> writes:

> Am 01.08.2012 22:02, schrieb Anthony Liguori:
>> Devices do one of two things today:
>> 
>> 1) register a reset callback
>> 
>> 2) implement a reset method that is invoked through it's parent bus
>> 
>> Since I don't expect CPUs to exist on a bus, it's not immediately clear
>> to me that (1) isn't going to be what we do for quite some time.
>
> Err, I thought devices implement a function assigned to a
> DeviceClass::reset, no? That would be (2) on your list and we've been
> working on ripping out (1) for devices, on sPAPR for instance.
> (2) is what we already have with CPUClass::reset.

Something has to call DeviceClass::reset.  That's done through a
BusState.  Whenever a bus is created, a qemu_register_reset() call is
made to invoke the reset method on any device that's part of the bus.

So just implementing DeviceClass::reset doesn't automatically mean the
reset function will be called.  In the short term, I think we'll need to
still register a reset handler.

> The only remaining issue is that the CPUClass::reset callback is not
> automatically called on machine/bus reset yet.
>
> And what I was saying is that moving the code is NOT an improvement. It
> is NO functional change and it is NOT a prerequisite for any change on
> the list today. So it is not needed for the to be released 1.2.
>
> A very low hanging fruit for 1.2 would be to register a SINGLE central
> reset callback that iterates through the globally available CPU list and
> calls ->reset on each! Then we can drop the reset callbacks in most
> machines rather than moving old code around.

Relying on the CPU list for this isn't very QOM-like.  A better approach
would be to make all CPUs appear in a container and then have the reset
propagate through container.

Reset is a complicated beast.  While we model a single reset line today,
this isn't technically correct.  I believe the distinction between reset
types start to matter with PCI-e actually.

I do think any reduction in what's happening in machine is a net win.
Even if we refactor this later, having the machine code do less and
devices do more is an improvement.

Regards,

Anthony Liguori

>
> Regards,
> Andreas
>
>> 
>> Regards,
>> 
>> Anthony Liguori
>> 
>>>
>>> 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
>> 
>
>
> -- 
> 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 0/2 v3] target-i386: refactor reset handling and move it into cpu.c

[Andreas Färber]

Am 01.08.2012 22:02, schrieb Anthony Liguori:
> Andreas Färber <afaerber@suse.de> writes:
> 
>> Am 01.08.2012 20:25, schrieb Anthony Liguori:
>>> Andreas Färber <afaerber@suse.de> writes:
>>>
>>>> Am 01.08.2012 17:43, schrieb Anthony Liguori:
>>>>> Igor Mammedov <imammedo@redhat.com> writes:
>>>>>
>>>>>> v2:
>>>>>>   ommited moving of x86_cpu_realize() from cpu_x86_init() to pc_new_cpu(),
>>>>>>   to keep cpu_init implementation in -softmmu and -user targets the same
>>>>>>   in single place and maintanable.
>>>>>>
>>>>>> v3:
>>>>>>   reuse cpu_is_bsp() rather than open code check if apicbase has BSP bit set
>>>>>>
>>>>>> tree for testing:
>>>>>>   https://github.com/imammedo/qemu/tree/x86_reset_v3
>>>>>>
>>>>>> comiple & run tested with x86_64-linux-user, x86_64-softmmu targets
>>>>>>
>>>>>> Igor Mammedov (2):
>>>>>>   target-i386: move cpu halted decision into x86_cpu_reset
>>>>>>   target-i386: move cpu_reset and reset callback to cpu.c
>>>>>
>>>>> Applied all.  Thanks.
>>>>
>>>> So do you intend to refactor all machines accordingly or leave it
>>>> inconsistent now?
>>>
>>> Are you asking me?
>>>
>>> No, I have no intention of touching any other machine.  We're not going
>>> to limit cleaning up target-i386 unless every other machine is cleaned
>>> up too.
>>>
>>> Reset logic should live in the CPU.  Seems like a no-brainer to me.
>>
>> Yes, I'm asking you, since you replied and applied the series without
>> responding to my review comment on patch 2/2. You probably applied it
>> locally before reading my comments but then I would still have expected
>> a reply on how to proceed in light of those comments:
> 
> No, I saw your comment, although I had already decided to apply it by
> then.

If you did read it then you forgot my Reviewed-by on 1/2. :(

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 0/2 v3] target-i386: refactor reset handling and move it into cpu.c

[Anthony Liguori]

Andreas Färber <afaerber@suse.de> writes:

> Am 01.08.2012 22:02, schrieb Anthony Liguori:
>> Andreas Färber <afaerber@suse.de> writes:
>> 
>>> Am 01.08.2012 20:25, schrieb Anthony Liguori:
>>>> Andreas Färber <afaerber@suse.de> writes:
>>>>
>>>>> Am 01.08.2012 17:43, schrieb Anthony Liguori:
>>>>>> Igor Mammedov <imammedo@redhat.com> writes:
>>>>>>
>>>>>>> v2:
>>>>>>>   ommited moving of x86_cpu_realize() from cpu_x86_init() to pc_new_cpu(),
>>>>>>>   to keep cpu_init implementation in -softmmu and -user targets the same
>>>>>>>   in single place and maintanable.
>>>>>>>
>>>>>>> v3:
>>>>>>>   reuse cpu_is_bsp() rather than open code check if apicbase has BSP bit set
>>>>>>>
>>>>>>> tree for testing:
>>>>>>>   https://github.com/imammedo/qemu/tree/x86_reset_v3
>>>>>>>
>>>>>>> comiple & run tested with x86_64-linux-user, x86_64-softmmu targets
>>>>>>>
>>>>>>> Igor Mammedov (2):
>>>>>>>   target-i386: move cpu halted decision into x86_cpu_reset
>>>>>>>   target-i386: move cpu_reset and reset callback to cpu.c
>>>>>>
>>>>>> Applied all.  Thanks.
>>>>>
>>>>> So do you intend to refactor all machines accordingly or leave it
>>>>> inconsistent now?
>>>>
>>>> Are you asking me?
>>>>
>>>> No, I have no intention of touching any other machine.  We're not going
>>>> to limit cleaning up target-i386 unless every other machine is cleaned
>>>> up too.
>>>>
>>>> Reset logic should live in the CPU.  Seems like a no-brainer to me.
>>>
>>> Yes, I'm asking you, since you replied and applied the series without
>>> responding to my review comment on patch 2/2. You probably applied it
>>> locally before reading my comments but then I would still have expected
>>> a reply on how to proceed in light of those comments:
>> 
>> No, I saw your comment, although I had already decided to apply it by
>> then.
>
> If you did read it then you forgot my Reviewed-by on 1/2. :(

Oh, sorry, I switched to a new patch apply script.  Maybe it's not
working correctly.  I'll look into it.

Regards,

Anthony Liguori

>
> 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 0/2 v3] target-i386: refactor reset handling and move it into cpu.c

[Andreas Färber]

Am 01.08.2012 22:47, schrieb Anthony Liguori:
> Andreas Färber <afaerber@suse.de> writes:
> 
>> Am 01.08.2012 22:02, schrieb Anthony Liguori:
>>> Devices do one of two things today:
>>>
>>> 1) register a reset callback
>>>
>>> 2) implement a reset method that is invoked through it's parent bus
>>>
>>> Since I don't expect CPUs to exist on a bus, it's not immediately clear
>>> to me that (1) isn't going to be what we do for quite some time.
>>
>> Err, I thought devices implement a function assigned to a
>> DeviceClass::reset, no? That would be (2) on your list and we've been
>> working on ripping out (1) for devices, on sPAPR for instance.
>> (2) is what we already have with CPUClass::reset.
> 
> Something has to call DeviceClass::reset.  That's done through a
> BusState.  Whenever a bus is created, a qemu_register_reset() call is
> made to invoke the reset method on any device that's part of the bus.
> 
> So just implementing DeviceClass::reset doesn't automatically mean the
> reset function will be called.  In the short term, I think we'll need to
> still register a reset handler.
> 
>> The only remaining issue is that the CPUClass::reset callback is not
>> automatically called on machine/bus reset yet.
>>
>> And what I was saying is that moving the code is NOT an improvement. It
>> is NO functional change and it is NOT a prerequisite for any change on
>> the list today. So it is not needed for the to be released 1.2.
>>
>> A very low hanging fruit for 1.2 would be to register a SINGLE central
>> reset callback that iterates through the globally available CPU list and
>> calls ->reset on each! Then we can drop the reset callbacks in most
>> machines rather than moving old code around.
> 
> Relying on the CPU list for this isn't very QOM-like.  A better approach
> would be to make all CPUs appear in a container and then have the reset
> propagate through container.

That doesn't work since our CPU modelling was going to be machine/SoC
specific. For x86, agreement seemed to be /machine/cpu[n]. For ARM,
Peter requested path/to/SoC/cortex/cpu[n].

I have just posted (and tested on x86_64) a really trivial patch that
avoids walking the CPU list, reuses existing infrastructure and lets us
drop the reset registration from target-i386, too. More reset callback
registrations can then be dropped as follow-ups from other machines.

Whether we walk a list or not, my point is to reduce redundancy. If it's
in one central location we can easily exchange the implementation. And I
do believe that calling my QOM method is very QOM-like rather than
besmearing my nice new CPUClass::reset concept by unnecessarily
introducing ugly old-style callbacks to my shiny QOM realize code. :)

Regards,
Andreas

> 
> Reset is a complicated beast.  While we model a single reset line today,
> this isn't technically correct.  I believe the distinction between reset
> types start to matter with PCI-e actually.
> 
> I do think any reduction in what's happening in machine is a net win.
> Even if we refactor this later, having the machine code do less and
> devices do more is an improvement.
> 
> Regards,
> 
> Anthony Liguori
> 
>>
>> Regards,
>> Andreas
>>
>>>
>>> Regards,
>>>
>>> Anthony Liguori
>>>
>>>>
>>>> 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
>>>
>>
>>
>> -- 
>> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
>> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
> 


-- 
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 0/2 v3] target-i386: refactor reset handling and move it into cpu.c

[Peter Maydell]

On 1 August 2012 22:25, Andreas Färber <afaerber@suse.de> wrote:
> Am 01.08.2012 22:47, schrieb Anthony Liguori:
>> Relying on the CPU list for this isn't very QOM-like.  A better approach
>> would be to make all CPUs appear in a container and then have the reset
>> propagate through container.
>
> That doesn't work since our CPU modelling was going to be machine/SoC
> specific. For x86, agreement seemed to be /machine/cpu[n]. For ARM,
> Peter requested path/to/SoC/cortex/cpu[n].

I don't think I got that specific (and I definitely wouldn't have
suggested using 'cortex' outside the context of a product name like that).
I do think that there should be a container with the 4 (or whatever) CPUs,
4 timers, GIC, etc, similarly to the way the hardware is a selfcontained
unit. (And that unit would then sit in a container with the other devices
that live in the SoC). I don't think that's hugely different from how
an x86 model would look.

(The phrasing I tend to use is "one cpu with 4 cores", but if QEMU
in general is going to call a single cpu core a "cpu" that's fine.
We do need a term for the thing with all the cores, though...)

>> Reset is a complicated beast.  While we model a single reset line today,
>> this isn't technically correct.  I believe the distinction between reset
>> types start to matter with PCI-e actually.

We already have one system (exynos) that would like to model a
difference between system hard reset and warm reset of some kind.

But really unless we want to bite the bullet and actually
model reset properly (ie as a set of Pins wired up by the machine
model, with no particular magic behaviour) it's always going to be
a 'this kind of works and isn't too ugly to deal with' compromise,
and I don't have very strong feelings about exactly how we
compromise.

-- PMM

Re: [Qemu-devel] [PATCH 0/2 v3] target-i386: refactor reset handling and move it into cpu.c

[Andreas Färber]

Am 01.08.2012 23:43, schrieb Peter Maydell:
> On 1 August 2012 22:25, Andreas Färber <afaerber@suse.de> wrote:
>> Am 01.08.2012 22:47, schrieb Anthony Liguori:
>>> Relying on the CPU list for this isn't very QOM-like.  A better approach
>>> would be to make all CPUs appear in a container and then have the reset
>>> propagate through container.
>>
>> That doesn't work since our CPU modelling was going to be machine/SoC
>> specific. For x86, agreement seemed to be /machine/cpu[n]. For ARM,
>> Peter requested path/to/SoC/cortex/cpu[n].
> 
> I don't think I got that specific (and I definitely wouldn't have
> suggested using 'cortex' outside the context of a product name like that).

http://lists.gnu.org/archive/html/qemu-devel/2012-03/msg02470.html

Context was Exynos and it was <soc>/cortex-a9/core[n] to be exact; I had
suggested to place them onto the SoC directly, you wanted to refine that.

My point here is that the container is not just for the CPUs.
And if we go ahead with /machine for x86 then for the time being that is
not a device and as an Object does not have a reset mechanism that could
trigger the CPUs' reset.

I am leaning towards that a SoC should be a DeviceState but am unclear
whether a SoC should have a central reset callback triggering the CPUs'
reset. Doesn't sound like real hardware behavior to me.

> I do think that there should be a container with the 4 (or whatever) CPUs,
> 4 timers, GIC, etc, similarly to the way the hardware is a selfcontained
> unit. (And that unit would then sit in a container with the other devices
> that live in the SoC). I don't think that's hugely different from how
> an x86 model would look.
> 
> (The phrasing I tend to use is "one cpu with 4 cores", but if QEMU
> in general is going to call a single cpu core a "cpu" that's fine.
> We do need a term for the thing with all the cores, though...)

We had that discussion too and Anthony had some aggressive refactoring
ideas CPU -> core -> thread, but I see that in the pretty distant future
still. We haven't even managed to get a single data field into CPUState
yet, which would be a prerequisite for splitting it up - my series until
today conflicting with Igor's APIC refactoring. (Still working on making
at least a baby step into that direction for 1.2!)

>>> Reset is a complicated beast.  While we model a single reset line today,
>>> this isn't technically correct.  I believe the distinction between reset
>>> types start to matter with PCI-e actually.
> 
> We already have one system (exynos) that would like to model a
> difference between system hard reset and warm reset of some kind.
> 
> But really unless we want to bite the bullet and actually
> model reset properly (ie as a set of Pins wired up by the machine
> model, with no particular magic behaviour) it's always going to be
> a 'this kind of works and isn't too ugly to deal with' compromise,
> and I don't have very strong feelings about exactly how we
> compromise.

Well, we can easily add more callback hooks to CPUClass and/or change
the implementation of my central cpu_reset(). A clear advantage over the
old decentral cpu_[state_]reset(). :-)

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 1/2] target-i386: move cpu halted decision into x86_cpu_reset

[Igor Mammedov]

On 08/01/2012 04:00 PM, Andreas Färber wrote:
> Am 23.07.2012 15:22, schrieb 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
>>
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>
>> ---
>> Changelog:
>>   v2:
>>     - fix build for i386-linux-user
>>        spotted-by: Peter Maydell <peter.maydell@linaro.org>
>>   v3:
>>     - style change requested by Andreas Färber <afaerber@suse.de>
>>
>>   v4:
>>     - reuse cpu_is_bsp() rather than open code check if apicbase has BSP bit set
>>        requested by  Gleb Natapov <gleb@redhat.com>
>>     - hijacked patch [1] to use X86CPU instead of CPUX86State in cpu_is_bsp()
>>
>>   v5:
>>     - move Changelog under ---
>>        requested by: Peter Maydell <peter.maydell@linaro.org>
>>
>>   1) [PATCH qom-next 06/59] pc: Pass X86CPU to cpu_is_bsp()
>>     SoB: Andreas Färber <afaerber@suse.de>
>>     http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg03185.html
>
> Reviewed-by: Andreas Färber <afaerber@suse.de>
>
> I'm fine with this patch, including the squashing of my trivial patch
> without SoB, but would appreciate someone who knows the APIC (Jan?) to
> add an explicit Acked-by.
Andreas,

I apologise for not including explicitly your SoB here. Next time I'll 
ask if SoB should be included.

> Andreas
>

Re: [Qemu-devel] [PATCH 0/2 v3] target-i386: refactor reset handling and move it into cpu.c

[Igor Mammedov]

On 08/01/2012 11:43 PM, Peter Maydell wrote:
> On 1 August 2012 22:25, Andreas Färber <afaerber@suse.de> wrote:
>> Am 01.08.2012 22:47, schrieb Anthony Liguori:
>>> Relying on the CPU list for this isn't very QOM-like.  A better approach
>>> would be to make all CPUs appear in a container and then have the reset
>>> propagate through container.
>>
>> That doesn't work since our CPU modelling was going to be machine/SoC
>> specific. For x86, agreement seemed to be /machine/cpu[n]. For ARM,
>> Peter requested path/to/SoC/cortex/cpu[n].
>
> I don't think I got that specific (and I definitely wouldn't have
> suggested using 'cortex' outside the context of a product name like that).
> I do think that there should be a container with the 4 (or whatever) CPUs,
> 4 timers, GIC, etc, similarly to the way the hardware is a selfcontained
> unit. (And that unit would then sit in a container with the other devices
> that live in the SoC). I don't think that's hugely different from how
> an x86 model would look.
>
> (The phrasing I tend to use is "one cpu with 4 cores", but if QEMU
> in general is going to call a single cpu core a "cpu" that's fine.
> We do need a term for the thing with all the cores, though...)
IMHO: CPU in qemu currently is more like a logical CPU.
And x86 target might need a container for them as well if we are ever to 
model CPU hotplug at socket level. It could be useful for NUMA modelling 
as well.

>
>>> Reset is a complicated beast.  While we model a single reset line today,
>>> this isn't technically correct.  I believe the distinction between reset
>>> types start to matter with PCI-e actually.
>
> We already have one system (exynos) that would like to model a
> difference between system hard reset and warm reset of some kind.
>
> But really unless we want to bite the bullet and actually
> model reset properly (ie as a set of Pins wired up by the machine
> model, with no particular magic behaviour) it's always going to be
> a 'this kind of works and isn't too ugly to deal with' compromise,
> and I don't have very strong feelings about exactly how we
> compromise.
>
> -- PMM
>

Re: [Qemu-devel] [PATCH 1/2] target-i386: move cpu halted decision into x86_cpu_reset

[Igor Mammedov]

On 07/12/2012 08:38 AM, Gleb Natapov wrote:
> On Tue, Jul 10, 2012 at 03:15:51PM +0200, Igor Mammedov wrote:
>>   /* pc.c */
>> -int cpu_is_bsp(CPUX86State *env);
> Why remove it instead of modifying it to check BSP bit in apic base?
> I think it will make the patch smaller and open code the check does not
> look nice.

It's smaller than if I keep cpu_is_bsp() because keeping it would require
as minimum adapting Andreas' patch [1] and without cpu_is_bsp() there won't
be need in it as well.

plain check doesn't look horrible though. It's documented in patch
description and Intel's SDM also mentions APIC base and BSP bit in it. So
may be it's better to use it this way.

Anyway,
I've made a version that keeps cpu_is_bsp() with a bits from [1].
I'll send it as followup to this email, please see if that way is any better.


1) [PATCH qom-next 06/59] pc: Pass X86CPU to cpu_is_bsp()
      http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg03185.html
-- 
-----
  Igor

Re: [Qemu-devel] [PATCH 1/2] target-i386: move cpu halted decision into x86_cpu_reset

[Gleb Natapov]

On Tue, Jul 10, 2012 at 03:15:51PM +0200, Igor Mammedov wrote:
>  /* pc.c */
> -int cpu_is_bsp(CPUX86State *env);
Why remove it instead of modifying it to check BSP bit in apic base?
I think it will make the patch smaller and open code the check does not
look nice.


--
			Gleb.

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

[Igor Mammedov]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 5507 bytes --]

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 Färber <afaerber@suse.de>

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 c7e9ab3..50c1715 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 5521709..f9ed6d8 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1686,6 +1686,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 d3af6ea..b748d90 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1191,7 +1191,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.1