qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH 1/1] Introduce a new bus "ICC" to connect APIC


From: Anthony Liguori
Subject: Re: [Qemu-devel] [PATCH 1/1] Introduce a new bus "ICC" to connect APIC
Date: Wed, 19 Oct 2011 07:54:48 -0500
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.21) Gecko/20110831 Lightning/1.0b2 Thunderbird/3.1.13

On 10/19/2011 05:53 AM, Jan Kiszka wrote:
On 2011-10-19 03:55, address@hidden wrote:
From: Liu Ping Fan<address@hidden>

Introduce a new structure CPUS as the controller of ICC (INTERRUPT
CONTROLLER COMMUNICATIONS), and new bus "ICC" to hold APIC,instead
of sysbus. So we can support APIC hot-plug feature.

Signed-off-by: liu ping fan<address@hidden>
---
  Makefile.target |    1 +
  hw/apic.c       |   25 +++++++++++----
  hw/apic.h       |    1 +
  hw/icc_bus.c    |   91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
  hw/icc_bus.h    |   56 ++++++++++++++++++++++++++++++++++
  hw/pc.c         |   11 ++++--
  6 files changed, 174 insertions(+), 11 deletions(-)
  create mode 100644 hw/icc_bus.c
  create mode 100644 hw/icc_bus.h

diff --git a/Makefile.target b/Makefile.target
index 9011f28..5607c6d 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -241,6 +241,7 @@ obj-i386-$(CONFIG_KVM) += kvmclock.o
  obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
  obj-i386-y += testdev.o
  obj-i386-y += acpi.o acpi_piix4.o
+obj-i386-y += icc_bus.o

  obj-i386-y += pcspk.o i8254.o
  obj-i386-$(CONFIG_KVM_PIT) += i8254-kvm.o
diff --git a/hw/apic.c b/hw/apic.c
index 69d6ac5..00d2297 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -21,9 +21,10 @@
  #include "ioapic.h"
  #include "qemu-timer.h"
  #include "host-utils.h"
-#include "sysbus.h"
+#include "icc_bus.h"
  #include "trace.h"
  #include "kvm.h"
+#include "exec-memory.h"

Mmh, don't your rather want memory.h?


  /* APIC Local Vector Table */
  #define APIC_LVT_TIMER   0
