[PATCH v5 0/2] semihosting/next (SYS_HEAPINFO)

[PATCH v5 0/2] semihosting/next (SYS_HEAPINFO)

[Alex Bennée]

Hi,

I'm working through the backlog of stalled patches in my queue so here
is the latest version of the semihosting info fixes with Peter's
comments addressed. Both patches are still missing their elusive r-b
tags ;-)

Alex Bennée (2):
  semihosting/arm-compat: replace heuristic for softmmu SYS_HEAPINFO
  tests/tcg: port SYS_HEAPINFO to a system test

 include/hw/loader.h                 |  14 +++
 hw/core/loader.c                    |  86 +++++++++++++++++++
 semihosting/arm-compat-semi.c       | 129 +++++++++++++++-------------
 tests/tcg/aarch64/system/semiheap.c |  93 ++++++++++++++++++++
 MAINTAINERS                         |   1 +
 5 files changed, 262 insertions(+), 61 deletions(-)
 create mode 100644 tests/tcg/aarch64/system/semiheap.c

-- 
2.30.2

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

[Alex Bennée]

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>

---
v2
  - report some known information (limits)
  - reword the commit message
v3
  - rework to use the ROM blob scanning suggested by Peter
  - drop arch specific wrappers
  - dropped rb/tb tags as it's a rework
v4
  - search for the largest RAM which should be the main RAM
  - implement the biggest gap algorithm
  - make stackbase the inverse of heap info
v5
  - move rom_find_largest_gap description to above fn and reword
  - add documentation of sort behaviour
  - handle matching se flags (- -1 -1) and (- 1 1) == 0
  - add helper function and sentinal
  - fix off-by-ones in comparisons
  - allow a rambase at 0

squash! semihosting/arm-compat: replace heuristic for softmmu SYS_HEAPINFO
---
 include/hw/loader.h           |  14 ++++
 hw/core/loader.c              |  86 +++++++++++++++++++++++
 semihosting/arm-compat-semi.c | 129 ++++++++++++++++++----------------
 3 files changed, 168 insertions(+), 61 deletions(-)

diff --git a/include/hw/loader.h b/include/hw/loader.h
index 4fa485bd61..5572108ba5 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -343,4 +343,18 @@ int rom_add_option(const char *file, int32_t bootindex);
  * overflow on real hardware too. */
 #define UBOOT_MAX_GUNZIP_BYTES (64 << 20)
 
+typedef struct RomGap {
+    hwaddr base;
+    size_t size;
+} RomGap;
+
+/**
+ * rom_find_largest_gap_between: return largest gap between ROMs in given range
+ *
+ * Given a range of addresses, this function finds the largest
+ * contiguous subrange which has no ROMs loaded to it. That is,
+ * it finds the biggest gap which is free for use for other things.
+ */
+RomGap rom_find_largest_gap_between(hwaddr base, size_t size);
+
 #endif
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 19edb928e9..ca2f2431fb 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1333,6 +1333,92 @@ static Rom *find_rom(hwaddr addr, size_t size)
     return NULL;
 }
 
