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

[Qemu-devel] [RESEND PATCH v8 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.

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

v6:
 -reword commit message
 -drop NULL check for APIC device
 -use C cast instead of QOM cast

v5:
 -convert DEVICE() casts to C casts
 -use a local variable instead of doing the cast inline twice
 -drop to set cpu's parent bus
 -rename patch 3's subject
 -fix a bug about setting cpu's apic base

v4:
 -add wrapper to get root memory region from address space
 -set cpu apic base's default value in x86_cpu_apic_create()
 -drop NULL check for cpu apic_state
 -put drop of the unused files about icc_bus into a seprate patch
 -put DEVICE() casts into a seprate patch

v3:
 -replace init apic by object_new()
 -add reset apic at the time of CPU reset

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

Zhu Guihua (2):
  hw: add a wrapper for registering reset handler
  icc_bus: drop the unused files

 default-configs/i386-softmmu.mak   |   1 -
 default-configs/x86_64-softmmu.mak |   1 -
 exec.c                             |   5 ++
 hw/cpu/Makefile.objs               |   1 -
 hw/cpu/icc_bus.c                   | 118 -------------------------------------
 hw/i386/pc.c                       |  31 +++-------
 hw/i386/pc_piix.c                  |   9 +--
 hw/i386/pc_q35.c                   |   9 +--
 hw/intc/apic_common.c              |  19 +++---
 include/exec/memory.h              |   5 ++
 include/hw/cpu/icc_bus.h           |  82 --------------------------
 include/hw/hw.h                    |   4 ++
 include/hw/i386/apic_internal.h    |   7 ++-
 include/hw/i386/pc.h               |   2 +-
 target-i386/cpu.c                  |  25 +++++---
 target-i386/cpu.h                  |   1 +
 vl.c                               |  18 +++++-
 17 files changed, 75 insertions(+), 263 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 v8 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>
---
 exec.c                |  5 +++++
 hw/i386/pc.c          |  7 -------
 hw/intc/apic_common.c | 14 ++++++++------
 include/exec/memory.h |  5 +++++
 target-i386/cpu.c     |  2 ++
 5 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/exec.c b/exec.c
index f7883d2..1cd2e74 100644
--- a/exec.c
+++ b/exec.c
@@ -2710,6 +2710,11 @@ void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
     cpu_notify_map_clients();
 }
 
+MemoryRegion *address_space_root_memory_region(AddressSpace *as)
+{
+    return as->root;
+}
+
 void *cpu_physical_memory_map(hwaddr addr,
                               hwaddr *plen,
                               int is_write)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 7072930..9f16128 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1076,13 +1076,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..cf105f5 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -296,7 +296,8 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
     APICCommonClass *info;
     static DeviceState *vapic;
     static int apic_no;
-    static bool mmio_registered;
+    CPUState *cpu = CPU(s->cpu);
+    MemoryRegion *root;
 
     if (apic_no >= MAX_APICS) {
         error_setg(errp, "%s initialization failed.",
@@ -307,11 +308,12 @@ 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;
-    }
+
+    root = address_space_root_memory_region(cpu->as);
+    memory_region_add_subregion_overlap(root,
+                                        s->apicbase & MSR_IA32_APICBASE_BASE,
+                                        &s->io_memory,
+                                        0x1000);
 
     /* Note: We need at least 1M to map the VAPIC option ROM */
     if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK &&
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 8ae004e..811f027 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1308,6 +1308,11 @@ void *address_space_map(AddressSpace *as, hwaddr addr,
 void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
                          int is_write, hwaddr access_len);
 
+/* address_space_root_memory_region: get root memory region
+ *
+ * @as: #AddressSpace to be accessed
+ */
+MemoryRegion *address_space_root_memory_region(AddressSpace *as);
 
 #endif
 
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 36b07f9..1fb88f6 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2741,6 +2741,8 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
     /* TODO: convert to link<> */
     apic = APIC_COMMON(cpu->apic_state);
     apic->cpu = cpu;
+    cpu_set_apic_base(cpu->apic_state,
+                      APIC_DEFAULT_ADDRESS | MSR_IA32_APICBASE_ENABLE);
 }
 
 static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
-- 
1.9.3

[Qemu-devel] [RESEND PATCH v8 2/4] hw: add a wrapper for registering reset handler

[Zhu Guihua]

Add a wrapper to specify reset order when registering reset handler,
instead of non-obvious initiazation code ordering.

Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
---
 include/hw/hw.h |  4 ++++
 vl.c            | 18 +++++++++++++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/include/hw/hw.h b/include/hw/hw.h
index c78adae..d9375e7 100644
--- a/include/hw/hw.h
+++ b/include/hw/hw.h
@@ -37,7 +37,11 @@
 #endif
 
 typedef void QEMUResetHandler(void *opaque);
+typedef uint64_t QEMUResetOrder;
+#define default_reset_order 0x0
 
+void qemu_register_reset_common(QEMUResetHandler *func, void *opaque,
+                                QEMUResetOrder reset_order);
 void qemu_register_reset(QEMUResetHandler *func, void *opaque);
 void qemu_unregister_reset(QEMUResetHandler *func, void *opaque);
 
diff --git a/vl.c b/vl.c
index 69ad90c..b205a9b 100644
--- a/vl.c
+++ b/vl.c
@@ -1589,6 +1589,7 @@ typedef struct QEMUResetEntry {
     QTAILQ_ENTRY(QEMUResetEntry) entry;
     QEMUResetHandler *func;
     void *opaque;
+    QEMUResetOrder reset_order;
 } QEMUResetEntry;
 
 static QTAILQ_HEAD(reset_handlers, QEMUResetEntry) reset_handlers =
@@ -1672,15 +1673,30 @@ static int qemu_debug_requested(void)
     return r;
 }
 
-void qemu_register_reset(QEMUResetHandler *func, void *opaque)
+void qemu_register_reset_common(QEMUResetHandler *func, void *opaque,
+                                QEMUResetOrder reset_order)
 {
+    QEMUResetEntry *item;
     QEMUResetEntry *re = g_malloc0(sizeof(QEMUResetEntry));
 
     re->func = func;
     re->opaque = opaque;
+    re->reset_order = reset_order;
+
+    QTAILQ_FOREACH(item, &reset_handlers, entry) {
+        if (re->reset_order >= item->reset_order)
+            continue;
+        QTAILQ_INSERT_BEFORE(item, re, entry);
+        return;
+    }
     QTAILQ_INSERT_TAIL(&reset_handlers, re, entry);
 }
 
+void qemu_register_reset(QEMUResetHandler *func, void *opaque)
+{
+    qemu_register_reset_common(func, opaque, default_reset_order);
+}
+
 void qemu_unregister_reset(QEMUResetHandler *func, void *opaque)
 {
     QEMUResetEntry *re;
-- 
1.9.3

[Qemu-devel] [RESEND PATCH v8 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                    | 24 +++++++++---------------
 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               | 23 +++++++++++++++--------
 target-i386/cpu.h               |  1 +
 8 files changed, 34 insertions(+), 46 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 9f16128..c547d74 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"
@@ -969,27 +968,25 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
     }
 }
 
+#define x86_cpu_apic_reset_order 0x1
+
 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);
 
+    qemu_register_reset_common(x86_cpu_apic_reset, cpu,
+                               x86_cpu_apic_reset_order);
+
 out:
     if (local_err) {
         error_propagate(errp, local_err);
@@ -1003,7 +1000,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;
@@ -1032,9 +1028,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;
@@ -1042,7 +1036,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;
@@ -1068,7 +1062,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 e142f75..7e9a185 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"
@@ -98,7 +97,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;
 
@@ -142,11 +140,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();
@@ -229,7 +223,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 082cd93..9af1d06 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"
 
@@ -85,7 +84,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];
@@ -133,11 +131,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();
@@ -239,7 +233,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 cf105f5..f4c7f08 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -427,13 +427,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()
@@ -443,7 +442,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 86c5651..832f68c 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -161,7 +161,7 @@ extern int fd_bootchk;
 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 1fb88f6..e169ce3 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"
@@ -2719,7 +2718,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";
 
@@ -2729,11 +2727,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);
@@ -2754,6 +2748,20 @@ static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
                              errp);
 }
 
+void x86_cpu_apic_reset(void *opaque)
+{
+    X86CPU *cpu = opaque;
+    DeviceClass *dc;
+
+    if (cpu->apic_state) {
+        dc = DEVICE_GET_CLASS(cpu->apic_state);
+
+        if (dc->reset != NULL) {
+            (*dc->reset)(cpu->apic_state);
+        }
+    }
+}
+
 static void x86_cpu_machine_done(Notifier *n, void *unused)
 {
     X86CPU *cpu = container_of(n, X86CPU, machine_done);
@@ -3136,7 +3144,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;
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 603aaf0..94f646d 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1117,6 +1117,7 @@ void x86_stl_phys_notdirty(CPUState *cs, hwaddr addr, uint32_t val);
 void x86_stw_phys(CPUState *cs, hwaddr addr, uint32_t val);
 void x86_stl_phys(CPUState *cs, hwaddr addr, uint32_t val);
 void x86_stq_phys(CPUState *cs, hwaddr addr, uint64_t val);
+void x86_cpu_apic_reset(void *opaque);
 #endif
 
 static inline bool hw_local_breakpoint_enabled(unsigned long dr7, int index)
-- 
1.9.3

[Qemu-devel] [RESEND PATCH v8 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 91d602c..0759f22 100644
--- a/default-configs/i386-softmmu.mak
+++ b/default-configs/i386-softmmu.mak
@@ -42,7 +42,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 62575eb..08aac8c 100644
--- a/default-configs/x86_64-softmmu.mak
+++ b/default-configs/x86_64-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/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 v8 1/4] apic: map APIC's MMIO region at each CPU's address space

[Andreas Färber]

Am 25.06.2015 um 04:17 schrieb 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>
> ---
>  exec.c                |  5 +++++
>  hw/i386/pc.c          |  7 -------
>  hw/intc/apic_common.c | 14 ++++++++------
>  include/exec/memory.h |  5 +++++
>  target-i386/cpu.c     |  2 ++
>  5 files changed, 20 insertions(+), 13 deletions(-)

Eduardo, is this for you?

> diff --git a/exec.c b/exec.c
> index f7883d2..1cd2e74 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2710,6 +2710,11 @@ void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
>      cpu_notify_map_clients();
>  }
>  
> +MemoryRegion *address_space_root_memory_region(AddressSpace *as)
> +{
> +    return as->root;
> +}
> +
>  void *cpu_physical_memory_map(hwaddr addr,
>                                hwaddr *plen,
>                                int is_write)

This looks trivial and could've been a separate preparatory patch.
Paolo, is this part okay with you?

> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 7072930..9f16128 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1076,13 +1076,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..cf105f5 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -296,7 +296,8 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
>      APICCommonClass *info;
>      static DeviceState *vapic;
>      static int apic_no;
> -    static bool mmio_registered;
> +    CPUState *cpu = CPU(s->cpu);
> +    MemoryRegion *root;

Nit: These variables were sorted by non-static vs. static, so the new
ones should've gone before vapic.

