[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 04/16] hw/i386: Introduce init_topo_info to initialize X86
From: |
Babu Moger |
Subject: |
Re: [PATCH v4 04/16] hw/i386: Introduce init_topo_info to initialize X86CPUTopoInfo |
Date: |
Mon, 24 Feb 2020 10:54:16 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 |
On 2/24/20 2:18 AM, Igor Mammedov wrote:
> On Fri, 21 Feb 2020 11:51:15 -0600
> Babu Moger <address@hidden> wrote:
>
>> On 2/21/20 11:05 AM, Igor Mammedov wrote:
>>> On Thu, 13 Feb 2020 12:16:51 -0600
>>> Babu Moger <address@hidden> 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;
>>>>
>>>
>>
>
- [PATCH v4 00/16] APIC ID fixes for AMD EPYC CPU model, Babu Moger, 2020/02/13
- [PATCH v4 01/16] hw/i386: Rename X86CPUTopoInfo structure to X86CPUTopoIDs, Babu Moger, 2020/02/13
- [PATCH v4 02/16] hw/i386: Introduce X86CPUTopoInfo to contain topology info, Babu Moger, 2020/02/13
- [PATCH v4 03/16] hw/i386: Consolidate topology functions, Babu Moger, 2020/02/13
- [PATCH v4 04/16] hw/i386: Introduce init_topo_info to initialize X86CPUTopoInfo, Babu Moger, 2020/02/13
- [PATCH v4 05/16] machine: Add SMP Sockets in CpuTopology, Babu Moger, 2020/02/13
- [PATCH v4 06/16] hw/i386: Update structures for nodes_per_pkg, Babu Moger, 2020/02/13
- [PATCH v4 07/16] hw/i386: Rename apicid_from_topo_ids to x86_apicid_from_topo_ids, Babu Moger, 2020/02/13
- [PATCH v4 08/16] hw/386: Add EPYC mode topology decoding functions, Babu Moger, 2020/02/13