[PATCH v1 1/3] semihosting: Move include/hw/semihosting/ -> include/semihosting/

[PATCH v1 1/3] semihosting: Move include/hw/semihosting/ -> include/semihosting/

[Alex Bennée]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

We want to move the semihosting code out of hw/ in the next patch.

This patch contains the mechanical steps, created using:

  $ git mv include/hw/semihosting/ include/
  $ sed -i s,hw/semihosting,semihosting, $(git grep -l hw/semihosting)

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20210226131356.3964782-2-f4bug@amsat.org>
---
 include/{hw => }/semihosting/console.h  | 0
 include/{hw => }/semihosting/semihost.h | 0
 gdbstub.c                               | 2 +-
 hw/mips/malta.c                         | 2 +-
 hw/semihosting/arm-compat-semi.c        | 6 +++---
 hw/semihosting/config.c                 | 2 +-
 hw/semihosting/console.c                | 4 ++--
 linux-user/aarch64/cpu_loop.c           | 2 +-
 linux-user/arm/cpu_loop.c               | 2 +-
 linux-user/riscv/cpu_loop.c             | 2 +-
 linux-user/semihost.c                   | 2 +-
 softmmu/vl.c                            | 2 +-
 stubs/semihost.c                        | 2 +-
 target/arm/helper.c                     | 4 ++--
 target/arm/m_helper.c                   | 4 ++--
 target/arm/translate-a64.c              | 2 +-
 target/arm/translate.c                  | 2 +-
 target/lm32/helper.c                    | 2 +-
 target/m68k/op_helper.c                 | 2 +-
 target/mips/cpu.c                       | 2 +-
 target/mips/mips-semi.c                 | 4 ++--
 target/mips/translate.c                 | 2 +-
 target/nios2/helper.c                   | 2 +-
 target/riscv/cpu_helper.c               | 2 +-
 target/unicore32/helper.c               | 2 +-
 target/xtensa/translate.c               | 2 +-
 target/xtensa/xtensa-semi.c             | 2 +-
 MAINTAINERS                             | 2 +-
 28 files changed, 32 insertions(+), 32 deletions(-)
 rename include/{hw => }/semihosting/console.h (100%)
 rename include/{hw => }/semihosting/semihost.h (100%)

diff --git a/include/hw/semihosting/console.h b/include/semihosting/console.h
similarity index 100%
rename from include/hw/semihosting/console.h
rename to include/semihosting/console.h
diff --git a/include/hw/semihosting/semihost.h b/include/semihosting/semihost.h
similarity index 100%
rename from include/hw/semihosting/semihost.h
rename to include/semihosting/semihost.h
diff --git a/gdbstub.c b/gdbstub.c
index 3ee40479b6..e51e33cc70 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -49,7 +49,7 @@
 #include "sysemu/hw_accel.h"
 #include "sysemu/kvm.h"
 #include "sysemu/runstate.h"
-#include "hw/semihosting/semihost.h"
+#include "semihosting/semihost.h"
 #include "exec/exec-all.h"
 #include "sysemu/replay.h"
 
diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index 9afc0b427b..26e7b1bd9f 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -58,7 +58,7 @@
 #include "qemu/error-report.h"
 #include "hw/misc/empty_slot.h"
 #include "sysemu/kvm.h"
-#include "hw/semihosting/semihost.h"
+#include "semihosting/semihost.h"
 #include "hw/mips/cps.h"
 #include "hw/qdev-clock.h"
 
diff --git a/hw/semihosting/arm-compat-semi.c b/hw/semihosting/arm-compat-semi.c
index 23c6e3edcb..94950b6c56 100644
--- a/hw/semihosting/arm-compat-semi.c
+++ b/hw/semihosting/arm-compat-semi.c
@@ -34,9 +34,9 @@
 #include "qemu/osdep.h"
 
 #include "cpu.h"
-#include "hw/semihosting/semihost.h"
-#include "hw/semihosting/console.h"
-#include "hw/semihosting/common-semi.h"
+#include "semihosting/semihost.h"
+#include "semihosting/console.h"
+#include "semihosting/common-semi.h"
 #include "qemu/log.h"
 #include "qemu/timer.h"
 #ifdef CONFIG_USER_ONLY
diff --git a/hw/semihosting/config.c b/hw/semihosting/config.c
index 9807f10cb0..3548e0f627 100644
--- a/hw/semihosting/config.c
+++ b/hw/semihosting/config.c
@@ -22,7 +22,7 @@
 #include "qemu/option.h"
 #include "qemu/config-file.h"
 #include "qemu/error-report.h"
-#include "hw/semihosting/semihost.h"
+#include "semihosting/semihost.h"
 #include "chardev/char.h"
 #include "sysemu/sysemu.h"
 