+typedef struct RomSec {
+    hwaddr base;
+    int se; /* start/end flag */
+} RomSec;
+
+
+/*
+ * 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;
+}
+
+static GList *add_romsec_to_list(GList *secs, hwaddr base, int se)
+{
+   RomSec *cand = g_new(RomSec, 1);
+   cand->base = base;
+   cand->se = se;
+   return g_list_prepend(secs, cand);
+}
+
+RomGap rom_find_largest_gap_between(hwaddr base, size_t size)
+{
+    Rom *rom;
+    RomSec *cand;
+    RomGap res = {0, 0};
+    hwaddr gapstart = base;
+    GList *it, *secs = NULL;
+    int count = 0;
+
+    QTAILQ_FOREACH(rom, &roms, next) {
+        /* Ignore blobs being loaded to special places */
+        if (rom->mr || rom->fw_file) {
+            continue;
+        }
+        /* ignore anything finishing bellow base */
+        if (rom->addr + rom->romsize <= base) {
+            continue;
+        }
+        /* ignore anything starting above the region */
+        if (rom->addr >= base + size) {
+            continue;
+        }
+
+        /* Save the start and end of each relevant ROM */
+        secs = add_romsec_to_list(secs, rom->addr, 1);
+
+        if (rom->addr + rom->romsize < base + size) {
+            secs = add_romsec_to_list(secs, rom->addr + rom->romsize, -1);
+        }
+    }
+
+    /* sentinel */
+    secs = add_romsec_to_list(secs, base + size, 1);
+
+    secs = g_list_sort(secs, sort_secs);
+
+    for (it = g_list_first(secs); it; it = g_list_next(it)) {
+        cand = (RomSec *) it->data;
+        if (count == 0 && count + cand->se == 1) {
+            size_t gap = cand->base - gapstart;
+            if (gap > res.size) {
+                res.base = gapstart;
+                res.size = gap;
+            }
+        } else if (count == 1 && count + cand->se == 0) {
+            gapstart = cand->base;
+        }
+        count += cand->se;
+    }
+
+    g_list_free_full(secs, g_free);
+    return res;
+}
+
 /*
  * Copies memory from registered ROMs to dest. Any memory that is contained in
  * a ROM between addr and addr + size is copied. Note that this can involve
diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
index 37963becae..3704b250b2 100644
--- a/semihosting/arm-compat-semi.c
+++ b/semihosting/arm-compat-semi.c
@@ -44,6 +44,7 @@
 #define COMMON_SEMI_HEAP_SIZE (128 * 1024 * 1024)
 #else
 #include "qemu/cutils.h"
+#include "hw/loader.h"
 #ifdef TARGET_ARM
 #include "hw/arm/boot.h"
 #endif
@@ -144,33 +145,69 @@ typedef struct GuestFD {
 static GArray *guestfd_array;
 
 #ifndef CONFIG_USER_ONLY
-#include "exec/address-spaces.h"
-/*
- * Find the base of a RAM region containing the specified address
+
+/**
+ * common_semi_find_bases: find information about ram and heap base
+ *
+ * This function attempts to provide meaningful numbers for RAM and
+ * HEAP base addresses. The rambase is simply the lowest addressable
+ * RAM position. For the heapbase we ask the loader to scan the
+ * address space and the largest available gap by querying the "ROM"
+ * regions.
+ *
+ * Returns: a structure with the numbers we need.
  */
-static inline hwaddr
-common_semi_find_region_base(hwaddr addr)
+
+typedef struct LayoutInfo {
+    target_ulong rambase;
+    size_t ramsize;
+    hwaddr heapbase;
+    hwaddr heaplimit;
+} LayoutInfo;
+
+static bool find_ram_cb(Int128 start, Int128 len, const MemoryRegion *mr,
+                        hwaddr offset_in_region, void *opaque)
+{
+    LayoutInfo *info = (LayoutInfo *) opaque;
+    uint64_t size = int128_get64(len);
+
+    if (!mr->ram || mr->readonly) {
+        return false;
+    }
+
+    if (size > info->ramsize) {
+        info->rambase = int128_get64(start);
+        info->ramsize = size;
+    }
+
+    /* search exhaustively for largest RAM */
+    return false;
+}
+
+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);
+    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;
 }
+
 #endif
 
 #ifdef TARGET_ARM
@@ -204,28 +241,6 @@ common_semi_sys_exit_extended(CPUState *cs, int nr)
     return (nr == TARGET_SYS_EXIT_EXTENDED || is_a64(cs->env_ptr));
 }
 
-#ifndef CONFIG_USER_ONLY
-#include "hw/arm/boot.h"
-static inline target_ulong
-common_semi_rambase(CPUState *cs)
-{
-    CPUArchState *env = cs->env_ptr;
-    const struct arm_boot_info *info = env->boot_info;
-    target_ulong sp;
-
-    if (info) {
-        return info->loader_start;
-    }
-
-    if (is_a64(env)) {
-        sp = env->xregs[31];
-    } else {
-        sp = env->regs[13];
-    }
-    return common_semi_find_region_base(sp);
-}
-#endif
-
 #endif /* TARGET_ARM */
 
 #ifdef TARGET_RISCV
@@ -251,17 +266,6 @@ common_semi_sys_exit_extended(CPUState *cs, int nr)
     return (nr == TARGET_SYS_EXIT_EXTENDED || sizeof(target_ulong) == 8);
 }
 
