Re: [PATCH v9 11/21] i386/cpu: Decouple CPUID[0x1F] subleaf with specific topology level

[Zhao Liu <zhao1.liu@linux.intel.com>]

On Mon, Mar 11, 2024 at 04:45:41PM +0800, Xiaoyao Li wrote:
> Date: Mon, 11 Mar 2024 16:45:41 +0800
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> Subject: Re: [PATCH v9 11/21] i386/cpu: Decouple CPUID[0x1F] subleaf with
>  specific topology level
> 
> On 2/27/2024 6:32 PM, Zhao Liu wrote:
> > From: Zhao Liu <zhao1.liu@intel.com>
> > 
> > At present, the subleaf 0x02 of CPUID[0x1F] is bound to the "die" level.
> > 
> > In fact, the specific topology level exposed in 0x1F depends on the
> > platform's support for extension levels (module, tile and die).
> > 
> > To help expose "module" level in 0x1F, decouple CPUID[0x1F] subleaf
> > with specific topology level.
> > 
> > Tested-by: Yongwei Ma <yongwei.ma@intel.com>
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> 
> Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

Thanks!

> Besides, some nits below.
>

[snip]

> > +static void encode_topo_cpuid1f(CPUX86State *env, uint32_t count,
> > +                                X86CPUTopoInfo *topo_info,
> > +                                uint32_t *eax, uint32_t *ebx,
> > +                                uint32_t *ecx, uint32_t *edx)
> > +{
> > +    X86CPU *cpu = env_archcpu(env);
> > +    unsigned long level;
> > +    uint32_t num_threads_next_level, offset_next_level;
> > +
> > +    assert(count + 1 < CPU_TOPO_LEVEL_MAX);
> > +
> > +    /*
> > +     * Find the No.count topology levels in avail_cpu_topo bitmap.
> > +     * Start from bit 0 (CPU_TOPO_LEVEL_INVALID).
> 
> AFAICS, it starts from bit 1 (CPU_TOPO_LEVEL_SMT). Because the initial value
> of level is CPU_TOPO_LEVEL_INVALID, but the first round of the loop is to
> find the valid bit starting from (level + 1).

Yes, this description is much clearer.

> > +     */
> > +    level = CPU_TOPO_LEVEL_INVALID;
> > +    for (int i = 0; i <= count; i++) {
> > +        level = find_next_bit(env->avail_cpu_topo,
> > +                              CPU_TOPO_LEVEL_PACKAGE,
> > +                              level + 1);
> > +
> > +        /*
> > +         * CPUID[0x1f] doesn't explicitly encode the package level,
> > +         * and it just encode the invalid level (all fields are 0)
> > +         * into the last subleaf of 0x1f.
> > +         */
> 
> QEMU will never set bit CPU_TOPO_LEVEL_PACKAGE in env->avail_cpu_topo.

In the patch 9 [1], I set the CPU_TOPO_LEVEL_PACKAGE in bitmap. This
level is a basic topology level in general, so it's worth being set.

Only in Intel's 0x1F, it doesn't have a corresponding type, and where
I use it as a termination condition for 0x1F encoding (not an error case).

[1]: https://lore.kernel.org/qemu-devel/20240227103231.1556302-10-zhao1.liu@linux.intel.com/

> So I think we should assert() it instead of fixing it silently.
> 
> > +        if (level == CPU_TOPO_LEVEL_PACKAGE) {
> > +            level = CPU_TOPO_LEVEL_INVALID;
> > +            break;
> > +        }
> > +    }
> > +
> > +    if (level == CPU_TOPO_LEVEL_INVALID) {
> > +        num_threads_next_level = 0;
> > +        offset_next_level = 0;
> > +    } else {
> > +        unsigned long next_level;
> 
> please define it at the beginning of the function. e.g.,

Okay, I'll put the declaration of "next_level" at the beginning of this
function with a current variable "level".

> 
> > +        next_level = find_next_bit(env->avail_cpu_topo,
> > +                                   CPU_TOPO_LEVEL_PACKAGE,
> > +                                   level + 1);
> > +        num_threads_next_level = num_threads_by_topo_level(topo_info,
> > +                                                           next_level);
> > +        offset_next_level = apicid_offset_by_topo_level(topo_info,
> > +                                                        next_level);
> > +    }
> > +
> > +    *eax = offset_next_level;
> > +    *ebx = num_threads_next_level;
> > +    *ebx &= 0xffff; /* The count doesn't need to be reliable. */
> 
> we can combine them together. e.g.,
> 
> *ebx = num_threads_next_level & 0xffff; /* ... */
> 
> > +    *ecx = count & 0xff;
> > +    *ecx |= cpuid1f_topo_type(level) << 8;
> 
> Ditto,
> 
> *ecx = count & 0xff | cpuid1f_topo_type(level) << 8;

OK, will combine these.

> > +    *edx = cpu->apic_id;
> > +
> > +    assert(!(*eax & ~0x1f));
> > +}
> > +