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

2024-05-28 Thread Nicholas Piggin 
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

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

2024-05-28 Thread Harsh Prateek Bora 

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 ?

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

2024-05-28 Thread Harsh Prateek Bora 




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 
---
  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 = >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;

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

2024-05-26 Thread Nicholas Piggin 
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 
---
 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)
 
 #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 = >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;
-- 
2.43.0