[PATCH for-5.2] target/arm: Make SYS_HEAPINFO work with RAM that doesn't start at 0

[PATCH for-5.2] target/arm: Make SYS_HEAPINFO work with RAM that doesn't start at 0

[Peter Maydell]

The semihosting SYS_HEAPINFO call is supposed to return an array
of four guest addresses:
 * base of heap memory
 * limit of heap memory
 * base of stack memory
 * limit of stack memory

Some semihosting programs (including those compiled to use the
'newlib' embedded C library) use this call to work out where they
should initialize themselves to.

QEMU's implementation when in system emulation mode is very
simplistic: we say that the heap starts halfway into RAM and
continues to the end of RAM, and the stack starts at the top of RAM
and works down to the bottom.  Unfortunately the code assumes that
the base address of RAM is at address 0, so on boards like 'virt'
where this is not true the addresses returned will all be wrong and
the guest application will usually crash.

Conveniently since all Arm boards call arm_load_kernel() we have the
base address of the main RAM block in the arm_boot_info struct which
is accessible via the CPU object.  Use this to return sensible values
from SYS_HEAPINFO.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Marked for-5.2 as it's a relatively simple bug fix, though on the
other hand the bug has been present since forever, so if it
doesn't make it in it's not a huge deal.

 target/arm/arm-semi.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index c1df664f7e5..c892e0e674e 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -36,6 +36,7 @@
 #else
 #include "exec/gdbstub.h"
 #include "qemu/cutils.h"
+#include "hw/arm/boot.h"
 #endif
 
 #define TARGET_SYS_OPEN        0x01
@@ -1014,6 +1015,9 @@ target_ulong do_arm_semihosting(CPUARMState *env)
             int i;
 #ifdef CONFIG_USER_ONLY
             TaskState *ts = cs->opaque;
+#else
+            const struct arm_boot_info *info = env->boot_info;
+            target_ulong rambase = info->loader_start;
 #endif
 
             GET_ARG(0);
@@ -1046,10 +1050,10 @@ target_ulong do_arm_semihosting(CPUARMState *env)
 #else
             limit = ram_size;
             /* TODO: Make this use the limit of the loaded application.  */
-            retvals[0] = limit / 2;
-            retvals[1] = limit;
-            retvals[2] = limit; /* Stack base */
-            retvals[3] = 0; /* Stack limit.  */
+            retvals[0] = rambase + limit / 2;
+            retvals[1] = rambase + limit;
+            retvals[2] = rambase + limit; /* Stack base */
+            retvals[3] = rambase; /* Stack limit.  */
 #endif
 
             for (i = 0; i < ARRAY_SIZE(retvals); i++) {
-- 
2.20.1

Re: [PATCH for-5.2] target/arm: Make SYS_HEAPINFO work with RAM that doesn't start at 0

[Alex Bennée]

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

> The semihosting SYS_HEAPINFO call is supposed to return an array
> of four guest addresses:
>  * base of heap memory
>  * limit of heap memory
>  * base of stack memory
>  * limit of stack memory
>
> Some semihosting programs (including those compiled to use the
> 'newlib' embedded C library) use this call to work out where they
> should initialize themselves to.
>
> QEMU's implementation when in system emulation mode is very
> simplistic: we say that the heap starts halfway into RAM and
> continues to the end of RAM, and the stack starts at the top of RAM
> and works down to the bottom.

So there is nothing to stop this value being in the middle of say a
loaded kernel or something that happens to overrun into the second half
of memory? AFAICT if an initrd is ever used it will likely smash that.

> Unfortunately the code assumes that
> the base address of RAM is at address 0, so on boards like 'virt'
> where this is not true the addresses returned will all be wrong and
> the guest application will usually crash.
>
> Conveniently since all Arm boards call arm_load_kernel() we have the
> base address of the main RAM block in the arm_boot_info struct which
> is accessible via the CPU object.  Use this to return sensible values
> from SYS_HEAPINFO.

It's certainly an improvement but it feels like it could be a bit
smarter. Maybe it's not an issue for sort of things that use
semihosting?

Anyway:

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

-- 
Alex Bennée

Re: [PATCH for-5.2] target/arm: Make SYS_HEAPINFO work with RAM that doesn't start at 0

[Peter Maydell]

On Thu, 19 Nov 2020 at 11:26, Alex Bennée <alex.bennee@linaro.org> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> > QEMU's implementation when in system emulation mode is very
> > simplistic: we say that the heap starts halfway into RAM and
> > continues to the end of RAM, and the stack starts at the top of RAM
> > and works down to the bottom.
>
> So there is nothing to stop this value being in the middle of say a
> loaded kernel or something that happens to overrun into the second half
> of memory? AFAICT if an initrd is ever used it will likely smash that.

Correct, but the only code that uses SYS_HEAPINFO will be
the libc runtime startup code in simple 'bare metal' test
applications, which are likely to be pretty small.

> > Unfortunately the code assumes that
> > the base address of RAM is at address 0, so on boards like 'virt'
> > where this is not true the addresses returned will all be wrong and
> > the guest application will usually crash.
> >
> > Conveniently since all Arm boards call arm_load_kernel() we have the
> > base address of the main RAM block in the arm_boot_info struct which
> > is accessible via the CPU object.  Use this to return sensible values
> > from SYS_HEAPINFO.
>
> It's certainly an improvement but it feels like it could be a bit
> smarter. Maybe it's not an issue for sort of things that use
> semihosting?

This is what the comment about "TODO: Make this use the limit
of the loaded application" is about -- in theory if we know
what the ELF file we've just loaded is we could put the
heap start there rather than just assuming "the loaded
application is probably not using half of RAM for its image".
But the benefit in trying to do that is not great and the
complexity is quite large, given how many ways we have of
loading guest code. (I guess the theoretical ideal would
involve asking the rom-blob code for the largest available
empty space?)

thanks
-- PMM