Re: [PATCH v1 3/3] semihosting/arg-compat: fix up handling of SYS_HEAPINFO

[Peter Maydell]

On Fri, 5 Mar 2021 at 13:54, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> I'm not sure this every worked properly and it's certainly not
> exercised by check-tcg or Peter's semihosting tests. Hoist it into
> it's own helper function and attempt to validate the results in the
> linux-user semihosting test at the least.
>
> Bug: https://bugs.launchpad.net/bugs/1915925
> Cc: Bug 1915925 <1915925@bugs.launchpad.net>
> Cc: Keith Packard <keithp@keithp.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  tests/tcg/arm/semicall.h      |   1 +
>  semihosting/arm-compat-semi.c | 129 +++++++++++++++++++---------------
>  tests/tcg/arm/semihosting.c   |  34 ++++++++-
>  3 files changed, 107 insertions(+), 57 deletions(-)
> +#else
> +    limit = current_machine->ram_size;
> +    /* TODO: Make this use the limit of the loaded application.  */
> +    info.heap_base = rambase + limit / 2;
> +    info.heap_limit = rambase + limit;
> +    info.stack_base = rambase + limit; /* Stack base */
> +    info.stack_limit = rambase; /* Stack limit.  */
> +
> +    if (cpu_memory_rw_debug(cs, arg0, &info, sizeof(info), true)) {

Blatting a C struct into guest memory has endianness and padding
problems. Why not just do things the way the old Arm code did it ?

Also, you don't seem to have the correct "is the CPU in
32-bit or 64-bit mode" test here: you cannot rely on target_ulong
being the right size, you must make a runtime check.

I suggested in the other email the way I think we should fix this.

thanks
-- PMM