[Qemu-devel] [RESEND PATCH v9 0/4] remove icc bus/bridge

[Qemu-devel] [RESEND PATCH v9 0/4] remove icc bus/bridge

[Zhu Guihua]

ICC Bus was used for providing a hotpluggable bus for APIC and CPU, but now we
use HotplugHandler to make hotplug. So ICC Bus is unnecessary.

This code has passed the new pc-cpu-test.
And I have tested with kvm along with kernel_irqchip=on/off, it works fine.

This patch series is based on the latest master.

v9:
 -use a callback to correct reset sequence for x86
 -update apic mmio mapping

v8:
 -add a wrapper to specify reset order

v7:
 -update to register reset handler for main_system_bus when created
 -register reset handler for apic after all devices are initialized

Chen Fan (2):
  apic: map APIC's MMIO region at each CPU's address space
  cpu/apic: drop icc bus/bridge

Zhu Guihua (2):
  x86: use new method to correct reset sequence
  icc_bus: drop the unused files

 default-configs/i386-softmmu.mak   |   1 -
 default-configs/x86_64-softmmu.mak |   1 -
 hw/cpu/Makefile.objs               |   1 -
 hw/cpu/icc_bus.c                   | 118 -------------------------------------
 hw/i386/pc.c                       |  43 +++++++-------
 hw/i386/pc_piix.c                  |   9 +--
 hw/i386/pc_q35.c                   |   9 +--
 hw/intc/apic_common.c              |  11 +---
 include/hw/cpu/icc_bus.h           |  82 --------------------------
 include/hw/i386/apic_internal.h    |   7 ++-
 include/hw/i386/pc.h               |   2 +-
 target-i386/cpu.c                  |  30 +++++++---
 12 files changed, 52 insertions(+), 262 deletions(-)
 delete mode 100644 hw/cpu/icc_bus.c
 delete mode 100644 include/hw/cpu/icc_bus.h

-- 
1.9.3

[Qemu-devel] [RESEND PATCH v9 1/4] apic: map APIC's MMIO region at each CPU's address space

[Zhu Guihua]

From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>

Replace mapping APIC at global system address space with
mapping it at per-CPU address spaces.

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
---
 hw/i386/pc.c          |  7 -------
 hw/intc/apic_common.c |  6 ------
 target-i386/cpu.c     | 21 +++++++++++++++++++++
 3 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 9f2924e..8b7dbe5 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1158,13 +1158,6 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
         object_unref(OBJECT(cpu));
     }
 
-    /* map APIC MMIO area if CPU has APIC */
-    if (cpu && cpu->apic_state) {
-        /* XXX: what if the base changes? */
-        sysbus_mmio_map_overlap(SYS_BUS_DEVICE(icc_bridge), 0,
-                                APIC_DEFAULT_ADDRESS, 0x1000);
-    }
-
     /* tell smbios about cpuid version and features */
     smbios_set_cpuid(cpu->env.cpuid_version, cpu->env.features[FEAT_1_EDX]);
 }
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 0032b97..c0b32eb 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -296,7 +296,6 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
     APICCommonClass *info;
     static DeviceState *vapic;
     static int apic_no;
-    static bool mmio_registered;
 
     if (apic_no >= MAX_APICS) {
         error_setg(errp, "%s initialization failed.",
@@ -307,11 +306,6 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
 
     info = APIC_COMMON_GET_CLASS(s);
     info->realize(dev, errp);
-    if (!mmio_registered) {
-        ICCBus *b = ICC_BUS(qdev_get_parent_bus(dev));
-        memory_region_add_subregion(b->apic_address_space, 0, &s->io_memory);
-        mmio_registered = true;
-    }
 
     /* Note: We need at least 1M to map the VAPIC option ROM */
     if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK &&
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index cfb8aa7..8eed88c 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2745,6 +2745,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
     /* TODO: convert to link<> */
     apic = APIC_COMMON(cpu->apic_state);
     apic->cpu = cpu;
+    apic->apicbase = APIC_DEFAULT_ADDRESS | MSR_IA32_APICBASE_ENABLE;
 }
 
 static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
@@ -2789,8 +2790,10 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     X86CPU *cpu = X86_CPU(dev);
     X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
     CPUX86State *env = &cpu->env;
+    APICCommonState *apic;
     Error *local_err = NULL;
     static bool ht_warned;
+    static bool apic_mmio_map_once;
 
     if (cpu->apic_id < 0) {
         error_setg(errp, "apic-id property was not initialized properly");
@@ -2877,6 +2880,24 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     if (local_err != NULL) {
         goto out;
     }
+
+    /* map APIC MMIO area */
+    apic = APIC_COMMON(cpu->apic_state);
+    if (tcg_enabled()) {
+        memory_region_add_subregion_overlap(cpu->cpu_as_root,
+                                            apic->apicbase &
+                                            MSR_IA32_APICBASE_BASE,
+                                            &apic->io_memory,
+                                            0x1000);
+    } else if (!apic_mmio_map_once) {
+        memory_region_add_subregion_overlap(get_system_memory(),
+                                            apic->apicbase &
+                                            MSR_IA32_APICBASE_BASE,
+                                            &apic->io_memory,
+                                            0x1000);
+        apic_mmio_map_once = true;
+    }
+
     cpu_reset(cs);
 
     xcc->parent_realize(dev, &local_err);
-- 
1.9.3

[Qemu-devel] [RESEND PATCH v9 2/4] x86: use new method to correct reset sequence

[Zhu Guihua]

Something must be occur during reset of the X86 platform in a specific
order. For example, the apic reset should be after some devices (such
as hpet, rtc) reset, so that the apic register could be set to default
values.

This patch uses the new QEMUMachine reset method to solve the above
problem, ensuring the various reset happen in the correct order.

Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
---
 hw/i386/pc.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 8b7dbe5..0e83dc9 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1929,6 +1929,22 @@ static void pc_machine_initfn(Object *obj)
                              NULL, &error_abort);
 }
 
