diff mbox series

[RFC,06/10] ppc: Add a core_index to CPUPPCState for SMT vCPUs

Message ID 20240526122612.473476-7-npiggin@gmail.com (mailing list archive)
State New, archived
Headers show
Series ppc/pnv: Better big-core model, lpar-per-core, PC unit | expand

Commit Message

Nicholas Piggin May 26, 2024, 12:26 p.m. UTC
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(-)

Comments

Harsh Prateek Bora May 28, 2024, 8:48 a.m. UTC | #1
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;
Harsh Prateek Bora May 28, 2024, 8:52 a.m. UTC | #2
corrected typo, it's bitwise.

On 5/28/24 14:18, Harsh Prateek Bora wrote:
>> -    (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 bitwise & with ~(cs->nr_threads - 1) ?
> How's it taken care ?
Nicholas Piggin May 29, 2024, 12:28 a.m. UTC | #3
On Tue May 28, 2024 at 6:52 PM AEST, Harsh Prateek Bora wrote:
> corrected typo, it's bitwise.
>
> On 5/28/24 14:18, Harsh Prateek Bora wrote:
> >> -    (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 bitwise & with ~(cs->nr_threads - 1) ?
> > How's it taken care ?

For these accessors it actually just wants to have something that
compares if a CPU belongs to the same core or not, so exact value
doesn't really matter.

Maybe the helpers should do that comparison. It could possibly even
be a class method to be really clean, although that's more costly
to call (but writing to a SMT shared register is pretty costly anyway
so maybe doesn't matter).

I'll think a bit more.

Thanks,
Nick
diff mbox series

Patch

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)
 
 #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;
     cs->cpu_index = cc->core_id + i;
     if (!spapr_set_vcpu_id(cpu, cs->cpu_index, errp)) {
         return NULL;