Re: [Qemu-devel] [PATCH qom-cpu v2 24/29] cpu: Drop qemu_for_each_cpu()

[Michael S. Tsirkin]

On Sun, Jun 16, 2013 at 05:57:44PM +0200, Andreas Färber wrote:
> Revert commit d6b9e0d60cc511eca210834428bb74508cff3d33 (cpu: Add
> qemu_for_each_cpu()) and its usage in favor of open-coding CPU loops,
> now that they are based on CPUState.
> 
> Suggested-by: Markus Armbruster <address@hidden>
> Signed-off-by: Andreas Färber <address@hidden>

Open-coding is kind of nasty though.

How about
#define qemu_for_each_cpu(cpu) \
    for (cpu = first_cpu; cpu != NULL; cpu = cpu->next_cpu)

> ---
>  cpus.c            | 11 ++++-------
>  exec.c            | 11 -----------
>  hw/acpi/piix4.c   | 20 +++++++++-----------
>  include/qom/cpu.h |  9 ---------
>  qom/cpu.c         | 30 +++++++++---------------------
>  5 files changed, 22 insertions(+), 59 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index ec38644..691c435 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -803,12 +803,6 @@ static void *qemu_dummy_cpu_thread_fn(void *arg)
>  
>  static void tcg_exec_all(void);
>  
> -static void tcg_signal_cpu_creation(CPUState *cpu, void *data)
> -{
> -    cpu->thread_id = qemu_get_thread_id();
> -    cpu->created = true;
> -}
> -
>  static void *qemu_tcg_cpu_thread_fn(void *arg)
>  {
>      CPUState *cpu = arg;
> @@ -817,7 +811,10 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
>      qemu_thread_get_self(cpu->thread);
>  
>      qemu_mutex_lock(&qemu_global_mutex);
> -    qemu_for_each_cpu(tcg_signal_cpu_creation, NULL);
> +    for (cpu = first_cpu; cpu != NULL; cpu = cpu->next_cpu) {
> +        cpu->thread_id = qemu_get_thread_id();
> +        cpu->created = true;
> +    }
>      qemu_cond_signal(&qemu_cpu_cond);
>  
>      /* wait for initial kick-off after machine start */
> diff --git a/exec.c b/exec.c
> index 191eb4e..5f3aba1 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -276,17 +276,6 @@ CPUState *qemu_get_cpu(int index)
>      return cpu;
>  }
>  
> -void qemu_for_each_cpu(void (*func)(CPUState *cpu, void *data), void *data)
> -{
> -    CPUState *cpu;
> -
> -    cpu = first_cpu;
> -    while (cpu) {
> -        func(cpu, data);
> -        cpu = cpu->next_cpu;
> -    }
> -}
> -
>  void cpu_exec_init(CPUArchState *env)
>  {
>      CPUState *cpu = ENV_GET_CPU(env);
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 756df3b..3234452 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -654,22 +654,14 @@ static void piix4_cpu_added_req(Notifier *n, void 
> *opaque)
>      piix4_cpu_hotplug_req(s, CPU(opaque), PLUG);
>  }
>  
> -static void piix4_init_cpu_status(CPUState *cpu, void *data)
> -{
> -    CPUStatus *g = (CPUStatus *)data;
> -    CPUClass *k = CPU_GET_CLASS(cpu);
> -    int64_t id = k->get_arch_id(cpu);
> -
> -    g_assert((id / 8) < PIIX4_PROC_LEN);
> -    g->sts[id / 8] |= (1 << (id % 8));
> -}
> -
>  static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
>                                  PCIHotplugState state);
>  
>  static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
>                                             PCIBus *bus, PIIX4PMState *s)
>  {
> +    CPUState *cpu;
> +
>      memory_region_init_io(&s->io_gpe, &piix4_gpe_ops, s, "apci-gpe0",
>                            GPE_LEN);
>      memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
> @@ -680,7 +672,13 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion 
> *parent,
>                                  &s->io_pci);
>      pci_bus_hotplug(bus, piix4_device_hotplug, &s->dev.qdev);
>  
> -    qemu_for_each_cpu(piix4_init_cpu_status, &s->gpe_cpu);
> +    for (cpu = first_cpu; cpu != NULL; cpu = cpu->next_cpu) {
> +        CPUClass *cc = CPU_GET_CLASS(cpu);
> +        int64_t id = cc->get_arch_id(cpu);
> +
> +        g_assert((id / 8) < PIIX4_PROC_LEN);
> +        s->gpe_cpu.sts[id / 8] |= (1 << (id % 8));
> +    }
>      memory_region_init_io(&s->io_cpu, &cpu_hotplug_ops, s, 
> "apci-cpu-hotplug",
>                            PIIX4_PROC_LEN);
>      memory_region_add_subregion(parent, PIIX4_PROC_BASE, &s->io_cpu);
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 2a64af2..467896c 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -356,15 +356,6 @@ bool cpu_is_stopped(CPUState *cpu);
>  void run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data);
>  
>  /**
> - * qemu_for_each_cpu:
> - * @func: The function to be executed.
> - * @data: Data to pass to the function.
> - *
> - * Executes @func for each CPU.
> - */
> -void qemu_for_each_cpu(void (*func)(CPUState *cpu, void *data), void *data);
> -
> -/**
>   * qemu_get_cpu:
>   * @index: The address@hidden value of the CPU to obtain.
>   *
> diff --git a/qom/cpu.c b/qom/cpu.c
> index ee8f632..94fa2b8 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -24,30 +24,18 @@
>  #include "qemu/notify.h"
>  #include "sysemu/sysemu.h"
>  
> -typedef struct CPUExistsArgs {
> -    int64_t id;
> -    bool found;
> -} CPUExistsArgs;
> -
> -static void cpu_exist_cb(CPUState *cpu, void *data)
> -{
> -    CPUClass *klass = CPU_GET_CLASS(cpu);
> -    CPUExistsArgs *arg = data;
> -
> -    if (klass->get_arch_id(cpu) == arg->id) {
> -        arg->found = true;
> -    }
> -}
> -
>  bool cpu_exists(int64_t id)
>  {
> -    CPUExistsArgs data = {
> -        .id = id,
> -        .found = false,
> -    };
> +    CPUState *cpu;
> +
> +    for (cpu = first_cpu; cpu != NULL; cpu = cpu->next_cpu) {
> +        CPUClass *cc = CPU_GET_CLASS(cpu);
>  
> -    qemu_for_each_cpu(cpu_exist_cb, &data);
> -    return data.found;
> +        if (cc->get_arch_id(cpu) == id) {
> +            return true;
> +        }
> +    }
> +    return false;
>  }
>  
>  bool cpu_paging_enabled(const CPUState *cpu)
> -- 
> 1.8.1.4