>  
>      if (apic_no >= MAX_APICS) {
>          error_setg(errp, "%s initialization failed.",
> @@ -307,11 +308,12 @@ 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;
> -    }
> +
> +    root = address_space_root_memory_region(cpu->as);
> +    memory_region_add_subregion_overlap(root,
> +                                        s->apicbase & MSR_IA32_APICBASE_BASE,
> +                                        &s->io_memory,
> +                                        0x1000);
>  
>      /* Note: We need at least 1M to map the VAPIC option ROM */
>      if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK &&
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 8ae004e..811f027 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1308,6 +1308,11 @@ void *address_space_map(AddressSpace *as, hwaddr addr,
>  void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
>                           int is_write, hwaddr access_len);
>  
> +/* address_space_root_memory_region: get root memory region
> + *
> + * @as: #AddressSpace to be accessed
> + */
> +MemoryRegion *address_space_root_memory_region(AddressSpace *as);
>  
>  #endif
>  
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 36b07f9..1fb88f6 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2741,6 +2741,8 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>      /* TODO: convert to link<> */
>      apic = APIC_COMMON(cpu->apic_state);
>      apic->cpu = cpu;
> +    cpu_set_apic_base(cpu->apic_state,
> +                      APIC_DEFAULT_ADDRESS | MSR_IA32_APICBASE_ENABLE);
>  }
>  
>  static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)

Otherwise, if it works now, LGTM.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)

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

[Paolo Bonzini]

On 25/06/2015 18:00, Andreas Färber wrote:
>> -    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;
>> -    }
>> +
>> +    root = address_space_root_memory_region(cpu->as);
>> +    memory_region_add_subregion_overlap(root,
>> +                                        s->apicbase & MSR_IA32_APICBASE_BASE,
>> +                                        &s->io_memory,
>> +                                        0x1000);
>>  

I had already remarked that this patch is wrong.  cpu->as is completely
unused on KVM, for example.

Paolo

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

[Andreas Färber]

Am 25.06.2015 um 18:02 schrieb Paolo Bonzini:
> On 25/06/2015 18:00, Andreas Färber wrote:
>>> -    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;
>>> -    }
>>> +
>>> +    root = address_space_root_memory_region(cpu->as);
>>> +    memory_region_add_subregion_overlap(root,
>>> +                                        s->apicbase & MSR_IA32_APICBASE_BASE,
>>> +                                        &s->io_memory,
>>> +                                        0x1000);
>>>  
> 
> I had already remarked that this patch is wrong.  cpu->as is completely
> unused on KVM, for example.

Ah, then I don't understand this [RESEND]. ;)
Either way, not on my plate ATM, it seems.

Did you also outline how it is supposed to be done instead?

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)

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

[Andreas Färber]

Am 25.06.2015 um 04:17 schrieb 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                    | 24 +++++++++---------------
>  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               | 23 +++++++++++++++--------
>  target-i386/cpu.h               |  1 +
>  8 files changed, 34 insertions(+), 46 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 9f16128..c547d74 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"
> @@ -969,27 +968,25 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
>      }
>  }
>  
> +#define x86_cpu_apic_reset_order 0x1
> +
>  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);
>  
> +    qemu_register_reset_common(x86_cpu_apic_reset, cpu,
> +                               x86_cpu_apic_reset_order);

This usage of an out-of-file reset handler with opaque argument is a
little ugly.

> +
>  out:
>      if (local_err) {
>          error_propagate(errp, local_err);
> @@ -1003,7 +1000,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;
> @@ -1032,9 +1028,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;
> @@ -1042,7 +1036,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;
> @@ -1068,7 +1062,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/target-i386/cpu.c b/target-i386/cpu.c
> index 1fb88f6..e169ce3 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"
> @@ -2719,7 +2718,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";
>  
> @@ -2729,11 +2727,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);
> @@ -2754,6 +2748,20 @@ static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
>                               errp);
>  }
>  
> +void x86_cpu_apic_reset(void *opaque)
> +{
> +    X86CPU *cpu = opaque;
> +    DeviceClass *dc;
> +
> +    if (cpu->apic_state) {
> +        dc = DEVICE_GET_CLASS(cpu->apic_state);
> +
> +        if (dc->reset != NULL) {
> +            (*dc->reset)(cpu->apic_state);

This is the same as dc->reset(cpu->apic_state); ...

> +        }

... which makes this the same as device_reset(cpu->apic_state);.
Why does this need to be a reset handler of its own?

> +    }
> +}
> +
>  static void x86_cpu_machine_done(Notifier *n, void *unused)
>  {
>      X86CPU *cpu = container_of(n, X86CPU, machine_done);
> @@ -3136,7 +3144,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;
[snip]

More comments on the preceding patch.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)

Re: [Qemu-devel] [RESEND PATCH v8 2/4] hw: add a wrapper for registering reset handler

[Andreas Färber]

Am 25.06.2015 um 04:17 schrieb Zhu Guihua:
> Add a wrapper to specify reset order when registering reset handler,
> instead of non-obvious initiazation code ordering.

"initialization", and this sentence is not really telling to me.

What issue is this solving or, more likely, working around?

In the next patch the APIC is being moved from ICC bus (which, as it has
a device parent, would seem to not register its own reset handler in
qdev.c) to no bus and thus seemingly no implicit reset handler either.

We've been trying to get away from reset handlers and move to reset
callbacks that propagate through devices and buses for a long time. This
patch feels like a step backwards. If there is indeed no simpler
solution, this at least deserves a better justification.

If more people start using values like 0x1, we'll have the same ordering
issues unless we use some global enum to coordinate them.

> 
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> ---
>  include/hw/hw.h |  4 ++++
>  vl.c            | 18 +++++++++++++++++-
>  2 files changed, 21 insertions(+), 1 deletion(-)

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)

Re: [Qemu-devel] [RESEND PATCH v8 2/4] hw: add a wrapper for registering reset handler

[Paolo Bonzini]

On 25/06/2015 04:17, Zhu Guihua wrote:
> Add a wrapper to specify reset order when registering reset handler,
> instead of non-obvious initiazation code ordering.
> 
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>

I'm sorry, this is not really acceptable.  The initialization code
ordering is good because it should be okay to run reset handlers in the
same order as code is run.  If there are dependencies between reset
handlers, a random integer is not a maintainable way to maintain them.

Instead, you should have a single reset handler that calls the reset
handlers in the right order; for example a qdev bus such as icc_bus
always resets children before parents.

Are you sure that you want to remove the icc_bus?... What are you
gaining exactly by doing so?

Paolo

