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