diff --git a/hw/semihosting/console.c b/hw/semihosting/console.c
index 9b4fee9260..c9ebd6fdd0 100644
--- a/hw/semihosting/console.c
+++ b/hw/semihosting/console.c
@@ -17,8 +17,8 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
-#include "hw/semihosting/semihost.h"
-#include "hw/semihosting/console.h"
+#include "semihosting/semihost.h"
+#include "semihosting/console.h"
 #include "exec/gdbstub.h"
 #include "exec/exec-all.h"
 #include "qemu/log.h"
diff --git a/linux-user/aarch64/cpu_loop.c b/linux-user/aarch64/cpu_loop.c
index 7c42f65706..ee72a1c20f 100644
--- a/linux-user/aarch64/cpu_loop.c
+++ b/linux-user/aarch64/cpu_loop.c
@@ -22,7 +22,7 @@
 #include "qemu.h"
 #include "cpu_loop-common.h"
 #include "qemu/guest-random.h"
-#include "hw/semihosting/common-semi.h"
+#include "semihosting/common-semi.h"
 #include "target/arm/syndrome.h"
 
 #define get_user_code_u32(x, gaddr, env)                \
diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
index cadfb7fa43..989d03cd89 100644
--- a/linux-user/arm/cpu_loop.c
+++ b/linux-user/arm/cpu_loop.c
@@ -22,7 +22,7 @@
 #include "qemu.h"
 #include "elf.h"
 #include "cpu_loop-common.h"
