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

[Qemu-devel] [PATCH v6 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 rebased on Eduardo's x86 tree.
https://github.com/ehabkost/qemu.git

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):
  apic: convert ->busdev.qdev casts to C casts
  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                       |  26 ++------
 hw/i386/pc_piix.c                  |   9 +--
 hw/i386/pc_q35.c                   |   9 +--
 hw/intc/apic.c                     |   9 ++-
 hw/intc/apic_common.c              |  19 +++---
 include/exec/memory.h              |   5 ++
 include/hw/cpu/icc_bus.h           |  82 --------------------------
 include/hw/i386/apic_internal.h    |   7 ++-
 include/hw/i386/pc.h               |   2 +-
 target-i386/cpu.c                  |  16 ++---
 15 files changed, 45 insertions(+), 265 deletions(-)
 delete mode 100644 hw/cpu/icc_bus.c
 delete mode 100644 include/hw/cpu/icc_bus.h

-- 
1.9.3

[Qemu-devel] [PATCH v6 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 ae37b98..623069b 100644
--- a/exec.c
+++ b/exec.c
@@ -2702,6 +2702,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 578ad63..bc06bb4 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1096,13 +1096,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 d38d24b..1c83753 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -297,7 +297,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.",
@@ -308,11 +309,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 0ccfd3b..9735923 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1293,6 +1293,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 3305e09..f83e526 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2740,6 +2740,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

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

[Igor Mammedov]

On Wed, 20 May 2015 10:40:46 +0800
Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:

> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> 
> Replace mapping APIC at global system address space with
> mapping it at per-CPU address spaces.
> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
patch doesn't apply due to a small conflict in hw/i386/pc.c

otherwise
Reviewed-by: Igor Mammedov <imammedo@redhat.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 ae37b98..623069b 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2702,6 +2702,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 578ad63..bc06bb4 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1096,13 +1096,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 d38d24b..1c83753 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -297,7 +297,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.",
> @@ -308,11 +309,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 0ccfd3b..9735923 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1293,6 +1293,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 3305e09..f83e526 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2740,6 +2740,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)

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

[Eduardo Habkost]

On Wed, May 20, 2015 at 01:46:45PM +0200, Igor Mammedov wrote:
> On Wed, 20 May 2015 10:40:46 +0800
> Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:
> 
> > From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> > 
> > Replace mapping APIC at global system address space with
> > mapping it at per-CPU address spaces.
> > 
> > Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> > Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> patch doesn't apply due to a small conflict in hw/i386/pc.c

The series applies cleanly on top of the x86 branch at
 https://github.com/ehabkost/qemu.git

> 
> otherwise
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>

Thanks!

-- 
Eduardo

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

[Igor Mammedov]

On Wed, 20 May 2015 09:38:16 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, May 20, 2015 at 01:46:45PM +0200, Igor Mammedov wrote:
> > On Wed, 20 May 2015 10:40:46 +0800
> > Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:
> > 
> > > From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> > > 
> > > Replace mapping APIC at global system address space with
> > > mapping it at per-CPU address spaces.
> > > 
> > > Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> > > Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> > patch doesn't apply due to a small conflict in hw/i386/pc.c
> 
> The series applies cleanly on top of the x86 branch at
>  https://github.com/ehabkost/qemu.git

Would you pull it through your tree?

> > 
> > otherwise
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> 
> Thanks!
> 

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

[Eduardo Habkost]

On Wed, May 20, 2015 at 03:50:29PM +0200, Igor Mammedov wrote:
> On Wed, 20 May 2015 09:38:16 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Wed, May 20, 2015 at 01:46:45PM +0200, Igor Mammedov wrote:
> > > On Wed, 20 May 2015 10:40:46 +0800
> > > Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:
> > > 
> > > > From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> > > > 
> > > > Replace mapping APIC at global system address space with
> > > > mapping it at per-CPU address spaces.
> > > > 
> > > > Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> > > > Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> > > patch doesn't apply due to a small conflict in hw/i386/pc.c
> > 
> > The series applies cleanly on top of the x86 branch at
> >  https://github.com/ehabkost/qemu.git
> 
> Would you pull it through your tree?

I plan to, as it's x86-specific and directly related to CPU code.
(I will explicitly ask for Acked-by lines in a reply to the cover
letter)

-- 
Eduardo

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

