Merge qemu-riscv port functionality into qemu-arm port #15743

dpgeorge · 2024-08-29T04:16:58Z

Summary

Currently both the qemu-arm and qemu-riscv share a lot of code and functionality. In particular the recent improvement to running tests on qemu-arm in #15624 would need to be duplicated in the qemu-riscv port.

This PR merges the qemu-riscv port into the qemu-arm port. The only real differences between the two are the toolchains used to build the code, and the initialisation/startup framework. Everything else is pretty much the same, so this brings the following benefits:

less code duplication
less burden on maintenance
generalised qemu port, could in the future support other architectures

A new board VIRT_RV32 has been added to the qemu-arm port which is the existing RISC-V from the qemu-riscv port. To build it is simply:

$ make BOARD=VIRT_RV32 repl

To cleanly separate the code for the different architectures, startup code has been moved to ports/qemu-arm/mcu/<arch>/.

Testing

The CI has been updated to run the VIRT_RV32 board against the test suite.

Trade-offs and Alternatives

We could keep the ports separate, but there's a lot of code duplication, and an extra burden on maintenance.

Eventually we would rename the qemu-arm port to qemu, but I didn't do that in this PR.

dpgeorge · 2024-08-29T04:23:31Z

@agatti I would appreciate your comments/review of this, thanks!

dpgeorge · 2024-08-29T04:24:36Z

ports/qemu-arm/mpconfigport.h

 #define MICROPY_PY_SYS_PLATFORM     "qemu-arm"
+#elif defined(__riscv)
+#define MICROPY_PY_SYS_PLATFORM     "qemu-riscv32"
+#endif


Probably this would be replaced with simply "qemu" when we rename the port.

github-actions · 2024-08-29T04:33:02Z

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

codecov · 2024-08-29T04:51:48Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.43%. Comparing base (6be1dbc) to head (0426934).
Report is 4 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #15743   +/-   ##
=======================================
  Coverage   98.43%   98.43%           
=======================================
  Files         163      163           
  Lines       21294    21294           
=======================================
  Hits        20960    20960           
  Misses        334      334

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

agatti

Besides a couple of minor Makefile things, that looks good to me!

agatti · 2024-08-29T05:42:52Z

ports/qemu-arm/Makefile

+# architecture name.  "Make" unfortunately does not provide any simple way
+# to perform numeric comparisons, so to keep things simple we assume that
+# GCC is at least version 10 for the time being.
+ifeq ($(GCC_VERSION),10)


Maybe you may want to replace this bit with ifeq ($(shell test $(GCC_VERSION) -ge 10; echo $$?),0) and remove the second sentence in the original comment. I found a solution about that issue long after the code was committed...

Good idea, done. But I changed -ge to -le because I think that's what you meant.

agatti · 2024-08-29T05:50:52Z

ports/qemu-arm/Makefile

+# `make debug` will block QEMU until a debugger is connected to port 1234.
+.PHONY: debug
+debug: $(BUILD)/firmware.elf
+	$(QEMU_SYSTEM) $(QEMU_ARGS) -serial mon:stdio -S -s -kernel $<


How about moving the -s parameter to its own $(QEMU_DBG) variable so it can be overridden with other things like https://www.qemu.org/docs/master/system/gdb.html#using-unix-sockets. If you do that, $(QEMU_ARGS) should also pick up externally defined arguments and add to them rather than replacing its contents.

I added a $(QEMU_DEBUG_ARGS) option, defaulting to -s.

agatti · 2024-08-29T05:53:16Z

ports/qemu-arm/Makefile

+.PHONY: debug
+debug: $(BUILD)/firmware.elf
+	$(QEMU_SYSTEM) $(QEMU_ARGS) -serial mon:stdio -S -s -kernel $<
+
 .PHONY: test
 test: $(BUILD)/firmware.elf


I'd add a testdebug target too. How would you be able to see things like #15589 in a debugger otherwise?

That issue was reproduced by running several tests in a row - with that debug target definition there would be no way to get that under gdb or lldb or anything else.

It's not possible to used both -S and the run-tests.py infrastructure, because the latter consumes stdio of qemu-system-X and so you won't be able to start the emulator because the monitor is not accessible.

That issue was reproduced by running several tests in a row -

