T8132 smp

[yuyuyureka]

According to the discussion here, the RVBAR at cpu_impl_reg+0x00 is already set to the m1n1 entrypoint and locked by iBoot for all cores on M4.

Indeed, I can see the following values:

m1n1 base: 0x100034f4000
E-Cores cpu_impl_reg at 0x210X50000: 0x100100034f4001
P-Cores cpu_impl_reg at 0x211X50000: 0x1100100034f4001

And voila, when skipping the writes when the value is already correct (which previously produced SErrors when writing to the P-Core cpu_impl_reg), smp_start_secondaries() now works as expected.

[yuyuyureka]

@abrender would you mind testing it again?

[xarkes]

Hello, I tried your PR on my M4 MacbookAir but I'm hitting the exception you introduced when trying to use the chainload.
After booting m1n1, here are my CPUs state:

  cpu0: [waiting] [0x210050000] = 0x10004c48000 (val: 10010004c44001)
  cpu1: [waiting] [0x210150000] = 0x10004c48000 (val: 10010004c44001)
  cpu2: [waiting] [0x210250000] = 0x10004c48000 (val: 10010004c44001)
  cpu3: [waiting] [0x210350000] = 0x10004c48000 (val: 10010004c44001)
  cpu4: [waiting] [0x210450000] = 0x10004c48000 (val: 10010004c44001)
  cpu5: [waiting] [0x210550000] = 0x10004c48000 (val: 10010004c44001)
  cpu6: [running] [0x211050000] = 0x10004c48000 (val: 110010004c44001)
  cpu7: [waiting] [0x211150000] = 0x10004c48000 (val: 110010004c44001)
  cpu8: [waiting] [0x211250000] = 0x10004c48000 (val: 110010004c44001)
  cpu9: [waiting] [0x211350000] = 0x10004c48000 (val: 110010004c44001)

According to your initial comment on the PR, it seems you have a similar state (wrong value + locked), making me think you should also raise this exception when trying to chainload.
I'm going to experiment and document myself more about it, meanwhile I'm happy to discuss on IRC.

Note: Not changing the secondary CPUs RVBARs allows me to complete the "chainload" (e.g. loading a patched m1n1), thus I'm wondering if this script should do it at all?
[EDIT]: I just understood the code, yes it does :D

[yuyuyureka]

I tested the following on my M4 Mac Mini:

• Installed m1n1 using kmutil
• The uartproxy appears
• I can chainload m1n1 using proxyclient/tools/chainload.py

IMO this PR is ready to be merged, as it solves the problem of causing the SError in smp init (for example when chainloading).

[xarkes]

Tested on my M4 Macbook Air, chainloading works properly and I don't hit the Python Exception anymore.

[abrender]

@abrender would you mind testing it again?

Sorry for the delayed response - I no longer have access to the M4 to test :(

[jannau]

functionally ok now, sorry for the nitpicking

[jannau]

please use ~0xfffUL.
update_rvbar sounds to me clearer than write_rvbar

[yuyuyureka]

~0xfffUL would not work. According to the notes in the PR description, the value I read from the register was 0x100100034f4001 for E-cores and 0x1100100034f4001 for P-Cores, while the m1n1 base is 0x100034f4000.

So, we want to skip if the last 48 bits (except the last 12 bits) are already set to the expected value, since the first 16 bits may contain some other information and are not used for the actual jump address.

[jannau]

ok, please use defines then

#define RVBAR_LOCK BIT(0)
#define RVBAR_ADDR GENMASK(47, 12)

I don't think we need rvbar_locked then since FIELD_GET(RVBAR_LOCK, val) is as descriptive.

[jannau]

I think

if val & ~0xfff != rvbar:
    if val & 1:
        raise Exception(f"RVBAR for {cpu.name} is locked but differs from entry point")

    p.write64(addr, rvbar)

is easier to follow than the ifs with two booleans. print(f" {cpu.name}: [0x{addr:x}] = 0x{rvbar:x}") should stay directly above the write.

Not sure if we shouldn't just drop this. I think this is a leftover from old times before we updated in-place

[jannau]

same here

[jannau]

Please prefix it with Failed! \n to make it clear that starting the secondary CPU failed.
"and differs from entry point" sound imo nicer. Printing the values could make clear which one is which