+static void pc_machine_reset(void)
+{
+    CPUState *cs;
+    X86CPU *cpu;
+
+    qemu_devices_reset();
+
+    CPU_FOREACH(cs) {
+        cpu = X86_CPU(cs);
+
+        if (cpu->apic_state) {
+            device_reset(cpu->apic_state);
+        }
+    }
+}
+
 static unsigned pc_cpu_index_to_socket_id(unsigned cpu_index)
 {
     unsigned pkg_id, core_id, smt_id;
@@ -1949,6 +1965,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     mc->default_boot_order = "cad";
     mc->hot_add_cpu = pc_hot_add_cpu;
     mc->max_cpus = 255;
+    mc->reset = pc_machine_reset;
     hc->plug = pc_machine_device_plug_cb;
     hc->unplug_request = pc_machine_device_unplug_request_cb;
     hc->unplug = pc_machine_device_unplug_cb;
-- 
1.9.3

[Qemu-devel] [RESEND PATCH v9 3/4] cpu/apic: drop icc bus/bridge

[Zhu Guihua]

From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>

After CPU hotplug has been converted to BUS-less hot-plug infrastructure,
the only function ICC bus performs is to propagate reset to LAPICs. However
LAPIC could be reset by registering its reset handler after all device are
initialized.
Do so and drop ~200LOC of not needed anymore ICCBus related code.

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
---
 hw/i386/pc.c                    | 19 ++++---------------
 hw/i386/pc_piix.c               |  9 +--------
 hw/i386/pc_q35.c                |  9 +--------
 hw/intc/apic_common.c           |  5 ++---
 include/hw/i386/apic_internal.h |  7 ++++---
 include/hw/i386/pc.h            |  2 +-
 target-i386/cpu.c               |  9 +--------
 7 files changed, 14 insertions(+), 46 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 0e83dc9..7291037 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -59,7 +59,6 @@
 #include "qemu/error-report.h"
 #include "hw/acpi/acpi.h"
 #include "hw/acpi/cpu_hotplug.h"
-#include "hw/cpu/icc_bus.h"
 #include "hw/boards.h"
 #include "hw/pci/pci_host.h"
 #include "acpi-build.h"
@@ -1052,23 +1051,16 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
 }
 
 static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
-                          DeviceState *icc_bridge, Error **errp)
+                          Error **errp)
 {
     X86CPU *cpu = NULL;
     Error *local_err = NULL;
 
-    if (icc_bridge == NULL) {
-        error_setg(&local_err, "Invalid icc-bridge value");
-        goto out;
-    }
-
     cpu = cpu_x86_create(cpu_model, &local_err);
     if (local_err != NULL) {
         goto out;
     }
 
-    qdev_set_parent_bus(DEVICE(cpu), qdev_get_child_bus(icc_bridge, "icc"));
-
     object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
     object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
 
@@ -1085,7 +1077,6 @@ static const char *current_cpu_model;
 
 void pc_hot_add_cpu(const int64_t id, Error **errp)
 {
-    DeviceState *icc_bridge;
     X86CPU *cpu;
     int64_t apic_id = x86_cpu_apic_id_from_index(id);
     Error *local_err = NULL;
@@ -1114,9 +1105,7 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
         return;
     }
 
-    icc_bridge = DEVICE(object_resolve_path_type("icc-bridge",
-                                                 TYPE_ICC_BRIDGE, NULL));
-    cpu = pc_new_cpu(current_cpu_model, apic_id, icc_bridge, &local_err);
+    cpu = pc_new_cpu(current_cpu_model, apic_id, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
@@ -1124,7 +1113,7 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
     object_unref(OBJECT(cpu));
 }
 
-void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
+void pc_cpus_init(const char *cpu_model)
 {
     int i;
     X86CPU *cpu = NULL;
@@ -1150,7 +1139,7 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
 
     for (i = 0; i < smp_cpus; i++) {
         cpu = pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i),
-                         icc_bridge, &error);
+                         &error);
         if (error) {
             error_report_err(error);
             exit(1);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 9558467..b6d68aa 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -39,7 +39,6 @@
 #include "hw/kvm/clock.h"
 #include "sysemu/sysemu.h"
 #include "hw/sysbus.h"
-#include "hw/cpu/icc_bus.h"
 #include "sysemu/arch_init.h"
 #include "sysemu/block-backend.h"
 #include "hw/i2c/smbus.h"
@@ -96,7 +95,6 @@ static void pc_init1(MachineState *machine)
     MemoryRegion *ram_memory;
     MemoryRegion *pci_memory;
     MemoryRegion *rom_memory;
-    DeviceState *icc_bridge;
     PcGuestInfo *guest_info;
     ram_addr_t lowmem;
 
@@ -141,11 +139,7 @@ static void pc_init1(MachineState *machine)
         exit(1);
     }
 