[Eduardo Habkost]

On Wed, May 20, 2015 at 10:40:46AM +0800, Zhu Guihua wrote:
> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> 
> Replace mapping APIC at global system address space with
> mapping it at per-CPU address spaces.
> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>

Applied to the x86 tree. Thanks.

-- 
Eduardo

[Qemu-devel] [PATCH v6 2/4] apic: convert ->busdev.qdev casts to C casts

[Zhu Guihua]

Use C casts to avoid accessing ICCDevice's qdev field
directly.

Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
---
 hw/intc/apic.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index 0f97b47..77b639c 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -370,13 +370,14 @@ static int apic_irq_pending(APICCommonState *s)
 static void apic_update_irq(APICCommonState *s)
 {
     CPUState *cpu;
+    DeviceState *dev = (DeviceState *)s;
 
     cpu = CPU(s->cpu);
     if (!qemu_cpu_is_self(cpu)) {
         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(dev) || !pic_get_output(isa_pic)) {
         cpu_reset_interrupt(cpu, CPU_INTERRUPT_HARD);
     }
 }
@@ -549,10 +550,12 @@ 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)) {
+    DeviceState *dev = (DeviceState *)s;
+
+    if (!apic_accept_pic_intr(dev) || !pic_get_output(isa_pic)) {
         return false;
     }
-    apic_deliver_pic_intr(&s->busdev.qdev, 1);
+    apic_deliver_pic_intr(dev, 1);
     return true;
 }
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH v6 2/4] apic: convert ->busdev.qdev casts to C casts

[Igor Mammedov]

On Wed, 20 May 2015 10:40:47 +0800
Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:

> Use C casts to avoid accessing ICCDevice's qdev field
> directly.
> 
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/intc/apic.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/intc/apic.c b/hw/intc/apic.c
> index 0f97b47..77b639c 100644
> --- a/hw/intc/apic.c
> +++ b/hw/intc/apic.c
> @@ -370,13 +370,14 @@ static int apic_irq_pending(APICCommonState *s)
>  static void apic_update_irq(APICCommonState *s)
>  {
>      CPUState *cpu;
> +    DeviceState *dev = (DeviceState *)s;
>  
>      cpu = CPU(s->cpu);
>      if (!qemu_cpu_is_self(cpu)) {
>          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(dev) || !pic_get_output(isa_pic)) {
>          cpu_reset_interrupt(cpu, CPU_INTERRUPT_HARD);
>      }
>  }
> @@ -549,10 +550,12 @@ 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)) {
> +    DeviceState *dev = (DeviceState *)s;
> +
> +    if (!apic_accept_pic_intr(dev) || !pic_get_output(isa_pic)) {
>          return false;
>      }
> -    apic_deliver_pic_intr(&s->busdev.qdev, 1);
> +    apic_deliver_pic_intr(dev, 1);
>      return true;
>  }
>  

Re: [Qemu-devel] [PATCH v6 2/4] apic: convert ->busdev.qdev casts to C casts

[Andreas Färber]

Am 20.05.2015 um 04:40 schrieb Zhu Guihua:
> Use C casts to avoid accessing ICCDevice's qdev field
> directly.
> 
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> ---
>  hw/intc/apic.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

Reviewed-by: Andreas Färber <afaerber@suse.de>

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] [PATCH v6 2/4] apic: convert ->busdev.qdev casts to C casts

[Eduardo Habkost]

On Wed, May 20, 2015 at 10:40:47AM +0800, Zhu Guihua wrote:
> Use C casts to avoid accessing ICCDevice's qdev field
> directly.
> 
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>

Applied to the x86 tree. Thanks.

-- 
Eduardo

[Qemu-devel] [PATCH v6 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 its parent (CPU) directly when CPU is being reset.
Do so and drop ~200LOC of not needed anymore ICCBus related code.

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

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index bc06bb4..4458dbb 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"
@@ -990,23 +989,16 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
 }
 
 static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
