Hi Xiaoyao,
On Sun, Jan 14, 2024 at 08:42:00PM +0800, Xiaoyao Li wrote:
> Date: Sun, 14 Jan 2024 20:42:00 +0800
> From: Xiaoyao Li <[email protected]>
> Subject: Re: [PATCH v7 09/16] i386: Support module_id in X86CPUTopoIDs
>
> On 1/8/2024 4:27 PM, Zhao Liu wrote:
> > From: Zhuocheng Ding <[email protected]>
> >
> > Add module_id member in X86CPUTopoIDs.
> >
> > module_id can be parsed from APIC ID, so also update APIC ID parsing
> > rule to support module level. With this support, the conversions with
> > module level between X86CPUTopoIDs, X86CPUTopoInfo and APIC ID are
> > completed.
> >
> > module_id can be also generated from cpu topology, and before i386
> > supports "clusters" in smp, the default "clusters per die" is only 1,
> > thus the module_id generated in this way is 0, so that it will not
> > conflict with the module_id generated by APIC ID.
> >
> > Signed-off-by: Zhuocheng Ding <[email protected]>
> > Co-developed-by: Zhao Liu <[email protected]>
> > 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 v1:
> > * Merge the patch "i386: Update APIC ID parsing rule to support module
> > level" into this one. (Yanan)
> > * Move the apicid_module_width() and apicid_module_offset() support
> > into the previous modules_per_die related patch. (Yanan)
> > ---
> > hw/i386/x86.c | 28 +++++++++++++++++++++-------
> > include/hw/i386/topology.h | 17 +++++++++++++----
> > 2 files changed, 34 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> > index 85b847ac7914..5269aae3a5c2 100644
> > --- a/hw/i386/x86.c
> > +++ b/hw/i386/x86.c
> > @@ -315,11 +315,11 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
> > /*
> > * If APIC ID is not set,
> > - * set it based on socket/die/core/thread properties.
> > + * set it based on socket/die/cluster/core/thread properties.
> > */
> > if (cpu->apic_id == UNASSIGNED_APIC_ID) {
> > - int max_socket = (ms->smp.max_cpus - 1) /
> > - smp_threads / smp_cores / ms->smp.dies;
> > + int max_socket = (ms->smp.max_cpus - 1) / smp_threads / smp_cores /
> > + ms->smp.clusters / ms->smp.dies;
> > /*
> > * die-id was optional in QEMU 4.0 and older, so keep it optional
> > @@ -366,17 +366,27 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
> > topo_ids.die_id = cpu->die_id;
> > topo_ids.core_id = cpu->core_id;
> > topo_ids.smt_id = cpu->thread_id;
> > +
> > + /*
> > + * TODO: This is the temporary initialization for
> > topo_ids.module_id to
> > + * avoid "maybe-uninitialized" compilation errors. Will remove when
> > + * X86CPU supports cluster_id.
> > + */
> > + topo_ids.module_id = 0;
> >
>
> if you put patch 10 before this patch, then we don't need this trick.
Then, we need another trick to resolve "cpu->cluster_id = topo_ids.module_id;"
in patch 10. ;-)
Thanks,
Zhao
>
> > cpu->apic_id = x86_apicid_from_topo_ids(&topo_info, &topo_ids);
> > }
> > cpu_slot = x86_find_cpu_slot(MACHINE(x86ms), cpu->apic_id, &idx);
> > if (!cpu_slot) {
> > x86_topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids);
> > +
> > error_setg(errp,
> > - "Invalid CPU [socket: %u, die: %u, core: %u, thread: %u] with"
> > - " APIC ID %" PRIu32 ", valid index range 0:%d",
> > - topo_ids.pkg_id, topo_ids.die_id, topo_ids.core_id,
> > topo_ids.smt_id,
> > - cpu->apic_id, ms->possible_cpus->len - 1);
> > + "Invalid CPU [socket: %u, die: %u, module: %u, core: %u,
> > thread: %u]"
> > + " with APIC ID %" PRIu32 ", valid index range 0:%d",
> > + topo_ids.pkg_id, topo_ids.die_id, topo_ids.module_id,
> > + topo_ids.core_id, topo_ids.smt_id, cpu->apic_id,
> > + ms->possible_cpus->len - 1);
> > return;
> > }
> > @@ -502,6 +512,10 @@ const CPUArchIdList
> > *x86_possible_cpu_arch_ids(MachineState *ms)
> > ms->possible_cpus->cpus[i].props.has_die_id = true;
> > ms->possible_cpus->cpus[i].props.die_id = topo_ids.die_id;
> > }
> > + if (ms->smp.clusters > 1) {
> > + ms->possible_cpus->cpus[i].props.has_cluster_id = true;
> > + ms->possible_cpus->cpus[i].props.cluster_id =
> > topo_ids.module_id;
> > + }
> > ms->possible_cpus->cpus[i].props.has_core_id = true;
> > ms->possible_cpus->cpus[i].props.core_id = topo_ids.core_id;
> > ms->possible_cpus->cpus[i].props.has_thread_id = true;
> > diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
> > index 517e51768c13..ed1f3d6c1d5e 100644
> > --- a/include/hw/i386/topology.h
> > +++ b/include/hw/i386/topology.h
> > @@ -50,6 +50,7 @@ typedef uint32_t apic_id_t;
> > typedef struct X86CPUTopoIDs {
> > unsigned pkg_id;
> > unsigned die_id;
> > + unsigned module_id;
> > unsigned core_id;
> > unsigned smt_id;
> > } X86CPUTopoIDs;
> > @@ -127,6 +128,7 @@ static inline apic_id_t
> > x86_apicid_from_topo_ids(X86CPUTopoInfo *topo_info,
> > {
> > return (topo_ids->pkg_id << apicid_pkg_offset(topo_info)) |
> > (topo_ids->die_id << apicid_die_offset(topo_info)) |
> > + (topo_ids->module_id << apicid_module_offset(topo_info)) |
> > (topo_ids->core_id << apicid_core_offset(topo_info)) |
> > topo_ids->smt_id;
> > }
> > @@ -140,12 +142,16 @@ static inline void
> > x86_topo_ids_from_idx(X86CPUTopoInfo *topo_info,
> > X86CPUTopoIDs *topo_ids)
> > {
> > unsigned nr_dies = topo_info->dies_per_pkg;
> > - unsigned nr_cores = topo_info->cores_per_module *
> > - topo_info->modules_per_die;
> > + unsigned nr_modules = topo_info->modules_per_die;
> > + unsigned nr_cores = topo_info->cores_per_module;
> > unsigned nr_threads = topo_info->threads_per_core;
> > - topo_ids->pkg_id = cpu_index / (nr_dies * nr_cores * nr_threads);
> > - topo_ids->die_id = cpu_index / (nr_cores * nr_threads) % nr_dies;
> > + topo_ids->pkg_id = cpu_index / (nr_dies * nr_modules *
> > + nr_cores * nr_threads);
> > + topo_ids->die_id = cpu_index / (nr_modules * nr_cores *
> > + nr_threads) % nr_dies;
> > + topo_ids->module_id = cpu_index / (nr_cores * nr_threads) %
> > + nr_modules;
> > topo_ids->core_id = cpu_index / nr_threads % nr_cores;
> > topo_ids->smt_id = cpu_index % nr_threads;
> > }
> > @@ -163,6 +169,9 @@ static inline void x86_topo_ids_from_apicid(apic_id_t
> > apicid,
> > topo_ids->core_id =
> > (apicid >> apicid_core_offset(topo_info)) &
> > ~(0xFFFFFFFFUL << apicid_core_width(topo_info));
> > + topo_ids->module_id =
> > + (apicid >> apicid_module_offset(topo_info)) &
> > + ~(0xFFFFFFFFUL << apicid_module_width(topo_info));
> > topo_ids->die_id =
> > (apicid >> apicid_die_offset(topo_info)) &
> > ~(0xFFFFFFFFUL << apicid_die_width(topo_info));
>