-    icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
-    object_property_add_child(qdev_get_machine(), "icc-bridge",
-                              OBJECT(icc_bridge), NULL);
-
-    pc_cpus_init(machine->cpu_model, icc_bridge);
+    pc_cpus_init(machine->cpu_model);
 
     if (kvm_enabled() && kvmclock_enabled) {
         kvmclock_create();
@@ -223,7 +217,6 @@ static void pc_init1(MachineState *machine)
     if (pci_enabled) {
         ioapic_init_gsi(gsi_state, "i440fx");
     }
-    qdev_init_nofail(icc_bridge);
 
     pc_register_ferr_irq(gsi[13]);
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index c07d65b..ef3c490 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -43,7 +43,6 @@
 #include "hw/ide/pci.h"
 #include "hw/ide/ahci.h"
 #include "hw/usb.h"
-#include "hw/cpu/icc_bus.h"
 #include "qemu/error-report.h"
 #include "migration/migration.h"
 
@@ -83,7 +82,6 @@ static void pc_q35_init(MachineState *machine)
     int i;
     ICH9LPCState *ich9_lpc;
     PCIDevice *ahci;
-    DeviceState *icc_bridge;
     PcGuestInfo *guest_info;
     ram_addr_t lowmem;
     DriveInfo *hd[MAX_SATA_PORTS];
@@ -132,11 +130,7 @@ static void pc_q35_init(MachineState *machine)
         exit(1);
     }
 
-    icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
-    object_property_add_child(qdev_get_machine(), "icc-bridge",
-                              OBJECT(icc_bridge), NULL);
-
-    pc_cpus_init(machine->cpu_model, icc_bridge);
+    pc_cpus_init(machine->cpu_model);
     pc_acpi_init("q35-acpi-dsdt.aml");
 
     kvmclock_create();
@@ -237,7 +231,6 @@ static void pc_q35_init(MachineState *machine)
     if (pci_enabled) {
         ioapic_init_gsi(gsi_state, "q35");
     }