-#include "hw/semihosting/common-semi.h"
+#include "semihosting/common-semi.h"
 
 #define get_user_code_u32(x, gaddr, env)                \
     ({ abi_long __r = get_user_u32((x), (gaddr));       \
diff --git a/linux-user/riscv/cpu_loop.c b/linux-user/riscv/cpu_loop.c
index 9665dabb09..6767f941e8 100644
--- a/linux-user/riscv/cpu_loop.c
+++ b/linux-user/riscv/cpu_loop.c
@@ -23,7 +23,7 @@
 #include "qemu.h"
 #include "cpu_loop-common.h"
 #include "elf.h"
-#include "hw/semihosting/common-semi.h"
+#include "semihosting/common-semi.h"
 
 void cpu_loop(CPURISCVState *env)
 {
diff --git a/linux-user/semihost.c b/linux-user/semihost.c
index c0015ee7f6..82013b8b48 100644
--- a/linux-user/semihost.c
+++ b/linux-user/semihost.c
@@ -12,7 +12,7 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
-#include "hw/semihosting/console.h"
+#include "semihosting/console.h"
 #include "qemu.h"
 #include <termios.h>
 
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 10bd8a10a3..ac06dfbae0 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -108,7 +108,7 @@
 #include "qapi/opts-visitor.h"
 #include "qapi/clone-visitor.h"
 #include "qom/object_interfaces.h"
-#include "hw/semihosting/semihost.h"
+#include "semihosting/semihost.h"
 #include "crypto/init.h"
 #include "sysemu/replay.h"
 #include "qapi/qapi-events-run-state.h"
diff --git a/stubs/semihost.c b/stubs/semihost.c
index 1d8b37f7b2..1b30f38b03 100644
--- a/stubs/semihost.c
+++ b/stubs/semihost.c
@@ -11,7 +11,7 @@
 #include "qemu/osdep.h"
 #include "qemu/option.h"
 #include "qemu/error-report.h"
-#include "hw/semihosting/semihost.h"
+#include "semihosting/semihost.h"
 #include "sysemu/sysemu.h"
 
 /* Empty config */
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 0e1a3b9421..d763f376c6 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -22,7 +22,7 @@
 #include "exec/exec-all.h"
 #include <zlib.h> /* For crc32 */
 #include "hw/irq.h"
-#include "hw/semihosting/semihost.h"
+#include "semihosting/semihost.h"
 #include "sysemu/cpus.h"
 #include "sysemu/cpu-timers.h"
 #include "sysemu/kvm.h"
@@ -34,7 +34,7 @@
 #ifdef CONFIG_TCG
 #include "arm_ldst.h"
 #include "exec/cpu_ldst.h"
-#include "hw/semihosting/common-semi.h"
+#include "semihosting/common-semi.h"
 #endif
 
 #define ARM_CPU_FREQ 1000000000 /* FIXME: 1 GHz, should be configurable */
diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
index 731c435c00..d63ae465e1 100644
--- a/target/arm/m_helper.c
+++ b/target/arm/m_helper.c
@@ -21,7 +21,7 @@
 #include "qemu/qemu-print.h"
 #include "exec/exec-all.h"
 #include <zlib.h> /* For crc32 */
-#include "hw/semihosting/semihost.h"
+#include "semihosting/semihost.h"
 #include "sysemu/cpus.h"
 #include "sysemu/kvm.h"
 #include "qemu/range.h"
@@ -31,7 +31,7 @@
 #ifdef CONFIG_TCG
 #include "arm_ldst.h"
 #include "exec/cpu_ldst.h"
-#include "hw/semihosting/common-semi.h"
+#include "semihosting/common-semi.h"
 #endif
 
 static void v7m_msr_xpsr(CPUARMState *env, uint32_t mask,
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index b23a8975d5..6d002e2c63 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -28,7 +28,7 @@
 #include "internals.h"
 #include "qemu/host-utils.h"
 
-#include "hw/semihosting/semihost.h"
+#include "semihosting/semihost.h"
 #include "exec/gen-icount.h"
 
 #include "exec/helper-proto.h"
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 1653cca1aa..62b1c2081b 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -29,7 +29,7 @@
 #include "qemu/log.h"
 #include "qemu/bitops.h"
 #include "arm_ldst.h"
-#include "hw/semihosting/semihost.h"
+#include "semihosting/semihost.h"
 
 #include "exec/helper-proto.h"
 #include "exec/helper-gen.h"
diff --git a/target/lm32/helper.c b/target/lm32/helper.c
index 7c52ae76d6..01cc3c53a5 100644
--- a/target/lm32/helper.c
+++ b/target/lm32/helper.c
@@ -21,7 +21,7 @@
 #include "cpu.h"
 #include "exec/exec-all.h"
 #include "qemu/host-utils.h"
-#include "hw/semihosting/semihost.h"
+#include "semihosting/semihost.h"
 #include "exec/log.h"
 
 bool lm32_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
index 202498deb5..730cdf7744 100644
--- a/target/m68k/op_helper.c
+++ b/target/m68k/op_helper.c
@@ -21,7 +21,7 @@
 #include "exec/helper-proto.h"
 #include "exec/exec-all.h"
 #include "exec/cpu_ldst.h"
-#include "hw/semihosting/semihost.h"
+#include "semihosting/semihost.h"
 
 #if defined(CONFIG_USER_ONLY)
 
diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index bf70c77295..bd4dca571f 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -31,7 +31,7 @@
 #include "exec/exec-all.h"
 #include "hw/qdev-properties.h"
 #include "hw/qdev-clock.h"
-#include "hw/semihosting/semihost.h"
+#include "semihosting/semihost.h"
 #include "qapi/qapi-commands-machine-target.h"
 #include "fpu_helper.h"
 
diff --git a/target/mips/mips-semi.c b/target/mips/mips-semi.c
index 898251aa02..6de60fa6dd 100644
--- a/target/mips/mips-semi.c
+++ b/target/mips/mips-semi.c
@@ -22,8 +22,8 @@
 #include "qemu/log.h"
 #include "exec/helper-proto.h"
 #include "exec/softmmu-semi.h"
-#include "hw/semihosting/semihost.h"
-#include "hw/semihosting/console.h"
+#include "semihosting/semihost.h"
+#include "semihosting/console.h"
 
 typedef enum UHIOp {
     UHI_exit = 1,
diff --git a/target/mips/translate.c b/target/mips/translate.c
index 70891c37cd..0b6d82d228 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -29,7 +29,7 @@
 #include "exec/translator.h"
 #include "exec/helper-proto.h"
 #include "exec/helper-gen.h"
-#include "hw/semihosting/semihost.h"
+#include "semihosting/semihost.h"
 
 #include "target/mips/trace.h"
 #include "trace-tcg.h"
diff --git a/target/nios2/helper.c b/target/nios2/helper.c
index 57c97bde3c..53be8398e9 100644
--- a/target/nios2/helper.c
+++ b/target/nios2/helper.c
@@ -26,7 +26,7 @@
 #include "exec/cpu_ldst.h"
 #include "exec/log.h"
 #include "exec/helper-proto.h"
-#include "hw/semihosting/semihost.h"
+#include "semihosting/semihost.h"
 
 #if defined(CONFIG_USER_ONLY)
 
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 2f43939fb6..83a6bcfad0 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -24,7 +24,7 @@
 #include "exec/exec-all.h"
 #include "tcg/tcg-op.h"
 #include "trace.h"
-#include "hw/semihosting/common-semi.h"
+#include "semihosting/common-semi.h"
 
 int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
 {
diff --git a/target/unicore32/helper.c b/target/unicore32/helper.c
index 54c26871fe..704393c27f 100644
--- a/target/unicore32/helper.c
+++ b/target/unicore32/helper.c
@@ -14,7 +14,7 @@
 #include "cpu.h"
 #include "exec/exec-all.h"
 #include "exec/helper-proto.h"
-#include "hw/semihosting/console.h"
+#include "semihosting/console.h"
 
 #undef DEBUG_UC32
 
diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
index 944a157747..0ae4efc48a 100644
--- a/target/xtensa/translate.c
+++ b/target/xtensa/translate.c
@@ -37,7 +37,7 @@
 #include "qemu/log.h"
 #include "qemu/qemu-print.h"
 #include "exec/cpu_ldst.h"
-#include "hw/semihosting/semihost.h"
+#include "semihosting/semihost.h"
 #include "exec/translator.h"
 
 #include "exec/helper-proto.h"
diff --git a/target/xtensa/xtensa-semi.c b/target/xtensa/xtensa-semi.c
index 25f57a6500..79f2b043f2 100644
--- a/target/xtensa/xtensa-semi.c
+++ b/target/xtensa/xtensa-semi.c
@@ -29,7 +29,7 @@
 #include "cpu.h"
 #include "chardev/char-fe.h"
 #include "exec/helper-proto.h"
-#include "hw/semihosting/semihost.h"
+#include "semihosting/semihost.h"
 #include "qapi/error.h"
 #include "qemu/log.h"
 
diff --git a/MAINTAINERS b/MAINTAINERS
index 1443278059..37ddf90669 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3227,7 +3227,7 @@ Semihosting
 M: Alex Bennée <alex.bennee@linaro.org>
 S: Maintained
 F: hw/semihosting/
-F: include/hw/semihosting/
+F: include/semihosting/
 
 Multi-process QEMU
 M: Elena Ufimtseva <elena.ufimtseva@oracle.com>
-- 
2.20.1

[PATCH v1 3/3] semihosting/arg-compat: fix up handling of SYS_HEAPINFO

[Alex Bennée]

I'm not sure this every worked properly and it's certainly not
exercised by check-tcg or Peter's semihosting tests. Hoist it into
it's own helper function and attempt to validate the results in the
linux-user semihosting test at the least.

Bug: https://bugs.launchpad.net/bugs/1915925
Cc: Bug 1915925 <1915925@bugs.launchpad.net>
Cc: Keith Packard <keithp@keithp.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/tcg/arm/semicall.h      |   1 +
 semihosting/arm-compat-semi.c | 129 +++++++++++++++++++---------------
 tests/tcg/arm/semihosting.c   |  34 ++++++++-
 3 files changed, 107 insertions(+), 57 deletions(-)

diff --git a/tests/tcg/arm/semicall.h b/tests/tcg/arm/semicall.h
index d4f6818192..676a542be5 100644
--- a/tests/tcg/arm/semicall.h
+++ b/tests/tcg/arm/semicall.h
@@ -9,6 +9,7 @@
 
 #define SYS_WRITE0      0x04
 #define SYS_READC       0x07
+#define SYS_HEAPINFO    0x16
 #define SYS_REPORTEXC   0x18
 
 uintptr_t __semi_call(uintptr_t type, uintptr_t arg0)
diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
index 94950b6c56..a8fdbceb5f 100644
--- a/semihosting/arm-compat-semi.c
+++ b/semihosting/arm-compat-semi.c
@@ -822,6 +822,78 @@ static const GuestFDFunctions guestfd_fns[] = {
     put_user_utl(val, args + (n) * sizeof(target_ulong))
 #endif
 
+/*
+ * SYS_HEAPINFO is a little weird: "On entry, the PARAMETER REGISTER
+ * contains the address of a pointer to a four-field data block" which
+ * we then fill in. The PARAMETER REGISTER is unchanged.
+ */
+
+struct HeapInfo {
+    target_ulong heap_base;
+    target_ulong heap_limit;
+    target_ulong stack_base;
+    target_ulong stack_limit;
+};
+
+static bool do_heapinfo(CPUState *cs, target_long arg0)
+{
+    target_ulong limit;
+    struct HeapInfo info = {};
+#ifdef CONFIG_USER_ONLY
+    TaskState *ts = cs->opaque;
+#else
+    target_ulong rambase = common_semi_rambase(cs);
+#endif
+
+#ifdef CONFIG_USER_ONLY
+    /*
+     * Some C libraries assume the heap immediately follows .bss, so
+     * allocate it using sbrk.
+     */
+    if (!ts->heap_limit) {
+        abi_ulong ret;
+
+        ts->heap_base = do_brk(0);
+        limit = ts->heap_base + COMMON_SEMI_HEAP_SIZE;
+        /* Try a big heap, and reduce the size if that fails.  */
+        for (;;) {
+            ret = do_brk(limit);
+            if (ret >= limit) {
+                break;
+            }
+            limit = (ts->heap_base >> 1) + (limit >> 1);
+        }
+        ts->heap_limit = limit;
+    }
+
+    info.heap_base = ts->heap_base;
+    info.heap_limit = ts->heap_limit;
+    info.stack_base = ts->stack_base;
+    info.stack_limit = 0; /* Stack limit.  */
+
+    if (copy_to_user(arg0, &info, sizeof(info))) {
+        errno = EFAULT;
+        return  set_swi_errno(cs, -1);
+    }
+#else
+    limit = current_machine->ram_size;
+    /* TODO: Make this use the limit of the loaded application.  */
+    info.heap_base = rambase + limit / 2;
+    info.heap_limit = rambase + limit;
+    info.stack_base = rambase + limit; /* Stack base */
+    info.stack_limit = rambase; /* Stack limit.  */
+
+    if (cpu_memory_rw_debug(cs, arg0, &info, sizeof(info), true)) {
+        errno = EFAULT;
+        return  set_swi_errno(cs, -1);
+    }
+
+#endif
+
+    return 0;
+}
+
+
 /*
  * Do a semihosting call.
  *
@@ -1184,63 +1256,8 @@ target_ulong do_common_semihosting(CPUState *cs)
         }
     case TARGET_SYS_HEAPINFO:
         {
-            target_ulong retvals[4];
-            target_ulong limit;
-            int i;
-#ifdef CONFIG_USER_ONLY
-            TaskState *ts = cs->opaque;
-#else
-            target_ulong rambase = common_semi_rambase(cs);
-#endif
-
             GET_ARG(0);
-
-#ifdef CONFIG_USER_ONLY
-            /*
-             * Some C libraries assume the heap immediately follows .bss, so
-             * allocate it using sbrk.
-             */
-            if (!ts->heap_limit) {
-                abi_ulong ret;
-
-                ts->heap_base = do_brk(0);
-                limit = ts->heap_base + COMMON_SEMI_HEAP_SIZE;
-                /* Try a big heap, and reduce the size if that fails.  */
-                for (;;) {
-                    ret = do_brk(limit);
-                    if (ret >= limit) {
-                        break;
-                    }
-                    limit = (ts->heap_base >> 1) + (limit >> 1);
-                }
-                ts->heap_limit = limit;
-            }
-
-            retvals[0] = ts->heap_base;
-            retvals[1] = ts->heap_limit;
-            retvals[2] = ts->stack_base;
-            retvals[3] = 0; /* Stack limit.  */
-#else
-            limit = current_machine->ram_size;
-            /* TODO: Make this use the limit of the loaded application.  */
-            retvals[0] = rambase + limit / 2;
-            retvals[1] = rambase + limit;
-            retvals[2] = rambase + limit; /* Stack base */
-            retvals[3] = rambase; /* Stack limit.  */
-#endif
-
-            for (i = 0; i < ARRAY_SIZE(retvals); i++) {
-                bool fail;
-
-                fail = SET_ARG(i, retvals[i]);
-
-                if (fail) {
-                    /* Couldn't write back to argument block */
-                    errno = EFAULT;
-                    return set_swi_errno(cs, -1);
-                }
-            }
-            return 0;
+            return do_heapinfo(cs, arg0);
         }
     case TARGET_SYS_EXIT:
     case TARGET_SYS_EXIT_EXTENDED:
diff --git a/tests/tcg/arm/semihosting.c b/tests/tcg/arm/semihosting.c
index 33faac9916..fd5780ec3c 100644
--- a/tests/tcg/arm/semihosting.c
+++ b/tests/tcg/arm/semihosting.c
@@ -7,7 +7,13 @@
  * SPDX-License-Identifier: GPL-3.0-or-later
  */
 
+#define _GNU_SOURCE  /* asprintf is a GNU extension */
+
 #include <stdint.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <unistd.h>
 #include "semicall.h"
 
 int main(int argc, char *argv[argc])
@@ -18,8 +24,34 @@ int main(int argc, char *argv[argc])
     uintptr_t exit_block[2] = {0x20026, 0};
     uintptr_t exit_code = (uintptr_t) &exit_block;
 #endif
+    struct {
+        void *heap_base;
+        void *heap_limit;
+        void *stack_base;
+        void *stack_limit;
+    } info;
+    void *ptr_to_info = (void *) &info;
+    char *heap_info, *stack_info;
+    void *brk = sbrk(0);
+
+    __semi_call(SYS_WRITE0, (uintptr_t) "Hello World\n");
+
+    memset(&info, 0, sizeof(info));
+    __semi_call(SYS_HEAPINFO, (uintptr_t) &ptr_to_info);
+
+    asprintf(&heap_info, "heap: %p -> %p\n", info.heap_base, info.heap_limit);
+    __semi_call(SYS_WRITE0, (uintptr_t) heap_info);
+    if (info.heap_base != brk) {
+        sprintf(heap_info, "heap mismatch: %p\n", brk);
+        __semi_call(SYS_WRITE0, (uintptr_t) heap_info);
+        return -1;
+    }
+
+    asprintf(&stack_info, "stack: %p -> %p\n", info.stack_base, info.stack_limit);
+    __semi_call(SYS_WRITE0, (uintptr_t) stack_info);
+    free(heap_info);
+    free(stack_info);
 
-    __semi_call(SYS_WRITE0, (uintptr_t) "Hello World");
     __semi_call(SYS_REPORTEXC, exit_code);
     /* if we get here we failed */
     return -1;
-- 
2.20.1

Re: [PATCH v1 3/3] semihosting/arg-compat: fix up handling of SYS_HEAPINFO

[Peter Maydell]

On Fri, 5 Mar 2021 at 13:54, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> I'm not sure this every worked properly and it's certainly not
> exercised by check-tcg or Peter's semihosting tests. Hoist it into
> it's own helper function and attempt to validate the results in the
> linux-user semihosting test at the least.
>
> Bug: https://bugs.launchpad.net/bugs/1915925
> Cc: Bug 1915925 <1915925@bugs.launchpad.net>
> Cc: Keith Packard <keithp@keithp.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  tests/tcg/arm/semicall.h      |   1 +
>  semihosting/arm-compat-semi.c | 129 +++++++++++++++++++---------------
>  tests/tcg/arm/semihosting.c   |  34 ++++++++-
>  3 files changed, 107 insertions(+), 57 deletions(-)
> +#else
> +    limit = current_machine->ram_size;
> +    /* TODO: Make this use the limit of the loaded application.  */
> +    info.heap_base = rambase + limit / 2;
> +    info.heap_limit = rambase + limit;
> +    info.stack_base = rambase + limit; /* Stack base */
> +    info.stack_limit = rambase; /* Stack limit.  */
> +
> +    if (cpu_memory_rw_debug(cs, arg0, &info, sizeof(info), true)) {

Blatting a C struct into guest memory has endianness and padding
problems. Why not just do things the way the old Arm code did it ?

Also, you don't seem to have the correct "is the CPU in
32-bit or 64-bit mode" test here: you cannot rely on target_ulong
being the right size, you must make a runtime check.

I suggested in the other email the way I think we should fix this.

thanks
-- PMM

Re: [PATCH v1 3/3] semihosting/arg-compat: fix up handling of SYS_HEAPINFO

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 625 bytes --]

Alex Bennée <alex.bennee@linaro.org> writes:

> I'm not sure this every worked properly and it's certainly not
> exercised by check-tcg or Peter's semihosting tests. Hoist it into
> it's own helper function and attempt to validate the results in the
> linux-user semihosting test at the least.

The patch is mostly code motion, moving the existing heapinfo stuff into
a separate function. That makes it really hard to see how you've
changed the values being returned. I'd love to see a two patch series,
one of which moves the code as-is and a second patch which fixes
whatever bugs you've found.

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v1 3/3] semihosting/arg-compat: fix up handling of SYS_HEAPINFO

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 373 bytes --]

Peter Maydell <peter.maydell@linaro.org> writes:

> Also, you don't seem to have the correct "is the CPU in
> 32-bit or 64-bit mode" test here: you cannot rely on target_ulong
> being the right size, you must make a runtime check.

Do you mean whether a dual aarch64/arm core is in arm or aarch64 mode,
or whether an aarch64 is running a 32-bit ABI?

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v1 3/3] semihosting/arg-compat: fix up handling of SYS_HEAPINFO

[Peter Maydell]

On Fri, 5 Mar 2021 at 20:22, Keith Packard <keithp@keithp.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > Also, you don't seem to have the correct "is the CPU in
> > 32-bit or 64-bit mode" test here: you cannot rely on target_ulong
> > being the right size, you must make a runtime check.
>
> Do you mean whether a dual aarch64/arm core is in arm or aarch64 mode,
> or whether an aarch64 is running a 32-bit ABI?

For semihosting for Arm what matters is "what state is the core
in at the point where it makes the semihosting SVC/HLT/etc insn?".

How does RISCV specify it?

thanks
-- PMM

Re: [PATCH v1 3/3] semihosting/arg-compat: fix up handling of SYS_HEAPINFO

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 1018 bytes --]

Peter Maydell <peter.maydell@linaro.org> writes:

> For semihosting for Arm what matters is "what state is the core
> in at the point where it makes the semihosting SVC/HLT/etc insn?".

Ok, that means we *aren't* talking about -mabi=ilp32, which is good --
in my current picolibc implementation, the semihosting code uses a pure
64-bit interface for aarch64 targets, even when using ilp32 ABI.

> How does RISCV specify it?

Because the ISA is identical between 64- and 32- bit (and 128-bit)
execution modes, the only difference between the two is the Machine XLEN
value which encodes the native base integer ISA width. You switch modes
by modifying this value.

I don't know of any implementation in hardware or software that supports
modifying this value. I'm not sure we need to support this in the
semihosting code for qemu as I'm pretty sure getting qemu to support
dynamic XLEN values would be a large project (a project which I don't
personally feel would offer much value).

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v1 3/3] semihosting/arg-compat: fix up handling of SYS_HEAPINFO

[Richard Henderson]

On 3/5/21 3:54 PM, Keith Packard via wrote:
> I don't know of any implementation in hardware or software that supports
> modifying this value. I'm not sure we need to support this in the
> semihosting code for qemu as I'm pretty sure getting qemu to support
> dynamic XLEN values would be a large project

I think it would be fairly trivial, now.  We have riscv_cpu_is_32bit (which 
checks misa, not xlen, fwiw).  It's not examined in cpu_get_tb_cpu_state(), but 
it is many other places.

> (a project which I don't
> personally feel would offer much value).

Now that's a different story.  I'll take no position there.

However!  We do have a function, that can be made into a TCG hook, which could 
then be queried by semihosting/.


r~

Re: [PATCH v1 3/3] semihosting/arg-compat: fix up handling of SYS_HEAPINFO

[Peter Maydell]

On Fri, 5 Mar 2021 at 23:54, Keith Packard <keithp@keithp.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > For semihosting for Arm what matters is "what state is the core
> > in at the point where it makes the semihosting SVC/HLT/etc insn?".
>
> Ok, that means we *aren't* talking about -mabi=ilp32, which is good --
> in my current picolibc implementation, the semihosting code uses a pure
> 64-bit interface for aarch64 targets, even when using ilp32 ABI.

ILP32 for AArch64 is a zombie target -- it is kinda-sorta
supported in some toolchains but has no support in eg
the Linux syscall ABI. The semihosting ABI does not implement
any kind of ILP32 variant -- you can have A32/T32 (AArch32)
semihosting, where register and field sizes are 32 bit, or
you can have A64 (AArch64) semihosting, where register and
field sizes are 64 bit.

> > How does RISCV specify it?
>
> Because the ISA is identical between 64- and 32- bit (and 128-bit)
> execution modes, the only difference between the two is the Machine XLEN
> value which encodes the native base integer ISA width. You switch modes
> by modifying this value.

I meant, how does the RISCV semihosting ABI specify what
the field size is? To answer my own question, I just looked at
the spec doc and it says "depends on whether the caller is
32-bit or 64-bit", which implies that we need to look at the
current state of the CPU in some way.

> I don't know of any implementation in hardware or software that supports
> modifying this value. I'm not sure we need to support this in the
> semihosting code for qemu as I'm pretty sure getting qemu to support
> dynamic XLEN values would be a large project (a project which I don't
> personally feel would offer much value).

Part of why I asked is that the current RISCV implementation
is just looking at sizeof(target_ulong); but the qemu-system-riscv64
executable AIUI now supports emulating both "this is a 64 bit
guest CPU" and "this is a 32 bit host CPU", and so looking at
a QEMU-compile-time value like "sizeof(target_ulong)" will
produce the wrong answer for 32-bit CPUs emulated in
the qemu-system-riscv64 binary. My guess is maybe
it should be looking at the result of riscv_cpu_is_32bit() instead.

thanks
-- PMM

Re: [PATCH v1 3/3] semihosting/arg-compat: fix up handling of SYS_HEAPINFO

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 2275 bytes --]

