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

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

On Thu May 30, 2024 at 5:46 PM AEST, Cédric Le Goater wrote:
>
> >>> @@ -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  ?

That might work better. I'll try it.

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

Sure. There wll be logic inside the core and chip that controls the
switch so it is not incorrect to model that way.

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

We'll keep it for now.

Thanks,
Nick