[Qemu-devel] [PATCH qom-next v2 0/6] target-i386: re-factor CPU creation/initialization to QOM

[Qemu-devel] [PATCH qom-next v2 0/6] target-i386: re-factor CPU creation/initialization to QOM

[Igor Mammedov]

Moving code related to CPU creation and initialization internal parts
from board level into apic and cpu objects will allow X86CPU to better
model QOM object life-cycle.
It will allow to create X86CPU as any other object by creating it with
object_new() then setting properties and then calling x86_cpu_realize()
to make it running. Later x86_cpu_realize() should become realize property.


 1  pc: Enable MSI support at APIC level
     http://thread.gmane.org/gmane.comp.emulators.kvm.devel/91186
     included here to prevent confilict since qom-next doesn't have it
     but patches 4, 5 are depending on it. I guess it would be best way to
     avoid confilicts, correct me if I'm wrong.

 2  target-i386: move cpu halted decision into x86_cpu_reset
 3  target-i386: add cpu-model property to x86_cpu
 4  pc: move apic_mapped initialization into common apic init code
 5  target-i386: make initialize CPU in QOM way
 6  target-i386: move reset callback to cpu.c


git tree for testing: 
  https://github.com/imammedo/qemu/tree/x86-cpu-realize-v2

Compile & Run tested:
  target-i386: tcg and kvm mode
  i386-linux-user: running of /bin/ls and /usr/bin/make on qemu tree

[Qemu-devel] [PATCH qom-next 1/6] pc: Enable MSI support at APIC level

[Igor Mammedov]

From: Jan Kiszka <jan.kiszka@siemens.com>

Push msi_supported enabling to the APIC implementations where we can
encapsulate the decision more cleanly, hiding the details from the
generic code.

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
---
 hw/apic.c     |    3 +++
 hw/pc.c       |    9 ---------
 hw/xen.h      |   10 ----------
 hw/xen_apic.c |    5 +++++
 4 files changed, 8 insertions(+), 19 deletions(-)

diff --git a/hw/apic.c b/hw/apic.c
index 4eeaf88..5fbf01c 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -19,6 +19,7 @@
 #include "apic_internal.h"
 #include "apic.h"
 #include "ioapic.h"
+#include "msi.h"
 #include "host-utils.h"
 #include "trace.h"
 #include "pc.h"
@@ -862,6 +863,8 @@ static void apic_init(APICCommonState *s)
 
     s->timer = qemu_new_timer_ns(vm_clock, apic_timer, s);
     local_apics[s->idx] = s;
+
+    msi_supported = true;
 }
 
 static void apic_class_init(ObjectClass *klass, void *data)
diff --git a/hw/pc.c b/hw/pc.c
index 4167782..ad27f36 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -911,15 +911,6 @@ static DeviceState *apic_init(void *env, uint8_t apic_id)
         apic_mapped = 1;
     }
 
-    /* KVM does not support MSI yet. */
-    if (!kvm_irqchip_in_kernel()) {
-        msi_supported = true;
-    }
-
-    if (xen_msi_support()) {
-        msi_supported = true;
-    }
-
     return dev;
 }
 
diff --git a/hw/xen.h b/hw/xen.h
index 3ae4cd0..e5926b7 100644
--- a/hw/xen.h
+++ b/hw/xen.h
@@ -57,14 +57,4 @@ void xen_register_framebuffer(struct MemoryRegion *mr);
 #  define HVM_MAX_VCPUS 32
 #endif
 
-static inline int xen_msi_support(void)
-{
-#if defined(CONFIG_XEN_CTRL_INTERFACE_VERSION) \
-    && CONFIG_XEN_CTRL_INTERFACE_VERSION >= 420
-    return xen_enabled();
-#else
-    return 0;
-#endif
-}
-
 #endif /* QEMU_HW_XEN_H */
diff --git a/hw/xen_apic.c b/hw/xen_apic.c
index 1725ff6..a9e101f 100644
--- a/hw/xen_apic.c
+++ b/hw/xen_apic.c
@@ -40,6 +40,11 @@ static void xen_apic_init(APICCommonState *s)
 {
     memory_region_init_io(&s->io_memory, &xen_apic_io_ops, s, "xen-apic-msi",
                           MSI_SPACE_SIZE);
+
+#if defined(CONFIG_XEN_CTRL_INTERFACE_VERSION) \
+    && CONFIG_XEN_CTRL_INTERFACE_VERSION >= 420
+    msi_supported = true;
+#endif
 }
 
 static void xen_apic_set_base(APICCommonState *s, uint64_t val)
-- 
1.7.7.6

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

[Igor Mammedov]

From: Igor Mammedov <niallain@gmail.com>

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>

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/apic.h            |    2 +-
 hw/apic_common.c     |   18 ++++++++++++------
 hw/pc.c              |    9 ---------
 target-i386/cpu.c    |    9 +++++++++
 target-i386/helper.c |    1 -
 target-i386/kvm.c    |    5 +++--
 6 files changed, 25 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..23d51e8 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,28 @@ void apic_init_reset(DeviceState *d)
     s->timer_expiry = -1;
 }
 
