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

Sonata Testing Infrastructure #26

Merged
merged 7 commits into from
Jul 30, 2024
Merged

Sonata Testing Infrastructure #26

merged 7 commits into from
Jul 30, 2024

Conversation

HU90m
Copy link
Member

@HU90m HU90m commented Jul 20, 2024

This is currently blocked on lowRISC/sonata-system#143 . It expects the possibly new default baud rate.

  • There is a new directory tests which will hold the test suite, like can be found in the CHERIoT RTOS. Currently there are two simple UART tests.
  • There's the new scripts/test_runner.py that can run tests against the FPGA or simulator and monitor the UART output for a success or failure message.
  • The software is now built in CI within a nix sandbox.
  • The nix tests-software check that runs the test suites against the simulator in a nix sandbox.
  • The nix tests-fpga-runner can be used to run the test suites against an FPGA.

This infrastructure can be used in the sonata-system CI as well, by building from the sonata-software's flake in the sonata-system CI to get the tests-runner and software builds, e.g.

nix build github:lowrisc/sonata-software#sonata-tests -o sonata-tests
nix build .#sonata-sim-boot-stub -o sim-boot-stub
nix build .#sonata-simulator -o sonata-simulator

nix run github:lowrisc/sonata-software#tests-runner -- sim \
  --elf-file ./sonata-tests/share/sonata_test_suite
  --sim-boot-stub ./sim-boot-stub/share/sim_boot_stub
  --simulator-binary ./sonata-simulator/bin/sonata-simulator

@HU90m HU90m force-pushed the tests branch 4 times, most recently from 0f15b6d to fe08d91 Compare July 20, 2024 17:05
@HU90m HU90m marked this pull request as ready for review July 25, 2024 12:17
Copy link
Contributor

@marnovandermaas marnovandermaas left a comment

Choose a reason for hiding this comment

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

I tried the first instruction and got an error. Any idea how to fix this?

❯ nix build .#sonata-tests -o sonata-tests
error:
       … while calling the 'derivationStrict' builtin
         at <nix/derivation-internal.nix>:9:12:
            8|
            9|   strict = derivationStrict drvAttrs;
             |            ^
           10|

       … while evaluating derivation 'sonata-tests'
         whose name attribute is located at /nix/store/hkv63lvykwr520xf8yv2gcw3s78fjfpd-source/pkgs/stdenv/generic/make-derivation.nix:333:7

       … while evaluating attribute 'src' of derivation 'sonata-tests'
         at /nix/store/fbxziq9q61c0j2m6gjaqb4f65rn8n98p-source/flake.nix:58:11:
           57|           name = "sonata-tests";
           58|           src = fs.toSource {
             |           ^
           59|             root = ./.;

       (stack trace truncated; use '--show-trace' to show the full, detailed trace)

       error: lib.fileset.unions: Element 2 (/nix/store/fbxziq9q61c0j2m6gjaqb4f65rn8n98p-source/cheriot-rtos) is a path that does not exist.
           To create a file set from a path that may not exist, use `lib.fileset.maybeMissing`.

Copy link
Contributor

@marnovandermaas marnovandermaas left a comment

Choose a reason for hiding this comment

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

Trying the commands from CI, I get a different error:

$ nix build .?submodules=1#sonata-examples
warning: Git tree '/home/mvdmaas/repos/sw-sonata' is dirty
error: builder for '/nix/store/9yrsmsffyj870i1msqwclz818yj2rbba-sonata-examples.drv' failed with exit code 255;
       last 25 log lines:
       > no configure script, doing nothing
       > Running phase: buildPhase
       > checking for platform ... cheriot
       > checking for architecture ... cheriot
       > generating /build/source/cheriot-rtos/sdk/firmware.ldscript.in ... ok
       > generating /build/source/cheriot-rtos/sdk/firmware.ldscript.in ... ok
       > generating /build/source/cheriot-rtos/sdk/firmware.ldscript.in ... ok
       > generating /build/source/cheriot-rtos/sdk/firmware.ldscript.in ... ok
       > [ 32%]: cache compiling.release ../cheriot-rtos/sdk/core/scheduler/main.cc
       > [ 32%]: cache compiling.release ../third_party/display_drivers/core/lcd_base.c
       > [ 33%]: cache compiling.release ../cheriot-rtos/sdk/lib/cxxrt/guard.cc
       > [ 34%]: cache compiling.release all/gpiolib.cc
       > [ 34%]: cache compiling.release all/echo.cc
       > [ 34%]: cache compiling.release ../cheriot-rtos/sdk/core/scheduler/main.cc
       > [ 34%]: cache compiling.release ../third_party/display_drivers/st7735/lcd_st7735.c
       > [ 34%]: cache compiling.release ../libraries/lcd.cc
       > [ 34%]: cache compiling.release ../third_party/display_drivers/core/m3x6_16pt.c
       > [ 34%]: cache compiling.release all/i2c_example.cc
       > [ 34%]: cache compiling.release all/led_walk_raw.cc
       > [ 34%]: cache compiling.release all/lcd_test.cc
       > error: /build/source/cheriot-rtos/sdk/include/cheri.hh:12:10: fatal error: 'magic_enum/magic_enum.hpp' file not found
       > #include <magic_enum/magic_enum.hpp>
       >          ^~~~~~~~~~~~~~~~~~~~~~~~~~~
       > 1 error generated.
       >   > in ../cheriot-rtos/sdk/core/scheduler/main.cc
       For full logs, run 'nix log /nix/store/9yrsmsffyj870i1msqwclz818yj2rbba-sonata-examples.drv'.

@HU90m
Copy link
Member Author

HU90m commented Jul 25, 2024

It looks like you haven't recursively cloned the submodules in your checkout

Copy link
Contributor

@nbdd0121 nbdd0121 left a comment

Choose a reason for hiding this comment

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

It'll be ideal to be able to provide cheriot-rtos without using submodules to avoid having to use ?submodules=1 in all places. But that can be left as a future task.

PASSED_MESSAGE: str = "All tests finished"
FAILED_MESSAGE: str = "Test(s) Failed"
TICK_SECONDS: float = 0.01
SONATA_DRIVE_GLOBS: tuple[str] = ("/run/media/**/SONATA",)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be a list instead of tuple.

Copy link
Contributor

Choose a reason for hiding this comment

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

tuple[str, ...] is also fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

😈

flake.nix Outdated
fileset = fs.unions [./cheriot-rtos ./scripts/elf2uf2.sh];
};
buildPhase = ''
xmake f -P cheriot-rtos/tests/ --board=sonata
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
xmake f -P cheriot-rtos/tests/ --board=sonata
xmake config -P cheriot-rtos/tests/ --board=sonata

