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

[Zhao Liu]

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@intel.com>
> 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! :)