Initial implementation of ARMv8 support #35
Conversation
thomasdangl
requested review from
rageagainsthepc,
K-Mayer,
cakeless and
ssentanoe
as code owners
thomasdangl
force-pushed
the
main
branch
3 times, most recently
from
60cd4c4
to
40eabe2
Compare
|
Please note that I also turned off include sorting in clang format in this PR. Otherwise it will sort the libvmi includes in |
This has been fixed some time ago: libvmi/libvmi#995 |
|
Thanks for the information. Unfortunately, the libvmi port for ARM is based off this version Maybe it would make sense to rebase it onto the current master branch before we continue merging this? |
|
Alright, I have rebased to current libvmi upstream. |
| ) | ||
| set(libvmi_ENABLE_STATIC OFF) | ||
| set(libvmi_BUILD_EXAMPLES OFF) | ||
| set(ENABLE_STATIC OFF CACHE INTERNAL "") |
There was a problem hiding this comment.
I would prefer the scoped notation instead of internal cache vars.
| { | ||
| return event.x86_regs->r8; | ||
| return (registers_t*)event.x86_regs; |
There was a problem hiding this comment.
I like the getRegisters() approach though. It would probably make sense to create an abstract IRegisters class and implement that. (yes, I know it's a lot of boiler plate, I am not happy about that either).
| @@ -151,6 +151,9 @@ uint VmiHub::run(const std::unordered_map<std::string, std::vector<std::string>> | |||
| } | |||
| case VMI_OS_WINDOWS: | |||
| { | |||
| #if defined(ARM64) | |||
| throw new std::runtime_error("No support for Windows on ARM yet."); | |||
There was a problem hiding this comment.
| throw new std::runtime_error("No support for Windows on ARM yet."); | |
| throw std::runtime_error("No support for Windows on ARM yet."); |
| ) | ||
| set(libvmi_ENABLE_STATIC OFF) | ||
| set(libvmi_BUILD_EXAMPLES OFF) | ||
| set(ENABLE_STATIC OFF CACHE INTERNAL "") |
There was a problem hiding this comment.
Those are configurable options and it would therefore be confusing if they were not displayed by ccmake.
| @@ -164,6 +192,9 @@ set(test_files | |||
| test/vmi/LibvmiInterface_UnitTest.cpp | |||
| test/vmi/SingleStepSupervisor_UnitTest.cpp) | |||
|
|
|||
| configure_file(src/config.h.in ${PROJECT_BINARY_DIR}/config.h) | |||
There was a problem hiding this comment.
We already add some definitions via add_definitions(). It would make sense to consolidate these two methods.
| #elif defined(ARM64) | ||
| addr_t eventPA; | ||
| if (VMI_SUCCESS != | ||
| vmi_pagetable_lookup(vmi, event->arm_regs->ttbr1 & VMI_BIT_MASK(12, 47), event->arm_regs->pc, &eventPA)) |
There was a problem hiding this comment.
I would prefer making the bitmask a named constant so it becomes clear what it is supposed to accomplish
| @@ -9,7 +9,6 @@ | |||
| #include <codecvt> | |||
| #include <fmt/core.h> | |||
| #include <functional> | |||
| #include <libvmi/events.h> | |||
There was a problem hiding this comment.
Why did you remove this?
|
@thomasdangl At the moment it is not possible to run CI builds from forks. You need to push your branch to our upstream repo. |
rageagainsthepc
force-pushed
the
main
branch
9 times, most recently
from
1aa3e07
to
dbd9a46
Compare
|
Is this PR still relevant? 🤔 |
rageagainsthepc
force-pushed
the
main
branch
2 times, most recently
from
0808756
to
29b1340
Compare
This PR provides preliminary support of all current features for ARMv8 on Linux.
What remains to be done: