[PATCH v2 02/95] semihosting: Initialize heap once per process

Richard Henderson posted 95 patches 3 months, 2 weeks ago
Maintainers: Riku Voipio <riku.voipio@iki.fi>, Laurent Vivier <laurent@vivier.eu>, Brian Cain <brian.cain@oss.qualcomm.com>, Paolo Bonzini <pbonzini@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, "Alex Bennée" <alex.bennee@linaro.org>
There is a newer version of this series
[PATCH v2 02/95] semihosting: Initialize heap once per process
Posted by Richard Henderson 3 months, 2 weeks ago
While semihosting isn't really thread aware, the current
implementation allocates space for the heap per-thread.

Remove the heap_base and heap_limit fields from TaskState.
Replace with static variables within do_common_semihosting.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/qemu.h             |  5 -----
 linux-user/aarch64/cpu_loop.c |  7 -------
 linux-user/arm/cpu_loop.c     | 32 +++++++++++++++-----------------
 linux-user/m68k/cpu_loop.c    |  8 --------
 linux-user/riscv/cpu_loop.c   |  4 ----
 semihosting/arm-compat-semi.c | 22 +++++++++-------------
 6 files changed, 24 insertions(+), 54 deletions(-)

diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index b6621536b3..4d6fad28c6 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -121,11 +121,6 @@ struct TaskState {
     abi_ulong child_tidptr;
 #ifdef TARGET_M68K
     abi_ulong tp_value;
-#endif
-#if defined(TARGET_ARM) || defined(TARGET_M68K) || defined(TARGET_RISCV)
-    /* Extra fields for semihosted binaries.  */
-    abi_ulong heap_base;
-    abi_ulong heap_limit;
 #endif
     int used; /* non zero if used */
     struct image_info *info;
diff --git a/linux-user/aarch64/cpu_loop.c b/linux-user/aarch64/cpu_loop.c
index b65999a75b..030a630c93 100644
--- a/linux-user/aarch64/cpu_loop.c
+++ b/linux-user/aarch64/cpu_loop.c
@@ -140,9 +140,6 @@ void cpu_loop(CPUARMState *env)
 void target_cpu_copy_regs(CPUArchState *env, target_pt_regs *regs)
 {
     ARMCPU *cpu = env_archcpu(env);
-    CPUState *cs = env_cpu(env);
-    TaskState *ts = get_task_state(cs);
-    struct image_info *info = ts->info;
     int i;
 
     if (!(arm_feature(env, ARM_FEATURE_AARCH64))) {
@@ -167,8 +164,4 @@ void target_cpu_copy_regs(CPUArchState *env, target_pt_regs *regs)
     if (cpu_isar_feature(aa64_pauth, cpu)) {
         qemu_guest_getrandom_nofail(&env->keys, sizeof(env->keys));
     }
-
-    ts->heap_base = info->brk;
-    /* This will be filled in on the first SYS_HEAPINFO call.  */
-    ts->heap_limit = 0;
 }
diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
index e40d6beafa..e2b4099aa4 100644
--- a/linux-user/arm/cpu_loop.c
+++ b/linux-user/arm/cpu_loop.c
@@ -482,9 +482,6 @@ void cpu_loop(CPUARMState *env)
 
 void target_cpu_copy_regs(CPUArchState *env, target_pt_regs *regs)
 {
-    CPUState *cpu = env_cpu(env);
-    TaskState *ts = get_task_state(cpu);
-    struct image_info *info = ts->info;
     int i;
 
     cpsr_write(env, regs->uregs[16], CPSR_USER | CPSR_EXEC,
@@ -492,19 +489,20 @@ void target_cpu_copy_regs(CPUArchState *env, target_pt_regs *regs)
     for(i = 0; i < 16; i++) {
         env->regs[i] = regs->uregs[i];
     }
-#if TARGET_BIG_ENDIAN
-    /* Enable BE8.  */
-    if (EF_ARM_EABI_VERSION(info->elf_flags) >= EF_ARM_EABI_VER4
-        && (info->elf_flags & EF_ARM_BE8)) {
-        env->uncached_cpsr |= CPSR_E;
-        env->cp15.sctlr_el[1] |= SCTLR_E0E;
-    } else {
-        env->cp15.sctlr_el[1] |= SCTLR_B;
-    }
-    arm_rebuild_hflags(env);
-#endif
 
-    ts->heap_base = info->brk;
-    /* This will be filled in on the first SYS_HEAPINFO call.  */
-    ts->heap_limit = 0;
+    if (TARGET_BIG_ENDIAN) {
+        CPUState *cpu = env_cpu(env);
+        TaskState *ts = get_task_state(cpu);
+        struct image_info *info = ts->info;
+
+        /* Enable BE8.  */
+        if (EF_ARM_EABI_VERSION(info->elf_flags) >= EF_ARM_EABI_VER4
+            && (info->elf_flags & EF_ARM_BE8)) {
+            env->uncached_cpsr |= CPSR_E;
+            env->cp15.sctlr_el[1] |= SCTLR_E0E;
+        } else {
+            env->cp15.sctlr_el[1] |= SCTLR_B;
+        }
+        arm_rebuild_hflags(env);
+    }
 }
diff --git a/linux-user/m68k/cpu_loop.c b/linux-user/m68k/cpu_loop.c
index 3aaaf02ca4..23693f3358 100644
--- a/linux-user/m68k/cpu_loop.c
+++ b/linux-user/m68k/cpu_loop.c
@@ -94,10 +94,6 @@ void cpu_loop(CPUM68KState *env)
 
 void target_cpu_copy_regs(CPUArchState *env, target_pt_regs *regs)
 {
-    CPUState *cpu = env_cpu(env);
-    TaskState *ts = get_task_state(cpu);
-    struct image_info *info = ts->info;
-
     env->pc = regs->pc;
     env->dregs[0] = regs->d0;
     env->dregs[1] = regs->d1;
@@ -116,8 +112,4 @@ void target_cpu_copy_regs(CPUArchState *env, target_pt_regs *regs)
     env->aregs[6] = regs->a6;
     env->aregs[7] = regs->usp;
     env->sr = regs->sr;
-
-    ts->heap_base = info->brk;
-    /* This will be filled in on the first SYS_HEAPINFO call.  */
-    ts->heap_limit = 0;
 }
diff --git a/linux-user/riscv/cpu_loop.c b/linux-user/riscv/cpu_loop.c
index 541de765ff..2dd30c7b28 100644
--- a/linux-user/riscv/cpu_loop.c
+++ b/linux-user/riscv/cpu_loop.c
@@ -108,8 +108,4 @@ void target_cpu_copy_regs(CPUArchState *env, target_pt_regs *regs)
         error_report("Incompatible ELF: RVE cpu requires RVE ABI binary");
         exit(EXIT_FAILURE);
     }
-
-    ts->heap_base = info->brk;
-    /* This will be filled in on the first SYS_HEAPINFO call.  */
-    ts->heap_limit = 0;
 }
diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
index bc04b02eba..bcd13cd6df 100644
--- a/semihosting/arm-compat-semi.c
+++ b/semihosting/arm-compat-semi.c
@@ -666,7 +666,7 @@ void do_common_semihosting(CPUState *cs)
             int i;
 #ifdef CONFIG_USER_ONLY
             TaskState *ts = get_task_state(cs);
-            target_ulong limit;
+            static abi_ulong heapbase, heaplimit;
 #else
             LayoutInfo info = common_semi_find_bases(cs);
 #endif
@@ -678,24 +678,20 @@ void do_common_semihosting(CPUState *cs)
              * 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;
+            if (!heaplimit) {
+                heapbase = do_brk(0);
                 /* Try a big heap, and reduce the size if that fails.  */
-                for (;;) {
-                    ret = do_brk(limit);
+                for (abi_ulong size = COMMON_SEMI_HEAP_SIZE; ; size >>= 1) {
+                    abi_ulong limit = heapbase + size;
+                    abi_ulong ret = do_brk(limit);
                     if (ret >= limit) {
+                        heaplimit = limit;
                         break;
                     }
-                    limit = (ts->heap_base >> 1) + (limit >> 1);
                 }
-                ts->heap_limit = limit;
             }
-
-            retvals[0] = ts->heap_base;
-            retvals[1] = ts->heap_limit;
+            retvals[0] = heapbase;
+            retvals[1] = heaplimit;
             /*
              * Note that semihosting is *not* thread aware.
              * Always return the stack base of the main thread.
-- 
2.43.0
Re: [PATCH v2 02/95] semihosting: Initialize heap once per process
Posted by Peter Maydell 3 months, 1 week ago
On Sun, 3 Aug 2025 at 00:06, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> While semihosting isn't really thread aware, the current
> implementation allocates space for the heap per-thread.
>
> Remove the heap_base and heap_limit fields from TaskState.
> Replace with static variables within do_common_semihosting.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> @@ -492,19 +489,20 @@ void target_cpu_copy_regs(CPUArchState *env, target_pt_regs *regs)
>      for(i = 0; i < 16; i++) {
>          env->regs[i] = regs->uregs[i];
>      }
> -#if TARGET_BIG_ENDIAN
> -    /* Enable BE8.  */
> -    if (EF_ARM_EABI_VERSION(info->elf_flags) >= EF_ARM_EABI_VER4
> -        && (info->elf_flags & EF_ARM_BE8)) {
> -        env->uncached_cpsr |= CPSR_E;
> -        env->cp15.sctlr_el[1] |= SCTLR_E0E;
> -    } else {
> -        env->cp15.sctlr_el[1] |= SCTLR_B;
> -    }
> -    arm_rebuild_hflags(env);
> -#endif
>
> -    ts->heap_base = info->brk;
> -    /* This will be filled in on the first SYS_HEAPINFO call.  */
> -    ts->heap_limit = 0;
> +    if (TARGET_BIG_ENDIAN) {
> +        CPUState *cpu = env_cpu(env);
> +        TaskState *ts = get_task_state(cpu);
> +        struct image_info *info = ts->info;
> +
> +        /* Enable BE8.  */
> +        if (EF_ARM_EABI_VERSION(info->elf_flags) >= EF_ARM_EABI_VER4
> +            && (info->elf_flags & EF_ARM_BE8)) {
> +            env->uncached_cpsr |= CPSR_E;
> +            env->cp15.sctlr_el[1] |= SCTLR_E0E;
> +        } else {
> +            env->cp15.sctlr_el[1] |= SCTLR_B;
> +        }
> +        arm_rebuild_hflags(env);
> +    }

This change to the big-endian handling code seems unrelated.

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

> @@ -678,24 +678,20 @@ void do_common_semihosting(CPUState *cs)
>               * 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;
> +            if (!heaplimit) {
> +                heapbase = do_brk(0);
>                  /* Try a big heap, and reduce the size if that fails.  */
> -                for (;;) {
> -                    ret = do_brk(limit);
> +                for (abi_ulong size = COMMON_SEMI_HEAP_SIZE; ; size >>= 1) {
> +                    abi_ulong limit = heapbase + size;
> +                    abi_ulong ret = do_brk(limit);
>                      if (ret >= limit) {
> +                        heaplimit = limit;
>                          break;
>                      }
> -                    limit = (ts->heap_base >> 1) + (limit >> 1);

The old code was doing this in a very weird way; what you
have here is much clearer...

>                  }
> -                ts->heap_limit = limit;
>              }

-- PMM
Re: [PATCH v2 02/95] semihosting: Initialize heap once per process
Posted by Richard Henderson 3 months, 1 week ago
On 8/3/25 20:41, Peter Maydell wrote:
> On Sun, 3 Aug 2025 at 00:06, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> While semihosting isn't really thread aware, the current
>> implementation allocates space for the heap per-thread.
>>
>> Remove the heap_base and heap_limit fields from TaskState.
>> Replace with static variables within do_common_semihosting.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> 
>> @@ -492,19 +489,20 @@ void target_cpu_copy_regs(CPUArchState *env, target_pt_regs *regs)
>>       for(i = 0; i < 16; i++) {
>>           env->regs[i] = regs->uregs[i];
>>       }
>> -#if TARGET_BIG_ENDIAN
>> -    /* Enable BE8.  */
>> -    if (EF_ARM_EABI_VERSION(info->elf_flags) >= EF_ARM_EABI_VER4
>> -        && (info->elf_flags & EF_ARM_BE8)) {
>> -        env->uncached_cpsr |= CPSR_E;
>> -        env->cp15.sctlr_el[1] |= SCTLR_E0E;
>> -    } else {
>> -        env->cp15.sctlr_el[1] |= SCTLR_B;
>> -    }
>> -    arm_rebuild_hflags(env);
>> -#endif
>>
>> -    ts->heap_base = info->brk;
>> -    /* This will be filled in on the first SYS_HEAPINFO call.  */
>> -    ts->heap_limit = 0;
>> +    if (TARGET_BIG_ENDIAN) {
>> +        CPUState *cpu = env_cpu(env);
>> +        TaskState *ts = get_task_state(cpu);
>> +        struct image_info *info = ts->info;
>> +
>> +        /* Enable BE8.  */
>> +        if (EF_ARM_EABI_VERSION(info->elf_flags) >= EF_ARM_EABI_VER4
>> +            && (info->elf_flags & EF_ARM_BE8)) {
>> +            env->uncached_cpsr |= CPSR_E;
>> +            env->cp15.sctlr_el[1] |= SCTLR_E0E;
>> +        } else {
>> +            env->cp15.sctlr_el[1] |= SCTLR_B;
>> +        }
>> +        arm_rebuild_hflags(env);
>> +    }
> 
> This change to the big-endian handling code seems unrelated.

The ifdef caused unused variable Werrors for little-endian.
The best solution seemed to be to convert to a C if.


r~