Re: [Qemu-devel] [RFC 2 PATCH 03/16] hw/i386: Introduce X86CPUTopoInfo to contain topology info

[Eduardo Habkost <ehabkost@redhat.com>]

On Fri, Sep 06, 2019 at 07:11:57PM +0000, Moger, Babu wrote:
> This is an effort to re-arrange few data structure for better
> readability. Add X86CPUTopoInfo which will have all the topology
> informations required to build the cpu topology. There is no
> functional changes.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  hw/i386/pc.c               |   40 +++++++++++++++++++++++++++-------------
>  include/hw/i386/topology.h |   40 ++++++++++++++++++++++++++--------------
>  2 files changed, 53 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index ada445f8f3..95aab8e5e7 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -930,11 +930,15 @@ static uint32_t x86_cpu_apic_id_from_index(PCMachineState *pcms,
>  {
>      MachineState *ms = MACHINE(pcms);
>      PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> +    X86CPUTopoInfo topo_info;
>      uint32_t correct_id;
>      static bool warned;
>  
> -    correct_id = x86_apicid_from_cpu_idx(pcms->smp_dies, ms->smp.cores,
> -                                         ms->smp.threads, cpu_index);
> +    topo_info.nr_dies = pcms->smp_dies;
> +    topo_info.nr_cores = ms->smp.cores;
> +    topo_info.nr_threads = ms->smp.threads;
> +
> +    correct_id = x86_apicid_from_cpu_idx(&topo_info, cpu_index);

If you are using the struct in function calls, please make sure
all fields are filled correctly, so we won't introduce bugs
accidentally if we start using the new fields inside the topology
functions.

Alternatively, you can leave the struct without the numa_nodes
and nr_sockets fields by now (because they are unused), and add
the fields in another patch.

Except for this, the patch looks good.  However, I don't
understand yet the use case for the `numa_nodes` field yet.  I
will probably comment on the `numa_nodes` field once I see the
patches that use the new field.

-- 
Eduardo