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

[Cédric Le Goater]

Hello Nick,

[ ... ]

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

What I meant is that if you could map one xscom subregion per thread in
an overall container region, each region could have its own 'opaque' data
(the thread object you are interested in) and you wouldn't need to do the
CPU lookup. A bit like for the ICP on POWER8 :


    0003ffff80000000-0003ffff800fffff (prio 0, i/o): icp-0
      0003ffff80008000-0003ffff80008fff (prio 0, i/o): icp-thread
      0003ffff80010000-0003ffff80010fff (prio 0, i/o): icp-thread
      0003ffff80018000-0003ffff80018fff (prio 0, i/o): icp-thread
      0003ffff80020000-0003ffff80020fff (prio 0, i/o): icp-thread
      0003ffff80028000-0003ffff80028fff (prio 0, i/o): icp-thread
      0003ffff80030000-0003ffff80030fff (prio 0, i/o): icp-thread
      0003ffff80048000-0003ffff80048fff (prio 0, i/o): icp-thread
      0003ffff80050000-0003ffff80050fff (prio 0, i/o): icp-thread


But, I missed that part :

+        if (val & PPC_BIT(35)) { /* SCOM addressing */
+            uint32_t addr = val >> 32;
+            uint32_t reg = addr & 0xfff;

and pnv_get_vcpu_by_xscom_base() is a bit of a hack. That's why you need
a backpointer to the chip which is always a bit suspicious for a sub unit.
An address space would be cleaner since writing to this register generates
another transaction it seems.

Do you plan to support other registers than PC_TOD ?

The part handling "SCOM addressing" deserves its own patch IMO. It would
clarifies how the logic works and the modeling.

[ ... ]

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

XCSOM doesn't have a QOM model but it should be done that way. XIVE is
bit special because the TIMA is an overlapping mapping on all chips.

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

It really shouldn't be too much work, mostly setting properties in the
chip realize routine :

    object_property_set_bool(OBJECT(&chip10->chiptod), "primary",
                             chip->chip_id == 0, &error_abort);
    object_property_set_bool(OBJECT(&chip10->chiptod), "secondary",
                             chip->chip_id == 1, &error_abort);
    if (!qdev_realize(DEVICE(&chip10->chiptod), NULL, errp)) {
        return;
    }

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


Thanks,

C.