Re: [Qemu-devel] [RFC v3 2/3] i386: use CpuTopoInfo instead apic_id as argument for pc_new_cpu()

[Eduardo Habkost]

On Tue, Mar 11, 2014 at 06:58:53PM +0800, chen.fan.fnst wrote:
> From: "chen.fan.fnst" <address@hidden>
> 
> Signed-off-by: Chen Fan <address@hidden>
> ---
>  hw/i386/pc.c      | 12 ++++++++----
>  target-i386/cpu.c |  2 --
>  target-i386/cpu.h |  3 +++
>  3 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index e715a33..c754042 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -927,11 +927,12 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int 
> level)
>      }
>  }
>  
> -static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
> +static X86CPU *pc_new_cpu(const char *cpu_model, CpuTopoInfo *topo,
>                            DeviceState *icc_bridge, Error **errp)
>  {
>      X86CPU *cpu;
>      Error *local_err = NULL;
> +    uint32_t apic_id = apicid_from_topo_ids(smp_cores, smp_threads, topo);
>  
>      cpu = cpu_x86_create(cpu_model, icc_bridge, &local_err);
>      if (local_err != NULL) {
> @@ -956,6 +957,7 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
>  {
>      DeviceState *icc_bridge;
>      int64_t apic_id = x86_cpu_apic_id_from_index(id);

With this patch, pc_new_cpu() is CPUTopoInfo-based, but cpu_exists() is
apic_id-based. This requires apic_id to be calculated here, but
pc_new_cpu() doesn't require it anymore.

We could make this more consistent, and avoid calculating the APIC ID in
two different places. We are eveng using two different methods: here we
use x86_cpu_apic_id_from_index() and pc_new_cpu() is using
apicid_from_topo_ids().

Maybe we could simply move the cpu_exists() check inside pc_new_cpu()?


> +    CpuTopoInfo topo;
>  
>      if (id < 0) {
>          error_setg(errp, "Invalid CPU id: %" PRIi64, id);
> @@ -976,7 +978,8 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
>  
>      icc_bridge = DEVICE(object_resolve_path_type("icc-bridge",
>                                                   TYPE_ICC_BRIDGE, NULL));
> -    pc_new_cpu(current_cpu_model, apic_id, icc_bridge, errp);
> +    x86_topo_ids_from_idx(smp_cores, smp_threads, id, &topo);
> +    pc_new_cpu(current_cpu_model, &topo, icc_bridge, errp);
>  }
>  
>  void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
> @@ -996,8 +999,9 @@ void pc_cpus_init(const char *cpu_model, DeviceState 
> *icc_bridge)
>      current_cpu_model = cpu_model;
>  
>      for (i = 0; i < smp_cpus; i++) {
> -        cpu = pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i),
> -                         icc_bridge, &error);

x86_cpu_apic_id_from_index() has compat code to keep compatibility on
older machine-types, and you are removing the compat behavior.

To keep the compat code isolated on pc_cpus_init() and keep pc_new_cpu()
interface saner, we could do something like this, on pc_cpus_init()
only:

    CPUTopoInfo topo;
    x86_topo_ids_from_idx(cpu_index, &topo);
    if (apic_id_compat) {
        correct_apic_id = apicid_from_topo_ids(&topo);
        apic_id = cpu_index;
        if (apic_id != correct_apic_id) {
            error_report("APIC IDs set in compatibility mode, "
                         "CPU topology won't match the configuration");
        }
        // get_topo_from_apic_id() needs to be written:
        get_topo_from_apic_id(apic_id, &topo);
        // just in case:
        assert(apicid_from_topo_ids(&topo) == apic_id);
    }

And on pc_hot_add_cpu():

    if (apic_id_compat) {
        error_setg(errp, "CPU hotplug is not supported on this machine");
        return;
    }



> +        CpuTopoInfo topo;
> +        x86_topo_ids_from_idx(smp_cores, smp_threads, i, &topo);
> +        cpu = pc_new_cpu(cpu_model, &topo, icc_bridge, &error);
>          if (error) {
>              error_report("%s", error_get_pretty(error));
>              error_free(error);
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 0e8812a..3a5df33 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -23,8 +23,6 @@
>  
>  #include "cpu.h"
>  #include "sysemu/kvm.h"
> -#include "sysemu/cpus.h"
> -#include "topology.h"
>  
>  #include "qemu/option.h"
>  #include "qemu/config-file.h"
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 0014acc..8bc7b45 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -22,6 +22,9 @@
>  #include "config.h"
>  #include "qemu-common.h"
>  
> +#include "sysemu/cpus.h"
> +#include "topology.h"
> +
>  #ifdef TARGET_X86_64
>  #define TARGET_LONG_BITS 64
>  #else
> -- 
> 1.8.1.4
> 

-- 
Eduardo