diff mbox series

[RFC,02/10] ppc/pnv: Move timebase state into PnvCore

Message ID 20240526122612.473476-3-npiggin@gmail.com
State New
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 timebase state machine is per per-core state and can be driven
by any thread in the core. It is currently implemented as a hack
where the state is in a CPU structure and only thread 0's state is
accessed by the chiptod, which limits programming the timebase
side of the state machine to thread 0 of a core.

Move the state out into PnvCore and share it among all threads.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 include/hw/ppc/pnv_core.h    | 17 ++++++++++++
 target/ppc/cpu.h             | 20 --------------
 hw/ppc/pnv_chiptod.c         |  6 ++--
 target/ppc/timebase_helper.c | 53 ++++++++++++++++++++----------------
 4 files changed, 49 insertions(+), 47 deletions(-)

Comments

Harsh Prateek Bora May 28, 2024, 6:28 a.m. UTC | #1
On 5/26/24 17:56, Nicholas Piggin wrote:
> The timebase state machine is per per-core state and can be driven
> by any thread in the core. It is currently implemented as a hack
> where the state is in a CPU structure and only thread 0's state is
> accessed by the chiptod, which limits programming the timebase
> side of the state machine to thread 0 of a core.
> 
> Move the state out into PnvCore and share it among all threads.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   include/hw/ppc/pnv_core.h    | 17 ++++++++++++
>   target/ppc/cpu.h             | 20 --------------
>   hw/ppc/pnv_chiptod.c         |  6 ++--
>   target/ppc/timebase_helper.c | 53 ++++++++++++++++++++----------------
>   4 files changed, 49 insertions(+), 47 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
> index 30c1e5b1a3..f434c71547 100644
> --- a/include/hw/ppc/pnv_core.h
> +++ b/include/hw/ppc/pnv_core.h
> @@ -25,6 +25,20 @@
>   #include "hw/ppc/pnv.h"
>   #include "qom/object.h"
>   
> +/* ChipTOD and TimeBase State Machine */
> +struct pnv_tod_tbst {
> +    int tb_ready_for_tod; /* core TB ready to receive TOD from chiptod */
> +    int tod_sent_to_tb;   /* chiptod sent TOD to the core TB */
> +
> +    /*
> +     * "Timers" for async TBST events are simulated by mfTFAC because TFAC
> +     * is polled for such events. These are just used to ensure firmware
> +     * performs the polling at least a few times.
> +     */
> +    int tb_state_timer;
> +    int tb_sync_pulse_timer;
> +};
> +
>   #define TYPE_PNV_CORE "powernv-cpu-core"
>   OBJECT_DECLARE_TYPE(PnvCore, PnvCoreClass,
>                       PNV_CORE)
> @@ -38,6 +52,9 @@ struct PnvCore {
>       uint32_t pir;
>       uint32_t hwid;
>       uint64_t hrmor;
> +
> +    struct pnv_tod_tbst pnv_tod_tbst;
> +

Now that it is part of struct PnvCore itself, we can drop pnv_ prefix
and just call the member variable as tod_tbst ?

>       PnvChip *chip;
>   
>       MemoryRegion xscom_regs;
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 2015e603d4..1e86658da6 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1196,21 +1196,6 @@ DEXCR_ASPECT(SRAPD, 4)
>   DEXCR_ASPECT(NPHIE, 5)
>   DEXCR_ASPECT(PHIE, 6)
>   
> -/*****************************************************************************/
> -/* PowerNV ChipTOD and TimeBase State Machine */
> -struct pnv_tod_tbst {
> -    int tb_ready_for_tod; /* core TB ready to receive TOD from chiptod */
> -    int tod_sent_to_tb;   /* chiptod sent TOD to the core TB */
> -
> -    /*
> -     * "Timers" for async TBST events are simulated by mfTFAC because TFAC
> -     * is polled for such events. These are just used to ensure firmware
> -     * performs the polling at least a few times.
> -     */
> -    int tb_state_timer;
> -    int tb_sync_pulse_timer;
> -};
> -
>   /*****************************************************************************/
>   /* The whole PowerPC CPU context */
>   
> @@ -1292,11 +1277,6 @@ struct CPUArchState {
>   #define TLB_NEED_LOCAL_FLUSH   0x1
>   #define TLB_NEED_GLOBAL_FLUSH  0x2
>   
> -#if defined(TARGET_PPC64)
> -    /* PowerNV chiptod / timebase facility state. */
> -    /* Would be nice to put these into PnvCore */
> -    struct pnv_tod_tbst pnv_tod_tbst;
> -#endif
>   #endif
>   
>       /* Other registers */
> diff --git a/hw/ppc/pnv_chiptod.c b/hw/ppc/pnv_chiptod.c
> index 3831a72101..3eaddd66f0 100644
> --- a/hw/ppc/pnv_chiptod.c
> +++ b/hw/ppc/pnv_chiptod.c
> @@ -365,7 +365,7 @@ static void pnv_chiptod_xscom_write(void *opaque, hwaddr addr,
>                             " TOD_MOVE_TOD_TO_TB_REG with no slave target\n");
>           } else {
>               PowerPCCPU *cpu = chiptod->slave_pc_target->threads[0];
> -            CPUPPCState *env = &cpu->env;
> +            PnvCore *pc = pnv_cpu_state(cpu)->core;
>   
>               /*
>                * Moving TOD to TB will set the TB of all threads in a
> @@ -377,8 +377,8 @@ static void pnv_chiptod_xscom_write(void *opaque, hwaddr addr,
>                * thread 0.
>                */
>   
> -            if (env->pnv_tod_tbst.tb_ready_for_tod) {
> -                env->pnv_tod_tbst.tod_sent_to_tb = 1;
> +            if (pc->pnv_tod_tbst.tb_ready_for_tod) {
> +                pc->pnv_tod_tbst.tod_sent_to_tb = 1;
>               } else {
>                   qemu_log_mask(LOG_GUEST_ERROR, "pnv_chiptod: xscom write reg"
>                                 " TOD_MOVE_TOD_TO_TB_REG with TB not ready to"
> diff --git a/target/ppc/timebase_helper.c b/target/ppc/timebase_helper.c
> index 39d397416e..788c498d63 100644
> --- a/target/ppc/timebase_helper.c
> +++ b/target/ppc/timebase_helper.c
> @@ -19,6 +19,7 @@
>   #include "qemu/osdep.h"
>   #include "cpu.h"
>   #include "hw/ppc/ppc.h"
> +#include "hw/ppc/pnv_core.h"
>   #include "exec/helper-proto.h"
>   #include "exec/exec-all.h"
>   #include "qemu/log.h"
> @@ -298,8 +299,17 @@ static void write_tfmr(CPUPPCState *env, target_ulong val)
>       }
>   }
>   
> +static struct pnv_tod_tbst *cpu_get_tbst(PowerPCCPU *cpu)
> +{
> +    PnvCore *pc = pnv_cpu_state(cpu)->core;
> +
> +    return &pc->pnv_tod_tbst;
> +}
> +
>   static void tb_state_machine_step(CPUPPCState *env)
>   {
> +    PowerPCCPU *cpu = env_archcpu(env);
> +    struct pnv_tod_tbst *pnv_tod_tbst = cpu_get_tbst(cpu);

Since cpu is not used anywhere later, we could just do 
cpu_get_tbst(env_archcpu(env)) ?

>       uint64_t tfmr = env->spr[SPR_TFMR];
>       unsigned int tbst = tfmr_get_tb_state(tfmr);
>   
> @@ -307,15 +317,15 @@ static void tb_state_machine_step(CPUPPCState *env)
>           return;
>       }
>   
> -    if (env->pnv_tod_tbst.tb_sync_pulse_timer) {
> -        env->pnv_tod_tbst.tb_sync_pulse_timer--;
> +    if (pnv_tod_tbst->tb_sync_pulse_timer) {
> +        pnv_tod_tbst->tb_sync_pulse_timer--;
>       } else {
>           tfmr |= TFMR_TB_SYNC_OCCURED;
>           write_tfmr(env, tfmr);
>       }
>   
> -    if (env->pnv_tod_tbst.tb_state_timer) {
> -        env->pnv_tod_tbst.tb_state_timer--;
> +    if (pnv_tod_tbst->tb_state_timer) {
> +        pnv_tod_tbst->tb_state_timer--;
>           return;
>       }
>   
> @@ -332,20 +342,20 @@ static void tb_state_machine_step(CPUPPCState *env)
>       } else if (tfmr & TFMR_MOVE_CHIP_TOD_TO_TB) {
>           if (tbst == TBST_SYNC_WAIT) {
>               tfmr = tfmr_new_tb_state(tfmr, TBST_GET_TOD);
> -            env->pnv_tod_tbst.tb_state_timer = 3;
> +            pnv_tod_tbst->tb_state_timer = 3;
>           } else if (tbst == TBST_GET_TOD) {
> -            if (env->pnv_tod_tbst.tod_sent_to_tb) {
> +            if (pnv_tod_tbst->tod_sent_to_tb) {
>                   tfmr = tfmr_new_tb_state(tfmr, TBST_TB_RUNNING);
>                   tfmr &= ~TFMR_MOVE_CHIP_TOD_TO_TB;
> -                env->pnv_tod_tbst.tb_ready_for_tod = 0;
> -                env->pnv_tod_tbst.tod_sent_to_tb = 0;
> +                pnv_tod_tbst->tb_ready_for_tod = 0;
> +                pnv_tod_tbst->tod_sent_to_tb = 0;
>               }
>           } else {
>               qemu_log_mask(LOG_GUEST_ERROR, "TFMR error: MOVE_CHIP_TOD_TO_TB "
>                             "state machine in invalid state 0x%x\n", tbst);
>               tfmr = tfmr_new_tb_state(tfmr, TBST_TB_ERROR);
>               tfmr |= TFMR_FIRMWARE_CONTROL_ERROR;
> -            env->pnv_tod_tbst.tb_ready_for_tod = 0;
> +            pnv_tod_tbst->tb_ready_for_tod = 0;
>           }
>       }
>   
> @@ -361,6 +371,8 @@ target_ulong helper_load_tfmr(CPUPPCState *env)
>   
>   void helper_store_tfmr(CPUPPCState *env, target_ulong val)
>   {
> +    PowerPCCPU *cpu = env_archcpu(env);
> +    struct pnv_tod_tbst *pnv_tod_tbst = cpu_get_tbst(cpu);

... similarly here as well.

With suggested minor improvements,
Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

>       uint64_t tfmr = env->spr[SPR_TFMR];
>       uint64_t clear_on_write;
>       unsigned int tbst = tfmr_get_tb_state(tfmr);
> @@ -384,14 +396,7 @@ void helper_store_tfmr(CPUPPCState *env, target_ulong val)
>        * after the second mfspr.
>        */
>       tfmr &= ~TFMR_TB_SYNC_OCCURED;
> -    env->pnv_tod_tbst.tb_sync_pulse_timer = 1;
> -
> -    if (ppc_cpu_tir(env_archcpu(env)) != 0 &&
> -        (val & (TFMR_LOAD_TOD_MOD | TFMR_MOVE_CHIP_TOD_TO_TB))) {
> -        qemu_log_mask(LOG_UNIMP, "TFMR timebase state machine can only be "
> -                                 "driven by thread 0\n");
> -        goto out;
> -    }
> +    pnv_tod_tbst->tb_sync_pulse_timer = 1;
>   
>       if (((tfmr | val) & (TFMR_LOAD_TOD_MOD | TFMR_MOVE_CHIP_TOD_TO_TB)) ==
>                           (TFMR_LOAD_TOD_MOD | TFMR_MOVE_CHIP_TOD_TO_TB)) {
> @@ -399,7 +404,7 @@ void helper_store_tfmr(CPUPPCState *env, target_ulong val)
>                                          "MOVE_CHIP_TOD_TO_TB both set\n");
>           tfmr = tfmr_new_tb_state(tfmr, TBST_TB_ERROR);
>           tfmr |= TFMR_FIRMWARE_CONTROL_ERROR;
> -        env->pnv_tod_tbst.tb_ready_for_tod = 0;
> +        pnv_tod_tbst->tb_ready_for_tod = 0;
>           goto out;
>       }
>   
> @@ -413,8 +418,8 @@ void helper_store_tfmr(CPUPPCState *env, target_ulong val)
>           tfmr &= ~TFMR_LOAD_TOD_MOD;
>           tfmr &= ~TFMR_MOVE_CHIP_TOD_TO_TB;
>           tfmr &= ~TFMR_FIRMWARE_CONTROL_ERROR; /* XXX: should this be cleared? */
> -        env->pnv_tod_tbst.tb_ready_for_tod = 0;
> -        env->pnv_tod_tbst.tod_sent_to_tb = 0;
> +        pnv_tod_tbst->tb_ready_for_tod = 0;
> +        pnv_tod_tbst->tod_sent_to_tb = 0;
>           goto out;
>       }
>   
> @@ -427,19 +432,19 @@ void helper_store_tfmr(CPUPPCState *env, target_ulong val)
>   
>       if (tfmr & TFMR_LOAD_TOD_MOD) {
>           /* Wait for an arbitrary 3 mfspr until the next state transition. */
> -        env->pnv_tod_tbst.tb_state_timer = 3;
> +        pnv_tod_tbst->tb_state_timer = 3;
>       } else if (tfmr & TFMR_MOVE_CHIP_TOD_TO_TB) {
>           if (tbst == TBST_NOT_SET) {
>               tfmr = tfmr_new_tb_state(tfmr, TBST_SYNC_WAIT);
> -            env->pnv_tod_tbst.tb_ready_for_tod = 1;
> -            env->pnv_tod_tbst.tb_state_timer = 3; /* arbitrary */
> +            pnv_tod_tbst->tb_ready_for_tod = 1;
> +            pnv_tod_tbst->tb_state_timer = 3; /* arbitrary */
>           } else {
>               qemu_log_mask(LOG_GUEST_ERROR, "TFMR error: MOVE_CHIP_TOD_TO_TB "
>                                              "not in TB not set state 0x%x\n",
>                                              tbst);
>               tfmr = tfmr_new_tb_state(tfmr, TBST_TB_ERROR);
>               tfmr |= TFMR_FIRMWARE_CONTROL_ERROR;
> -            env->pnv_tod_tbst.tb_ready_for_tod = 0;
> +            pnv_tod_tbst->tb_ready_for_tod = 0;
>           }
>       }
>
Cédric Le Goater May 28, 2024, 7:52 a.m. UTC | #2
On 5/28/24 08:28, Harsh Prateek Bora wrote:
> 
> 
> On 5/26/24 17:56, Nicholas Piggin wrote:
>> The timebase state machine is per per-core state and can be driven
>> by any thread in the core. It is currently implemented as a hack
>> where the state is in a CPU structure and only thread 0's state is
>> accessed by the chiptod, which limits programming the timebase
>> side of the state machine to thread 0 of a core.
>>
>> Move the state out into PnvCore and share it among all threads.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>   include/hw/ppc/pnv_core.h    | 17 ++++++++++++
>>   target/ppc/cpu.h             | 20 --------------
>>   hw/ppc/pnv_chiptod.c         |  6 ++--
>>   target/ppc/timebase_helper.c | 53 ++++++++++++++++++++----------------
>>   4 files changed, 49 insertions(+), 47 deletions(-)
>>
>> diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
>> index 30c1e5b1a3..f434c71547 100644
>> --- a/include/hw/ppc/pnv_core.h
>> +++ b/include/hw/ppc/pnv_core.h
>> @@ -25,6 +25,20 @@
>>   #include "hw/ppc/pnv.h"
>>   #include "qom/object.h"
>> +/* ChipTOD and TimeBase State Machine */
>> +struct pnv_tod_tbst {
>> +    int tb_ready_for_tod; /* core TB ready to receive TOD from chiptod */
>> +    int tod_sent_to_tb;   /* chiptod sent TOD to the core TB */
>> +
>> +    /*
>> +     * "Timers" for async TBST events are simulated by mfTFAC because TFAC
>> +     * is polled for such events. These are just used to ensure firmware
>> +     * performs the polling at least a few times.
>> +     */
>> +    int tb_state_timer;
>> +    int tb_sync_pulse_timer;
>> +};
>> +
>>   #define TYPE_PNV_CORE "powernv-cpu-core"
>>   OBJECT_DECLARE_TYPE(PnvCore, PnvCoreClass,
>>                       PNV_CORE)
>> @@ -38,6 +52,9 @@ struct PnvCore {
>>       uint32_t pir;
>>       uint32_t hwid;
>>       uint64_t hrmor;
>> +
>> +    struct pnv_tod_tbst pnv_tod_tbst;
>> +
> 
> Now that it is part of struct PnvCore itself, we can drop pnv_ prefix
> and just call the member variable as tod_tbst ?

yes and rename pnv_tod_tbst using CamelCase please.


Thanks,

C.




> 
>>       PnvChip *chip;
>>       MemoryRegion xscom_regs;
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index 2015e603d4..1e86658da6 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -1196,21 +1196,6 @@ DEXCR_ASPECT(SRAPD, 4)
>>   DEXCR_ASPECT(NPHIE, 5)
>>   DEXCR_ASPECT(PHIE, 6)
>> -/*****************************************************************************/
>> -/* PowerNV ChipTOD and TimeBase State Machine */
>> -struct pnv_tod_tbst {
>> -    int tb_ready_for_tod; /* core TB ready to receive TOD from chiptod */
>> -    int tod_sent_to_tb;   /* chiptod sent TOD to the core TB */
>> -
>> -    /*
>> -     * "Timers" for async TBST events are simulated by mfTFAC because TFAC
>> -     * is polled for such events. These are just used to ensure firmware
>> -     * performs the polling at least a few times.
>> -     */
>> -    int tb_state_timer;
>> -    int tb_sync_pulse_timer;
>> -};
>> -
>>   /*****************************************************************************/
>>   /* The whole PowerPC CPU context */
>> @@ -1292,11 +1277,6 @@ struct CPUArchState {
>>   #define TLB_NEED_LOCAL_FLUSH   0x1
>>   #define TLB_NEED_GLOBAL_FLUSH  0x2
>> -#if defined(TARGET_PPC64)
>> -    /* PowerNV chiptod / timebase facility state. */
>> -    /* Would be nice to put these into PnvCore */
>> -    struct pnv_tod_tbst pnv_tod_tbst;
>> -#endif
>>   #endif
>>       /* Other registers */
>> diff --git a/hw/ppc/pnv_chiptod.c b/hw/ppc/pnv_chiptod.c
>> index 3831a72101..3eaddd66f0 100644
>> --- a/hw/ppc/pnv_chiptod.c
>> +++ b/hw/ppc/pnv_chiptod.c
>> @@ -365,7 +365,7 @@ static void pnv_chiptod_xscom_write(void *opaque, hwaddr addr,
>>                             " TOD_MOVE_TOD_TO_TB_REG with no slave target\n");
>>           } else {
>>               PowerPCCPU *cpu = chiptod->slave_pc_target->threads[0];
>> -            CPUPPCState *env = &cpu->env;
>> +            PnvCore *pc = pnv_cpu_state(cpu)->core;
>>               /*
>>                * Moving TOD to TB will set the TB of all threads in a
>> @@ -377,8 +377,8 @@ static void pnv_chiptod_xscom_write(void *opaque, hwaddr addr,
>>                * thread 0.
>>                */
>> -            if (env->pnv_tod_tbst.tb_ready_for_tod) {
>> -                env->pnv_tod_tbst.tod_sent_to_tb = 1;
>> +            if (pc->pnv_tod_tbst.tb_ready_for_tod) {
>> +                pc->pnv_tod_tbst.tod_sent_to_tb = 1;
>>               } else {
>>                   qemu_log_mask(LOG_GUEST_ERROR, "pnv_chiptod: xscom write reg"
>>                                 " TOD_MOVE_TOD_TO_TB_REG with TB not ready to"
>> diff --git a/target/ppc/timebase_helper.c b/target/ppc/timebase_helper.c
>> index 39d397416e..788c498d63 100644
>> --- a/target/ppc/timebase_helper.c
>> +++ b/target/ppc/timebase_helper.c
>> @@ -19,6 +19,7 @@
>>   #include "qemu/osdep.h"
>>   #include "cpu.h"
>>   #include "hw/ppc/ppc.h"
>> +#include "hw/ppc/pnv_core.h"
>>   #include "exec/helper-proto.h"
>>   #include "exec/exec-all.h"
>>   #include "qemu/log.h"
>> @@ -298,8 +299,17 @@ static void write_tfmr(CPUPPCState *env, target_ulong val)
>>       }
>>   }
>> +static struct pnv_tod_tbst *cpu_get_tbst(PowerPCCPU *cpu)
>> +{
>> +    PnvCore *pc = pnv_cpu_state(cpu)->core;
>> +
>> +    return &pc->pnv_tod_tbst;
>> +}
>> +
>>   static void tb_state_machine_step(CPUPPCState *env)
>>   {
>> +    PowerPCCPU *cpu = env_archcpu(env);
>> +    struct pnv_tod_tbst *pnv_tod_tbst = cpu_get_tbst(cpu);
> 
> Since cpu is not used anywhere later, we could just do cpu_get_tbst(env_archcpu(env)) ?
> 
>>       uint64_t tfmr = env->spr[SPR_TFMR];
>>       unsigned int tbst = tfmr_get_tb_state(tfmr);
>> @@ -307,15 +317,15 @@ static void tb_state_machine_step(CPUPPCState *env)
>>           return;
>>       }
>> -    if (env->pnv_tod_tbst.tb_sync_pulse_timer) {
>> -        env->pnv_tod_tbst.tb_sync_pulse_timer--;
>> +    if (pnv_tod_tbst->tb_sync_pulse_timer) {
>> +        pnv_tod_tbst->tb_sync_pulse_timer--;
>>       } else {
>>           tfmr |= TFMR_TB_SYNC_OCCURED;
>>           write_tfmr(env, tfmr);
>>       }
>> -    if (env->pnv_tod_tbst.tb_state_timer) {
>> -        env->pnv_tod_tbst.tb_state_timer--;
>> +    if (pnv_tod_tbst->tb_state_timer) {
>> +        pnv_tod_tbst->tb_state_timer--;
>>           return;
>>       }
>> @@ -332,20 +342,20 @@ static void tb_state_machine_step(CPUPPCState *env)
>>       } else if (tfmr & TFMR_MOVE_CHIP_TOD_TO_TB) {
>>           if (tbst == TBST_SYNC_WAIT) {
>>               tfmr = tfmr_new_tb_state(tfmr, TBST_GET_TOD);
>> -            env->pnv_tod_tbst.tb_state_timer = 3;
>> +            pnv_tod_tbst->tb_state_timer = 3;
>>           } else if (tbst == TBST_GET_TOD) {
>> -            if (env->pnv_tod_tbst.tod_sent_to_tb) {
>> +            if (pnv_tod_tbst->tod_sent_to_tb) {
>>                   tfmr = tfmr_new_tb_state(tfmr, TBST_TB_RUNNING);
>>                   tfmr &= ~TFMR_MOVE_CHIP_TOD_TO_TB;
>> -                env->pnv_tod_tbst.tb_ready_for_tod = 0;
>> -                env->pnv_tod_tbst.tod_sent_to_tb = 0;
>> +                pnv_tod_tbst->tb_ready_for_tod = 0;
>> +                pnv_tod_tbst->tod_sent_to_tb = 0;
>>               }
>>           } else {
>>               qemu_log_mask(LOG_GUEST_ERROR, "TFMR error: MOVE_CHIP_TOD_TO_TB "
>>                             "state machine in invalid state 0x%x\n", tbst);
>>               tfmr = tfmr_new_tb_state(tfmr, TBST_TB_ERROR);
>>               tfmr |= TFMR_FIRMWARE_CONTROL_ERROR;
>> -            env->pnv_tod_tbst.tb_ready_for_tod = 0;
>> +            pnv_tod_tbst->tb_ready_for_tod = 0;
>>           }
>>       }
>> @@ -361,6 +371,8 @@ target_ulong helper_load_tfmr(CPUPPCState *env)
>>   void helper_store_tfmr(CPUPPCState *env, target_ulong val)
>>   {
>> +    PowerPCCPU *cpu = env_archcpu(env);
>> +    struct pnv_tod_tbst *pnv_tod_tbst = cpu_get_tbst(cpu);
> 
> ... similarly here as well.
> 
> With suggested minor improvements,
> Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> 
>>       uint64_t tfmr = env->spr[SPR_TFMR];
>>       uint64_t clear_on_write;
>>       unsigned int tbst = tfmr_get_tb_state(tfmr);
>> @@ -384,14 +396,7 @@ void helper_store_tfmr(CPUPPCState *env, target_ulong val)
>>        * after the second mfspr.
>>        */
>>       tfmr &= ~TFMR_TB_SYNC_OCCURED;
>> -    env->pnv_tod_tbst.tb_sync_pulse_timer = 1;
>> -
>> -    if (ppc_cpu_tir(env_archcpu(env)) != 0 &&
>> -        (val & (TFMR_LOAD_TOD_MOD | TFMR_MOVE_CHIP_TOD_TO_TB))) {
>> -        qemu_log_mask(LOG_UNIMP, "TFMR timebase state machine can only be "
>> -                                 "driven by thread 0\n");
>> -        goto out;
>> -    }
>> +    pnv_tod_tbst->tb_sync_pulse_timer = 1;
>>       if (((tfmr | val) & (TFMR_LOAD_TOD_MOD | TFMR_MOVE_CHIP_TOD_TO_TB)) ==
>>                           (TFMR_LOAD_TOD_MOD | TFMR_MOVE_CHIP_TOD_TO_TB)) {
>> @@ -399,7 +404,7 @@ void helper_store_tfmr(CPUPPCState *env, target_ulong val)
>>                                          "MOVE_CHIP_TOD_TO_TB both set\n");
>>           tfmr = tfmr_new_tb_state(tfmr, TBST_TB_ERROR);
>>           tfmr |= TFMR_FIRMWARE_CONTROL_ERROR;
>> -        env->pnv_tod_tbst.tb_ready_for_tod = 0;
>> +        pnv_tod_tbst->tb_ready_for_tod = 0;
>>           goto out;
>>       }
>> @@ -413,8 +418,8 @@ void helper_store_tfmr(CPUPPCState *env, target_ulong val)
>>           tfmr &= ~TFMR_LOAD_TOD_MOD;
>>           tfmr &= ~TFMR_MOVE_CHIP_TOD_TO_TB;
>>           tfmr &= ~TFMR_FIRMWARE_CONTROL_ERROR; /* XXX: should this be cleared? */
>> -        env->pnv_tod_tbst.tb_ready_for_tod = 0;
>> -        env->pnv_tod_tbst.tod_sent_to_tb = 0;
>> +        pnv_tod_tbst->tb_ready_for_tod = 0;
>> +        pnv_tod_tbst->tod_sent_to_tb = 0;
>>           goto out;
>>       }
>> @@ -427,19 +432,19 @@ void helper_store_tfmr(CPUPPCState *env, target_ulong val)
>>       if (tfmr & TFMR_LOAD_TOD_MOD) {
>>           /* Wait for an arbitrary 3 mfspr until the next state transition. */
>> -        env->pnv_tod_tbst.tb_state_timer = 3;
>> +        pnv_tod_tbst->tb_state_timer = 3;
>>       } else if (tfmr & TFMR_MOVE_CHIP_TOD_TO_TB) {
>>           if (tbst == TBST_NOT_SET) {
>>               tfmr = tfmr_new_tb_state(tfmr, TBST_SYNC_WAIT);
>> -            env->pnv_tod_tbst.tb_ready_for_tod = 1;
>> -            env->pnv_tod_tbst.tb_state_timer = 3; /* arbitrary */
>> +            pnv_tod_tbst->tb_ready_for_tod = 1;
>> +            pnv_tod_tbst->tb_state_timer = 3; /* arbitrary */
>>           } else {
>>               qemu_log_mask(LOG_GUEST_ERROR, "TFMR error: MOVE_CHIP_TOD_TO_TB "
>>                                              "not in TB not set state 0x%x\n",
>>                                              tbst);
>>               tfmr = tfmr_new_tb_state(tfmr, TBST_TB_ERROR);
>>               tfmr |= TFMR_FIRMWARE_CONTROL_ERROR;
>> -            env->pnv_tod_tbst.tb_ready_for_tod = 0;
>> +            pnv_tod_tbst->tb_ready_for_tod = 0;
>>           }
>>       }
Nicholas Piggin May 29, 2024, 12:19 a.m. UTC | #3
On Tue May 28, 2024 at 5:52 PM AEST, Cédric Le Goater wrote:
> On 5/28/24 08:28, Harsh Prateek Bora wrote:
> > 
> > 
> > On 5/26/24 17:56, Nicholas Piggin wrote:
> >> The timebase state machine is per per-core state and can be driven
> >> by any thread in the core. It is currently implemented as a hack
> >> where the state is in a CPU structure and only thread 0's state is
> >> accessed by the chiptod, which limits programming the timebase
> >> side of the state machine to thread 0 of a core.
> >>
> >> Move the state out into PnvCore and share it among all threads.
> >>
> >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> >> ---
> >>   include/hw/ppc/pnv_core.h    | 17 ++++++++++++
> >>   target/ppc/cpu.h             | 20 --------------
> >>   hw/ppc/pnv_chiptod.c         |  6 ++--
> >>   target/ppc/timebase_helper.c | 53 ++++++++++++++++++++----------------
> >>   4 files changed, 49 insertions(+), 47 deletions(-)
> >>
> >> diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
> >> index 30c1e5b1a3..f434c71547 100644
> >> --- a/include/hw/ppc/pnv_core.h
> >> +++ b/include/hw/ppc/pnv_core.h
> >> @@ -25,6 +25,20 @@
> >>   #include "hw/ppc/pnv.h"
> >>   #include "qom/object.h"
> >> +/* ChipTOD and TimeBase State Machine */
> >> +struct pnv_tod_tbst {
> >> +    int tb_ready_for_tod; /* core TB ready to receive TOD from chiptod */
> >> +    int tod_sent_to_tb;   /* chiptod sent TOD to the core TB */
> >> +
> >> +    /*
> >> +     * "Timers" for async TBST events are simulated by mfTFAC because TFAC
> >> +     * is polled for such events. These are just used to ensure firmware
> >> +     * performs the polling at least a few times.
> >> +     */
> >> +    int tb_state_timer;
> >> +    int tb_sync_pulse_timer;
> >> +};
> >> +
> >>   #define TYPE_PNV_CORE "powernv-cpu-core"
> >>   OBJECT_DECLARE_TYPE(PnvCore, PnvCoreClass,
> >>                       PNV_CORE)
> >> @@ -38,6 +52,9 @@ struct PnvCore {
> >>       uint32_t pir;
> >>       uint32_t hwid;
> >>       uint64_t hrmor;
> >> +
> >> +    struct pnv_tod_tbst pnv_tod_tbst;
> >> +
> > 
> > Now that it is part of struct PnvCore itself, we can drop pnv_ prefix
> > and just call the member variable as tod_tbst ?
>
> yes and rename pnv_tod_tbst using CamelCase please.

Okay will do. That'll look nicer.

Thanks,
Nick
diff mbox series

Patch

diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
index 30c1e5b1a3..f434c71547 100644
--- a/include/hw/ppc/pnv_core.h
+++ b/include/hw/ppc/pnv_core.h
@@ -25,6 +25,20 @@ 
 #include "hw/ppc/pnv.h"
 #include "qom/object.h"
 
+/* ChipTOD and TimeBase State Machine */
+struct pnv_tod_tbst {
+    int tb_ready_for_tod; /* core TB ready to receive TOD from chiptod */
+    int tod_sent_to_tb;   /* chiptod sent TOD to the core TB */
+
+    /*
+     * "Timers" for async TBST events are simulated by mfTFAC because TFAC
+     * is polled for such events. These are just used to ensure firmware
+     * performs the polling at least a few times.
+     */
+    int tb_state_timer;
+    int tb_sync_pulse_timer;
+};
+
 #define TYPE_PNV_CORE "powernv-cpu-core"
 OBJECT_DECLARE_TYPE(PnvCore, PnvCoreClass,
                     PNV_CORE)
@@ -38,6 +52,9 @@  struct PnvCore {
     uint32_t pir;
     uint32_t hwid;
     uint64_t hrmor;
+
+    struct pnv_tod_tbst pnv_tod_tbst;
+
     PnvChip *chip;
 
     MemoryRegion xscom_regs;
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 2015e603d4..1e86658da6 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1196,21 +1196,6 @@  DEXCR_ASPECT(SRAPD, 4)
 DEXCR_ASPECT(NPHIE, 5)
 DEXCR_ASPECT(PHIE, 6)
 
-/*****************************************************************************/
-/* PowerNV ChipTOD and TimeBase State Machine */
-struct pnv_tod_tbst {
-    int tb_ready_for_tod; /* core TB ready to receive TOD from chiptod */
-    int tod_sent_to_tb;   /* chiptod sent TOD to the core TB */
-
-    /*
-     * "Timers" for async TBST events are simulated by mfTFAC because TFAC
-     * is polled for such events. These are just used to ensure firmware
-     * performs the polling at least a few times.
-     */
-    int tb_state_timer;
-    int tb_sync_pulse_timer;
-};
-
 /*****************************************************************************/
 /* The whole PowerPC CPU context */
 
@@ -1292,11 +1277,6 @@  struct CPUArchState {
 #define TLB_NEED_LOCAL_FLUSH   0x1
 #define TLB_NEED_GLOBAL_FLUSH  0x2
 
-#if defined(TARGET_PPC64)
-    /* PowerNV chiptod / timebase facility state. */
-    /* Would be nice to put these into PnvCore */
-    struct pnv_tod_tbst pnv_tod_tbst;
-#endif
 #endif
 
     /* Other registers */
diff --git a/hw/ppc/pnv_chiptod.c b/hw/ppc/pnv_chiptod.c
index 3831a72101..3eaddd66f0 100644
--- a/hw/ppc/pnv_chiptod.c
+++ b/hw/ppc/pnv_chiptod.c
@@ -365,7 +365,7 @@  static void pnv_chiptod_xscom_write(void *opaque, hwaddr addr,
                           " TOD_MOVE_TOD_TO_TB_REG with no slave target\n");
         } else {
             PowerPCCPU *cpu = chiptod->slave_pc_target->threads[0];
-            CPUPPCState *env = &cpu->env;
+            PnvCore *pc = pnv_cpu_state(cpu)->core;
 
             /*
              * Moving TOD to TB will set the TB of all threads in a
@@ -377,8 +377,8 @@  static void pnv_chiptod_xscom_write(void *opaque, hwaddr addr,
              * thread 0.
              */
 
-            if (env->pnv_tod_tbst.tb_ready_for_tod) {
-                env->pnv_tod_tbst.tod_sent_to_tb = 1;
+            if (pc->pnv_tod_tbst.tb_ready_for_tod) {
+                pc->pnv_tod_tbst.tod_sent_to_tb = 1;
             } else {
                 qemu_log_mask(LOG_GUEST_ERROR, "pnv_chiptod: xscom write reg"
                               " TOD_MOVE_TOD_TO_TB_REG with TB not ready to"
diff --git a/target/ppc/timebase_helper.c b/target/ppc/timebase_helper.c
index 39d397416e..788c498d63 100644
--- a/target/ppc/timebase_helper.c
+++ b/target/ppc/timebase_helper.c
@@ -19,6 +19,7 @@ 
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "hw/ppc/ppc.h"
+#include "hw/ppc/pnv_core.h"
 #include "exec/helper-proto.h"
 #include "exec/exec-all.h"
 #include "qemu/log.h"
@@ -298,8 +299,17 @@  static void write_tfmr(CPUPPCState *env, target_ulong val)
     }
 }
 
+static struct pnv_tod_tbst *cpu_get_tbst(PowerPCCPU *cpu)
+{
+    PnvCore *pc = pnv_cpu_state(cpu)->core;
+
+    return &pc->pnv_tod_tbst;
+}
+
 static void tb_state_machine_step(CPUPPCState *env)
 {
+    PowerPCCPU *cpu = env_archcpu(env);
+    struct pnv_tod_tbst *pnv_tod_tbst = cpu_get_tbst(cpu);
     uint64_t tfmr = env->spr[SPR_TFMR];
     unsigned int tbst = tfmr_get_tb_state(tfmr);
 
@@ -307,15 +317,15 @@  static void tb_state_machine_step(CPUPPCState *env)
         return;
     }
 
-    if (env->pnv_tod_tbst.tb_sync_pulse_timer) {
-        env->pnv_tod_tbst.tb_sync_pulse_timer--;
+    if (pnv_tod_tbst->tb_sync_pulse_timer) {
+        pnv_tod_tbst->tb_sync_pulse_timer--;
     } else {
         tfmr |= TFMR_TB_SYNC_OCCURED;
         write_tfmr(env, tfmr);
     }
 
-    if (env->pnv_tod_tbst.tb_state_timer) {
-        env->pnv_tod_tbst.tb_state_timer--;
+    if (pnv_tod_tbst->tb_state_timer) {
+        pnv_tod_tbst->tb_state_timer--;
         return;
     }
 
@@ -332,20 +342,20 @@  static void tb_state_machine_step(CPUPPCState *env)
     } else if (tfmr & TFMR_MOVE_CHIP_TOD_TO_TB) {
         if (tbst == TBST_SYNC_WAIT) {
             tfmr = tfmr_new_tb_state(tfmr, TBST_GET_TOD);
-            env->pnv_tod_tbst.tb_state_timer = 3;
+            pnv_tod_tbst->tb_state_timer = 3;
         } else if (tbst == TBST_GET_TOD) {
-            if (env->pnv_tod_tbst.tod_sent_to_tb) {
+            if (pnv_tod_tbst->tod_sent_to_tb) {
                 tfmr = tfmr_new_tb_state(tfmr, TBST_TB_RUNNING);
                 tfmr &= ~TFMR_MOVE_CHIP_TOD_TO_TB;
-                env->pnv_tod_tbst.tb_ready_for_tod = 0;
-                env->pnv_tod_tbst.tod_sent_to_tb = 0;
+                pnv_tod_tbst->tb_ready_for_tod = 0;
+                pnv_tod_tbst->tod_sent_to_tb = 0;
             }
         } else {
             qemu_log_mask(LOG_GUEST_ERROR, "TFMR error: MOVE_CHIP_TOD_TO_TB "
                           "state machine in invalid state 0x%x\n", tbst);
             tfmr = tfmr_new_tb_state(tfmr, TBST_TB_ERROR);
             tfmr |= TFMR_FIRMWARE_CONTROL_ERROR;
-            env->pnv_tod_tbst.tb_ready_for_tod = 0;
+            pnv_tod_tbst->tb_ready_for_tod = 0;
         }
     }
 
@@ -361,6 +371,8 @@  target_ulong helper_load_tfmr(CPUPPCState *env)
 
 void helper_store_tfmr(CPUPPCState *env, target_ulong val)
 {
+    PowerPCCPU *cpu = env_archcpu(env);
+    struct pnv_tod_tbst *pnv_tod_tbst = cpu_get_tbst(cpu);
     uint64_t tfmr = env->spr[SPR_TFMR];
     uint64_t clear_on_write;
     unsigned int tbst = tfmr_get_tb_state(tfmr);
@@ -384,14 +396,7 @@  void helper_store_tfmr(CPUPPCState *env, target_ulong val)
      * after the second mfspr.
      */
     tfmr &= ~TFMR_TB_SYNC_OCCURED;
-    env->pnv_tod_tbst.tb_sync_pulse_timer = 1;
-
-    if (ppc_cpu_tir(env_archcpu(env)) != 0 &&
-        (val & (TFMR_LOAD_TOD_MOD | TFMR_MOVE_CHIP_TOD_TO_TB))) {
-        qemu_log_mask(LOG_UNIMP, "TFMR timebase state machine can only be "
-                                 "driven by thread 0\n");
-        goto out;
-    }
+    pnv_tod_tbst->tb_sync_pulse_timer = 1;
 
     if (((tfmr | val) & (TFMR_LOAD_TOD_MOD | TFMR_MOVE_CHIP_TOD_TO_TB)) ==
                         (TFMR_LOAD_TOD_MOD | TFMR_MOVE_CHIP_TOD_TO_TB)) {
@@ -399,7 +404,7 @@  void helper_store_tfmr(CPUPPCState *env, target_ulong val)
                                        "MOVE_CHIP_TOD_TO_TB both set\n");
         tfmr = tfmr_new_tb_state(tfmr, TBST_TB_ERROR);
         tfmr |= TFMR_FIRMWARE_CONTROL_ERROR;
-        env->pnv_tod_tbst.tb_ready_for_tod = 0;
+        pnv_tod_tbst->tb_ready_for_tod = 0;
         goto out;
     }
 
@@ -413,8 +418,8 @@  void helper_store_tfmr(CPUPPCState *env, target_ulong val)
         tfmr &= ~TFMR_LOAD_TOD_MOD;
         tfmr &= ~TFMR_MOVE_CHIP_TOD_TO_TB;
         tfmr &= ~TFMR_FIRMWARE_CONTROL_ERROR; /* XXX: should this be cleared? */
-        env->pnv_tod_tbst.tb_ready_for_tod = 0;
-        env->pnv_tod_tbst.tod_sent_to_tb = 0;
+        pnv_tod_tbst->tb_ready_for_tod = 0;
+        pnv_tod_tbst->tod_sent_to_tb = 0;
         goto out;
     }
 
@@ -427,19 +432,19 @@  void helper_store_tfmr(CPUPPCState *env, target_ulong val)
 
     if (tfmr & TFMR_LOAD_TOD_MOD) {
         /* Wait for an arbitrary 3 mfspr until the next state transition. */
-        env->pnv_tod_tbst.tb_state_timer = 3;
+        pnv_tod_tbst->tb_state_timer = 3;
     } else if (tfmr & TFMR_MOVE_CHIP_TOD_TO_TB) {
         if (tbst == TBST_NOT_SET) {
             tfmr = tfmr_new_tb_state(tfmr, TBST_SYNC_WAIT);
-            env->pnv_tod_tbst.tb_ready_for_tod = 1;
-            env->pnv_tod_tbst.tb_state_timer = 3; /* arbitrary */
+            pnv_tod_tbst->tb_ready_for_tod = 1;
+            pnv_tod_tbst->tb_state_timer = 3; /* arbitrary */
         } else {
             qemu_log_mask(LOG_GUEST_ERROR, "TFMR error: MOVE_CHIP_TOD_TO_TB "
                                            "not in TB not set state 0x%x\n",
                                            tbst);
             tfmr = tfmr_new_tb_state(tfmr, TBST_TB_ERROR);
             tfmr |= TFMR_FIRMWARE_CONTROL_ERROR;
-            env->pnv_tod_tbst.tb_ready_for_tod = 0;
+            pnv_tod_tbst->tb_ready_for_tod = 0;
         }
     }