[Qemu-devel] [PATCH v2 0/2] tcg: Introduce probe_access() and return a host pointer

[Qemu-devel] [PATCH v2 0/2] tcg: Introduce probe_access() and return a host pointer

[David Hildenbrand]

Let's return a host pointer from probe_access() if possible and provide
probe_access(). s390x will soon make use of probe_access() and use the
provided host pointer.

v1 -> v2:
- Rebased to tcg-next
- "tcg: Make probe_write() return a pointer to the host page"
-- Rephrase documentation
- "tcg: Factor out probe_write() logic into probe_access()"
-- Don't check the MMU index

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Riku Voipio <riku.voipio@iki.fi>

David Hildenbrand (2):
  tcg: Make probe_write() return a pointer to the host page
  tcg: Factor out probe_write() logic into probe_access()

 accel/tcg/cputlb.c      | 60 +++++++++++++++++++++++++++++++----------
 accel/tcg/user-exec.c   | 28 ++++++++++++++-----
 include/exec/exec-all.h |  9 +++++--
 3 files changed, 75 insertions(+), 22 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH v2 1/2] tcg: Make probe_write() return a pointer to the host page

[David Hildenbrand]

... similar to tlb_vaddr_to_host(); however, allow access to the host
page except when TLB_NOTDIRTY or TLB_MMIO is set.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 accel/tcg/cputlb.c      | 21 ++++++++++++++++-----
 accel/tcg/user-exec.c   |  6 ++++--
 include/exec/exec-all.h |  4 ++--
 3 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 707adf7631..cb969d8372 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1078,11 +1078,11 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
 /* Probe for whether the specified guest write access is permitted.
  * If it is not permitted then an exception will be taken in the same
  * way as if this were a real write access (and we will not return).
- * Otherwise the function will return, and there will be a valid
- * entry in the TLB for this access.
+ * If the size is 0 or the page requires I/O access, returns NULL; otherwise,
+ * returns the address of the host page similar to tlb_vaddr_to_host().
  */
-void probe_write(CPUArchState *env, target_ulong addr, int size, int mmu_idx,
-                 uintptr_t retaddr)
+void *probe_write(CPUArchState *env, target_ulong addr, int size, int mmu_idx,
+                  uintptr_t retaddr)
 {
     uintptr_t index = tlb_index(env, mmu_idx, addr);
     CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr);
@@ -1101,12 +1101,23 @@ void probe_write(CPUArchState *env, target_ulong addr, int size, int mmu_idx,
         tlb_addr = tlb_addr_write(entry);
     }
 
+    if (!size) {
+        return NULL;
+    }
+
     /* Handle watchpoints.  */
-    if ((tlb_addr & TLB_WATCHPOINT) && size > 0) {
+    if (tlb_addr & TLB_WATCHPOINT) {
         cpu_check_watchpoint(env_cpu(env), addr, size,
                              env_tlb(env)->d[mmu_idx].iotlb[index].attrs,
                              BP_MEM_WRITE, retaddr);
     }
+
+    if (tlb_addr & (TLB_NOTDIRTY | TLB_MMIO)) {
+        /* I/O access */
+        return NULL;
+    }
+
+    return (void *)((uintptr_t)addr + entry->addend);
 }
 
 void *tlb_vaddr_to_host(CPUArchState *env, abi_ptr addr,
diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index 625c33f893..5720bf8056 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -188,8 +188,8 @@ static inline int handle_cpu_signal(uintptr_t pc, siginfo_t *info,
     g_assert_not_reached();
 }
 
-void probe_write(CPUArchState *env, target_ulong addr, int size, int mmu_idx,
-                 uintptr_t retaddr)
+void *probe_write(CPUArchState *env, target_ulong addr, int size, int mmu_idx,
+                  uintptr_t retaddr)
 {
     g_assert(-(addr | TARGET_PAGE_MASK) >= size);
 
@@ -202,6 +202,8 @@ void probe_write(CPUArchState *env, target_ulong addr, int size, int mmu_idx,
                      retaddr);
         g_assert_not_reached();
     }
+
+    return size ? g2h(addr) : NULL;
 }
 
 #if defined(__i386__)
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index cbcc85add3..a7893ed16b 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -310,8 +310,8 @@ static inline void tlb_flush_by_mmuidx_all_cpus_synced(CPUState *cpu,
 {
 }
 #endif
-void probe_write(CPUArchState *env, target_ulong addr, int size, int mmu_idx,
-                 uintptr_t retaddr);
+void *probe_write(CPUArchState *env, target_ulong addr, int size, int mmu_idx,
+                  uintptr_t retaddr);
 
 #define CODE_GEN_ALIGN           16 /* must be >= of the size of a icache line */
 
-- 
2.21.0

Re: [Qemu-devel] [PATCH v2 1/2] tcg: Make probe_write() return a pointer to the host page

[Richard Henderson]

On 8/30/19 3:09 AM, David Hildenbrand wrote:
> ... similar to tlb_vaddr_to_host(); however, allow access to the host
> page except when TLB_NOTDIRTY or TLB_MMIO is set.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  accel/tcg/cputlb.c      | 21 ++++++++++++++++-----
>  accel/tcg/user-exec.c   |  6 ++++--
>  include/exec/exec-all.h |  4 ++--
>  3 files changed, 22 insertions(+), 9 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH v2 2/2] tcg: Factor out probe_write() logic into probe_access()