-                          DeviceState *icc_bridge, Error **errp)
+                          Error **errp)
 {
     X86CPU *cpu = NULL;
     Error *local_err = NULL;
 
-    if (icc_bridge == NULL) {
-        error_setg(&local_err, "Invalid icc-bridge value");
-        goto out;
-    }
-
     cpu = cpu_x86_create(cpu_model, &local_err);
     if (local_err != NULL) {
         goto out;
     }
 
-    qdev_set_parent_bus(DEVICE(cpu), qdev_get_child_bus(icc_bridge, "icc"));
-
     object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
     object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
 
@@ -1023,7 +1015,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;
@@ -1052,9 +1043,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;
@@ -1062,7 +1051,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;
@@ -1088,7 +1077,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 1fe7bfb..6004580 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 dcc17c0..3b89e6a 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_common.c b/hw/intc/apic_common.c
index 1c83753..edb4e42 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -430,13 +430,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()
@@ -446,7 +445,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 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 f83e526..4080909 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"
@@ -2595,6 +2594,11 @@ static void x86_cpu_reset(CPUState *s)
 
     xcc->parent_reset(s);
 
+    /* since APIC is a bus-less device, propagate reset to it manually */
+    if (cpu->apic_state) {
+        device_reset(DEVICE(cpu->apic_state));
+    }
+
     memset(env, 0, offsetof(CPUX86State, cpuid_level));
 
     tlb_flush(s, 1);
@@ -2718,7 +2722,6 @@ static void mce_init(X86CPU *cpu)
 #ifndef CONFIG_USER_ONLY
 static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
 {
-    DeviceState *dev = DEVICE(cpu);
     APICCommonState *apic;
     const char *apic_type = "apic";
 
@@ -2728,11 +2731,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
         apic_type = "xen-apic";
     }
 
-    cpu->apic_state = qdev_try_create(qdev_get_parent_bus(dev), apic_type);
-    if (cpu->apic_state == NULL) {
-        error_setg(errp, "APIC device '%s' could not be created", apic_type);
-        return;
-    }
+    cpu->apic_state = DEVICE(object_new(apic_type));
 
     object_property_add_child(OBJECT(cpu), "apic",
                               OBJECT(cpu->apic_state), NULL);
@@ -2969,7 +2968,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

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

[Igor Mammedov]

On Wed, 20 May 2015 10:40:48 +0800
Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:

> 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 its parent (CPU) directly when CPU is being reset.
> 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>

This patch regresses emulated APIC,
during RHEL7 boot:

[    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 ]---


void setup_local_APIC(void)
...
        } while (queued && max_loops > 0);
        WARN_ON(max_loops <= 0);                     <=== here
...

reproducer:
  qemu-system-x86_64 -enable-kvm -m 2048  -smp 4 -machine kernel_irqchip=off rhel7.img
or just slower plain TCG
  qemu-system-x86_64 -m 2048 -smp 4 rhel7.img