-    qdev_init_nofail(icc_bridge);
 
     pc_register_ferr_irq(gsi[13]);
 
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index c0b32eb..ad959c4 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -419,13 +419,12 @@ static Property apic_properties_common[] = {
 
 static void apic_common_class_init(ObjectClass *klass, void *data)
 {
-    ICCDeviceClass *idc = ICC_DEVICE_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->vmsd = &vmstate_apic_common;
     dc->reset = apic_reset_common;
     dc->props = apic_properties_common;
-    idc->realize = apic_common_realize;
+    dc->realize = apic_common_realize;
     /*
      * Reason: APIC and CPU need to be wired up by
      * x86_cpu_apic_create()
@@ -435,7 +434,7 @@ static void apic_common_class_init(ObjectClass *klass, void *data)
 
 static const TypeInfo apic_common_type = {
     .name = TYPE_APIC_COMMON,
-    .parent = TYPE_ICC_DEVICE,
+    .parent = TYPE_DEVICE,
     .instance_size = sizeof(APICCommonState),
     .class_size = sizeof(APICCommonClass),
     .class_init = apic_common_class_init,
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index dc7a89d..08d6f9b 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -21,7 +21,6 @@
 #define QEMU_APIC_INTERNAL_H
 
 #include "exec/memory.h"
-#include "hw/cpu/icc_bus.h"
 #include "qemu/timer.h"
 
 /* APIC Local Vector Table */
@@ -78,7 +77,7 @@ typedef struct APICCommonState APICCommonState;
 
 typedef struct APICCommonClass
 {
-    ICCDeviceClass parent_class;
+    DeviceClass parent_class;
 
     DeviceRealize realize;
     void (*set_base)(APICCommonState *s, uint64_t val);
@@ -93,7 +92,9 @@ typedef struct APICCommonClass
 } APICCommonClass;
 
 struct APICCommonState {
-    ICCDevice busdev;
+    /*< private >*/
+    DeviceState parent_obj;
+    /*< public >*/
 
     MemoryRegion io_memory;
     X86CPU *cpu;
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index d0cad87..87c6600 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -161,7 +161,7 @@ bool pc_machine_is_smm_enabled(PCMachineState *pcms);
 void pc_register_ferr_irq(qemu_irq irq);
 void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
 
-void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge);
+void pc_cpus_init(const char *cpu_model);
 void pc_hot_add_cpu(const int64_t id, Error **errp);
 void pc_acpi_init(const char *default_dsdt);
 
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 8eed88c..5bdafaa 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -43,7 +43,6 @@
 
 #include "sysemu/sysemu.h"
 #include "hw/qdev-properties.h"
-#include "hw/cpu/icc_bus.h"
 #ifndef CONFIG_USER_ONLY
 #include "exec/address-spaces.h"
 #include "hw/xen/xen.h"
@@ -2723,7 +2722,6 @@ static void mce_init(X86CPU *cpu)
 #ifndef CONFIG_USER_ONLY
 static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
 {
-    DeviceState *dev = DEVICE(cpu);
     APICCommonState *apic;
     const char *apic_type = "apic";
 
@@ -2733,11 +2731,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
         apic_type = "xen-apic";
     }
 
-    cpu->apic_state = qdev_try_create(qdev_get_parent_bus(dev), apic_type);
-    if (cpu->apic_state == NULL) {
-        error_setg(errp, "APIC device '%s' could not be created", apic_type);
-        return;
-    }
+    cpu->apic_state = DEVICE(object_new(apic_type));
 
     object_property_add_child(OBJECT(cpu), "apic",
                               OBJECT(cpu->apic_state), NULL);
@@ -3159,7 +3153,6 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
 
     xcc->parent_realize = dc->realize;
     dc->realize = x86_cpu_realizefn;
-    dc->bus_type = TYPE_ICC_BUS;
     dc->props = x86_cpu_properties;
 
     xcc->parent_reset = cc->reset;
-- 
1.9.3

[Qemu-devel] [RESEND PATCH v9 4/4] icc_bus: drop the unused files

[Zhu Guihua]

ICC bus impl has been droped, so all icc related files are not useful
any more; delete them.

Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
---
 default-configs/i386-softmmu.mak   |   1 -
 default-configs/x86_64-softmmu.mak |   1 -
 hw/cpu/Makefile.objs               |   1 -
 hw/cpu/icc_bus.c                   | 118 -------------------------------------
 include/hw/cpu/icc_bus.h           |  82 --------------------------
 5 files changed, 203 deletions(-)
 delete mode 100644 hw/cpu/icc_bus.c
 delete mode 100644 include/hw/cpu/icc_bus.h

diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
index 5eaafa1..70391ed 100644
--- a/default-configs/i386-softmmu.mak
+++ b/default-configs/i386-softmmu.mak
@@ -43,7 +43,6 @@ CONFIG_LPC_ICH9=y
 CONFIG_PCI_Q35=y
 CONFIG_APIC=y
 CONFIG_IOAPIC=y
-CONFIG_ICC_BUS=y
 CONFIG_PVPANIC=y
 CONFIG_MEM_HOTPLUG=y
 CONFIG_XIO3130=y
diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
index 28e2099..dfb8095 100644
--- a/default-configs/x86_64-softmmu.mak
+++ b/default-configs/x86_64-softmmu.mak
@@ -44,7 +44,6 @@ CONFIG_LPC_ICH9=y
 CONFIG_PCI_Q35=y
 CONFIG_APIC=y
 CONFIG_IOAPIC=y
-CONFIG_ICC_BUS=y
 CONFIG_PVPANIC=y
 CONFIG_MEM_HOTPLUG=y
 CONFIG_XIO3130=y
diff --git a/hw/cpu/Makefile.objs b/hw/cpu/Makefile.objs
index 6381238..0954a18 100644
--- a/hw/cpu/Makefile.objs
+++ b/hw/cpu/Makefile.objs
@@ -2,5 +2,4 @@ obj-$(CONFIG_ARM11MPCORE) += arm11mpcore.o
 obj-$(CONFIG_REALVIEW) += realview_mpcore.o
 obj-$(CONFIG_A9MPCORE) += a9mpcore.o
 obj-$(CONFIG_A15MPCORE) += a15mpcore.o
-obj-$(CONFIG_ICC_BUS) += icc_bus.o
 
diff --git a/hw/cpu/icc_bus.c b/hw/cpu/icc_bus.c
deleted file mode 100644
index 6646ea2..0000000
--- a/hw/cpu/icc_bus.c
+++ /dev/null
@@ -1,118 +0,0 @@
-/* icc_bus.c
- * emulate x86 ICC (Interrupt Controller Communications) bus
- *
- * Copyright (c) 2013 Red Hat, Inc
- *
- * Authors:
- *     Igor Mammedov <imammedo@redhat.com>
- *
- * 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 "hw/cpu/icc_bus.h"
-#include "hw/sysbus.h"
-
-/* icc-bridge implementation */
-
-static const TypeInfo icc_bus_info = {
-    .name = TYPE_ICC_BUS,
-    .parent = TYPE_BUS,
-    .instance_size = sizeof(ICCBus),
-};
-
-
-/* icc-device implementation */
-
-static void icc_device_realize(DeviceState *dev, Error **errp)
-{
-    ICCDeviceClass *idc = ICC_DEVICE_GET_CLASS(dev);
-
-    /* convert to QOM */
-    if (idc->realize) {
-        idc->realize(dev, errp);
-    }
-
-}
-
-static void icc_device_class_init(ObjectClass *oc, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(oc);
-
-    dc->realize = icc_device_realize;
-    dc->bus_type = TYPE_ICC_BUS;
-}
-
-static const TypeInfo icc_device_info = {
-    .name = TYPE_ICC_DEVICE,
-    .parent = TYPE_DEVICE,
-    .abstract = true,
-    .instance_size = sizeof(ICCDevice),
-    .class_size = sizeof(ICCDeviceClass),
-    .class_init = icc_device_class_init,
-};
-
-
-/*  icc-bridge implementation */
-
-typedef struct ICCBridgeState {
-    /*< private >*/
-    SysBusDevice parent_obj;
-    /*< public >*/
-
-    ICCBus icc_bus;
-    MemoryRegion apic_container;
-} ICCBridgeState;
-
-#define ICC_BRIDGE(obj) OBJECT_CHECK(ICCBridgeState, (obj), TYPE_ICC_BRIDGE)
-
-static void icc_bridge_init(Object *obj)
-{
-    ICCBridgeState *s = ICC_BRIDGE(obj);
-    SysBusDevice *sb = SYS_BUS_DEVICE(obj);
-
-    qbus_create_inplace(&s->icc_bus, sizeof(s->icc_bus), TYPE_ICC_BUS,
-                        DEVICE(s), "icc");
-
-    /* Do not change order of registering regions,
-     * APIC must be first registered region, board maps it by 0 index
-     */
-    memory_region_init(&s->apic_container, obj, "icc-apic-container",
-                       APIC_SPACE_SIZE);
-    sysbus_init_mmio(sb, &s->apic_container);
-    s->icc_bus.apic_address_space = &s->apic_container;
-}
-
-static void icc_bridge_class_init(ObjectClass *oc, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(oc);
-
-    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
-}
-
-static const TypeInfo icc_bridge_info = {
-    .name  = TYPE_ICC_BRIDGE,
-    .parent = TYPE_SYS_BUS_DEVICE,
-    .instance_init  = icc_bridge_init,
-    .instance_size  = sizeof(ICCBridgeState),
-    .class_init = icc_bridge_class_init,
-};
-
-
-static void icc_bus_register_types(void)
-{
-    type_register_static(&icc_bus_info);
-    type_register_static(&icc_device_info);
-    type_register_static(&icc_bridge_info);
-}
-
-type_init(icc_bus_register_types)
diff --git a/include/hw/cpu/icc_bus.h b/include/hw/cpu/icc_bus.h
deleted file mode 100644
index 98a979f..0000000
--- a/include/hw/cpu/icc_bus.h
+++ /dev/null
@@ -1,82 +0,0 @@
-/* icc_bus.h
- * emulate x86 ICC (Interrupt Controller Communications) bus
- *
- * Copyright (c) 2013 Red Hat, Inc
- *
- * Authors:
- *     Igor Mammedov <imammedo@redhat.com>
- *
- * 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 ICC_BUS_H
-#define ICC_BUS_H
-
-#include "exec/memory.h"
-#include "hw/qdev-core.h"
-
-#define TYPE_ICC_BUS "icc-bus"
-
-#ifndef CONFIG_USER_ONLY
-
-/**
- * ICCBus:
- *
- * ICC bus
- */
-typedef struct ICCBus {
-    /*< private >*/
-    BusState parent_obj;
-    /*< public >*/
-
-    MemoryRegion *apic_address_space;
-} ICCBus;
-
-#define ICC_BUS(obj) OBJECT_CHECK(ICCBus, (obj), TYPE_ICC_BUS)
-
-/**
- * ICCDevice:
- *
- * ICC device
- */
-typedef struct ICCDevice {
-    /*< private >*/
-    DeviceState qdev;
-    /*< public >*/
-} ICCDevice;
-
-/**
- * ICCDeviceClass:
- * @init: Initialization callback for derived classes.
- *
- * ICC device class
- */
-typedef struct ICCDeviceClass {
-    /*< private >*/
-    DeviceClass parent_class;
-    /*< public >*/
-
-    DeviceRealize realize;
-} ICCDeviceClass;
-
-#define TYPE_ICC_DEVICE "icc-device"
-#define ICC_DEVICE(obj) OBJECT_CHECK(ICCDevice, (obj), TYPE_ICC_DEVICE)
-#define ICC_DEVICE_CLASS(klass) \
-     OBJECT_CLASS_CHECK(ICCDeviceClass, (klass), TYPE_ICC_DEVICE)
-#define ICC_DEVICE_GET_CLASS(obj) \
-     OBJECT_GET_CLASS(ICCDeviceClass, (obj), TYPE_ICC_DEVICE)
-
-#define TYPE_ICC_BRIDGE "icc-bridge"
-
-#endif /* CONFIG_USER_ONLY */
-#endif
-- 
1.9.3

Re: [Qemu-devel] [RESEND PATCH v9 1/4] apic: map APIC's MMIO region at each CPU's address space

[Eduardo Habkost]

On Wed, Aug 19, 2015 at 05:36:29PM +0800, Zhu Guihua wrote:
> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> 
> Replace mapping APIC at global system address space with
> mapping it at per-CPU address spaces.

Can you improve the commit message by explaining not just what is being
done, but why this is needed?

> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> ---
>  hw/i386/pc.c          |  7 -------
>  hw/intc/apic_common.c |  6 ------
>  target-i386/cpu.c     | 21 +++++++++++++++++++++
>  3 files changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 9f2924e..8b7dbe5 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1158,13 +1158,6 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
>          object_unref(OBJECT(cpu));
>      }
>  
> -    /* map APIC MMIO area if CPU has APIC */
> -    if (cpu && cpu->apic_state) {
> -        /* XXX: what if the base changes? */
> -        sysbus_mmio_map_overlap(SYS_BUS_DEVICE(icc_bridge), 0,
> -                                APIC_DEFAULT_ADDRESS, 0x1000);
> -    }
> -
>      /* tell smbios about cpuid version and features */
>      smbios_set_cpuid(cpu->env.cpuid_version, cpu->env.features[FEAT_1_EDX]);
>  }
> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index 0032b97..c0b32eb 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -296,7 +296,6 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
>      APICCommonClass *info;
>      static DeviceState *vapic;
>      static int apic_no;
> -    static bool mmio_registered;
>  
>      if (apic_no >= MAX_APICS) {
>          error_setg(errp, "%s initialization failed.",
> @@ -307,11 +306,6 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
>  
>      info = APIC_COMMON_GET_CLASS(s);
>      info->realize(dev, errp);
> -    if (!mmio_registered) {
> -        ICCBus *b = ICC_BUS(qdev_get_parent_bus(dev));
> -        memory_region_add_subregion(b->apic_address_space, 0, &s->io_memory);
> -        mmio_registered = true;
> -    }
>  
>      /* Note: We need at least 1M to map the VAPIC option ROM */
>      if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK &&
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index cfb8aa7..8eed88c 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2745,6 +2745,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>      /* TODO: convert to link<> */
>      apic = APIC_COMMON(cpu->apic_state);
>      apic->cpu = cpu;
> +    apic->apicbase = APIC_DEFAULT_ADDRESS | MSR_IA32_APICBASE_ENABLE;
>  }
>  
>  static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
> @@ -2789,8 +2790,10 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>      X86CPU *cpu = X86_CPU(dev);
>      X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
>      CPUX86State *env = &cpu->env;
> +    APICCommonState *apic;
>      Error *local_err = NULL;
>      static bool ht_warned;
> +    static bool apic_mmio_map_once;
>  
>      if (cpu->apic_id < 0) {
>          error_setg(errp, "apic-id property was not initialized properly");
> @@ -2877,6 +2880,24 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>      if (local_err != NULL) {
>          goto out;
>      }
> +
> +    /* map APIC MMIO area */
> +    apic = APIC_COMMON(cpu->apic_state);
> +    if (tcg_enabled()) {
> +        memory_region_add_subregion_overlap(cpu->cpu_as_root,
> +                                            apic->apicbase &
> +                                            MSR_IA32_APICBASE_BASE,
> +                                            &apic->io_memory,
> +                                            0x1000);

Why exactly is this necessary? If this is necessary, why don't we need
to do this for non-TCG accelerators?


> +    } else if (!apic_mmio_map_once) {
> +        memory_region_add_subregion_overlap(get_system_memory(),
> +                                            apic->apicbase &
> +                                            MSR_IA32_APICBASE_BASE,
> +                                            &apic->io_memory,
> +                                            0x1000);
> +        apic_mmio_map_once = true;
> +    }

I see that you are doing two things at the same time:
1) Moving the memory region registration to x86_cpu_realizefn();
2) Adding a special case for TCG that uses cpu->cpu_as_root.

Doing this in two separate patches seems more appropriate.

-- 
Eduardo

Re: [Qemu-devel] [RESEND PATCH v9 2/4] x86: use new method to correct reset sequence

[Eduardo Habkost]

On Wed, Aug 19, 2015 at 05:36:30PM +0800, Zhu Guihua wrote:
> Something must be occur during reset of the X86 platform in a specific
> order. For example, the apic reset should be after some devices (such
> as hpet, rtc) reset, so that the apic register could be set to default
> values.

Can you explain in the commit message (and in a comment below) why
exactly the apic register won't be set to default values if it isn't
reset after all other devices?

> 
> This patch uses the new QEMUMachine reset method to solve the above
> problem, ensuring the various reset happen in the correct order.
> 
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> ---
>  hw/i386/pc.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 8b7dbe5..0e83dc9 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1929,6 +1929,22 @@ static void pc_machine_initfn(Object *obj)
>                               NULL, &error_abort);
>  }
>  
> +static void pc_machine_reset(void)
> +{
> +    CPUState *cs;
> +    X86CPU *cpu;
> +
> +    qemu_devices_reset();
> +

Please add a comment to the function explaining why this ordering is
important.

> +    CPU_FOREACH(cs) {
> +        cpu = X86_CPU(cs);
> +
> +        if (cpu->apic_state) {
> +            device_reset(cpu->apic_state);
> +        }
> +    }
> +}
> +
>  static unsigned pc_cpu_index_to_socket_id(unsigned cpu_index)
>  {
>      unsigned pkg_id, core_id, smt_id;
> @@ -1949,6 +1965,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      mc->default_boot_order = "cad";
>      mc->hot_add_cpu = pc_hot_add_cpu;
>      mc->max_cpus = 255;
> +    mc->reset = pc_machine_reset;
>      hc->plug = pc_machine_device_plug_cb;
>      hc->unplug_request = pc_machine_device_unplug_request_cb;
>      hc->unplug = pc_machine_device_unplug_cb;
> -- 
> 1.9.3
> 

-- 
Eduardo

Re: [Qemu-devel] [RESEND PATCH v9 1/4] apic: map APIC's MMIO region at each CPU's address space

[Paolo Bonzini]

On 21/08/2015 15:54, Eduardo Habkost wrote:
> > +    if (tcg_enabled()) {
> > +        memory_region_add_subregion_overlap(cpu->cpu_as_root,
> > +                                            apic->apicbase &
> > +                                            MSR_IA32_APICBASE_BASE,
> > +                                            &apic->io_memory,
> > +                                            0x1000);
> 
> Why exactly is this necessary? If this is necessary, why don't we need
> to do this for non-TCG accelerators?

At least KVM and qtest do not support per-CPU address spaces.  I'm not
sure about Xen, it looks like it could but it would be slower.

Paolo

Re: [Qemu-devel] [RESEND PATCH v9 1/4] apic: map APIC's MMIO region at each CPU's address space

[Peter Maydell]

On 24 August 2015 at 01:55, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 21/08/2015 15:54, Eduardo Habkost wrote:
>> > +    if (tcg_enabled()) {
>> > +        memory_region_add_subregion_overlap(cpu->cpu_as_root,
>> > +                                            apic->apicbase &
>> > +                                            MSR_IA32_APICBASE_BASE,
>> > +                                            &apic->io_memory,
>> > +                                            0x1000);
>>
>> Why exactly is this necessary? If this is necessary, why don't we need
>> to do this for non-TCG accelerators?
>
> At least KVM and qtest do not support per-CPU address spaces.

Right, but given this restriction why can't we also do whatever
we need to work without the per-CPU address spaces with TCG?

thanks
-- PMM

Re: [Qemu-devel] [RESEND PATCH v9 1/4] apic: map APIC's MMIO region at each CPU's address space

[Eduardo Habkost]

On Mon, Aug 24, 2015 at 03:56:56PM +0100, Peter Maydell wrote:
> On 24 August 2015 at 01:55, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> >
> > On 21/08/2015 15:54, Eduardo Habkost wrote:
> >> > +    if (tcg_enabled()) {
> >> > +        memory_region_add_subregion_overlap(cpu->cpu_as_root,
> >> > +                                            apic->apicbase &
> >> > +                                            MSR_IA32_APICBASE_BASE,
> >> > +                                            &apic->io_memory,
> >> > +                                            0x1000);
> >>
> >> Why exactly is this necessary? If this is necessary, why don't we need
> >> to do this for non-TCG accelerators?
> >
> > At least KVM and qtest do not support per-CPU address spaces.
> 
> Right, but given this restriction why can't we also do whatever
> we need to work without the per-CPU address spaces with TCG?

Yeah, that was my question. I know why we can't use cpu->cpu_as_root in
KVM, but I don't understand why we need to use cpu->cpu_as_root with
TCG.

If that's really an actual per-accelerator requirement we can't avoid, I
would prefer to implement it as an AccelClass method instead of a
tcg_enabled() check.

-- 
Eduardo

Re: [Qemu-devel] [RESEND PATCH v9 1/4] apic: map APIC's MMIO region at each CPU's address space

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 1694 bytes --]

(sorry for top posting)

Because the emulation quality is indeed a bit better with the per-CPU address spaces; you could move each APIC's base address independent of the others. However, this is not a feature that is actually used by anything in practice, so I doubt anyone cares about TCG implementing it correctly.

Paolo


-----Original Message-----
From: Peter Maydell [peter.maydell@linaro.org]
Received: lunedì, 24 ago 2015, 16:57
To: Paolo Bonzini [pbonzini@redhat.com]
CC: Eduardo Habkost [ehabkost@redhat.com]; Zhu Guihua [zhugh.fnst@cn.fujitsu.com]; ChenFan [chen.fan.fnst@cn.fujitsu.com]; Igor Mammedov [imammedo@redhat.com]; izumi.taku@jp.fujitsu.com, QEMU Developers [qemu-devel@nongnu.org]; Andreas Färber [afaerber@suse.de]
Subject: Re: [Qemu-devel] [RESEND PATCH v9 1/4] apic: map APIC's MMIO region at each CPU's address space

On 24 August 2015 at 01:55, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 21/08/2015 15:54, Eduardo Habkost wrote:
>> > +    if (tcg_enabled()) {
>> > +        memory_region_add_subregion_overlap(cpu->cpu_as_root,
>> > +                                            apic->apicbase &
>> > +                                            MSR_IA32_APICBASE_BASE,
>> > +                                            &apic->io_memory,
>> > +                                            0x1000);
>>
>> Why exactly is this necessary? If this is necessary, why don't we need
>> to do this for non-TCG accelerators?
>
> At least KVM and qtest do not support per-CPU address spaces.

Right, but given this restriction why can't we also do whatever
we need to work without the per-CPU address spaces with TCG?

thanks
-- PMM

[-- Attachment #2: Type: text/html, Size: 2466 bytes --]

Re: [Qemu-devel] [RESEND PATCH v9 1/4] apic: map APIC's MMIO region at each CPU's address space

[Eduardo Habkost]

On Wed, Aug 26, 2015 at 11:27:08AM -0400, Paolo Bonzini wrote:
[...]
> > >> > +    if (tcg_enabled()) {
> > >> > +        memory_region_add_subregion_overlap(cpu->cpu_as_root,
> > >> > +                                            apic->apicbase &
> > >> > +                                            MSR_IA32_APICBASE_BASE,
> > >> > +                                            &apic->io_memory,
> > >> > +                                            0x1000);
> > >>
> > >> Why exactly is this necessary? If this is necessary, why don't we need
> > >> to do this for non-TCG accelerators?
> > >
> > > At least KVM and qtest do not support per-CPU address spaces.
> > 
> > Right, but given this restriction why can't we also do whatever
> > we need to work without the per-CPU address spaces with TCG?
> > 
> 
> Because the emulation quality is indeed a bit better with the per-CPU
> address spaces; you could move each APIC's base address independent of
> the others. However, this is not a feature that is actually used by
> anything in practice, so I doubt anyone cares about TCG implementing
> it correctly.

Do we need additional changes in TCG to implement it correctly, or is
this going to work out of the box as soon as we apply this series?

If it's the latter, the patch makes sense to me (but please add a
comment to the code explaining why). If it's the former, I don't see the
point of making the code more complex before that feature is actually
implemented by TCG.

Also, we could make the logic simpler if we just check if
cpu->cpu_as_root is set, e.g.:

    /* Use per-CPU address space if available (TCG supports it, KVM
     * doesn't). This allows the APIC base address of each CPU
     * to be moved independently.
     */
    memory_region_add_subregion_overlap(cpu->cpu_as_root ?:
                                        get_system_memory(),
                                        apic->apicbase &
                                        MSR_IA32_APICBASE_BASE,
                                        &apic->io_memory,
                                        0x1000);

-- 
Eduardo

Re: [Qemu-devel] [RESEND PATCH v9 1/4] apic: map APIC's MMIO region at each CPU's address space

[Zhu Guihua]

On 08/26/2015 11:49 PM, Eduardo Habkost wrote:
> On Wed, Aug 26, 2015 at 11:27:08AM -0400, Paolo Bonzini wrote:
> [...]
>>>>>> +    if (tcg_enabled()) {
>>>>>> +        memory_region_add_subregion_overlap(cpu->cpu_as_root,
>>>>>> +                                            apic->apicbase &
>>>>>> +                                            MSR_IA32_APICBASE_BASE,
>>>>>> +                                            &apic->io_memory,
>>>>>> +                                            0x1000);
>>>>> Why exactly is this necessary? If this is necessary, why don't we need
>>>>> to do this for non-TCG accelerators?
>>>> At least KVM and qtest do not support per-CPU address spaces.
>>> Right, but given this restriction why can't we also do whatever
>>> we need to work without the per-CPU address spaces with TCG?
>>>
>> Because the emulation quality is indeed a bit better with the per-CPU
>> address spaces; you could move each APIC's base address independent of
>> the others. However, this is not a feature that is actually used by
>> anything in practice, so I doubt anyone cares about TCG implementing
>> it correctly.
> Do we need additional changes in TCG to implement it correctly, or is
> this going to work out of the box as soon as we apply this series?
>
> If it's the latter, the patch makes sense to me (but please add a
> comment to the code explaining why). If it's the former, I don't see the
> point of making the code more complex before that feature is actually
> implemented by TCG.
>
> Also, we could make the logic simpler if we just check if
> cpu->cpu_as_root is set, e.g.:
>
>      /* Use per-CPU address space if available (TCG supports it, KVM
>       * doesn't). This allows the APIC base address of each CPU
>       * to be moved independently.
>       */
>      memory_region_add_subregion_overlap(cpu->cpu_as_root ?:
>                                          get_system_memory(),
>                                          apic->apicbase &
>                                          MSR_IA32_APICBASE_BASE,
>                                          &apic->io_memory,
>                                          0x1000);

Yeah, the logic is better. I will take this, thanks.
And, comments will be added in next version.

Thanks,
Zhu