Peter Maydell <peter.maydell@linaro.org> writes:

> ILP32 for AArch64 is a zombie target -- it is kinda-sorta
> supported in some toolchains but has no support in eg
> the Linux syscall ABI. The semihosting ABI does not implement
> any kind of ILP32 variant -- you can have A32/T32 (AArch32)
> semihosting, where register and field sizes are 32 bit, or
> you can have A64 (AArch64) semihosting, where register and
> field sizes are 64 bit.

Yeah, I did ILP32 support for Picolibc; all of the aarch64 asm support
needed fixing as ilp32 doesn't specify that register arguments clear the
top 32 bits. Seems pretty obvious that it's little used.

For semihosting, as the ABI isn't visible to the hardware/emulator, the
only reasonable answer that I could come up with was to treat ILP32 the
same as the LP64 and pass 64 bit parameters.

As picolibc is designed for bare-metal environments, it's pretty easy to
support ilp32 otherwise.

> I meant, how does the RISCV semihosting ABI specify what
> the field size is? To answer my own question, I just looked at
> the spec doc and it says "depends on whether the caller is
> 32-bit or 64-bit", which implies that we need to look at the
> current state of the CPU in some way.

Yes. As qemu currently fixes that value based on compilation parameters,
we can use the relevant native types directly and ignore the CPU
state. Adding dynamic XLEN support to qemu would involve a bunch of work
as the same code can be run in both 64- and 32- bit modes, so you'd have
to translate it twice and select which to execute based on the CPU
state.