I think we could make this debug target use -serial pty and then you can run the tests manually as many times as you like against the exposed serial device, eg:

$ make debug <in a separate terminal> $ gdb ... <in a separate terminal> $ ./run-tests.py --target qemu-arm --device /dev/pts/123

I just tried the version I committed a few days ago as agatti@ce8a877, using -serial pty, and it seems to work when running make testdebug in the first terminal and when it's ready spawning another one and run gdb attaching to the qemu instance. Unless something else changed there's no need to use three terminals :)

OK, I will test.

But what I'm also saying above is that it's possible to rerun the test suite over and over, or whatever combination of tests you like. That requires 3 terminals.

But what I'm also saying above is that it's possible to rerun the test suite over and over, or whatever combination of tests you like. That requires 3 terminals.

Oh, never figured that was a possibility. Almost every time I've had the need for this it turned out to be a fatal crash so there was no point in rerunning the test without restarting qemu as well. In that case then yes, you're absolutely right.

You are right, you can use -S -s with the make test target.

Instead of duplicating all targets with a debug version, I added an orthogonal option QEMU_DEBUG which can be enabled for any qemu target, eg:

$ make QEMU_DEBUG=1 test

That will run the test suite with -S -s.

I also added QEMU_DEBUG_EXTRA, and updated the README with descriptions of these options.

agatti · 2024-08-29T06:10:31Z

ports/qemu-arm/Makefile


 .PHONY: test
 test: $(BUILD)/firmware.elf
 	$(eval DIRNAME=ports/$(notdir $(CURDIR)))
-	cd $(TOP)/tests && ./run-tests.py --target qemu-arm --device execpty:"qemu-system-arm $(QEMU_ARGS) -serial pty -kernel ../$(DIRNAME)/$<" $(TESTS_EXCLUDE)
+	cd $(TOP)/tests && ./run-tests.py --target qemu-arm --device execpty:"$(QEMU_SYSTEM) $(QEMU_ARGS) -serial pty -kernel ../$(DIRNAME)/$<" $(TESTS_EXCLUDE)


I'd add another argument to this for additional user arguments for run-tests.py, or rename TESTS_EXCLUDE into something else and have it kick in only if it's not defined.

OK, how about adding $(RUN_TESTS_EXTRA)? If it's not defined it won't do anything. The user can set it from the command line, eg to -d basics.

That would work, too. Would it help if these extra arguments (along with the other targets) are documented or are niche enough that if you need them you'd have probably figured them out already?

I can document the extra args, and the possible options for BOARD=.

I added RUN_TEST_EXTRA, and documented it in the README, along with a list of available boards.

dpgeorge · 2024-09-03T00:52:01Z

@agatti this has been updated based on your feedback, if you'd like to take another look, thanks!

projectgus

These changes LGTM! Have one question about CI, but that's all.

I support the idea of renaming the port to qemu as a follow-up.

tools/ci.sh

agatti · 2024-09-03T02:19:53Z

@dpgeorge the new changes look good to me as well!

Just one minor thing: given that picolibc for Arm is also an option on the CI machine, would it make sense to extract the libc selection bit that's currently in place for RV32 and make it available for all ports?

I haven't tried to build qemu-arm with picolibc but it'd be an interesting experiment to try (@projectgus expressed some interest back then if memory serves). All the known libm incompatibilities should be already sorted out, or it wouldn't build the RV32 target at all, would it.

dpgeorge · 2024-09-03T02:53:34Z

Just one minor thing: given that picolibc for Arm is also an option on the CI machine, would it make sense to extract the libc selection bit that's currently in place for RV32 and make it available for all ports?

I have now applied this suggestion and moved the picolibc bit of the Makefile so that it's accessible by both ARM and RISC-V targets. As part of this I needed to make ARM use CC for linking instead of LD, to match RISC-V. That anyway makes things more consistent.

dpgeorge · 2024-09-03T02:59:36Z

And of course the CI fails now with picolibc...

agatti · 2024-09-03T11:55:46Z

Interestingly, Arm targets with picolibc are ever so slightly larger than their newlib counterparts:

Picolibc

  text    data     bss     dec     hex filename
181640      16  103124  284780   4586c build-MPS2_AN385/firmware.elf
264044      16  103124  367184   59a50 build-SABRELITE/firmware.elf

