Am 25.06.2015 um 04:17 schrieb Zhu Guihua:
> From: Chen Fan <chen.fan.f...@cn.fujitsu.com>
>
> After CPU hotplug has been converted to BUS-less hot-plug infrastructure,
> the only function ICC bus performs is to propagate reset to LAPICs. However
> LAPIC could be reset by registering its reset handler after all device are
> initialized.
> Do so and drop ~200LOC of not needed anymore ICCBus related code.
>
> Signed-off-by: Chen Fan <chen.fan.f...@cn.fujitsu.com>
> Signed-off-by: Zhu Guihua <zhugh.f...@cn.fujitsu.com>
> ---
> hw/i386/pc.c | 24 +++++++++---------------
> hw/i386/pc_piix.c | 9 +--------
> hw/i386/pc_q35.c | 9 +--------
> hw/intc/apic_common.c | 5 ++---
> include/hw/i386/apic_internal.h | 7 ++++---
> include/hw/i386/pc.h | 2 +-
> target-i386/cpu.c | 23 +++++++++++++++--------
> target-i386/cpu.h | 1 +
> 8 files changed, 34 insertions(+), 46 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 9f16128..c547d74 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -59,7 +59,6 @@
> #include "qemu/error-report.h"
> #include "hw/acpi/acpi.h"
> #include "hw/acpi/cpu_hotplug.h"
> -#include "hw/cpu/icc_bus.h"
> #include "hw/boards.h"
> #include "hw/pci/pci_host.h"
> #include "acpi-build.h"
> @@ -969,27 +968,25 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int
> level)
> }
> }
>
> +#define x86_cpu_apic_reset_order 0x1
> +
> static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
> - DeviceState *icc_bridge, Error **errp)
> + Error **errp)
> {
> X86CPU *cpu = NULL;
> Error *local_err = NULL;
>
> - if (icc_bridge == NULL) {
> - error_setg(&local_err, "Invalid icc-bridge value");
> - goto out;
> - }
> -
> cpu = cpu_x86_create(cpu_model, &local_err);
> if (local_err != NULL) {
> goto out;
> }
>
> - qdev_set_parent_bus(DEVICE(cpu), qdev_get_child_bus(icc_bridge, "icc"));
> -
> object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
> object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
>
> + qemu_register_reset_common(x86_cpu_apic_reset, cpu,
> + x86_cpu_apic_reset_order);
This usage of an out-of-file reset handler with opaque argument is a
little ugly.
> +
> out:
> if (local_err) {
> error_propagate(errp, local_err);
> @@ -1003,7 +1000,6 @@ static const char *current_cpu_model;
>
> void pc_hot_add_cpu(const int64_t id, Error **errp)
> {
> - DeviceState *icc_bridge;
> X86CPU *cpu;
> int64_t apic_id = x86_cpu_apic_id_from_index(id);
> Error *local_err = NULL;
> @@ -1032,9 +1028,7 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
> return;
> }
>
> - icc_bridge = DEVICE(object_resolve_path_type("icc-bridge",
> - TYPE_ICC_BRIDGE, NULL));
> - cpu = pc_new_cpu(current_cpu_model, apic_id, icc_bridge, &local_err);
> + cpu = pc_new_cpu(current_cpu_model, apic_id, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> return;
> @@ -1042,7 +1036,7 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
> object_unref(OBJECT(cpu));
> }
>
> -void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
> +void pc_cpus_init(const char *cpu_model)
> {
> int i;
> X86CPU *cpu = NULL;
> @@ -1068,7 +1062,7 @@ void pc_cpus_init(const char *cpu_model, DeviceState
> *icc_bridge)
>
> for (i = 0; i < smp_cpus; i++) {
> cpu = pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i),
> - icc_bridge, &error);
> + &error);
> if (error) {
> error_report_err(error);
> exit(1);
[...]
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 1fb88f6..e169ce3 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -43,7 +43,6 @@
>
> #include "sysemu/sysemu.h"
> #include "hw/qdev-properties.h"
> -#include "hw/cpu/icc_bus.h"
> #ifndef CONFIG_USER_ONLY
> #include "exec/address-spaces.h"
> #include "hw/xen/xen.h"
> @@ -2719,7 +2718,6 @@ static void mce_init(X86CPU *cpu)
> #ifndef CONFIG_USER_ONLY
> static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
> {
> - DeviceState *dev = DEVICE(cpu);
> APICCommonState *apic;
> const char *apic_type = "apic";
>
> @@ -2729,11 +2727,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error
> **errp)
> apic_type = "xen-apic";
> }
>
> - cpu->apic_state = qdev_try_create(qdev_get_parent_bus(dev), apic_type);
> - if (cpu->apic_state == NULL) {
> - error_setg(errp, "APIC device '%s' could not be created", apic_type);
> - return;
> - }
> + cpu->apic_state = DEVICE(object_new(apic_type));
>
> object_property_add_child(OBJECT(cpu), "apic",
> OBJECT(cpu->apic_state), NULL);
> @@ -2754,6 +2748,20 @@ static void x86_cpu_apic_realize(X86CPU *cpu, Error
> **errp)
> errp);
> }
>
> +void x86_cpu_apic_reset(void *opaque)
> +{
> + X86CPU *cpu = opaque;
> + DeviceClass *dc;
> +
> + if (cpu->apic_state) {
> + dc = DEVICE_GET_CLASS(cpu->apic_state);
> +
> + if (dc->reset != NULL) {
> + (*dc->reset)(cpu->apic_state);
This is the same as dc->reset(cpu->apic_state); ...
> + }
... which makes this the same as device_reset(cpu->apic_state);.
Why does this need to be a reset handler of its own?
> + }
> +}
> +
> static void x86_cpu_machine_done(Notifier *n, void *unused)
> {
> X86CPU *cpu = container_of(n, X86CPU, machine_done);
> @@ -3136,7 +3144,6 @@ static void x86_cpu_common_class_init(ObjectClass *oc,
> void *data)
>
> xcc->parent_realize = dc->realize;
> dc->realize = x86_cpu_realizefn;
> - dc->bus_type = TYPE_ICC_BUS;
> dc->props = x86_cpu_properties;
>
> xcc->parent_reset = cc->reset;
[snip]
More comments on the preceding patch.
Regards,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)