> Part of why I asked is that the current RISCV implementation
> is just looking at sizeof(target_ulong); but the qemu-system-riscv64
> executable AIUI now supports emulating both "this is a 64 bit
> guest CPU" and "this is a 32 bit host CPU", and so looking at
> a QEMU-compile-time value like "sizeof(target_ulong)" will
> produce the wrong answer for 32-bit CPUs emulated in
> the qemu-system-riscv64 binary. My guess is maybe
> it should be looking at the result of riscv_cpu_is_32bit() instead.

Wow. I read through the code and couldn't find anything that looked like
it supported that, sounds like I must have missed something?

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v1 3/3] semihosting/arg-compat: fix up handling of SYS_HEAPINFO

[Peter Maydell]

On Sat, 6 Mar 2021 at 16:54, Keith Packard <keithp@keithp.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
> > Part of why I asked is that the current RISCV implementation
> > is just looking at sizeof(target_ulong); but the qemu-system-riscv64
> > executable AIUI now supports emulating both "this is a 64 bit
> > guest CPU" and "this is a 32 bit host CPU", and so looking at
> > a QEMU-compile-time value like "sizeof(target_ulong)" will
> > produce the wrong answer for 32-bit CPUs emulated in
> > the qemu-system-riscv64 binary. My guess is maybe
> > it should be looking at the result of riscv_cpu_is_32bit() instead.
>
> Wow. I read through the code and couldn't find anything that looked like
> it supported that, sounds like I must have missed something?

