Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New build system #4

Merged
merged 32 commits into from
Jul 22, 2024
Merged

New build system #4

merged 32 commits into from
Jul 22, 2024

Conversation

mkannwischer
Copy link
Contributor

@mkannwischer mkannwischer commented Jul 11, 2024

This proposes a new and much simpler build system with some advantages:

  • No more copying
  • No more symlinks
  • No more code duplication
  • Adding a test does not require touching the root Makefile (except for including the test)
  • Adding a platform does not require touching the root Makefile
  • Significantly less code required for the build system.

I have not migrated all tests and platforms yet, as I wanted to ask for feedback first.
So far I have ported two platforms (m55-an547, m85-an555) and two tests (ntt_kyber, and ntt_dilithium).

Adding a test only requires creating a new folder in tests/ containing a .mk file and including it in the root Makefile, e.g.,

# Test name - needs to match the directory name
TESTS += ntt-dilithium

# All further variables must be prefixed with the capitalized test name

# Platforms this test should run on (matching the directory name in envs/)
NTT_DILITHIUM_PLATFORMS += m55-an547
NTT_DILITHIUM_PLATFORMS += m85-an555

# C sources required for this test
NTT_DILITHIUM_SOURCES += main.c

# Assembly sources required for this test
NTT_DILITHIUM_ASM_DIR = ../../asm/manual/ntt_dilithium
NTT_DILITHIUM_ASMS += $(NTT_DILITHIUM_ASM_DIR)/ntt_dilithium_12_34_56_78_no_trans_vld4_opt_m55.s
NTT_DILITHIUM_ASMS += $(NTT_DILITHIUM_ASM_DIR)/ntt_dilithium_12_34_56_78_no_trans_vld4_opt_m85.s
NTT_DILITHIUM_ASMS += $(NTT_DILITHIUM_ASM_DIR)/ntt_dilithium_12_34_56_78_no_trans_vld4.s
NTT_DILITHIUM_ASMS += $(NTT_DILITHIUM_ASM_DIR)/ntt_dilithium_12_34_56_78_opt_m55.s
NTT_DILITHIUM_ASMS += $(NTT_DILITHIUM_ASM_DIR)/ntt_dilithium_12_34_56_78_opt_m85.s
NTT_DILITHIUM_ASMS += $(NTT_DILITHIUM_ASM_DIR)/ntt_dilithium_12_34_56_78.s
NTT_DILITHIUM_ASMS += $(NTT_DILITHIUM_ASM_DIR)/ntt_dilithium_123_456_78_opt_size_m55.s
NTT_DILITHIUM_ASMS += $(NTT_DILITHIUM_ASM_DIR)/ntt_dilithium_123_456_78_opt_size_m85.s
NTT_DILITHIUM_ASMS += $(NTT_DILITHIUM_ASM_DIR)/ntt_dilithium_123_456_78.s

It consists of:

  • a test name (matching the folder name) added to the global variable TESTS
  • a list of supported platforms (matching the folder name in envs/)
  • a list of c sources
  • a list of assembly sources

Including this in the core

include tests/ntt-dilithium/ntt-dilithium.mk

will add {build,run,clean}-{m55-an547,m85-an555}_ntt-dilithium make targets meaning that autocompletion works fine and we can continue to call something like:

make run-m55-an547_ntt-dilithium

Adding a platform is similarly simple by just creating a new folder in envs/ containing a Makefile.
This one will be called from the root Makefile with the corresponding SOURCES and ASMS variables set.
The Makefile itself then just needs to implement the {clean,build,run} targets. For example this is the Makefile for the m55:

# Makefile for images for AN547

