release/powerpc/powerpc64le.conf
10 ↗	(On Diff #76900)	Regarding the temporal paradox here: This kernel config file is a later part of the main patchset. I can roll it in if necessary, but I'm planning on getting the main body of things in at the same time so the window where someone would notice this should be pretty small.

release/powerpc/powerpc64le.conf
10 ↗	(On Diff #76900)	You could also just leave the release/ change out until later, although I don't think it matters too much.

stand/defs.mk
177–178	So this is weird to me. We normally build $NATIVE_ENDIAN for the loader. why wouldn't we do that here? The force_le thing is a hack for loading a big endian kernel with a little endian loader...
sys/conf/files.powerpc
139	there's little endian AIM machines?
239	So why isn't this just standard?
sys/conf/kern.mk
188	The more times I see this, I wonder if we shouldn't be doing: .if ${MACHINE_ARCH:Mpowerpc64*} to match what we do with 64 bit mips.

andrew added inline comments.

share/mk/bsd.cpu.mk
131	You can use `${MACHINE_ARCH:Mpowerpc64*}` here.
sys/conf/files.powerpc
239	I guess it could be `!powerpcspe`

stand/defs.mk
177–178	yeah, it seems like `WITH_LOADER_FORCE_LE` on a little-endian build should be a no-op
sys/conf/kern.mk
188	Yeah, assuming we're not planning to grow other powerpc64 variants that are significantly different.

release/powerpc/powerpc64le.conf
10 ↗	(On Diff #76900)	I have now split this out in my local until after I commit the kernel config.
share/mk/bsd.cpu.mk
131	Thanks, will switch to doing that.
stand/defs.mk
177–178	In this case, we are indeed loading a little endian kernel with a big endian loader. SLOF is always big endian, so for pseries, this is correct.
177–178	except we actually want big endian loaders on LE, because the only functioning one at the moment is for use with SLOF, which is always big endian. Transition to little endian happens after entry into kernel. The reason I constructed it like this was so that -mlittle-endian and -mbig-endian would never be on the same line. I could simply have it .error if you try to set WITH_LOADER_FORCE_LE on LE.
sys/conf/files.powerpc
139	aim is "all Book-S" as opposed to the Book-E processors.
239	Technically it also shouldn't be built on e5500 either, but that's currently handled as custom CPUTYPE for powerpc64 and isn't really accessible from here.
sys/conf/kern.mk
188	Good idea.

stand/defs.mk
177–178	Submitted D26430 to rip out WITH_LOADER_FORCE_LE entirely. Afterwards, will rework this to just unconditionally -mbig-endian on powerpc64le to ensure the loaders are built big-endian as intended.

stand/defs.mk
177–178	👍

Addressing review comments.

Retested on POWER8.

bdragon marked 7 inline comments as done.Sep 15 2020, 6:21 AM

bdragon added inline comments.

stand/defs.mk
177–178	After removal of WITH_LOADER_FORCE_LE, it made more sense to move this up to the main block above.
sys/conf/files.powerpc
139	At some point we could rename aim to books, but that's busywork for another day.
239	jhibbits corrected me there, e5500 does need altivec.c, so yeah, !powerpcspe would make sense.

Addressing a review comment I missed.

Work branch reverified on powerpc64 (G5), powerpc (G4), powerpcspe (RB800)

bdragon marked 2 inline comments as done.Sep 16 2020, 1:37 AM

LE retested on POWER8.

I didn't check files.powerpc extremely carefully, but overall LGTM

Makefile
503	I suppose powerpcspe is, too. You could just leave this comment out, this whole section is for "rarely used, semi-supported architectures"

Makefile
503	Actually I consider it a bug that powerpcspe hasn't been reenabled here, the clang issues were fixed and it's part of CI builds, so I occasionally have to fix stuff that someone's local tinderbox build didn't catch. It might actually get a few more users if the AmigaOne A1222 manages to make it out of development hell. (Fingers crossed there, I'm on the early adopters list for that) Even though it's pretty much me and jhibbits that use it at the moment, it's a good way to ensure Book-E continues working correctly.