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
186 ↗	(On Diff #76900)	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 ↗	(On Diff #76900)	there's little endian AIM machines?
239 ↗	(On Diff #76900)	So why isn't this just standard?
sys/conf/kern.mk
188 ↗	(On Diff #76900)	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 ↗	(On Diff #76900)	You can use `${MACHINE_ARCH:Mpowerpc64*}` here.
sys/conf/files.powerpc
239 ↗	(On Diff #76900)	I guess it could be `!powerpcspe`

stand/defs.mk
186 ↗	(On Diff #76900)	yeah, it seems like `WITH_LOADER_FORCE_LE` on a little-endian build should be a no-op
sys/conf/kern.mk
188 ↗	(On Diff #76900)	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 ↗	(On Diff #76900)	Thanks, will switch to doing that.
stand/defs.mk
186 ↗	(On Diff #76900)	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.
186 ↗	(On Diff #76900)	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 ↗	(On Diff #76900)	aim is "all Book-S" as opposed to the Book-E processors.
239 ↗	(On Diff #76900)	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 ↗	(On Diff #76900)	Good idea.

stand/defs.mk
186 ↗	(On Diff #76900)	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
186 ↗	(On Diff #76900)	👍

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
186 ↗	(On Diff #76900)	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 ↗	(On Diff #76900)	At some point we could rename aim to books, but that's busywork for another day.
239 ↗	(On Diff #76900)	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 ↗	(On Diff #77087)	I suppose powerpcspe is, too. You could just leave this comment out, this whole section is for "rarely used, semi-supported architectures"

Makefile
503 ↗	(On Diff #77087)	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.