On 5/26/24 17:56, Nicholas Piggin wrote:
> The way SMT thread siblings are matched is clunky, using hard-coded
> logic that checks the PIR SPR.
>
> Change that to use a new core_index variable in the CPUPPCState,
> where all siblings have the same core_index. CPU realize routines have
> flexibility in setting core/sibling topology.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> target/ppc/cpu.h | 5 ++++-
> hw/ppc/pnv_core.c | 2 ++
> hw/ppc/spapr_cpu_core.c | 3 +++
> 3 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index dac13d4dac..9a89083932 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1247,6 +1247,9 @@ struct CPUArchState {
> /* when a memory exception occurs, the access type is stored here */
> int access_type;
>
> + /* For SMT processors */
> + int core_index;
> +
> #if !defined(CONFIG_USER_ONLY)
> /* MMU context, only relevant for full system emulation */
> #if defined(TARGET_PPC64)
> @@ -1404,7 +1407,7 @@ struct CPUArchState {
> };
>
> #define _CORE_ID(cs) \
> - (POWERPC_CPU(cs)->env.spr_cb[SPR_PIR].default_value & ~(cs->nr_threads - 1))
> + (POWERPC_CPU(cs)->env.core_index)
Dont we want to keep the logical & with ~(cs->nr_threads - 1) ?
How's it taken care ?
>
> #define THREAD_SIBLING_FOREACH(cs, cs_sibling) \
> CPU_FOREACH(cs_sibling) \
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index 9b5edd9e48..0f61aabb77 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -252,6 +252,8 @@ static void pnv_core_cpu_realize(PnvCore *pc, PowerPCCPU *cpu, Error **errp,
> pir_spr->default_value = pir;
> tir_spr->default_value = tir;
>
> + env->core_index = core_hwid;
> +
> /* Set time-base frequency to 512 MHz */
> cpu_ppc_tb_init(env, PNV_TIMEBASE_FREQ);
> }
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index e7c9edd033..059d372c8a 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -300,16 +300,19 @@ static PowerPCCPU *spapr_create_vcpu(SpaprCpuCore *sc, int i, Error **errp)
> g_autofree char *id = NULL;
> CPUState *cs;
> PowerPCCPU *cpu;
> + CPUPPCState *env;
>
> obj = object_new(scc->cpu_type);
>
> cs = CPU(obj);
> cpu = POWERPC_CPU(obj);
> + env = &cpu->env;
> /*
> * All CPUs start halted. CPU0 is unhalted from the machine level reset code
> * and the rest are explicitly started up by the guest using an RTAS call.
> */
> qdev_prop_set_bit(DEVICE(obj), "start-powered-off", true);
> + env->core_index = cc->core_id;
We could just do cpu->env.core_index and avoid creating local var env.
regards,
Harsh
> cs->cpu_index = cc->core_id + i;
> if (!spapr_set_vcpu_id(cpu, cs->cpu_index, errp)) {
> return NULL;