+void apic_designate_bsp(DeviceState *d)
+{
+    if (d) {
+        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 ad27f36..c609770 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -870,12 +870,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) {
@@ -926,10 +920,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 89b4ac7..14c0f64 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1704,6 +1704,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 2cc8097..3ceefad 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.7.6

[Qemu-devel] [PATCH qom-next 3/6] target-i386: add cpu-model property to x86_cpu

[Igor Mammedov]

it's probably intermidiate step till cpu modeled as
sub-classes. After then we probably could drop it.

However it still could be used for overiding default
cpu subclasses definition, and probably renamed to
something like 'features'.

v2:
 - remove accidential tcg_* init code move

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 cpu-defs.h           |    2 +-
 hw/pc.c              |   10 ----------
 target-i386/cpu.c    |   24 ++++++++++++++++++++++++
 target-i386/helper.c |   16 ++++++++++++----
 4 files changed, 37 insertions(+), 15 deletions(-)

diff --git a/cpu-defs.h b/cpu-defs.h
index f49e950..8f4623c 100644
--- a/cpu-defs.h
+++ b/cpu-defs.h
@@ -221,7 +221,7 @@ typedef struct CPUWatchpoint {
     struct QemuCond *halt_cond;                                         \
     int thread_kicked;                                                  \
     struct qemu_work_item *queued_work_first, *queued_work_last;        \
-    const char *cpu_model_str;                                          \
+    char *cpu_model_str;                                                \
     struct KVMState *kvm_state;                                         \
     struct kvm_run *kvm_run;                                            \
     int kvm_fd;                                                         \
diff --git a/hw/pc.c b/hw/pc.c
index c609770..1aa90a2 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -930,7 +930,6 @@ static X86CPU *pc_new_cpu(const char *cpu_model)
 
     cpu = cpu_x86_init(cpu_model);
     if (cpu == NULL) {
-        fprintf(stderr, "Unable to find x86 CPU definition\n");
         exit(1);
     }
     env = &cpu->env;
@@ -946,15 +945,6 @@ void pc_cpus_init(const char *cpu_model)
 {
     int i;
 
-    /* init CPUs */
-    if (cpu_model == NULL) {
-#ifdef TARGET_X86_64
-        cpu_model = "qemu64";
-#else
-        cpu_model = "qemu32";
-#endif
-    }
-
     for(i = 0; i < smp_cpus; i++) {
         pc_new_cpu(cpu_model);
     }
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 14c0f64..e655129 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1731,6 +1731,27 @@ static void mce_init(X86CPU *cpu)
     }
 }
 
+static char *x86_get_cpu_model(Object *obj, Error **errp)
+{
+    X86CPU *cpu = X86_CPU(obj);
+    CPUX86State *env = &cpu->env;
+    return g_strdup(env->cpu_model_str);
+}
+
+static void x86_set_cpu_model(Object *obj, const char *value, Error **errp)
+{
+    X86CPU *cpu = X86_CPU(obj);
+    CPUX86State *env = &cpu->env;
+
+    g_free((gpointer)env->cpu_model_str);
+    env->cpu_model_str = g_strdup(value);
+
+    if (cpu_x86_register(cpu, env->cpu_model_str) < 0) {
+        fprintf(stderr, "Unable to find x86 CPU definition\n");
+        error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
+    }
+}
+
 void x86_cpu_realize(Object *obj, Error **errp)
 {
     X86CPU *cpu = X86_CPU(obj);
@@ -1771,6 +1792,9 @@ static void x86_cpu_initfn(Object *obj)
                         x86_cpuid_get_tsc_freq,
                         x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
 
+    object_property_add_str(obj, "cpu-model",
+        x86_get_cpu_model, x86_set_cpu_model, NULL);
+
     env->cpuid_apic_id = env->cpu_index;
 }
 
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 3ceefad..fbaeeea 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1154,12 +1154,10 @@ int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector,
 X86CPU *cpu_x86_init(const char *cpu_model)
 {
     X86CPU *cpu;
-    CPUX86State *env;
+    Error *errp = NULL;
     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) {
@@ -1170,7 +1168,17 @@ X86CPU *cpu_x86_init(const char *cpu_model)
             cpu_set_debug_excp_handler(breakpoint_handler);
 #endif
     }
-    if (cpu_x86_register(cpu, cpu_model) < 0) {
+
+    if (cpu_model) {
+        object_property_set_str(OBJECT(cpu), cpu_model, "cpu-model", &errp);
+    } else {
+#ifdef TARGET_X86_64
+        object_property_set_str(OBJECT(cpu), "qemu64", "cpu-model", &errp);
+#else
+        object_property_set_str(OBJECT(cpu), "qemu32", "cpu-model", &errp);
+#endif
+    }
+    if (errp) {
         object_delete(OBJECT(cpu));
         return NULL;
     }
-- 
1.7.7.6

[Qemu-devel] [PATCH qom-next 4/6] pc: move apic_mapped initialization into common apic init code

[Igor Mammedov]

Move from apic_init in pc.c, the code that belongs to apic_init_common.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/apic_common.c |   11 +++++++++++
 hw/msi.h         |    2 ++
 hw/pc.c          |   12 ------------
 3 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/hw/apic_common.c b/hw/apic_common.c
index 23d51e8..c3fa66b 100644
--- a/hw/apic_common.c
+++ b/hw/apic_common.c
@@ -21,6 +21,7 @@
 #include "apic_internal.h"
 #include "trace.h"
 #include "kvm.h"
+#include "msi.h"
 
 static int apic_irq_delivered;
 bool apic_report_tpr_access;
@@ -284,6 +285,7 @@ static int apic_init_common(SysBusDevice *dev)
     APICCommonClass *info;
     static DeviceState *vapic;
     static int apic_no;
+    static int apic_mapped;
 
     if (apic_no >= MAX_APICS) {
         return -1;
@@ -295,6 +297,15 @@ static int apic_init_common(SysBusDevice *dev)
 
     sysbus_init_mmio(dev, &s->io_memory);
 
+    /* XXX: mapping more APICs at the same memory location */
+    if (apic_mapped == 0) {
+        /* NOTE: the APIC is directly connected to the CPU - it is not
+           on the global memory bus. */
+        /* XXX: what if the base changes? */
+        sysbus_mmio_map(sysbus_from_qdev(&s->busdev.qdev), 0, MSI_ADDR_BASE);
+        apic_mapped = 1;
+    }
+
     if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK) {
         vapic = sysbus_create_simple("kvmvapic", -1, NULL);
     }
diff --git a/hw/msi.h b/hw/msi.h
index 3040bb0..abd52b6 100644
--- a/hw/msi.h
+++ b/hw/msi.h
@@ -40,4 +40,6 @@ static inline bool msi_present(const PCIDevice *dev)
     return dev->cap_present & QEMU_PCI_CAP_MSI;
 }
 
+#define MSI_ADDR_BASE 0xfee00000
+
 #endif /* QEMU_MSI_H */
diff --git a/hw/pc.c b/hw/pc.c
index 1aa90a2..1ccfc6e 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -70,8 +70,6 @@
 #define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
 #define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
 
-#define MSI_ADDR_BASE 0xfee00000
-
 #define E820_NR_ENTRIES		16
 
 struct e820_entry {
@@ -882,7 +880,6 @@ DeviceState *cpu_get_current_apic(void)
 static DeviceState *apic_init(void *env, uint8_t apic_id)
 {
     DeviceState *dev;
-    static int apic_mapped;
 
     if (kvm_irqchip_in_kernel()) {
         dev = qdev_create(NULL, "kvm-apic");
@@ -896,15 +893,6 @@ static DeviceState *apic_init(void *env, uint8_t apic_id)
     qdev_prop_set_ptr(dev, "cpu_env", env);
     qdev_init_nofail(dev);
 
-    /* XXX: mapping more APICs at the same memory location */
-    if (apic_mapped == 0) {
-        /* NOTE: the APIC is directly connected to the CPU - it is not
-           on the global memory bus. */
-        /* XXX: what if the base changes? */
-        sysbus_mmio_map(sysbus_from_qdev(dev), 0, MSI_ADDR_BASE);
-        apic_mapped = 1;
-    }
-
     return dev;
 }
 
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH qom-next 4/6] pc: move apic_mapped initialization into common apic init code

[Peter Maydell]

On 23 May 2012 17:39, Igor Mammedov <imammedo@redhat.com> wrote:
> @@ -295,6 +297,15 @@ static int apic_init_common(SysBusDevice *dev)
>
>     sysbus_init_mmio(dev, &s->io_memory);
>
> +    /* XXX: mapping more APICs at the same memory location */
> +    if (apic_mapped == 0) {
> +        /* NOTE: the APIC is directly connected to the CPU - it is not
> +           on the global memory bus. */
> +        /* XXX: what if the base changes? */
> +        sysbus_mmio_map(sysbus_from_qdev(&s->busdev.qdev), 0, MSI_ADDR_BASE);
> +        apic_mapped = 1;
> +    }
> +
>     if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK) {
>         vapic = sysbus_create_simple("kvmvapic", -1, NULL);
>     }

This looks wrong -- sysbus device init functions shouldn't
be mapping MMIO regions themselves, in general. They should
expose MMIO regions to be mapped by whichever device or board
model creates them. Which is what the code before this patch
was doing -- why do you want to move this code?

-- PMM

Re: [Qemu-devel] [PATCH qom-next 4/6] pc: move apic_mapped initialization into common apic init code

[Jan Kiszka]

On 2012-05-23 13:44, Peter Maydell wrote:
> On 23 May 2012 17:39, Igor Mammedov <imammedo@redhat.com> wrote:
>> @@ -295,6 +297,15 @@ static int apic_init_common(SysBusDevice *dev)
>>
>>     sysbus_init_mmio(dev, &s->io_memory);
>>
>> +    /* XXX: mapping more APICs at the same memory location */
>> +    if (apic_mapped == 0) {
>> +        /* NOTE: the APIC is directly connected to the CPU - it is not
>> +           on the global memory bus. */
>> +        /* XXX: what if the base changes? */
>> +        sysbus_mmio_map(sysbus_from_qdev(&s->busdev.qdev), 0, MSI_ADDR_BASE);
>> +        apic_mapped = 1;
>> +    }
>> +
>>     if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK) {
>>         vapic = sysbus_create_simple("kvmvapic", -1, NULL);
>>     }
> 
> This looks wrong -- sysbus device init functions shouldn't
> be mapping MMIO regions themselves, in general. They should
> expose MMIO regions to be mapped by whichever device or board
> model creates them. Which is what the code before this patch
> was doing -- why do you want to move this code?

I fact, the CPU normally decides about where the APIC is mapped,
specifically when it is remapped via the MSR (which QEMU cannot support
yet). Well, unless we are talking about an external APIC and a 486 CPU.
Then the board decides. But I guess we could live with ignoring that
corner case and stick the mapping setup into the CPU initialization code.

Jan

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

Re: [Qemu-devel] [PATCH qom-next 4/6] pc: move apic_mapped initialization into common apic init code

[Igor Mammedov]

----- Original Message -----
> From: "Peter Maydell" <peter.maydell@linaro.org>
> To: "Igor Mammedov" <imammedo@redhat.com>
> Cc: aliguori@us.ibm.com, "wei liu2" <wei.liu2@citrix.com>, ehabkost@redhat.com, "stefano stabellini"
> <stefano.stabellini@eu.citrix.com>, sw@weilnetz.de, mtosatti@redhat.com, qemu-devel@nongnu.org, agraf@suse.de,
> blauwirbel@gmail.com, avi@redhat.com, "jan kiszka" <jan.kiszka@siemens.com>, "anthony perard"
> <anthony.perard@citrix.com>, pbonzini@redhat.com, afaerber@suse.de
> Sent: Wednesday, May 23, 2012 6:44:30 PM
> Subject: Re: [Qemu-devel] [PATCH qom-next 4/6] pc: move apic_mapped initialization into common apic init code
>
> On 23 May 2012 17:39, Igor Mammedov <imammedo@redhat.com> wrote:
> > @@ -295,6 +297,15 @@ static int apic_init_common(SysBusDevice *dev)
> >
> >     sysbus_init_mmio(dev, &s->io_memory);
> >
> > +    /* XXX: mapping more APICs at the same memory location */
> > +    if (apic_mapped == 0) {
> > +        /* NOTE: the APIC is directly connected to the CPU - it is
> > not
> > +           on the global memory bus. */
> > +        /* XXX: what if the base changes? */
> > +        sysbus_mmio_map(sysbus_from_qdev(&s->busdev.qdev), 0,
> > MSI_ADDR_BASE);
> > +        apic_mapped = 1;
> > +    }
> > +
> >     if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK) {
> >         vapic = sysbus_create_simple("kvmvapic", -1, NULL);
> >     }
>
> This looks wrong -- sysbus device init functions shouldn't
> be mapping MMIO regions themselves, in general. They should
> expose MMIO regions to be mapped by whichever device or board
> model creates them. Which is what the code before this patch
> was doing -- why do you want to move this code?

