Re: [PATCH 5/7] pnv/chiptod: Implement the ChipTOD to Core transfer

[Cédric Le Goater]

On 11/23/23 11:30, Nicholas Piggin wrote:
> One of the functions of the ChipTOD is to transfer TOD to the Core
> (aka PC - Pervasive Core) timebase facility.
>
> The ChipTOD can be programmed with a target address to send the TOD
> value to. The hardware implementation seems to perform this by
> sending the TOD value to a SCOM addres.

address

> This implementation grabs the core directly and manipulates the
> timebase facility state in the core. This is a hack, but it works
> enough for now. A better implementation would implement the transfer
> to the PnvCore xscom register and drive the timebase state machine
> from there.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   include/hw/ppc/pnv_chiptod.h |  3 ++
>   include/hw/ppc/pnv_core.h    |  4 ++
>   target/ppc/cpu.h             |  7 +++
>   hw/ppc/pnv.c                 | 33 +++++++++++++
>   hw/ppc/pnv_chiptod.c         | 92 ++++++++++++++++++++++++++++++++++++
>   5 files changed, 139 insertions(+)
>
> diff --git a/include/hw/ppc/pnv_chiptod.h b/include/hw/ppc/pnv_chiptod.h
> index e2765c3bfc..ffcc376e80 100644
> --- a/include/hw/ppc/pnv_chiptod.h
> +++ b/include/hw/ppc/pnv_chiptod.h
> @@ -29,6 +29,8 @@ enum tod_state {
>       tod_stopped = 1,
>   };
>   
> +typedef struct PnvCore PnvCore;
> +
>   struct PnvChipTOD {
>       DeviceState xd;
>   
> @@ -40,6 +42,7 @@ struct PnvChipTOD {
>       enum tod_state tod_state;
>       uint64_t tod_error;
>       uint64_t pss_mss_ctrl_reg;
> +    PnvCore *slave_pc_target;
>   };
>   
>   struct PnvChipTODClass {
> diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
> index 4db21229a6..5f52ae7dbf 100644
> --- a/include/hw/ppc/pnv_core.h
> +++ b/include/hw/ppc/pnv_core.h
> @@ -85,4 +85,8 @@ struct PnvQuad {
>       MemoryRegion xscom_regs;
>       MemoryRegion xscom_qme_regs;
>   };
> +
> +PnvCore *pnv_get_core_by_xscom_base(PnvChip *chip, uint32_t xscom_base);
> +PnvCore *pnv_get_core_by_id(PnvChip *chip, uint32_t core_id);
> +
>   #endif /* PPC_PNV_CORE_H */
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 848e583c2d..8df5626939 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1258,6 +1258,13 @@ struct CPUArchState {
>       uint32_t tlb_need_flush; /* Delayed flush needed */
>   #define TLB_NEED_LOCAL_FLUSH   0x1
>   #define TLB_NEED_GLOBAL_FLUSH  0x2
> +
> +#if defined(TARGET_PPC64)
> +    /* Would be nice to put these into PnvCore */

This is going to be complex to do from the insn implementation.


> +    /* PowerNV chiptod / timebase facility state. */
> +    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 */
> +#endif
>   #endif
>   
>       /* Other registers */
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 546266ae3d..fa0dc26732 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -2032,6 +2032,39 @@ static void pnv_chip_core_realize(PnvChip *chip, Error 
> **errp)
>       }
>   }
>   
> +PnvCore *pnv_get_core_by_xscom_base(PnvChip *chip, uint32_t xscom_base)

please use a pnv_chip_ prefix and move this routine definition in file
pnv_chiptod.c if possible. We don't want this routine to be used too
widely ... if not possible, please add a comment saying this is a
shortcut to avoid complex modeling of HW which is not exposed to the
software. In any case, add the comment.

> +{
> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
> +    int i;
> +
> +    for (i = 0; i < chip->nr_cores; i++) {
> +        PnvCore *pc = chip->cores[i];
> +        CPUCore *cc = CPU_CORE(pc);
> +        int core_hwid = cc->core_id;
> +
> +        if (pcc->xscom_core_base(chip, core_hwid) == xscom_base) {
> +            return pc;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +PnvCore *pnv_get_core_by_id(PnvChip *chip, uint32_t core_id)

please use a pnv_chip_ prefix and move this routine definition close
to pnv_chip_find_cpu()

> +{
> +    int i;
> +
> +    for (i = 0; i < chip->nr_cores; i++) {
> +        PnvCore *pc = chip->cores[i];
> +        CPUCore *cc = CPU_CORE(pc);
> +
> +        if (cc->core_id == core_id) {
> +            return pc;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +
>   static void pnv_chip_realize(DeviceState *dev, Error **errp)
>   {
>       PnvChip *chip = PNV_CHIP(dev);
> diff --git a/hw/ppc/pnv_chiptod.c b/hw/ppc/pnv_chiptod.c
> index a7bfe4298d..a2c1c83800 100644
> --- a/hw/ppc/pnv_chiptod.c
> +++ b/hw/ppc/pnv_chiptod.c
> @@ -201,6 +201,62 @@ static void pnv_chiptod_xscom_write(void *opaque, hwaddr 
> addr,
>           chiptod->pss_mss_ctrl_reg = val & PPC_BITMASK(0, 31);
>           break;
>   
> +    case TOD_TX_TTYPE_CTRL_REG:
> +        /*
> +         * This register sets the target of the TOD value transfer initiated
> +         * by TOD_MOVE_TOD_TO_TB. The TOD is able to send the address to
> +         * any target register, though in practice only the PC TOD register
> +         * should be used. ChipTOD has a "SCOM addressing" mode which fully
> +         * specifies the SCOM address, and a core-ID mode which uses the
> +         * core ID to target the PC TOD for a given core.
> +         *
> +         * skiboot uses SCOM for P10 and ID for P9, possibly due to hardware
> +         * weirdness. For this reason, that is all we implement here.
> +         */
> +        if (val & PPC_BIT(35)) { /* SCOM addressing */
> +            uint32_t addr2 = val >> 32;
> +            uint32_t reg = addr2 & 0xfff;
> +
> +            if (reg != PC_TOD) {
> +                qemu_log_mask(LOG_GUEST_ERROR, "pnv_chiptod: SCOM addressing: "
> +                              "unimplemented slave register 0x%" PRIx32 "\n",
> +                              reg);
> +                return;
> +            }
> +
> +            /*
> +             * This may not deal with P10 big-core addressing at the moment.
> +             * The big-core code in skiboot syncs small cores, but it targets
> +             * the even PIR (first small-core) when syncing second small-core.
> +             */
> +            chiptod->slave_pc_target =
> +                    pnv_get_core_by_xscom_base(chiptod->chip, addr2 & ~0xfff);
> +            if (!chiptod->slave_pc_target) {
> +                qemu_log_mask(LOG_GUEST_ERROR, "pnv_chiptod: xscom write reg"
> +                              " TOD_TX_TTYPE_CTRL_REG val 0x%" PRIx64
> +                              " invalid slave XSCOM address\n", val);
> +            }
So this is preparing the chiptod to initiate a SCOM memop on the
targetted core.

> +        } else { /* PIR addressing */
> +            uint32_t core_id;

I suppose "PIR addressing" is the previous way of doing the same.

> +
> +            if (!is_power9) {

Please transform 'is_power9' into a class attribute

   bool PnvChipTODClass::pir_addressing

> +                qemu_log_mask(LOG_GUEST_ERROR, "pnv_chiptod: PIR addressing"
> +                              " is only implemented for POWER9\n");

".... for POWER10" or "for this CPU"


> +                return;
> +            }
> +
> +            core_id = GETFIELD(TOD_TX_TTYPE_PIB_SLAVE_ADDR, val) & 0x1f;
> +            chiptod->slave_pc_target = pnv_get_core_by_id(chiptod->chip,
> +                                                           core_id);
> +            if (!chiptod->slave_pc_target) {
> +                qemu_log_mask(LOG_GUEST_ERROR, "pnv_chiptod: xscom write reg"
> +                              " TOD_TX_TTYPE_CTRL_REG val 0x%" PRIx64
> +                              " invalid slave core ID 0x%" PRIx32 "\n",
> +                              val, core_id);
> +            }
> +        }
> +        break;
>       case TOD_ERROR_REG:
>           chiptod->tod_error &= ~val;
>           break;
> @@ -215,6 +271,42 @@ static void pnv_chiptod_xscom_write(void *opaque, hwaddr 
> addr,
>       case TOD_LOAD_TOD_REG:
>           chiptod->tod_state = tod_running;
>           break;
> +    case TOD_MOVE_TOD_TO_TB_REG:
> +        /*
> +         * XXX: it should be a cleaner model to have this drive a SCOM
> +         * transaction to the target address, and implement the state machine
> +         * in the PnvCore. For now, this hack makes things work.
> +         */
> +        if (!(val & PPC_BIT(0))) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "pnv_chiptod: xscom write reg"
> +                          " TOD_MOVE_TOD_TO_TB_REG with bad val 0x%016lx\n",

Use PRIx64 please


> +                          val);
> +        } else if (chiptod->slave_pc_target == NULL) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "pnv_chiptod: xscom write reg"
> +                          " TOD_MOVE_TOD_TO_TB_REG with no slave target\n");
> +        } else {
> +            PowerPCCPU *cpu = chiptod->slave_pc_target->threads[0];
> +            CPUPPCState *env = &cpu->env;
> +
> +            /*
> +             * Moving TOD to TB will set the TB of all threads in a
> +             * core, so skiboot only does this once per thread0, so
> +             * that is where we keep the timebase state machine.
> +             *
> +             * It is likely possible for TBST to be driven from other
> +             * threads in the core, but for now we only implement it for
> +             * thread 0.
> +             */


and here, the memop is done and the status of the transaction should be
read in SPR_TFMR. This is not a friendly HW interface !


Thanks,

C.



> +            if (env->tb_ready_for_tod) {
> +                env->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"
> +                              " receive TOD\n");
> +            }
> +        }
> +        break;
>       case TOD_TX_TTYPE_4_REG:
>           if (is_power9) {
>               chiptod_power9_send_remotes(chiptod);