@@ -80,7 +81,7 @@
  typedef struct APICState APICState;

  struct APICState {
-    SysBusDevice busdev;
+    ICCBusDevice busdev;
      MemoryRegion io_memory;
      void *cpu_env;
      uint32_t apicbase;
@@ -1104,9 +1105,20 @@ static const MemoryRegionOps apic_io_ops = {
      .endianness = DEVICE_NATIVE_ENDIAN,
  };

-static int apic_init1(SysBusDevice *dev)
+/**/

Empty comment.

+int apic_mmio_map(DeviceState *dev, target_phys_addr_t base)
  {
-    APICState *s = FROM_SYSBUS(APICState, dev);
+    APICState *s = DO_UPCAST(APICState, busdev.qdev, dev);
+
+    memory_region_add_subregion(get_system_memory(),
+                                base,
+&s->io_memory);
+    return 0;
+}
+
+static int apic_init1(ICCBusDevice *dev)
+{
+    APICState *s = DO_UPCAST(APICState, busdev, dev);
      static int last_apic_idx;

      if (last_apic_idx>= MAX_APICS) {
@@ -1114,7 +1126,6 @@ static int apic_init1(SysBusDevice *dev)
      }
      memory_region_init_io(&s->io_memory,&apic_io_ops, s, "apic",
                            MSI_ADDR_SIZE);
-    sysbus_init_mmio_region(dev,&s->io_memory);

      s->timer = qemu_new_timer_ns(vm_clock, apic_timer, s);
      s->idx = last_apic_idx++;
@@ -1122,7 +1133,7 @@ static int apic_init1(SysBusDevice *dev)
      return 0;
  }

-static SysBusDeviceInfo apic_info = {
+static ICCBusDeviceInfo apic_info = {
      .init = apic_init1,
      .qdev.name = "apic",
      .qdev.size = sizeof(APICState),
@@ -1138,7 +1149,7 @@ static SysBusDeviceInfo apic_info = {

  static void apic_register_devices(void)
  {
-    sysbus_register_withprop(&apic_info);
+    iccbus_register_devinfo(&apic_info);
  }

  device_init(apic_register_devices)
diff --git a/hw/apic.h b/hw/apic.h
index c857d52..e2c0af5 100644
--- a/hw/apic.h
+++ b/hw/apic.h
@@ -20,6 +20,7 @@ void cpu_set_apic_tpr(DeviceState *s, uint8_t val);
  uint8_t cpu_get_apic_tpr(DeviceState *s);
  void apic_init_reset(DeviceState *s);
  void apic_sipi(DeviceState *s);
+int apic_mmio_map(DeviceState *dev, target_phys_addr_t base);

  /* pc.c */
  int cpu_is_bsp(CPUState *env);
diff --git a/hw/icc_bus.c b/hw/icc_bus.c
new file mode 100644
index 0000000..61a408e
--- /dev/null
+++ b/hw/icc_bus.c
@@ -0,0 +1,91 @@
+/* icc_bus.c
+ * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus

Copyright?

+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see<http://www.gnu.org/licenses/>
+ */
+#include "icc_bus.h"
+
+static CPUS *dummy_cpus;

Why "dummy"? Also, no statics please. The bus owner is the chipset
(440fx), so embedded it there. That also avoid that strange "cpus"
device below.

That's an odd model IMHO. The i440fx doesn't implement the ICC bus. The ICC bus is entirely independent of the northbridge.

Maybe CPUSockets would be a better device name?

Regards,

Anthony Liguori


+
+
+static ICCBusInfo icc_bus_info = {
+    .qinfo.name = "icc",
+    .qinfo.size = sizeof(ICCBus),
+    .qinfo.props = (Property[]) {
+        DEFINE_PROP_END_OF_LIST(),
+    }
+};
+
+
+

A single line-break is sufficient, here and elsewhere.

+static int iccbus_device_init(DeviceState *dev, DeviceInfo *base)
+{
+    ICCBusDeviceInfo *info = container_of(base, ICCBusDeviceInfo, qdev);
+    ICCBusDevice *idev = DO_UPCAST(ICCBusDevice, qdev, dev);
+
+    return info->init(idev);
+}
+
+void iccbus_register_devinfo(ICCBusDeviceInfo *info)
+{
+    info->qdev.init = iccbus_device_init;
+    info->qdev.bus_info =&icc_bus_info.qinfo;
+    assert(info->qdev.size>= sizeof(ICCBusDevice));
+    qdev_register(&info->qdev);
+}
+
+
+
+BusState *get_icc_bus(void)
+{
+    return&dummy_cpus->icc_bus->qbus;
+}

See above: the chipset should manage this. Do we have a circular
dependency then?

+
+static void cpus_reset(DeviceState *d)
+{
+}

[ qdev.reset is an optional callback. But this is obsolete if you embed
the bus into the chipset. ]

+
+/*Must be called before vcpu's creation*/
+static int cpus_init(SysBusDevice *dev)
+{
+    CPUS *cpus = DO_UPCAST(CPUS, sysdev, dev);
+    BusState *b = qbus_create(&icc_bus_info.qinfo,
+&dummy_cpus->sysdev.qdev,
+                              "icc");
+    if (b == NULL) {
+        return -1;
+    }
+    b->allow_hotplug = 1; /* Yes, we can */
+    cpus->icc_bus = DO_UPCAST(ICCBus, qbus, b);
+    dummy_cpus = cpus;
+    return 0;
+
+}
+
+
+static SysBusDeviceInfo cpus_info = {
+    .init = cpus_init,
+    .qdev.name = "cpus",
+    .qdev.size = sizeof(CPUS),
+    /*.qdev.vmsd =&vmstate_apic,*/

[ No empty vmsd please. Either a device is unmigratable (I hope this one
is not :) ) or it is supposed to provide a stub vmsd with empty 'fields'
array. ]

+    .qdev.reset = cpus_reset,
+    .qdev.no_user = 1,
+};
+
+static void cpus_register_devices(void)
+{
+    sysbus_register_withprop(&cpus_info);
+}
+
+device_init(cpus_register_devices)
diff --git a/hw/icc_bus.h b/hw/icc_bus.h
new file mode 100644
index 0000000..64ac153
--- /dev/null
+++ b/hw/icc_bus.h
@@ -0,0 +1,56 @@
+/* ICCBus.h
+ * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see<http://www.gnu.org/licenses/>
+ */
+#ifndef QEMU_ICC_H
+#define QEMU_ICC_H
+
+#include "qdev.h"
+#include "sysbus.h"
+
+typedef struct CPUS CPUS;
+typedef struct ICCBus ICCBus;
+typedef struct ICCBusInfo ICCBusInfo;
+typedef struct ICCBusDevice ICCBusDevice;
+typedef struct ICCBusDeviceInfo ICCBusDeviceInfo;
+
+struct CPUS {
+    SysBusDevice sysdev;
+    ICCBus *icc_bus;
+};
+
+struct ICCBus {
+    BusState qbus;
+};
+
+struct ICCBusInfo {
+    BusInfo qinfo;
+};
+struct ICCBusDevice {
+    DeviceState qdev;
+};
+
+typedef int (*iccbus_initfn)(ICCBusDevice *dev);
+
+struct ICCBusDeviceInfo {
+    DeviceInfo qdev;
+    iccbus_initfn init;
+};
+
+
+void iccbus_register_devinfo(ICCBusDeviceInfo *info);
+BusState *get_icc_bus(void);
+
+#endif
diff --git a/hw/pc.c b/hw/pc.c
index 6b3662e..78b7826 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -24,6 +24,7 @@
  #include "hw.h"
  #include "pc.h"
  #include "apic.h"
+#include "icc_bus.h"
  #include "fdc.h"
  #include "ide.h"
  #include "pci.h"
@@ -875,21 +876,21 @@ DeviceState *cpu_get_current_apic(void)
  static DeviceState *apic_init(void *env, uint8_t apic_id)
  {
      DeviceState *dev;
-    SysBusDevice *d;
+    BusState *b;
      static int apic_mapped;

-    dev = qdev_create(NULL, "apic");
+    b = get_icc_bus();
+    dev = qdev_create(b, "apic");
      qdev_prop_set_uint8(dev, "id", apic_id);
      qdev_prop_set_ptr(dev, "cpu_env", env);
      qdev_init_nofail(dev);
-    d = sysbus_from_qdev(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(d, 0, MSI_ADDR_BASE);
+        apic_mmio_map(dev, MSI_ADDR_BASE);
          apic_mapped = 1;
      }

@@ -955,6 +956,8 @@ CPUState *pc_new_cpu(const char *cpu_model)
  void pc_cpus_init(const char *cpu_model)
  {
      int i;
+    DeviceState *d = qdev_create(NULL, "cpus");
+    qdev_init_nofail(d);

      /* init CPUs */
      for(i = 0; i<  smp_cpus; i++) {

Cool. Looking forward to seeing CPU hotplug work again, and then finally
in upstream!

Jan





reply via email to

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