Why:
For cpu-hotplug it was suggested to use device_add/del
interface for it. To do so in a generalized way hot-plugged cpu
should follow general QOM object creation sequence, i.e.
  - create new cpu instance
  - set properties
  - realize instance
without creating precedent of special case for cpus in device_add/del
if possible. So goal is to have a self-sufficient cpu object that
doesn't require external hooks to create/initialize it. It looks
possible do so for target-i386 at least.

Some thoughts why mapping should be inside of apic object initfn:

LAPIC is a part of cpu so it's created and initialized by cpu. (not
true for 486 but for later cpus should be)
I've looked in Intel SDM and chapter "10.4.4 Local APIC Status and Location"
says:

"APIC Base field, bits 12 through 35 ⎯ Specifies the base address of the APIC
registers. ---cut--- Following a power-up or reset, the field is set to FEE0 0000H."

it suggests that apic base is mapped right after cpu power-on and power-on could be
roughly mapped to object_new(),set_prop,realize sequence for cpu. So when cpu
creates child object "apic", then mapping of default apic base inside apic/cpu
objects looks more appropriate. (depending on cpu model, but we could chose a more
convenient case to implement).

Thanks,
  Igor

Re: [Qemu-devel] [PATCH qom-next 4/6] pc: move apic_mapped initialization into common apic init code

[Peter Maydell]

On 23 May 2012 22:09, Igor Mammedov <imammedo@redhat.com> wrote:
> For cpu-hotplug it was suggested to use device_add/del
> interface for it. To do so in a generalized way hot-plugged cpu
> should follow general QOM object creation sequence, i.e.
>  - create new cpu instance
>  - set properties
>  - realize instance
> without creating precedent of special case for cpus in device_add/del
> if possible. So goal is to have a self-sufficient cpu object that
> doesn't require external hooks to create/initialize it. It looks
> possible do so for target-i386 at least.

I think your self-sufficient CPU object should probably be a
container QOM object which contains the CPU core itself and
the APIC device. Then the container object's initialisation
can map the APIC device.

-- PMM

Re: [Qemu-devel] [PATCH qom-next 4/6] pc: move apic_mapped initialization into common apic init code

[Igor Mammedov]

