[Qemu-devel] [PATCH v2 0/2] remove icc bus/bridge

[Qemu-devel] [PATCH v2 0/2] remove icc bus/bridge

[Chen Fan]

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

this codes has passed the new pc-cpu-test.

Chen Fan (2):
  cpu/apic: drop icc bus/bridge/
  icc_bus: remove icc related files

 default-configs/i386-softmmu.mak   |   1 -
 default-configs/x86_64-softmmu.mak |   1 -
 hw/cpu/Makefile.objs               |   2 -
 hw/cpu/icc_bus.c                   | 118 -------------------------------------
 hw/i386/pc.c                       |  29 ++++-----
 hw/i386/pc_piix.c                  |   9 +--
 hw/i386/pc_q35.c                   |   9 +--
 hw/intc/apic.c                     |   6 +-
 hw/intc/apic_common.c              |  11 +---
 include/hw/cpu/icc_bus.h           |  82 --------------------------
 include/hw/i386/apic_internal.h    |   5 +-
 include/hw/i386/pc.h               |   2 +-
 target-i386/cpu.c                  |   2 -
 13 files changed, 21 insertions(+), 256 deletions(-)
 delete mode 100644 hw/cpu/icc_bus.c
 delete mode 100644 include/hw/cpu/icc_bus.h

-- 
1.9.3

[Qemu-devel] [PATCH v2 1/2] cpu/apic: drop icc bus/bridge/

[Chen Fan]

ICC bus was invented only to provide hotplug capability to
CPU and APIC because at the time being hotplug was available only for
BUS attached devices.

Now this patch is to drop ICC bus impl, and switch to bus-less
CPU+APIC hotplug, handling them in the same manner as pc-dimm.

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
 hw/i386/pc.c                    | 29 +++++++++++------------------
 hw/i386/pc_piix.c               |  9 +--------
 hw/i386/pc_q35.c                |  9 +--------
 hw/intc/apic.c                  |  6 +++---
 hw/intc/apic_common.c           | 11 ++---------
 include/hw/i386/apic_internal.h |  5 ++---
 include/hw/i386/pc.h            |  2 +-
 target-i386/cpu.c               |  2 --
 8 files changed, 21 insertions(+), 52 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 4b46c29..5d15473 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -58,7 +58,6 @@
 #include "qemu/config-file.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"
@@ -66,6 +65,7 @@
 #include "trace.h"
 #include "qapi/visitor.h"
 #include "qapi-visit.h"
+#include "hw/i386/apic_internal.h"
 
 /* debug PC/ISA interrupts */
 //#define DEBUG_IRQ
@@ -991,24 +991,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_unref(OBJECT(cpu));
-
     object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
     object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
 
@@ -1025,7 +1017,6 @@ static const char *current_cpu_model;
 
 void pc_hot_add_cpu(const int64_t id, Error **errp)
 {
-    DeviceState *icc_bridge;
     int64_t apic_id = x86_cpu_apic_id_from_index(id);
 
     if (id < 0) {
@@ -1052,17 +1043,16 @@ 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));
-    pc_new_cpu(current_cpu_model, apic_id, icc_bridge, errp);
+    pc_new_cpu(current_cpu_model, apic_id, errp);
 }
 
-void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
+void pc_cpus_init(const char *cpu_model)
 {
     int i;
     X86CPU *cpu = NULL;
     Error *error = NULL;
     unsigned long apic_id_limit;
+    APICCommonState *apic = NULL;
 
     /* init CPUs */
     if (cpu_model == NULL) {
@@ -1083,7 +1073,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);
@@ -1093,8 +1083,11 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
     /* 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);
+        apic = APIC_COMMON(cpu->apic_state);
+        memory_region_add_subregion_overlap(CPU(cpu)->as->root,
+                                            APIC_DEFAULT_ADDRESS,
+                                            &apic->io_memory,
+                                            0x1000);
     }
 
     /* tell smbios about cpuid version and features */
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 36c69d7..5c51dfe 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;
     FWCfgState *fw_cfg = NULL;
     PcGuestInfo *guest_info;
     ram_addr_t lowmem;
@@ -143,11 +141,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();
@@ -230,7 +224,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 bc40537..e98bfbc 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"
 
 /* ICH9 AHCI has 6 ports */
@@ -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];
@@ -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();
@@ -240,7 +234,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.c b/hw/intc/apic.c
index 0f97b47..00ae0ec 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -376,7 +376,7 @@ static void apic_update_irq(APICCommonState *s)
         cpu_interrupt(cpu, CPU_INTERRUPT_POLL);
     } else if (apic_irq_pending(s) > 0) {
         cpu_interrupt(cpu, CPU_INTERRUPT_HARD);
-    } else if (!apic_accept_pic_intr(&s->busdev.qdev) || !pic_get_output(isa_pic)) {
+    } else if (!apic_accept_pic_intr(DEVICE(s)) || !pic_get_output(isa_pic)) {
         cpu_reset_interrupt(cpu, CPU_INTERRUPT_HARD);
     }
 }
@@ -549,10 +549,10 @@ static void apic_deliver(DeviceState *dev, uint8_t dest, uint8_t dest_mode,
 
 static bool apic_check_pic(APICCommonState *s)
 {
-    if (!apic_accept_pic_intr(&s->busdev.qdev) || !pic_get_output(isa_pic)) {
+    if (!apic_accept_pic_intr(DEVICE(s)) || !pic_get_output(isa_pic)) {
         return false;
     }
-    apic_deliver_pic_intr(&s->busdev.qdev, 1);
+    apic_deliver_pic_intr(DEVICE(s), 1);
     return true;
 }
 
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 0858b45..9cc6bda 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -302,7 +302,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.",
@@ -313,11 +312,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 &&
@@ -433,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()
@@ -449,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..6eeceff 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,7 @@ typedef struct APICCommonClass
 } APICCommonClass;
 
 struct APICCommonState {
-    ICCDevice busdev;
+    DeviceState dev;
 
     MemoryRegion io_memory;
     X86CPU *cpu;
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 1b35168..7c9d044 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -163,7 +163,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 f01690b..2385e6b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -42,7 +42,6 @@
 
 #include "sysemu/sysemu.h"
 #include "hw/qdev-properties.h"
-#include "hw/cpu/icc_bus.h"
 #ifndef CONFIG_USER_ONLY
 #include "hw/xen/xen.h"
 #include "hw/i386/apic_internal.h"
@@ -2941,7 +2940,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] [PATCH v2 2/2] icc_bus: remove icc related files

[Chen Fan]

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

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
 default-configs/i386-softmmu.mak   |   1 -
 default-configs/x86_64-softmmu.mak |   1 -
 hw/cpu/Makefile.objs               |   2 -
 hw/cpu/icc_bus.c                   | 118 -------------------------------------
 include/hw/cpu/icc_bus.h           |  82 --------------------------
 5 files changed, 204 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 6a74e00..3ac6324 100644
--- a/default-configs/i386-softmmu.mak
+++ b/default-configs/i386-softmmu.mak
@@ -39,7 +39,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 46b87dd..5c89ea8 100644
--- a/default-configs/x86_64-softmmu.mak
+++ b/default-configs/x86_64-softmmu.mak
@@ -39,7 +39,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..193e489 100644
--- a/hw/cpu/Makefile.objs
+++ b/hw/cpu/Makefile.objs
@@ -2,5 +2,3 @@ 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] [PATCH v2 1/2] cpu/apic: drop icc bus/bridge/

[Igor Mammedov]

On Mon, 23 Mar 2015 13:54:23 +0800
Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:

> ICC bus was invented only to provide hotplug capability to
> CPU and APIC because at the time being hotplug was available only for
> BUS attached devices.
> 
> Now this patch is to drop ICC bus impl, and switch to bus-less
> CPU+APIC hotplug, handling them in the same manner as pc-dimm.
> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
>  hw/i386/pc.c                    | 29 +++++++++++------------------
>  hw/i386/pc_piix.c               |  9 +--------
>  hw/i386/pc_q35.c                |  9 +--------
>  hw/intc/apic.c                  |  6 +++---
>  hw/intc/apic_common.c           | 11 ++---------
>  include/hw/i386/apic_internal.h |  5 ++---
>  include/hw/i386/pc.h            |  2 +-
>  target-i386/cpu.c               |  2 --
>  8 files changed, 21 insertions(+), 52 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 4b46c29..5d15473 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
[...]