it happens only on VM startup, there isn't any warning when booting after reset.
> ---
>  hw/i386/pc.c                    | 19 ++++---------------
>  hw/i386/pc_piix.c               |  9 +--------
>  hw/i386/pc_q35.c                |  9 +--------
>  hw/intc/apic_common.c           |  5 ++---
>  include/hw/i386/apic_internal.h |  7 ++++---
>  include/hw/i386/pc.h            |  2 +-
>  target-i386/cpu.c               | 14 ++++++--------
>  7 files changed, 19 insertions(+), 46 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index bc06bb4..4458dbb 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"
> @@ -990,23 +989,16 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
>  }
>  
>  static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
> -                          DeviceState *icc_bridge, Error **errp)
> +                          Error **errp)
>  {
>      X86CPU *cpu = NULL;
>      Error *local_err = NULL;
>  
> -    if (icc_bridge == NULL) {
> -        error_setg(&local_err, "Invalid icc-bridge value");
> -        goto out;
> -    }
> -
>      cpu = cpu_x86_create(cpu_model, &local_err);
>      if (local_err != NULL) {
>          goto out;
>      }
>  
> -    qdev_set_parent_bus(DEVICE(cpu), qdev_get_child_bus(icc_bridge, "icc"));
> -
>      object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
>      object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
>  
> @@ -1023,7 +1015,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;
> @@ -1052,9 +1043,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;
> @@ -1062,7 +1051,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;
> @@ -1088,7 +1077,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 1fe7bfb..6004580 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 dcc17c0..3b89e6a 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_common.c b/hw/intc/apic_common.c
> index 1c83753..edb4e42 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -430,13 +430,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()
> @@ -446,7 +445,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 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 f83e526..4080909 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"
> @@ -2595,6 +2594,11 @@ static void x86_cpu_reset(CPUState *s)
>  
>      xcc->parent_reset(s);
>  
> +    /* since APIC is a bus-less device, propagate reset to it manually */
> +    if (cpu->apic_state) {
> +        device_reset(DEVICE(cpu->apic_state));
> +    }
> +
>      memset(env, 0, offsetof(CPUX86State, cpuid_level));
>  
>      tlb_flush(s, 1);
> @@ -2718,7 +2722,6 @@ static void mce_init(X86CPU *cpu)
>  #ifndef CONFIG_USER_ONLY
>  static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>  {
> -    DeviceState *dev = DEVICE(cpu);
>      APICCommonState *apic;
>      const char *apic_type = "apic";
>  
> @@ -2728,11 +2731,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>          apic_type = "xen-apic";
>      }
>  
> -    cpu->apic_state = qdev_try_create(qdev_get_parent_bus(dev), apic_type);
> -    if (cpu->apic_state == NULL) {
> -        error_setg(errp, "APIC device '%s' could not be created", apic_type);
> -        return;
> -    }
> +    cpu->apic_state = DEVICE(object_new(apic_type));
>  
>      object_property_add_child(OBJECT(cpu), "apic",
>                                OBJECT(cpu->apic_state), NULL);
> @@ -2969,7 +2968,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;

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

[Chen Fan]

On 05/20/2015 10:53 PM, Igor Mammedov wrote:
> On Wed, 20 May 2015 10:40:48 +0800
> Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:
>
>> 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 its parent (CPU) directly when CPU is being reset.
>> 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>
> This patch regresses emulated APIC,
> during RHEL7 boot:
>
> [    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 ]---
>
>
> void setup_local_APIC(void)
> ...
>          } while (queued && max_loops > 0);
>          WARN_ON(max_loops <= 0);                     <=== here
> ...
>
> reproducer:
>    qemu-system-x86_64 -enable-kvm -m 2048  -smp 4 -machine kernel_irqchip=off rhel7.img
> or just slower plain TCG
>    qemu-system-x86_64 -m 2048 -smp 4 rhel7.img
>
> it happens only on VM startup, there isn't any warning when booting after reset.
Hi Igor, Thanks for you pointing it out.

I had found that the problem appeared after we moved the apic reset into 
cpu reset.

the original operation is that there are devices (such as hpet, rtc) 
reset before apic reset,
when these devices reset, it would send irq to apic, before the change, 
the apic reset
is behind these devices reset. so the apic register is set to default 
values.

but after the change, thanks to the cpu reset is before the qemu system 
reset which causes
that the apic reset ahead the other devices reset. but before guest boot 
up, the irq request
should be rejected.  so when linux enable local apic, it would found 
there were irr requests.
then cause warn_on.

so I make a enforce cpu reset after qemu system reset. and I also change 
the apicbase value
by default, because cpu_set_apic_base()/apic_set_base() can not enable 
the APICBASE_ENABLE
bit in apic model. and I have test it, it seems work fine. the hasty 
change is that:

diff --git a/cpus.c b/cpus.c
index de6469f..b99e6ec 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1196,6 +1196,15 @@ void resume_all_vcpus(void)
      }
  }

+void reset_all_vcpus(void)
+{
+    CPUState *cpu;
+
+    CPU_FOREACH(cpu) {
+        cpu_reset(cpu);
+    }
+}
+
  /* For temporary buffers for forming a name */
  #define VCPU_THREAD_NAME_SIZE 16

diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
index 3f162a9..5c1e9f2 100644
--- a/include/sysemu/cpus.h
+++ b/include/sysemu/cpus.h
@@ -5,6 +5,7 @@
  void qemu_init_cpu_loop(void);
  void resume_all_vcpus(void);
  void pause_all_vcpus(void);
+void reset_all_vcpus(void);
  void cpu_stop_current(void);

  void cpu_synchronize_all_states(void);
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 4080909..18bbe35 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2694,13 +2694,6 @@ bool cpu_is_bsp(X86CPU *cpu)
  {
      return cpu_get_apic_base(cpu->apic_state) & MSR_IA32_APICBASE_BSP;
  }