I thought Alistair had done that work (which brings riscv into
line with the other 32/64 bit QEMU targets, which all support
the 32-bit CPU types in the 64-bit-capable executable). But maybe
it hasn't landed in master yet?

thanks
-- PMM

Re: [PATCH v1 3/3] semihosting/arg-compat: fix up handling of SYS_HEAPINFO

[Alistair Francis]

On Mon, Mar 8, 2021 at 5:10 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Sat, 6 Mar 2021 at 16:54, Keith Packard <keithp@keithp.com> wrote:
> >
> > Peter Maydell <peter.maydell@linaro.org> writes:
> > > Part of why I asked is that the current RISCV implementation
> > > is just looking at sizeof(target_ulong); but the qemu-system-riscv64
> > > executable AIUI now supports emulating both "this is a 64 bit
> > > guest CPU" and "this is a 32 bit host CPU", and so looking at
> > > a QEMU-compile-time value like "sizeof(target_ulong)" will
> > > produce the wrong answer for 32-bit CPUs emulated in
> > > the qemu-system-riscv64 binary. My guess is maybe
> > > it should be looking at the result of riscv_cpu_is_32bit() instead.
> >
> > Wow. I read through the code and couldn't find anything that looked like
> > it supported that, sounds like I must have missed something?

riscv_cpu_is_32bit() is somewhat new, so it might not have been there
when you wrote the patches.

>
> I thought Alistair had done that work (which brings riscv into
> line with the other 32/64 bit QEMU targets, which all support
> the 32-bit CPU types in the 64-bit-capable executable). But maybe
> it hasn't landed in master yet?

I have started on the effort, but I have not finished yet. Adding
riscv_cpu_is_32bit() was the first step there and I have some more
patches locally but I don't have anything working yet.

Alistair

>
> thanks
> -- PMM

Re: [PATCH v1 3/3] semihosting/arg-compat: fix up handling of SYS_HEAPINFO

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 446 bytes --]

Alistair Francis <alistair23@gmail.com> writes:

> I have started on the effort, but I have not finished yet. Adding
> riscv_cpu_is_32bit() was the first step there and I have some more
> patches locally but I don't have anything working yet.

That's awesome. I think waiting until we see what APIs you're developing
for detecting and operating in 32-bit mode on a 64-bit capable processor
seems like a good idea for now.

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]