Fix physical address truncation on 32-bit systems with addressing extensions

[ExhoAR22]

While experimenting with Unicorn to emulate an ARMv7 system that heavily utilizes LPAE, I noticed Unicorn would translate the virtual addresses properly. However, the address got truncated to 32-bits somewhere in the pipeline, which rendered my emulator inoperable.

I tracked down the problem to a couple of types in the CPUTLBEntry struct and surrounding functions. I've changed them from target_ulong to hwaddr (hopefully I didn't miss any).

This change seems to work for my setup, I haven't tried other architectures (such as x86 with PAE) but if my understanding of the code is correct, the change should be architecture-angostic.

When running cargo test, all tests seem to pass. However, I have not added new ones for this scenario.

[wtdcode]

Looks good to me. Let's see if this broken any unit test in C.

[wtdcode]

Could you also add a small test to here: https://github.com/unicorn-engine/unicorn/blob/master/tests/unit/test_arm.c ?

[ExhoAR22]

Could you also add a small test to here: https://github.com/unicorn-engine/unicorn/blob/master/tests/unit/test_arm.c ?

Doing that would mean writing code that initializes the MMU and creates a page table.

Since there aren't any existing tests that utilize the MMU for ARM that would mean writing them from scratch, and while I don't mind doing that, I won't have the time to do it until the weekend.

Does this small fix really necessitate a test?

[PhilippTakacs]

As far as I see this can also be tested with UC_TLB_VIRTUAL. With this testing this is a lot simpler.

[wtdcode]

Could you also add a small test to here: https://github.com/unicorn-engine/unicorn/blob/master/tests/unit/test_arm.c ?

Doing that would mean writing code that initializes the MMU and creates a page table.

Since there aren't any existing tests that utilize the MMU for ARM that would mean writing them from scratch, and while I don't mind doing that, I won't have the time to do it until the weekend.

It’s okay and takes your time.

Does this small fix really necessitate a test?

The bug was introduced in #1746 and Unicorn is too complex so that even I could make mistakes and ignore such trivial errors. Therefore, writing a test can ensure your use case is guaranteed to work in our future releases, both beneficial to your project and Unicorn Engine in a long run.

That said, I sincerely appreciate if you could write a test case or if you are really too busy, we can go as it is, but at a risk of breaking the code in the future. Take your time and feel free to make a choice, I’m not pushing you anyway, i.e. it’s optional.

[wtdcode]

Considering you are using rust, it's also okay to add to https://github.com/unicorn-engine/unicorn/blob/master/tests/rust-tests/main.rs if you prefer to add a test. Our CI will run these tests for every commit.

[PhilippTakacs]

Here is a test for this using UC_TLB_VIRTUAL:

diff --git a/tests/unit/test_arm.c b/tests/unit/test_arm.c
index 538a91c0..14fbd9d9 100644
--- a/tests/unit/test_arm.c
+++ b/tests/unit/test_arm.c
@@ -956,6 +956,47 @@ static void test_arm_cp15_c1_c0_2(void)
     OK(uc_close(uc));
 }
 
+static bool test_arm_v7_lpae_hook_tlb(uc_engine *uc, uint64_t addr,
+                                    uc_mem_type type, uc_tlb_entry *result,
+                                    void *user_data)
+{
+    result->paddr = addr + 0x100000000;
+    result->perms = UC_PROT_ALL;
+    return 1;
+}
+
+static void test_arm_v7_lpae_hook_read(uc_engine *uc, uc_mem_type type,
+                                        uint64_t address, int size, uint64_t value,
+					void *user_data)
+{
+    TEST_CHECK(address == 0x100001000);
+}
+
+static void test_arm_v7_lpae(void)
+{
+    uc_engine *uc;
+    uc_hook hook_read, hook_tlb;
+    uint32_t reg;
+    char code[] = "\x00\x10\x90\xe5"; // ldr r1, [r0]
+    OK(uc_open(UC_ARCH_ARM, UC_MODE_ARM, &uc));
+    OK(uc_ctl_set_cpu_model(uc, UC_CPU_ARM_CORTEX_A7));
+
+
+    OK(uc_ctl_tlb_mode(uc, UC_TLB_VIRTUAL));
+    OK(uc_hook_add(uc, &hook_tlb, UC_HOOK_TLB_FILL, test_arm_v7_lpae_hook_tlb, NULL, 1, 0));
+    OK(uc_hook_add(uc, &hook_read, UC_HOOK_MEM_READ, test_arm_v7_lpae_hook_read, NULL, 1, 0));
+
+    reg = 0x1000;
+    OK(uc_reg_write(uc, UC_ARM_REG_R0, &reg));
+    OK(uc_mem_map(uc, 0x100001000, 0x1000, UC_PROT_ALL));
+    OK(uc_mem_write(uc, 0x100001000, code, sizeof(code)));
+    OK(uc_emu_start(uc, 0x1000, 0x1000 + sizeof(code) - 1, 0, 0));
+    OK(uc_reg_read(uc, UC_ARM_REG_R1, &reg));
+    TEST_CHECK(reg == 0xe5901000);
+
+    OK(uc_close(uc));
+}
+
 TEST_LIST = {{"test_arm_nop", test_arm_nop},
              {"test_arm_thumb_sub", test_arm_thumb_sub},
              {"test_armeb_sub", test_armeb_sub},
@@ -985,4 +1026,5 @@ TEST_LIST = {{"test_arm_nop", test_arm_nop},
              {"test_arm_tcg_opcode_cmp", test_arm_tcg_opcode_cmp},
              {"test_arm_thumb_tcg_opcode_cmn", test_arm_thumb_tcg_opcode_cmn},
              {"test_arm_cp15_c1_c0_2", test_arm_cp15_c1_c0_2},
+             {"test_arm_v7_lpae", test_arm_v7_lpae},
              {NULL, NULL}};

edit: actually check a memory access

[ExhoAR22]

Thanks @PhilippTakacs, I added your patch!
Wasn't familiar with this feature, it'll probably be quite useful to me in the future.

[wtdcode]

Thanks for both of you =).