On 05/23/2012 11:26 PM, Peter Maydell wrote:
> On 23 May 2012 22:09, Igor Mammedov<imammedo@redhat.com>  wrote:
>> For cpu-hotplug it was suggested to use device_add/del
>> interface for it. To do so in a generalized way hot-plugged cpu
>> should follow general QOM object creation sequence, i.e.
>>   - create new cpu instance
>>   - set properties
>>   - realize instance
>> without creating precedent of special case for cpus in device_add/del
>> if possible. So goal is to have a self-sufficient cpu object that
>> doesn't require external hooks to create/initialize it. It looks
>> possible do so for target-i386 at least.
>
> I think your self-sufficient CPU object should probably be a
> container QOM object which contains the CPU core itself and
> the APIC device. Then the container object's initialisation
> can map the APIC device.

For x86 it would be artificial thing without a real hardware to
model after, that would needlessly complicate code and interface.
I'd rather avoid this.

Meanwhile I've found only two devices that do mapping inside
device's initfn :(

hw/lm32_sys.c
hw/bonito.c

and lm32_sys has comment:
     /* Note: This device is not created in the board initialization,
      * instead it has to be added with the -device parameter. Therefore,
      * the device maps itself. */

So for some devices it's possible to do so.
And since APIC after power-on/reset should have it's regs mapped at initial
APIC base, APIC's initfn looks like a legitimate place to do it.

However if you are strongly opposed to this idea, I'll try Jan's idea to put
mapping into CPU code that creates APIC instance. It would be uglier than
if mapping were in APIC itself, but at least it could be justified by fact
that Integrated APIC is a part of cpu and manuals don't specify which of them
is responsible for setting mapping (mapping just exists after cpu power-on/reset).

-- 
-----
Thanks,
   Igor

Re: [Qemu-devel] [PATCH qom-next 4/6] pc: move apic_mapped initialization into common apic init code

[Peter Maydell]

On 24 May 2012 14:10, Igor Mammedov <imammedo@redhat.com> wrote:
> On 05/23/2012 11:26 PM, Peter Maydell wrote:
>> I think your self-sufficient CPU object should probably be a
>> container QOM object which contains the CPU core itself and
>> the APIC device. Then the container object's initialisation
>> can map the APIC device.
>
> For x86 it would be artificial thing without a real hardware to
> model after, that would needlessly complicate code and interface.

I think the real hardware looks exactly like a container which
has both the cpu-core proper and the APIC on it...

-- PMM

Re: [Qemu-devel] [PATCH qom-next 4/6] pc: move apic_mapped initialization into common apic init code

[Jan Kiszka]

On 2012-05-24 10:10, Igor Mammedov wrote:
> On 05/23/2012 11:26 PM, Peter Maydell wrote:
>> On 23 May 2012 22:09, Igor Mammedov<imammedo@redhat.com>  wrote:
>>> For cpu-hotplug it was suggested to use device_add/del
>>> interface for it. To do so in a generalized way hot-plugged cpu
>>> should follow general QOM object creation sequence, i.e.
>>>   - create new cpu instance
>>>   - set properties
>>>   - realize instance
>>> without creating precedent of special case for cpus in device_add/del
>>> if possible. So goal is to have a self-sufficient cpu object that
>>> doesn't require external hooks to create/initialize it. It looks
>>> possible do so for target-i386 at least.
>>
>> I think your self-sufficient CPU object should probably be a
>> container QOM object which contains the CPU core itself and
>> the APIC device. Then the container object's initialisation
>> can map the APIC device.
> 
> For x86 it would be artificial thing without a real hardware to
> model after, that would needlessly complicate code and interface.
> I'd rather avoid this.

No, letting the CPU map the APIC is really the proper way to deal with
it, specifically once we will support remapping.

Jan

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

Re: [Qemu-devel] [PATCH qom-next 4/6] pc: move apic_mapped initialization into common apic init code

[Igor Mammedov]

On 05/24/2012 03:25 PM, Jan Kiszka wrote:
> On 2012-05-24 10:10, Igor Mammedov wrote:
>> On 05/23/2012 11:26 PM, Peter Maydell wrote:
>>> On 23 May 2012 22:09, Igor Mammedov<imammedo@redhat.com>   wrote:
>>>> For cpu-hotplug it was suggested to use device_add/del
>>>> interface for it. To do so in a generalized way hot-plugged cpu
>>>> should follow general QOM object creation sequence, i.e.
>>>>    - create new cpu instance
>>>>    - set properties
>>>>    - realize instance
>>>> without creating precedent of special case for cpus in device_add/del
>>>> if possible. So goal is to have a self-sufficient cpu object that
>>>> doesn't require external hooks to create/initialize it. It looks
>>>> possible do so for target-i386 at least.
>>>
>>> I think your self-sufficient CPU object should probably be a
>>> container QOM object which contains the CPU core itself and
>>> the APIC device. Then the container object's initialisation
>>> can map the APIC device.
>>
>> For x86 it would be artificial thing without a real hardware to
>> model after, that would needlessly complicate code and interface.
>> I'd rather avoid this.
>
> No, letting the CPU map the APIC is really the proper way to deal with
> it, specifically once we will support remapping.

Ok, I'll move mapping at cpu level.

-- 
-----
Thanks,
   Igor

Re: [Qemu-devel] [PATCH qom-next 4/6] pc: move apic_mapped initialization into common apic init code

[Andreas Färber]

Am 24.05.2012 15:10, schrieb Igor Mammedov:
> On 05/23/2012 11:26 PM, Peter Maydell wrote:
>> On 23 May 2012 22:09, Igor Mammedov<imammedo@redhat.com>  wrote:
>>> For cpu-hotplug it was suggested to use device_add/del
>>> interface for it. To do so in a generalized way hot-plugged cpu
>>> should follow general QOM object creation sequence, i.e.
>>>   - create new cpu instance
>>>   - set properties
>>>   - realize instance
>>> without creating precedent of special case for cpus in device_add/del
>>> if possible. So goal is to have a self-sufficient cpu object that
>>> doesn't require external hooks to create/initialize it. It looks
>>> possible do so for target-i386 at least.
>>
>> I think your self-sufficient CPU object should probably be a
>> container QOM object which contains the CPU core itself and
>> the APIC device. Then the container object's initialisation
>> can map the APIC device.
> 
> For x86 it would be artificial thing without a real hardware to
> model after, that would needlessly complicate code and interface.
> I'd rather avoid this.
> 
> Meanwhile I've found only two devices that do mapping inside
> device's initfn :(
> 
> hw/lm32_sys.c
> hw/bonito.c
> 
> and lm32_sys has comment:
>     /* Note: This device is not created in the board initialization,
>      * instead it has to be added with the -device parameter. Therefore,
>      * the device maps itself. */
> 
> So for some devices it's possible to do so.

Simply put: Doing the mmio_map after qdev init is historical and no
reason to keep doing so. It breaks the desired late-realize model.

I believe Anthony's controversial series on GitHub with the
"boilerplate" accessors that Peter so disliked tried to fix this?

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 qom-next 5/6] target-i386: make initialize CPU in QOM way

[Igor Mammedov]

