Paths

Table of Contentst

Remove -DMACHINE_ARCH hack from Makefile.riscv
ClosedPublic
Actions

Authored by imp on Feb 24 2020, 7:15 AM.

Details

Reviewers

philip
kp
mhorne
jrtc27
jhb

Group Reviewers

riscv

Commits

rS358393: Better check for floating point type.

Summary

Instead, define the proper CFLAGS for the flavor or riscv we're building, taken
from bsd.cpu.mk.

Test Plan

# cd sys/riscv/conf
# config GENERIC
# config GENERICSF
# cd ../compile/GENERIC
# make CC="riscv64-unknown-freebsd13.0-gcc -E" kern_mib.o -n | grep gcc | sh | grep machine_arch | grep char
 static const char machine_arch[] = "riscv64";
# cd ../compile/GENERICSF
# make CC="riscv64-unknown-freebsd13.0-gcc -E" kern_mib.o -n | grep gcc | sh | grep machine_arch | grep char
 static const char machine_arch[] = "riscv64sf";
#

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 29571
Build 27432: arc lint + arc unit

Event Timeline

imp created this revision.Feb 24 2020, 7:15 AM

Harbormaster completed remote builds in B29565: Diff 68734.Feb 24 2020, 7:15 AM

Hope this cleans up kp's issue with ports and stuff.

sys/conf/kern.mk
158	I was afraid to delete this line...

imp mentioned this in D23812: Add a soft-float riscv kernel config.Feb 24 2020, 7:21 AM

I can't test-boot this change, however... I'm not sure about the lp64->lp64d for mabi.

In D23813#523305, @imp wrote:

I can't test-boot this change, however... I'm not sure about the lp64->lp64d for mabi.

Give me a few hours and I'll test-build & boot (in qemu at least) both riscv64 and riscv64sf. That's the only thing I'm concerned about, everything else (and in the related reviews) looks good.

sys/conf/kern.mk
141	This comment is now outdated.

In D23813#523305, @imp wrote:

I can't test-boot this change, however... I'm not sure about the lp64->lp64d for mabi.

The lp64 -> lp64d change breaks the build (with LLVM at least):

ld: error: mw88W8363fw.o: cannot link object files with different floating-point ABI.

I'm somewhat confused as to what's going on in this build, because it looks like the problem here is that the mw88W8363fw.o build uses <bsd.prog.mk>, which for some reason is deciding to use 'lp64' (i.e. no 'd') for user space builds.

With it reverted it does build and boot, but reports `hw.machine_arch: riscv64sf. I think we're not getting the params.h MACHINE_ARCH to behave as we expect.

In D23813#523346, @kp wrote:

In D23813#523305, @imp wrote:

I can't test-boot this change, however... I'm not sure about the lp64->lp64d for mabi.

The lp64 -> lp64d change breaks the build (with LLVM at least):

ld: error: mw88W8363fw.o: cannot link object files with different floating-point ABI.

I'm somewhat confused as to what's going on in this build, because it looks like the problem here is that the mw88W8363fw.o build uses <bsd.prog.mk>, which for some reason is deciding to use 'lp64' (i.e. no 'd') for user space builds.

With it reverted it does build and boot, but reports `hw.machine_arch: riscv64sf. I think we're not getting the params.h MACHINE_ARCH to behave as we expect.

I tried to describe the situation in D23741. Both kernels should be compiled with the soft-float ABI, -mabi=lp64. This option does not inhibit floating point instructions entirely, it just guarantees that FPRs won't be used as part of the calling convention.

For riscv64sf, we need to disable the use of FPRs altogether, hence remove the "f" and "d" extensions, which Warner has done here. This means it should also be compiled without the FPE kernel option, or compilation will fail at the first use of a floating-point instruction. Better yet, I think FPE should be demoted from a kernel option to a flag based on the target.

Finally, the check in riscv/param.h should be changed to use __riscv_flen rather than __riscv_soft_float_abi. This allows riscv64sf to be distinguished in both the kernel and userland cases, since the hardware-floating point extensions will be disabled for both.

With this done the title of this revision should change to something like "Disable hardware floating-point extensions for the riscv64sf kernel".

sys/conf/kern.mk
156	Should be `-mabi=lp64`.

This revision now requires changes to proceed.Feb 24 2020, 1:57 PM

mhorne added reviewers: jrtc27, jhb.Feb 24 2020, 1:57 PM

OK. This isn't ready, so I'll not land it.

sys/conf/kern.mk
156	OK. I'll rework the patch as you've described in the comments.

Update from the review.

Harbormaster completed remote builds in B29571: Diff 68759.Feb 24 2020, 5:51 PM

I'll acknowledge that longer term we want to move in the direction that @jhb has discussed. This is a short-term fix to make riscv conform to how things are currently done in FreeBSD. John's notions are better than the status quo, but things have to work with the status quo until we're ready to cut over to his notions.

Perhaps the _sf suffix should be split out into a separate _SF_SUFFIX define like we do for _V/_EB_SUFFIX on arm, and _N64/_EL/_HF_SUFFIX on mips? We currently only have the single option in upstream FreeBSD, but that may change in future, and downstream in CheriBSD we have an additional CHERI/non-CHERI difference. I guess we could also have an _XLEN_SUFFIX if we cared enough about a hypothetical riscv32 but that doesn't concern me personally.

In D23813#523539, @imp wrote:

I'll acknowledge that longer term we want to move in the direction that @jhb has discussed. This is a short-term fix to make riscv conform to how things are currently done in FreeBSD. John's notions are better than the status quo, but things have to work with the status quo until we're ready to cut over to his notions.

Thanks Warner for your willingness to fix this. It sounds like a better separation of kernel/userland ABI in the future will have its benefits, especially for RISC-V as the ISA will gain more optional extensions.

This looks good to me as long as @kp's testing goes well.

This revision is now accepted and ready to land.Feb 24 2020, 7:58 PM

With this patch both riscv64 and riscv64sf TARGET_ARCH build result in something that can build ports. They both report the expected hw.machine_arch, and both have the correct MACHINE_ARCH set in bmake.

jhb added inline comments.Feb 25 2020, 12:11 AM

sys/conf/kern.mk
145	I would perhaps drop the last sentence as I think it's redundant with the first. Note that I suspect we will end up reverting this bit once the sysctl is fixed to go back to always using hard-float in -march. (This also creates conflicts with CheriBSD so flipping it back and forth is a bit annoying, but we can deal)

imp added inline comments.Feb 25 2020, 2:29 AM

sys/conf/kern.mk
145	You made the same mistake that made me add the comment. We are always going to build with the soft float API. What we will revert when we go to a dynamic sysctl is the ISA will always be hardware float. The ISA comes from -march the ABI is -mabi.

In D23813#523547, @jrtc27 wrote:

Perhaps the _sf suffix should be split out into a separate _SF_SUFFIX define like we do for _V/_EB_SUFFIX on arm, and _N64/_EL/_HF_SUFFIX on mips? We currently only have the single option in upstream FreeBSD, but that may change in future, and downstream in CheriBSD we have an additional CHERI/non-CHERI difference. I guess we could also have an _XLEN_SUFFIX if we cared enough about a hypothetical riscv32 but that doesn't concern me personally.

Since this is going away when jhb's suggestion is implemented, I'd like to defer this until then.

Closed by commit rS358393: Better check for floating point type. (authored by imp). · Explain WhyFeb 27 2020, 3:34 PM

This revision was automatically updated to reflect the committed changes.

imp added a commit: rS358393: Better check for floating point type..