Re: [RFC PATCH 08/10] ppc/pnv: Invert the design for big-core machine modelling

[Cédric Le Goater]

> > > @@ -157,6 +157,14 @@ static int pnv_dt_core(PnvChip *chip, PnvCore *pc, void 
> > > *fdt)
> > >    
> > >        pnv_cc->processor_id(chip, pc->hwid, 0, &pir, &tir);
> > >    
> > > +    /* Only one DT node per (big) core */
> > > +    if (tir != 0) {
> > > +        g_assert(pc->big_core);
> > > +        g_assert(tir == 1);
> > > +        g_assert(pc->hwid & 1);
> > > +        return -1;
> >
> > return is -1 but it's not an error. right ?
>
> Not an error just a "no CPU node to insert".
>
> It's a bit ugly. Could return bool for yes/no and take a *offset
> maybe?

or we could pass the pa_features array  ?


> > > +        if (machine->smp.threads > 8) {
> > > +            error_report("Cannot support more than 8 threads/core "
> > > +                         "on a powernv9/10  machine");
> > > +            exit(1);
> > > +        }
> > > +        if (machine->smp.threads % 2 == 1) {
> >
> > is_power_of_2()
>
> It does have that check later in pnv_init(), but I wanted
> to be careful that we're dividing by 2 below I think it makes
> it more obvious (and big-core can't have 1 thread per big core).

ok


> > > @@ -1099,6 +1157,8 @@ static void pnv_power9_init(MachineState *machine)
> > >    
> > >    static void pnv_power10_init(MachineState *machine)
> > >    {
> > > +    PnvMachineState *pnv = PNV_MACHINE(machine);
> > > +    pnv->big_core_tbst_quirk = true;
> > >        pnv_power9_init(machine);
> > >    }
> > >    
> > > @@ -1169,9 +1229,15 @@ static void pnv_processor_id_p9(PnvChip *chip,
> > >                                    uint32_t core_id, uint32_t thread_id,
> > >                                    uint32_t *pir, uint32_t *tir)
> > >    {
> > > -    if (chip->nr_threads == 8) {
> > > -        *pir = (chip->chip_id << 8) | ((thread_id & 1) << 2) | (core_id << 3) |
> > > -               (thread_id >> 1);
> > > +    PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
> >
> > arg. We should avoid these qdev_get_machine() calls. Could big_core be a
> > chip property instead ?
>
> We could, but per machine probably makes more sense. It's
> funny there seems to be no good way to get machine from CPU.
> Maybe we can just add a machine pointer in PnvChip?


It would be easier/cleaner to propagate the machine settings to
the chip unit and subunits. If I remember correctly, real HW has a
scan init sequence doing something similar.

> I'l probably leave that for another series and try to convert
> most things.
>
> > > +static bool pnv_machine_get_hb(Object *obj, Error **errp)
> > > +{
> > > +    PnvMachineState *pnv = PNV_MACHINE(obj);
> > > +
> > > +    return !!pnv->fw_load_addr;
> > > +}
> > > +
> > > +static void pnv_machine_set_hb(Object *obj, bool value, Error **errp)
> > > +{
> > > +    PnvMachineState *pnv = PNV_MACHINE(obj);
> > > +
> > > +    if (value) {
> > > +        pnv->fw_load_addr = 0x8000000;
> > > +    }
> > > +}
> >
> > we might want to get rid of the hostboot mode oneday. This was really
> > experimental stuff.
>
> Okay sure, I don't use it. Although we may want to run the
> open source hostboot part of the firmware on QEMU one day,
> we can always add back some options for it.

It's not invasive either. Let's keep it. It use to work with a
trimdown Linux image.


Thanks,

C.


> We do have a PowerVM mode too which tweaks a few things, but
> that's no use for upstream.
>
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > index 059d372c8a..05195527a5 100644
> > > --- a/hw/ppc/spapr_cpu_core.c
> > > +++ b/hw/ppc/spapr_cpu_core.c
> >
> > This change should come in another patch preferably
>
> Yeah this might have got into the wrong patch.
>
> Thanks,
> Nick