> @@ -1093,8 +1083,11 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
>      /* 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);
> +        apic = APIC_COMMON(cpu->apic_state);
> +        memory_region_add_subregion_overlap(CPU(cpu)->as->root,
> +                                            APIC_DEFAULT_ADDRESS,
> +                                            &apic->io_memory,
> +                                            0x1000);
Why is it here?
Shouldn't it be mapped not once but for each CPU since we are using
per CPU address spaces?

Split this change out into a separate patch please, with commit message
describing what it does.

PS:
It should be part of APIC code or at worst case part of CPU's realize.

PS2:
new cpu tests don't test actual CPU execution, so they can't validate
this change. To test it you need to run test in TCG (at least) or
TCG + KVM mode, with some guest code that programs and checks APIC
of each CPU.

PS3:
the rest of the patch I'd suggest to merge with 2/2 patch that
removes unused icc_bridge code, there isn't point in splitting
that from removing icc_bridge from other files.

[...]
>  
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index f01690b..2385e6b 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -42,7 +42,6 @@
>  
>  #include "sysemu/sysemu.h"
>  #include "hw/qdev-properties.h"
> -#include "hw/cpu/icc_bus.h"
>  #ifndef CONFIG_USER_ONLY
>  #include "hw/xen/xen.h"
>  #include "hw/i386/apic_internal.h"
> @@ -2941,7 +2940,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;
that isn't the only place in this file that should be changed.

See x86_cpu_apic_create():
  cpu->apic_state = qdev_try_create(qdev_get_parent_bus(dev), apic_type);

probably it's not right to try get parent bus from bus-less device,
qdev_try_create() call should be replaced by object_new()/object_unref() pair.

>      dc->props = x86_cpu_properties;
>  
>      xcc->parent_reset = cc->reset;

Re: [Qemu-devel] [PATCH v2 1/2] cpu/apic: drop icc bus/bridge/

[Chen Fan]

On 03/23/2015 04:23 PM, Igor Mammedov wrote:
> On Mon, 23 Mar 2015 13:54:23 +0800
> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
>
>> ICC bus was invented only to provide hotplug capability to
>> CPU and APIC because at the time being hotplug was available only for
>> BUS attached devices.
>>
>> Now this patch is to drop ICC bus impl, and switch to bus-less
>> CPU+APIC hotplug, handling them in the same manner as pc-dimm.
>>
>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>> ---
>>   hw/i386/pc.c                    | 29 +++++++++++------------------
>>   hw/i386/pc_piix.c               |  9 +--------
>>   hw/i386/pc_q35.c                |  9 +--------
>>   hw/intc/apic.c                  |  6 +++---
>>   hw/intc/apic_common.c           | 11 ++---------
>>   include/hw/i386/apic_internal.h |  5 ++---
>>   include/hw/i386/pc.h            |  2 +-
>>   target-i386/cpu.c               |  2 --
>>   8 files changed, 21 insertions(+), 52 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 4b46c29..5d15473 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
> [...]
>
>> @@ -1093,8 +1083,11 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
>>       /* 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);
>> +        apic = APIC_COMMON(cpu->apic_state);
>> +        memory_region_add_subregion_overlap(CPU(cpu)->as->root,
>> +                                            APIC_DEFAULT_ADDRESS,
>> +                                            &apic->io_memory,
>> +                                            0x1000);
> Why is it here?
> Shouldn't it be mapped not once but for each CPU since we are using
> per CPU address spaces?
>
> Split this change out into a separate patch please, with commit message
> describing what it does.
Hi Igor,

     in your previous mail said, "It might be that kvm_irqchip don't 
need it at all."
I don't know why kvm_irqchip don't need it ?
because I have test that for kernel_irqchip=on, qemu emulator the 
kvm-apic object,
and sent the MSI to kernel irqchip for pcie devices. it also need map 
the region.

Thanks,
Chen


>
> PS:
> It should be part of APIC code or at worst case part of CPU's realize.
>
> PS2:
> new cpu tests don't test actual CPU execution, so they can't validate
> this change. To test it you need to run test in TCG (at least) or
> TCG + KVM mode, with some guest code that programs and checks APIC
> of each CPU.
>
> PS3:
> the rest of the patch I'd suggest to merge with 2/2 patch that
> removes unused icc_bridge code, there isn't point in splitting
> that from removing icc_bridge from other files.
>
> [...]
>>   
>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>> index f01690b..2385e6b 100644
>> --- a/target-i386/cpu.c
>> +++ b/target-i386/cpu.c
>> @@ -42,7 +42,6 @@
>>   
>>   #include "sysemu/sysemu.h"
>>   #include "hw/qdev-properties.h"
>> -#include "hw/cpu/icc_bus.h"
>>   #ifndef CONFIG_USER_ONLY
>>   #include "hw/xen/xen.h"
>>   #include "hw/i386/apic_internal.h"
>> @@ -2941,7 +2940,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;
> that isn't the only place in this file that should be changed.
>
> See x86_cpu_apic_create():
>    cpu->apic_state = qdev_try_create(qdev_get_parent_bus(dev), apic_type);
>
> probably it's not right to try get parent bus from bus-less device,
> qdev_try_create() call should be replaced by object_new()/object_unref() pair.
>
>>       dc->props = x86_cpu_properties;
>>   
>>       xcc->parent_reset = cc->reset;
> .
>

Re: [Qemu-devel] [PATCH v2 1/2] cpu/apic: drop icc bus/bridge/

[Chen Fan]

On 03/23/2015 05:43 PM, Igor Mammedov wrote:
> On Mon, 23 Mar 2015 17:07:29 +0800
> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
>
>> On 03/23/2015 04:23 PM, Igor Mammedov wrote:
>>> On Mon, 23 Mar 2015 13:54:23 +0800
>>> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
>>>
>>>> ICC bus was invented only to provide hotplug capability to
>>>> CPU and APIC because at the time being hotplug was available only for
>>>> BUS attached devices.
>>>>
>>>> Now this patch is to drop ICC bus impl, and switch to bus-less
>>>> CPU+APIC hotplug, handling them in the same manner as pc-dimm.
>>>>
>>>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>>>> ---
>>>>    hw/i386/pc.c                    | 29 +++++++++++------------------
>>>>    hw/i386/pc_piix.c               |  9 +--------
>>>>    hw/i386/pc_q35.c                |  9 +--------
>>>>    hw/intc/apic.c                  |  6 +++---
>>>>    hw/intc/apic_common.c           | 11 ++---------
>>>>    include/hw/i386/apic_internal.h |  5 ++---
>>>>    include/hw/i386/pc.h            |  2 +-
>>>>    target-i386/cpu.c               |  2 --
>>>>    8 files changed, 21 insertions(+), 52 deletions(-)
>>>>
>>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>>> index 4b46c29..5d15473 100644
>>>> --- a/hw/i386/pc.c
>>>> +++ b/hw/i386/pc.c
>>> [...]
>>>
>>>> @@ -1093,8 +1083,11 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
>>>>        /* 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);
>>>> +        apic = APIC_COMMON(cpu->apic_state);
>>>> +        memory_region_add_subregion_overlap(CPU(cpu)->as->root,
>>>> +                                            APIC_DEFAULT_ADDRESS,
>>>> +                                            &apic->io_memory,
>>>> +                                            0x1000);
>>> Why is it here?
>>> Shouldn't it be mapped not once but for each CPU since we are using
>>> per CPU address spaces?
>>>
>>> Split this change out into a separate patch please, with commit message
>>> describing what it does.
>> Hi Igor,
>>
>>       in your previous mail said, "It might be that kvm_irqchip don't
>> need it at all."
>> I don't know why kvm_irqchip don't need it ?
> That's why it's MIGHT, I'm not sure since I've not look at that code for a while.
>
>> because I have test that for kernel_irqchip=on, qemu emulator the
>> kvm-apic object,
>> and sent the MSI to kernel irqchip for pcie devices. it also need map
>> the region.
> Can we have this test as a patch to qemu/tests? so it would be easier to
> discuss it.
no problem.

Thanks,
Chen

>
>> Thanks,
>> Chen
>>
>>
>>> PS:
>>> It should be part of APIC code or at worst case part of CPU's realize.
>>>
>>> PS2:
>>> new cpu tests don't test actual CPU execution, so they can't validate
>>> this change. To test it you need to run test in TCG (at least) or
>>> TCG + KVM mode, with some guest code that programs and checks APIC
>>> of each CPU.
>>>
>>> PS3:
>>> the rest of the patch I'd suggest to merge with 2/2 patch that
>>> removes unused icc_bridge code, there isn't point in splitting
>>> that from removing icc_bridge from other files.
>>>
>>> [...]
>>>>    
>>>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>>>> index f01690b..2385e6b 100644
>>>> --- a/target-i386/cpu.c
>>>> +++ b/target-i386/cpu.c
>>>> @@ -42,7 +42,6 @@
>>>>    
>>>>    #include "sysemu/sysemu.h"
>>>>    #include "hw/qdev-properties.h"
>>>> -#include "hw/cpu/icc_bus.h"
>>>>    #ifndef CONFIG_USER_ONLY
>>>>    #include "hw/xen/xen.h"
>>>>    #include "hw/i386/apic_internal.h"
>>>> @@ -2941,7 +2940,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;
>>> that isn't the only place in this file that should be changed.
>>>
>>> See x86_cpu_apic_create():
>>>     cpu->apic_state = qdev_try_create(qdev_get_parent_bus(dev), apic_type);
>>>
>>> probably it's not right to try get parent bus from bus-less device,
>>> qdev_try_create() call should be replaced by object_new()/object_unref() pair.
>>>
>>>>        dc->props = x86_cpu_properties;
>>>>    
>>>>        xcc->parent_reset = cc->reset;
>>> .
>>>
> .
>

Re: [Qemu-devel] [PATCH v2 1/2] cpu/apic: drop icc bus/bridge/

[Igor Mammedov]

On Mon, 23 Mar 2015 17:07:29 +0800
Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:

> 
> On 03/23/2015 04:23 PM, Igor Mammedov wrote:
> > On Mon, 23 Mar 2015 13:54:23 +0800
> > Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> >
> >> ICC bus was invented only to provide hotplug capability to
> >> CPU and APIC because at the time being hotplug was available only for
> >> BUS attached devices.
> >>
> >> Now this patch is to drop ICC bus impl, and switch to bus-less
> >> CPU+APIC hotplug, handling them in the same manner as pc-dimm.
> >>
> >> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> >> ---
> >>   hw/i386/pc.c                    | 29 +++++++++++------------------
> >>   hw/i386/pc_piix.c               |  9 +--------
> >>   hw/i386/pc_q35.c                |  9 +--------
> >>   hw/intc/apic.c                  |  6 +++---
> >>   hw/intc/apic_common.c           | 11 ++---------
> >>   include/hw/i386/apic_internal.h |  5 ++---
> >>   include/hw/i386/pc.h            |  2 +-
> >>   target-i386/cpu.c               |  2 --
> >>   8 files changed, 21 insertions(+), 52 deletions(-)
> >>
> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >> index 4b46c29..5d15473 100644
> >> --- a/hw/i386/pc.c
> >> +++ b/hw/i386/pc.c
> > [...]
> >
> >> @@ -1093,8 +1083,11 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
> >>       /* 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);
> >> +        apic = APIC_COMMON(cpu->apic_state);
> >> +        memory_region_add_subregion_overlap(CPU(cpu)->as->root,
> >> +                                            APIC_DEFAULT_ADDRESS,
> >> +                                            &apic->io_memory,
> >> +                                            0x1000);
> > Why is it here?
> > Shouldn't it be mapped not once but for each CPU since we are using
> > per CPU address spaces?
> >
> > Split this change out into a separate patch please, with commit message
> > describing what it does.
> Hi Igor,
> 
>      in your previous mail said, "It might be that kvm_irqchip don't 
> need it at all."
> I don't know why kvm_irqchip don't need it ?
That's why it's MIGHT, I'm not sure since I've not look at that code for a while.

> because I have test that for kernel_irqchip=on, qemu emulator the 
> kvm-apic object,
> and sent the MSI to kernel irqchip for pcie devices. it also need map 
> the region.
Can we have this test as a patch to qemu/tests? so it would be easier to
discuss it.

> 
> Thanks,
> Chen
> 
> 
> >
> > PS:
> > It should be part of APIC code or at worst case part of CPU's realize.
> >
> > PS2:
> > new cpu tests don't test actual CPU execution, so they can't validate
> > this change. To test it you need to run test in TCG (at least) or
> > TCG + KVM mode, with some guest code that programs and checks APIC
> > of each CPU.
> >
> > PS3:
> > the rest of the patch I'd suggest to merge with 2/2 patch that
> > removes unused icc_bridge code, there isn't point in splitting
> > that from removing icc_bridge from other files.
> >
> > [...]
> >>   
> >> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> >> index f01690b..2385e6b 100644
> >> --- a/target-i386/cpu.c
> >> +++ b/target-i386/cpu.c
> >> @@ -42,7 +42,6 @@
> >>   
> >>   #include "sysemu/sysemu.h"
> >>   #include "hw/qdev-properties.h"
> >> -#include "hw/cpu/icc_bus.h"
> >>   #ifndef CONFIG_USER_ONLY
> >>   #include "hw/xen/xen.h"
> >>   #include "hw/i386/apic_internal.h"
> >> @@ -2941,7 +2940,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;
> > that isn't the only place in this file that should be changed.
> >
> > See x86_cpu_apic_create():
> >    cpu->apic_state = qdev_try_create(qdev_get_parent_bus(dev), apic_type);
> >
> > probably it's not right to try get parent bus from bus-less device,
> > qdev_try_create() call should be replaced by object_new()/object_unref() pair.
> >
> >>       dc->props = x86_cpu_properties;
> >>   
> >>       xcc->parent_reset = cc->reset;
> > .
> >
> 

Re: [Qemu-devel] [PATCH v2 1/2] cpu/apic: drop icc bus/bridge/

[Chen Fan]

On 03/23/2015 05:43 PM, Igor Mammedov wrote:
> On Mon, 23 Mar 2015 17:07:29 +0800
> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
>
>> On 03/23/2015 04:23 PM, Igor Mammedov wrote:
>>> On Mon, 23 Mar 2015 13:54:23 +0800
>>> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
>>>
>>>> ICC bus was invented only to provide hotplug capability to
>>>> CPU and APIC because at the time being hotplug was available only for
>>>> BUS attached devices.
>>>>
>>>> Now this patch is to drop ICC bus impl, and switch to bus-less
>>>> CPU+APIC hotplug, handling them in the same manner as pc-dimm.
>>>>
>>>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>>>> ---
>>>>    hw/i386/pc.c                    | 29 +++++++++++------------------
>>>>    hw/i386/pc_piix.c               |  9 +--------
>>>>    hw/i386/pc_q35.c                |  9 +--------
>>>>    hw/intc/apic.c                  |  6 +++---
>>>>    hw/intc/apic_common.c           | 11 ++---------
>>>>    include/hw/i386/apic_internal.h |  5 ++---
>>>>    include/hw/i386/pc.h            |  2 +-
>>>>    target-i386/cpu.c               |  2 --
>>>>    8 files changed, 21 insertions(+), 52 deletions(-)
>>>>
>>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>>> index 4b46c29..5d15473 100644
>>>> --- a/hw/i386/pc.c
>>>> +++ b/hw/i386/pc.c
>>> [...]
>>>
>>>> @@ -1093,8 +1083,11 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
>>>>        /* 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);
>>>> +        apic = APIC_COMMON(cpu->apic_state);
>>>> +        memory_region_add_subregion_overlap(CPU(cpu)->as->root,
>>>> +                                            APIC_DEFAULT_ADDRESS,
>>>> +                                            &apic->io_memory,
>>>> +                                            0x1000);
>>> Why is it here?
>>> Shouldn't it be mapped not once but for each CPU since we are using
>>> per CPU address spaces?
>>>
>>> Split this change out into a separate patch please, with commit message
>>> describing what it does.
>> Hi Igor,
>>
>>       in your previous mail said, "It might be that kvm_irqchip don't
>> need it at all."
>> I don't know why kvm_irqchip don't need it ?
> That's why it's MIGHT, I'm not sure since I've not look at that code for a while.
>
>> because I have test that for kernel_irqchip=on, qemu emulator the
>> kvm-apic object,
>> and sent the MSI to kernel irqchip for pcie devices. it also need map
>> the region.
> Can we have this test as a patch to qemu/tests? so it would be easier to
> discuss it.
kernel_irqchip is only used for kvm acc, do qtest can use kvm accel?

I used GDB to intercept the kvm_apic_mem_write(), we could find
that:

#0  kvm_apic_mem_write (opaque=0x55555652ddb0, addr=0, data=16465, 
size=4) at /home/chenfan/data/qemu-latest/hw/i386/kvm/apic.c:157
#1  0x000055555565c871 in memory_region_write_accessor 
(mr=0x55555652de28, addr=0, value=0x7fffe5027538, size=4, shift=0, 
mask=4294967295)
     at /home/chenfan/data/qemu-latest/memory.c:430
#2  0x000055555565c9b9 in access_with_adjusted_size (addr=0, 
value=0x7fffe5027538, size=4, access_size_min=1, access_size_max=4, access=
     0x55555565c7d9 <memory_region_write_accessor>, mr=0x55555652de28) 
at /home/chenfan/data/qemu-latest/memory.c:467
#3  0x000055555565f9d1 in memory_region_dispatch_write 
(mr=0x55555652de28, addr=0, data=16465, size=4)
     at /home/chenfan/data/qemu-latest/memory.c:1103
#4  0x000055555566356e in io_mem_write (mr=0x55555652de28, addr=0, 
val=16465, size=4) at /home/chenfan/data/qemu-latest/memory.c:2003
#5  0x00005555556060f2 in stl_phys_internal (as=0x5555577568a8, 
addr=4276092928, val=16465, endian=DEVICE_LITTLE_ENDIAN)
#6  0x000055555560621e in stl_le_phys (as=0x5555577568a8, 
addr=4276092928, val=16465) at /home/chenfan/data/qemu-latest/exec.c:2920
#7  0x000055555587d35e in *msi_notify* (dev=0x5555577566a0, vector=0) at 
hw/pci/msi.c:294
#8  0x0000555555836f77 in ahci_irq_raise (s=0x555557756f20, dev=0x0) at 
hw/ide/ahci.c:134
#9  0x00005555558370f2 in ahci_check_irq (s=0x555557756f20) at 
hw/ide/ahci.c:169
#10 0x000055555583733a in ahci_port_write (s=0x555557756f20, port=0, 
offset=20, val=2017460351) at hw/ide/ahci.c:225
#11 0x0000555555837811 in ahci_mem_write (opaque=0x555557756f20, 
addr=276, val=2017460351, size=4) at hw/ide/ahci.c:382
#12 0x000055555565c871 in memory_region_write_accessor 
(mr=0x555557756f40, addr=276, value=0x7fffe50278b8, size=4, shift=0, 
mask=4294967295)
     at /home/chenfan/data/qemu-latest/memory.c:430
#13 0x000055555565c9b9 in access_with_adjusted_size (addr=276, 
value=0x7fffe50278b8, size=4, access_size_min=1, access_size_max=4, access=
     0x55555565c7d9 <memory_region_write_accessor>, mr=0x555557756f40) 
at /home/chenfan/data/qemu-latest/memory.c:467
#14 0x000055555565f9d1 in memory_region_dispatch_write 
(mr=0x555557756f40, addr=276, data=2017460351, size=4)
     at /home/chenfan/data/qemu-latest/memory.c:1103
#15 0x000055555566356e in io_mem_write (mr=0x555557756f40, addr=276, 
val=2017460351, size=4) at /home/chenfan/data/qemu-latest/memory.c:2003



Thanks,
Chen


>> Thanks,
>> Chen
>>
>>
>>> PS:
>>> It should be part of APIC code or at worst case part of CPU's realize.
>>>
>>> PS2:
>>> new cpu tests don't test actual CPU execution, so they can't validate
>>> this change. To test it you need to run test in TCG (at least) or
>>> TCG + KVM mode, with some guest code that programs and checks APIC
>>> of each CPU.
>>>
>>> PS3:
>>> the rest of the patch I'd suggest to merge with 2/2 patch that
>>> removes unused icc_bridge code, there isn't point in splitting
>>> that from removing icc_bridge from other files.
>>>
>>> [...]
>>>>    
>>>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>>>> index f01690b..2385e6b 100644
>>>> --- a/target-i386/cpu.c
>>>> +++ b/target-i386/cpu.c
>>>> @@ -42,7 +42,6 @@
>>>>    
>>>>    #include "sysemu/sysemu.h"
>>>>    #include "hw/qdev-properties.h"
>>>> -#include "hw/cpu/icc_bus.h"
>>>>    #ifndef CONFIG_USER_ONLY
>>>>    #include "hw/xen/xen.h"
>>>>    #include "hw/i386/apic_internal.h"
>>>> @@ -2941,7 +2940,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;
>>> that isn't the only place in this file that should be changed.
>>>
>>> See x86_cpu_apic_create():
>>>     cpu->apic_state = qdev_try_create(qdev_get_parent_bus(dev), apic_type);
>>>
>>> probably it's not right to try get parent bus from bus-less device,
>>> qdev_try_create() call should be replaced by object_new()/object_unref() pair.
>>>
>>>>        dc->props = x86_cpu_properties;
>>>>    
>>>>        xcc->parent_reset = cc->reset;
>>> .
>>>
> .
>

Re: [Qemu-devel] [PATCH v2 1/2] cpu/apic: drop icc bus/bridge/

[Igor Mammedov]

On Mon, 30 Mar 2015 18:12:06 +0800
Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:

> 
> On 03/23/2015 05:43 PM, Igor Mammedov wrote:
> > On Mon, 23 Mar 2015 17:07:29 +0800
> > Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> >
> >> On 03/23/2015 04:23 PM, Igor Mammedov wrote:
> >>> On Mon, 23 Mar 2015 13:54:23 +0800
> >>> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> >>>
> >>>> ICC bus was invented only to provide hotplug capability to
> >>>> CPU and APIC because at the time being hotplug was available only for
> >>>> BUS attached devices.
> >>>>
> >>>> Now this patch is to drop ICC bus impl, and switch to bus-less
> >>>> CPU+APIC hotplug, handling them in the same manner as pc-dimm.
> >>>>
> >>>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> >>>> ---
> >>>>    hw/i386/pc.c                    | 29 +++++++++++------------------
> >>>>    hw/i386/pc_piix.c               |  9 +--------
> >>>>    hw/i386/pc_q35.c                |  9 +--------
> >>>>    hw/intc/apic.c                  |  6 +++---
> >>>>    hw/intc/apic_common.c           | 11 ++---------
> >>>>    include/hw/i386/apic_internal.h |  5 ++---
> >>>>    include/hw/i386/pc.h            |  2 +-
> >>>>    target-i386/cpu.c               |  2 --
> >>>>    8 files changed, 21 insertions(+), 52 deletions(-)
> >>>>
> >>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >>>> index 4b46c29..5d15473 100644
> >>>> --- a/hw/i386/pc.c
> >>>> +++ b/hw/i386/pc.c
> >>> [...]
> >>>
> >>>> @@ -1093,8 +1083,11 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
> >>>>        /* 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);
> >>>> +        apic = APIC_COMMON(cpu->apic_state);
> >>>> +        memory_region_add_subregion_overlap(CPU(cpu)->as->root,
> >>>> +                                            APIC_DEFAULT_ADDRESS,
> >>>> +                                            &apic->io_memory,
> >>>> +                                            0x1000);
> >>> Why is it here?
> >>> Shouldn't it be mapped not once but for each CPU since we are using
> >>> per CPU address spaces?
> >>>
> >>> Split this change out into a separate patch please, with commit message
> >>> describing what it does.
> >> Hi Igor,
> >>
> >>       in your previous mail said, "It might be that kvm_irqchip don't
> >> need it at all."
> >> I don't know why kvm_irqchip don't need it ?
> > That's why it's MIGHT, I'm not sure since I've not look at that code for a while.
> >
> >> because I have test that for kernel_irqchip=on, qemu emulator the
> >> kvm-apic object,
> >> and sent the MSI to kernel irqchip for pcie devices. it also need map
> >> the region.
> > Can we have this test as a patch to qemu/tests? so it would be easier to
> > discuss it.
> kernel_irqchip is only used for kvm acc, do qtest can use kvm accel?
As far as I know it can't by I might be wrong.

I'd suggest execute tests with accel=tcg and conditionally with 
accel=kvm if KVM on host is available and accessible to make check user. 

> 
> I used GDB to intercept the kvm_apic_mem_write(), we could find
> that:
> 
> #0  kvm_apic_mem_write (opaque=0x55555652ddb0, addr=0, data=16465, 
> size=4) at /home/chenfan/data/qemu-latest/hw/i386/kvm/apic.c:157
> #1  0x000055555565c871 in memory_region_write_accessor 
> (mr=0x55555652de28, addr=0, value=0x7fffe5027538, size=4, shift=0, 
> mask=4294967295)
>      at /home/chenfan/data/qemu-latest/memory.c:430
> #2  0x000055555565c9b9 in access_with_adjusted_size (addr=0, 
> value=0x7fffe5027538, size=4, access_size_min=1, access_size_max=4, access=
>      0x55555565c7d9 <memory_region_write_accessor>, mr=0x55555652de28) 
> at /home/chenfan/data/qemu-latest/memory.c:467
> #3  0x000055555565f9d1 in memory_region_dispatch_write 
> (mr=0x55555652de28, addr=0, data=16465, size=4)
>      at /home/chenfan/data/qemu-latest/memory.c:1103
> #4  0x000055555566356e in io_mem_write (mr=0x55555652de28, addr=0, 
> val=16465, size=4) at /home/chenfan/data/qemu-latest/memory.c:2003
> #5  0x00005555556060f2 in stl_phys_internal (as=0x5555577568a8, 
> addr=4276092928, val=16465, endian=DEVICE_LITTLE_ENDIAN)
> #6  0x000055555560621e in stl_le_phys (as=0x5555577568a8, 
> addr=4276092928, val=16465) at /home/chenfan/data/qemu-latest/exec.c:2920
> #7  0x000055555587d35e in *msi_notify* (dev=0x5555577566a0, vector=0) at 
> hw/pci/msi.c:294
> #8  0x0000555555836f77 in ahci_irq_raise (s=0x555557756f20, dev=0x0) at 
> hw/ide/ahci.c:134
> #9  0x00005555558370f2 in ahci_check_irq (s=0x555557756f20) at 
> hw/ide/ahci.c:169
> #10 0x000055555583733a in ahci_port_write (s=0x555557756f20, port=0, 
> offset=20, val=2017460351) at hw/ide/ahci.c:225
> #11 0x0000555555837811 in ahci_mem_write (opaque=0x555557756f20, 
> addr=276, val=2017460351, size=4) at hw/ide/ahci.c:382
> #12 0x000055555565c871 in memory_region_write_accessor 
> (mr=0x555557756f40, addr=276, value=0x7fffe50278b8, size=4, shift=0, 
> mask=4294967295)
>      at /home/chenfan/data/qemu-latest/memory.c:430
> #13 0x000055555565c9b9 in access_with_adjusted_size (addr=276, 
> value=0x7fffe50278b8, size=4, access_size_min=1, access_size_max=4, access=
>      0x55555565c7d9 <memory_region_write_accessor>, mr=0x555557756f40) 
> at /home/chenfan/data/qemu-latest/memory.c:467
> #14 0x000055555565f9d1 in memory_region_dispatch_write 
> (mr=0x555557756f40, addr=276, data=2017460351, size=4)
>      at /home/chenfan/data/qemu-latest/memory.c:1103
> #15 0x000055555566356e in io_mem_write (mr=0x555557756f40, addr=276, 
> val=2017460351, size=4) at /home/chenfan/data/qemu-latest/memory.c:2003
> 
> 
> 
> Thanks,
> Chen
> 
> 
> >> Thanks,
> >> Chen
> >>
> >>
> >>> PS:
> >>> It should be part of APIC code or at worst case part of CPU's realize.
> >>>
> >>> PS2:
> >>> new cpu tests don't test actual CPU execution, so they can't validate
> >>> this change. To test it you need to run test in TCG (at least) or
> >>> TCG + KVM mode, with some guest code that programs and checks APIC
> >>> of each CPU.
> >>>
> >>> PS3:
> >>> the rest of the patch I'd suggest to merge with 2/2 patch that
> >>> removes unused icc_bridge code, there isn't point in splitting
> >>> that from removing icc_bridge from other files.
> >>>
> >>> [...]
> >>>>    
> >>>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> >>>> index f01690b..2385e6b 100644
> >>>> --- a/target-i386/cpu.c
> >>>> +++ b/target-i386/cpu.c
> >>>> @@ -42,7 +42,6 @@
> >>>>    
> >>>>    #include "sysemu/sysemu.h"
> >>>>    #include "hw/qdev-properties.h"
> >>>> -#include "hw/cpu/icc_bus.h"
> >>>>    #ifndef CONFIG_USER_ONLY
> >>>>    #include "hw/xen/xen.h"
> >>>>    #include "hw/i386/apic_internal.h"
> >>>> @@ -2941,7 +2940,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;
> >>> that isn't the only place in this file that should be changed.
> >>>
> >>> See x86_cpu_apic_create():
> >>>     cpu->apic_state = qdev_try_create(qdev_get_parent_bus(dev), apic_type);
> >>>
> >>> probably it's not right to try get parent bus from bus-less device,
> >>> qdev_try_create() call should be replaced by object_new()/object_unref() pair.
> >>>
> >>>>        dc->props = x86_cpu_properties;
> >>>>    
> >>>>        xcc->parent_reset = cc->reset;
> >>> .
> >>>
> > .
> >
> 
> 

Re: [Qemu-devel] [PATCH v2 1/2] cpu/apic: drop icc bus/bridge/

[Chen Fan]

On 03/30/2015 08:45 PM, Igor Mammedov wrote:
> On Mon, 30 Mar 2015 18:12:06 +0800
> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
>
>> On 03/23/2015 05:43 PM, Igor Mammedov wrote:
>>> On Mon, 23 Mar 2015 17:07:29 +0800
>>> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
>>>
>>>> On 03/23/2015 04:23 PM, Igor Mammedov wrote:
>>>>> On Mon, 23 Mar 2015 13:54:23 +0800
>>>>> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
>>>>>
>>>>>> ICC bus was invented only to provide hotplug capability to
>>>>>> CPU and APIC because at the time being hotplug was available only for
>>>>>> BUS attached devices.
>>>>>>
>>>>>> Now this patch is to drop ICC bus impl, and switch to bus-less
>>>>>> CPU+APIC hotplug, handling them in the same manner as pc-dimm.
>>>>>>
>>>>>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>>>>>> ---
>>>>>>     hw/i386/pc.c                    | 29 +++++++++++------------------
>>>>>>     hw/i386/pc_piix.c               |  9 +--------
>>>>>>     hw/i386/pc_q35.c                |  9 +--------
>>>>>>     hw/intc/apic.c                  |  6 +++---
>>>>>>     hw/intc/apic_common.c           | 11 ++---------
>>>>>>     include/hw/i386/apic_internal.h |  5 ++---
>>>>>>     include/hw/i386/pc.h            |  2 +-
>>>>>>     target-i386/cpu.c               |  2 --
>>>>>>     8 files changed, 21 insertions(+), 52 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>>>>> index 4b46c29..5d15473 100644
>>>>>> --- a/hw/i386/pc.c
>>>>>> +++ b/hw/i386/pc.c
>>>>> [...]
>>>>>
>>>>>> @@ -1093,8 +1083,11 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
>>>>>>         /* 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);
>>>>>> +        apic = APIC_COMMON(cpu->apic_state);
>>>>>> +        memory_region_add_subregion_overlap(CPU(cpu)->as->root,
>>>>>> +                                            APIC_DEFAULT_ADDRESS,
>>>>>> +                                            &apic->io_memory,
>>>>>> +                                            0x1000);
>>>>> Why is it here?
>>>>> Shouldn't it be mapped not once but for each CPU since we are using
>>>>> per CPU address spaces?
>>>>>
>>>>> Split this change out into a separate patch please, with commit message
>>>>> describing what it does.
>>>> Hi Igor,
>>>>
>>>>        in your previous mail said, "It might be that kvm_irqchip don't
>>>> need it at all."
>>>> I don't know why kvm_irqchip don't need it ?
>>> That's why it's MIGHT, I'm not sure since I've not look at that code for a while.
>>>
>>>> because I have test that for kernel_irqchip=on, qemu emulator the
>>>> kvm-apic object,
>>>> and sent the MSI to kernel irqchip for pcie devices. it also need map
>>>> the region.
>>> Can we have this test as a patch to qemu/tests? so it would be easier to
>>> discuss it.
>> kernel_irqchip is only used for kvm acc, do qtest can use kvm accel?
> As far as I know it can't by I might be wrong.
>
> I'd suggest execute tests with accel=tcg and conditionally with
> accel=kvm if KVM on host is available and accessible to make check user.
I hadn't found a good way to test this case, do you any idea?

Thanks,
Chen

>
>> I used GDB to intercept the kvm_apic_mem_write(), we could find
>> that:
>>
>> #0  kvm_apic_mem_write (opaque=0x55555652ddb0, addr=0, data=16465,
>> size=4) at /home/chenfan/data/qemu-latest/hw/i386/kvm/apic.c:157
>> #1  0x000055555565c871 in memory_region_write_accessor
>> (mr=0x55555652de28, addr=0, value=0x7fffe5027538, size=4, shift=0,
>> mask=4294967295)
>>       at /home/chenfan/data/qemu-latest/memory.c:430
>> #2  0x000055555565c9b9 in access_with_adjusted_size (addr=0,
>> value=0x7fffe5027538, size=4, access_size_min=1, access_size_max=4, access=
>>       0x55555565c7d9 <memory_region_write_accessor>, mr=0x55555652de28)
>> at /home/chenfan/data/qemu-latest/memory.c:467
>> #3  0x000055555565f9d1 in memory_region_dispatch_write
>> (mr=0x55555652de28, addr=0, data=16465, size=4)
>>       at /home/chenfan/data/qemu-latest/memory.c:1103
>> #4  0x000055555566356e in io_mem_write (mr=0x55555652de28, addr=0,
>> val=16465, size=4) at /home/chenfan/data/qemu-latest/memory.c:2003
>> #5  0x00005555556060f2 in stl_phys_internal (as=0x5555577568a8,
>> addr=4276092928, val=16465, endian=DEVICE_LITTLE_ENDIAN)
>> #6  0x000055555560621e in stl_le_phys (as=0x5555577568a8,
>> addr=4276092928, val=16465) at /home/chenfan/data/qemu-latest/exec.c:2920
>> #7  0x000055555587d35e in *msi_notify* (dev=0x5555577566a0, vector=0) at
>> hw/pci/msi.c:294
>> #8  0x0000555555836f77 in ahci_irq_raise (s=0x555557756f20, dev=0x0) at
>> hw/ide/ahci.c:134
>> #9  0x00005555558370f2 in ahci_check_irq (s=0x555557756f20) at
>> hw/ide/ahci.c:169
>> #10 0x000055555583733a in ahci_port_write (s=0x555557756f20, port=0,
>> offset=20, val=2017460351) at hw/ide/ahci.c:225
>> #11 0x0000555555837811 in ahci_mem_write (opaque=0x555557756f20,
>> addr=276, val=2017460351, size=4) at hw/ide/ahci.c:382
>> #12 0x000055555565c871 in memory_region_write_accessor
>> (mr=0x555557756f40, addr=276, value=0x7fffe50278b8, size=4, shift=0,
>> mask=4294967295)
>>       at /home/chenfan/data/qemu-latest/memory.c:430
>> #13 0x000055555565c9b9 in access_with_adjusted_size (addr=276,
>> value=0x7fffe50278b8, size=4, access_size_min=1, access_size_max=4, access=
>>       0x55555565c7d9 <memory_region_write_accessor>, mr=0x555557756f40)
>> at /home/chenfan/data/qemu-latest/memory.c:467
>> #14 0x000055555565f9d1 in memory_region_dispatch_write
>> (mr=0x555557756f40, addr=276, data=2017460351, size=4)
>>       at /home/chenfan/data/qemu-latest/memory.c:1103
>> #15 0x000055555566356e in io_mem_write (mr=0x555557756f40, addr=276,
>> val=2017460351, size=4) at /home/chenfan/data/qemu-latest/memory.c:2003
>>
>>
>>
>> Thanks,
>> Chen
>>
>>
>>>> Thanks,
>>>> Chen
>>>>
>>>>
>>>>> PS:
>>>>> It should be part of APIC code or at worst case part of CPU's realize.
>>>>>
>>>>> PS2:
>>>>> new cpu tests don't test actual CPU execution, so they can't validate
>>>>> this change. To test it you need to run test in TCG (at least) or
>>>>> TCG + KVM mode, with some guest code that programs and checks APIC
>>>>> of each CPU.
>>>>>
>>>>> PS3:
>>>>> the rest of the patch I'd suggest to merge with 2/2 patch that
>>>>> removes unused icc_bridge code, there isn't point in splitting
>>>>> that from removing icc_bridge from other files.
>>>>>
>>>>> [...]
>>>>>>     
>>>>>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>>>>>> index f01690b..2385e6b 100644
>>>>>> --- a/target-i386/cpu.c
>>>>>> +++ b/target-i386/cpu.c
>>>>>> @@ -42,7 +42,6 @@
>>>>>>     
>>>>>>     #include "sysemu/sysemu.h"
>>>>>>     #include "hw/qdev-properties.h"
>>>>>> -#include "hw/cpu/icc_bus.h"
>>>>>>     #ifndef CONFIG_USER_ONLY
>>>>>>     #include "hw/xen/xen.h"
>>>>>>     #include "hw/i386/apic_internal.h"
>>>>>> @@ -2941,7 +2940,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;
>>>>> that isn't the only place in this file that should be changed.
>>>>>
>>>>> See x86_cpu_apic_create():
>>>>>      cpu->apic_state = qdev_try_create(qdev_get_parent_bus(dev), apic_type);
>>>>>
>>>>> probably it's not right to try get parent bus from bus-less device,
>>>>> qdev_try_create() call should be replaced by object_new()/object_unref() pair.
>>>>>
>>>>>>         dc->props = x86_cpu_properties;
>>>>>>     
>>>>>>         xcc->parent_reset = cc->reset;
>>>>> .
>>>>>
>>> .
>>>
>>
> .
>

Re: [Qemu-devel] [PATCH v2 1/2] cpu/apic: drop icc bus/bridge/

[Igor Mammedov]

On Tue, 31 Mar 2015 16:54:43 +0800
Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:

> 
> On 03/30/2015 08:45 PM, Igor Mammedov wrote:
> > On Mon, 30 Mar 2015 18:12:06 +0800
> > Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> >
> >> On 03/23/2015 05:43 PM, Igor Mammedov wrote:
> >>> On Mon, 23 Mar 2015 17:07:29 +0800
> >>> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> >>>
> >>>> On 03/23/2015 04:23 PM, Igor Mammedov wrote:
> >>>>> On Mon, 23 Mar 2015 13:54:23 +0800
> >>>>> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> >>>>>
> >>>>>> ICC bus was invented only to provide hotplug capability to
> >>>>>> CPU and APIC because at the time being hotplug was available
> >>>>>> only for BUS attached devices.
> >>>>>>
> >>>>>> Now this patch is to drop ICC bus impl, and switch to bus-less
> >>>>>> CPU+APIC hotplug, handling them in the same manner as pc-dimm.
> >>>>>>
> >>>>>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> >>>>>> ---
> >>>>>>     hw/i386/pc.c                    | 29
> >>>>>> +++++++++++------------------ hw/i386/pc_piix.c
> >>>>>> |  9 +-------- hw/i386/pc_q35.c                |  9 +--------
> >>>>>>     hw/intc/apic.c                  |  6 +++---
> >>>>>>     hw/intc/apic_common.c           | 11 ++---------
> >>>>>>     include/hw/i386/apic_internal.h |  5 ++---
> >>>>>>     include/hw/i386/pc.h            |  2 +-
> >>>>>>     target-i386/cpu.c               |  2 --
> >>>>>>     8 files changed, 21 insertions(+), 52 deletions(-)
> >>>>>>
> >>>>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >>>>>> index 4b46c29..5d15473 100644
> >>>>>> --- a/hw/i386/pc.c
> >>>>>> +++ b/hw/i386/pc.c
> >>>>> [...]
> >>>>>
> >>>>>> @@ -1093,8 +1083,11 @@ void pc_cpus_init(const char
> >>>>>> *cpu_model, DeviceState *icc_bridge) /* 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);
> >>>>>> +        apic = APIC_COMMON(cpu->apic_state);
> >>>>>> +
> >>>>>> memory_region_add_subregion_overlap(CPU(cpu)->as->root,
> >>>>>> +
> >>>>>> APIC_DEFAULT_ADDRESS,
> >>>>>> +                                            &apic->io_memory,
> >>>>>> +                                            0x1000);
> >>>>> Why is it here?
> >>>>> Shouldn't it be mapped not once but for each CPU since we are
> >>>>> using per CPU address spaces?
> >>>>>
> >>>>> Split this change out into a separate patch please, with commit
> >>>>> message describing what it does.
> >>>> Hi Igor,
> >>>>
> >>>>        in your previous mail said, "It might be that kvm_irqchip
> >>>> don't need it at all."
> >>>> I don't know why kvm_irqchip don't need it ?
> >>> That's why it's MIGHT, I'm not sure since I've not look at that
> >>> code for a while.
> >>>
> >>>> because I have test that for kernel_irqchip=on, qemu emulator the
> >>>> kvm-apic object,
> >>>> and sent the MSI to kernel irqchip for pcie devices. it also
> >>>> need map the region.
> >>> Can we have this test as a patch to qemu/tests? so it would be
> >>> easier to discuss it.
> >> kernel_irqchip is only used for kvm acc, do qtest can use kvm
> >> accel?
> > As far as I know it can't by I might be wrong.
> >
> > I'd suggest execute tests with accel=tcg and conditionally with
> > accel=kvm if KVM on host is available and accessible to make check
> > user.
> I hadn't found a good way to test this case, do you any idea?
maybe check that /dev/kvm exists and accessible to user, if yes than
run additional tests with accel=kvm

> 
> Thanks,
> Chen
> 
> >
> >> I used GDB to intercept the kvm_apic_mem_write(), we could find
> >> that:
> >>
> >> #0  kvm_apic_mem_write (opaque=0x55555652ddb0, addr=0, data=16465,
> >> size=4) at /home/chenfan/data/qemu-latest/hw/i386/kvm/apic.c:157
> >> #1  0x000055555565c871 in memory_region_write_accessor
> >> (mr=0x55555652de28, addr=0, value=0x7fffe5027538, size=4, shift=0,
> >> mask=4294967295)
> >>       at /home/chenfan/data/qemu-latest/memory.c:430
> >> #2  0x000055555565c9b9 in access_with_adjusted_size (addr=0,
> >> value=0x7fffe5027538, size=4, access_size_min=1,
> >> access_size_max=4, access= 0x55555565c7d9
> >> <memory_region_write_accessor>, mr=0x55555652de28)
> >> at /home/chenfan/data/qemu-latest/memory.c:467 #3
> >> 0x000055555565f9d1 in memory_region_dispatch_write
> >> (mr=0x55555652de28, addr=0, data=16465, size=4)
> >> at /home/chenfan/data/qemu-latest/memory.c:1103 #4
> >> 0x000055555566356e in io_mem_write (mr=0x55555652de28, addr=0,
> >> val=16465, size=4) at /home/chenfan/data/qemu-latest/memory.c:2003
> >> #5  0x00005555556060f2 in stl_phys_internal (as=0x5555577568a8,
> >> addr=4276092928, val=16465, endian=DEVICE_LITTLE_ENDIAN) #6
> >> 0x000055555560621e in stl_le_phys (as=0x5555577568a8,
> >> addr=4276092928, val=16465)
> >> at /home/chenfan/data/qemu-latest/exec.c:2920 #7
> >> 0x000055555587d35e in *msi_notify* (dev=0x5555577566a0, vector=0)
> >> at hw/pci/msi.c:294 #8  0x0000555555836f77 in ahci_irq_raise
> >> (s=0x555557756f20, dev=0x0) at hw/ide/ahci.c:134 #9
> >> 0x00005555558370f2 in ahci_check_irq (s=0x555557756f20) at
> >> hw/ide/ahci.c:169 #10 0x000055555583733a in ahci_port_write
> >> (s=0x555557756f20, port=0, offset=20, val=2017460351) at
> >> hw/ide/ahci.c:225 #11 0x0000555555837811 in ahci_mem_write
> >> (opaque=0x555557756f20, addr=276, val=2017460351, size=4) at
> >> hw/ide/ahci.c:382 #12 0x000055555565c871 in
> >> memory_region_write_accessor (mr=0x555557756f40, addr=276,
> >> value=0x7fffe50278b8, size=4, shift=0, mask=4294967295)
> >>       at /home/chenfan/data/qemu-latest/memory.c:430
> >> #13 0x000055555565c9b9 in access_with_adjusted_size (addr=276,
> >> value=0x7fffe50278b8, size=4, access_size_min=1,
> >> access_size_max=4, access= 0x55555565c7d9
> >> <memory_region_write_accessor>, mr=0x555557756f40)
> >> at /home/chenfan/data/qemu-latest/memory.c:467 #14
> >> 0x000055555565f9d1 in memory_region_dispatch_write
> >> (mr=0x555557756f40, addr=276, data=2017460351, size=4)
> >> at /home/chenfan/data/qemu-latest/memory.c:1103 #15
> >> 0x000055555566356e in io_mem_write (mr=0x555557756f40, addr=276,
> >> val=2017460351, size=4)
> >> at /home/chenfan/data/qemu-latest/memory.c:2003
> >>
> >>
> >>
> >> Thanks,
> >> Chen
> >>
> >>
> >>>> Thanks,
> >>>> Chen
> >>>>
> >>>>
> >>>>> PS:
> >>>>> It should be part of APIC code or at worst case part of CPU's
> >>>>> realize.
> >>>>>
> >>>>> PS2:
> >>>>> new cpu tests don't test actual CPU execution, so they can't
> >>>>> validate this change. To test it you need to run test in TCG
> >>>>> (at least) or TCG + KVM mode, with some guest code that
> >>>>> programs and checks APIC of each CPU.
> >>>>>
> >>>>> PS3:
> >>>>> the rest of the patch I'd suggest to merge with 2/2 patch that
> >>>>> removes unused icc_bridge code, there isn't point in splitting
> >>>>> that from removing icc_bridge from other files.
> >>>>>
> >>>>> [...]
> >>>>>>     
> >>>>>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> >>>>>> index f01690b..2385e6b 100644
> >>>>>> --- a/target-i386/cpu.c
> >>>>>> +++ b/target-i386/cpu.c
> >>>>>> @@ -42,7 +42,6 @@
> >>>>>>     
> >>>>>>     #include "sysemu/sysemu.h"
> >>>>>>     #include "hw/qdev-properties.h"
> >>>>>> -#include "hw/cpu/icc_bus.h"
> >>>>>>     #ifndef CONFIG_USER_ONLY
> >>>>>>     #include "hw/xen/xen.h"
> >>>>>>     #include "hw/i386/apic_internal.h"
> >>>>>> @@ -2941,7 +2940,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;
> >>>>> that isn't the only place in this file that should be changed.
> >>>>>
> >>>>> See x86_cpu_apic_create():
> >>>>>      cpu->apic_state =
> >>>>> qdev_try_create(qdev_get_parent_bus(dev), apic_type);
> >>>>>
> >>>>> probably it's not right to try get parent bus from bus-less
> >>>>> device, qdev_try_create() call should be replaced by
> >>>>> object_new()/object_unref() pair.
> >>>>>
> >>>>>>         dc->props = x86_cpu_properties;
> >>>>>>     
> >>>>>>         xcc->parent_reset = cc->reset;
> >>>>> .
> >>>>>
> >>> .
> >>>
> >>
> > .
> >
> 
> 

Re: [Qemu-devel] [PATCH v2 1/2] cpu/apic: drop icc bus/bridge/

[Chen Fan]

On 03/31/2015 05:51 PM, Igor Mammedov wrote:
> On Tue, 31 Mar 2015 16:54:43 +0800
> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
>
>> On 03/30/2015 08:45 PM, Igor Mammedov wrote:
>>> On Mon, 30 Mar 2015 18:12:06 +0800
>>> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
>>>
>>>> On 03/23/2015 05:43 PM, Igor Mammedov wrote:
>>>>> On Mon, 23 Mar 2015 17:07:29 +0800
>>>>> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
>>>>>
>>>>>> On 03/23/2015 04:23 PM, Igor Mammedov wrote:
>>>>>>> On Mon, 23 Mar 2015 13:54:23 +0800
>>>>>>> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
>>>>>>>
>>>>>>>> ICC bus was invented only to provide hotplug capability to
>>>>>>>> CPU and APIC because at the time being hotplug was available
>>>>>>>> only for BUS attached devices.
>>>>>>>>
>>>>>>>> Now this patch is to drop ICC bus impl, and switch to bus-less
>>>>>>>> CPU+APIC hotplug, handling them in the same manner as pc-dimm.
>>>>>>>>
>>>>>>>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>>>>>>>> ---
>>>>>>>>      hw/i386/pc.c                    | 29
>>>>>>>> +++++++++++------------------ hw/i386/pc_piix.c
>>>>>>>> |  9 +-------- hw/i386/pc_q35.c                |  9 +--------
>>>>>>>>      hw/intc/apic.c                  |  6 +++---
>>>>>>>>      hw/intc/apic_common.c           | 11 ++---------
>>>>>>>>      include/hw/i386/apic_internal.h |  5 ++---
>>>>>>>>      include/hw/i386/pc.h            |  2 +-
>>>>>>>>      target-i386/cpu.c               |  2 --
>>>>>>>>      8 files changed, 21 insertions(+), 52 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>>>>>>> index 4b46c29..5d15473 100644
>>>>>>>> --- a/hw/i386/pc.c
>>>>>>>> +++ b/hw/i386/pc.c
>>>>>>> [...]
>>>>>>>
>>>>>>>> @@ -1093,8 +1083,11 @@ void pc_cpus_init(const char
>>>>>>>> *cpu_model, DeviceState *icc_bridge) /* 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);
>>>>>>>> +        apic = APIC_COMMON(cpu->apic_state);
>>>>>>>> +
>>>>>>>> memory_region_add_subregion_overlap(CPU(cpu)->as->root,
>>>>>>>> +
>>>>>>>> APIC_DEFAULT_ADDRESS,
>>>>>>>> +                                            &apic->io_memory,
>>>>>>>> +                                            0x1000);
>>>>>>> Why is it here?
>>>>>>> Shouldn't it be mapped not once but for each CPU since we are
>>>>>>> using per CPU address spaces?
>>>>>>>
>>>>>>> Split this change out into a separate patch please, with commit
>>>>>>> message describing what it does.
>>>>>> Hi Igor,
>>>>>>
>>>>>>         in your previous mail said, "It might be that kvm_irqchip
>>>>>> don't need it at all."
>>>>>> I don't know why kvm_irqchip don't need it ?
>>>>> That's why it's MIGHT, I'm not sure since I've not look at that
>>>>> code for a while.
>>>>>
>>>>>> because I have test that for kernel_irqchip=on, qemu emulator the
>>>>>> kvm-apic object,
>>>>>> and sent the MSI to kernel irqchip for pcie devices. it also
>>>>>> need map the region.
>>>>> Can we have this test as a patch to qemu/tests? so it would be
>>>>> easier to discuss it.
>>>> kernel_irqchip is only used for kvm acc, do qtest can use kvm
>>>> accel?
>>> As far as I know it can't by I might be wrong.
>>>
>>> I'd suggest execute tests with accel=tcg and conditionally with
>>> accel=kvm if KVM on host is available and accessible to make check
>>> user.
>> I hadn't found a good way to test this case, do you any idea?
> maybe check that /dev/kvm exists and accessible to user, if yes than
> run additional tests with accel=kvm
I mean that how to make the tests concretely can check the case
that when kernel_irqchip on also need kvm-apic mapped ?

Thanks,
Chen


>> Thanks,
>> Chen
>>
>>>> I used GDB to intercept the kvm_apic_mem_write(), we could find
>>>> that:
>>>>
>>>> #0  kvm_apic_mem_write (opaque=0x55555652ddb0, addr=0, data=16465,
>>>> size=4) at /home/chenfan/data/qemu-latest/hw/i386/kvm/apic.c:157
>>>> #1  0x000055555565c871 in memory_region_write_accessor
>>>> (mr=0x55555652de28, addr=0, value=0x7fffe5027538, size=4, shift=0,
>>>> mask=4294967295)
>>>>        at /home/chenfan/data/qemu-latest/memory.c:430
>>>> #2  0x000055555565c9b9 in access_with_adjusted_size (addr=0,
>>>> value=0x7fffe5027538, size=4, access_size_min=1,
>>>> access_size_max=4, access= 0x55555565c7d9
>>>> <memory_region_write_accessor>, mr=0x55555652de28)
>>>> at /home/chenfan/data/qemu-latest/memory.c:467 #3
>>>> 0x000055555565f9d1 in memory_region_dispatch_write
>>>> (mr=0x55555652de28, addr=0, data=16465, size=4)
>>>> at /home/chenfan/data/qemu-latest/memory.c:1103 #4
>>>> 0x000055555566356e in io_mem_write (mr=0x55555652de28, addr=0,
>>>> val=16465, size=4) at /home/chenfan/data/qemu-latest/memory.c:2003
>>>> #5  0x00005555556060f2 in stl_phys_internal (as=0x5555577568a8,
>>>> addr=4276092928, val=16465, endian=DEVICE_LITTLE_ENDIAN) #6
>>>> 0x000055555560621e in stl_le_phys (as=0x5555577568a8,
>>>> addr=4276092928, val=16465)
>>>> at /home/chenfan/data/qemu-latest/exec.c:2920 #7
>>>> 0x000055555587d35e in *msi_notify* (dev=0x5555577566a0, vector=0)
>>>> at hw/pci/msi.c:294 #8  0x0000555555836f77 in ahci_irq_raise
>>>> (s=0x555557756f20, dev=0x0) at hw/ide/ahci.c:134 #9
>>>> 0x00005555558370f2 in ahci_check_irq (s=0x555557756f20) at
>>>> hw/ide/ahci.c:169 #10 0x000055555583733a in ahci_port_write
>>>> (s=0x555557756f20, port=0, offset=20, val=2017460351) at
>>>> hw/ide/ahci.c:225 #11 0x0000555555837811 in ahci_mem_write
>>>> (opaque=0x555557756f20, addr=276, val=2017460351, size=4) at
>>>> hw/ide/ahci.c:382 #12 0x000055555565c871 in
>>>> memory_region_write_accessor (mr=0x555557756f40, addr=276,
>>>> value=0x7fffe50278b8, size=4, shift=0, mask=4294967295)
>>>>        at /home/chenfan/data/qemu-latest/memory.c:430
>>>> #13 0x000055555565c9b9 in access_with_adjusted_size (addr=276,
>>>> value=0x7fffe50278b8, size=4, access_size_min=1,
>>>> access_size_max=4, access= 0x55555565c7d9
>>>> <memory_region_write_accessor>, mr=0x555557756f40)
>>>> at /home/chenfan/data/qemu-latest/memory.c:467 #14
>>>> 0x000055555565f9d1 in memory_region_dispatch_write
>>>> (mr=0x555557756f40, addr=276, data=2017460351, size=4)
>>>> at /home/chenfan/data/qemu-latest/memory.c:1103 #15
>>>> 0x000055555566356e in io_mem_write (mr=0x555557756f40, addr=276,
>>>> val=2017460351, size=4)
>>>> at /home/chenfan/data/qemu-latest/memory.c:2003
>>>>
>>>>
>>>>
>>>> Thanks,
>>>> Chen
>>>>
>>>>
>>>>>> Thanks,
>>>>>> Chen
>>>>>>
>>>>>>
>>>>>>> PS:
>>>>>>> It should be part of APIC code or at worst case part of CPU's
>>>>>>> realize.
>>>>>>>
>>>>>>> PS2:
>>>>>>> new cpu tests don't test actual CPU execution, so they can't
>>>>>>> validate this change. To test it you need to run test in TCG
>>>>>>> (at least) or TCG + KVM mode, with some guest code that
>>>>>>> programs and checks APIC of each CPU.
>>>>>>>
>>>>>>> PS3:
>>>>>>> the rest of the patch I'd suggest to merge with 2/2 patch that
>>>>>>> removes unused icc_bridge code, there isn't point in splitting
>>>>>>> that from removing icc_bridge from other files.
>>>>>>>
>>>>>>> [...]
>>>>>>>>      
>>>>>>>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>>>>>>>> index f01690b..2385e6b 100644
>>>>>>>> --- a/target-i386/cpu.c
>>>>>>>> +++ b/target-i386/cpu.c
>>>>>>>> @@ -42,7 +42,6 @@
>>>>>>>>      
>>>>>>>>      #include "sysemu/sysemu.h"
>>>>>>>>      #include "hw/qdev-properties.h"
>>>>>>>> -#include "hw/cpu/icc_bus.h"
>>>>>>>>      #ifndef CONFIG_USER_ONLY
>>>>>>>>      #include "hw/xen/xen.h"
>>>>>>>>      #include "hw/i386/apic_internal.h"
>>>>>>>> @@ -2941,7 +2940,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;
>>>>>>> that isn't the only place in this file that should be changed.
>>>>>>>
>>>>>>> See x86_cpu_apic_create():
>>>>>>>       cpu->apic_state =
>>>>>>> qdev_try_create(qdev_get_parent_bus(dev), apic_type);
>>>>>>>
>>>>>>> probably it's not right to try get parent bus from bus-less
>>>>>>> device, qdev_try_create() call should be replaced by
>>>>>>> object_new()/object_unref() pair.
>>>>>>>
>>>>>>>>          dc->props = x86_cpu_properties;
>>>>>>>>      
>>>>>>>>          xcc->parent_reset = cc->reset;
>>>>>>> .
>>>>>>>
>>>>> .
>>>>>
>>> .
>>>
>>
> .
>

Re: [Qemu-devel] [PATCH v2 1/2] cpu/apic: drop icc bus/bridge/

[Chen Fan]

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


On 04/01/2015 09:40 AM, Chen Fan wrote:
>
> On 03/31/2015 05:51 PM, Igor Mammedov wrote:
>> On Tue, 31 Mar 2015 16:54:43 +0800
>> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
>>
>>> On 03/30/2015 08:45 PM, Igor Mammedov wrote:
>>>> On Mon, 30 Mar 2015 18:12:06 +0800
>>>> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
>>>>
>>>>> On 03/23/2015 05:43 PM, Igor Mammedov wrote:
>>>>>> On Mon, 23 Mar 2015 17:07:29 +0800
>>>>>> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
>>>>>>
>>>>>>> On 03/23/2015 04:23 PM, Igor Mammedov wrote:
>>>>>>>> On Mon, 23 Mar 2015 13:54:23 +0800
>>>>>>>> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
>>>>>>>>
>>>>>>>>> ICC bus was invented only to provide hotplug capability to
>>>>>>>>> CPU and APIC because at the time being hotplug was available
>>>>>>>>> only for BUS attached devices.
>>>>>>>>>
>>>>>>>>> Now this patch is to drop ICC bus impl, and switch to bus-less
>>>>>>>>> CPU+APIC hotplug, handling them in the same manner as pc-dimm.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>>>>>>>>> ---
>>>>>>>>>      hw/i386/pc.c                    | 29
>>>>>>>>> +++++++++++------------------ hw/i386/pc_piix.c
>>>>>>>>> |  9 +-------- hw/i386/pc_q35.c                | 9 +--------
>>>>>>>>>      hw/intc/apic.c                  |  6 +++---
>>>>>>>>>      hw/intc/apic_common.c           | 11 ++---------
>>>>>>>>>      include/hw/i386/apic_internal.h |  5 ++---
>>>>>>>>>      include/hw/i386/pc.h            |  2 +-
>>>>>>>>>      target-i386/cpu.c               |  2 --
>>>>>>>>>      8 files changed, 21 insertions(+), 52 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>>>>>>>> index 4b46c29..5d15473 100644
>>>>>>>>> --- a/hw/i386/pc.c
>>>>>>>>> +++ b/hw/i386/pc.c
>>>>>>>> [...]
>>>>>>>>
>>>>>>>>> @@ -1093,8 +1083,11 @@ void pc_cpus_init(const char
>>>>>>>>> *cpu_model, DeviceState *icc_bridge) /* 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);
>>>>>>>>> +        apic = APIC_COMMON(cpu->apic_state);
>>>>>>>>> +
>>>>>>>>> memory_region_add_subregion_overlap(CPU(cpu)->as->root,
>>>>>>>>> +
>>>>>>>>> APIC_DEFAULT_ADDRESS,
>>>>>>>>> + &apic->io_memory,
>>>>>>>>> + 0x1000);
>>>>>>>> Why is it here?
>>>>>>>> Shouldn't it be mapped not once but for each CPU since we are
>>>>>>>> using per CPU address spaces?
>>>>>>>>
>>>>>>>> Split this change out into a separate patch please, with commit
>>>>>>>> message describing what it does.
>>>>>>> Hi Igor,
>>>>>>>
>>>>>>>         in your previous mail said, "It might be that kvm_irqchip
>>>>>>> don't need it at all."
>>>>>>> I don't know why kvm_irqchip don't need it ?
>>>>>> That's why it's MIGHT, I'm not sure since I've not look at that
>>>>>> code for a while.
>>>>>>
>>>>>>> because I have test that for kernel_irqchip=on, qemu emulator the
>>>>>>> kvm-apic object,
>>>>>>> and sent the MSI to kernel irqchip for pcie devices. it also
>>>>>>> need map the region.
>>>>>> Can we have this test as a patch to qemu/tests? so it would be
>>>>>> easier to discuss it.
>>>>> kernel_irqchip is only used for kvm acc, do qtest can use kvm
>>>>> accel?
>>>> As far as I know it can't by I might be wrong.
>>>>
>>>> I'd suggest execute tests with accel=tcg and conditionally with
>>>> accel=kvm if KVM on host is available and accessible to make check
>>>> user.
>>> I hadn't found a good way to test this case, do you any idea?
>> maybe check that /dev/kvm exists and accessible to user, if yes than
>> run additional tests with accel=kvm
> I mean that how to make the tests concretely can check the case
> that when kernel_irqchip on also need kvm-apic mapped ?
for pcie supported MSI, the MSI Message Adress is usually based on 
0xFEEX-XXXX,
which is the same as APIC mmio base address on x86. so I can think is
to send MSI by write the address space then check whether could 
intercept the msi irq.

>
> Thanks,
> Chen
>
>
>>> Thanks,
>>> Chen
>>>
>>>>> I used GDB to intercept the kvm_apic_mem_write(), we could find
>>>>> that:
>>>>>
>>>>> #0  kvm_apic_mem_write (opaque=0x55555652ddb0, addr=0, data=16465,
>>>>> size=4) at /home/chenfan/data/qemu-latest/hw/i386/kvm/apic.c:157
>>>>> #1  0x000055555565c871 in memory_region_write_accessor
>>>>> (mr=0x55555652de28, addr=0, value=0x7fffe5027538, size=4, shift=0,
>>>>> mask=4294967295)
>>>>>        at /home/chenfan/data/qemu-latest/memory.c:430
>>>>> #2  0x000055555565c9b9 in access_with_adjusted_size (addr=0,
>>>>> value=0x7fffe5027538, size=4, access_size_min=1,
>>>>> access_size_max=4, access= 0x55555565c7d9
>>>>> <memory_region_write_accessor>, mr=0x55555652de28)
>>>>> at /home/chenfan/data/qemu-latest/memory.c:467 #3
>>>>> 0x000055555565f9d1 in memory_region_dispatch_write
>>>>> (mr=0x55555652de28, addr=0, data=16465, size=4)
>>>>> at /home/chenfan/data/qemu-latest/memory.c:1103 #4
>>>>> 0x000055555566356e in io_mem_write (mr=0x55555652de28, addr=0,
>>>>> val=16465, size=4) at /home/chenfan/data/qemu-latest/memory.c:2003
>>>>> #5  0x00005555556060f2 in stl_phys_internal (as=0x5555577568a8,
>>>>> addr=4276092928, val=16465, endian=DEVICE_LITTLE_ENDIAN) #6
>>>>> 0x000055555560621e in stl_le_phys (as=0x5555577568a8,
>>>>> addr=4276092928, val=16465)
>>>>> at /home/chenfan/data/qemu-latest/exec.c:2920 #7
>>>>> 0x000055555587d35e in *msi_notify* (dev=0x5555577566a0, vector=0)
>>>>> at hw/pci/msi.c:294 #8  0x0000555555836f77 in ahci_irq_raise
>>>>> (s=0x555557756f20, dev=0x0) at hw/ide/ahci.c:134 #9
>>>>> 0x00005555558370f2 in ahci_check_irq (s=0x555557756f20) at
>>>>> hw/ide/ahci.c:169 #10 0x000055555583733a in ahci_port_write
>>>>> (s=0x555557756f20, port=0, offset=20, val=2017460351) at
>>>>> hw/ide/ahci.c:225 #11 0x0000555555837811 in ahci_mem_write
>>>>> (opaque=0x555557756f20, addr=276, val=2017460351, size=4) at
>>>>> hw/ide/ahci.c:382 #12 0x000055555565c871 in
>>>>> memory_region_write_accessor (mr=0x555557756f40, addr=276,
>>>>> value=0x7fffe50278b8, size=4, shift=0, mask=4294967295)
>>>>>        at /home/chenfan/data/qemu-latest/memory.c:430
>>>>> #13 0x000055555565c9b9 in access_with_adjusted_size (addr=276,
>>>>> value=0x7fffe50278b8, size=4, access_size_min=1,
>>>>> access_size_max=4, access= 0x55555565c7d9
>>>>> <memory_region_write_accessor>, mr=0x555557756f40)
>>>>> at /home/chenfan/data/qemu-latest/memory.c:467 #14
>>>>> 0x000055555565f9d1 in memory_region_dispatch_write
>>>>> (mr=0x555557756f40, addr=276, data=2017460351, size=4)
>>>>> at /home/chenfan/data/qemu-latest/memory.c:1103 #15
>>>>> 0x000055555566356e in io_mem_write (mr=0x555557756f40, addr=276,
>>>>> val=2017460351, size=4)
>>>>> at /home/chenfan/data/qemu-latest/memory.c:2003
>>>>>
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Chen
>>>>>
>>>>>
>>>>>>> Thanks,
>>>>>>> Chen
>>>>>>>
>>>>>>>
>>>>>>>> PS:
>>>>>>>> It should be part of APIC code or at worst case part of CPU's
>>>>>>>> realize.
>>>>>>>>
>>>>>>>> PS2:
>>>>>>>> new cpu tests don't test actual CPU execution, so they can't
>>>>>>>> validate this change. To test it you need to run test in TCG
>>>>>>>> (at least) or TCG + KVM mode, with some guest code that
>>>>>>>> programs and checks APIC of each CPU.
>>>>>>>>
>>>>>>>> PS3:
>>>>>>>> the rest of the patch I'd suggest to merge with 2/2 patch that
>>>>>>>> removes unused icc_bridge code, there isn't point in splitting
>>>>>>>> that from removing icc_bridge from other files.
>>>>>>>>
>>>>>>>> [...]
>>>>>>>>>      diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>>>>>>>>> index f01690b..2385e6b 100644
>>>>>>>>> --- a/target-i386/cpu.c
>>>>>>>>> +++ b/target-i386/cpu.c
>>>>>>>>> @@ -42,7 +42,6 @@
>>>>>>>>>           #include "sysemu/sysemu.h"
>>>>>>>>>      #include "hw/qdev-properties.h"
>>>>>>>>> -#include "hw/cpu/icc_bus.h"
>>>>>>>>>      #ifndef CONFIG_USER_ONLY
>>>>>>>>>      #include "hw/xen/xen.h"
>>>>>>>>>      #include "hw/i386/apic_internal.h"
>>>>>>>>> @@ -2941,7 +2940,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;
>>>>>>>> that isn't the only place in this file that should be changed.
>>>>>>>>
>>>>>>>> See x86_cpu_apic_create():
>>>>>>>>       cpu->apic_state =
>>>>>>>> qdev_try_create(qdev_get_parent_bus(dev), apic_type);
>>>>>>>>
>>>>>>>> probably it's not right to try get parent bus from bus-less
>>>>>>>> device, qdev_try_create() call should be replaced by
>>>>>>>> object_new()/object_unref() pair.
>>>>>>>>
>>>>>>>>>          dc->props = x86_cpu_properties;
>>>>>>>>>               xcc->parent_reset = cc->reset;
>>>>>>>> .
>>>>>>>>
>>>>>> .
>>>>>>
>>>> .
>>>>
>>>
>> .
>>
>
>
> .
>


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

Re: [Qemu-devel] [PATCH v2 1/2] cpu/apic: drop icc bus/bridge/

[Chen Fan]

On 03/23/2015 04:23 PM, Igor Mammedov wrote:
> On Mon, 23 Mar 2015 13:54:23 +0800
> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
>
>> ICC bus was invented only to provide hotplug capability to
>> CPU and APIC because at the time being hotplug was available only for
>> BUS attached devices.
>>
>> Now this patch is to drop ICC bus impl, and switch to bus-less
>> CPU+APIC hotplug, handling them in the same manner as pc-dimm.
>>
>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>> ---
>>   hw/i386/pc.c                    | 29 +++++++++++------------------
>>   hw/i386/pc_piix.c               |  9 +--------
>>   hw/i386/pc_q35.c                |  9 +--------
>>   hw/intc/apic.c                  |  6 +++---
>>   hw/intc/apic_common.c           | 11 ++---------
>>   include/hw/i386/apic_internal.h |  5 ++---
>>   include/hw/i386/pc.h            |  2 +-
>>   target-i386/cpu.c               |  2 --
>>   8 files changed, 21 insertions(+), 52 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 4b46c29..5d15473 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
> [...]
>
>> @@ -1093,8 +1083,11 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
>>       /* 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);
>> +        apic = APIC_COMMON(cpu->apic_state);
>> +        memory_region_add_subregion_overlap(CPU(cpu)->as->root,
>> +                                            APIC_DEFAULT_ADDRESS,
>> +                                            &apic->io_memory,
>> +                                            0x1000);
> Why is it here?
> Shouldn't it be mapped not once but for each CPU since we are using
> per CPU address spaces?
>
> Split this change out into a separate patch please, with commit message
> describing what it does.
>
> PS:
> It should be part of APIC code or at worst case part of CPU's realize.
>
> PS2:
> new cpu tests don't test actual CPU execution, so they can't validate
> this change. To test it you need to run test in TCG (at least) or
> TCG + KVM mode, with some guest code that programs and checks APIC
> of each CPU.
>
> PS3:
> the rest of the patch I'd suggest to merge with 2/2 patch that
> removes unused icc_bridge code, there isn't point in splitting
> that from removing icc_bridge from other files.
>
> [...]
>>   
>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>> index f01690b..2385e6b 100644
>> --- a/target-i386/cpu.c
>> +++ b/target-i386/cpu.c
>> @@ -42,7 +42,6 @@
>>   
>>   #include "sysemu/sysemu.h"
>>   #include "hw/qdev-properties.h"
>> -#include "hw/cpu/icc_bus.h"
>>   #ifndef CONFIG_USER_ONLY
>>   #include "hw/xen/xen.h"
>>   #include "hw/i386/apic_internal.h"
>> @@ -2941,7 +2940,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;
> that isn't the only place in this file that should be changed.
>
> See x86_cpu_apic_create():
>    cpu->apic_state = qdev_try_create(qdev_get_parent_bus(dev), apic_type);
>
> probably it's not right to try get parent bus from bus-less device,
> qdev_try_create() call should be replaced by object_new()/object_unref() pair.
Hi Igor,

when I tested for this, use object_new() to replace 
qdev_try_create(qdev_get_parent_bus(dev), apic_type),
run with tcg, it works OK, but if run with kvm, it failed. did apic must 
need sysbus?

Thanks,
Chen


>
>>       dc->props = x86_cpu_properties;
>>   
>>       xcc->parent_reset = cc->reset;
> .
>