> ---
>  include/hw/hw.h |  4 ++++
>  vl.c            | 18 +++++++++++++++++-
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/hw.h b/include/hw/hw.h
> index c78adae..d9375e7 100644
> --- a/include/hw/hw.h
> +++ b/include/hw/hw.h
> @@ -37,7 +37,11 @@
>  #endif
>  
>  typedef void QEMUResetHandler(void *opaque);
> +typedef uint64_t QEMUResetOrder;
> +#define default_reset_order 0x0
>  
> +void qemu_register_reset_common(QEMUResetHandler *func, void *opaque,
> +                                QEMUResetOrder reset_order);
>  void qemu_register_reset(QEMUResetHandler *func, void *opaque);
>  void qemu_unregister_reset(QEMUResetHandler *func, void *opaque);
>  
> diff --git a/vl.c b/vl.c
> index 69ad90c..b205a9b 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1589,6 +1589,7 @@ typedef struct QEMUResetEntry {
>      QTAILQ_ENTRY(QEMUResetEntry) entry;
>      QEMUResetHandler *func;
>      void *opaque;
> +    QEMUResetOrder reset_order;
>  } QEMUResetEntry;
>  
>  static QTAILQ_HEAD(reset_handlers, QEMUResetEntry) reset_handlers =
> @@ -1672,15 +1673,30 @@ static int qemu_debug_requested(void)
>      return r;
>  }
>  
> -void qemu_register_reset(QEMUResetHandler *func, void *opaque)
> +void qemu_register_reset_common(QEMUResetHandler *func, void *opaque,
> +                                QEMUResetOrder reset_order)
>  {
> +    QEMUResetEntry *item;
>      QEMUResetEntry *re = g_malloc0(sizeof(QEMUResetEntry));
>  
>      re->func = func;
>      re->opaque = opaque;
> +    re->reset_order = reset_order;
> +
> +    QTAILQ_FOREACH(item, &reset_handlers, entry) {
> +        if (re->reset_order >= item->reset_order)
> +            continue;
> +        QTAILQ_INSERT_BEFORE(item, re, entry);
> +        return;
> +    }
>      QTAILQ_INSERT_TAIL(&reset_handlers, re, entry);
>  }
>  
> +void qemu_register_reset(QEMUResetHandler *func, void *opaque)
> +{
> +    qemu_register_reset_common(func, opaque, default_reset_order);
> +}
> +
>  void qemu_unregister_reset(QEMUResetHandler *func, void *opaque)
>  {
>      QEMUResetEntry *re;
> 

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

[Paolo Bonzini]

On 25/06/2015 18:10, Andreas Färber wrote:
> Am 25.06.2015 um 18:02 schrieb Paolo Bonzini:
>> On 25/06/2015 18:00, Andreas Färber wrote:
>>>> -    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;
>>>> -    }
>>>> +
>>>> +    root = address_space_root_memory_region(cpu->as);
>>>> +    memory_region_add_subregion_overlap(root,
>>>> +                                        s->apicbase & MSR_IA32_APICBASE_BASE,
>>>> +                                        &s->io_memory,
>>>> +                                        0x1000);
>>>>  
>>
>> I had already remarked that this patch is wrong.  cpu->as is completely
>> unused on KVM, for example.
> 
> Ah, then I don't understand this [RESEND]. ;)
> Either way, not on my plate ATM, it seems.
> 
> Did you also outline how it is supposed to be done instead?

I said "I think this patch is incorrect, because you do not install a
separate address space for each CPU.  Also, the CPU address space is
only used with TCG so it should be guarded by "if (tcg_enabled())"."

By the way, now TCG _is_ installing a separate address space per CPU
already, so the patch can simply guard the code with "if (tcg_enabled())".

Paolo

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

[Andreas Färber]

Am 25.06.2015 um 19:02 schrieb Paolo Bonzini:
> On 25/06/2015 18:10, Andreas Färber wrote:
>> Am 25.06.2015 um 18:02 schrieb Paolo Bonzini:
>>> On 25/06/2015 18:00, Andreas Färber wrote:
>>>>> -    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;
>>>>> -    }
>>>>> +
>>>>> +    root = address_space_root_memory_region(cpu->as);
>>>>> +    memory_region_add_subregion_overlap(root,
>>>>> +                                        s->apicbase & MSR_IA32_APICBASE_BASE,
>>>>> +                                        &s->io_memory,
>>>>> +                                        0x1000);
>>>>>  
>>>
>>> I had already remarked that this patch is wrong.  cpu->as is completely
>>> unused on KVM, for example.
>>
>> Ah, then I don't understand this [RESEND]. ;)
>> Either way, not on my plate ATM, it seems.
>>
>> Did you also outline how it is supposed to be done instead?
> 
> I said "I think this patch is incorrect, because you do not install a
> separate address space for each CPU.  Also, the CPU address space is
> only used with TCG so it should be guarded by "if (tcg_enabled())"."
> 
> By the way, now TCG _is_ installing a separate address space per CPU
> already, so the patch can simply guard the code with "if (tcg_enabled())".

Is the APIC MemoryRegion not used by KVM? Otherwise if we still need the
ugly code path for KVM, that's not much of an improvement here.

And is installing a separate address space per CPU for KVM difficult due
to kernel limitations, or is this just a few lines of QEMU code that Zhu
or someone would need to write? :)

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)

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

[Paolo Bonzini]

On 25/06/2015 19:08, Andreas Färber wrote:
> > I said "I think this patch is incorrect, because you do not install a
> > separate address space for each CPU.  Also, the CPU address space is
> > only used with TCG so it should be guarded by "if (tcg_enabled())"."
> > 
> > By the way, now TCG _is_ installing a separate address space per CPU
> > already, so the patch can simply guard the code with "if (tcg_enabled())".
> 
> Is the APIC MemoryRegion not used by KVM?

It's used if the userspace LAPIC is.  It's not used together with the
kernel LAPIC (accesses really are trapped by the kernel).