flake.nix Outdated

tests-runner = pkgs.writers.writePython3Bin "tests-runner" {
libraries = [pkgs.python3Packages.pyserial];
} (builtins.readFile ./scripts/test_runner.py);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
} (builtins.readFile ./scripts/test_runner.py);
} ./scripts/test_runner.py;

HU90m added 3 commits July 30, 2024 15:58
The `test_runner.py` script watches the UART output of the sonata system
running a test. When it encounters a recognised pass or fail message it
will exit with the appropriate error code.
@HU90m HU90m force-pushed the tests branch 2 times, most recently from 97f9dc5 to a4a99dd Compare July 30, 2024 15:10
HU90m added 2 commits July 30, 2024 16:31
The `tests-simulator` runs the test suites in a sandbox.
The `tests-fpga-runner` runs the tests suites on the FPGA.
@HU90m
Copy link
Member Author

HU90m commented Jul 30, 2024

It'll be ideal to be able to provide cheriot-rtos without using submodules to avoid having to use ?submodules=1 in all places. But that can be left as a future task.

I think that would be nicer. I think the best way to keep it up to date with the commit is probably a lint-submodules which checks the commit's submodules' hash is the same as the one in nix. I held back from adding that to this PR because I felt it was big enough already.

@HU90m HU90m requested a review from nbdd0121 July 30, 2024 15:42
@HU90m
Copy link
Member Author

HU90m commented Jul 30, 2024

@GregAC I'm merging this now to unblock bringing Alex onto test writing. We'll avoid depending on any feature that would make porting to bare-metal tests harder, just in case we want to move testing into the sonata-system repository.

@HU90m HU90m merged commit 69a5df4 into lowRISC:main Jul 30, 2024
1 check passed
@GregAC
Copy link
Contributor

GregAC commented Jul 30, 2024

Thanks for your work on this @HU90m the general flow and the test_runner script all look good but I do have two main concerns:

Use of CHERIoT RTOS First of all there's a significant boot time (as it goes through memory writing invalid capabilities). When you run the UART tests with the simulator they complete quickly but are far slower when you enable tracing (both Ibex instruction tracing and a wave dump). They take 4m30 to run to completion on my laptop with tracing enabled and around half of the simulator cycles are spent in the RTOS boot. I think a good chunk of the other half are down to the slow UART baud (I was running with the 115'200 ratre) so with that improved we'll be looking at say 70/80% of that run time on RTOS boot.

