Page MenuHomeFreeBSD

[draft] powerpc/mpc85xx: Ensure the BSP gets a CPU ID of 0
Needs ReviewPublic

Authored by olce on Wed, Oct 7, 1:10 PM.

Details

Reviewers
jhibbits
Group Reviewers
PowerPC
Summary

As this was the only platform not providing this guarantee, this change
establishes the invariant that the BSP is CPU ID 0 regardless of the
platform/machine/architecture.

Add a check (under INVARIANTS) that the ID returned through
platform_smp_get_bsp() is indeed 0. This is done in cpu_mp_start() so
that the machine is sufficiently initialized to be able to print
a panic.

Test Plan

No hardware around. Can someone test for me, or provide a QEMU recipe?

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 77809
Build 74692: arc lint + arc unit

Event Timeline

olce requested review of this revision.Wed, Oct 7, 1:10 PM

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.

olce edited the test plan for this revision. (Show Details)
sys/powerpc/mpc85xx/platform_mpc85xx.c
330

Whoops, that should read cpuref->cr_hwref = idx_in_cpus.

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.