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

[Alex Bennée]

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

> 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.

When ra->se and rb->se are the same it returns 0. Is that not what you want?

>
>>      /*
>> -     * 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...

Stale comment, deleted.

>
>> +            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


-- 
Alex Bennée