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

[Babu Moger]

On 10/10/19 9:29 PM, Eduardo Habkost wrote:
> 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 <address@hidden>
>> ---
>>  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.

Yes. Separated the patches and added the new fields separately.

> 
> 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.
>