-
-/* TODO: remove me, when reset over QOM tree is implemented */
-static void x86_cpu_machine_reset_cb(void *opaque)
-{
-    X86CPU *cpu = opaque;
-    cpu_reset(CPU(cpu));
-}
  #endif

  static void mce_init(X86CPU *cpu)
@@ -2739,8 +2732,7 @@ 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);
+    apic->apicbase = APIC_DEFAULT_ADDRESS | MSR_IA32_APICBASE_ENABLE;
  }

  static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
@@ -2801,8 +2793,6 @@ static void x86_cpu_realizefn(DeviceState *dev, 
Error **errp)
      }

  #ifndef CONFIG_USER_ONLY
-    qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
-
      if (cpu->env.features[FEAT_1_EDX] & CPUID_APIC || smp_cpus > 1) {
          x86_cpu_apic_create(cpu, &local_err);
          if (local_err != NULL) {
diff --git a/vl.c b/vl.c
index 15bccc4..0c53053 100644
--- a/vl.c
+++ b/vl.c
@@ -1618,6 +1618,7 @@ void qemu_devices_reset(void)
      QTAILQ_FOREACH_SAFE(re, &reset_handlers, entry, nre) {
          re->func(re->opaque);
      }
+    reset_all_vcpus();
  }

  void qemu_system_reset(bool report)

>> ---
>>   hw/i386/pc.c                    | 19 ++++---------------
>>   hw/i386/pc_piix.c               |  9 +--------
>>   hw/i386/pc_q35.c                |  9 +--------
>>   hw/intc/apic_common.c           |  5 ++---
>>   include/hw/i386/apic_internal.h |  7 ++++---
>>   include/hw/i386/pc.h            |  2 +-
>>   target-i386/cpu.c               | 14 ++++++--------
>>   7 files changed, 19 insertions(+), 46 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index bc06bb4..4458dbb 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"
>> @@ -990,23 +989,16 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
>>   }
>>   
>>   static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
>> -                          DeviceState *icc_bridge, Error **errp)
>> +                          Error **errp)
>>   {
>>       X86CPU *cpu = NULL;
>>       Error *local_err = NULL;
>>   
>> -    if (icc_bridge == NULL) {
>> -        error_setg(&local_err, "Invalid icc-bridge value");
>> -        goto out;
>> -    }
>> -
>>       cpu = cpu_x86_create(cpu_model, &local_err);
>>       if (local_err != NULL) {
>>           goto out;
>>       }
>>   
>> -    qdev_set_parent_bus(DEVICE(cpu), qdev_get_child_bus(icc_bridge, "icc"));
>> -
>>       object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
>>       object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
>>   
>> @@ -1023,7 +1015,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;
>> @@ -1052,9 +1043,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;
>> @@ -1062,7 +1051,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;
>> @@ -1088,7 +1077,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 1fe7bfb..6004580 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 dcc17c0..3b89e6a 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_common.c b/hw/intc/apic_common.c
>> index 1c83753..edb4e42 100644
>> --- a/hw/intc/apic_common.c
>> +++ b/hw/intc/apic_common.c
>> @@ -430,13 +430,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()
>> @@ -446,7 +445,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 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 f83e526..4080909 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"
>> @@ -2595,6 +2594,11 @@ static void x86_cpu_reset(CPUState *s)
>>   
>>       xcc->parent_reset(s);
>>   
>> +    /* since APIC is a bus-less device, propagate reset to it manually */
>> +    if (cpu->apic_state) {
>> +        device_reset(DEVICE(cpu->apic_state));
>> +    }
>> +
>>       memset(env, 0, offsetof(CPUX86State, cpuid_level));
>>   
>>       tlb_flush(s, 1);
>> @@ -2718,7 +2722,6 @@ static void mce_init(X86CPU *cpu)
>>   #ifndef CONFIG_USER_ONLY
>>   static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>>   {
>> -    DeviceState *dev = DEVICE(cpu);
>>       APICCommonState *apic;
>>       const char *apic_type = "apic";
>>   
>> @@ -2728,11 +2731,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>>           apic_type = "xen-apic";
>>       }
>>   
>> -    cpu->apic_state = qdev_try_create(qdev_get_parent_bus(dev), apic_type);
>> -    if (cpu->apic_state == NULL) {
>> -        error_setg(errp, "APIC device '%s' could not be created", apic_type);
>> -        return;
>> -    }
>> +    cpu->apic_state = DEVICE(object_new(apic_type));
>>   
>>       object_property_add_child(OBJECT(cpu), "apic",
>>                                 OBJECT(cpu->apic_state), NULL);
>> @@ -2969,7 +2968,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;
> .
>

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

[Eduardo Habkost]

On Fri, May 22, 2015 at 03:44:53PM +0800, Chen Fan wrote:
> On 05/20/2015 10:53 PM, Igor Mammedov wrote:
> >On Wed, 20 May 2015 10:40:48 +0800
> >Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:
> >
> >>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 its parent (CPU) directly when CPU is being reset.
> >>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>
> >This patch regresses emulated APIC,
> >during RHEL7 boot:
> >
> >[    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 ]---
> >
> >
> >void setup_local_APIC(void)
> >...
> >         } while (queued && max_loops > 0);
> >         WARN_ON(max_loops <= 0);                     <=== here
> >...
> >
> >reproducer:
> >   qemu-system-x86_64 -enable-kvm -m 2048  -smp 4 -machine kernel_irqchip=off rhel7.img
> >or just slower plain TCG
> >   qemu-system-x86_64 -m 2048 -smp 4 rhel7.img
> >
> >it happens only on VM startup, there isn't any warning when booting after reset.
> Hi Igor, Thanks for you pointing it out.
> 
> I had found that the problem appeared after we moved the apic reset into cpu
> reset.
> 
> the original operation is that there are devices (such as hpet, rtc) reset
> before apic reset,
> when these devices reset, it would send irq to apic, before the change, the
> apic reset
> is behind these devices reset. so the apic register is set to default
> values.
> 
> but after the change, thanks to the cpu reset is before the qemu system
> reset which causes
> that the apic reset ahead the other devices reset. but before guest boot up,
> the irq request
> should be rejected.  so when linux enable local apic, it would found there
> were irr requests.
> then cause warn_on.
> 
[...]
>  static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
> @@ -2801,8 +2793,6 @@ static void x86_cpu_realizefn(DeviceState *dev, Error
> **errp)
>      }
> 
>  #ifndef CONFIG_USER_ONLY
> -    qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
> -
>      if (cpu->env.features[FEAT_1_EDX] & CPUID_APIC || smp_cpus > 1) {
>          x86_cpu_apic_create(cpu, &local_err);
>          if (local_err != NULL) {
> diff --git a/vl.c b/vl.c
> index 15bccc4..0c53053 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1618,6 +1618,7 @@ void qemu_devices_reset(void)
>      QTAILQ_FOREACH_SAFE(re, &reset_handlers, entry, nre) {
>          re->func(re->opaque);
>      }
> +    reset_all_vcpus();
>  }

What about all the other architectures and machines that may expect
different reset ordering, and that already register their own CPU reset
handlers?

If x86 has specific CPU reset ordering requirements, we should be able
to ensure the expected ordering in x86-specific code (in pc.c?), not
hardcode reset ordering for all machines.

(BTW, what was the motivation to move qemu_register_reset() from pc.c to
target-i386/cpu.c? The only architectures that register reset handlers
inside the CPU code are x86 and s390x, all others register reset
handlers inside machine code.)

-- 
Eduardo

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

[Andreas Färber]

Am 22.05.2015 um 18:56 schrieb Eduardo Habkost:
> On Fri, May 22, 2015 at 03:44:53PM +0800, Chen Fan wrote:
>>  static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
>> @@ -2801,8 +2793,6 @@ static void x86_cpu_realizefn(DeviceState *dev, Error
>> **errp)
>>      }
>>
>>  #ifndef CONFIG_USER_ONLY
>> -    qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
>> -
>>      if (cpu->env.features[FEAT_1_EDX] & CPUID_APIC || smp_cpus > 1) {
>>          x86_cpu_apic_create(cpu, &local_err);
>>          if (local_err != NULL) {
>> diff --git a/vl.c b/vl.c
>> index 15bccc4..0c53053 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -1618,6 +1618,7 @@ void qemu_devices_reset(void)
>>      QTAILQ_FOREACH_SAFE(re, &reset_handlers, entry, nre) {
>>          re->func(re->opaque);
>>      }
>> +    reset_all_vcpus();
>>  }
> 
> What about all the other architectures and machines that may expect
> different reset ordering, and that already register their own CPU reset
> handlers?
> 
> If x86 has specific CPU reset ordering requirements, we should be able
> to ensure the expected ordering in x86-specific code (in pc.c?), not
> hardcode reset ordering for all machines.

+1

In particular pseries has special ordering requirements.

> (BTW, what was the motivation to move qemu_register_reset() from pc.c to
> target-i386/cpu.c? The only architectures that register reset handlers
> inside the CPU code are x86 and s390x, all others register reset
> handlers inside machine code.)

I don't remember the motivation, it was Anthony overturning my objection
against that exception from the rule though. If we can bring x86 or the
other targets back in line, feel free to make an RFC.
I still have an old branch with initial reset support for alpha and rth
had a patch to that effect, too.

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)

