Re: [PATCH v4 2/9] hw/core: Make CPU topology enumeration arch-agnostic

[Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>]

On 22/10/24 10:51, Zhao Liu wrote:
> Cache topology needs to be defined based on CPU topology levels. Thus,
> define CPU topology enumeration in qapi/machine.json to make it generic
> for all architectures.
>
> To match the general topology naming style, rename CPU_TOPO_LEVEL_* to
> CPU_TOPOLOGY_LEVEL_*, and rename SMT and package levels to thread and
> socket.
>
> Also, enumerate additional topology levels for non-i386 arches, and add
> a CPU_TOPOLOGY_LEVEL_DEFAULT to help future smp-cache object to work
> with compatibility requirement of arch-specific cache topology models.
>
> Signed-off-by: Zhao Liu <zhao1.liu@xxxxxxxxx>
> Tested-by: Yongwei Ma <yongwei.ma@xxxxxxxxx>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> ---
> Changes since Patch v3:
>   * Dropped "invalid" level to avoid an unsettable option. (Daniel)
> ---
>   hw/i386/x86-common.c       |   4 +-
>   include/hw/i386/topology.h |  23 ++----
>   qapi/machine-common.json   |  44 +++++++++++-
>   target/i386/cpu.c          | 144 ++++++++++++++++++-------------------
>   target/i386/cpu.h          |   4 +-
>   5 files changed, 123 insertions(+), 96 deletions(-)
>
> diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c
> index b86c38212eab..bc360a9ea44b 100644
> --- a/hw/i386/x86-common.c
> +++ b/hw/i386/x86-common.c
> @@ -273,12 +273,12 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
>   
>       if (ms->smp.modules > 1) {
>           env->nr_modules = ms->smp.modules;
> -        set_bit(CPU_TOPO_LEVEL_MODULE, env->avail_cpu_topo);
> +        set_bit(CPU_TOPOLOGY_LEVEL_MODULE, env->avail_cpu_topo);
>       }
>   
>       if (ms->smp.dies > 1) {
>           env->nr_dies = ms->smp.dies;
> -        set_bit(CPU_TOPO_LEVEL_DIE, env->avail_cpu_topo);
> +        set_bit(CPU_TOPOLOGY_LEVEL_DIE, env->avail_cpu_topo);
>       }
>   
>       /*
> diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
> index 48b43edc5a90..b2c8bf2de158 100644
> --- a/include/hw/i386/topology.h
> +++ b/include/hw/i386/topology.h
> @@ -39,7 +39,7 @@
>    *  CPUID Fn8000_0008_ECX[ApicIdCoreIdSize[3:0]] is set to apicid_core_width().
>    */
>   
> -
> +#include "qapi/qapi-types-machine-common.h"
>   #include "qemu/bitops.h"
>   
>   /*
> @@ -62,22 +62,7 @@ typedef struct X86CPUTopoInfo {
>       unsigned threads_per_core;
>   } X86CPUTopoInfo;
>   
> -#define CPU_TOPO_LEVEL_INVALID CPU_TOPO_LEVEL_MAX
> -
> -/*
> - * CPUTopoLevel is the general i386 topology hierarchical representation,
> - * ordered by increasing hierarchical relationship.
> - * Its enumeration value is not bound to the type value of Intel (CPUID[0x1F])
> - * or AMD (CPUID[0x80000026]).
> - */
> -enum CPUTopoLevel {
> -    CPU_TOPO_LEVEL_SMT,
> -    CPU_TOPO_LEVEL_CORE,
> -    CPU_TOPO_LEVEL_MODULE,
> -    CPU_TOPO_LEVEL_DIE,
> -    CPU_TOPO_LEVEL_PACKAGE,
> -    CPU_TOPO_LEVEL_MAX,
> -};
> +#define CPU_TOPOLOGY_LEVEL_INVALID CPU_TOPOLOGY_LEVEL__MAX


> @@ -341,18 +341,18 @@ static uint32_t apicid_offset_by_topo_level(X86CPUTopoInfo *topo_info,
>       return 0;
>   }
>   
> -static uint32_t cpuid1f_topo_type(enum CPUTopoLevel topo_level)
> +static uint32_t cpuid1f_topo_type(enum CpuTopologyLevel topo_level)
>   {
>       switch (topo_level) {
> -    case CPU_TOPO_LEVEL_INVALID:
> +    case CPU_TOPOLOGY_LEVEL_INVALID:

Since we use an enum, I'd rather directly use CPU_TOPOLOGY_LEVEL__MAX.

Or maybe in this case ...

>           return CPUID_1F_ECX_TOPO_LEVEL_INVALID;
> -    case CPU_TOPO_LEVEL_SMT:
> +    case CPU_TOPOLOGY_LEVEL_THREAD:
>           return CPUID_1F_ECX_TOPO_LEVEL_SMT;
> -    case CPU_TOPO_LEVEL_CORE:
> +    case CPU_TOPOLOGY_LEVEL_CORE:
>           return CPUID_1F_ECX_TOPO_LEVEL_CORE;
> -    case CPU_TOPO_LEVEL_MODULE:
> +    case CPU_TOPOLOGY_LEVEL_MODULE:
>           return CPUID_1F_ECX_TOPO_LEVEL_MODULE;
> -    case CPU_TOPO_LEVEL_DIE:
> +    case CPU_TOPOLOGY_LEVEL_DIE:
>           return CPUID_1F_ECX_TOPO_LEVEL_DIE;
>       default:
           /* Other types are not supported in QEMU. */
           g_assert_not_reached();

... return CPUID_1F_ECX_TOPO_LEVEL_INVALID as default.

Can be cleaned on top, so:

Acked-by: Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>