Page MenuHomeFreeBSD

Fix loader on powerpc32
ClosedPublic

Authored by bdragon on Aug 17 2019, 12:08 AM.

Details

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.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

bdragon created this revision.Aug 17 2019, 12:08 AM
imp accepted this revision.Aug 17 2019, 2:00 AM

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.

This revision is now accepted and ready to land.Aug 17 2019, 2:00 AM
jhibbits accepted this revision.Aug 17 2019, 2:13 AM

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

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 added a comment.Aug 19 2019, 1:45 PM

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.

In D21297#463774, @imp 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 added a comment.Aug 19 2019, 2:11 PM
In D21297#463774, @imp 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 accepted this revision.Aug 19 2019, 3:44 PM
In D21297#463780, @imp wrote:
In D21297#463774, @imp 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.