Initialize all the parameters in one function init_topo_info.
Move the data structure X86CPUTopoIDs and X86CPUTopoInfo into
x86.h.
Signed-off-by: Babu Moger <babu.moger@amd.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
hw/i386/pc.c | 4 +---
hw/i386/x86.c | 14 +++-----------
include/hw/i386/topology.h | 26 ++++++++++----------------
include/hw/i386/x86.h | 17 +++++++++++++++++
4 files changed, 31 insertions(+), 30 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 2adf7f6afa..9803413dd9 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1749,9 +1749,7 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
return;
}
- topo_info.dies_per_pkg = x86ms->smp_dies;
- topo_info.cores_per_die = smp_cores;
- topo_info.threads_per_core = smp_threads;
+ init_topo_info(&topo_info, x86ms);
env->nr_dies = x86ms->smp_dies;
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index f18cab8e5c..083effb2f5 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -63,15 +63,12 @@ static size_t pvh_start_addr;
uint32_t x86_cpu_apic_id_from_index(X86MachineState *x86ms,
unsigned int cpu_index)
{
- MachineState *ms = MACHINE(x86ms);
X86MachineClass *x86mc = X86_MACHINE_GET_CLASS(x86ms);
X86CPUTopoInfo topo_info;
uint32_t correct_id;
static bool warned;
- topo_info.dies_per_pkg = x86ms->smp_dies;
- topo_info.cores_per_die = ms->smp.cores;
- topo_info.threads_per_core = ms->smp.threads;
+ init_topo_info(&topo_info, x86ms);
correct_id = x86_apicid_from_cpu_idx(&topo_info, cpu_index);
if (x86mc->compat_apic_id_mode) {
@@ -146,10 +143,7 @@ int64_t x86_get_default_cpu_node_id(const MachineState *ms, int idx)
X86MachineState *x86ms = X86_MACHINE(ms);
X86CPUTopoInfo topo_info;
- topo_info.dies_per_pkg = x86ms->smp_dies;
- topo_info.cores_per_die = ms->smp.cores;
- topo_info.threads_per_core = ms->smp.threads;
-
+ init_topo_info(&topo_info, x86ms);
assert(idx < ms->possible_cpus->len);
x86_topo_ids_from_apicid(ms->possible_cpus->cpus[idx].arch_id,
@@ -177,9 +171,7 @@ const CPUArchIdList *x86_possible_cpu_arch_ids(MachineState *ms)
sizeof(CPUArchId) * max_cpus);
ms->possible_cpus->len = max_cpus;
- topo_info.dies_per_pkg = x86ms->smp_dies;
- topo_info.cores_per_die = ms->smp.cores;
- topo_info.threads_per_core = ms->smp.threads;
+ init_topo_info(&topo_info, x86ms);
for (i = 0; i < ms->possible_cpus->len; i++) {
X86CPUTopoIDs topo_ids;
diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
index ba52d49079..ef0ab0b6a3 100644
--- a/include/hw/i386/topology.h
+++ b/include/hw/i386/topology.h
@@ -40,23 +40,17 @@
#include "qemu/bitops.h"
+#include "hw/i386/x86.h"
-/* 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;
+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;
+}
/* 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;
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
>
> Move the data structure X86CPUTopoIDs and X86CPUTopoInfo into
> x86.h.
A reason why it's moved should be here.
>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> hw/i386/pc.c | 4 +---
> hw/i386/x86.c | 14 +++-----------
> include/hw/i386/topology.h | 26 ++++++++++----------------
> include/hw/i386/x86.h | 17 +++++++++++++++++
> 4 files changed, 31 insertions(+), 30 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 2adf7f6afa..9803413dd9 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1749,9 +1749,7 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> return;
> }
>
> - topo_info.dies_per_pkg = x86ms->smp_dies;
> - topo_info.cores_per_die = smp_cores;
> - topo_info.threads_per_core = smp_threads;
> + init_topo_info(&topo_info, x86ms);
>
> env->nr_dies = x86ms->smp_dies;
>
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index f18cab8e5c..083effb2f5 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -63,15 +63,12 @@ static size_t pvh_start_addr;
> uint32_t x86_cpu_apic_id_from_index(X86MachineState *x86ms,
> unsigned int cpu_index)
> {
> - MachineState *ms = MACHINE(x86ms);
> X86MachineClass *x86mc = X86_MACHINE_GET_CLASS(x86ms);
> X86CPUTopoInfo topo_info;
> uint32_t correct_id;
> static bool warned;
>
> - topo_info.dies_per_pkg = x86ms->smp_dies;
> - topo_info.cores_per_die = ms->smp.cores;
> - topo_info.threads_per_core = ms->smp.threads;
> + init_topo_info(&topo_info, x86ms);
>
> correct_id = x86_apicid_from_cpu_idx(&topo_info, cpu_index);
> if (x86mc->compat_apic_id_mode) {
> @@ -146,10 +143,7 @@ int64_t x86_get_default_cpu_node_id(const MachineState *ms, int idx)
> X86MachineState *x86ms = X86_MACHINE(ms);
> X86CPUTopoInfo topo_info;
>
> - topo_info.dies_per_pkg = x86ms->smp_dies;
> - topo_info.cores_per_die = ms->smp.cores;
> - topo_info.threads_per_core = ms->smp.threads;
> -
> + init_topo_info(&topo_info, x86ms);
>
> assert(idx < ms->possible_cpus->len);
> x86_topo_ids_from_apicid(ms->possible_cpus->cpus[idx].arch_id,
> @@ -177,9 +171,7 @@ const CPUArchIdList *x86_possible_cpu_arch_ids(MachineState *ms)
> sizeof(CPUArchId) * max_cpus);
> ms->possible_cpus->len = max_cpus;
>
> - topo_info.dies_per_pkg = x86ms->smp_dies;
> - topo_info.cores_per_die = ms->smp.cores;
> - topo_info.threads_per_core = ms->smp.threads;
> + init_topo_info(&topo_info, x86ms);
>
> for (i = 0; i < ms->possible_cpus->len; i++) {
> X86CPUTopoIDs topo_ids;
> diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
> index ba52d49079..ef0ab0b6a3 100644
> --- a/include/hw/i386/topology.h
> +++ b/include/hw/i386/topology.h
> @@ -40,23 +40,17 @@
>
>
> #include "qemu/bitops.h"
> +#include "hw/i386/x86.h"
>
> -/* 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;
> +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;
> +}
>
> /* 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;
>
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
>
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>> ---
>> hw/i386/pc.c | 4 +---
>> hw/i386/x86.c | 14 +++-----------
>> include/hw/i386/topology.h | 26 ++++++++++----------------
>> include/hw/i386/x86.h | 17 +++++++++++++++++
>> 4 files changed, 31 insertions(+), 30 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 2adf7f6afa..9803413dd9 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1749,9 +1749,7 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>> return;
>> }
>>
>> - topo_info.dies_per_pkg = x86ms->smp_dies;
>> - topo_info.cores_per_die = smp_cores;
>> - topo_info.threads_per_core = smp_threads;
>> + init_topo_info(&topo_info, x86ms);
>>
>> env->nr_dies = x86ms->smp_dies;
>>
>> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
>> index f18cab8e5c..083effb2f5 100644
>> --- a/hw/i386/x86.c
>> +++ b/hw/i386/x86.c
>> @@ -63,15 +63,12 @@ static size_t pvh_start_addr;
>> uint32_t x86_cpu_apic_id_from_index(X86MachineState *x86ms,
>> unsigned int cpu_index)
>> {
>> - MachineState *ms = MACHINE(x86ms);
>> X86MachineClass *x86mc = X86_MACHINE_GET_CLASS(x86ms);
>> X86CPUTopoInfo topo_info;
>> uint32_t correct_id;
>> static bool warned;
>>
>> - topo_info.dies_per_pkg = x86ms->smp_dies;
>> - topo_info.cores_per_die = ms->smp.cores;
>> - topo_info.threads_per_core = ms->smp.threads;
>> + init_topo_info(&topo_info, x86ms);
>>
>> correct_id = x86_apicid_from_cpu_idx(&topo_info, cpu_index);
>> if (x86mc->compat_apic_id_mode) {
>> @@ -146,10 +143,7 @@ int64_t x86_get_default_cpu_node_id(const MachineState *ms, int idx)
>> X86MachineState *x86ms = X86_MACHINE(ms);
>> X86CPUTopoInfo topo_info;
>>
>> - topo_info.dies_per_pkg = x86ms->smp_dies;
>> - topo_info.cores_per_die = ms->smp.cores;
>> - topo_info.threads_per_core = ms->smp.threads;
>> -
>> + init_topo_info(&topo_info, x86ms);
>>
>> assert(idx < ms->possible_cpus->len);
>> x86_topo_ids_from_apicid(ms->possible_cpus->cpus[idx].arch_id,
>> @@ -177,9 +171,7 @@ const CPUArchIdList *x86_possible_cpu_arch_ids(MachineState *ms)
>> sizeof(CPUArchId) * max_cpus);
>> ms->possible_cpus->len = max_cpus;
>>
>> - topo_info.dies_per_pkg = x86ms->smp_dies;
>> - topo_info.cores_per_die = ms->smp.cores;
>> - topo_info.threads_per_core = ms->smp.threads;
>> + init_topo_info(&topo_info, x86ms);
>>
>> for (i = 0; i < ms->possible_cpus->len; i++) {
>> X86CPUTopoIDs topo_ids;
>> diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
>> index ba52d49079..ef0ab0b6a3 100644
>> --- a/include/hw/i386/topology.h
>> +++ b/include/hw/i386/topology.h
>> @@ -40,23 +40,17 @@
>>
>>
>> #include "qemu/bitops.h"
>> +#include "hw/i386/x86.h"
>>
>> -/* 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;
>> +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;
>> +}
>>
>> /* 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;
>>
>
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
[...]
> >> +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.
> >>
> >> /* 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;
> >>
> >
>
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;
>>>>
>>>
>>
>
© 2016 - 2026 Red Hat, Inc.