On 2/24/20 2:18 AM, Igor Mammedov wrote:
> On Fri, 21 Feb 2020 11:51:15 -0600
> Babu Moger <babu.mo...@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.mo...@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;
>>>>
>>>
>>
>