Re: [PATCH 1/4] pnv/chiptod: Add POWER9/10 chiptod model

[Nicholas Piggin]

On Tue Jun 6, 2023 at 12:57 AM AEST, Cédric Le Goater wrote:
> On 6/4/23 01:36, Nicholas Piggin wrote:

> > diff --git a/hw/ppc/pnv_chiptod.c b/hw/ppc/pnv_chiptod.c
> > new file mode 100644
> > index 0000000000..04ef703e0f
> > --- /dev/null
> > +++ b/hw/ppc/pnv_chiptod.c
> > @@ -0,0 +1,488 @@
> > +/*
> > + * QEMU PowerPC PowerNV Emulation of some CHIPTOD behaviour
> > + *
> > + * Copyright (c) 2022-2023, IBM Corporation.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License, version 2, as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, see <http://www.gnu.org/licenses/>.
>
> You can simplify the header with
>
>   * SPDX-License-Identifier: GPL-2.0-or-later

Sure.

> > +
> > +static void chiptod_power9_send_remotes(PnvChipTOD *chiptod)
>
> Adding a class handler could be an alternative implementation.

Might be a good idea, I'll see how it looks.

> > +{
> > +    PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
> > +    int i;
> > +
> > +    for (i = 0; i < pnv->num_chips; i++) {
>
> There are a few other models (XIVE, XIVE2) which loop on the chips,
> is it time to introduce a pnv_foreach_chip(fn, data) routine ?
>
>
> > +        Pnv9Chip *chip9 = PNV9_CHIP(pnv->chips[i]);
> > +        if (&chip9->chiptod != chiptod) {
> > +            chip9->chiptod.tod_state = tod_running;
> > +        }
> > +    }
> > +}

[snip]

> > +    case TOD_TX_TTYPE_CTRL_REG:
> > +        if (val & PPC_BIT(35)) { /* SCOM addressing */
> > +            uint32_t addr = val >> 32;
> > +            uint32_t reg = addr & 0xfff;
> > +            PnvCore *pc;
> > +
> > +            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.
> > +             */
> > +            pc = pnv_get_vcpu_by_xscom_base(chiptod->chip, addr & ~0xfff);
>
> hmm, couldn't we map xscom subregions, one for each thread instead ?

I'm not sure what you mean. This scom-addressing uses the xscom
address of the core's PC unit (where its time facilities are) to
point nest chiptod transfers to cores.

>
> > +            if (pc) {
> > +                /*
> > +                 * If TCG implements SMT, TFMR is a per-core SPR and should
> > +                 * be updated such that it is reflected in all threads.
> > +                 * Same for TB if the chiptod model ever actually adjusted 
> > it.
> > +                 */
> > +                chiptod->slave_cpu_target = pc->threads[0];
>
> ah ! SMT is implemented :) The xscom subregions would help to get the
> CPU pointer.

I think I may be mistaken at least in the "get_vcpu" part. I think
it should be get core, I don't know if chiptod is capable of addressing
threads individually.

I could test it with SMT patches and see what skiboot does.

> > +static int pnv_chiptod_dt_xscom(PnvXScomInterface *dev, void *fdt,
> > +                                int xscom_offset,
> > +                                const char compat[], size_t compat_size)
> > +{
> > +    PnvChipTOD *chiptod = PNV_CHIPTOD(dev);
> > +    g_autofree char *name = NULL;
> > +    int offset;
> > +    uint32_t lpc_pcba = PNV9_XSCOM_CHIPTOD_BASE;
>
> lpc ?

Shh don't tell anybody I mostly code via copy and paste :P

[snip]

> > +static void pnv_chiptod_realize(DeviceState *dev, Error **errp)
> > +{
> > +    static bool got_primary = false;
> > +    static bool got_secondary = false;
> > +
> > +    PnvChipTOD *chiptod = PNV_CHIPTOD(dev);
> > +    PnvChipTODClass *pctc = PNV_CHIPTOD_GET_CLASS(chiptod);
> > +
> > +    if (!got_primary) {
> > +        got_primary = true;
> > +        chiptod->primary = true;
> > +        chiptod->pss_mss_ctrl_reg |= PPC_BIT(1); /* TOD is master */
> > +    } else if (!got_secondary) {
> > +        got_secondary = true;
> > +        chiptod->secondary = true;
> > +    }
>
> It would be cleaner to introduce "primary" and "secondary" properties
> defined by the model realizing the PnvChipTOD objects.

Hum. There's a few hard-coded primaries on chip_id == 0 already
in the tree. I don't know how closely related they are, chiptod
can set other chips as primary AFAIK but maybe it just makes
sense to make primary a chip property.

I might dig a bit more into what we (and other IBM firmware) want
to test or expect with these primaries. Maybe another pass to
tidy all that up can be done.

[...]

> > +#ifndef PPC_PNV_CHIPTOD_H
> > +#define PPC_PNV_CHIPTOD_H
> > +
> > +#include "qom/object.h"
> > +
> > +#define TYPE_PNV_CHIPTOD "pnv-chiptod"
> > +OBJECT_DECLARE_TYPE(PnvChipTOD, PnvChipTODClass, PNV_CHIPTOD)
> > +#define TYPE_PNV9_CHIPTOD TYPE_PNV_CHIPTOD "-POWER9"
> > +DECLARE_INSTANCE_CHECKER(PnvChipTOD, PNV9_CHIPTOD, TYPE_PNV9_CHIPTOD)
> > +#define TYPE_PNV10_CHIPTOD TYPE_PNV_CHIPTOD "-POWER10"
> > +DECLARE_INSTANCE_CHECKER(PnvChipTOD, PNV10_CHIPTOD, TYPE_PNV10_CHIPTOD)
> > +
> > +enum tod_state {
>
> PnvChipTODState would be better naming.

Agree.

Thanks,
Nick