Hi Xiaoyao,
On Thu, Jan 11, 2024 at 11:19:34AM +0800, Xiaoyao Li wrote:
> Date: Thu, 11 Jan 2024 11:19:34 +0800
> From: Xiaoyao Li <[email protected]>
> Subject: Re: [PATCH v7 05/16] i386: Decouple CPUID[0x1F] subleaf with
> specific topology level
>
> On 1/8/2024 4:27 PM, Zhao Liu wrote:
> > From: Zhao Liu <[email protected]>
> >
> > 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.
> >
> > Signed-off-by: Zhao Liu <[email protected]>
> > Tested-by: Babu Moger <[email protected]>
> > Tested-by: Yongwei Ma <[email protected]>
> > Acked-by: Michael S. Tsirkin <[email protected]>
> > ---
> > Changes since v3:
> > * New patch to prepare to expose module level in 0x1F.
> > * Move the CPUTopoLevel enumeration definition from "i386: Add cache
> > topology info in CPUCacheInfo" to this patch. Note, to align with
> > topology types in SDM, revert the name of CPU_TOPO_LEVEL_UNKNOW to
> > CPU_TOPO_LEVEL_INVALID.
> > ---
> > target/i386/cpu.c | 136 +++++++++++++++++++++++++++++++++++++---------
> > target/i386/cpu.h | 15 +++++
> > 2 files changed, 126 insertions(+), 25 deletions(-)
> >
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index bc440477d13d..5c295c9a9e2d 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -269,6 +269,116 @@ static void encode_cache_cpuid4(CPUCacheInfo *cache,
> > (cache->complex_indexing ? CACHE_COMPLEX_IDX : 0);
> > }
> > +static uint32_t num_cpus_by_topo_level(X86CPUTopoInfo *topo_info,
> > + enum CPUTopoLevel topo_level)
> > +{
> > + switch (topo_level) {
> > + case CPU_TOPO_LEVEL_SMT:
> > + return 1;
> > + case CPU_TOPO_LEVEL_CORE:
> > + return topo_info->threads_per_core;
> > + case CPU_TOPO_LEVEL_DIE:
> > + return topo_info->threads_per_core * topo_info->cores_per_die;
> > + case CPU_TOPO_LEVEL_PACKAGE:
> > + return topo_info->threads_per_core * topo_info->cores_per_die *
> > + topo_info->dies_per_pkg;
> > + default:
> > + g_assert_not_reached();
> > + }
> > + return 0;
> > +}
> > +
> > +static uint32_t apicid_offset_by_topo_level(X86CPUTopoInfo *topo_info,
> > + enum CPUTopoLevel topo_level)
> > +{
> > + switch (topo_level) {
> > + case CPU_TOPO_LEVEL_SMT:
> > + return 0;
> > + case CPU_TOPO_LEVEL_CORE:
> > + return apicid_core_offset(topo_info);
> > + case CPU_TOPO_LEVEL_DIE:
> > + return apicid_die_offset(topo_info);
> > + case CPU_TOPO_LEVEL_PACKAGE:
> > + return apicid_pkg_offset(topo_info);
> > + default:
> > + g_assert_not_reached();
> > + }
> > + return 0;
> > +}
> > +
> > +static uint32_t cpuid1f_topo_type(enum CPUTopoLevel topo_level)
> > +{
> > + switch (topo_level) {
> > + case CPU_TOPO_LEVEL_INVALID:
> > + return CPUID_1F_ECX_TOPO_LEVEL_INVALID;
> > + case CPU_TOPO_LEVEL_SMT:
> > + return CPUID_1F_ECX_TOPO_LEVEL_SMT;
> > + case CPU_TOPO_LEVEL_CORE:
> > + return CPUID_1F_ECX_TOPO_LEVEL_CORE;
> > + case CPU_TOPO_LEVEL_DIE:
> > + return CPUID_1F_ECX_TOPO_LEVEL_DIE;
> > + default:
> > + /* Other types are not supported in QEMU. */
> > + g_assert_not_reached();
> > + }
> > + return 0;
> > +}
> > +
> > +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)
> > +{
> > + static DECLARE_BITMAP(topo_bitmap, CPU_TOPO_LEVEL_MAX);
> > + X86CPU *cpu = env_archcpu(env);
> > + unsigned long level, next_level;
> > + uint32_t num_cpus_next_level, offset_next_level;
>
> again, I dislike the name of cpus to represent the logical process or
> thread. we can call it, num_lps_next_level, or num_threads_next_level;
Okay, will use num_threads_next_level ;-)
>
> > +
> > + /*
> > + * Initialize the bitmap to decide which levels should be
> > + * encoded in 0x1f.
> > + */
> > + if (!count) {
>
> using static bitmap and initialize the bitmap on (count == 0), looks bad to
> me. It highly relies on the order of how encode_topo_cpuid1f() is called,
> and fragile.
>
> Instead, we can maintain an array in CPUX86State, e.g.,
>
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1904,6 +1904,8 @@ typedef struct CPUArchState {
>
> /* Number of dies within this CPU package. */
> unsigned nr_dies;
> +
> + unint8_t valid_cpu_topo[CPU_TOPO_LEVEL_MAX];
> } CPUX86State;
>
>
> and initialize it as below, when initializing the env
>
> env->valid_cpu_topo[0] = CPU_TOPO_LEVEL_SMT;
> env->valid_cpu_topo[1] = CPU_TOPO_LEVEL_CORE;
> if (env->nr_dies > 1) {
> env->valid_cpu_topo[2] = CPU_TOPO_LEVEL_DIE;
> }
>
> then in encode_topo_cpuid1f(), we can get level and next_level as
>
> level = env->valid_cpu_topo[count];
> next_level = env->valid_cpu_topo[count + 1];
>
Good idea, let me try this way.
Thanks,
Zhao