Make CPU creation/initialization consistent with QOM object
behavior in this, by moving tcg and apic initialization from board
level into CPU's initfn/realize calls and cpu_model property setter.

Which makes CPU object self-sufficient in respect of creation/initialization
and matches a typical object creation sequence, i.e.:
  - create CPU instance
  - set properties
  - realize object - (x86_cpu_realize will be converted into realize
      property setter, when it is implemented)

v2:
  - fix moving of tcg_* initialization into cpu.c from helper.c
      spotted-by: <Jan Kiszka jan.kiszka@siemens.com>
  - make it compile/work on i386-linux-user target

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/pc.c              |   45 ++++------------------------
 target-i386/cpu.c    |   81 ++++++++++++++++++++++++++++++++++++++++++++++++-
 target-i386/helper.c |   39 ------------------------
 3 files changed, 85 insertions(+), 80 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 1ccfc6e..d7845ea 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -42,7 +42,6 @@
 #include "sysbus.h"
 #include "sysemu.h"
 #include "kvm.h"
-#include "xen.h"
 #include "blockdev.h"
 #include "ui/qemu-spice.h"
 #include "memory.h"
@@ -877,25 +876,6 @@ DeviceState *cpu_get_current_apic(void)
     }
 }
 
-static DeviceState *apic_init(void *env, uint8_t apic_id)
-{
-    DeviceState *dev;
-
-    if (kvm_irqchip_in_kernel()) {
-        dev = qdev_create(NULL, "kvm-apic");
-    } else if (xen_enabled()) {
-        dev = qdev_create(NULL, "xen-apic");
-    } else {
-        dev = qdev_create(NULL, "apic");
-    }
-
-    qdev_prop_set_uint8(dev, "id", apic_id);
-    qdev_prop_set_ptr(dev, "cpu_env", env);
-    qdev_init_nofail(dev);
-
-    return dev;
-}
-
 void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
 {
     CPUX86State *s = opaque;
@@ -911,30 +891,17 @@ static void pc_cpu_reset(void *opaque)
     cpu_reset(CPU(cpu));
 }
 
-static X86CPU *pc_new_cpu(const char *cpu_model)
-{
-    X86CPU *cpu;
-    CPUX86State *env;
-
-    cpu = cpu_x86_init(cpu_model);
-    if (cpu == NULL) {
-        exit(1);
-    }
-    env = &cpu->env;
-    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);
-    return cpu;
-}
-
 void pc_cpus_init(const char *cpu_model)
 {
+    X86CPU *cpu;
     int i;
 
     for(i = 0; i < smp_cpus; i++) {
-        pc_new_cpu(cpu_model);
+        cpu = cpu_x86_init(cpu_model);
+        if (cpu == NULL) {
+            exit(1);
+        }
+        qemu_register_reset(pc_cpu_reset, cpu);
     }
 }
 
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e655129..99ef891 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -23,6 +23,7 @@
 
 #include "cpu.h"
 #include "kvm.h"
+#include "hw/xen.h"
 
 #include "qemu-option.h"
 #include "qemu-config.h"
@@ -31,6 +32,9 @@
 
 #include "hyperv.h"
 
+#include "hw/qdev.h"
+#include "sysemu.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.
@@ -1749,24 +1753,89 @@ static void x86_set_cpu_model(Object *obj, const char *value, Error **errp)
     if (cpu_x86_register(cpu, env->cpu_model_str) < 0) {
         fprintf(stderr, "Unable to find x86 CPU definition\n");
         error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
+        return;
+    }
+
+#ifndef CONFIG_USER_ONLY
+    if (((env->cpuid_features & CPUID_APIC) || smp_cpus > 1)) {
+        if (kvm_irqchip_in_kernel()) {
+            env->apic_state = qdev_create(NULL, "kvm-apic");
+        } else if (xen_enabled()) {
+            env->apic_state = qdev_create(NULL, "xen-apic");
+        } else {
+            env->apic_state = qdev_create(NULL, "apic");
+        }
+        object_property_add_child(OBJECT(cpu), "apic",
+            OBJECT(env->apic_state), NULL);
+
+        qdev_prop_set_uint8(env->apic_state, "id", env->cpuid_apic_id);
+        qdev_prop_set_ptr(env->apic_state, "cpu_env", env);
+    }
+#endif
+}
+
+#ifndef CONFIG_USER_ONLY
+static CPUDebugExcpHandler *prev_debug_excp_handler;
+
+static void breakpoint_handler(CPUX86State *env)
+{
+    CPUBreakpoint *bp;
+
+    if (env->watchpoint_hit) {
+        if (env->watchpoint_hit->flags & BP_CPU) {
+            env->watchpoint_hit = NULL;
+            if (check_hw_breakpoints(env, 0)) {
+                raise_exception_env(EXCP01_DB, env);
+            } else {
+                cpu_resume_from_signal(env, NULL);
+            }
+        }
+    } else {
+        QTAILQ_FOREACH(bp, &env->breakpoints, entry)
+            if (bp->pc == env->eip) {
+                if (bp->flags & BP_CPU) {
+                    check_hw_breakpoints(env, 1);
+                    raise_exception_env(EXCP01_DB, env);
+                }
+                break;
+            }
+    }
+    if (prev_debug_excp_handler) {
+        prev_debug_excp_handler(env);
     }
 }
+#endif
 
 void x86_cpu_realize(Object *obj, Error **errp)
 {
     X86CPU *cpu = X86_CPU(obj);
+#ifndef CONFIG_USER_ONLY
+    CPUX86State *env = &cpu->env;
+
+    if (env->apic_state) {
+        if (qdev_init(env->apic_state) < 0) {
+            error_set(errp, QERR_DEVICE_INIT_FAILED,
+                object_get_typename(OBJECT(env->apic_state)));
+            return;
+        }
+    }
+#endif
 
     mce_init(cpu);
-    qemu_init_vcpu(&cpu->env);
+    qemu_init_vcpu(env);
+    cpu_reset(CPU(cpu));
 }
 
 static void x86_cpu_initfn(Object *obj)
 {
     X86CPU *cpu = X86_CPU(obj);
     CPUX86State *env = &cpu->env;
+    static int inited;
 
     cpu_exec_init(env);
 
+    env->cpuid_apic_id = env->cpu_index;
+
     object_property_add(obj, "family", "int",
                         x86_cpuid_version_get_family,
                         x86_cpuid_version_set_family, NULL, NULL, NULL);
@@ -1795,7 +1864,15 @@ static void x86_cpu_initfn(Object *obj)
     object_property_add_str(obj, "cpu-model",
         x86_get_cpu_model, x86_set_cpu_model, 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
+        prev_debug_excp_handler =
+            cpu_set_debug_excp_handler(breakpoint_handler);
+#endif
+    }
 }
 
 static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