[Qemu-devel] [PATCH v6 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 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..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] [PATCH v6 0/4] remove icc bus/bridge

[Eduardo Habkost]

Paolo, Michael, Andreas: I plan to apply this on the x86 tree once it
gets reviewed. If you don't object, can I get Acked-by lines from you?

On Wed, May 20, 2015 at 10:40:45AM +0800, Zhu Guihua wrote:
> 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 rebased on Eduardo's x86 tree.
> https://github.com/ehabkost/qemu.git
> 
> 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):
>   apic: convert ->busdev.qdev casts to C casts
>   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                       |  26 ++------
>  hw/i386/pc_piix.c                  |   9 +--
>  hw/i386/pc_q35.c                   |   9 +--
>  hw/intc/apic.c                     |   9 ++-
>  hw/intc/apic_common.c              |  19 +++---
>  include/exec/memory.h              |   5 ++
>  include/hw/cpu/icc_bus.h           |  82 --------------------------
>  include/hw/i386/apic_internal.h    |   7 ++-
>  include/hw/i386/pc.h               |   2 +-
>  target-i386/cpu.c                  |  16 ++---
>  15 files changed, 45 insertions(+), 265 deletions(-)
>  delete mode 100644 hw/cpu/icc_bus.c
>  delete mode 100644 include/hw/cpu/icc_bus.h
> 
> -- 
> 1.9.3
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v6 0/4] remove icc bus/bridge

