Fix loader on powerpc32

[bdragon]

Summary

Due to this code needing to run on actual 32 bit hardware, it is necessary to do the CAS check AFTER we've already done the PVR check.

[imp]

This does what the comment says it does... I'm not qualified too know if that's actually the right thing to do or not.

[jhibbits]

It shouldn't be built for powerpc32, only powerpc64. However, it doesn't hurt anything, and is a good belt-and-suspenders change.

[luporl]

I agree with @jhibbits: cas.c shouldn't be built for powerpc32, only powerpc64, but the change is OK.

If we go this way, then it is better to remove the (incorrect) conditional compilation of cas.c.
Else, powerpc/ofw/Makefile should be fixed.
This is the relevant part of it:

.if ${MACHINE_ARCH} == "powerpc64"
SRCS+=          cas.c
CFLAGS+=        -DCAS
.endif

It seems this is wrong when cross building to powerpc.
Perhaps it could be changed to something like this:

.if (!defined(TARGET_ARCH) && ${MACHINE_ARCH} == "powerpc64") || "${TARGET_ARCH}" == "powerpc64"
SRCS+=          cas.c
CFLAGS+=        -DCAS
.endif

But I'm also fine with just removing the if block.

[imp]

In D21297#463764, @luporl wrote:

I agree with @jhibbits: cas.c shouldn't be built for powerpc32, only powerpc64, but the change is OK.

If we go this way, then it is better to remove the (incorrect) conditional compilation of cas.c.
Else, powerpc/ofw/Makefile should be fixed.
This is the relevant part of it:

.if ${MACHINE_ARCH} == "powerpc64"
SRCS+=          cas.c
CFLAGS+=        -DCAS
.endif

It seems this is wrong when cross building to powerpc.
Perhaps it could be changed to something like this:

.if (!defined(TARGET_ARCH) && ${MACHINE_ARCH} == "powerpc64") || "${TARGET_ARCH}" == "powerpc64"
SRCS+=          cas.c
CFLAGS+=        -DCAS
.endif

But I'm also fine with just removing the if block.

TARGET_ARCH isn't a thing you can test anywhere outside of Makefile.inc1, so this suggestion can't be right. In the cross build case, we always set MACHINE_ARCH correctly.

[luporl]

In D21297#463774, @imp wrote:

In D21297#463764, @luporl wrote:

I agree with @jhibbits: cas.c shouldn't be built for powerpc32, only powerpc64, but the change is OK.

If we go this way, then it is better to remove the (incorrect) conditional compilation of cas.c.
Else, powerpc/ofw/Makefile should be fixed.
This is the relevant part of it:

.if ${MACHINE_ARCH} == "powerpc64"
SRCS+=          cas.c
CFLAGS+=        -DCAS
.endif

It seems this is wrong when cross building to powerpc.
Perhaps it could be changed to something like this:

.if (!defined(TARGET_ARCH) && ${MACHINE_ARCH} == "powerpc64") || "${TARGET_ARCH}" == "powerpc64"
SRCS+=          cas.c
CFLAGS+=        -DCAS
.endif

But I'm also fine with just removing the if block.

TARGET_ARCH isn't a thing you can test anywhere outside of Makefile.inc1, so this suggestion can't be right. In the cross build case, we always set MACHINE_ARCH correctly.

Ok, but then the following use case is not supported, right?

cd /usr/src/stand && make TARGET=powerpc TARGET_ARCH=powerpc

Because the following command outputs "powerpc64":

cd /usr/src/stand && make TARGET=powerpc TARGET_ARCH=powerpc -V MACHINE_ARCH

But when first performing a buildenv, then MACHINE_ARCH is correctly set:

cd /usr/src && make TARGET=powerpc TARGET_ARCH=powerpc buildenv
cd stand && make -V MACHINE_ARCH

If this is the case, the Makefile is already correct and no issues should occur on 32-bit systems.
Then this change would not be necessary, although it doesn't hurt.

[imp]

In D21297#463778, @luporl wrote:

In D21297#463774, @imp wrote:

In D21297#463764, @luporl wrote:

I agree with @jhibbits: cas.c shouldn't be built for powerpc32, only powerpc64, but the change is OK.

