diff mbox series

[v1,1/2] tcg: Make probe_write() return a pointer to the host page

Message ID 20190828111004.28013-2-david@redhat.com
State New
Headers show
Series tcg: Introduce probe_access() and return a host pointer | expand

Commit Message

David Hildenbrand Aug. 28, 2019, 11:10 a.m. UTC 
... 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      | 22 +++++++++++++++++-----
 accel/tcg/user-exec.c   |  6 ++++--
 include/exec/exec-all.h |  4 ++--
 3 files changed, 23 insertions(+), 9 deletions(-)

Comments

Richard Henderson Aug. 28, 2019, 2:44 p.m. UTC | #1 
On 8/28/19 4:10 AM, David Hildenbrand wrote:
> + * If the access is permitted, returns the host address similar to
> + * tlb_vaddr_to_host(). Returns NULL in case direct access to the host page
> + * is not allowed or if the size is 0.

Maybe we can find some better language -- "not allowed" sounds like a
permissions check, and we longjmp out on permission check failures.

Perhaps "if the page requires i/o access"?

Why are you returning NULL for size 0?  Just because the caller hasn't
committed to a size and thus we haven't checked watchpoints?


r~
David Hildenbrand Aug. 28, 2019, 8:34 p.m. UTC | #2 
On 28.08.19 16:44, Richard Henderson wrote:
> On 8/28/19 4:10 AM, David Hildenbrand wrote:
>> + * If the access is permitted, returns the host address similar to
>> + * tlb_vaddr_to_host(). Returns NULL in case direct access to the host page
>> + * is not allowed or if the size is 0.
> 
> Maybe we can find some better language -- "not allowed" sounds like a
> permissions check, and we longjmp out on permission check failures.
> 
> Perhaps "if the page requires i/o access"?

Yeah, and NOTDIRTY, emulated via i/o access. I will rephrase.

> 
> Why are you returning NULL for size 0?  Just because the caller hasn't
> committed to a size and thus we haven't checked watchpoints?

Yes, I think it's the right thing to do. Watchpoints are one part of the
story, the other part is TLB_RECHECK (a.k.a. TLB_INVALID_MASK or
"subpage protection"). Allowing access could lead to hidden BUGs.

I consider somebody wanting to get access to a host page with an access
size of zero an interface violation.
diff mbox series

Patch

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 09fe4cdcc4..2077685da0 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1078,11 +1078,12 @@  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 access is permitted, returns the host address similar to
+ * tlb_vaddr_to_host(). Returns NULL in case direct access to the host page
+ * is not allowed or if the size is 0.
  */
-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 +1102,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)) {
+        /* IO 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 b25a342eaa..420184571f 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)
 {
     CPUState *cpu = env_cpu(env);
     CPUClass *cc;
@@ -203,6 +203,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 */