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