Newlib

  text    data     bss     dec     hex filename
181632      16  103124  284772   45864 build-MPS2_AN385/firmware.elf
264036      16  103124  367176   59a48 build-SABRELITE/firmware.elf

Not sure if that's due to some missing flags/ifdefs/whatever, but that's unexpected to say the least.

agatti · 2024-09-03T16:54:16Z

@dpgeorge I was also wondering, given that there's no RV32 platform in the code size report then maybe adding the size of the qemu repl build there could be nice to have. The qemu build isn't influenced by too much third party code so it would reflect core code increase more accurately I think.

If that's not in the scope for this PR, then if I can add that functionality myself I'll send one after this is merged (and the port gets renamed from qemu-arm to qemu).

projectgus · 2024-09-04T00:09:50Z

Interestingly, Arm targets with picolibc are ever so slightly larger than their newlib counterparts:

Huh, that's indeed interesting! I suppose MicroPython is pretty light-on with the libc features it uses, especially in these ports, so there may not be a lot of scope for size optimisation.

dpgeorge · 2024-09-04T06:31:31Z

I was also wondering, given that there's no RV32 platform in the code size report then maybe adding the size of the qemu repl build there could be nice to have. The qemu build isn't influenced by too much third party code so it would reflect core code increase more accurately I think.

That's a good idea. But we need to be aware of the time that the code-size CI job takes to run, it's currently one of the slowest jobs, and adding more build to it will make it even slower.

Feel free to open a PR for it.

Signed-off-by: Damien George <damien@micropython.org>

Currently both the qemu-arm and qemu-riscv ports share a lot of code and functionality. This commit merges the qemu-riscv port into the qemu-arm port. The only real differences between the two are the toolchains used to build the code, and the initialisation/startup framework. Everything else is pretty much the same, so this brings the following benefits: - less code duplication - less burden on maintenance - generalised qemu port, could in the future support other architectures A new board `VIRT_RV32` has been added to the qemu-arm port which is the existing RISC-V board from the qemu-riscv port. To build it: $ make BOARD=VIRT_RV32 repl To cleanly separate the code for the different architectures, startup code has been moved to ports/qemu-arm/mcu/<arch>/. Signed-off-by: Damien George <damien@micropython.org>

dpgeorge · 2024-09-04T07:08:47Z

See #15784 for the rename of the qemu-arm port to qemu.

agatti · 2024-09-04T11:37:17Z

@dpgeorge you may want to increase the heap size for RV32, I just bumped into #12853 (comment) again.

dpgeorge · 2024-09-05T00:48:05Z

you may want to increase the heap size for RV32, I just bumped into #12853 (comment) again.

The tests are no longer embedded in the firmware, but rather run individually over the serial port. Can you please explain what the issue is that you see? Something I can reproduce?

agatti · 2024-09-05T06:34:41Z

Sure, take a look at the test results of #15714: https://github.com/micropython/micropython/actions/runs/10700895755/job/29665631825?pr=15714 - I rebased the code to be compatible with the merged qemu port.

740 tests performed (22638 individual testcases)
739 tests passed
134 tests skipped: ...
1 tests failed: deflate_decompress
make: Leaving directory '/home/runner/work/micropython/micropython/ports/qemu-arm'

dpgeorge · 2024-09-05T07:17:43Z

Hmm, that's strange the failure comes up there, with seemingly unrelated changes. All that PR does is enable inline-riscv-asm, and that shouldn't change the amount of GC heap available, or how the GC heap is used by that failing test.

I guess you can just increase the qemu-arm GC heap size by a little bit in your PR, to get that test to pass. If that doesn't do it, it needs investigating.

agatti · 2024-09-05T08:18:55Z

I can also reproduce the issue locally on an Ubuntu 22.04 VM when running tests through source tools/ci.sh && ci_qemu_build_rv32. Increasing HEAP_SIZE from 100k to 120k makes the test pass (it has to be between 117k and 120k, with 116k it would still fail).

If a GC sweep is performed after each test, I wonder what's lingering in the GC heap when that test is scheduled to run. I'm not sure decompressing the zlib data from that failing test should take that much memory to begin with.

If needed I can easily attach a debugger, reproduce the issue, and report back on what's happening - but I'd need a couple of pointers on what to look for and where to look.