-#ifndef CONFIG_USER_ONLY
-
-static inline target_ulong
-common_semi_rambase(CPUState *cs)
-{
-    RISCVCPU *cpu = RISCV_CPU(cs);
-    CPURISCVState *env = &cpu->env;
-    return common_semi_find_region_base(env->gpr[xSP]);
-}
-#endif
-
 #endif
 
 /*
@@ -1165,12 +1169,12 @@ target_ulong do_common_semihosting(CPUState *cs)
     case TARGET_SYS_HEAPINFO:
         {
             target_ulong retvals[4];
-            target_ulong limit;
             int i;
 #ifdef CONFIG_USER_ONLY
             TaskState *ts = cs->opaque;
+            target_ulong limit;
 #else
-            target_ulong rambase = common_semi_rambase(cs);
+            LayoutInfo info = common_semi_find_bases(cs);
 #endif
 
             GET_ARG(0);
@@ -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.
+             */
+            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++) {
-- 
2.30.2

[PATCH v5 2/2] tests/tcg: port SYS_HEAPINFO to a system test

[Alex Bennée]

This allows us to check our new SYS_HEAPINFO implementation generates
sane values.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v5
  - static init of heapinfo structure
  - clean-up comment on why we can test stack position
  - add memory clobber for semicall
  - test we can read/write to a portion of the heap
  - fix MAINTAINERS
---
 tests/tcg/aarch64/system/semiheap.c | 93 +++++++++++++++++++++++++++++
 MAINTAINERS                         |  1 +
 2 files changed, 94 insertions(+)
 create mode 100644 tests/tcg/aarch64/system/semiheap.c

diff --git a/tests/tcg/aarch64/system/semiheap.c b/tests/tcg/aarch64/system/semiheap.c
new file mode 100644
index 0000000000..4ed258476d
--- /dev/null
+++ b/tests/tcg/aarch64/system/semiheap.c
@@ -0,0 +1,93 @@
+/*
+ * Semihosting System HEAPINFO Test
+ *
+ * Copyright (c) 2021 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include <inttypes.h>
+#include <stddef.h>
+#include <minilib.h>
+
+#define SYS_HEAPINFO    0x16
+
+uintptr_t __semi_call(uintptr_t type, uintptr_t arg0)
+{
+    register uintptr_t t asm("x0") = type;
+    register uintptr_t a0 asm("x1") = arg0;
+    asm("hlt 0xf000"
+        : "=r" (t)
+        : "r" (t), "r" (a0)
+        : "memory" );
+
+    return t;
+}
+
+int main(int argc, char *argv[argc])
+{
+    struct {
+        void *heap_base;
+        void *heap_limit;
+        void *stack_base;
+        void *stack_limit;
+    } info = { };
+    void *ptr_to_info = (void *) &info;
+    uint32_t *ptr_to_heap;
+    int i;
+
+    ml_printf("Semihosting Heap Info Test\n");
+
+    __semi_call(SYS_HEAPINFO, (uintptr_t) &ptr_to_info);
+
+    if (info.heap_base == NULL || info.heap_limit == NULL) {
+        ml_printf("null heap: %p -> %p\n", info.heap_base, info.heap_limit);
+        return -1;
+    }
+
+    /* Error if heap base is above limit */
+    if ((uintptr_t) info.heap_base >= (uintptr_t) info.heap_limit) {
+        ml_printf("heap base %p >= heap_limit %p\n",
+               info.heap_base, info.heap_limit);
+        return -2;
+    }
+
+    if (info.stack_base == NULL) {
+        ml_printf("null stack: %p -> %p\n", info.stack_base, info.stack_limit);
+        return -3;
+    }
+
+    /*
+     * boot.S put our stack somewhere inside the data segment of the
+     * ELF file, and we know that SYS_HEAPINFO won't pick a range
+     * that overlaps with part of a loaded ELF file. So the info
+     * struct (on the stack) should not be inside the reported heap.
+     */
+    if (ptr_to_info > info.heap_base && ptr_to_info < info.heap_limit) {
+        ml_printf("info appears to be inside the heap: %p in %p:%p\n",
+               ptr_to_info, info.heap_base, info.heap_limit);
+        return -4;
+    }
+
+    ml_printf("heap: %p -> %p\n", info.heap_base, info.heap_limit);
+    ml_printf("stack: %p <- %p\n", info.stack_limit, info.stack_base);
+
+    /* finally can we read/write the heap */
+    ptr_to_heap = (uint32_t *) info.heap_base;
+    for (i = 0; i < 512; i++) {
+        *ptr_to_heap++ = i;
+    }
+    ptr_to_heap = (uint32_t *) info.heap_base;
+    for (i = 0; i < 512; i++) {
+        uint32_t tmp = *ptr_to_heap;
+        if (tmp != i) {
+            ml_printf("unexpected value in heap: %d @ %p", tmp, ptr_to_heap);
+            return -5;
+        }
+        ptr_to_heap++;
+    }
+    ml_printf("r/w to heap upto %p\n", ptr_to_heap);
+
+    ml_printf("Passed HeapInfo checks\n");
+    return 0;
+}
diff --git a/MAINTAINERS b/MAINTAINERS
index b0b845f445..251f96af9e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3536,6 +3536,7 @@ S: Maintained
 F: semihosting/
 F: include/semihosting/
 F: tests/tcg/multiarch/arm-compat-semi/
