Re: [PATCH v5 1/2] semihosting/arm-compat: replace heuristic for softmmu SYS_HEAPINFO

[Peter Maydell]

On Thu, 10 Feb 2022 at 11:30, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> The previous numbers were a guess at best and rather arbitrary without
> taking into account anything that might be loaded. Instead of using
> guesses based on the state of registers implement a new function that:
>
>  a) scans the MemoryRegions for the largest RAM block
>  b) iterates through all "ROM" blobs looking for the biggest gap
>
> The "ROM" blobs include all code loaded via -kernel and the various
> -device loader techniques.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Andrew Strauss <astrauss11@gmail.com>
> Cc: Keith Packard <keithp@keithp.com>
> Message-Id: <20210601090715.22330-1-alex.bennee@linaro.org>
>


> +/*
> + * Sort into address order. We break ties between rom-startpoints
> + * and rom-endpoints in favour of the startpoint, by sorting the 0->1
> + * transition before the 1->0 transition. Either way round would
> + * work, but this way saves a little work later by avoiding
> + * dealing with "gaps" of 0 length.
> + */
> +static gint sort_secs(gconstpointer a, gconstpointer b)
> +{
> +    RomSec *ra = (RomSec *) a;
> +    RomSec *rb = (RomSec *) b;
> +
> +    if (ra->base == rb->base) {
> +        return ra->se - rb->se;
> +    }
> +    return ra->base > rb->base ? 1 : -1;
> +}

This sort comparator still doesn't report the equality
case as actually equal.

>      /*
> -     * Find the chunk of R/W memory containing the address.  This is
> -     * used for the SYS_HEAPINFO semihosting call, which should
> -     * probably be using information from the loaded application.
> +     * If we have found the RAM lets iterate through the ROM blobs to
> +     * workout the best place for the remainder of RAM and split it

"work out"

> +     * equally between stack and heap.
>       */

> @@ -1201,12 +1205,15 @@ target_ulong do_common_semihosting(CPUState *cs)
>              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.  */
> +            /*
> +             * Reporting 0 indicates we couldn't calculate the real
> +             * values which should force most software to fall back to
> +             * using information it has.
> +             */

What is this comment referring to? We aren't obviously
reporting 0 here...

> +            retvals[0] = info.heapbase;  /* Heap Base */
> +            retvals[1] = info.heaplimit; /* Heap Limit */
> +            retvals[2] = info.heaplimit; /* Stack base */
> +            retvals[3] = info.heapbase;  /* Stack limit.  */
>  #endif
>
>              for (i = 0; i < ARRAY_SIZE(retvals); i++) {

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

-- PMM