[PATCH 0/5] accel/tcg: Clean cpu_memory_rw_debug() up

[PATCH 0/5] accel/tcg: Clean cpu_memory_rw_debug() up

[Philippe Mathieu-Daudé]

Address Richard' suggestions from [*], cleaning
cpu_memory_rw_debug() user implementation.

branch: https://gitlab.com/philmd/qemu/-/commits/user_cpu_memory_rw_debug

Based-on: <20250123234415.59850-14-philmd@linaro.org>
[*] https://lore.kernel.org/qemu-devel/20250123234415.59850-1-philmd@linaro.org/

Philippe Mathieu-Daudé (5):
  accel/accel-cpu-target.h: Include missing 'cpu.h' header
  accel/tcg: Include missing bswap headers in user-exec.c
  accel/tcg: Take mmap lock in the whole cpu_memory_rw_debug() function
  accel/tcg: Avoid using lock_user() in cpu_memory_rw_debug()
  accel/tcg: Move cpu_memory_rw_debug() user implementation to
    user-exec.c

 include/accel/accel-cpu-target.h |   3 +
 accel/tcg/user-exec.c            |  82 +++++++++++++++++++++++++
 cpu-target.c                     | 101 +------------------------------
 3 files changed, 87 insertions(+), 99 deletions(-)

-- 
2.47.1

[PATCH 1/5] accel/accel-cpu-target.h: Include missing 'cpu.h' header

[Philippe Mathieu-Daudé]