[Andreas Färber]

Am 20.05.2015 um 16:14 schrieb Eduardo Habkost:
> Paolo, Michael, Andreas: I plan to apply this on the x86 tree once it
> gets reviewed. If you don't object, can I get Acked-by lines from you?

What's the state of my reference counting patch? Is that in your tree?
It will conflict and I will be on vacation soon, so please take care
this gets resolved. Other than that slightly short on time, on a brief
look it was okay and I was expecting this to go through your tree.

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] [PATCH v6 0/4] remove icc bus/bridge

[Eduardo Habkost]

On Wed, May 20, 2015 at 04:54:07PM +0200, Andreas Färber wrote:
> Am 20.05.2015 um 16:14 schrieb Eduardo Habkost:
> > Paolo, Michael, Andreas: I plan to apply this on the x86 tree once it
> > gets reviewed. If you don't object, can I get Acked-by lines from you?
> 
> What's the state of my reference counting patch? Is that in your tree?
> It will conflict and I will be on vacation soon, so please take care
> this gets resolved. Other than that slightly short on time, on a brief
> look it was okay and I was expecting this to go through your tree.

It is on my tree and this series is based on top of it.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v6 0/4] remove icc bus/bridge

[Andreas Färber]

Am 20.05.2015 um 16:58 schrieb Eduardo Habkost:
> On Wed, May 20, 2015 at 04:54:07PM +0200, Andreas Färber wrote:
>> Am 20.05.2015 um 16:14 schrieb Eduardo Habkost:
>>> Paolo, Michael, Andreas: I plan to apply this on the x86 tree once it
>>> gets reviewed. If you don't object, can I get Acked-by lines from you?
>>
>> What's the state of my reference counting patch? Is that in your tree?
>> It will conflict and I will be on vacation soon, so please take care
>> this gets resolved. Other than that slightly short on time, on a brief
>> look it was okay and I was expecting this to go through your tree.
> 
> It is on my tree and this series is based on top of it.

Great, then assuming you sort out the remaining issues,

Acked-by: Andreas Färber <afaerber@suse.de>

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)