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

[Philippe Mathieu-Daudé]

Hi Alex,

On 10/2/22 12:30, Alex Bennée 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>

> +static LayoutInfo common_semi_find_bases(CPUState *cs)
>   {
> -    MemoryRegion *subregion;
> +    FlatView *fv;
> +    LayoutInfo info = { 0, 0, 0, 0 };
> +
> +    RCU_READ_LOCK_GUARD();
> +
> +    fv = address_space_to_flatview(cs->as);

Why are we using the CPU view and not address_space_memory?

Does this function really need a CPUState argument?

Trying to find a counter example, if used on the ZynqMP, could a
A-profile core would report one heap layout, and a R-profile core
another layout?

Now if we want the per-CPU AS, shouldn't we pass the CPU AS ID and
call cpu_get_address_space() instead of cs->as?

> +    flatview_for_each_range(fv, find_ram_cb, &info);
>   
>       /*
> -     * 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
> +     * equally between stack and heap.
>        */
> -    QTAILQ_FOREACH(subregion, &get_system_memory()->subregions,
> -                   subregions_link) {
> -        if (subregion->ram && !subregion->readonly) {
> -            Int128 top128 = int128_add(int128_make64(subregion->addr),
> -                                       subregion->size);
> -            Int128 addr128 = int128_make64(addr);
> -            if (subregion->addr <= addr && int128_lt(addr128, top128)) {
> -                return subregion->addr;
> -            }
> -        }
> +    if (info.rambase || info.ramsize > 0) {
> +        RomGap gap = rom_find_largest_gap_between(info.rambase, info.ramsize);
> +        info.heapbase = gap.base;
> +        info.heaplimit = gap.base + gap.size;
>       }
> -    return 0;
> +
> +    return info;
>   }