> Otherwise if we still need the
> ugly code path for KVM, that's not much of an improvement here.
> 
> And is installing a separate address space per CPU for KVM difficult due
> to kernel limitations, or is this just a few lines of QEMU code that Zhu
> or someone would need to write? :)

It's basically impossible.  Even though support for multiple address
spaces is going to be in Linux 4.2, there are going to be just two: SMM
and not SMM.  You don't really want to do O(#cpus) stuff in KVM, where
the number of CPUs can be 200 or more.

TCG is okay because the #cpus is not really going to be more than 4-ish.

Paolo

Re: [Qemu-devel] [RESEND PATCH v8 2/4] hw: add a wrapper for registering reset handler

[Andreas Färber]

Am 25.06.2015 um 19:00 schrieb Paolo Bonzini:
> On 25/06/2015 04:17, Zhu Guihua wrote:
>> Add a wrapper to specify reset order when registering reset handler,
>> instead of non-obvious initiazation code ordering.
>>
>> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> 
> I'm sorry, this is not really acceptable.  The initialization code
> ordering is good because it should be okay to run reset handlers in the
> same order as code is run.  If there are dependencies between reset
> handlers, a random integer is not a maintainable way to maintain them.
> 
> Instead, you should have a single reset handler that calls the reset
> handlers in the right order; for example a qdev bus such as icc_bus
> always resets children before parents.
> 
> Are you sure that you want to remove the icc_bus?... What are you
> gaining exactly by doing so?

>From my view we would be gaining by making the APIC an integral part
(child<>) of the CPU in a follow-up step (there's a TODO to make things
link<>s).

But either way the CPU's existing reset handler should be able to handle
CPU/APIC interdependencies just fine, somehow. Which is what Eduardo
said on v6 and v7. (Another alternative he raised was a machine init
notifier, but I see no code for that after its mention on v7?)

Cheers,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)

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

[Peter Maydell]

On 25 June 2015 at 18:27, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 25/06/2015 19:08, Andreas Färber wrote:
>> And is installing a separate address space per CPU for KVM difficult due
>> to kernel limitations, or is this just a few lines of QEMU code that Zhu
>> or someone would need to write? :)
>
> It's basically impossible.  Even though support for multiple address
> spaces is going to be in Linux 4.2, there are going to be just two: SMM
> and not SMM.  You don't really want to do O(#cpus) stuff in KVM, where
> the number of CPUs can be 200 or more.

Can you explain what the issue is here? Shouldn't it just be a matter
of kvm_cpu_exec() doing a dispatch to cpu->as rather than calling
address_space_rw() ?  (Making it do that was one of the things on my
todo list for ARM at some point.)

I'm happy to assume that RAM is shared by all CPUs I guess.

> TCG is okay because the #cpus is not really going to be more than 4-ish.

Well, it might be more than that in future...

-- PMM

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

[Paolo Bonzini]

On 25/06/2015 19:32, Peter Maydell wrote:
> On 25 June 2015 at 18:27, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 25/06/2015 19:08, Andreas Färber wrote:
>>> And is installing a separate address space per CPU for KVM difficult due
>>> to kernel limitations, or is this just a few lines of QEMU code that Zhu
>>> or someone would need to write? :)
>>
>> It's basically impossible.  Even though support for multiple address
>> spaces is going to be in Linux 4.2, there are going to be just two: SMM
>> and not SMM.  You don't really want to do O(#cpus) stuff in KVM, where
>> the number of CPUs can be 200 or more.
> 
> Can you explain what the issue is here? Shouldn't it just be a matter
> of kvm_cpu_exec() doing a dispatch to cpu->as rather than calling
> address_space_rw() ?  (Making it do that was one of the things on my
> todo list for ARM at some point.)

One example of the problem is that different CPU address spaces can have
MMIO in different places.  These MMIO areas can hide RAM depending on
where they're placed and their relative priorities.  If they do, KVM
cannot really assume that a single set of page tables are okay to
convert gpa->hpa for all guest CPUs.

If you can tie this to CPU state (e.g. in or out of system management
mode), you only get a small, constant number of such address spaces.

See http://thread.gmane.org/gmane.comp.emulators.qemu/345230 for the
QEMU part of the multiple-address-space support.

Paolo

> I'm happy to assume that RAM is shared by all CPUs I guess.
> 
>> TCG is okay because the #cpus is not really going to be more than 4-ish.
> 
> Well, it might be more than that in future...
> 
> -- PMM
> 

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

[Igor Mammedov]

On Thu, 25 Jun 2015 19:27:47 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> 
> 
> On 25/06/2015 19:08, Andreas Färber wrote:
> > > I said "I think this patch is incorrect, because you do not install a
> > > separate address space for each CPU.  Also, the CPU address space is
> > > only used with TCG so it should be guarded by "if (tcg_enabled())"."
> > > 
> > > By the way, now TCG _is_ installing a separate address space per CPU
> > > already, so the patch can simply guard the code with "if (tcg_enabled())".
> > 
> > Is the APIC MemoryRegion not used by KVM?
> 
> It's used if the userspace LAPIC is.  It's not used together with the
> kernel LAPIC (accesses really are trapped by the kernel).
Isn't this memory region still handle MSI MMIO in case of kernel LAPIC?
kvm_apic_mem_write() -> kvm_irqchip_send_msi()

> 
> > Otherwise if we still need the
> > ugly code path for KVM, that's not much of an improvement here.
> > 
> > And is installing a separate address space per CPU for KVM difficult due
> > to kernel limitations, or is this just a few lines of QEMU code that Zhu
> > or someone would need to write? :)
> 
> It's basically impossible.  Even though support for multiple address
> spaces is going to be in Linux 4.2, there are going to be just two: SMM
> and not SMM.  You don't really want to do O(#cpus) stuff in KVM, where
> the number of CPUs can be 200 or more.
> 
> TCG is okay because the #cpus is not really going to be more than 4-ish.
> 
> Paolo

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

[Paolo Bonzini]