If we go this way, then it is better to remove the (incorrect) conditional compilation of cas.c.
Else, powerpc/ofw/Makefile should be fixed.
This is the relevant part of it:

.if ${MACHINE_ARCH} == "powerpc64"
SRCS+=          cas.c
CFLAGS+=        -DCAS
.endif

It seems this is wrong when cross building to powerpc.
Perhaps it could be changed to something like this:

.if (!defined(TARGET_ARCH) && ${MACHINE_ARCH} == "powerpc64") || "${TARGET_ARCH}" == "powerpc64"
SRCS+=          cas.c
CFLAGS+=        -DCAS
.endif

But I'm also fine with just removing the if block.

TARGET_ARCH isn't a thing you can test anywhere outside of Makefile.inc1, so this suggestion can't be right. In the cross build case, we always set MACHINE_ARCH correctly.

Ok, but then the following use case is not supported, right?

cd /usr/src/stand && make TARGET=powerpc TARGET_ARCH=powerpc

Because the following command outputs "powerpc64":

cd src/stand
make buildenv TARGET=....

is what you need to do to cross build.

cd /usr/src/stand && make TARGET=powerpc TARGET_ARCH=powerpc -V MACHINE_ARCH

But when first performing a buildenv, then MACHINE_ARCH is correctly set:

cd /usr/src && make TARGET=powerpc TARGET_ARCH=powerpc buildenv
cd stand && make -V MACHINE_ARCH

If this is the case, the Makefile is already correct and no issues should occur on 32-bit systems.
Then this change would not be necessary, although it doesn't hurt.

It wouldn't change anything since the -m32 needed to do the cross build on powerpc64 wouldn't be included, and the wrong architecture compiler would be used if you tried it on, say, x86. We never test for TARGET or TARGET_ARCH outside of the Makefile.inc1 at the top level (well, there is an exception for gdb and maybe gcc, but the TARGET variable there isn't the same one and pre-dates the system's use of Makefile and will go away soon anyway).

[luporl]

In D21297#463780, @imp wrote:

In D21297#463778, @luporl wrote:

In D21297#463774, @imp wrote:

In D21297#463764, @luporl wrote:

I agree with @jhibbits: cas.c shouldn't be built for powerpc32, only powerpc64, but the change is OK.

If we go this way, then it is better to remove the (incorrect) conditional compilation of cas.c.
Else, powerpc/ofw/Makefile should be fixed.
This is the relevant part of it:

.if ${MACHINE_ARCH} == "powerpc64"
SRCS+=          cas.c
CFLAGS+=        -DCAS
.endif

It seems this is wrong when cross building to powerpc.
Perhaps it could be changed to something like this:

.if (!defined(TARGET_ARCH) && ${MACHINE_ARCH} == "powerpc64") || "${TARGET_ARCH}" == "powerpc64"
SRCS+=          cas.c
CFLAGS+=        -DCAS
.endif

But I'm also fine with just removing the if block.

TARGET_ARCH isn't a thing you can test anywhere outside of Makefile.inc1, so this suggestion can't be right. In the cross build case, we always set MACHINE_ARCH correctly.

Ok, but then the following use case is not supported, right?

cd /usr/src/stand && make TARGET=powerpc TARGET_ARCH=powerpc

Because the following command outputs "powerpc64":

cd src/stand
make buildenv TARGET=....

is what you need to do to cross build.

cd /usr/src/stand && make TARGET=powerpc TARGET_ARCH=powerpc -V MACHINE_ARCH

But when first performing a buildenv, then MACHINE_ARCH is correctly set:

cd /usr/src && make TARGET=powerpc TARGET_ARCH=powerpc buildenv
cd stand && make -V MACHINE_ARCH

If this is the case, the Makefile is already correct and no issues should occur on 32-bit systems.
Then this change would not be necessary, although it doesn't hurt.

It wouldn't change anything since the -m32 needed to do the cross build on powerpc64 wouldn't be included, and the wrong architecture compiler would be used if you tried it on, say, x86. We never test for TARGET or TARGET_ARCH outside of the Makefile.inc1 at the top level (well, there is an exception for gdb and maybe gcc, but the TARGET variable there isn't the same one and pre-dates the system's use of Makefile and will go away soon anyway).

Ok, thanks for clarifying that. The change is fine then.