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

[Peter Maydell <peter.maydell@linaro.org>]

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