# default values (when not called from the root Makefile - used in standalone artifacts)
TARGET  ?= test.elf
SOURCES ?= $(wildcard test_src/*.c)
ASMS    ?= $(wildcard test_src/*.s)

CC = arm-none-eabi-gcc
LD := $(CC)

SRC_DIR=./src
BUILD_DIR=./build

COMMON_INC=common/inc/
ENV_INC=./inc/

SYSROOT := $(shell $(CC) --print-sysroot)
CFLAGS += \
	-O3 \
	-Wall -Wextra -Wshadow \
	-fno-common \
	-ffunction-sections \
	-fdata-sections \
	--sysroot=$(SYSROOT) \
	-DARMCM55 \
	-I$(COMMON_INC) \
	-I$(ENV_INC) \
	-I$(SRC_DIR) \
	-I$(SRC_DIR)/platform

ARCH_FLAGS += \
	-march=armv8.1-m.main+mve.fp \
	-mthumb \
	-mfloat-abi=softfp  \

CFLAGS += \
	$(ARCH_FLAGS) \
	--specs=nosys.specs

LDSCRIPT = $(SRC_DIR)/platform/mps3.ld

LDFLAGS += \
	-Wl,--gc-sections \
	-L.

LDFLAGS += \
	--specs=nosys.specs \
	-Wl,--wrap=_open \
	-Wl,--wrap=_read \
	-Wl,--wrap=_write \
	-ffreestanding \
	-T$(LDSCRIPT) \
	$(ARCH_FLAGS)

all: $(TARGET)

HAL_SOURCES = $(wildcard $(SRC_DIR)/*.c) $(wildcard $(SRC_DIR)/*/*.c) $(wildcard common/src/*.c)
OBJECTS_HAL = $(patsubst %.c, $(BUILD_DIR)/%.c.o, $(abspath $(HAL_SOURCES)))
OBJECTS_SOURCES=$(patsubst %.c, $(BUILD_DIR)/%.c.o, $(abspath $(SOURCES)))
OBJECTS_C = $(OBJECTS_SOURCES) $(OBJECTS_HAL)
OBJECTS_ASM = $(patsubst %.s, $(BUILD_DIR)/%.s.o, $(abspath $(ASMS)))

OBJECTS = $(OBJECTS_C) $(OBJECTS_ASM)

$(OBJECTS_C): $(BUILD_DIR)/%.o: %
	mkdir -p $(@D)
	$(CC) $(CFLAGS) -c -o $@ $<

$(OBJECTS_ASM): $(BUILD_DIR)/%.o: %
	mkdir -p $(@D)
	$(CC) -x assembler-with-cpp $(CFLAGS) -c -o $@ $<
 
$(TARGET): $(OBJECTS) $(LDSCRIPT)
	$(LD) $(LDFLAGS) -o $@ $(OBJECTS)

.PHONY: build
build: $(TARGET)

run: $(TARGET)
	qemu-system-arm -M mps3-an547  -nographic -semihosting -kernel $(TARGET)

clean: 
	rm -f *.elf
	rm -rf $(BUILD_DIR)

@mkannwischer
Copy link
Contributor Author

@hanno-becker, @dop-amin could you try this out and have a look if you are overall happy with the structure?
Then I'll migrate the other platforms and tests.

@mkannwischer
Copy link
Contributor Author

I added some CI today.
One for building everything for all supported platforms and one for running the qemu based test where possible.
The former works fine, the latter currently doesn't because I can't get qemu to pass through the return value of the main procedure. I'll need to look into that more.

Another limitation I encountered is that we can't run make -j16 run because for each platform we only build a single elf, so there is some overlap between the tests. We likely want to build one elf for each test/platform combination.
This is also the reason why the target is currently marked as .PHONY -- otherwise it would not rebuild when switching tests.

@mkannwischer
Copy link
Contributor Author

Another limitation I encountered is that we can't run make -j16 run because for each platform we only build a single elf, so there is some overlap between the tests. We likely want to build one elf for each test/platform combination. This is also the reason why the target is currently marked as .PHONY -- otherwise it would not rebuild when switching tests.

This part is no resolved by passing the TARGET from the root Makefile containing the test name.
The qemu problem isn't resolved yet.

@mkannwischer
Copy link
Contributor Author

Upon @hanno-becker's request: I added an option for building standalone artifacts.
For a platform/test-combination this will dump all the required files into a standalone/<test-name> directory in which you can simply type make to build the corresponding elf.
You can type make -j$(nproc) standalone to make them all or make standlone-XXX to build a single test/platform combination.

This introduces some limitations for the build, though:

  • The test Makefile now needs to indicate all files that are needed including headers and included assembly files. I added an additional _OTHER variable for that.
  • The platform Makefile can't use any paths that are in parent directories. I resolved this by symlinking the common dir where needed (those will be dereferenced by cp -L when building the artifact.)

@mkannwischer
Copy link
Contributor Author

mkannwischer commented Jul 15, 2024

I added some CI today. One for building everything for all supported platforms and one for running the qemu based test where possible. The former works fine, the latter currently doesn't because I can't get qemu to pass through the return value of the main procedure. I'll need to look into that more.

Apparently there is no way to directly get the exit codes for the Armv7-M/Armv8-M cores as it's not part of the the semihosting API. This is different for the A profile. After checking with Richard on this, the easiest way to get the rc is to attach a debugger and set a breakpoint at the exit function, then you can read out the rc.
I think in the case here this adds too much complexity for too little gain. For now I just added a print of "ALL GOOD" in the very end of the main function that only executes if all tests have passed successfully. That is easy to check then (I added an additional target check).

@hanno-becker
Copy link
Contributor

I think in the case here this adds too much complexity for too little gain. For now I just added a print of "ALL GOOD" in the very end of the main function that only executes if all tests have passed successfully. That is easy to check then (I added an additional target check).

Agree

before we had run-<PLATFORM>--<TEST> - the double-dash was needed
because the platform names contain - and this makes splitting the
platform and the target again from the target name too tedious.
I changed it to run-<PLATFORM>_<TEST> now, but that means we can't have
_ in both platform and test. There are some tests with _, but I think
that's bad practice anyway, so I'll rename them.
@mkannwischer
Copy link
Contributor Author

I'm fairly happy with this now. Waiting for some feedback from @dop-amin now.
I'm inclined to remove the standalone goals again as they are way too ugly and I don't see a big benefit.

@mkannwischer mkannwischer mentioned this pull request Jul 18, 2024
@mkannwischer
Copy link
Contributor Author

Han points out that MacOS users need to add this to their zshrc to get the autocompletion working

zstyle ':completion::complete:make:*:targets' call-command true
autoload -U compinit && compinit

It should look like this:

$ make run
run                          run-m55-an547_ntt-192        run-m55-an547_permute        run-m85-an555_fx-fft         run-m85-an555_ntt-dilithium
run-m55-an547_crt            run-m55-an547_ntt-256        run-m55-an547_poly           run-m85-an555_helloworld     run-m85-an555_ntt-kyber
run-m55-an547_ct             run-m55-an547_ntt-384        run-m55-an547_sqmag          run-m85-an555_intmulntt      run-m85-an555_ntt-n256
run-m55-an547_flt-fft        run-m55-an547_ntt-512        run-m55-an547_toom           run-m85-an555_karatsuba      run-m85-an555_permute
run-m55-an547_fx-fft         run-m55-an547_ntt-768        run-m55-an547_transpose      run-m85-an555_ntt-192        run-m85-an555_poly
run-m55-an547_helloworld     run-m55-an547_ntt-dilithium  run-m85-an555_crt            run-m85-an555_ntt-256        run-m85-an555_sqmag
run-m55-an547_intmulntt      run-m55-an547_ntt-kyber      run-m85-an555_ct             run-m85-an555_ntt-384        run-m85-an555_toom
run-m55-an547_karatsuba      run-m55-an547_ntt-n256       run-m85-an555_flt-fft        run-m85-an555_ntt-512        run-m85-an555_transpose

@mkannwischer
Copy link
Contributor Author

(1) I have ported all the tests - unfortunately, 5 of them (chunk, montgomery, ntt-1024, saber, schoolbook) are failing. I commented them out in the root Makefile so that CI still works. Unfortunately, I do not have the bandwidth to debug these. I'm rather convinced that this is not due to the new build system, but unfortunately, I cannot check because none of these tests has ever been setup to work with qemu. @hanno-becker, do you want to debug those or shall we remove them? These are all pre-dating SLOTHY, so they may not be needed here.

(2) I don't quite know what to do with envs/{core,fvp-corstone300-mps2,fvp-corstone-mps3} The core seems to be a duplicate of m55-an547. Can this savely be deleted, @hanno-becker? For the corstone ones, I have never actually tried to get those to work on my machine. If these are really still needed, then we should probably build some nix setup to download the required files and put them in the right place. @hanno-becker, if you are not actively using them, I'd propose we remove them? They are still present in https://gitlab.com/arm-research/security/pqmx/-/tree/master/envs?ref_type=heads, so old tests can still be built.

(3) I'll check now if the elfs build by the current build system are actually running on my MPS3 (M55+M85). If that all works fine, I'd like to get this merged ASAP, so I can add Cortex-M4 and Cortex-M7 support, so we can rebase https://github.com/slothy-optimizer/pqmx/tree/armv7m. Then I'd add the same build system to https://github.com/slothy-optimizer/pqax. Then, I think I'll need a drink.

@hanno-becker
Copy link
Contributor

@mkannwischer

  • Let's comment out the non-working tests and open an issue. I will try to investigate, and if it bit rots further, we'll remove it.
  • I would like to keep supporting the FVPs, but you're right that nobody is actively using this. I would suggest you remove it and open an issue as well?

@mkannwischer
Copy link
Contributor Author

I re-ran the tests (that are working on qemu) on the M55 and M85. On M55, all tests are passing. On M85, the intmulntt test fails - I think we have never tested that one.
I removed that target in f06b3a0 and opened an issue #6 to follow up.

I also created issues #5 , #7, #8 as discussed.

I updated the README too to reflect the recent changes.

This is ready to be reviewed now and I hope we can merge it soon.
I'll add the Cortex-M4 and Cortex-M7 support from https://github.com/slothy-optimizer/pqmx/tree/armv7m in a separate PR, so we can merge that soon, too.

@mkannwischer mkannwischer marked this pull request as ready for review July 22, 2024 03:23
@hanno-becker
Copy link
Contributor

Amazing, @mkannwischer. This is a huge simplification, thank you very much.

Copy link
Contributor

@hanno-becker hanno-becker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this locally on Apple M1, QEMU only (no MPS3). Worked out of the box.

@hanno-becker hanno-becker merged commit da7bf30 into main Jul 22, 2024
2 checks passed
@mkannwischer mkannwischer deleted the refactorbuild branch July 22, 2024 04:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants