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

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 15 Sep 2015 08:52:34 +0800
Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:

> 
> On 09/14/2015 09:18 PM, Igor Mammedov wrote:
> > On Wed, 2 Sep 2015 17:36:21 +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 registering its reset handler after all device are
> >> initialized.
> >> Do so and drop ~200LOC of not needed anymore ICCBus related code.
> > this patch drops about 30LOCs only ...
> >
> >
> > PS:
> > Doesn't apply to current master, please rebase.
> 
> This patch rebased on my another patch
> '[PATCH v3] i386: keep cpu_model field in MachineState uptodate'
> https://lists.nongnu.org/archive/html/qemu-devel/2015-08/msg03375.html
> 
> The patch has been reviewed by Eduardo, but not been merged.
> 
> Should I only rebase on current master?
you could pull dependency in this series, it's only one patch.

> 
> Thanks,
> Zhu
> 
> >> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> >> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
> >> ---
> >>   hw/i386/pc.c                    | 19 ++++---------------
> >>   hw/i386/pc_piix.c               |  9 +--------
> >>   hw/i386/pc_q35.c                |  9 +--------
> >>   hw/intc/apic_common.c           |  5 ++---
> >>   include/hw/i386/apic_internal.h |  7 ++++---
> >>   include/hw/i386/pc.h            |  2 +-
> >>   target-i386/cpu.c               |  9 +--------
> >>   7 files changed, 14 insertions(+), 46 deletions(-)
> >>
> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >> index 4b4a7f3..4c1d68a 100644
> >> --- a/hw/i386/pc.c
> >> +++ b/hw/i386/pc.c
> >> @@ -59,7 +59,6 @@
> >>   #include "qemu/error-report.h"
> >>   #include "hw/acpi/acpi.h"
> >>   #include "hw/acpi/cpu_hotplug.h"
> >> -#include "hw/cpu/icc_bus.h"
> >>   #include "hw/boards.h"
> >>   #include "hw/pci/pci_host.h"
> >>   #include "acpi-build.h"
> >> @@ -1052,23 +1051,16 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
> >>   }
> >>   
> >>   static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
> >> -                          DeviceState *icc_bridge, Error **errp)
> >> +                          Error **errp)
> >>   {
> >>       X86CPU *cpu = NULL;
> >>       Error *local_err = NULL;
> >>   
> >> -    if (icc_bridge == NULL) {
> >> -        error_setg(&local_err, "Invalid icc-bridge value");
> >> -        goto out;
> >> -    }
> >> -
> >>       cpu = cpu_x86_create(cpu_model, &local_err);
> >>       if (local_err != NULL) {
> >>           goto out;
> >>       }
> >>   
> >> -    qdev_set_parent_bus(DEVICE(cpu), qdev_get_child_bus(icc_bridge, "icc"));
> >> -
> >>       object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
> >>       object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
> >>   
> >> @@ -1083,7 +1075,6 @@ out:
> >>   
> >>   void pc_hot_add_cpu(const int64_t id, Error **errp)
> >>   {
> >> -    DeviceState *icc_bridge;
> >>       MachineState *machine = MACHINE(qdev_get_machine());
> >>       X86CPU *cpu;
> >>       int64_t apic_id = x86_cpu_apic_id_from_index(id);
> >> @@ -1113,9 +1104,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(machine->cpu_model, apic_id, icc_bridge, &local_err);
> >> +    cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err);
> >>       if (local_err) {
> >>           error_propagate(errp, local_err);
> >>           return;
> >> @@ -1123,7 +1112,7 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
> >>       object_unref(OBJECT(cpu));
> >>   }
> >>   
> >> -void pc_cpus_init(PCMachineState *pcms, DeviceState *icc_bridge)
> >> +void pc_cpus_init(PCMachineState *pcms)
> >>   {
> >>       int i;
> >>       X86CPU *cpu = NULL;
> >> @@ -1149,7 +1138,7 @@ void pc_cpus_init(PCMachineState *pcms, DeviceState *icc_bridge)
> >>   
> >>       for (i = 0; i < smp_cpus; i++) {
> >>           cpu = pc_new_cpu(machine->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 fd6130d..3a97826 100644
> >> --- a/hw/i386/pc_piix.c
> >> +++ b/hw/i386/pc_piix.c
> >> @@ -39,7 +39,6 @@
> >>   #include "hw/kvm/clock.h"
> >>   #include "sysemu/sysemu.h"
> >>   #include "hw/sysbus.h"
> >> -#include "hw/cpu/icc_bus.h"
> >>   #include "sysemu/arch_init.h"
> >>   #include "sysemu/block-backend.h"
> >>   #include "hw/i2c/smbus.h"
> >> @@ -96,7 +95,6 @@ static void pc_init1(MachineState *machine)
> >>       MemoryRegion *ram_memory;
> >>       MemoryRegion *pci_memory;
> >>       MemoryRegion *rom_memory;
> >> -    DeviceState *icc_bridge;
> >>       PcGuestInfo *guest_info;
> >>       ram_addr_t lowmem;
> >>   
> >> @@ -141,11 +139,7 @@ static void pc_init1(MachineState *machine)
> >>           exit(1);
> >>       }
> >>   
> >> -    icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
> >> -    object_property_add_child(qdev_get_machine(), "icc-bridge",
> >> -                              OBJECT(icc_bridge), NULL);
> >> -
> >> -    pc_cpus_init(pcms, icc_bridge);
> >> +    pc_cpus_init(pcms);
> >>   
> >>       if (kvm_enabled() && kvmclock_enabled) {
> >>           kvmclock_create();
> >> @@ -223,7 +217,6 @@ static void pc_init1(MachineState *machine)
> >>       if (pci_enabled) {
> >>           ioapic_init_gsi(gsi_state, "i440fx");
> >>       }
> >> -    qdev_init_nofail(icc_bridge);
> >>   
> >>       pc_register_ferr_irq(gsi[13]);
> >>   
> >> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> >> index 4f76535..414cbbb 100644
> >> --- a/hw/i386/pc_q35.c
> >> +++ b/hw/i386/pc_q35.c
> >> @@ -43,7 +43,6 @@
> >>   #include "hw/ide/pci.h"
> >>   #include "hw/ide/ahci.h"
> >>   #include "hw/usb.h"
> >> -#include "hw/cpu/icc_bus.h"
> >>   #include "qemu/error-report.h"
> >>   #include "migration/migration.h"
> >>   
> >> @@ -83,7 +82,6 @@ static void pc_q35_init(MachineState *machine)
> >>       int i;
> >>       ICH9LPCState *ich9_lpc;
> >>       PCIDevice *ahci;
> >> -    DeviceState *icc_bridge;
> >>       PcGuestInfo *guest_info;
> >>       ram_addr_t lowmem;
> >>       DriveInfo *hd[MAX_SATA_PORTS];
> >> @@ -132,11 +130,7 @@ static void pc_q35_init(MachineState *machine)
> >>           exit(1);
> >>       }
> >>   
> >> -    icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
> >> -    object_property_add_child(qdev_get_machine(), "icc-bridge",
> >> -                              OBJECT(icc_bridge), NULL);
> >> -
> >> -    pc_cpus_init(pcms, icc_bridge);
> >> +    pc_cpus_init(pcms);
> >>       pc_acpi_init("q35-acpi-dsdt.aml");
> >>   
> >>       kvmclock_create();
> >> @@ -237,7 +231,6 @@ static void pc_q35_init(MachineState *machine)
> >>       if (pci_enabled) {
> >>           ioapic_init_gsi(gsi_state, "q35");
> >>       }
> >> -    qdev_init_nofail(icc_bridge);
> >>   
> >>       pc_register_ferr_irq(gsi[13]);
> >>   
> >> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> >> index c0b32eb..ad959c4 100644
> >> --- a/hw/intc/apic_common.c
> >> +++ b/hw/intc/apic_common.c
> >> @@ -419,13 +419,12 @@ static Property apic_properties_common[] = {
> >>   
> >>   static void apic_common_class_init(ObjectClass *klass, void *data)
> >>   {
> >> -    ICCDeviceClass *idc = ICC_DEVICE_CLASS(klass);
> >>       DeviceClass *dc = DEVICE_CLASS(klass);
> >>   
> >>       dc->vmsd = &vmstate_apic_common;
> >>       dc->reset = apic_reset_common;
> >>       dc->props = apic_properties_common;
> >> -    idc->realize = apic_common_realize;
> >> +    dc->realize = apic_common_realize;
> >>       /*
> >>        * Reason: APIC and CPU need to be wired up by
> >>        * x86_cpu_apic_create()
> >> @@ -435,7 +434,7 @@ static void apic_common_class_init(ObjectClass *klass, void *data)
> >>   
> >>   static const TypeInfo apic_common_type = {
> >>       .name = TYPE_APIC_COMMON,
> >> -    .parent = TYPE_ICC_DEVICE,
> >> +    .parent = TYPE_DEVICE,
> >>       .instance_size = sizeof(APICCommonState),
> >>       .class_size = sizeof(APICCommonClass),
> >>       .class_init = apic_common_class_init,
> >> diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
> >> index 26632ac..905ada7 100644
> >> --- a/include/hw/i386/apic_internal.h
> >> +++ b/include/hw/i386/apic_internal.h
> >> @@ -22,7 +22,6 @@
> >>   
> >>   #include "cpu.h"
> >>   #include "exec/memory.h"
> >> -#include "hw/cpu/icc_bus.h"
> >>   #include "qemu/timer.h"
> >>   
> >>   /* APIC Local Vector Table */
> >> @@ -79,7 +78,7 @@ typedef struct APICCommonState APICCommonState;
> >>   
> >>   typedef struct APICCommonClass
> >>   {
> >> -    ICCDeviceClass parent_class;
> >> +    DeviceClass parent_class;
> >>   
> >>       DeviceRealize realize;
> >>       void (*set_base)(APICCommonState *s, uint64_t val);
> >> @@ -94,7 +93,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 3732ab5..8eef558 100644
> >> --- a/include/hw/i386/pc.h
> >> +++ b/include/hw/i386/pc.h
> >> @@ -161,7 +161,7 @@ bool pc_machine_is_smm_enabled(PCMachineState *pcms);
> >>   void pc_register_ferr_irq(qemu_irq irq);
> >>   void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
> >>   
> >> -void pc_cpus_init(PCMachineState *pcms, DeviceState *icc_bridge);
> >> +void pc_cpus_init(PCMachineState *pcms);
> >>   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 0a95162..6686039 100644
> >> --- a/target-i386/cpu.c
> >> +++ b/target-i386/cpu.c
> >> @@ -43,7 +43,6 @@
> >>   
> >>   #include "sysemu/sysemu.h"
> >>   #include "hw/qdev-properties.h"
> >> -#include "hw/cpu/icc_bus.h"
> >>   #ifndef CONFIG_USER_ONLY
> >>   #include "exec/address-spaces.h"
> >>   #include "hw/xen/xen.h"
> >> @@ -2723,7 +2722,6 @@ static void mce_init(X86CPU *cpu)
> >>   #ifndef CONFIG_USER_ONLY
> >>   static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
> >>   {
> >> -    DeviceState *dev = DEVICE(cpu);
> >>       APICCommonState *apic;
> >>       const char *apic_type = "apic";
> >>   
> >> @@ -2733,11 +2731,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
> >>           apic_type = "xen-apic";
> >>       }
> >>   
> >> -    cpu->apic_state = qdev_try_create(qdev_get_parent_bus(dev), apic_type);
> >> -    if (cpu->apic_state == NULL) {
> >> -        error_setg(errp, "APIC device '%s' could not be created", apic_type);
> >> -        return;
> >> -    }
> >> +    cpu->apic_state = DEVICE(object_new(apic_type));
> >>   
> >>       object_property_add_child(OBJECT(cpu), "apic",
> >>                                 OBJECT(cpu->apic_state), NULL);
> >> @@ -3162,7 +3156,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;
> > .
> >
> 
>