This change assumes that, on MPC85XX, the BSP's PIR coincides with the index of the CPU in /cpus. Some generic PowerPC code (in cpu_mp_start()) coupled with MPC85XX platform code weakly hints at this being the case (else we might not use pcpup at all and just fill __pcpu[], which in itself is not necessarily a problem).

That looks fishy to me, so I'd like confirmation that this is indeed the case or not. Additionally, I'm finding the existing code setting cr_hwref in mpc85xx_smp_next_cpu() a bit suspicious (based on some property, or simply on the index in /cpus). I'm not sure why setting it really matters anyway, as it looks like this platform does not use this field, and the generic code does not either except to print debugging information.

The change looks fine. cr_cpuid != cr_hwref because cr_hwref is the "hardware logical" CPU ID (multithreaded CPUs count by N, not 1). So on e6500 cpu1 to FreeBSD is hwref 2. The "reg" property on e6500 CPUs (T2080, etc) is actually an array of IDs. Since we currently don't support multithreading on Book-E (and I have no plans at this time to add it, but may if I get bored) we only accept the first member in the array. Looking through the code, though, for this platform you're right, that hwref is not used. cr_hwref (and pc_hwref) look to only be used on the AIM platforms (particularly powernv). I had to make a lot of changes to this code when doing the e6500 bringup earlier this year, but it could likely be removed. The only reason this difference between cpuid and hwref is needed is for the openpic interface, but the PIC_AP_INIT() change I added makes that unnecessary, the pc_pic field now takes care of that.

Regarding SPR_PIR, this also is thread-unique, and is also pretty meaningless in hardware, as it's software-writable, but we don't write to it. It looks like we only read the boot CPU's PIR, too.

I think one goal with pcpup was to eliminate the pcpu array at the global level, as CPU count grows, so keeping pcpup instead of pcpu[N] facilitates that.

Trying to understand a bit more of this platform:

In D60438#1386413, @jhibbits wrote:

cr_cpuid != cr_hwref because cr_hwref is the "hardware logical" CPU ID (multithreaded CPUs count by N, not 1). So on e6500 cpu1 to FreeBSD is hwref 2. The "reg" property on e6500 CPUs (T2080, etc) is actually an array of IDs. Since we currently don't support multithreading on Book-E (and I have no plans at this time to add it, but may if I get bored) we only accept the first member in the array.

That makes sense.

Regarding SPR_PIR, this also is thread-unique, and is also pretty meaningless in hardware, as it's software-writable, but we don't write to it. It looks like we only read the boot CPU's PIR, too.

I guess it's initialized to something that can be used for discrimination?

At a higher level, I'm trying to assess whether the PIR can be used to identify the BSP (well, logical thread) correctly, and how.

Before this change, the BSP got its internal ID assigned to its PIR, and during enumeration by the generic code (platform_smp_first_cpu() + platform_smp_next_cpu()) CPUs got assigned incremental internal IDs. Is it possible that this enumeration did not produce an internal ID equal to the PIR, in other words, is it possible that the enumeration did not return a CPU recognizable as the BSP? I see nothing that would go wrong in the current code if it is, hence my question (if not recognizing the BSP in the CPU enumeration, no pc_bsp is set to 1, and basically we then try to start the BSP as a secondary CPU, but only if it has a cpu-release-addr property, which it probably does not have).

Actually, I think there is a problem. The PIR discriminates hardware threads, and IIUC what you said, the CPUs under /cpus are cores. So, clearly the PIR cannot be the index of the matching core in /cpus. E.g., with two hardware threads per core, and two cores, there would be only 2 elements in /cpus, but the PIR could be 2 (or 3), i.e., out of bounds.

The PIR has to be matched against the content of reg, if any. Using the first element of reg only could be enough if we have the guarantee that the boot hardware thread is the first that is enumerated in the corresponding core. Else we have to match it against all reg elements anyway. Quite logically, I'm assuming that either all CPUs under /cpus have reg, or none have it.

So, to recap, I'm planning to change the CPU enumeration so that the first element of reg (if present; else just the index in /cpus) is matched against the PIR, and if it is, to set the cr_cpuid to 0, else to some monotonously increasing value. This entails keeping a mapping to the index in /cpus (as the cr_cpuid does not do it anymore) and modifying functions iterating over /cpus accordingly (mpc85xx_smp_start_cpu_epapr() is the only one I can see right now).

Is that OK for you? Or are you OK with the current code anyway, and prefer to amend it later yourself? Or am I misunderstanding what's going on?

I think one goal with pcpup was to eliminate the pcpu array at the global level, as CPU count grows, so keeping pcpup instead of pcpu[N] facilitates that.

Maybe. It might have been also a provision to allocate a per-CPU area differently for the BSP.