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