dpgeorge force-pushed the qemu-arm-support-riscv branch from 920e4c0 to 74b6a80 Compare August 29, 2024 04:22

dpgeorge requested a review from projectgus August 29, 2024 04:22

dpgeorge commented Aug 29, 2024

View reviewed changes

dpgeorge force-pushed the qemu-arm-support-riscv branch from 74b6a80 to a8d7fa0 Compare August 29, 2024 04:49

dpgeorge force-pushed the qemu-arm-support-riscv branch 2 times, most recently from 71908ed to b39dbcb Compare August 29, 2024 05:12

dpgeorge added the ports Relates to multiple ports, or a new/proposed port label Aug 29, 2024

agatti reviewed Aug 29, 2024

View reviewed changes

dpgeorge force-pushed the qemu-arm-support-riscv branch from 08c330d to a513d83 Compare September 3, 2024 00:49

projectgus approved these changes Sep 3, 2024

View reviewed changes

tools/ci.sh Show resolved Hide resolved

dpgeorge force-pushed the qemu-arm-support-riscv branch from a513d83 to 6128328 Compare September 3, 2024 02:07

agatti approved these changes Sep 3, 2024

View reviewed changes

dpgeorge force-pushed the qemu-arm-support-riscv branch from 6128328 to f120acb Compare September 3, 2024 02:52

dpgeorge force-pushed the qemu-arm-support-riscv branch 2 times, most recently from 6632c07 to 8ccd74a Compare September 3, 2024 03:33

dpgeorge force-pushed the qemu-arm-support-riscv branch from 8ccd74a to 0484368 Compare September 4, 2024 00:49

dpgeorge added 2 commits September 4, 2024 16:46

tools/mpy-tool.py: Support freezing rv32imc native code.

9396572

Signed-off-by: Damien George <damien@micropython.org>

qemu-arm: Factor board config to mk fragments.

dc9ecd5

Signed-off-by: Damien George <damien@micropython.org>

dpgeorge added 2 commits September 4, 2024 16:46

qemu-arm/Makefile: Clean up SRC and OBJ variables.

f769b43

Signed-off-by: Damien George <damien@micropython.org>

dpgeorge force-pushed the qemu-arm-support-riscv branch from 0484368 to 0426934 Compare September 4, 2024 06:47

dpgeorge merged commit 0426934 into micropython:master Sep 4, 2024
64 of 66 checks passed

dpgeorge deleted the qemu-arm-support-riscv branch September 4, 2024 07:07

dpgeorge mentioned this pull request Sep 4, 2024

qemu: Rename qemu-arm port to qemu. #15784

Merged

dpgeorge mentioned this pull request Sep 4, 2024

Remove tinytest framework, and --write-exp and --list-tests options from run-tests.py #15788

Merged

agatti mentioned this pull request Sep 12, 2024

tools/metrics.py: Add VIRT_RV32 to the code size metrics. #15843

Merged

Merge qemu-riscv port functionality into qemu-arm port #15743

Merge qemu-riscv port functionality into qemu-arm port #15743

Conversation

dpgeorge commented Aug 29, 2024 • edited Loading

Summary

Testing

Trade-offs and Alternatives

dpgeorge commented Aug 29, 2024

Choose a reason for hiding this comment

github-actions bot commented Aug 29, 2024

codecov bot commented Aug 29, 2024 • edited Loading

Codecov Report

agatti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agatti Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpgeorge commented Sep 3, 2024

projectgus left a comment

Choose a reason for hiding this comment

agatti commented Sep 3, 2024

dpgeorge commented Sep 3, 2024

dpgeorge commented Sep 3, 2024

agatti commented Sep 3, 2024

agatti commented Sep 3, 2024

projectgus commented Sep 4, 2024

dpgeorge commented Sep 4, 2024

dpgeorge commented Sep 4, 2024

agatti commented Sep 4, 2024

dpgeorge commented Sep 5, 2024

agatti commented Sep 5, 2024

dpgeorge commented Sep 5, 2024

agatti commented Sep 5, 2024

dpgeorge commented Aug 29, 2024 •

edited

Loading

codecov bot commented Aug 29, 2024 •

edited

Loading

agatti Aug 29, 2024 •

edited

Loading