Re: [Qemu-devel] [qemu] How to reliably obtain physaddr from vaddr

Re: [Qemu-devel] [qemu] How to reliably obtain physaddr from vaddr

[Richard Henderson]

On 03/15/2015 03:00 AM, Emilio G. Cota wrote:
> On a TLB hit this is trivial (just do nothing), but on
> a TLB miss I'm lost on what to do--I cannot even follow
> where helper_ld/st go (grep doesn't help), although I
> suspect it's TCG backend ops and I don't see an obvious
> way of adding a new operation there.

It goes into softmmu_template.h.  Which then tests a victim tlb, and finally
calls tlb_fill.  You'll probably need to do the same.


r~

Re: [Qemu-devel] [qemu] How to reliably obtain physaddr from vaddr

[Emilio G. Cota]

On Sun, Mar 15, 2015 at 16:10:21 -0700, Richard Henderson wrote:
> On 03/15/2015 03:00 AM, Emilio G. Cota wrote:
> > On a TLB hit this is trivial (just do nothing), but on
> > a TLB miss I'm lost on what to do--I cannot even follow
> > where helper_ld/st go (grep doesn't help), although I
> > suspect it's TCG backend ops and I don't see an obvious
> > way of adding a new operation there.
> 
> It goes into softmmu_template.h.  Which then tests a victim tlb, and finally
> calls tlb_fill.  You'll probably need to do the same.

Thanks, figured this out this morning after getting some sleep.

I've defined this vaddr->paddr as a helper and I'm calling it
before every aa32 store. However, this isn't a smooth sailing:

1. futex_init in the kernel causes an oops--it passes vaddr=0
   but the call happens with pagefaults disabled:
     http://lxr.free-electrons.com/source/kernel/futex.c?v=3.18#L590
   in the code below I'm just returning to avoid the oops.

2. The kernel (vexpress-a9 from buildroot) doesn't boot. It dies with:

> [...]
> devtmpfs: mounted
> Freeing unused kernel memory: 256K (805ea000 - 8062a000)
> Cannot continue, found non relative relocs during the bootstrap.
> Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000e00
> 
> CPU: 0 PID: 1 Comm: init Not tainted 3.18.8 #2
> [<800147cc>] (unwind_backtrace) from [<800115d4>] (show_stack+0x10/0x14)
> [<800115d4>] (show_stack) from [<80473e8c>] (dump_stack+0x84/0x94)
> [<80473e8c>] (dump_stack) from [<80471180>] (panic+0xa0/0x1f8)
> [<80471180>] (panic) from [<800240dc>] (complete_and_exit+0x0/0x1c)
> [<800240dc>] (complete_and_exit) from [<87827f70>] (0x87827f70)
> ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000e00

  Note that if I only call the helper before the "str[hb]*" stores,
  the kernel boots fine.

I'm appending the code (applies on top of current HEAD, 7ccfb495),
any help on what's going on greatly appreciated.

Thanks,

		Emilio

diff --git a/cputlb.c b/cputlb.c
index 38f2151..5596bae 100644
--- a/cputlb.c
+++ b/cputlb.c
@@ -329,6 +329,7 @@ void tlb_set_page(CPUState *cpu, target_ulong vaddr,
     } else {
         te->addr_write = -1;
     }
+    te->addr_phys = paddr;
 }
 
 /* NOTE: this function can trigger an exception */
diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
index 0ca6f0b..65f340c 100644
--- a/include/exec/cpu-defs.h
+++ b/include/exec/cpu-defs.h
@@ -74,10 +74,10 @@ typedef uint64_t target_ulong;
 /* use a fully associative victim tlb of 8 entries */
 #define CPU_VTLB_SIZE 8
 
-#if HOST_LONG_BITS == 32 && TARGET_LONG_BITS == 32
-#define CPU_TLB_ENTRY_BITS 4
-#else
+#if TARGET_LONG_BITS == 32
 #define CPU_TLB_ENTRY_BITS 5
+#else
+#define CPU_TLB_ENTRY_BITS 6
 #endif
 
 typedef struct CPUTLBEntry {
@@ -90,13 +90,14 @@ typedef struct CPUTLBEntry {
     target_ulong addr_read;
     target_ulong addr_write;
     target_ulong addr_code;
+    target_ulong addr_phys;
     /* Addend to virtual address to get host address.  IO accesses
        use the corresponding iotlb value.  */
     uintptr_t addend;
     /* padding to get a power of two size */
     uint8_t dummy[(1 << CPU_TLB_ENTRY_BITS) -
-                  (sizeof(target_ulong) * 3 +
-                   ((-sizeof(target_ulong) * 3) & (sizeof(uintptr_t) - 1)) +
+                  (sizeof(target_ulong) * 4 +
+                   ((-sizeof(target_ulong) * 4) & (sizeof(uintptr_t) - 1)) +
                    sizeof(uintptr_t))];
 } CPUTLBEntry;
 
diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
index 1673287..168cde9 100644
--- a/include/exec/cpu_ldst.h
+++ b/include/exec/cpu_ldst.h
@@ -127,6 +127,9 @@ void helper_stl_mmu(CPUArchState *env, target_ulong addr,
 void helper_stq_mmu(CPUArchState *env, target_ulong addr,
                     uint64_t val, int mmu_idx);
 
+hwaddr helper_st_paddr_mmu(CPUArchState *env, target_ulong addr,
+                           int mmu_idx);
+
 uint8_t helper_ldb_cmmu(CPUArchState *env, target_ulong addr, int mmu_idx);
 uint16_t helper_ldw_cmmu(CPUArchState *env, target_ulong addr, int mmu_idx);
 uint32_t helper_ldl_cmmu(CPUArchState *env, target_ulong addr, int mmu_idx);
diff --git a/include/exec/cpu_ldst_template.h b/include/exec/cpu_ldst_template.h
index 95ab750..39cde9d 100644
--- a/include/exec/cpu_ldst_template.h
+++ b/include/exec/cpu_ldst_template.h
@@ -129,6 +129,39 @@ glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr,
     }
 }
 
+#if DATA_SIZE == 1
+
+static inline hwaddr
+glue(cpu_st_paddr, MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
+{
+    int page_index;
+    target_ulong addr;
+    int mmu_idx;
+    hwaddr ret;
+
+    addr = ptr;
+    /*
+     * XXX Understand why this is necessary.
+     * futex_init on linux bootup calls cmpxchg on a NULL pointer. It expects
+     * -EFAULT to be read back, but when we do the below we get a kernel oops.
+     * However, when doing the load from TCG -EFAULT is read just fine--no oops.
+     */
+    if (unlikely(addr == 0))
+	    return 0;
+    page_index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
+    mmu_idx = CPU_MMU_INDEX;
+    if (unlikely(env->tlb_table[mmu_idx][page_index].addr_write !=
+                 (addr & (TARGET_PAGE_MASK | (DATA_SIZE - 1))))) {
+        ret = glue(helper_st_paddr, MMUSUFFIX)(env, addr, mmu_idx);
+    } else {
+        ret = env->tlb_table[mmu_idx][page_index].addr_phys;
+    }
+    ret |= (addr & ~TARGET_PAGE_MASK);
+    return ret;
+}
+
+#endif /* DATA_SIZE == 1 */
+
 #endif /* !SOFTMMU_CODE_ACCESS */
 
 #undef RES_TYPE
diff --git a/softmmu_template.h b/softmmu_template.h
index 0e3dd35..172b718 100644
--- a/softmmu_template.h
+++ b/softmmu_template.h
@@ -461,6 +461,36 @@ void helper_le_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val,
 #endif
 }
 
+#if DATA_SIZE == 1
+
+hwaddr helper_ret_st_paddr(CPUArchState *env, target_ulong addr,
+                           int mmu_idx, uintptr_t retaddr)
+{
+    int index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
+    target_ulong tlb_addr = env->tlb_table[mmu_idx][index].addr_write;
+
+    /* Adjust the given return address.  */
+    retaddr -= GETPC_ADJ;
+
+    /* If the TLB entry is for a different page, reload and try again.  */
+    if ((addr & TARGET_PAGE_MASK)
+        != (tlb_addr & (TARGET_PAGE_MASK | TLB_INVALID_MASK))) {
+        if (!VICTIM_TLB_HIT(addr_write)) {
+            tlb_fill(ENV_GET_CPU(env), addr, MMU_DATA_STORE, mmu_idx, retaddr);
+        }
+    }
+    return env->tlb_table[mmu_idx][index].addr_phys;
+}
+
+hwaddr
+glue(helper_st_paddr, MMUSUFFIX)(CPUArchState *env, target_ulong addr,
+                                 int mmu_idx)
+{
+    return helper_ret_st_paddr(env, addr, mmu_idx, GETRA());
+}
+
+#endif /* DATA_SIZE == 1 */
+
 #if DATA_SIZE > 1
 void helper_be_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val,
                        int mmu_idx, uintptr_t retaddr)
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 3bc20af..d329b42 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5795,6 +5795,11 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t reg, uint32_t val)
 
 #endif
 