On 26/06/2015 11:01, Igor Mammedov wrote:
> > It's used if the userspace LAPIC is.  It's not used together with the
> > kernel LAPIC (accesses really are trapped by the kernel).
>
> Isn't this memory region still handle MSI MMIO in case of kernel LAPIC?
> kvm_apic_mem_write() -> kvm_irqchip_send_msi()

I'm not sure if the MSI MMIO area moves together with the APIC base.  I
think it doesn't.

Paolo

Re: [Qemu-devel] [RESEND PATCH v8 2/4] hw: add a wrapper for registering reset handler

[Igor Mammedov]

On Thu, 25 Jun 2015 19:28:51 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 25.06.2015 um 19:00 schrieb Paolo Bonzini:
> > On 25/06/2015 04:17, Zhu Guihua wrote:
> >> Add a wrapper to specify reset order when registering reset handler,
> >> instead of non-obvious initiazation code ordering.
> >>
> >> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> > 
> > I'm sorry, this is not really acceptable.  The initialization code
> > ordering is good because it should be okay to run reset handlers in the
> > same order as code is run.  If there are dependencies between reset
> > handlers, a random integer is not a maintainable way to maintain them.
> > 
> > Instead, you should have a single reset handler that calls the reset
> > handlers in the right order; for example a qdev bus such as icc_bus
> > always resets children before parents.
> > 
> > Are you sure that you want to remove the icc_bus?... What are you
> > gaining exactly by doing so?
> 
> From my view we would be gaining by making the APIC an integral part
> (child<>) of the CPU in a follow-up step (there's a TODO to make things
> link<>s).
That's one of the reasons, I've asked to get rid of icc-bus.

Another reason is to move default APIC MMIO mapping to CPU from board
and handle region remapping from CPU itself as it's supposed to be if CPU
would program another APIC base.

Paolo,
would following change be acceptable:

x86_cpu_realize() {
   if (tcg) {
      memory_region_add_subregion_overlap(cpu->as, apic->mr)
   } else {
      if (map_once)
          memory_region_add_subregion_overlap(get_system_memory(), apic->mr)
   }
}
still a hack but a localized to CPU

> 
> But either way the CPU's existing reset handler should be able to handle
> CPU/APIC interdependencies just fine, somehow. Which is what Eduardo
> said on v6 and v7. (Another alternative he raised was a machine init
> notifier, but I see no code for that after its mention on v7?)
> 
> Cheers,
> Andreas
> 

Re: [Qemu-devel] [RESEND PATCH v8 2/4] hw: add a wrapper for registering reset handler

[Paolo Bonzini]

On 26/06/2015 11:19, Igor Mammedov wrote:
> That's one of the reasons, I've asked to get rid of icc-bus.
> 
> Another reason is to move default APIC MMIO mapping to CPU from board
> and handle region remapping from CPU itself as it's supposed to be if CPU
> would program another APIC base.
> 
> Paolo,
> would following change be acceptable:
> 
> x86_cpu_realize() {
>    if (tcg) {
>       memory_region_add_subregion_overlap(cpu->as, apic->mr)
>    } else {
>       if (map_once)
>           memory_region_add_subregion_overlap(get_system_memory(), apic->mr)
>    }
> }
> still a hack but a localized to CPU

Yes, this is better.

Paolo

Re: [Qemu-devel] [RESEND PATCH v8 2/4] hw: add a wrapper for registering reset handler

[Zhu Guihua]

On 06/26/2015 01:28 AM, Andreas Färber wrote:
> Am 25.06.2015 um 19:00 schrieb Paolo Bonzini:
>> On 25/06/2015 04:17, Zhu Guihua wrote:
>>> Add a wrapper to specify reset order when registering reset handler,
>>> instead of non-obvious initiazation code ordering.
>>>
>>> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
>> I'm sorry, this is not really acceptable.  The initialization code
>> ordering is good because it should be okay to run reset handlers in the
>> same order as code is run.  If there are dependencies between reset
>> handlers, a random integer is not a maintainable way to maintain them.
>>
>> Instead, you should have a single reset handler that calls the reset
>> handlers in the right order; for example a qdev bus such as icc_bus
>> always resets children before parents.
>>
>> Are you sure that you want to remove the icc_bus?... What are you
>> gaining exactly by doing so?
> >From my view we would be gaining by making the APIC an integral part
> (child<>) of the CPU in a follow-up step (there's a TODO to make things
> link<>s).
>
> But either way the CPU's existing reset handler should be able to handle
> CPU/APIC interdependencies just fine, somehow. Which is what Eduardo
> said on v6 and v7. (Another alternative he raised was a machine init
> notifier, but I see no code for that after its mention on v7?)

