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@xxxxxxxxx>
> 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@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Signed-off-by: Zhao Liu <zhao1.liu@xxxxxxxxx>
>
> Reviewed-by: Xiaoyao Li <xiaoyao.li@xxxxxxxxx>
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@xxxxxxxxxxxxxxx/
> 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));
> > +}
> > +
