On Wed, Mar 12, 2025 at 4:03 AM BALATON Zoltan <balaton@eik.bme.hu> wrote:
On Wed, 12 Mar 2025, Andrew Randrianasulu wrote:
ср, 12 мар. 2025 г., 02:43 BALATON Zoltan <balaton@eik.bme.hu>:
On Wed, 12 Mar 2025, Andrew Randrianasulu wrote:
ср, 12 мар. 2025 г., 01:10 BALATON Zoltan <balaton@eik.bme.hu>:
On Wed, 12 Mar 2025, Andrew Randrianasulu wrote:
---quote from user manual ---
2.1.1.4 Processor ID Register (PIR)
The Processor Identification Register (PIR) is a 32-bit register that
holds a processor identification tag. In the
970MP processing unit, this tag is in the three least-significant bits
(29:31). The least-significant bit of the
processor identification tag (PID) is hardwired to ‘0’ for PU0 and to
‘1’ for PU1. This tag is used to tag bus
transactions and to differentiate processors in multiprocessor
systems. The PIR is a read-only register. The
format of the register is as follows:
0:28—Reserved (read as zeros)
29:31 PID3-bit processor ID value (least-significant bit hardwired to
differentiate PU0 and PU1)
During power-on reset, PID is set to a unique value for each processor
in a multi processor system.
=====
7.2.2.4 Processor ID (PROCID[0:1])–Input
The 2-bit processor ID is used to assign unique IDs to the two 970MP
processing units in a system that can
have up to eight processors. The PROCID signals are sampled during
power-on reset, and the 2-bit value is
placed in the second and third lowest-order bits of the Processor ID
Register (PIR) of each processing unit.
The lowest-order PIR bit is hardwired to a '0' for PU0 and to '1' for
PU1.
Timing: These signals should be permanently tied to VDD or GND, as
appropriate for the required ID value.
=== quote end ===
This suggests that at least on G5 it's strapped in hardware by wiring
the
chip pins so each CPU has a different ID. The G4 could write it from
software but maybe it doesn't do that and sets it by hardware pins as
well. In any case, OpenBIOS does not write it but would set the reg
property based on it so probably it should be set in QEMU when creating
the CPUs. Then the part of the OpenBIOS patch that comments out the call
to set reg property based on pir and the part that sets the reg property
later can be dropped and thus the patch simplified (need less changes to
OpenBIOS).
To set PIR in QEMU look at what other machines do. By searching for PIR
in
hw/ppc I've found:
hw/ppc/spapr_cpu_core.c::spapr_realize_vcpu()
...
env->spr_cb[SPR_PIR].default_value = cs->cpu_index;
hw/ppc/e500.c::ppce500_init()
for (i = 0; i < smp_cpus; i++) {
...
env->spr_cb[SPR_BOOKE_PIR].default_value = cs->cpu_index = i;
The SPR number is different on e500 but the code is more similar to
mac99
so in mac99 I think we should do what e500 does but use SPR_PIR instead
of
SPR_BOOKE_PIR. What I'm not sure about if the cpu_reset in the kick
function would undo this but since it sets the default_value maybe that
means it would reset to this value so it should work. There's a -d
cpu_reset option to dump registers on cpu_reset but I think that shows
values before reset. You can try updating the patches accordingly (add a
line to QEMU as above and undo part of the openbios patch that should no
longer be needed)
I removed commenting out pir property from openbios patch but sadly it
still does not work with g3 machine :(
https://mail.coreboot.org/hyperkitty/list/openbios@openbios.org/thread/7NJJRBTM2TTH4HKE4CFNMQZOSHXA5Z5I/
I don't think that would fix it as it's called from cpu_g4_init which
should not be called for g3 at all which I think uses cpu_750_init.
However just removing the commenting of that line alone is not enough.
What you have now sets reg propery based on pir (probably 0 for all CPUs
if you made no changes to QEMU, it should print the pit it uses) then in
later part of the patch it sets reg again based on CPU number owerwiting
what cpu_add_pir_property did. That part should be also removed and
instead set PIR in QEMU. You can verify it worked as it should print
different PIR for each CPU in cpu_add_pir_property and set reg accordingly
that you can see in the device-tree. I could go down into more detail to
which lines to add and delete but then I'd almost write a patch by hand. I
hoped you understand what the lines in the patch do (it's not that complex
patch after all) and can find those and add one line to QEMU. If you don't
understand the openbios patch try to read it and get what the lines do.
I think we misunderstand each other.
My *main worry* for now that this openbios patch breaks non-mac99 machines
and I do not know why .....
Fixing the above might fix the other machines too. The question is why it
breaks.
It seems that original patch added looping with "state" stopped and
finish-device there.
And this last call to finish-device breaks booting into openbios on g3
machine :/
for now I conditionalized this on machine_id set to mac99 like it was
already done below, but ...
as you said I mostly mash things without good understading WHY they break
May be I should conditionolize whole newly-added loop? Or move it
somewhere .. (we can't get dual G3 machine?)
The openbios patch with -smp 1 only adds a new state property so
is that what should not be there on other machines or without smp? Does it
help if you remove that state=running property?
If it segfaults run it under gdb and get a backtrace to see where then it
should be easier to find out why.
I think this is quite critical bug, and you can't commit this as is because
it WILL bereak machines worked before.
I'll play around a bit more with finish-device placement.
You can't just move that around if you don't understand where it should be
so maybe try to get that first. Or you could but it's not likely to
succeed. I think setting PIR in QEMU first you can then move all of the
patch within cpu_add_pir_property which is only called for G4 so should
leave all other machines unchanged and you need no commenting out in
cpu_g4_init and only need the loop around CPU creation in arch_of_init. I
mean in cpu_add_pir_property you have CPU number in pir (once that's set
in QEMU) so you can move the set state = (pir ? "stopped" : "running")
there then you don't need to comment out anything just add the loop to
call the init func for all cpus but no need to patch any properties there
only in cpu_add_pir_property.
Regards,
BALATON Zoltan