diff --git a/target-i386/helper.c b/target-i386/helper.c
index fbaeeea..38ac25d 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -941,34 +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;
-
-    if (env->watchpoint_hit) {
-        if (env->watchpoint_hit->flags & BP_CPU) {
-            env->watchpoint_hit = NULL;
-            if (check_hw_breakpoints(env, 0))
-                raise_exception_env(EXCP01_DB, env);
-            else
-                cpu_resume_from_signal(env, NULL);
-        }
-    } else {
-        QTAILQ_FOREACH(bp, &env->breakpoints, entry)
-            if (bp->pc == env->eip) {
-                if (bp->flags & BP_CPU) {
-                    check_hw_breakpoints(env, 1);
-                    raise_exception_env(EXCP01_DB, env);
-                }
-                break;
-            }
-    }
-    if (prev_debug_excp_handler)
-        prev_debug_excp_handler(env);
-}
-
 typedef struct MCEInjectionParams {
     Monitor *mon;
     CPUX86State *env;
@@ -1155,20 +1127,9 @@ X86CPU *cpu_x86_init(const char *cpu_model)
 {
     X86CPU *cpu;
     Error *errp = NULL;
-    static int inited;
 
     cpu = X86_CPU(object_new(TYPE_X86_CPU));
 
-    /* init various static tables used in TCG mode */
-    if (tcg_enabled() && !inited) {
-        inited = 1;
-        optimize_flags_init();
-#ifndef CONFIG_USER_ONLY
-        prev_debug_excp_handler =
-            cpu_set_debug_excp_handler(breakpoint_handler);
-#endif
-    }
-
     if (cpu_model) {
         object_property_set_str(OBJECT(cpu), cpu_model, "cpu-model", &errp);
     } else {
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH qom-next 5/6] target-i386: make initialize CPU in QOM way

[Andreas Färber]

Am 23.05.2012 18:39, schrieb Igor Mammedov:
> Make CPU creation/initialization consistent with QOM object
> behavior in this, by moving tcg and apic initialization from board
> level into CPU's initfn/realize calls and cpu_model property setter.
> 
> Which makes CPU object self-sufficient in respect of creation/initialization
> and matches a typical object creation sequence, i.e.:
>   - create CPU instance
>   - set properties
>   - realize object - (x86_cpu_realize will be converted into realize
>       property setter, when it is implemented)
> 
> v2:
>   - fix moving of tcg_* initialization into cpu.c from helper.c
>       spotted-by: <Jan Kiszka jan.kiszka@siemens.com>
>   - make it compile/work on i386-linux-user target
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/pc.c              |   45 ++++------------------------
>  target-i386/cpu.c    |   81 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  target-i386/helper.c |   39 ------------------------
>  3 files changed, 85 insertions(+), 80 deletions(-)
> 
> diff --git a/hw/pc.c b/hw/pc.c
> index 1ccfc6e..d7845ea 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c

> @@ -911,30 +891,17 @@ static void pc_cpu_reset(void *opaque)
>      cpu_reset(CPU(cpu));
>  }
>  
> -static X86CPU *pc_new_cpu(const char *cpu_model)
> -{
> -    X86CPU *cpu;
> -    CPUX86State *env;
> -
> -    cpu = cpu_x86_init(cpu_model);
> -    if (cpu == NULL) {
> -        exit(1);
> -    }
> -    env = &cpu->env;
> -    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);
> -    return cpu;
> -}
> -
>  void pc_cpus_init(const char *cpu_model)
>  {
> +    X86CPU *cpu;
>      int i;
>  
>      for(i = 0; i < smp_cpus; i++) {

If we do changes here, please add the missing space. :)

> -        pc_new_cpu(cpu_model);
> +        cpu = cpu_x86_init(cpu_model);
> +        if (cpu == NULL) {
> +            exit(1);
> +        }
> +        qemu_register_reset(pc_cpu_reset, cpu);
>      }
>  }
>  
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index e655129..99ef891 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c

> @@ -1749,24 +1753,89 @@ static void x86_set_cpu_model(Object *obj, const char *value, Error **errp)
>      if (cpu_x86_register(cpu, env->cpu_model_str) < 0) {
>          fprintf(stderr, "Unable to find x86 CPU definition\n");
>          error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
> +        return;
> +    }
> +
> +#ifndef CONFIG_USER_ONLY
> +    if (((env->cpuid_features & CPUID_APIC) || smp_cpus > 1)) {
> +        if (kvm_irqchip_in_kernel()) {
> +            env->apic_state = qdev_create(NULL, "kvm-apic");
> +        } else if (xen_enabled()) {
> +            env->apic_state = qdev_create(NULL, "xen-apic");
> +        } else {
> +            env->apic_state = qdev_create(NULL, "apic");
> +        }
> +        object_property_add_child(OBJECT(cpu), "apic",
> +            OBJECT(env->apic_state), NULL);
> +
> +        qdev_prop_set_uint8(env->apic_state, "id", env->cpuid_apic_id);
> +        qdev_prop_set_ptr(env->apic_state, "cpu_env", env);

I'd like to avoid re-adding this set_ptr(). We can cherry-pick my
link<X86CPU> property from QOM CPUState part 4 series.

> +    }
> +#endif
> +}
> +
> +#ifndef CONFIG_USER_ONLY
> +static CPUDebugExcpHandler *prev_debug_excp_handler;
> +
> +static void breakpoint_handler(CPUX86State *env)
> +{
> +    CPUBreakpoint *bp;
> +
> +    if (env->watchpoint_hit) {
> +        if (env->watchpoint_hit->flags & BP_CPU) {
> +            env->watchpoint_hit = NULL;
> +            if (check_hw_breakpoints(env, 0)) {
> +                raise_exception_env(EXCP01_DB, env);
> +            } else {
> +                cpu_resume_from_signal(env, NULL);
> +            }
> +        }
> +    } else {
> +        QTAILQ_FOREACH(bp, &env->breakpoints, entry)
> +            if (bp->pc == env->eip) {
> +                if (bp->flags & BP_CPU) {
> +                    check_hw_breakpoints(env, 1);
> +                    raise_exception_env(EXCP01_DB, env);
> +                }
> +                break;
> +            }
> +    }
> +    if (prev_debug_excp_handler) {
> +        prev_debug_excp_handler(env);
>      }
>  }
> +#endif
>  
>  void x86_cpu_realize(Object *obj, Error **errp)
>  {
>      X86CPU *cpu = X86_CPU(obj);
> +#ifndef CONFIG_USER_ONLY
> +    CPUX86State *env = &cpu->env;
> +
> +    if (env->apic_state) {
> +        if (qdev_init(env->apic_state) < 0) {
> +            error_set(errp, QERR_DEVICE_INIT_FAILED,
> +                object_get_typename(OBJECT(env->apic_state)));
> +            return;
> +        }
> +    }
> +#endif
>  
>      mce_init(cpu);
> -    qemu_init_vcpu(&cpu->env);
> +    qemu_init_vcpu(env);

This only works because currently qemu_init_vcpu() is a no-op macro that
doesn't use the parameter. Please don't change it back, I guess it's a
mismerge.

We can avoid the env variable if I finish merging Paolo's series - by
realizing the CPU the APIC as its child would get realized, too. Is the
ordering before mce_init() mandatory here or is it just to reduce the
#ifndef'ery?

> +    cpu_reset(CPU(cpu));
>  }
>  
>  static void x86_cpu_initfn(Object *obj)
>  {
>      X86CPU *cpu = X86_CPU(obj);
>      CPUX86State *env = &cpu->env;
> +    static int inited;
>  
>      cpu_exec_init(env);
>  
> +    env->cpuid_apic_id = env->cpu_index;
> +
>      object_property_add(obj, "family", "int",
>                          x86_cpuid_version_get_family,
>                          x86_cpuid_version_set_family, NULL, NULL, NULL);
> @@ -1795,7 +1864,15 @@ static void x86_cpu_initfn(Object *obj)
>      object_property_add_str(obj, "cpu-model",
>          x86_get_cpu_model, x86_set_cpu_model, 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
> +        prev_debug_excp_handler =
> +            cpu_set_debug_excp_handler(breakpoint_handler);
> +#endif
> +    }

Did you forget to put that into its own patch or did that not work?
My idea was to have it first in the series so that other changes here
and elsewhere can be rebased onto it.

Also I wonder whether it would better be placed into the class_init? I'd
tend towards initfn because that will not be invoked during type
enumeration.

>  }
>  
>  static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index fbaeeea..38ac25d 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -941,34 +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;
> -
> -    if (env->watchpoint_hit) {
> -        if (env->watchpoint_hit->flags & BP_CPU) {
> -            env->watchpoint_hit = NULL;
> -            if (check_hw_breakpoints(env, 0))
> -                raise_exception_env(EXCP01_DB, env);
> -            else
> -                cpu_resume_from_signal(env, NULL);
> -        }
> -    } else {
> -        QTAILQ_FOREACH(bp, &env->breakpoints, entry)
> -            if (bp->pc == env->eip) {
> -                if (bp->flags & BP_CPU) {
> -                    check_hw_breakpoints(env, 1);
> -                    raise_exception_env(EXCP01_DB, env);
> -                }
> -                break;
> -            }
> -    }
> -    if (prev_debug_excp_handler)
> -        prev_debug_excp_handler(env);
> -}
> -

