seL4_tools icon indicating copy to clipboard operation
seL4_tools copied to clipboard

Published 20 hours ago • verified publisher icon seL4

Elfloader: NVIDIA Jetson Orin support

Open andybui01 opened this issue 1 year ago • 6 comments

Elfloader portion of the Jetson Orin port effort (https://github.com/seL4/seL4/pull/1135)

This patch set is quite big and might be hard to review, if the reviewers would like me to break this into separate PRs please let me know.

andybui01 avatar Feb 08 '24 03:02 andybui01

Ok, failing on all the ARMv8 32 bit builds, the change in sys_boot.c needs to be simplified since really all we changed was ARMv8 64 bit code.

andybui01 avatar Feb 09 '24 11:02 andybui01

Two failures on the HW run:

  • TX2 clang (all configs): cannot connect to mq?
  • TQMA, passes all tests but output is cut short, CI thinks it failed?

Besides that, there are positive signs that the elfloader changes (and especially the aarch64 changes) don't introduce any regressions.

cc: @kent-mcleod @axel-h @Indanz I'd appreciate your reviews, let me know if the diff can be minimized or cleaned up

andybui01 avatar Feb 26 '24 05:02 andybui01

I'd appreciate your reviews, let me know if the diff can be minimized or cleaned up

I'd find it easier to fit in time and attention for reviewing if the PR was split up and merged incrementally.

  • The EFI commits and many of the bugfix commits that are self-contained fixes can likely be reviewed and merged with limited discussion required through their own PR. Specifically:
    • 0108b774b0511bc81e4ccb86aae69aba36468aea
    • 7b3c2f32491a3491817e2196f4fa40d4f0c93284
    • 1aa2fb03cae603fdb3a83fb791fbcabe2fb866f5
    • c329bef78ba38a83aa0b97663fae96b18ce25601
    • e8a15ec23cf48b473133069a33674c15d2fc4d74
    • dea692439d0e8e3f07edf74ab63a845f11fcfee8
    • 65b49ae06483a221b269b622bb3d9c748b7890d6
    • fa81fff7f15e341737554d55f3731b9b88f31fe3
    • 33f00c8babeb5dd3014115cd8b6e5c5ac39a557b
    • 5ebb1fcef3b1a3813063336188a19dbb63d877c6
    • 80b8902c968523183e62eb6f90a0450a4e8c040a
    • 0e0a99d2a78880cd4afb7c73ee8ee81af1b2a767
  • The Orin platform port commit (9f2a8d027d79777fa843e29dd5ee4e2f4f09821d) is trivial once all the dependent changes have been landed. This can probably be merged in advance of the other changes.
  • The remaining commits will probably require the bulk of discussion in my opinion as they change structure of common code.

So if you are able to split out the above commits into a separate PR and leave the rest in this one I think that will help us proceed.

kent-mcleod avatar Feb 26 '24 10:02 kent-mcleod

I agree with Kent.

I think 4996de7d5e086f894fa49d2f2b6e35a7d9c44aee is the most controversial change and hardest to review for correctness, all other commits seem fairly straightforward.

Indanz avatar Feb 26 '24 11:02 Indanz

No problem, thanks guys.

What I'll do is split this into 3 PRs then:

  1. General stability/EFI changes
  2. The series of commits that changes ARM booting and splits aarch64 out, including the humongous page table commit (https://github.com/seL4/seL4_tools/commit/4996de7d5e086f894fa49d2f2b6e35a7d9c44aee)
  3. Finally, the Jetson Orin port

andybui01 avatar Feb 27 '24 00:02 andybui01

I would prefer the problematic commit to be in its own PR, so we can discuss it there without holding up the rest.

Indanz avatar Feb 27 '24 11:02 Indanz