On Wed, 12 Mar 2025, Andrew Randrianasulu wrote:
> 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 :/
Now I've looked at the patch... When you have g3 it will call cpu_750_init
which already does finish-device and the second one we hacked in cannot be
done. It works for G4 because we commented out the one in cpu_g4_init.
Instead of conditionalising the finish device in the loop remove it,
revert the comments in cpu_g4_init and move the setting of the reg
property and state property in cpu_add_pir_property. You'll need the cpu
number there which is should get from PIR which has to be set in QEMU when
creating the CPUs. Is this still not clear enough? I don't know how to
explain it more clearly other than writing the patch but I won't do that.
There are enough people here who are more interested in getting this
working than me.
/me laughs.
Well, thanks for helping, this was useful reply and I'll try to implement those suggestions.
I build openbios in debian (amd64) VM so copying text from graphical terminal there is not something command line qemu does out of the box, so I create patch, rsync it to host, post it from there. It adds latency and I guess frustration on my end.
But may be working at 4:42AM is not my best time ;) Anyway, thanks for helping, have good sleep/rest.
Regards,
BALATON Zoltan
> 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
>