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

[Andreas Färber]

Am 25.06.2015 um 04:17 schrieb Zhu Guihua:
> From: Chen Fan <address@hidden>
> 
> 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 <address@hidden>
> Signed-off-by: Zhu Guihua <address@hidden>
> ---
>  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)