qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH qom-next 11/12] target-i386: initialize APIC at


From: Igor Mammedov
Subject: Re: [Qemu-devel] [PATCH qom-next 11/12] target-i386: initialize APIC at CPU level
Date: Wed, 30 May 2012 16:27:34 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20120430 Thunderbird/12.0.1

On 05/30/2012 12:41 PM, Jan Kiszka wrote:
On 2012-05-30 00:10, Igor Mammedov wrote:
(L)APIC is a part of cpu [1] so move APIC initialization inside of
x86_cpu object. Since cpu_model and override flags currently specify
whether APIC should be created or not, APIC creation is moved into
cpu_model property setter. And APIC initialization is moved into
x86_cpu_apic_init() which is called from x86_cpu_realize().

[1] - all x86 cpus have integrated APIC if we overlook existence of i486,
and it's more convenient to model after majority of them.

v2:
  * init APIC mapping at cpu level, due to Peter's objection to putting
    it into APIC's initfn and Jan's suggestion to do it inside cpu.

Signed-off-by: Igor Mammedov<address@hidden>
---
  hw/pc.c           |   44 -------------------------------------
  target-i386/cpu.c |   62 +++++++++++++++++++++++++++++++++++++++++++++++++++++
  2 files changed, 62 insertions(+), 44 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index d178801..986f119 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"
@@ -70,8 +69,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 {
@@ -879,42 +876,6 @@ DeviceState *cpu_get_current_apic(void)
      }
  }

-static DeviceState *apic_init(void *env, uint8_t apic_id)
-{
-    DeviceState *dev;
-    Error *error = NULL;
-    static int apic_mapped;
-
-    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);
-    object_property_set_link(OBJECT(dev), OBJECT(ENV_GET_CPU(env)), "cpu",
-&error);
-    if (error_is_set(&error)) {
-        qerror_report_err(error);
-        error_free(error);
-        exit(1);
-    }
-    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;
-}
-
  void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
  {
      CPUX86State *s = opaque;
@@ -933,17 +894,12 @@ static void pc_cpu_reset(void *opaque)
  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;
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 2610d96..b649904 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,11 @@

  #include "hyperv.h"

+#include "sysemu.h"
+#ifndef CONFIG_USER_ONLY
+#include "hw/sysbus.h"
+#endif
+
  /* 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,13 +1755,69 @@ 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);
+        object_property_set_link(OBJECT(env->apic_state), OBJECT(cpu), "cpu",
+                                 errp);
+        if (error_is_set(errp)) {
+            return;
+        }
+    }
+#endif
+}
+
+#ifndef CONFIG_USER_ONLY
+#define MSI_ADDR_BASE 0xfee00000
+
+static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
+{
+    static int apic_mapped;
+    CPUX86State *env =&cpu->env;
+
+    if (env->apic_state == NULL) {
+        return;
+    }
+
+    if (qdev_init(env->apic_state)) {
+        error_set(errp, QERR_DEVICE_INIT_FAILED,
+                  object_get_typename(OBJECT(env->apic_state)));
+        return;
+    }
+
+    /* 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(env->apic_state), 0, MSI_ADDR_BASE);
+        apic_mapped = 1;
      }
  }
+#endif

  void x86_cpu_realize(Object *obj, Error **errp)
  {
      X86CPU *cpu = X86_CPU(obj);

+    x86_cpu_apic_init(cpu, errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+

I guess you are lacking #ifndef CONFIG_USER_ONLY here already (for the
sake of bisectability). Or, likely better, let x86_cpu_apic_init do
nothing for usermode emulation.
initially x86_cpu_apic_init() was doing nothing for usermode, hence lack of
#ifndef CONFIG_USER_ONLY, but then for patch 12/12 #ifndef CONFIG_USER_ONLY
required here any way so I've removed usermode stub x86_cpu_apic_init() and
squashed change in wrong patch :(.

And since I need #ifndef in initfn anyway then probably there is no point
in having x86_cpu_apic_init(), I'll move its body in initfn then.

Thanks




Jan


--
-----
 Igor



reply via email to

[Prev in Thread] Current Thread [Next in Thread]