Getting decent turnaround times on simulation when you're debugging hardware issues is very useful. It's not always possible but here it seems we can have a UART test that can complete with full tracing enabled in simulation that runs in say under 30 seconds, were it not for the CHERIoT RTOS boot.

Second we're relying on the drivers and device base addresses from a CHERIoT RTOS repository. If you make a hardware change that requires a driver change or an alteration to these address you'll need a commit to that repository first to make the required changes before you can make a commit to the sonata system repository (which needs the CHERIoT RTOS change to pass CI). This is discussed further below.

The device base addresses being a fixed part of the CHERIoT RTOS repository complicates the multi-top story and supporting a Sonata system builder. Whilst we're not there yet we want people to be able to easily create their own Sonata top-levels which will allow them complete freedom in which peripherals are included and the memory map.

Finally CHERIoT RTOS abstractions and infrastructure just make the test more complex. When you're debugging the hardware you're looking at raw PCs and memory addresses, it makes your life easier if the test is as stripped down as possible from a software point of view (so you don't spend 30 minutes wondering what the test is up to only to realise you're just in the depths of the comparment switcher and it's not really relevant to what you're trying to debug).

Repository for tests Part of this question comes down to what sonata-software is for. To my mind it's for end users of Sonata and these tests aren't relevant to them. It's more examples and labs.

The other key point is when do you split across repositories and when do you keep them in the same repository? In my view it's all about how tightly coupled things are and the dependance relationship between them. Current sonata-software is loosely coupled to the sonata-system repository and it's a one way dependance relationship (sonata-software will work on some verson of sonata-system but it doesn't have to be the top of main). So having a separate repository makes sense.

With these tests in sonata-software they're far more tightly coupled and you've now got a two way dependenance relationship. We have the existing sonata-software -> sonata-system relationship and now so we can run CI and have confidence the latest hardware version passes our tests we have a new sonata-system -> sonata-software relationship, one which potentially demands the tips of main in both repositories is kept in sync with one another. In that setup I don't think two seperate repositories makes sense.

Consider a sonata-system change that makes a hardware modification that requires an alteration to the tests. You now need to juggle PRs in two repositories and merge the sonata-software one first so you can run sonata-system CI. Review feedback that effects the hardware change can also effect the tests and that's awkwardly spread over two seperate reviews. You temporarily end up with a sonata-software tip of main that cannot run its test on any sonata-system commit. If you've got a register change or something that requires a driver alteration we've now got three PRs in flight to juggle (CHERIoT RTOS, sonata-software and sonata-system).

When we've got mock SPI/I2C devices where the simulation model just plays out some transacations supplied from a config file that transaction file is clearly closely coupled to the simulation and also to the test (which will be expecting that specific set of transactions). I'd say it clearly doesn't belong in sonata-software but equally it's awkward if it lives in sonata-system away from the test it's coupled to.

To my mind we're better off with tests in sonata-system and to have a light dependance on parts of CHERIoT RTOS rather than running them as a framework built within CHERIoT RTOS (how our current modest set of software in sonata-system works). This avoids basically all of the issues listed above.

We'd still have the CHERIoT RTOS driver reuse pain point but we can hopefully minimize places this is actually a problem and always do a temporary version of a test that maybe uses a local (to the repository) version of a driver so we can do the sonata-system change as a single PR and then move the updated driver into our CHERIoT RTOS fork.

finish_running("One or more tests failed");
}

[[noreturn]] void __cheri_compartment("test_runner") run_tests()
Copy link
Contributor

Choose a reason for hiding this comment

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

Another smaller concern - I don't think we should run all tests in a single binary like this. May be convenient from a software point of view but adds more pain to the hardware debug.

@HU90m
Copy link
Member Author

HU90m commented Jul 31, 2024

I agree on both major points. test_runner.py was originally developed in sonata-system, but I moved it here to simplify getting this all working. I wasn't sure about whether getting a freestanding bare-metal runtime would be worth the engineering effort, but you've convinced me it is with your start-up-cost-when-tracing numbers.

The next step is to get a freestanding bare-metal runtime set up in the sonata-system repo, then we can move these tests across. The CHERIoT RTOS test suite build would make sense in the lowrisc-nix repo. After these moves, we can run all the tests with no dependency on sonata-software.

We'd still have the CHERIoT RTOS driver reuse pain point but we can hopefully minimize places this is actually a problem and always do a temporary version of a test that maybe uses a local (to the repository) version of a driver so we can do the sonata-system change as a single PR and then move the updated driver into our CHERIoT RTOS fork.

I think the current flow in the sonata-system where one can override the cmake define with a local checkout of the RTOS, should nullify this problem.

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.

4 participants