[David Hildenbrand]

Let's also allow to probe other access types.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 accel/tcg/cputlb.c      | 43 ++++++++++++++++++++++++++++++-----------
 accel/tcg/user-exec.c   | 26 +++++++++++++++++++------
 include/exec/exec-all.h |  9 +++++++--
 3 files changed, 59 insertions(+), 19 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index cb969d8372..abae79650c 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1075,30 +1075,51 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
     return qemu_ram_addr_from_host_nofail(p);
 }
 
-/* Probe for whether the specified guest write access is permitted.
- * If it is not permitted then an exception will be taken in the same
- * way as if this were a real write access (and we will not return).
+/*
+ * Probe for whether the specified guest access is permitted. If it is not
+ * permitted then an exception will be taken in the same way as if this
+ * were a real access (and we will not return).
  * If the size is 0 or the page requires I/O access, returns NULL; otherwise,
  * returns the address of the host page similar to tlb_vaddr_to_host().
  */
-void *probe_write(CPUArchState *env, target_ulong addr, int size, int mmu_idx,
-                  uintptr_t retaddr)
+void *probe_access(CPUArchState *env, target_ulong addr, int size,
+                   MMUAccessType access_type, int mmu_idx, uintptr_t retaddr)
 {
     uintptr_t index = tlb_index(env, mmu_idx, addr);
     CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr);
-    target_ulong tlb_addr = tlb_addr_write(entry);
+    target_ulong tlb_addr;
+    size_t elt_ofs;
+    int wp_access;
 
     g_assert(-(addr | TARGET_PAGE_MASK) >= size);
 