I wonder if that could rather stay here as non-static?

>  typedef struct MCEInjectionParams {
>      Monitor *mon;
>      CPUX86State *env;
> @@ -1155,20 +1127,9 @@ X86CPU *cpu_x86_init(const char *cpu_model)
>  {
>      X86CPU *cpu;
>      Error *errp = NULL;
> -    static int inited;
>  
>      cpu = X86_CPU(object_new(TYPE_X86_CPU));
>  
> -    /* init various static tables used in TCG mode */
> -    if (tcg_enabled() && !inited) {
> -        inited = 1;
> -        optimize_flags_init();
> -#ifndef CONFIG_USER_ONLY
> -        prev_debug_excp_handler =
> -            cpu_set_debug_excp_handler(breakpoint_handler);
> -#endif
> -    }
> -
>      if (cpu_model) {
>          object_property_set_str(OBJECT(cpu), cpu_model, "cpu-model", &errp);
>      } else {

/-F

-- 
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 qom-next 5/6] target-i386: make initialize CPU in QOM way

[Igor Mammedov]

On 05/23/2012 11:27 PM, Andreas Färber wrote:
> Am 23.05.2012 18:39, schrieb Igor Mammedov:
>> Make CPU creation/initialization consistent with QOM object
>> behavior in this, by moving tcg and apic initialization from board
>> level into CPU's initfn/realize calls and cpu_model property setter.
>>
>> Which makes CPU object self-sufficient in respect of creation/initialization
>> and matches a typical object creation sequence, i.e.:
>>    - create CPU instance
>>    - set properties
>>    - realize object - (x86_cpu_realize will be converted into realize
>>        property setter, when it is implemented)
>>
>> v2:
>>    - fix moving of tcg_* initialization into cpu.c from helper.c
>>        spotted-by:<Jan Kiszka jan.kiszka@siemens.com>
>>    - make it compile/work on i386-linux-user target
>>
>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
>> ---
>>   hw/pc.c              |   45 ++++------------------------
>>   target-i386/cpu.c    |   81 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>   target-i386/helper.c |   39 ------------------------
>>   3 files changed, 85 insertions(+), 80 deletions(-)
>>
>> diff --git a/hw/pc.c b/hw/pc.c
>> index 1ccfc6e..d7845ea 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>
>> @@ -911,30 +891,17 @@ static void pc_cpu_reset(void *opaque)
>>       cpu_reset(CPU(cpu));
>>   }
>>
>> -static X86CPU *pc_new_cpu(const char *cpu_model)
>> -{
>> -    X86CPU *cpu;
>> -    CPUX86State *env;
>> -
>> -    cpu = cpu_x86_init(cpu_model);
>> -    if (cpu == NULL) {
>> -        exit(1);
>> -    }
>> -    env =&cpu->env;
>> -    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);
>> -    return cpu;
>> -}
>> -
>>   void pc_cpus_init(const char *cpu_model)
>>   {
>> +    X86CPU *cpu;
>>       int i;
>>
>>       for(i = 0; i<  smp_cpus; i++) {
>
> If we do changes here, please add the missing space. :)
I'll fix it.

>
>> -        pc_new_cpu(cpu_model);
>> +        cpu = cpu_x86_init(cpu_model);
>> +        if (cpu == NULL) {
>> +            exit(1);
>> +        }
>> +        qemu_register_reset(pc_cpu_reset, cpu);
>>       }
>>   }
>>
>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>> index e655129..99ef891 100644
>> --- a/target-i386/cpu.c
>> +++ b/target-i386/cpu.c
>
>> @@ -1749,24 +1753,89 @@ static void x86_set_cpu_model(Object *obj, const char *value, Error **errp)
>>       if (cpu_x86_register(cpu, env->cpu_model_str)<  0) {
>>           fprintf(stderr, "Unable to find x86 CPU definition\n");
>>           error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
>> +        return;
>> +    }
>> +
>> +#ifndef CONFIG_USER_ONLY
>> +    if (((env->cpuid_features&  CPUID_APIC) || smp_cpus>  1)) {
>> +        if (kvm_irqchip_in_kernel()) {
>> +            env->apic_state = qdev_create(NULL, "kvm-apic");
>> +        } else if (xen_enabled()) {
>> +            env->apic_state = qdev_create(NULL, "xen-apic");
>> +        } else {
>> +            env->apic_state = qdev_create(NULL, "apic");
>> +        }
>> +        object_property_add_child(OBJECT(cpu), "apic",
>> +            OBJECT(env->apic_state), NULL);
>> +
>> +        qdev_prop_set_uint8(env->apic_state, "id", env->cpuid_apic_id);
>> +        qdev_prop_set_ptr(env->apic_state, "cpu_env", env);
>
> I'd like to avoid re-adding this set_ptr(). We can cherry-pick my
> link<X86CPU>  property from QOM CPUState part 4 series.
sure, I'll add you link<X86CPU> patches and rebase on top of it

>
>> +    }
>> +#endif
>> +}
>> +
>> +#ifndef CONFIG_USER_ONLY
>> +static CPUDebugExcpHandler *prev_debug_excp_handler;
>> +
>> +static void breakpoint_handler(CPUX86State *env)
>> +{
>> +    CPUBreakpoint *bp;
>> +
>> +    if (env->watchpoint_hit) {
>> +        if (env->watchpoint_hit->flags&  BP_CPU) {
>> +            env->watchpoint_hit = NULL;
>> +            if (check_hw_breakpoints(env, 0)) {
>> +                raise_exception_env(EXCP01_DB, env);
>> +            } else {
>> +                cpu_resume_from_signal(env, NULL);
>> +            }
>> +        }
>> +    } else {
>> +        QTAILQ_FOREACH(bp,&env->breakpoints, entry)
>> +            if (bp->pc == env->eip) {
>> +                if (bp->flags&  BP_CPU) {
>> +                    check_hw_breakpoints(env, 1);
>> +                    raise_exception_env(EXCP01_DB, env);
>> +                }
>> +                break;
>> +            }
>> +    }
>> +    if (prev_debug_excp_handler) {
>> +        prev_debug_excp_handler(env);
>>       }
>>   }
>> +#endif
>>
>>   void x86_cpu_realize(Object *obj, Error **errp)
>>   {
>>       X86CPU *cpu = X86_CPU(obj);
>> +#ifndef CONFIG_USER_ONLY
>> +    CPUX86State *env =&cpu->env;
>> +
>> +    if (env->apic_state) {
>> +        if (qdev_init(env->apic_state)<  0) {
>> +            error_set(errp, QERR_DEVICE_INIT_FAILED,
>> +                object_get_typename(OBJECT(env->apic_state)));
>> +            return;
>> +        }
>> +    }
>> +#endif
>>
>>       mce_init(cpu);
>> -    qemu_init_vcpu(&cpu->env);
>> +    qemu_init_vcpu(env);
>
> This only works because currently qemu_init_vcpu() is a no-op macro that
> doesn't use the parameter. Please don't change it back, I guess it's a
> mismerge.
I'll fix it.
>
> We can avoid the env variable if I finish merging Paolo's series - by
> realizing the CPU the APIC as its child would get realized, too. Is the
> ordering before mce_init() mandatory here or is it just to reduce the
> #ifndef'ery?

mce_init() ordering here is not important and it is less #ifndef-s this way.
yep, with Paolo's realize patch whole #ifndef block should be thrown away.
I can split it into a separate patch, that could be easily discarded when
Paolo's realize is committed.


>> +    cpu_reset(CPU(cpu));
>>   }
>>
>>   static void x86_cpu_initfn(Object *obj)
>>   {
>>       X86CPU *cpu = X86_CPU(obj);
>>       CPUX86State *env =&cpu->env;
>> +    static int inited;
>>
>>       cpu_exec_init(env);
>>
>> +    env->cpuid_apic_id = env->cpu_index;
>> +
>>       object_property_add(obj, "family", "int",
>>                           x86_cpuid_version_get_family,
>>                           x86_cpuid_version_set_family, NULL, NULL, NULL);
>> @@ -1795,7 +1864,15 @@ static void x86_cpu_initfn(Object *obj)
>>       object_property_add_str(obj, "cpu-model",
>>           x86_get_cpu_model, x86_set_cpu_model, 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
>> +        prev_debug_excp_handler =
>> +            cpu_set_debug_excp_handler(breakpoint_handler);
>> +#endif
>> +    }
>
> Did you forget to put that into its own patch or did that not work?
> My idea was to have it first in the series so that other changes here
> and elsewhere can be rebased onto it.
>
> Also I wonder whether it would better be placed into the class_init? I'd
> tend towards initfn because that will not be invoked during type
> enumeration.

Ok, I'll split it into separate patch.
On the second thought there is no compelling reason to move this hunk in cpu.c
It could be left at board level, just moved in the beginning of pc_cpus_init()
and called only once there.

>
>>   }
>>
>>   static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
>> diff --git a/target-i386/helper.c b/target-i386/helper.c
>> index fbaeeea..38ac25d 100644
>> --- a/target-i386/helper.c
>> +++ b/target-i386/helper.c
>> @@ -941,34 +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;
>> -
>> -    if (env->watchpoint_hit) {
>> -        if (env->watchpoint_hit->flags&  BP_CPU) {
>> -            env->watchpoint_hit = NULL;
>> -            if (check_hw_breakpoints(env, 0))
>> -                raise_exception_env(EXCP01_DB, env);
>> -            else
>> -                cpu_resume_from_signal(env, NULL);
>> -        }
>> -    } else {
>> -        QTAILQ_FOREACH(bp,&env->breakpoints, entry)
>> -            if (bp->pc == env->eip) {
>> -                if (bp->flags&  BP_CPU) {
>> -                    check_hw_breakpoints(env, 1);
>> -                    raise_exception_env(EXCP01_DB, env);
>> -                }
>> -                break;
>> -            }
>> -    }
>> -    if (prev_debug_excp_handler)
>> -        prev_debug_excp_handler(env);
>> -}
>> -
>
> I wonder if that could rather stay here as non-static?
Any preference in what header file it should be?


