diff mbox series

[RFC,09/52] hw/machine: Introduce core type for hybrid topology

Message ID 20230213095035.158240-10-zhao1.liu@linux.intel.com
State New
Headers show
Series Introduce hybrid CPU topology | expand

Commit Message

Zhao Liu Feb. 13, 2023, 9:49 a.m. UTC 
From: Zhao Liu <zhao1.liu@intel.com>

Under the hybrid cpu topology, some CPUs care about the core type.
For example, Intel's Alder Lake series CPU contains two types of cores:
Intel Core and Intel Atom. The type information of these two types is
exposed in 1A leaf of CPUID.

Core type should also be part of the hybrid topology, and
MachineState.cpu_type cannot provide different type information for
different cpus in the same machine, so add a type field for the core
level in the hybrid cpu topology.

Additionally, add a helper to get core type information from
MachineState. Though core_type is only used in hybrid case, don't
use assert since it may be used to initialize some generic fields.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/core/machine-topo.c        | 12 ++++++++++++
 include/hw/boards.h           |  3 +++
 include/hw/cpu/cpu-topology.h |  2 ++
 3 files changed, 17 insertions(+)

Comments

Philippe Mathieu-Daudé Feb. 13, 2023, 1:13 p.m. UTC | #1 
On 13/2/23 10:49, Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> Under the hybrid cpu topology, some CPUs care about the core type.
> For example, Intel's Alder Lake series CPU contains two types of cores:
> Intel Core and Intel Atom. The type information of these two types is
> exposed in 1A leaf of CPUID.
> 
> Core type should also be part of the hybrid topology, and
> MachineState.cpu_type cannot provide different type information for
> different cpus in the same machine, so add a type field for the core
> level in the hybrid cpu topology.
> 
> Additionally, add a helper to get core type information from
> MachineState. Though core_type is only used in hybrid case, don't
> use assert since it may be used to initialize some generic fields.
> 
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>   hw/core/machine-topo.c        | 12 ++++++++++++
>   include/hw/boards.h           |  3 +++
>   include/hw/cpu/cpu-topology.h |  2 ++
>   3 files changed, 17 insertions(+)
> 
> diff --git a/hw/core/machine-topo.c b/hw/core/machine-topo.c
> index b20160479629..e0ec07b53d42 100644
> --- a/hw/core/machine-topo.c
> +++ b/hw/core/machine-topo.c
> @@ -51,6 +51,18 @@ unsigned int machine_topo_get_smp_threads(const MachineState *ms)
>       return ms->topo.smp.threads;
>   }
>   
> +unsigned int machine_topo_get_hybrid_core_type(const MachineState *ms,
> +                                               unsigned int cluster_id,
> +                                               unsigned int core_id)
> +{
> +    if (!machine_topo_is_smp(ms)) {
> +        return ms->topo.hybrid.cluster_list[cluster_id]
> +                       .core_list[core_id].core_type;
> +    } else {
> +        return 0;

Is '0' an invalid type?

> +    }
> +}
Mi, Dapeng1 Feb. 14, 2023, 1:14 a.m. UTC | #2 
> From: Zhao Liu <zhao1.liu@linux.intel.com>
> Sent: Monday, February 13, 2023 5:50 PM
> To: Eduardo Habkost <eduardo@habkost.net>; Marcel Apfelbaum
> <marcel.apfelbaum@gmail.com>; Philippe Mathieu-Daudé <philmd@linaro.org>;
> Yanan Wang <wangyanan55@huawei.com>; Michael S . Tsirkin
> <mst@redhat.com>; Richard Henderson <richard.henderson@linaro.org>; Paolo
> Bonzini <pbonzini@redhat.com>; Eric Blake <eblake@redhat.com>; Markus
> Armbruster <armbru@redhat.com>
> Cc: qemu-devel@nongnu.org; Wang, Zhenyu Z <zhenyu.z.wang@intel.com>; Mi,
> Dapeng1 <dapeng1.mi@intel.com>; Ding, Zhuocheng
> <zhuocheng.ding@intel.com>; Robert Hoo <robert.hu@linux.intel.com>;
> Christopherson,, Sean <seanjc@google.com>; Like Xu
> <like.xu.linux@gmail.com>; Liu, Zhao1 <zhao1.liu@intel.com>
> Subject: [RFC 09/52] hw/machine: Introduce core type for hybrid topology
> 
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> Under the hybrid cpu topology, some CPUs care about the core type.
> For example, Intel's Alder Lake series CPU contains two types of cores:
> Intel Core and Intel Atom. The type information of these two types is exposed in
> 1A leaf of CPUID.
> 
> Core type should also be part of the hybrid topology, and
> MachineState.cpu_type cannot provide different type information for different
> cpus in the same machine, so add a type field for the core level in the hybrid cpu
> topology.
> 
> Additionally, add a helper to get core type information from MachineState.
> Though core_type is only used in hybrid case, don't use assert since it may be
> used to initialize some generic fields.
> 
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>  hw/core/machine-topo.c        | 12 ++++++++++++
>  include/hw/boards.h           |  3 +++
>  include/hw/cpu/cpu-topology.h |  2 ++
>  3 files changed, 17 insertions(+)
> 
> diff --git a/hw/core/machine-topo.c b/hw/core/machine-topo.c index
> b20160479629..e0ec07b53d42 100644
> --- a/hw/core/machine-topo.c
> +++ b/hw/core/machine-topo.c
> @@ -51,6 +51,18 @@ unsigned int machine_topo_get_smp_threads(const
> MachineState *ms)
>      return ms->topo.smp.threads;
>  }
> 
> +unsigned int machine_topo_get_hybrid_core_type(const MachineState *ms,
> +                                               unsigned int cluster_id,
> +                                               unsigned int core_id) {
> +    if (!machine_topo_is_smp(ms)) {
> +        return ms->topo.hybrid.cluster_list[cluster_id]
> +                       .core_list[core_id].core_type;
> +    } else {
> +        return 0;
> +    }
> +}
> +

