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

["Nicholas Piggin" <npiggin@gmail.com>]

On Fri Nov 24, 2023 at 4:34 AM AEST, Cédric Le Goater wrote:
> 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.

Done.

> > +{
> > +    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()

Done.

> > @@ -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.

Yes.

>
> > +        } else { /* PIR addressing */
> > +            uint32_t core_id;
>
> I suppose "PIR addressing" is the previous way of doing the same.

Yes, I should have written "core ID addresing", and it seems to do the
same thing, but just gives you a shortcut to find the PC_TOD scom
address for that core. The WB basically says they are equivalent AFAIKS.

>
> > +
> > +            if (!is_power9) {
>
> Please transform 'is_power9' into a class attribute
>
>     bool PnvChipTODClass::pir_addressing

I made the entire transmit operation a class function, which works
okay. There's possibly more than one quirk in HW (based on my trouble
making skiboot work on real HW) so it could be useful to model various
other things too.

> > +        } 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 !

Yes, workbook says to poll TFMR[18] on the target core after this scom.

It is a bit weird, I guess there's a lot of history (and features I
don't understand) behind it. TFMR can be read via SCOM in the core PC
unit I think, so it could be programmed more naturally that way. Not
that we model SCOM access to PC registers AFAIK...

All other formatting, naming, wording comments I snipped, I agree with
and havechanged.

Thanks,
Nick