-- 
-----
Thanks
   Igor

[Qemu-devel] [PATCH qom-next 6/6] target-i386: move reset callback to cpu.c

[Igor Mammedov]

Moving reset callback into cpu object from board level will allow
properly create object during run-time (hotplug).

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

v2:
  - fix build for i386-linux-target

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

diff --git a/hw/pc.c b/hw/pc.c
index d7845ea..868dbe7 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -885,12 +885,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));
-}
-
 void pc_cpus_init(const char *cpu_model)
 {
     X86CPU *cpu;
@@ -901,7 +895,6 @@ void pc_cpus_init(const char *cpu_model)
         if (cpu == NULL) {
             exit(1);
         }
-        qemu_register_reset(pc_cpu_reset, cpu);
     }
 }
 
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 99ef891..ad39f71 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1775,6 +1775,13 @@ static void x86_set_cpu_model(Object *obj, const char *value, Error **errp)
 }
 
 #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));
+}
+
 static CPUDebugExcpHandler *prev_debug_excp_handler;
 
 static void breakpoint_handler(CPUX86State *env)
@@ -1823,6 +1830,10 @@ void x86_cpu_realize(Object *obj, Error **errp)
 
     mce_init(cpu);
     qemu_init_vcpu(env);
+
+#ifndef CONFIG_USER_ONLY
+    qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
+#endif
     cpu_reset(CPU(cpu));
 }
 
-- 
1.7.7.6