+F: tests/tcg/aarch64/system/semiheap.c
 
 Multi-process QEMU
 M: Elena Ufimtseva <elena.ufimtseva@oracle.com>
-- 
2.30.2

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

[Philippe Mathieu-Daudé via]

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;
>   }

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

[Peter Maydell]

On Thu, 10 Feb 2022 at 11:48, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> 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?

If you have a choice between "use the right view of an
address space" and "use the global address_space_memory",
it's better to prefer the former, I think. We use the
latter in lots of places, but it's not conceptually the
right way to think about how the memory system works IMHO.

-- PMM

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:48, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> 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?
>
> If you have a choice between "use the right view of an
> address space" and "use the global address_space_memory",
> it's better to prefer the former, I think. We use the
> latter in lots of places, but it's not conceptually the
> right way to think about how the memory system works IMHO.

In this case the addresses have to be as the CPU sees them because it's
between the CPU and the semihosting backend to share data.

-- 
Alex Bennée

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

[Philippe Mathieu-Daudé via]

On 11/2/22 14:22, Alex Bennée wrote:
> 
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
>> On Thu, 10 Feb 2022 at 11:48, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>
>>> 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?
>>
>> If you have a choice between "use the right view of an
>> address space" and "use the global address_space_memory",
>> it's better to prefer the former, I think. We use the
>> latter in lots of places, but it's not conceptually the
>> right way to think about how the memory system works IMHO.

Yes I agree.

For user-mode, this patch makes sense. For system-mode it is
not obvious to make sense of SYS_HEAPINFO (except focusing in
cores targeting embedded systems eventually).

> In this case the addresses have to be as the CPU sees them because it's
> between the CPU and the semihosting backend to share data.

Beside the sysemu curiosity, the patch logic is fine, so:

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Thanks,

Phil.

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

[Peter Maydell]

On Fri, 11 Feb 2022 at 16:18, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> For user-mode, this patch makes sense. For system-mode it is
> not obvious to make sense of SYS_HEAPINFO (except focusing in
> cores targeting embedded systems eventually).

The main user of semihosting in system mode is standalone
binaries compiled with the gnu toolchain using a newlib
that targets semihosting. These generally use SYS_HEAPINFO
on startup to find out where to put the stack etc, rather
than hardwiring it in the linker map. This applies to both
M-profile and A-profile.

-- PMM

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

Re: [PATCH v5 2/2] tests/tcg: port SYS_HEAPINFO to a system test

[Peter Maydell]

On Thu, 10 Feb 2022 at 11:30, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> This allows us to check our new SYS_HEAPINFO implementation generates
> sane values.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>

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

thanks
-- PMM

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

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

[Peter Maydell]

On Mon, 21 Feb 2022 at 17:06, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> 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?

Oops, yes it does. I misread it because I was expecting it to be
structured differently. (AFAIK there is no "standard" way to
structure a comparator-function that works with multiple fields,
so the way you have it is fine.)

-- PMM

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 Mon, 21 Feb 2022 at 17:06, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> 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?
>
> Oops, yes it does. I misread it because I was expecting it to be
> structured differently. (AFAIK there is no "standard" way to
> structure a comparator-function that works with multiple fields,
> so the way you have it is fine.)

The other options were all ugly ;-)

-- 
Alex Bennée