+void HELPER(st_pre)(CPUARMState *env, uint32_t vaddr)
+{
+    cpu_st_paddr_data(env, vaddr);
+}
+
 void HELPER(dc_zva)(CPUARMState *env, uint64_t vaddr_in)
 {
     /* Implement DC ZVA, which zeroes a fixed-length block of memory.
diff --git a/target-arm/helper.h b/target-arm/helper.h
index dec3728..335b970 100644
--- a/target-arm/helper.h
+++ b/target-arm/helper.h
@@ -526,6 +526,8 @@ DEF_HELPER_FLAGS_3(crc32, TCG_CALL_NO_RWG_SE, i32, i32, i32, i32)
 DEF_HELPER_FLAGS_3(crc32c, TCG_CALL_NO_RWG_SE, i32, i32, i32, i32)
 DEF_HELPER_2(dc_zva, void, env, i64)
 
+DEF_HELPER_2(st_pre, void, env, i32)
+
 DEF_HELPER_FLAGS_2(neon_pmull_64_lo, TCG_CALL_NO_RWG_SE, i64, i64, i64)
 DEF_HELPER_FLAGS_2(neon_pmull_64_hi, TCG_CALL_NO_RWG_SE, i64, i64, i64)
 
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 36868ed..b37c6a7 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -896,6 +896,7 @@ static inline void gen_aa32_ld##SUFF(TCGv_i32 val, TCGv_i32 addr, int index) \
 #define DO_GEN_ST(SUFF, OPC)                                             \
 static inline void gen_aa32_st##SUFF(TCGv_i32 val, TCGv_i32 addr, int index) \
 {                                                                        \
+    gen_helper_st_pre(cpu_env, addr);                                    \
     tcg_gen_qemu_st_i32(val, addr, index, OPC);                          \
 }
 
diff --git a/tcg/tcg.h b/tcg/tcg.h
index f941965..0771ecf 100644
--- a/tcg/tcg.h
+++ b/tcg/tcg.h
@@ -902,6 +902,9 @@ void helper_be_stl_mmu(CPUArchState *env, target_ulong addr, uint32_t val,
 void helper_be_stq_mmu(CPUArchState *env, target_ulong addr, uint64_t val,
                        int mmu_idx, uintptr_t retaddr);
 
+hwaddr helper_ret_st_paddr(CPUArchState *env, target_ulong addr,
+                           int mmu_idx, uintptr_t retaddr);
+
 /* Temporary aliases until backends are converted.  */
 #ifdef TARGET_WORDS_BIGENDIAN
 # define helper_ret_ldsw_mmu  helper_be_ldsw_mmu

Re: [Qemu-devel] [qemu] How to reliably obtain physaddr from vaddr

[Emilio G. Cota]

On Sun, Mar 15, 2015 at 20:42:31 -0400, Emilio G. Cota wrote:
> On Sun, Mar 15, 2015 at 16:10:21 -0700, Richard Henderson wrote:
> > It goes into softmmu_template.h.  Which then tests a victim tlb, and finally
> > calls tlb_fill.  You'll probably need to do the same.
> 
> I've defined this vaddr->paddr as a helper and I'm calling it
> before every aa32 store. However, this isn't a smooth sailing:
> 
> 1. futex_init in the kernel causes an oops--it passes vaddr=0
>    but the call happens with pagefaults disabled:
>      http://lxr.free-electrons.com/source/kernel/futex.c?v=3.18#L590
>    in the code below I'm just returning to avoid the oops.

Please disregard this point--the oops doesn't happen with the code
I appended (it was triggered by previous iterations of it).

> 2. The kernel (vexpress-a9 from buildroot) doesn't boot.

Removing the call to tlb_fill() on a TLB miss solves the problem.
But of course this also means the helper doesn't work as intended.

I fail to see why calling tlb_fill() from the helper causes
trouble.  What I thought would happen is that the exception
(if any) is started from the helper, gets serviced, and then
both the helper and the subsequent store hit in the TLB.  I was
seeing this as a "TLB prefetch", but I cannot make it work.

What am I missing?

FWIW I'm appending the delta wrt my previous email.

Thanks,

		Emilio

diff --git a/include/exec/cpu_ldst_template.h b/include/exec/cpu_ldst_template.h
index 39cde9d..48c54f9 100644
--- a/include/exec/cpu_ldst_template.h
+++ b/include/exec/cpu_ldst_template.h
@@ -140,14 +140,6 @@ glue(cpu_st_paddr, MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
     hwaddr ret;
 
     addr = ptr;
-    /*
-     * XXX Understand why this is necessary.
-     * futex_init on linux bootup calls cmpxchg on a NULL pointer. It expects
-     * -EFAULT to be read back, but when we do the below we get a kernel oops.
-     * However, when doing the load from TCG -EFAULT is read just fine--no oops.
-     */
-    if (unlikely(addr == 0))
-	    return 0;
     page_index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
     mmu_idx = CPU_MMU_INDEX;
     if (unlikely(env->tlb_table[mmu_idx][page_index].addr_write !=
diff --git a/softmmu_template.h b/softmmu_template.h
index 172b718..1b6655e 100644
--- a/softmmu_template.h
+++ b/softmmu_template.h
@@ -476,7 +476,7 @@ hwaddr helper_ret_st_paddr(CPUArchState *env, target_ulong addr,
     if ((addr & TARGET_PAGE_MASK)
         != (tlb_addr & (TARGET_PAGE_MASK | TLB_INVALID_MASK))) {
         if (!VICTIM_TLB_HIT(addr_write)) {
-            tlb_fill(ENV_GET_CPU(env), addr, MMU_DATA_STORE, mmu_idx, retaddr);
+            return 0;
         }
     }
     return env->tlb_table[mmu_idx][index].addr_phys;

Re: [Qemu-devel] [qemu] How to reliably obtain physaddr from vaddr

[Peter Maydell]

On 16 March 2015 at 20:08, Emilio G. Cota <cota@braap.org> wrote:
> Removing the call to tlb_fill() on a TLB miss solves the problem.
> But of course this also means the helper doesn't work as intended.
>
> I fail to see why calling tlb_fill() from the helper causes
> trouble.  What I thought would happen is that the exception
> (if any) is started from the helper, gets serviced, and then
> both the helper and the subsequent store hit in the TLB.  I was
> seeing this as a "TLB prefetch", but I cannot make it work.

This isn't how tlb_fill handles page faults. What happens is:

1. tlb_fill calls arm_cpu_handle_mmu_fault to do the page table walk
2. if the page table indicates that the vaddr is invalid (ie
   we need to deliver a guest exception) then we return non-zero
   to tlb_fill
3. tlb_fill calls cpu_restore_state, passing it the address
   in generated TCG code where we were when the exception
   happened; magic happens here to fix up the CPU state
   (notably the guest PC) to the exact correct values at the
   guest load/store insn that caused the fault [using the
   (host) retaddr to determine exactly where in the TB we were
   and so which guest insn faulted]
3. tlb_fill calls raise_exception, which calls cpu_loop_exit
4. cpu_loop_exit *longjmps out of anything you were in the
   middle of*, back to the top level loop in cpu-exec.c
5. based on the changes to the CPU state made before calling
   cpu_loop_exit, the main loop determines that there's a
   pending exception, and resumes execution at the exception
   entry point
6. the guest OS may or may not end up fixing up the page tables
   and reattempting execution of whatever failed, but that's
   entirely up to it and might never happen

I suspect your problem is that the host retaddr in step 3
is wrong, which will result in our generating the guest
exception with a wrong value for "guest PC at point of fault".
Linux makes extensive use of "if guest PC for this fault
is in this magic bit of code then fix up the result so it
looks like this kernel load/store accessor function returned
-EFAULT". If you're reporting the wrong guest PC this won't
work and the kernel will end up in the default case path
of it being an unexpected kernel mode data abort and Oopsing.

I suggest you check whether the exception PC reported to
the guest is correct (it's probably reported by the kernel
somewhere in the oops output) compared to the addresses in
the kernel of the load/store/whatever that's faulted.

thanks
-- PMM

Re: [Qemu-devel] [qemu] How to reliably obtain physaddr from vaddr

[Emilio G. Cota]

On Mon, Mar 16, 2015 at 22:23:24 +0000, Peter Maydell wrote:
> On 16 March 2015 at 20:08, Emilio G. Cota <cota@braap.org> wrote:
> > I fail to see why calling tlb_fill() from the helper causes
> > trouble.  What I thought would happen is that the exception
> > (if any) is started from the helper, gets serviced, and then
> > both the helper and the subsequent store hit in the TLB.  I was
> > seeing this as a "TLB prefetch", but I cannot make it work.
> 
> This isn't how tlb_fill handles page faults. What happens is:
> 
> 1. tlb_fill calls arm_cpu_handle_mmu_fault to do the page table walk
(snip)
> 6. the guest OS may or may not end up fixing up the page tables
>    and reattempting execution of whatever failed, but that's
>    entirely up to it and might never happen

Great description--the last point wasn't all that clear to me.

> I suspect your problem is that the host retaddr in step 3
> is wrong, which will result in our generating the guest
> exception with a wrong value for "guest PC at point of fault".
> Linux makes extensive use of "if guest PC for this fault
> is in this magic bit of code then fix up the result so it
> looks like this kernel load/store accessor function returned
> -EFAULT". If you're reporting the wrong guest PC this won't
> work and the kernel will end up in the default case path
> of it being an unexpected kernel mode data abort and Oopsing.

That was indeed the problem, the TB from that retaddr couldn't
be found.

[ BTW why don't we check the return value of cpu_restore_state?
  I see this has been discussed before:
     http://lists.gnu.org/archive/html/qemu-devel/2012-09/msg02589.html
  Certainly an assert there would have helped me debug this. ]

> I suggest you check whether the exception PC reported to
> the guest is correct (it's probably reported by the kernel
> somewhere in the oops output) compared to the addresses in
> the kernel of the load/store/whatever that's faulted.

Yep. The fix is trivial:

+++ b/target-arm/helper.c
@@ -5797,7 +5797,7 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t reg, uint32_t val)
 
 void HELPER(st_pre)(CPUARMState *env, uint32_t vaddr)
 {
-    cpu_st_paddr_data(env, vaddr);
+    helper_ret_st_paddr(env, vaddr, cpu_mmu_index(env), GETRA());
 }

.. and with this I learned that cpu_ld/st are only to be called
from op helpers.

Thanks a lot for your help.

		Emilio