On Fri, Nov 01, 2024 at 10:38:56AM +0800, Zhao Liu wrote:
> Date: Fri, 1 Nov 2024 10:38:56 +0800
> From: Zhao Liu <zhao1.liu@xxxxxxxxx>
> Subject: Re: [PATCH v4 2/9] hw/core: Make CPU topology enumeration
> arch-agnostic
>
> Hi Phil,
>
> > > -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.
>
> I prefer the first way you mentioned since I want "default" to keep
> to detact unimplemented levels.
>
Ah, when I started working on it, I realized that clearing
CPU_TOPOLOGY_LEVEL_INVALID would reduce the readability of the
encode_topo_cpuid1f(). The encoding rules for the 0x1f leaf are somewhat
complex, so I want the topology (and names) in encode_topo_cpuid1f() to
be as consistent with the spec as possible. Therefore, I will keep this
name! :)