According to Eduardo's suggestions on v7, the simpler way is to add a 
ordering parameter
to qemu_register_reset(), so that we can ensure the order of apic reset 
handler(apic reset
must be after the other devices' reset on x86).

This way will  not influence the initialization code ordering expect 
apic reset.
Can we take this way? or someone have a better one?

Thanks,
Zhu

>
> Cheers,
> Andreas
>

Re: [Qemu-devel] [RESEND PATCH v8 2/4] hw: add a wrapper for registering reset handler

[Igor Mammedov]

On Tue, 30 Jun 2015 14:31:50 +0800
Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:

> 
> On 06/26/2015 01:28 AM, Andreas Färber wrote:
> > Am 25.06.2015 um 19:00 schrieb Paolo Bonzini:
> >> On 25/06/2015 04:17, Zhu Guihua wrote:
> >>> Add a wrapper to specify reset order when registering reset handler,
> >>> instead of non-obvious initiazation code ordering.
> >>>
> >>> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> >> I'm sorry, this is not really acceptable.  The initialization code
> >> ordering is good because it should be okay to run reset handlers in the
> >> same order as code is run.  If there are dependencies between reset
> >> handlers, a random integer is not a maintainable way to maintain them.
> >>
> >> Instead, you should have a single reset handler that calls the reset
> >> handlers in the right order; for example a qdev bus such as icc_bus
> >> always resets children before parents.
> >>
> >> Are you sure that you want to remove the icc_bus?... What are you
> >> gaining exactly by doing so?
> > >From my view we would be gaining by making the APIC an integral part
> > (child<>) of the CPU in a follow-up step (there's a TODO to make things
> > link<>s).
> >
> > But either way the CPU's existing reset handler should be able to handle
> > CPU/APIC interdependencies just fine, somehow. Which is what Eduardo
> > said on v6 and v7. (Another alternative he raised was a machine init
> > notifier, but I see no code for that after its mention on v7?)
> 
> According to Eduardo's suggestions on v7, the simpler way is to add a 
> ordering parameter
> to qemu_register_reset(), so that we can ensure the order of apic reset 
> handler(apic reset
> must be after the other devices' reset on x86).
> 
> This way will  not influence the initialization code ordering expect 
> apic reset.
> Can we take this way? or someone have a better one?
could you explain once more why apic->reset() doesn't work
when it's called from cpu->reset(), please?

> 
> Thanks,
> Zhu
> 
> >
> > Cheers,
> > Andreas
> >
> 

Re: [Qemu-devel] [RESEND PATCH v8 2/4] hw: add a wrapper for registering reset handler

[Andreas Färber]

Am 30.06.2015 um 08:31 schrieb Zhu Guihua:
> On 06/26/2015 01:28 AM, Andreas Färber wrote:
>> Am 25.06.2015 um 19:00 schrieb Paolo Bonzini:
>>> On 25/06/2015 04:17, Zhu Guihua wrote:
>>>> Add a wrapper to specify reset order when registering reset handler,
>>>> instead of non-obvious initiazation code ordering.
>>>>
>>>> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
>>> I'm sorry, this is not really acceptable.  The initialization code
>>> ordering is good because it should be okay to run reset handlers in the
>>> same order as code is run.  If there are dependencies between reset
>>> handlers, a random integer is not a maintainable way to maintain them.
>>>
>>> Instead, you should have a single reset handler that calls the reset
>>> handlers in the right order; for example a qdev bus such as icc_bus
>>> always resets children before parents.
>>>
>>> Are you sure that you want to remove the icc_bus?... What are you
>>> gaining exactly by doing so?
>> >From my view we would be gaining by making the APIC an integral part
>> (child<>) of the CPU in a follow-up step (there's a TODO to make things
>> link<>s).
>>
>> But either way the CPU's existing reset handler should be able to handle
>> CPU/APIC interdependencies just fine, somehow. Which is what Eduardo
>> said on v6 and v7. (Another alternative he raised was a machine init
>> notifier, but I see no code for that after its mention on v7?)
> 
> According to Eduardo's suggestions on v7, the simpler way is to add a
> ordering parameter
> to qemu_register_reset(), so that we can ensure the order of apic reset
> handler(apic reset
> must be after the other devices' reset on x86).

That is a very general statement. Surely the APIC does not need to be
reset after *all* other devices but after some particular device(s).
Which one is that if not the X86CPU?

> This way will  not influence the initialization code ordering expect
> apic reset.

And exactly that's the criticism: You're introducing a generic mechanism
to address a single very specific problem.

sPAPR already has the MachineClass::reset() callback to order CPU vs.
device reset. So if you want a new mechanism you'll need to explain in
detail why that is needed and not just say that it solves your issue.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)

Re: [Qemu-devel] [RESEND PATCH v8 2/4] hw: add a wrapper for registering reset handler

[Zhu Guihua]

On 06/30/2015 05:21 PM, Igor Mammedov wrote:
> On Tue, 30 Jun 2015 14:31:50 +0800
> Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:
>
>> On 06/26/2015 01:28 AM, Andreas Färber wrote:
>>> Am 25.06.2015 um 19:00 schrieb Paolo Bonzini:
>>>> On 25/06/2015 04:17, Zhu Guihua wrote:
>>>>> Add a wrapper to specify reset order when registering reset handler,
>>>>> instead of non-obvious initiazation code ordering.
>>>>>
>>>>> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
>>>> I'm sorry, this is not really acceptable.  The initialization code
>>>> ordering is good because it should be okay to run reset handlers in the
>>>> same order as code is run.  If there are dependencies between reset
>>>> handlers, a random integer is not a maintainable way to maintain them.
>>>>
>>>> Instead, you should have a single reset handler that calls the reset
>>>> handlers in the right order; for example a qdev bus such as icc_bus
>>>> always resets children before parents.
>>>>
>>>> Are you sure that you want to remove the icc_bus?... What are you
>>>> gaining exactly by doing so?
>>> >From my view we would be gaining by making the APIC an integral part
>>> (child<>) of the CPU in a follow-up step (there's a TODO to make things
>>> link<>s).
>>>
>>> But either way the CPU's existing reset handler should be able to handle
>>> CPU/APIC interdependencies just fine, somehow. Which is what Eduardo
>>> said on v6 and v7. (Another alternative he raised was a machine init
>>> notifier, but I see no code for that after its mention on v7?)
>> According to Eduardo's suggestions on v7, the simpler way is to add a
>> ordering parameter
>> to qemu_register_reset(), so that we can ensure the order of apic reset
>> handler(apic reset
>> must be after the other devices' reset on x86).
>>
>> This way will  not influence the initialization code ordering expect
>> apic reset.
>> Can we take this way? or someone have a better one?
> could you explain once more why apic->reset() doesn't work
> when it's called from cpu->reset(), please?

Originally, there are some devices (such as hpet, rtc) reset before apic 
reset.
When these devices reset, they would send irq to apic.
As apic reset is behind these devices reset, the apic register could be 
set to
default values.

If apic->reset() is called from cpu->reset(),  cpu reset is before some 
devices
reset, it lead to apic reset is before them too, so the apic register 
could not be set
to default values.
But before guest boots up, the irq request should be rejected. So when linux
enables local apic, it would find there are irr requests, then it will 
cause the
following warn_on.

  [    1.073487] ------------[ cut here ]------------
[    1.074019] WARNING: at arch/x86/kernel/apic/apic.c:1401 
setup_local_APIC+0x268/0x320()
[    1.075011] Modules linked in:
[    1.076474] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.10.0.sort+ #100
[    1.077012] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), 
BIOS rel-1.8.1-0-g4adadbd-20150316_085822-nilsson.home.kraxel.org 
04/01/2014
[    1.078011]  0000000000000000 00000000d1b49dbb ffff88007c787da8 
ffffffff81649983
[    1.082011]  ffff88007c787de0 ffffffff810b3241 0000000000000001 
0000000000000000
[    1.085012]  00000000000000f0 0000000000000000 00000000ffffffff 
ffff88007c787df0
[    1.088012] Call Trace:
[    1.089019]  [<ffffffff81649983>] dump_stack+0x19/0x1b
[    1.090017]  [<ffffffff810b3241>] warn_slowpath_common+0x61/0x80
[    1.091015]  [<ffffffff810b336a>] warn_slowpath_null+0x1a/0x20
[    1.092016]  [<ffffffff81089ae8>] setup_local_APIC+0x268/0x320
[    1.093019]  [<ffffffff81ad4f02>] native_smp_prepare_cpus+0x294/0x35b
[    1.094018]  [<ffffffff81ac1133>] kernel_init_freeable+0xbb/0x217
[    1.095017]  [<ffffffff81636fe0>] ? rest_init+0x80/0x80
[    1.096015]  [<ffffffff81636fee>] kernel_init+0xe/0x180
[    1.097016]  [<ffffffff816598fc>] ret_from_fork+0x7c/0xb0
[    1.098016]  [<ffffffff81636fe0>] ? rest_init+0x80/0x80
[    1.099017] ---[ end trace d99eba50bffa17c5 ]---

>
>> Thanks,
>> Zhu
>>
>>> Cheers,
>>> Andreas
>>>
> .
>

Re: [Qemu-devel] [RESEND PATCH v8 2/4] hw: add a wrapper for registering reset handler

[Peter Maydell]

On 30 June 2015 at 11:50, Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:
> Originally, there are some devices (such as hpet, rtc) reset before apic
> reset.
> When these devices reset, they would send irq to apic.

In our current (arguably broken) reset design, this is a bug in
those devices -- they shouldn't be raising interrupts from their
reset functions.

-- PMM

Re: [Qemu-devel] [RESEND PATCH v8 2/4] hw: add a wrapper for registering reset handler

[Eduardo Habkost]

On Tue, Jun 30, 2015 at 12:24:22PM +0200, Andreas Färber wrote:
> Am 30.06.2015 um 08:31 schrieb Zhu Guihua:
> > On 06/26/2015 01:28 AM, Andreas Färber wrote:
> >> Am 25.06.2015 um 19:00 schrieb Paolo Bonzini:
> >>> On 25/06/2015 04:17, Zhu Guihua wrote:
> >>>> Add a wrapper to specify reset order when registering reset handler,
> >>>> instead of non-obvious initiazation code ordering.
> >>>>
> >>>> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> >>> I'm sorry, this is not really acceptable.  The initialization code
> >>> ordering is good because it should be okay to run reset handlers in the
> >>> same order as code is run.  If there are dependencies between reset
> >>> handlers, a random integer is not a maintainable way to maintain them.
> >>>
> >>> Instead, you should have a single reset handler that calls the reset
> >>> handlers in the right order; for example a qdev bus such as icc_bus
> >>> always resets children before parents.
> >>>
> >>> Are you sure that you want to remove the icc_bus?... What are you
> >>> gaining exactly by doing so?
> >> >From my view we would be gaining by making the APIC an integral part
> >> (child<>) of the CPU in a follow-up step (there's a TODO to make things
> >> link<>s).
> >>
> >> But either way the CPU's existing reset handler should be able to handle
> >> CPU/APIC interdependencies just fine, somehow. Which is what Eduardo
> >> said on v6 and v7. (Another alternative he raised was a machine init
> >> notifier, but I see no code for that after its mention on v7?)
> > 
> > According to Eduardo's suggestions on v7, the simpler way is to add a
> > ordering parameter
> > to qemu_register_reset(), so that we can ensure the order of apic reset
> > handler(apic reset
> > must be after the other devices' reset on x86).
> 
> That is a very general statement. Surely the APIC does not need to be
> reset after *all* other devices but after some particular device(s).
> Which one is that if not the X86CPU?
> 
> > This way will  not influence the initialization code ordering expect
> > apic reset.
> 
> And exactly that's the criticism: You're introducing a generic mechanism
> to address a single very specific problem.
> 
> sPAPR already has the MachineClass::reset() callback to order CPU vs.
> device reset. So if you want a new mechanism you'll need to explain in
> detail why that is needed and not just say that it solves your issue.

My main point was that relying on a specific ordering of
qemu_register_reset() calls to ensure reset ordering was too fragile.
Adding an ordering argument to qemu_register_reset() was a suggestion,
but now I agree it is unnecessary. Having a reset handler that calls
reset code in the right order (like Paolo proposed earlier in this
thread) looks much simpler.

-- 
Eduardo

Re: [Qemu-devel] [RESEND PATCH v8 2/4] hw: add a wrapper for registering reset handler

[Eduardo Habkost]

On Tue, Jun 30, 2015 at 11:55:59AM +0100, Peter Maydell wrote:
> On 30 June 2015 at 11:50, Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:
> > Originally, there are some devices (such as hpet, rtc) reset before apic
> > reset.
> > When these devices reset, they would send irq to apic.
> 
> In our current (arguably broken) reset design, this is a bug in
> those devices -- they shouldn't be raising interrupts from their
> reset functions.

Do we know exactly which devices are currently broken and need to be
fixed? Should we add a warning to the interrupt code to detect those
cases?

It looks like we won't need a solution to ensure reset ordering if we
fix the devices (or at least the solution would be just temporary until
we fix them).

-- 
Eduardo