We'd better not to return the hard-coded '0'. Suggest to define a 'enum'
data structure to represent the core_type. This makes the code look more intuitively.

>  unsigned int machine_topo_get_threads(const MachineState *ms,
>                                        unsigned int cluster_id,
>                                        unsigned int core_id) diff --git a/include/hw/boards.h
> b/include/hw/boards.h index 34b64b012022..78e52af38cb1 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -484,6 +484,9 @@ unsigned int machine_topo_get_dies(const
> MachineState *ms);  unsigned int machine_topo_get_clusters(const
> MachineState *ms);  unsigned int machine_topo_get_smp_cores(const
> MachineState *ms);  unsigned int machine_topo_get_smp_threads(const
> MachineState *ms);
> +unsigned int machine_topo_get_hybrid_core_type(const MachineState *ms,
> +                                               unsigned int cluster_id,
> +                                               unsigned int core_id);
>  unsigned int machine_topo_get_threads(const MachineState *ms,
>                                        unsigned int cluster_id,
>                                        unsigned int core_id); diff --git a/include/hw/cpu/cpu-
> topology.h b/include/hw/cpu/cpu-topology.h index
> 8268ea3a8569..87d832556229 100644
> --- a/include/hw/cpu/cpu-topology.h
> +++ b/include/hw/cpu/cpu-topology.h
> @@ -45,9 +45,11 @@ typedef struct SmpCpuTopology {
>  /**
>   * HybridCore - hybrid core topology defination:
>   * @threads: the number of threads in one core.
> + * @core_type: the type of current core.
>   */
>  typedef struct HybridCore {
>      unsigned int threads;
> +    unsigned int core_type;
>  } HybridCore;
> 
>  /**
> --
> 2.34.1
Zhao Liu Feb. 14, 2023, 9:41 a.m. UTC | #3 
On Mon, Feb 13, 2023 at 02:13:40PM +0100, Philippe Mathieu-Daudé wrote:
> Date: Mon, 13 Feb 2023 14:13:40 +0100
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> Subject: Re: [RFC 09/52] hw/machine: Introduce core type for hybrid topology
> 
> On 13/2/23 10:49, Zhao Liu wrote:
> > From: Zhao Liu <zhao1.liu@intel.com>
> > 
> > Under the hybrid cpu topology, some CPUs care about the core type.
> > For example, Intel's Alder Lake series CPU contains two types of cores:
> > Intel Core and Intel Atom. The type information of these two types is
> > exposed in 1A leaf of CPUID.
> > 
> > Core type should also be part of the hybrid topology, and
> > MachineState.cpu_type cannot provide different type information for
> > different cpus in the same machine, so add a type field for the core
> > level in the hybrid cpu topology.
> > 
> > Additionally, add a helper to get core type information from
> > MachineState. Though core_type is only used in hybrid case, don't
> > use assert since it may be used to initialize some generic fields.
> > 
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > ---
> >   hw/core/machine-topo.c        | 12 ++++++++++++
> >   include/hw/boards.h           |  3 +++
> >   include/hw/cpu/cpu-topology.h |  2 ++
> >   3 files changed, 17 insertions(+)
> > 
> > diff --git a/hw/core/machine-topo.c b/hw/core/machine-topo.c
> > index b20160479629..e0ec07b53d42 100644
> > --- a/hw/core/machine-topo.c
> > +++ b/hw/core/machine-topo.c
> > @@ -51,6 +51,18 @@ unsigned int machine_topo_get_smp_threads(const MachineState *ms)
> >       return ms->topo.smp.threads;
> >   }
> > +unsigned int machine_topo_get_hybrid_core_type(const MachineState *ms,
> > +                                               unsigned int cluster_id,
> > +                                               unsigned int core_id)
> > +{
> > +    if (!machine_topo_is_smp(ms)) {
> > +        return ms->topo.hybrid.cluster_list[cluster_id]
> > +                       .core_list[core_id].core_type;
> > +    } else {
> > +        return 0;
> 
> Is '0' an invalid type?

My intention is to use 0 as the default type under smp (it can be
regarded as invalid under hybrid, this should depend on the check
of the architecture itself), I'll use a macro instead of hardcode.

> 
> > +    }
> > +}
>
Zhao Liu Feb. 15, 2023, 2:40 a.m. UTC | #4 
On Tue, Feb 14, 2023 at 09:14:13AM +0800, Mi, Dapeng1 wrote:
> Date: Tue, 14 Feb 2023 09:14:13 +0800
> From: "Mi, Dapeng1" <dapeng1.mi@intel.com>
> Subject: RE: [RFC 09/52] hw/machine: Introduce core type for hybrid topology
> 
> > From: Zhao Liu <zhao1.liu@linux.intel.com>
> > Sent: Monday, February 13, 2023 5:50 PM
> > To: Eduardo Habkost <eduardo@habkost.net>; Marcel Apfelbaum
> > <marcel.apfelbaum@gmail.com>; Philippe Mathieu-Daud? <philmd@linaro.org>;
> > Yanan Wang <wangyanan55@huawei.com>; Michael S . Tsirkin
> > <mst@redhat.com>; Richard Henderson <richard.henderson@linaro.org>; Paolo
> > Bonzini <pbonzini@redhat.com>; Eric Blake <eblake@redhat.com>; Markus
> > Armbruster <armbru@redhat.com>
> > Cc: qemu-devel@nongnu.org; Wang, Zhenyu Z <zhenyu.z.wang@intel.com>; Mi,
> > Dapeng1 <dapeng1.mi@intel.com>; Ding, Zhuocheng
> > <zhuocheng.ding@intel.com>; Robert Hoo <robert.hu@linux.intel.com>;
> > Christopherson,, Sean <seanjc@google.com>; Like Xu
> > <like.xu.linux@gmail.com>; Liu, Zhao1 <zhao1.liu@intel.com>
> > Subject: [RFC 09/52] hw/machine: Introduce core type for hybrid topology
> > 
> > From: Zhao Liu <zhao1.liu@intel.com>
> > 
> > Under the hybrid cpu topology, some CPUs care about the core type.
> > For example, Intel's Alder Lake series CPU contains two types of cores:
> > Intel Core and Intel Atom. The type information of these two types is exposed in
> > 1A leaf of CPUID.
> > 
> > Core type should also be part of the hybrid topology, and
> > MachineState.cpu_type cannot provide different type information for different
> > cpus in the same machine, so add a type field for the core level in the hybrid cpu
> > topology.
> > 
> > Additionally, add a helper to get core type information from MachineState.
> > Though core_type is only used in hybrid case, don't use assert since it may be
> > used to initialize some generic fields.
> > 
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > ---
> >  hw/core/machine-topo.c        | 12 ++++++++++++
> >  include/hw/boards.h           |  3 +++
> >  include/hw/cpu/cpu-topology.h |  2 ++
> >  3 files changed, 17 insertions(+)
> > 
> > diff --git a/hw/core/machine-topo.c b/hw/core/machine-topo.c index
> > b20160479629..e0ec07b53d42 100644
> > --- a/hw/core/machine-topo.c
> > +++ b/hw/core/machine-topo.c
> > @@ -51,6 +51,18 @@ unsigned int machine_topo_get_smp_threads(const
> > MachineState *ms)
> >      return ms->topo.smp.threads;
> >  }
> > 
> > +unsigned int machine_topo_get_hybrid_core_type(const MachineState *ms,
> > +                                               unsigned int cluster_id,
> > +                                               unsigned int core_id) {
> > +    if (!machine_topo_is_smp(ms)) {
> > +        return ms->topo.hybrid.cluster_list[cluster_id]
> > +                       .core_list[core_id].core_type;
> > +    } else {
> > +        return 0;
> > +    }
> > +}
> > +
> 
> We'd better not to return the hard-coded '0'. Suggest to define a 'enum'
> data structure to represent the core_type. This makes the code look more intuitively.

Yes. I defined a core type 'enum' in x86 code, Here I can use a macro to
avoid hardcoding.

Zhao

> 
> >  unsigned int machine_topo_get_threads(const MachineState *ms,
> >                                        unsigned int cluster_id,
> >                                        unsigned int core_id) diff --git a/include/hw/boards.h
> > b/include/hw/boards.h index 34b64b012022..78e52af38cb1 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -484,6 +484,9 @@ unsigned int machine_topo_get_dies(const
> > MachineState *ms);  unsigned int machine_topo_get_clusters(const
> > MachineState *ms);  unsigned int machine_topo_get_smp_cores(const
> > MachineState *ms);  unsigned int machine_topo_get_smp_threads(const
> > MachineState *ms);
> > +unsigned int machine_topo_get_hybrid_core_type(const MachineState *ms,
> > +                                               unsigned int cluster_id,
> > +                                               unsigned int core_id);
> >  unsigned int machine_topo_get_threads(const MachineState *ms,
> >                                        unsigned int cluster_id,
> >                                        unsigned int core_id); diff --git a/include/hw/cpu/cpu-
> > topology.h b/include/hw/cpu/cpu-topology.h index
> > 8268ea3a8569..87d832556229 100644
> > --- a/include/hw/cpu/cpu-topology.h
> > +++ b/include/hw/cpu/cpu-topology.h
> > @@ -45,9 +45,11 @@ typedef struct SmpCpuTopology {
> >  /**
> >   * HybridCore - hybrid core topology defination:
> >   * @threads: the number of threads in one core.
> > + * @core_type: the type of current core.
> >   */
> >  typedef struct HybridCore {
> >      unsigned int threads;
> > +    unsigned int core_type;
> >  } HybridCore;
> > 
> >  /**
> > --
> > 2.34.1
>
diff mbox series

Patch

diff --git a/hw/core/machine-topo.c b/hw/core/machine-topo.c
index b20160479629..e0ec07b53d42 100644
--- a/hw/core/machine-topo.c
+++ b/hw/core/machine-topo.c
@@ -51,6 +51,18 @@  unsigned int machine_topo_get_smp_threads(const MachineState *ms)
     return ms->topo.smp.threads;
 }
 
+unsigned int machine_topo_get_hybrid_core_type(const MachineState *ms,
+                                               unsigned int cluster_id,
+                                               unsigned int core_id)
+{
+    if (!machine_topo_is_smp(ms)) {
+        return ms->topo.hybrid.cluster_list[cluster_id]
+                       .core_list[core_id].core_type;
+    } else {
+        return 0;
+    }
+}
+
 unsigned int machine_topo_get_threads(const MachineState *ms,
                                       unsigned int cluster_id,
                                       unsigned int core_id)
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 34b64b012022..78e52af38cb1 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -484,6 +484,9 @@  unsigned int machine_topo_get_dies(const MachineState *ms);
 unsigned int machine_topo_get_clusters(const MachineState *ms);
 unsigned int machine_topo_get_smp_cores(const MachineState *ms);
 unsigned int machine_topo_get_smp_threads(const MachineState *ms);
+unsigned int machine_topo_get_hybrid_core_type(const MachineState *ms,
+                                               unsigned int cluster_id,
+                                               unsigned int core_id);
 unsigned int machine_topo_get_threads(const MachineState *ms,
                                       unsigned int cluster_id,
                                       unsigned int core_id);
diff --git a/include/hw/cpu/cpu-topology.h b/include/hw/cpu/cpu-topology.h
index 8268ea3a8569..87d832556229 100644
--- a/include/hw/cpu/cpu-topology.h
+++ b/include/hw/cpu/cpu-topology.h
@@ -45,9 +45,11 @@  typedef struct SmpCpuTopology {
 /**
  * HybridCore - hybrid core topology defination:
  * @threads: the number of threads in one core.
+ * @core_type: the type of current core.
  */
 typedef struct HybridCore {
     unsigned int threads;
+    unsigned int core_type;
 } HybridCore;
 
 /**