CPU_RESOLVING_TYPE is declared per target in "cpu.h". Include
it (along with "qom/object.h") to avoid when moving code around:

  include/accel/accel-cpu-target.h:26:50: error: expected ')'
     26 | DECLARE_CLASS_CHECKERS(AccelCPUClass, ACCEL_CPU, TYPE_ACCEL_CPU)
        |                                                  ^
  include/accel/accel-cpu-target.h:23:33: note: expanded from macro 'TYPE_ACCEL_CPU'
     23 | #define TYPE_ACCEL_CPU "accel-" CPU_RESOLVING_TYPE
        |                                 ^
  include/accel/accel-cpu-target.h:26:1: note: to match this '('
     26 | DECLARE_CLASS_CHECKERS(AccelCPUClass, ACCEL_CPU, TYPE_ACCEL_CPU)
        | ^
  include/qom/object.h:196:14: note: expanded from macro 'DECLARE_CLASS_CHECKERS'
    196 |     { return OBJECT_GET_CLASS(ClassType, obj, TYPENAME); } \
        |              ^
  include/qom/object.h:558:5: note: expanded from macro 'OBJECT_GET_CLASS'
    558 |     OBJECT_CLASS_CHECK(class, object_get_class(OBJECT(obj)), name)
        |     ^
  include/qom/object.h:544:74: note: expanded from macro 'OBJECT_CLASS_CHECK'
    544 |     ((class_type *)object_class_dynamic_cast_assert(OBJECT_CLASS(class), (name), \
        |                                                                          ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Acked-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20250123234415.59850-13-philmd@linaro.org>
---
 include/accel/accel-cpu-target.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/accel/accel-cpu-target.h b/include/accel/accel-cpu-target.h
index 0a8e518600d..37dde7fae3e 100644
--- a/include/accel/accel-cpu-target.h
+++ b/include/accel/accel-cpu-target.h
@@ -20,6 +20,9 @@
  * subclasses in target/, or the accel implementation itself in accel/
  */
 
+#include "qom/object.h"
+#include "cpu.h"
+
 #define TYPE_ACCEL_CPU "accel-" CPU_RESOLVING_TYPE
 #define ACCEL_CPU_NAME(name) (name "-" TYPE_ACCEL_CPU)
 typedef struct AccelCPUClass AccelCPUClass;
-- 
2.47.1

[PATCH 2/5] accel/tcg: Include missing bswap headers in user-exec.c

[Philippe Mathieu-Daudé]

Commit 35c653c4029 ("tcg: Add 128-bit guest memory
primitives") introduced the use of bswap128() which is
declared in "qemu/int128.h", commit de95016dfbf ("accel/tcg:
Implement helper_{ld,st}*_mmu for user-only") introduced the
other bswap*() uses, which are declared in "qemu/bswap.h".
Include the missing headers.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 accel/tcg/user-exec.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index c4454100ad7..9d53c9440ea 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -30,6 +30,8 @@
 #include "exec/page-protection.h"
 #include "exec/helper-proto.h"
 #include "qemu/atomic128.h"
+#include "qemu/bswap.h"
+#include "qemu/int128.h"
 #include "trace.h"
 #include "tcg/tcg-ldst.h"
 #include "internal-common.h"
-- 
2.47.1

Re: [PATCH 2/5] accel/tcg: Include missing bswap headers in user-exec.c

[Richard Henderson]

On 2/17/25 05:06, Philippe Mathieu-Daudé wrote:
> Commit 35c653c4029 ("tcg: Add 128-bit guest memory
> primitives") introduced the use of bswap128() which is
> declared in "qemu/int128.h", commit de95016dfbf ("accel/tcg:
> Implement helper_{ld,st}*_mmu for user-only") introduced the
> other bswap*() uses, which are declared in "qemu/bswap.h".
> Include the missing headers.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   accel/tcg/user-exec.c | 2 ++
>   1 file changed, 2 insertions(+)

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

r~

[PATCH 3/5] accel/tcg: Take mmap lock in the whole cpu_memory_rw_debug() function

[Philippe Mathieu-Daudé]

Simplify user implementation of cpu_memory_rw_debug() by
taking the mmap lock globally. See commit 87ab2704296
("linux-user: Allow gdbstub to ignore page protection")
for why this lock is necessary.

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 cpu-target.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/cpu-target.c b/cpu-target.c
index 20933bde7d4..b5230ce1837 100644
--- a/cpu-target.c
+++ b/cpu-target.c
@@ -380,6 +380,8 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
     int ret = -1;
     int fd = -1;
 
+    mmap_lock();
+
     while (len > 0) {
         page = addr & TARGET_PAGE_MASK;
         l = (page + TARGET_PAGE_SIZE) - addr;
@@ -414,11 +416,9 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
                  * be under mmap_lock() in order to prevent the creation of
                  * another TranslationBlock in between.
                  */
-                mmap_lock();
                 tb_invalidate_phys_range(addr, addr + l - 1);
                 written = pwrite(fd, buf, l,
                                  (off_t)(uintptr_t)g2h_untagged(addr));
-                mmap_unlock();
                 if (written != l) {
                     goto out_close;
                 }
@@ -454,6 +454,8 @@ out_close:
         close(fd);
     }
 out:
+    mmap_unlock();
+
     return ret;
 }
 #endif
-- 
2.47.1

Re: [PATCH 3/5] accel/tcg: Take mmap lock in the whole cpu_memory_rw_debug() function

[Richard Henderson]

On 2/17/25 05:06, Philippe Mathieu-Daudé wrote:
> Simplify user implementation of cpu_memory_rw_debug() by
> taking the mmap lock globally. See commit 87ab2704296
> ("linux-user: Allow gdbstub to ignore page protection")
> for why this lock is necessary.
> 
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   cpu-target.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)

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

r~

[PATCH 4/5] accel/tcg: Avoid using lock_user() in cpu_memory_rw_debug()

[Philippe Mathieu-Daudé]

We checked the page flags with page_get_flags(), so
locking the page is superfluous. Remove the lock_user()
calls and directly use g2h() in place.

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 cpu-target.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/cpu-target.c b/cpu-target.c
index b5230ce1837..3892ce12229 100644
--- a/cpu-target.c
+++ b/cpu-target.c
@@ -374,7 +374,6 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
 {
     int flags;
     vaddr l, page;
-    void * p;
     uint8_t *buf = ptr;
     ssize_t written;
     int ret = -1;
@@ -393,13 +392,7 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
         }
         if (is_write) {
             if (flags & PAGE_WRITE) {
-                /* XXX: this code should not depend on lock_user */
-                p = lock_user(VERIFY_WRITE, addr, l, 0);
-                if (!p) {
-                    goto out_close;
-                }
-                memcpy(p, buf, l);
-                unlock_user(p, addr, l);
+                memcpy(g2h(cpu, addr), buf, l);
             } else {
                 /* Bypass the host page protection using ptrace. */
                 if (fd == -1) {
@@ -424,13 +417,7 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
                 }
             }
         } else if (flags & PAGE_READ) {
-            /* XXX: this code should not depend on lock_user */
-            p = lock_user(VERIFY_READ, addr, l, 1);
-            if (!p) {
-                goto out_close;
-            }
-            memcpy(buf, p, l);
-            unlock_user(p, addr, 0);
+            memcpy(buf, g2h(cpu, addr), l);
         } else {
             /* Bypass the host page protection using ptrace. */
             if (fd == -1) {
-- 
2.47.1

Re: [PATCH 4/5] accel/tcg: Avoid using lock_user() in cpu_memory_rw_debug()

[Richard Henderson]

On 2/17/25 05:06, Philippe Mathieu-Daudé wrote:
> We checked the page flags with page_get_flags(), so
> locking the page is superfluous. Remove the lock_user()
> calls and directly use g2h() in place.
> 
> Suggested-by: Richard Henderson<richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   cpu-target.c | 17 ++---------------
>   1 file changed, 2 insertions(+), 15 deletions(-)

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

r~

[PATCH 5/5] accel/tcg: Move cpu_memory_rw_debug() user implementation to user-exec.c

[Philippe Mathieu-Daudé]

cpu_memory_rw_debug() system implementation is defined in
system/physmem.c. Move the user one to accel/tcg/user-exec.c
to simplify cpu-target.c maintenance.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 accel/tcg/user-exec.c | 80 ++++++++++++++++++++++++++++++++++++++
 cpu-target.c          | 90 +------------------------------------------
 2 files changed, 82 insertions(+), 88 deletions(-)

diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index 9d53c9440ea..2322181b151 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -19,6 +19,7 @@
 #include "qemu/osdep.h"
 #include "accel/tcg/cpu-ops.h"
 #include "disas/disas.h"
+#include "exec/vaddr.h"
 #include "exec/exec-all.h"
 #include "tcg/tcg.h"
 #include "qemu/bitops.h"
@@ -971,6 +972,85 @@ static void *cpu_mmu_lookup(CPUState *cpu, vaddr addr,
     return ret;
 }
 
+/* physical memory access (slow version, mainly for debug) */
+int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
+                        void *ptr, size_t len, bool is_write)
+{
+    int flags;
+    vaddr l, page;
+    uint8_t *buf = ptr;
+    ssize_t written;
+    int ret = -1;
+    int fd = -1;
+
+    mmap_lock();
+
+    while (len > 0) {
+        page = addr & TARGET_PAGE_MASK;
+        l = (page + TARGET_PAGE_SIZE) - addr;
+        if (l > len) {
+            l = len;
+        }
+        flags = page_get_flags(page);
+        if (!(flags & PAGE_VALID)) {
+            goto out_close;
+        }
+        if (is_write) {
+            if (flags & PAGE_WRITE) {
+                memcpy(g2h(cpu, addr), buf, l);
+            } else {
+                /* Bypass the host page protection using ptrace. */
+                if (fd == -1) {
+                    fd = open("/proc/self/mem", O_WRONLY);
+                    if (fd == -1) {
+                        goto out;
+                    }
+                }
+                /*
+                 * If there is a TranslationBlock and we weren't bypassing the
+                 * host page protection, the memcpy() above would SEGV,
+                 * ultimately leading to page_unprotect(). So invalidate the
+                 * translations manually. Both invalidation and pwrite() must
+                 * be under mmap_lock() in order to prevent the creation of
+                 * another TranslationBlock in between.
+                 */
+                tb_invalidate_phys_range(addr, addr + l - 1);
+                written = pwrite(fd, buf, l,
+                                 (off_t)(uintptr_t)g2h_untagged(addr));
+                if (written != l) {
+                    goto out_close;
+                }
+            }
+        } else if (flags & PAGE_READ) {
+            memcpy(buf, g2h(cpu, addr), l);
+        } else {
+            /* Bypass the host page protection using ptrace. */
+            if (fd == -1) {
+                fd = open("/proc/self/mem", O_RDONLY);
+                if (fd == -1) {
+                    goto out;
+                }
+            }
+            if (pread(fd, buf, l,
+                      (off_t)(uintptr_t)g2h_untagged(addr)) != l) {
+                goto out_close;
+            }
+        }
+        len -= l;
+        buf += l;
+        addr += l;
+    }
+    ret = 0;
+out_close:
+    if (fd != -1) {
+        close(fd);
+    }
+out:
+    mmap_unlock();
+
+    return ret;
+}
+
 #include "ldst_atomicity.c.inc"
 
 static uint8_t do_ld1_mmu(CPUState *cpu, vaddr addr, MemOpIdx oi,
diff --git a/cpu-target.c b/cpu-target.c
index 3892ce12229..83688f1d508 100644
--- a/cpu-target.c
+++ b/cpu-target.c
@@ -19,18 +19,12 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
-
-#include "exec/target_page.h"
-#include "exec/page-protection.h"
 #include "hw/qdev-core.h"
 #include "hw/qdev-properties.h"
 #include "qemu/error-report.h"
 #include "qemu/qemu-print.h"
 #include "migration/vmstate.h"
-#ifdef CONFIG_USER_ONLY
-#include "qemu.h"
-#include "user/page-protection.h"
-#else
+#ifndef CONFIG_USER_ONLY
 #include "hw/core/sysemu-cpu-ops.h"
 #include "exec/address-spaces.h"
 #include "exec/memory.h"
@@ -43,11 +37,11 @@
 #include "exec/cpu-common.h"
 #include "exec/exec-all.h"
 #include "exec/tb-flush.h"
-#include "exec/translation-block.h"
 #include "exec/log.h"
 #include "accel/accel-cpu-target.h"
 #include "trace/trace-root.h"
 #include "qemu/accel.h"
+#include "hw/core/cpu.h"
 
 #ifndef CONFIG_USER_ONLY
 static int cpu_common_post_load(void *opaque, int version_id)
@@ -367,86 +361,6 @@ void cpu_abort(CPUState *cpu, const char *fmt, ...)
     abort();
 }
 
-/* physical memory access (slow version, mainly for debug) */
-#if defined(CONFIG_USER_ONLY)
-int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
-                        void *ptr, size_t len, bool is_write)
-{
-    int flags;
-    vaddr l, page;
-    uint8_t *buf = ptr;
-    ssize_t written;
-    int ret = -1;
-    int fd = -1;
-
-    mmap_lock();
-
-    while (len > 0) {
-        page = addr & TARGET_PAGE_MASK;
-        l = (page + TARGET_PAGE_SIZE) - addr;
-        if (l > len)
-            l = len;
-        flags = page_get_flags(page);
-        if (!(flags & PAGE_VALID)) {
-            goto out_close;
-        }
-        if (is_write) {
-            if (flags & PAGE_WRITE) {
-                memcpy(g2h(cpu, addr), buf, l);
-            } else {
-                /* Bypass the host page protection using ptrace. */
-                if (fd == -1) {
-                    fd = open("/proc/self/mem", O_WRONLY);
-                    if (fd == -1) {
-                        goto out;
-                    }
-                }
-                /*
-                 * If there is a TranslationBlock and we weren't bypassing the
-                 * host page protection, the memcpy() above would SEGV,
-                 * ultimately leading to page_unprotect(). So invalidate the
-                 * translations manually. Both invalidation and pwrite() must
-                 * be under mmap_lock() in order to prevent the creation of
-                 * another TranslationBlock in between.
-                 */
-                tb_invalidate_phys_range(addr, addr + l - 1);
-                written = pwrite(fd, buf, l,
-                                 (off_t)(uintptr_t)g2h_untagged(addr));
-                if (written != l) {
-                    goto out_close;
-                }
-            }
-        } else if (flags & PAGE_READ) {
-            memcpy(buf, g2h(cpu, addr), l);
-        } else {
-            /* Bypass the host page protection using ptrace. */
-            if (fd == -1) {
-                fd = open("/proc/self/mem", O_RDONLY);
-                if (fd == -1) {
-                    goto out;
-                }
-            }
-            if (pread(fd, buf, l,
-                      (off_t)(uintptr_t)g2h_untagged(addr)) != l) {
-                goto out_close;
-            }
-        }
-        len -= l;
-        buf += l;
-        addr += l;
-    }
-    ret = 0;
-out_close:
-    if (fd != -1) {
-        close(fd);
-    }
-out:
-    mmap_unlock();
-
-    return ret;
-}
-#endif
-
 bool target_words_bigendian(void)
 {
     return TARGET_BIG_ENDIAN;
-- 
2.47.1

Re: [PATCH 5/5] accel/tcg: Move cpu_memory_rw_debug() user implementation to user-exec.c

[Richard Henderson]

On 2/17/25 05:06, Philippe Mathieu-Daudé wrote:
> cpu_memory_rw_debug() system implementation is defined in
> system/physmem.c. Move the user one to accel/tcg/user-exec.c
> to simplify cpu-target.c maintenance.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   accel/tcg/user-exec.c | 80 ++++++++++++++++++++++++++++++++++++++
>   cpu-target.c          | 90 +------------------------------------------
>   2 files changed, 82 insertions(+), 88 deletions(-)

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

r~

Re: [PATCH 0/5] accel/tcg: Clean cpu_memory_rw_debug() up

[Philippe Mathieu-Daudé]

On 17/2/25 14:06, Philippe Mathieu-Daudé wrote:

> Philippe Mathieu-Daudé (5):
>    accel/accel-cpu-target.h: Include missing 'cpu.h' header
>    accel/tcg: Include missing bswap headers in user-exec.c
>    accel/tcg: Take mmap lock in the whole cpu_memory_rw_debug() function
>    accel/tcg: Avoid using lock_user() in cpu_memory_rw_debug()
>    accel/tcg: Move cpu_memory_rw_debug() user implementation to
>      user-exec.c

Series queued, thanks.