+    switch (access_type) {
+    case MMU_DATA_LOAD:
+        elt_ofs = offsetof(CPUTLBEntry, addr_read);
+        wp_access = BP_MEM_READ;
+        break;
+    case MMU_DATA_STORE:
+        elt_ofs = offsetof(CPUTLBEntry, addr_write);
+        wp_access = BP_MEM_WRITE;
+        break;
+    case MMU_INST_FETCH:
+        elt_ofs = offsetof(CPUTLBEntry, addr_code);
+        wp_access = BP_MEM_READ;
+        break;
+    default:
+        g_assert_not_reached();
+    }
+    tlb_addr = tlb_read_ofs(entry, elt_ofs);
+
     if (unlikely(!tlb_hit(tlb_addr, addr))) {
-        if (!VICTIM_TLB_HIT(addr_write, addr)) {
-            tlb_fill(env_cpu(env), addr, size, MMU_DATA_STORE,
-                     mmu_idx, retaddr);
+        if (!victim_tlb_hit(env, mmu_idx, index, elt_ofs,
+                            addr & TARGET_PAGE_MASK)) {
+            tlb_fill(env_cpu(env), addr, size, access_type, mmu_idx, retaddr);
             /* TLB resize via tlb_fill may have moved the entry. */
             index = tlb_index(env, mmu_idx, addr);
             entry = tlb_entry(env, mmu_idx, addr);
         }
-        tlb_addr = tlb_addr_write(entry);
+        tlb_addr = tlb_read_ofs(entry, elt_ofs);
     }
 
     if (!size) {
@@ -1109,7 +1130,7 @@ void *probe_write(CPUArchState *env, target_ulong addr, int size, int mmu_idx,
     if (tlb_addr & TLB_WATCHPOINT) {
         cpu_check_watchpoint(env_cpu(env), addr, size,
                              env_tlb(env)->d[mmu_idx].iotlb[index].attrs,
-                             BP_MEM_WRITE, retaddr);
+                             wp_access, retaddr);
     }
 
     if (tlb_addr & (TLB_NOTDIRTY | TLB_MMIO)) {
diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index 5720bf8056..71c4bf6477 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -188,17 +188,31 @@ static inline int handle_cpu_signal(uintptr_t pc, siginfo_t *info,
     g_assert_not_reached();
 }
 
-void *probe_write(CPUArchState *env, target_ulong addr, int size, int mmu_idx,
-                  uintptr_t retaddr)
+void *probe_access(CPUArchState *env, target_ulong addr, int size,
+                   MMUAccessType access_type, int mmu_idx, uintptr_t retaddr)
 {
+    int flags;
+
     g_assert(-(addr | TARGET_PAGE_MASK) >= size);
 
-    if (!guest_addr_valid(addr) ||
-        page_check_range(addr, size, PAGE_WRITE) < 0) {
+    switch (access_type) {
+    case MMU_DATA_STORE:
+        flags = PAGE_WRITE;
+        break;
+    case MMU_DATA_LOAD:
+        flags = PAGE_READ;
+        break;
+    case MMU_INST_FETCH:
+        flags = PAGE_EXEC;
+        break;
+    default:
+        g_assert_not_reached();
+    }
+
+    if (!guest_addr_valid(addr) || page_check_range(addr, size, flags) < 0) {
         CPUState *cpu = env_cpu(env);
         CPUClass *cc = CPU_GET_CLASS(cpu);
-
-        cc->tlb_fill(cpu, addr, size, MMU_DATA_STORE, MMU_USER_IDX, false,
+        cc->tlb_fill(cpu, addr, size, access_type, MMU_USER_IDX, false,
                      retaddr);
         g_assert_not_reached();
     }
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index a7893ed16b..3eaf39cd0d 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -310,8 +310,13 @@ static inline void tlb_flush_by_mmuidx_all_cpus_synced(CPUState *cpu,
 {
 }
 #endif
-void *probe_write(CPUArchState *env, target_ulong addr, int size, int mmu_idx,
-                  uintptr_t retaddr);
+void *probe_access(CPUArchState *env, target_ulong addr, int size,
+                   MMUAccessType access_type, int mmu_idx, uintptr_t retaddr);
+static inline void *probe_write(CPUArchState *env, target_ulong addr, int size,
+                                int mmu_idx, uintptr_t retaddr)
+{
+    return probe_access(env, addr, size, MMU_DATA_STORE, mmu_idx, retaddr);
+}
 
 #define CODE_GEN_ALIGN           16 /* must be >= of the size of a icache line */
 
-- 
2.21.0

Re: [Qemu-devel] [PATCH v2 2/2] tcg: Factor out probe_write() logic into probe_access()

[Richard Henderson]

On 8/30/19 3:09 AM, David Hildenbrand wrote:
> Let's also allow to probe other access types.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  accel/tcg/cputlb.c      | 43 ++++++++++++++++++++++++++++++-----------
>  accel/tcg/user-exec.c   | 26 +++++++++++++++++++------
>  include/exec/exec-all.h |  9 +++++++--
>  3 files changed, 59 insertions(+), 19 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v2 0/2] tcg: Introduce probe_access() and return a host pointer

[Richard Henderson]

On 8/30/19 3:09 AM, David Hildenbrand wrote:
> Let's return a host pointer from probe_access() if possible and provide
> probe_access(). s390x will soon make use of probe_access() and use the
> provided host pointer.
> 
> v1 -> v2:
> - Rebased to tcg-next
> - "tcg: Make probe_write() return a pointer to the host page"
> -- Rephrase documentation
> - "tcg: Factor out probe_write() logic into probe_access()"
> -- Don't check the MMU index
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Riku Voipio <riku.voipio@iki.fi>
> 
> David Hildenbrand (2):
>   tcg: Make probe_write() return a pointer to the host page
>   tcg: Factor out probe_write() logic into probe_access()
> 
>  accel/tcg/cputlb.c      | 60 +++++++++++++++++++++++++++++++----------
>  accel/tcg/user-exec.c   | 28 ++++++++++++++-----
>  include/exec/exec-all.h |  9 +++++--
>  3 files changed, 75 insertions(+), 22 deletions(-)
> 

Queued to tcg-next.


r~