Re: [RFC 06/52] hw/cpu: Introduce hybrid CPU topology

[Zhao Liu]

On Tue, Feb 14, 2023 at 07:23:37PM +0800, wangyanan (Y) wrote:
> Date: Tue, 14 Feb 2023 19:23:37 +0800
> From: "wangyanan (Y)" <wangyanan55@huawei.com>
> Subject: Re: [RFC 06/52] hw/cpu: Introduce hybrid CPU topology
> 
> 在 2023/2/14 18:16, Zhao Liu 写道:
> > On Mon, Feb 13, 2023 at 09:18:05PM +0800, wangyanan (Y) wrote:
> > > Date: Mon, 13 Feb 2023 21:18:05 +0800
> > > From: "wangyanan (Y)" <wangyanan55@huawei.com>
> > > Subject: Re: [RFC 06/52] hw/cpu: Introduce hybrid CPU topology
> > > 
> > > Hi Zhao,
> > > 
> > > 在 2023/2/13 17:49, Zhao Liu 写道:
> > > > From: Zhao Liu <zhao1.liu@intel.com>
> > > > 
> > > > For smp systems, the parts in one topology level are the same. But now
> > > > there are more and more systems with hybrid architectures. Different
> > > > parts of the same topology level may have differences. For example,
> > > > Intel's Alder Lake series CPU has two types of cores, so the CPU
> > > > topology is no longer symmetrical.
> > > > 
> > > > The hybrid topology is compatible with the smp topology type, that is,
> > > > different parts on the same level of the hybrid topology can set to be
> > > > the same, but the hybrid topology will introduce more complexity (need
> > > > to allocate more memory, organized with array or linked-list), so the
> > > > original smp topology support is retained while introducing the hybrid
> > > > topology, and the hybrid topology is only built when the hybrid is
> > > > explicitly required.
> > > > 
> > > > Therefore, we introduce the definition support of hybrid cpu topology
> > > > here. At the same time, in order to unify with the original smp, we
> > > > introduce a new cpu topology structure that can support smp topology
> > > > or hybrid topology. This structure will replace the CpuTopology type (in
> > > > include/hw/boards.h) used by MachineState.smp.
> > > > 
> > > > As for now, we only support two hybrid topology levels: core and
> > > > cluster.
> > > > 
> > > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > > > ---
> > > >    MAINTAINERS                   |   1 +
> > > >    include/hw/cpu/cpu-topology.h | 117 
> > > > ++++++++++++++++++++++++++++++++++
> > > >    qapi/machine.json             |  12 ++++
> > > >    3 files changed, 130 insertions(+)
> > > >    create mode 100644 include/hw/cpu/cpu-topology.h
> > > > 
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 58794885ced3..918a9418d98e 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -1742,6 +1742,7 @@ F: qapi/machine-target.json
> > > >    F: include/hw/boards.h
> > > >    F: include/hw/core/cpu.h
> > > >    F: include/hw/cpu/cluster.h
> > > > +F: include/hw/cpu/cpu-topology.h
> > > Should't it be in include/hw/core/* directory？
> > Yes, I'll move it to the correct place.
> > 
> > > >    F: include/sysemu/numa.h
> > > >    F: tests/unit/test-smp-parse.c
> > > >    T: git https://gitlab.com/ehabkost/qemu.git machine-next
> > > > diff --git a/include/hw/cpu/cpu-topology.h 
> > > > b/include/hw/cpu/cpu-topology.h
> > > > new file mode 100644
> > > > index 000000000000..8268ea3a8569
> > > > --- /dev/null
> > > > +++ b/include/hw/cpu/cpu-topology.h
> > > > @@ -0,0 +1,117 @@
> > > > +/*
> > > > + * CPU topology defination for Machine core
> > > > + *
> > > > + * Copyright (c) 2023 Intel Corporation
> > > > + * Author: Zhao Liu <zhao1.liu@intel.com>
> > > > + *
> > > > + * This program is free software; you can redistribute it and/or modify
> > > > + * it under the terms of the GNU General Public License as published by
> > > > + * the Free Software Foundation; either version 2 of the License,
> > > > + * or (at your option) any later version.
> > > > + *
> > > > + * This program is distributed in the hope that it will be useful,
> > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > > + * GNU General Public License for more details.
> > > > + *
> > > > + * You should have received a copy of the GNU General Public License
> > > > + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> > > > + */
> > > > +
> > > > +#ifndef CPU_TOPOLOGY_H
> > > > +#define CPU_TOPOLOGY_H
> > > > +
> > > > +#include "qemu/queue.h"
> > > > +
> > > > +/**
> > > > + * SmpCpuTopology - smp cpu topology defination.
> > > > + *
> > > > + * For smp system, the parts in one topology level are the same.
> > > > + *
> > > > + * @sockets: the number of sockets on the machine
> > > > + * @dies: the number of dies in one socket
> > > > + * @clusters: the number of clusters in one die
> > > > + * @cores: the number of cores in one cluster
> > > > + * @threads: the number of threads in one core
> > > > + */
> > > > +typedef struct SmpCpuTopology {
> > > > +    unsigned int sockets;
> > > > +    unsigned int dies;
> > > > +    unsigned int clusters;
> > > > +    unsigned int cores;
> > > > +    unsigned int threads;
> > > > +} SmpCpuTopology;
> > > > +
> > > > +/**
> > > > + * HybridCore - hybrid core topology defination:
> > > > + * @threads: the number of threads in one core.
> > > > + */
> > > > +typedef struct HybridCore {
> > > > +    unsigned int threads;
> > > > +} HybridCore;
> > > > +
> > > > +/**
> > > > + * HybridCluster - hybrid cluster topology defination:
> > > > + * @cores: the number of cores in current cluster.
> > > > + * @core_list: the array includes all the cores that belong to current
> > > > + *             cluster.
> > > > + */
> > > > +typedef struct HybridCluster {
> > > > +    unsigned int cores;
> > > > +    HybridCore *core_list;
> > > > +} HybridCluster;
> > > > +
> > > > +/**
> > > > + * HybridCpuTopology - hybrid cpu topology defination.
> > > > + *
> > > > + * At present we only support two heterogeneous topology levels: core
> > > > + * and cluster. For heterogeneous levels, we need additional structs
> > > > + * to define their custom internal topology. So here we defines
> > > > + * symmetric topology levels, and use a list to point to heterogeneous
> > > > + * levels.
> > > > + *
> > > > + * @sockets: the number of sockets on the machine. All sockets are the
> > > > + *           same.
> > > > + * @dies: the number of dies in one socket. All dies are the same.
> > > > + * @clusters: the number of clusters in one die. Cluster may be
> > > > + *            different. This field indicates the length of
> > > > + *            cluster_list.
> > > > + * @cluster_list: the array includes all the clusters in one die.
> > > > + */
> > > > +typedef struct HybridCpuTopology {
> > > > +    unsigned int sockets;
> > > > +    unsigned int dies;
> > > > +    unsigned int clusters;
> > > > +    HybridCluster *cluster_list;
> > > > +} HybridCpuTopology;
> > > > +
> > > > +/**
> > > > + * GeneralCpuTopology - General cpu topology defination.
> > > > + *
> > > > + * It supports one of two topologies: smp topology or hybrid topology.
> > > > + *
> > > > + * @cpus: the number of present logical processors on the machine
> > > > + * @max_cpus: the maximum number of logical processors on the machine
> > > > + * @topo_type: the topology type of the machine and this decides which
> > > > + *             member of the union to visit: smp or hybrid.
> > > > + * @smp: the smp cpu topology informantion. Only valid when topo_type 
> > > > is
> > > > + *       CPU_TOPO_TYPE_SMP.
> > > > + * @hybrid: the hybrid cpu topology informantion. Only valid when
> > > > + *          topo_type is CPU_TOPO_TYPE_HYBRID.
> > > > + */
> > > > +typedef struct GeneralCpuTopology {
> > > > +    unsigned int cpus;
> > > > +    unsigned int max_cpus;
> > > > +    CpuTopoType topo_type;
> > > > +    union {
> > > > +        SmpCpuTopology smp;
> > > > +        HybridCpuTopology hybrid;
> > > > +    };
> > > > +} GeneralCpuTopology; /*
> > > > +                       * TODO: This name is temporary, just to 
> > > > distinguish it
> > > > +                       * from the CpuTopology in boards.h. When 
> > > > CpuTopology in
> > > > +                       * boards.h is merged here, it will be uniformly 
> > > > named as
> > > > +                       * CpuTopology.
> > > > +                       */
> > > > +
> > > A suggestion:
> > > 1、Move definition of CpuTopology from boards.h to cpu-topology.h
> > > and re-structure it to include SmpCpuTopology, being a generic cpu
> > > topology structure.
> > > 2、Rename "CpuTopology smp" in MachineState to a generic name
> > > "CpuTopology topo".
> > Here we need to change the access to MachineState.smp to
> > MachineState.topo.smp in other modules.
> > 
> > If replacement of MachineState.topo is in a single patch, do we also
> > need to include the modification of access to MachineState.topo.smp
> > in other modules? Otherwise, it will break the compilation.
> > 
> > In this way, the patch seems be too large.
> Oh, I understand. The temporary "GeneralCpuTopology" seems necessary.
> With this patch and help of the introduced generic helpers, we can replace
> access to MachineState.smp gradually. And that's what you are doing in
> this series.
> 
> But is it possible to strip the whole hybrid related code from the re-work
> of smp. After MachineState.smp is re-worked/generalized completely,
> then we add the hybrid extansion? Now the re-work code of smp and
> hybrid code looks coupled all the way.

I agree. This is more clear.

Thanks,
Zhao

> 
> Thanks,
> Yanan
> > 
> > Thanks,
> > Zhao
> > 
> > > 3、Adapt all the code in QEMU to above change.
> > > 
> > > If you can pack above into a single patch, and then add the hybird
> > > topology extansion in a next patch, we will not need the temporary
> > > thing "GeneralCpuTopology" and the TODO comments, which makes
> > > code clearer.
> > > 
> > > Thanks,
> > > Yanan
> > > > +#endif /* CPU_TOPOLOGY_H */
> > > > diff --git a/qapi/machine.json b/qapi/machine.json
> > > > index b9228a5e4616..bd7303f34497 100644
> > > > --- a/qapi/machine.json
> > > > +++ b/qapi/machine.json
> > > > @@ -36,6 +36,18 @@
> > > >                 'sh4eb', 'sparc', 'sparc64', 'tricore',
> > > >                 'x86_64', 'xtensa', 'xtensaeb' ] }
> > > > +##
> > > > +# @CpuTopoType:
> > > > +#
> > > > +# An enumeration of cpu topology type
> > > > +# TODO: Expose topology type in query-cpus-fast
> > > > +#
> > > > +# Since: 8.0
> > > > +##
> > > > +{ 'enum': 'CpuTopoType',
> > > > +  'prefix': 'CPU_TOPO_TYPE',
> > > > +  'data': [ 'smp', 'hybrid' ] }
> > > > +
> > > >    ##
> > > >    # @CpuS390State:
> > > >    #
>