Re: [PATCH v4 04/16] hw/i386: Introduce init_topo_info to initialize X86CPUTopoInfo

From: Babu Moger <babu.moger@amd.com>
To: Igor Mammedov <imammedo@redhat.com>
Cc: ehabkost@redhat.com, mst@redhat.com, qemu-devel@nongnu.org,
	pbonzini@redhat.com, rth@twiddle.net
Subject: Re: [PATCH v4 04/16] hw/i386: Introduce init_topo_info to initialize X86CPUTopoInfo
Date: Mon, 24 Feb 2020 10:54:16 -0600	[thread overview]
Message-ID: <47d531cb-1f52-f9b4-dcf2-24f119baadff@amd.com> (raw)
In-Reply-To: <20200224091833.2d3dec7f@redhat.com>

On 2/24/20 2:18 AM, Igor Mammedov wrote:
> On Fri, 21 Feb 2020 11:51:15 -0600
> Babu Moger <babu.moger@amd.com> wrote:
> 
>> On 2/21/20 11:05 AM, Igor Mammedov wrote:
>>> On Thu, 13 Feb 2020 12:16:51 -0600
>>> Babu Moger <babu.moger@amd.com> wrote:
>>>   
>>>> Initialize all the parameters in one function init_topo_info.  
>>>
>>> is it possible to squash it in 2/16
>>>   
>> Sure. We can do that.
>>>   
>>>>
>>>> Move the data structure X86CPUTopoIDs and X86CPUTopoInfo into
>>>> x86.h.  
>>> A reason why it's moved should be here.  
>>
>> Apicid functions will be part of X86MachineState data structure(patches
>> introduced later).These functions will use X86CPUTopoIDs and
>> X86CPUTopoInfo definition. Will add these details. Thanks
> 
> why not just include topology.h into the X86MachineState header,
> and keep topo structures/functions where they are now?
> (I dislike a little scattering consolidated pieces across multiple files,
> but what worries me more is that it makes target/i386/cpu.c via
> topology.h -> x86.h chain pull in a lot of unrelated dependencies)
> 
> So I'd keep X86CPUTopoIDs and X86CPUTopoInfo in topology.h 

Ok. Sure. we can do that.

> 
> [...]
>>>> +static inline void init_topo_info(X86CPUTopoInfo *topo_info,
>>>> +                                  const X86MachineState *x86ms)
>>>> +{
>>>> +    MachineState *ms = MACHINE(x86ms);
>>>> +
>>>> +    topo_info->dies_per_pkg = x86ms->smp_dies;
>>>> +    topo_info->cores_per_die = ms->smp.cores;
>>>> +    topo_info->threads_per_core = ms->smp.threads;
>>>> +}
> 
> this is pure machine specific helper, and aren't used anywhere else
> beside machine code.
> Suggest to put it in pc.c or x86.c to keep topology.h machine independent.

Ok. Will do.

> 
>>>>  
>>>>  /* Return the bit width needed for 'count' IDs
>>>>   */
>>>> diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
>>>> index 4b84917885..ad62b01cf2 100644
>>>> --- a/include/hw/i386/x86.h
>>>> +++ b/include/hw/i386/x86.h
>>>> @@ -36,6 +36,23 @@ typedef struct {
>>>>      bool compat_apic_id_mode;
>>>>  } X86MachineClass;
>>>>  
>>>> +/* APIC IDs can be 32-bit, but beware: APIC IDs > 255 require x2APIC support
>>>> + */
>>>> +typedef uint32_t apic_id_t;
>>>> +
>>>> +typedef struct X86CPUTopoIDs {
>>>> +    unsigned pkg_id;
>>>> +    unsigned die_id;
>>>> +    unsigned core_id;
>>>> +    unsigned smt_id;
>>>> +} X86CPUTopoIDs;
>>>> +
>>>> +typedef struct X86CPUTopoInfo {
>>>> +    unsigned dies_per_pkg;
>>>> +    unsigned cores_per_die;
>>>> +    unsigned threads_per_core;
>>>> +} X86CPUTopoInfo;
>>>> +
>>>>  typedef struct {
>>>>      /*< private >*/
>>>>      MachineState parent;
>>>>  
>>>   
>>
> 